Date: Tue, 13 Jun 2023 19:18:09 +0200
From: Toke Høiland-Jørgensen <toke@...nel.org>
To: Stanislav Fomichev <sdf@...gle.com>
Cc: bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
 andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org, yhs@...com,
 john.fastabend@...il.com, kpsingh@...nel.org, haoluo@...gle.com,
 jolsa@...nel.org, willemb@...gle.com, dsahern@...nel.org,
 magnus.karlsson@...el.com, bjorn@...nel.org, maciej.fijalkowski@...el.com,
 netdev@...r.kernel.org
Subject: Re: [RFC bpf-next 0/7] bpf: netdev TX metadata

Stanislav Fomichev <sdf@...gle.com> writes:

> On 06/12, Toke Høiland-Jørgensen wrote:
>> Some immediate thoughts after glancing through this:
>> 
>> > --- Use cases ---
>> >
>> > The goal of this series is to add two new standard-ish places
>> > in the transmit path:
>> >
>> > 1. Right before the packet is transmitted (with access to TX
>> >    descriptors)
>> > 2. Right after the packet is actually transmitted and we've received the
>> >    completion (again, with access to TX completion descriptors)
>> >
>> > Accessing TX descriptors unlocks the following use-cases:
>> >
>> > - Setting device hints at TX: XDP/AF_XDP might use these new hooks to
>> > use device offloads. The existing case implements TX timestamp.
>> > - Observability: global per-netdev hooks can be used for tracing
>> > the packets and exploring completion descriptors for all sorts of
>> > device errors.
>> >
>> > Accessing TX descriptors also means that the hooks have to be called
>> > from the drivers.
>> >
>> > The hooks are a light-weight alternative to XDP at egress and currently
>> > don't provide any packet modification abilities. However, eventually,
>> > can expose new kfuncs to operate on the packet (or, rather, the actual
>> > descriptors; for performance sake).
>> 
>> dynptr?
>
> Haven't considered, let me explore, but not sure what it buys us
> here?

API consistency, certainly. Possibly also performance, if using the
slice thing that gets you a direct pointer to the pkt data? Not sure
about that, though, haven't done extensive benchmarking of dynptr yet...

>> > --- UAPI ---
>> >
>> > The hooks are implemented in a HID-BPF style. Meaning they don't
>> > expose any UAPI and are implemented as tracing programs that call
>> > a bunch of kfuncs. The attach/detach operation happen via BPF syscall
>> > programs. The series expands device-bound infrastructure to tracing
>> > programs.
>> 
>> Not a fan of the "attach from BPF syscall program" thing. These are part
>> of the XDP data path API, and I think we should expose them as proper
>> bpf_link attachments from userspace with introspection etc. But I guess
>> the bpf_mprog thing will give us that?
>
> bpf_mprog will just make those attach kfuncs return the link fd. The
> syscall program will still stay :-(

Why does the attachment have to be done this way, exactly? Couldn't we
just use the regular bpf_link attachment from userspace? AFAICT it's not
really piggy-backing on the function override thing anyway when the
attachment is per-dev? Or am I misunderstanding how all this works?

>> > --- skb vs xdp ---
>> >
>> > The hooks operate on a new light-weight devtx_frame which contains:
>> > - data
>> > - len
>> > - sinfo
>> >
>> > This should allow us to have a unified (from BPF POW) place at TX
>> > and not be super-taxing (we need to copy 2 pointers + len to the stack
>> > for each invocation).
>> 
>> Not sure what I think about this one. At the very least I think we
>> should expose xdp->data_meta as well. I'm not sure what the use case for
>> accessing skbs is? If that *is* indeed useful, probably there will also
>> end up being a use case for accessing the full skb?
>
> skb_shared_info has meta_len, buf afaik, xdp doesn't use it. Maybe I
> a good opportunity to unify? Or probably won't work because if
> xdf_frame doesn't have frags, it won't have sinfo?

No, it won't. But why do we need this unification between the skb and
xdp paths in the first place? Doesn't the skb path already have support
for these things? Seems like we could just stick to making this xdp-only
and keeping xdp_frame as the ctx argument?

>> > --- Multiprog attachment ---
>> >
>> > Currently, attach/detach don't expose links and don't support multiple
>> > programs. I'm planning to use Daniel's bpf_mprog once it lands.
>> >
>> > --- TODO ---
>> >
>> > Things that I'm planning to do for the non-RFC series:
>> > - have some real device support to verify xdp_hw_metadata works
>> 
>> Would be good to see some performance numbers as well :)
>
> +1 :-)
>
>> > - freplace
>> > - Documentation/networking/xdp-rx-metadata.rst - like documentation
>> >
>> > --- CC ---
>> >
>> > CC'ing people only on the cover letter. Hopefully can find the rest via
>> > lore.
>> 
>> Well, I found it there, even though I was apparently left off the Cc
>> list :(
>> 
>> -Toke
>
> Sure, I'll CC you explicitly next time! But I know you diligently follow bpf
> list, so decided to explicitly cc mostly netdev folks that might miss
> it otherwise.

Haha, fair point! And no big deal, I did obviously see it. I was just
feeling a bit left out, that's all ;)

-Toke

Powered by blists - more mailing lists