On 03/11/2022 13.48, Toke Høiland-Jørgensen wrote:

Jesper Dangaard Brouer [off-list ref] writes:

quoted

On 03/11/2022 01.09, Toke Høiland-Jørgensen wrote:

quoted

Stanislav Fomichev [off-list ref] writes:

quoted

On Wed, Nov 2, 2022 at 3:02 PM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

Jesper Dangaard Brouer [off-list ref] writes:

quoted

On 01/11/2022 18.05, Martin KaFai Lau wrote:

quoted

On 10/31/22 6:59 PM, Stanislav Fomichev wrote:

quoted

On Mon, Oct 31, 2022 at 3:57 PM Martin KaFai Lau
[off-list ref] wrote:

quoted

On 10/31/22 10:00 AM, Stanislav Fomichev wrote:

quoted

quoted

2. AF_XDP programs won't be able to access the metadata without
using a
custom XDP program that calls the kfuncs and puts the data into the
metadata area. We could solve this with some code in libxdp,
though; if
this code can be made generic enough (so it just dumps the available
metadata functions from the running kernel at load time), it may be
possible to make it generic enough that it will be forward-compatible
with new versions of the kernel that add new fields, which should
alleviate Florian's concern about keeping things in sync.

Good point. I had to convert to a custom program to use the kfuncs :-(
But your suggestion sounds good; maybe libxdp can accept some extra
info about at which offset the user would like to place the metadata
and the library can generate the required bytecode?

quoted

3. It will make it harder to consume the metadata when building
SKBs. I
think the CPUMAP and veth use cases are also quite important, and that
we want metadata to be available for building SKBs in this path. Maybe
this can be resolved by having a convenient kfunc for this that can be
used for programs doing such redirects. E.g., you could just call
xdp_copy_metadata_for_skb() before doing the bpf_redirect, and that
would recursively expand into all the kfunc calls needed to extract
the
metadata supported by the SKB path?

So this xdp_copy_metadata_for_skb will create a metadata layout that

Can the xdp_copy_metadata_for_skb be written as a bpf prog itself?
Not sure where is the best point to specify this prog though.
Somehow during
bpf_xdp_redirect_map?
or this prog belongs to the target cpumap and the xdp prog
redirecting to this
cpumap has to write the meta layout in a way that the cpumap is
expecting?

We're probably interested in triggering it from the places where xdp
frames can eventually be converted into skbs?
So for plain 'return XDP_PASS' and things like bpf_redirect/etc? (IOW,
anything that's not XDP_DROP / AF_XDP redirect).
We can probably make it magically work, and can generate
kernel-digestible metadata whenever data == data_meta, but the
question - should we?
(need to make sure we won't regress any existing cases that are not
relying on the metadata)

Instead of having some kernel-digestible meta data, how about calling
another bpf prog to initialize the skb fields from the meta area after
__xdp_build_skb_from_frame() in the cpumap, so
run_xdp_set_skb_fileds_from_metadata() may be a better name.

I very much like this idea of calling another bpf prog to initialize the
SKB fields from the meta area. (As a reminder, data need to come from
meta area, because at this point the hardware RX-desc is out-of-scope).
I'm onboard with xdp_copy_metadata_for_skb() populating the meta area.

We could invoke this BPF-prog inside __xdp_build_skb_from_frame().

We might need a new BPF_PROG_TYPE_XDP2SKB as this new BPF-prog
run_xdp_set_skb_fields_from_metadata() would need both xdp_buff + SKB as
context inputs. Right?  (Not sure, if this is acceptable with the BPF
maintainers new rules)

quoted

The xdp_prog@rx sets the meta data and then redirect.  If the
xdp_prog@rx can also specify a xdp prog to initialize the skb fields
from the meta area, then there is no need to have a kfunc to enforce a
kernel-digestible layout.  Not sure what is a good way to specify this
xdp_prog though...

The challenge of running this (BPF_PROG_TYPE_XDP2SKB) BPF-prog inside
__xdp_build_skb_from_frame() is that it need to know howto decode the
meta area for every device driver or XDP-prog populating this (as veth
and cpumap can get redirected packets from multiple device drivers).

If we have the helper to copy the data "out of" the drivers, why do we
need a second BPF program to copy data to the SKB?

IMHO the second BPF program to populate the SKB is needed to add
flexibility and extensibility.

My end-goal here is to speedup packet parsing.
This BPF-prog should (in time) be able to update skb->transport_header
and skb->network_header.  As I mentioned before, HW RX-hash already tell
us the L3 and L4 protocols and in-most-cases header-len.  Even without
HW-hints, the XDP-prog likely have parsed the packet once. This parse
information is lost today, and redone by netstack. What about storing
this header parse info in meta data and re-use in this new XDP2SKB hook?

The reason for suggesting this BPF-prog to be a callback, associated
with the net_device, were that HW is going to differ on what HW hints
that HW support.  Thus, we can avoid a generic C-function that need to
check for all the possible hints, and instead have a BPF-prog that only
contains the code that is relevant for this net_device.

But that's exactly what the xdp_copy_metadata_for_skb() is! It's a
dynamic "BPF program" (generated as unrolled kfunc calls) just running
in the helper and stashing the results in an intermediate struct in the
metadata area. And once it's done that, we don't need *another* dynamic
BPF program to read it back out and populate the SKB, because the
intermediate format it's been stashed into is under the control of the
kernel (we just need a flag to indicate that it's there).

quoted

quoted

quoted

quoted

I.e., the XDP program calls xdp_copy_metadata_for_skb(); this invokes
each of the kfuncs needed for the metadata used by SKBs, all of which
get unrolled. The helper takes the output of these metadata-extracting
kfuncs and stores it "somewhere". This "somewhere" could well be the
metadata area; but in any case, since it's hidden away inside a helper
(or kfunc) from the calling XDP program's PoV, the helper can just stash
all the data in a fixed format, which __xdp_build_skb_from_frame() can
then just read statically. We could even make this format match the
field layout of struct sk_buff, so all we have to do is memcpy a
contiguous chunk of memory when building the SKB.

+1

Sorry, I think this "hiding" layout trick is going in a wrong direction.

Imagine the use-case of cpumap redirect.  The physical device XDP-prog
calls xdp_copy_metadata_for_skb() to extract info from RX-desc, then it
calls redirect into cpumap.  On remote CPU, the xdp_frame is picked up,
and then I want to run another XDP-prog that want to look at these
HW-hints, and then likely call XDP_PASS which creates the SKB, also
using these HW-hints.  I take it, it would not be possible when using
the xdp_copy_metadata_for_skb() helper?

You're right that it should be possible to read the values back out
again later. That is totally possible with this scheme, though; the
'xdp_to_skb_metadata' is going to be in the vmlinux BTF, so an XDP
program can just read that. We can explicitly support it by using the
BTF ID as the "magic value" as you suggest, which would be fine by me.

I'm on-board if we as you suggest, add the BTF_ID as the "magic value" 
(as last member due to AF_XDP processing).  When 
xdp_copy_metadata_for_skb() writes 'xdp_to_skb_metadata' in metadata 
area.  We should simply see this BTF-ID as a 'cookie' or magic number.

I still think we should be using the __randomize_layout trick, though,
precisely so that BPF consumers are forced to use BTF relocations to
read it; otherwise we risk the struct layout ossifying into UAPI because
people are just going to assume it's static...

I'm also on-board with some level of randomization to the struct to 
force consumers to use BTF for relocations. e.g BTF-ID cookie/magic 
should be at a fixed location.

quoted

quoted

quoted

I'm currently doing exactly what you're suggesting (minus matching skb layout):

struct xdp_to_skb_metadata {
    u32 magic; // randomized at boot
    ... skb-consumable-metadata in fixed format
} __randomize_layout;

bpf_xdp_copy_metadata_for_skb() does bpf_xdp_adjust_meta(ctx,
-sizeof(struct xdp_to_skb_metadata)) and then calls a bunch of kfuncs
to fill in the actual data.

Then, at __xdp_build_skb_from_frame time, I'm having a regular kernel
C code that parses that 'struct xdp_to_skb_metadata'.
(To be precise, I'm trying to parse the metadata from
skb_metadata_set; it's called from __xdp_build_skb_from_frame, but not
100% sure that's the right place).
(I also randomize the layout and magic to make sure userspace doesn't
depend on it because nothing stops this packet to be routed into xsk
socket..)

Ah, nice trick with __randomize_layout - I agree we need to do something
to prevent userspace from inadvertently starting to rely on this, and
this seems like a great solution!

Sorry, I disagree where this is going.  Why do all of a sudden want to
prevent userspace (e.g. AF_XDP) from using this data?!?

See above: I don't think we should prevent userspace from using it (and
we're not), but we should prevent the struct layout from ossifying.

Okay, then we are in agreement, avoid ossifying struct layout.

quoted

The hole exercise started with wanting to provide AF_XDP with these
HW-hints. The hints a standard AF_XDP user wants is likely very
similar to what the SKB user wants. Why do the AF_XDP user need to
open code this?

The BTF-ID scheme precisely allows us to expose this layout to
userspace, and at the same time have freedom to change this in kernel
space, as userspace must decode the BTF-layout before reading this.
I was hoping xdp_copy_metadata_for_skb() could simply use the BTF-ID
scheme, with the BTF-ID of struct xdp_hints_rx_common, is to too much to
ask for?  You can just consider the BTF-ID as the magic number, as it
will be more-or-less random per kernel build (and module load order).

As mentioned above, I would be totally fine with just having the
xdp_to_skb_metadata be part of BTF, enabling both XDP programs and
AF_XDP consumers to re-use it.

My use-case is that AF_XDP will need to key on this runtime BTF-ID magic 
value anyway to read out 'xdp_to_skb_metadata' values.  I have an 
XDP-prog running, that will RX-timestamp only the time-sync protocol 
packets.  Code wise, I will simply add my RX-timestamp on top of struct 
'xdp_to_skb_metadata' and then update the BTF-ID magic value.  I don't 
need to add a real BTF-ID but just some magic value that my AF_XDP 
userspace prog knows about.  In my current code[1], I'm playing nice and 
adds the BPF-prog's own local BTF-ID via bpf_core_type_id_local().

[1] 
https://github.com/xdp-project/bpf-examples/blob/master/AF_XDP-interaction/af_xdp_kern.c#L80

quoted

quoted

Look forward to seeing what the whole thing looks like in a more
complete form :)

I'm sort of on-board with the kfuncs and unroll-tricks, if I can see
some driver code that handles the issues of getting HW setup state
exposed needed to decode the RX-desc format.

I sense that I myself, haven't been good enough to explain/convey the
BTF-ID scheme.  Next week, I will code some examples that demo how
BTF-IDs can be used from BPF-progs, even as a communication channel
between different BPF-progs (e.g. drv XDP-prog -> cpumap XDP-prog ->
TC-BPF).

For my part at least, it's not a lack of understanding that makes me
prefer the kfunc approach. Rather, it's the complexity of having to
resolve the multiple BTF IDs, and the risk of ossifying the struct
layouts because people are going to do that wrong. Using kfuncs gives us
much more control of the API, especially if we combine it with struct
randomisation for the bits we do expose.

Translating what we've discussed above into the terms used in your patch
series, this would correspond to *only* having the xdp_metadata_common
struct exposed via BTF, and not bothering with all the other
driver-specific layouts. So an XDP/AF_XDP user that only wants to use
the metadata that's also used by the stack can just call
xdp_copy_metadata_for_skb(), and then read the resulting metadata area
(using BTF). And if someone wants to access metadata that's *not* used
by the stack, they'd have to call additional kfuncs to extract that.

I agree, that this patchset does/will simplify my patchset.  My driver 
specific structs for BTF-IDs will no-longer be needed.  As it is now 
up-to XDP-prog to explicitly extend with fields. This should reduce your 
concern with resolving multiple BTF IDs.

Maybe after this patchset, I would suggest that we could create some 
"kernel-central" struct's that have e.g. RX-timestamp and mark (mlx5 
have HW support for mark) and protocol types (via RX-hash). As these 
could be used by XDP-prog's that explicitly extract these, and 
communicate the layout via setting the BTF-ID (via calling 
bpf_core_type_id_kernel()).  Making it easier to consume from chained 
BPF-progs, AF_XDP and even via kernel C-code that updates SKB fields as 
the number of these magic BTF-ID's will be small enough.

And similarly, if someone wants only a subset of the metadata used by an
SKB, they can just *not* call xdp_copy_metadata_for_skb(), and instead
just call the individual kfuncs to extract just the fields they want.

I think this strikes a nice balance between the flexibility by the
kernel to change things, the flexibility of XDP consumers to request
only the data they want, and the ability for the same metadata to be
consumed at different points. WDYT?

With above comments, I think we are closer to an agreement again.

--Jesper