On Mon, Oct 31, 2022 at 7:22 AM Alexander Lobakin <alexandr.lobakin@xxxxxxxxx> wrote: > > From: Stanislav Fomichev <sdf@xxxxxxxxxx> > Date: Fri, 28 Oct 2022 11:46:14 -0700 > > > On Fri, Oct 28, 2022 at 3:37 AM Jesper Dangaard Brouer > > <jbrouer@xxxxxxxxxx> 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@xxxxxxxxx> > > > >> Cc: Jakub Kicinski <kuba@xxxxxxxxxx> > > > >> Cc: Willem de Bruijn <willemb@xxxxxxxxxx> > > > >> Cc: Jesper Dangaard Brouer <brouer@xxxxxxxxxx> > > > >> Cc: Anatoly Burakov <anatoly.burakov@xxxxxxxxx> > > > >> Cc: Alexander Lobakin <alexandr.lobakin@xxxxxxxxx> > > > >> Cc: Magnus Karlsson <magnus.karlsson@xxxxxxxxx> > > > >> Cc: Maryam Tahhan <mtahhan@xxxxxxxxxx> > > > >> Cc: xdp-hints@xxxxxxxxxxxxxxx > > > >> Cc: netdev@xxxxxxxxxxxxxxx > > > >> Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx> > > > >> --- > > > >> .../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!
