Date:   Mon, 31 Oct 2022 10:00:54 -0700
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
Cc:     Jesper Dangaard Brouer <jbrouer@...hat.com>,
        Martin KaFai Lau <martin.lau@...ux.dev>, brouer@...hat.com,
        ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
        song@...nel.org, yhs@...com, john.fastabend@...il.com,
        kpsingh@...nel.org, haoluo@...gle.com, jolsa@...nel.org,
        Jakub Kicinski <kuba@...nel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        Anatoly Burakov <anatoly.burakov@...el.com>,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        Maryam Tahhan <mtahhan@...hat.com>, xdp-hints@...-project.net,
        netdev@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [RFC bpf-next 5/5] selftests/bpf: Test rx_timestamp metadata in xskxceiver

On Mon, Oct 31, 2022 at 7:22 AM Alexander Lobakin
<alexandr.lobakin@...el.com> wrote:
>
> From: Stanislav Fomichev <sdf@...gle.com>
> Date: Fri, 28 Oct 2022 11:46:14 -0700
>
> > On Fri, Oct 28, 2022 at 3:37 AM Jesper Dangaard Brouer
> > <jbrouer@...hat.com> wrote:
> > >
> > >
> > > On 28/10/2022 08.22, Martin KaFai Lau wrote:
> > > > On 10/27/22 1:00 PM, Stanislav Fomichev wrote:
> > > >> Example on how the metadata is prepared from the BPF context
> > > >> and consumed by AF_XDP:
> > > >>
> > > >> - bpf_xdp_metadata_have_rx_timestamp to test whether it's supported;
> > > >>    if not, I'm assuming verifier will remove this "if (0)" branch
> > > >> - bpf_xdp_metadata_rx_timestamp returns a _copy_ of metadata;
> > > >>    the program has to bpf_xdp_adjust_meta+memcpy it;
> > > >>    maybe returning a pointer is better?
> > > >> - af_xdp consumer grabs it from data-<expected_metadata_offset> and
> > > >>    makes sure timestamp is not empty
> > > >> - when loading the program, we pass BPF_F_XDP_HAS_METADATA+prog_ifindex
> > > >>
> > > >> Cc: Martin KaFai Lau <martin.lau@...ux.dev>
> > > >> Cc: Jakub Kicinski <kuba@...nel.org>
> > > >> Cc: Willem de Bruijn <willemb@...gle.com>
> > > >> Cc: Jesper Dangaard Brouer <brouer@...hat.com>
> > > >> Cc: Anatoly Burakov <anatoly.burakov@...el.com>
> > > >> Cc: Alexander Lobakin <alexandr.lobakin@...el.com>
> > > >> Cc: Magnus Karlsson <magnus.karlsson@...il.com>
> > > >> Cc: Maryam Tahhan <mtahhan@...hat.com>
> > > >> Cc: xdp-hints@...-project.net
> > > >> Cc: netdev@...r.kernel.org
> > > >> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > > >> ---
> > > >>   .../testing/selftests/bpf/progs/xskxceiver.c  | 22 ++++++++++++++++++
> > > >>   tools/testing/selftests/bpf/xskxceiver.c      | 23 ++++++++++++++++++-
> > > >>   2 files changed, 44 insertions(+), 1 deletion(-)
>
> [...]
>
> > > IMHO sizeof() should come from a struct describing data_meta area see:
> > >
> > > https://github.com/xdp-project/bpf-examples/blob/master/AF_XDP-interaction/af_xdp_kern.c#L62
> >
> > I guess I should've used pointers for the return type instead, something like:
> >
> > extern __u64 *bpf_xdp_metadata_rx_timestamp(struct xdp_md *ctx) __ksym;
> >
> > {
> >    ...
> >     __u64 *rx_timestamp = bpf_xdp_metadata_rx_timestamp(ctx);
> >     if (rx_timestamp) {
> >         bpf_xdp_adjust_meta(ctx, -(int)sizeof(*rx_timestamp));
> >         __builtin_memcpy(data_meta, rx_timestamp, sizeof(*rx_timestamp));
> >     }
> > }
> >
> > Does that look better?
>
> I guess it will then be resolved to a direct store, right?
> I mean, to smth like
>
>         if (rx_timestamp)
>                 *(u32 *)data_meta = *rx_timestamp;
>
> , where *rx_timestamp points directly to the Rx descriptor?

