Message-ID: <CAJ+HfNiMksZg2yyGcPV-njA4NmXmeW_70MDpoPugBtD8pHsYZw@mail.gmail.com>
Date:   Tue, 4 Jun 2019 07:37:08 +0200
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Jonathan Lemon <jlemon@...gsvamp.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Netdev <netdev@...r.kernel.org>,
        Björn Töpel <bjorn.topel@...el.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        bpf <bpf@...r.kernel.org>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Saeed Mahameed <saeedm@...lanox.com>
Subject: Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW}
 to netdev

On Tue, 4 Jun 2019 at 01:11, Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 06/03/2019 10:39 AM, Björn Töpel wrote:
> > On Sat, 1 Jun 2019 at 20:12, Jonathan Lemon <jlemon@...gsvamp.com> wrote:
> >> On 31 May 2019, at 2:42, Björn Töpel wrote:
> >>> From: Björn Töpel <bjorn.topel@...el.com>
> >>>
> >>> 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).
>
> > 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.
>
> > 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.
>
> > 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.


> > ...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?
> >
> >> 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.
> >

Powered by blists - more mailing lists