On Tue, Jun 13, 2023 at 10:18 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote: > > Stanislav Fomichev <sdf@xxxxxxxxxx> 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... Same. Let's keep it on the table, I'll try to explore. I was just thinking that having less abstraction here might be better performance-wise. > >> > --- 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? It's UAPI vs non-UAPI. I'm assuming kfunc makes it non-UAPI and gives us an opportunity to fix things. We can do it via a regular syscall path if there is a consensus. > >> > --- 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? For skb path, I'm assuming we can read sinfo->meta_len; it feels nice to make it work for both cases? We can always export metadata len via some kfunc, sure. > >> > --- 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