Right. I should've used that form from the beginning, that memcpy is
confusing :-(

> >
> > > >> +        if (ret != 0)
> > > >> +            return XDP_DROP;
> > > >> +
> > > >> +        data = (void *)(long)ctx->data;
> > > >> +        data_meta = (void *)(long)ctx->data_meta;
> > > >> +
> > > >> +        if (data_meta + sizeof(__u32) > data)
> > > >> +            return XDP_DROP;
> > > >> +
> > > >> +        rx_timestamp = bpf_xdp_metadata_rx_timestamp(ctx);
> > > >> +        __builtin_memcpy(data_meta, &rx_timestamp, sizeof(__u32));
> > >
> > > So, this approach first stores hints on some other memory location, and
> > > then need to copy over information into data_meta area. That isn't good
> > > from a performance perspective.
> > >
> > > My idea is to store it in the final data_meta destination immediately.
> >
> > This approach doesn't have to store the hints in the other memory
> > location. xdp_buff->priv can point to the real hw descriptor and the
> > kfunc can have a bytecode that extracts the data from the hw
> > descriptors. For this particular RFC, we can think that 'skb' is that
> > hw descriptor for veth driver.
>
> I really do think intermediate stores can be avoided with this
> approach.
> Oh, and BTW, if we plan to use a particular Hint in the BPF program
> only, there's no need to place it in the metadata area at all, is
> there? You only want to get it in your code, so just retrieve it to
> the stack and that's it. data_meta is only for cases when you want
> hints to appear in AF_XDP.

Correct.

> > > Do notice that in my approach, the existing ethtool config setting and
> > > socket options (for timestamps) still apply.  Thus, each individual
> > > hardware hint are already configurable. Thus we already have a config
> > > interface. I do acknowledge, that in-case a feature is disabled it still
> > > takes up space in data_meta areas, but importantly it is NOT stored into
> > > the area (for performance reasons).
> >
> > That should be the case with this rfc as well, isn't it? Worst case
> > scenario, that kfunc bytecode can explicitly check ethtool options and
> > return false if it's disabled?
>
> (to Jesper)
>
> Once again, Ethtool idea doesn't work. Let's say you have roughly
> 50% of frames to be consumed by XDP, other 50% go to skb path and
> the stack. In skb, I want as many HW data as possible: checksums,
> hash and so on. Let's say in XDP prog I want only timestamp. What's
> then? Disable everything but stamp and kill skb path? Enable
> everything and kill XDP path?
> Stanislav's approach allows you to request only fields you need from
> the BPF prog directly, I don't see any reasons for adding one more
> layer "oh no I won't give you checksum because it's disabled via
> Ethtool".
> Maybe I get something wrong, pls explain then :P

Agree, good point.

> > > >> +    }
> > > >
> > > > Thanks for the patches.  I took a quick look at patch 1 and 2 but
> > > > haven't had a chance to look at the implementation details (eg.
> > > > KF_UNROLL...etc), yet.
> > > >
> > >
> > > Yes, thanks for the patches, even-though I don't agree with the
> > > approach, at-least until my concerns/use-case can be resolved.
> > > IMHO the best way to convince people is through code. So, thank you for
> > > the effort.  Hopefully we can use some of these ideas and I can also
> > > change/adjust my XDP-hints ideas to incorporate some of this :-)
> >
> > Thank you for the feedback as well, appreciate it!
> > Definitely, looking forward to a v2 from you with some more clarity on
> > how those btf ids are handled by the bpf/af_xdp side!
> >
> > > > Overall (with the example here) looks promising.  There is a lot of
> > > > flexibility on whether the xdp prog needs any hint at all, which hint it
> > > > needs, and how to store it.
> > > >
> > >
> > > I do see the advantage that XDP prog only populates metadata it needs.
> > > But how can we use/access this in __xdp_build_skb_from_frame() ?
> >
> > I don't think __xdp_build_skb_from_frame is automagically solved by
> > either proposal?
>
> It's solved in my approach[0], so that __xdp_build_skb_from_frame()
> are automatically get skb fields populated with the metadata if
> available. But I always use a fixed generic structure, which can't
> compete with your series in terms of flexibility (but solves stuff
> like inter-vendor redirects and so on).
> So in general I feel like there should be 2 options for metadata for
> users:
>
> 1) I use one particular vendor and I always compile AF_XDP programs
>    from fresh source code. I need to read/write only fields I want
>    to. I'd go with kfunc or kptr here (but I don't think BPF progs
>    should parse descriptor formats on their own, so your unroll NDO
>    approach looks optimal for me for that case);
> 2) I use multiple vendors, pre-compiled AF_XDP programs or just old
>    source code, I use veth and/or cpumap. So it's sorta
>    back-forward-left-right-compatibility path. So here we could just
>    use a fixed structure.

For (2) I really like Toke's suggestion about some extra helper that
prepares the metadata that the kernel path will later on be able to
understand.
The only downside I see is that it has to be called explicitly, but if
we assume that libxdp can also abstract this detail, doesn't sound
like a huge issue to me?

> > For this proposal, there has to be some expected kernel metadata
> > format that bpf programs will prepare and the kernel will understand?
> > Think of it like xdp_hints_common from your proposal; the program will
> > have to put together xdp_hints_skb into xdp metadata with the parts
> > that can be populated into skb by the kernel.
> >
> > For your btf ids proposal, it seems there has to be some extra kernel
> > code to parse all possible driver btf_if formats and copy the
> > metadata?
>
> That's why I define a "generic" struct, so that its consumers
> wouldn't have to if-else through a dozen of possible IDs :P
>
> [...]
>
> Great stuff from my PoV, I'd probably like to have some helpers for
> writing this new NDO, so that small vendors wouldn't be afraid of
> implementing it as Jakub mentioned. But still sorta optimal and
> elegant for me, I'm not sure I want to post a "demo version" of my
> series anymore :D
> I feel like this way + one more "everything-compat-fixed" couple
> would satisfy most of potential users.

Awesome, thanks for the review and the feedback!

Powered by blists - more mailing lists