On Tue, 4 Jun 2019 at 01:11, Daniel Borkmann [off-list ref] wrote:

On 06/03/2019 10:39 AM, Björn Töpel wrote:

quoted

On Sat, 1 Jun 2019 at 20:12, Jonathan Lemon [off-list ref] wrote:

quoted

On 31 May 2019, at 2:42, Björn Töpel wrote:

quoted

From: Björn Töpel <redacted>

All XDP capable drivers need to implement the XDP_QUERY_PROG{,_HW}
command of ndo_bpf. The query code is fairly generic. This commit
refactors the query code up from the drivers to the netdev level.

The struct net_device has gained two new members: xdp_prog_hw and
xdp_flags. The former is the offloaded XDP program, if any, and the
latter tracks the flags that the supplied when attaching the XDP
program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.

The xdp_prog member, previously only used for SKB_MODE, is shared with
DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
mutually exclusive. To differentiate between the two modes, a new
internal flag is introduced as well.

I'm not entirely clear why this new flag is needed - GENERIC seems to
be an alias for SKB_MODE, so why just use SKB_MODE directly?

If the user does not explicitly specify a type (skb|drv|hw), then the
command should choose the correct type and then behave as if this type
was specified.

Yes, this is kind of hairy.

SKB and DRV are mutually exclusive, but HW is not. IOW, valid options are:
SKB, DRV, HW, SKB+HW DRV+HW.

Correct, HW is a bit special here in that it helps offloading parts of
the DRV XDP program to NIC, but also do RSS steering in BPF etc, hence
this combo is intentionally allowed (see also git log).

quoted

What complicates things further, is that SKB and DRV can be implicitly
(auto/no flags) or explicitly enabled (flags).

Mainly out of historic context: originally the fallback to SKB mode was
implicit if the ndo_bpf was missing. But there are use cases where we
want to fail if the driver does not support native XDP to avoid surprises.

quoted

If a user doesn't pass any flags, the "best supported mode" should be
selected. If this "auto mode" is used, it should be seen as a third
mode. E.g.

ip link set dev eth0 xdp on -- OK
ip link set dev eth0 xdp off -- OK

ip link set dev eth0 xdp on -- OK # generic auto selected
ip link set dev eth0 xdpgeneric off -- NOK, bad flags

This would work if the auto selection would have selected XDP generic.

quoted

ip link set dev eth0 xdp on -- OK # drv auto selected
ip link set dev eth0 xdpdrv off -- NOK, bad flags

This would work if the auto selection chose native XDP previously. Are
you saying it's not the case?

Yes, that is *not* the case for some drivers. With the Intel drivers
we didn't check the flags at all at XDP attachment (check out the
usage of xdp_attachment_flags_ok), but e.g. nfp and netdevsim does.
Grep for 'program loaded with different flags' in the test_offload.py
selftest. I like this approach, and my patch does this flag check in
dev_change_xdp_fd.

Also, what is the use case in mixing these commands? It should be xdp
on+off, xdpdrv on+off, and so on. Are you saying you would prefer a
xdp{,any} off that uninstalls everything? Isn't this mostly a user space
issue to whatever orchestrates XDP?

No, I'm not suggesting a change. There is no use-case mixing them.
What the flags ok checks do is returning an error (like nfp and
netdevsim does) if a user tries to mix, say,  "xdp" and explicit
xdpdrv/xdpgeneric". This patch moves this check to the generic
function dev_change_xdp_fd.

There seems to be a confusion about how this is supposed to be used.
It was for me, e.g. I though using "enable with xdp and disable with
xdpdrv" was OK. This was the reason why I added an error on "disable
with xdpgeneric off, if xdpdrv is active" in my first revision of the
series. I removed this in v2, after Jakub pointed out the
test_offload.py test, which is a great showcase/test of what should be
allowed and what shouldn't in terms of flags.

TL;DR: Let's stick to what test_offload.py asserts, for all XDP.

quoted

...and so on. The idea is that a user should use the same set of flags always.

The internal "GENERIC" flag is only to determine if the xdp_prog
represents a DRV version or SKB version. Maybe it would be clearer
just to add an additional xdp_prog_drv to the net_device, instead?

quoted

The logic in dev_change_xdp_fd() is too complicated.  It disallows
setting (drv|skb), but allows (hw|skb), which I'm not sure is
intentional.

It should be clearer as to which combinations are allowed.

Fair point. I'll try to clean it up further.