Add XDP rx hw hints support performing XDP_REDIRECT

This series introduces the xdp_rx_meta struct in the xdp_buff/xdp_frame

one as a container to store the already supported xdp rx hw hints (rx_hash and rx_vlan, rx_timestamp will be stored in skb_shared_info area) when the eBPF program running on the nic performs XDP_REDIRECT. Doing so, we are able to set the skb metadata converting the xdp_buff/xdp_frame to a skb.

From: Lorenzo Bianconi <[email protected]>
Date: Sat, 21 Sep 2024 18:52:56 +0200

Quoted 1 line

This series introduces the xdp_rx_meta struct in the xdp_buff/xdp_frame

&xdp_buff is on the stack.
&xdp_frame consumes headroom.

IOW they're size-sensitive and putting metadata directly there might play bad; if not now, then later.

Our idea (me + Toke) was as follows:

- new BPF kfunc to build generic meta. If called, the driver builds a generic meta with hash, csum etc., in the data_meta area.

Yes, this also consumes headroom, but only when the corresponding func is called. Introducing new fields like you're doing will consume it unconditionally;

- when &xdp_frame gets converted to sk_buff, the function checks whether data_meta contains a generic structure filled with hints.

We also thought about &skb_shared_info, but it's also size-sensitive as it consumes tailroom.

Quoted 4 lines

one as a container to store the already supported xdp rx hw hints (rx_hash and rx_vlan, rx_timestamp will be stored in skb_shared_info area) when the eBPF program running on the nic performs XDP_REDIRECT. Doing so, we are able to set the skb metadata converting the xdp_buff/xdp_frame to a skb.

Thanks,
Olek

Quoted 13 lines

From: Lorenzo Bianconi <[email protected]>
Date: Sat, 21 Sep 2024 18:52:56 +0200

Quoted 1 line

This series introduces the xdp_rx_meta struct in the xdp_buff/xdp_frame

&xdp_buff is on the stack.
&xdp_frame consumes headroom.

IOW they're size-sensitive and putting metadata directly there might play bad; if not now, then later.

Our idea (me + Toke) was as follows:

Quoted 2 lines

- new BPF kfunc to build generic meta. If called, the driver builds a generic meta with hash, csum etc., in the data_meta area.

I do agree that it should be the XDP prog (via a new BPF kfunc) that decide if xdp_frame should be updated to contain a generic meta struct. *BUT* I think we should use the xdp_frame area, and not the xdp->data_meta area.

A details is that I think this kfunc should write data directly into xdp_frame area, even then we are only operating on the xdp_buff, as we do have access to the area xdp_frame will be created in.

When using data_meta area, then netstack encap/decap needs to move the data_meta area (extra cycles). The xdp_frame area (live in top) don't have this issue.

It is easier to allow xdp_frame area to survive longer together with the SKB. Today we "release" this xdp_frame area to be used by SKB for extra headroom (see xdp_scrub_frame). I can imagine that we can move SKB fields to this area, and reduce the size of the SKB alloc. (This then becomes the mini-SKB we discussed a couple of years ago).

Quoted 3 lines

Yes, this also consumes headroom, but only when the corresponding func is called. Introducing new fields like you're doing will consume it unconditionally;

We agree on the kfunc call marks area as consumed/in-use. We can extend xdp_frame statically like Lorenzo does (with struct xdp_rx_meta), but xdp_frame->flags can be used for marking this area as used or not.

Quoted 5 lines

- when &xdp_frame gets converted to sk_buff, the function checks whether data_meta contains a generic structure filled with hints.

Agree, but take data from xdp_frame->xdp_rx_meta.

When XDP returns XDP_PASS, then I also want to see this data applied to the SKB. In patchset[1] Yan called this xdp_frame_fixup_skb_offloading() and xdp_buff_fixup_skb_offloading(). (Perhaps "fixup" isn't the right term, "apply" is perhaps better). Having this generic-name allow us to extend with newer offloads, and eventually move members out of SKB.

We called it "fixup", because our use-case is that our XDP load-balancer (Unimog) XDP_TX bounce packets with in GRE header encap, and on the receiving NIC (due to encap) we lost the HW hash/csum, which we want to transfer from the original NIC, decap in XDP and apply the original HW hash/csum via this "fixup" call.

--Jesper

[1] https://lore.kernel.org/all/[email protected]/

Quoted 10 lines

We also thought about &skb_shared_info, but it's also size-sensitive as it consumes tailroom.

Quoted 4 lines

one as a container to store the already supported xdp rx hw hints (rx_hash and rx_vlan, rx_timestamp will be stored in skb_shared_info area) when the eBPF program running on the nic performs XDP_REDIRECT. Doing so, we are able to set the skb metadata converting the xdp_buff/xdp_frame to a skb.

Thanks,
Olek

Quoted 16 lines · Alexander Lobakin

Quoted 13 lines

From: Lorenzo Bianconi <[email protected]>
Date: Sat, 21 Sep 2024 18:52:56 +0200

Quoted 1 line

This series introduces the xdp_rx_meta struct in the xdp_buff/xdp_frame

&xdp_buff is on the stack.
&xdp_frame consumes headroom.

IOW they're size-sensitive and putting metadata directly there might play bad; if not now, then later.

Our idea (me + Toke) was as follows:

Quoted 7 lines

Quoted 2 lines

- new BPF kfunc to build generic meta. If called, the driver builds a generic meta with hash, csum etc., in the data_meta area.

I do agree that it should be the XDP prog (via a new BPF kfunc) that decide if xdp_frame should be updated to contain a generic meta struct. *BUT* I think we should use the xdp_frame area, and not the xdp->data_meta area.

ack, I will add a new kfunc for it.

Quoted 4 lines

A details is that I think this kfunc should write data directly into xdp_frame area, even then we are only operating on the xdp_buff, as we do have access to the area xdp_frame will be created in.

this would avoid to copy it when we convert from xdp_buff to xdp_frame, nice :)

Quoted 17 lines

When using data_meta area, then netstack encap/decap needs to move the data_meta area (extra cycles). The xdp_frame area (live in top) don't have this issue.

It is easier to allow xdp_frame area to survive longer together with the SKB. Today we "release" this xdp_frame area to be used by SKB for extra headroom (see xdp_scrub_frame). I can imagine that we can move SKB fields to this area, and reduce the size of the SKB alloc. (This then becomes the mini-SKB we discussed a couple of years ago).

Quoted 3 lines

Yes, this also consumes headroom, but only when the corresponding func is called. Introducing new fields like you're doing will consume it unconditionally;

Quoted 3 lines

We agree on the kfunc call marks area as consumed/in-use. We can extend xdp_frame statically like Lorenzo does (with struct xdp_rx_meta), but xdp_frame->flags can be used for marking this area as used or not.

the only downside with respect to a TLV approach would be to consume all the xdp_rx_meta as soon as we set a single xdp rx hw hint in it, right? The upside is it is easier and it requires less instructions.

Quoted 54 lines · Lorenzo Bianconi

Quoted 16 lines · Alexander Lobakin

Quoted 13 lines

From: Lorenzo Bianconi <[email protected]>
Date: Sat, 21 Sep 2024 18:52:56 +0200

Quoted 1 line

This series introduces the xdp_rx_meta struct in the xdp_buff/xdp_frame

&xdp_buff is on the stack.
&xdp_frame consumes headroom.

IOW they're size-sensitive and putting metadata directly there might play bad; if not now, then later.

Our idea (me + Toke) was as follows:

Quoted 7 lines

Quoted 2 lines

- new BPF kfunc to build generic meta. If called, the driver builds a generic meta with hash, csum etc., in the data_meta area.

I do agree that it should be the XDP prog (via a new BPF kfunc) that decide if xdp_frame should be updated to contain a generic meta struct. *BUT* I think we should use the xdp_frame area, and not the xdp->data_meta area.

ack, I will add a new kfunc for it.

Quoted 4 lines

A details is that I think this kfunc should write data directly into xdp_frame area, even then we are only operating on the xdp_buff, as we do have access to the area xdp_frame will be created in.

this would avoid to copy it when we convert from xdp_buff to xdp_frame, nice :)

Quoted 20 lines

When using data_meta area, then netstack encap/decap needs to move the data_meta area (extra cycles). The xdp_frame area (live in top) don't have this issue.

It is easier to allow xdp_frame area to survive longer together with the SKB. Today we "release" this xdp_frame area to be used by SKB for extra headroom (see xdp_scrub_frame). I can imagine that we can move SKB fields to this area, and reduce the size of the SKB alloc. (This then becomes the mini-SKB we discussed a couple of years ago).

Quoted 3 lines

Yes, this also consumes headroom, but only when the corresponding func is called. Introducing new fields like you're doing will consume it unconditionally;

We agree on the kfunc call marks area as consumed/in-use. We can extend xdp_frame statically like Lorenzo does (with struct xdp_rx_meta), but xdp_frame->flags can be used for marking this area as used or not.

Quoted 3 lines

the only downside with respect to a TLV approach would be to consume all the xdp_rx_meta as soon as we set a single xdp rx hw hint in it, right? The upside is it is easier and it requires less instructions.

FYI, we also had a discussion related to this at LPC on Friday, in this
session: https://lpc.events/event/18/contributions/1935/

The context here was that Arthur and Jakub want to also support extended rich metadata all the way through the SKB path, and are looking at the same area used for XDP metadata to store it. So there's a need to manage both the kernel's own usage of that area, and userspace/BPF usage of it.

I'll try to summarise some of the points of that discussion (all interpretations are my own, of course):

- We want something that can be carried with a frame all the way from the XDP layer, through all SKB layers and to userspace (to replace the use of skb->mark for this purpose).

- We want different applications running on the system (of which the kernel itself if one, cf this discussion) to be able to share this field, without having to have an out of band registry (like a Github repository where applications can agree on which bits to use). Which probably means that the kernel needs to be in the loop somehow to explicitly allocate space in the metadata area and track offsets.

- Having an explicit API to access this from userspace, without having to go through BPF (i.e., a socket- or CMSG-based API) would be useful.

The TLV format was one of the suggestions in Arthur and Jakub's talk, but AFAICT, there was not a lot of enthusiasm about this in the room (myself included), because of the parsing overhead and complexity. I believe the alternative that was seen as most favourable was a map lookup-style API, where applications can request a metadata area of arbitrary size and get an ID assigned that they can then use to set/get values in the data path.

So, sketching this out, this could be realised by something like:

/* could be called from BPF, or through netlink or sysfs; may fail, if
* there is no more space
*/
int metadata_id = register_packet_metadata_field(sizeof(struct my_meta));

The ID is just an opaque identifier that can then be passed to getter/setter functions (for both SKB and XDP), like:

ret = bpf_set_packet_metadata_field(pkt, metadata_id,
&my_meta_value, sizeof(my_meta_value))

ret = bpf_get_packet_metadata_field(pkt, metadata_id,
&my_meta_value, sizeof(my_meta_value))

On the kernel side, the implementation would track registered fields in a global structure somewhere, say:

struct pkt_metadata_entry {
int id;
u8 sz;
u8 offset;
u8 bit;
};

struct pkt_metadata_registry { /* allocated as a system-wide global */
u8 num_entries;
u8 total_size;
struct pkt_metadata_entry entries[MAX_ENTRIES];
};

struct xdp_rx_meta { /* at then end of xdp_frame */
u8 sz; /* set to pkt_metadata_registry->total_size on alloc */ u8 fields_set; /* bitmap of fields that have been set, see below */ u8 data[];
};

int register_packet_metadata_field(u8 size) {
struct pkt_metadata_registry *reg = get_global_registry(); struct pkt_metadata_entry *entry;

if (size + reg->total_size > MAX_METADATA_SIZE)

    return -ENOSPC;

  entry = &reg->entries[reg->num_entries++];
  entry->id = assign_id();
  entry->sz = size;
  entry->offset = reg->total_size;
  entry->bit = reg->num_entries - 1;
  reg->total_size += size;

return entry->id;
}

int bpf_set_packet_metadata_field(struct xdp_frame *frm, int id, void
*value, size_t sz)
{
struct pkt_metadata_entry *entry = get_metadata_entry_by_id(id);

if (!entry)

    return -ENOENT;

if (entry->sz != sz)
return -EINVAL; /* user error */

if (frm->rx_meta.sz < entry->offset + sz)
return -EFAULT; /* entry allocated after xdp_frame was initialised */

  memcpy(&frm->rx_meta.data + entry->offset, value, sz);
  frm->rx_meta.fields_set |= BIT(entry->bit);

return 0;
}

int bpf_get_packet_metadata_field(struct xdp_frame *frm, int id, void
*value, size_t sz)
{
struct pkt_metadata_entry *entry = get_metadata_entry_by_id(id);

if (!entry)

    return -ENOENT;

if (entry->sz != sz)

    return -EINVAL;

if (frm->rx_meta.sz < entry->offset + sz)
return -EFAULT; /* entry allocated after xdp_frame was initialised */

if (!(frm->rx_meta.fields_set & BIT(entry->bit)))

    return -ENOENT;

  memcpy(value, &frm->rx_meta.data + entry->offset, sz);

return 0;
}

I'm hinting at some complications here (with the EFAULT return) that needs to be resolved: there is no guarantee that a given packet will be in sync with the current status of the registered metadata, so we need explicit checks for this. If metadata entries are de-registered again this also means dealing with holes and/or reshuffling the metadata layout to reuse the released space (incidentally, this is the one place where a TLV format would have advantages).

The nice thing about an API like this, though, is that it's extensible, and the kernel itself can be just another consumer of it for the metadata fields Lorenzo is adding in this series. I.e., we could just pre-define some IDs for metadata vlan, timestamp etc, and use the same functions as above from within the kernel to set and get those values; using the registry, there could even be an option to turn those off if an application wants more space for its own usage. Or, alternatively, we could keep the kernel-internal IDs hardcoded and always allocated, and just use the getter/setter functions as the BPF API for accessing them.

-Toke

Quoted 7 lines

I'm hinting at some complications here (with the EFAULT return) that needs to be resolved: there is no guarantee that a given packet will be in sync with the current status of the registered metadata, so we need explicit checks for this. If metadata entries are de-registered again this also means dealing with holes and/or reshuffling the metadata layout to reuse the released space (incidentally, this is the one place where a TLV format would have advantages).

I like this approach but it seems to me more suitable for 'sw' metadata (this is main Arthur and Jakub use case iiuc) where the userspace would enable/disable these functionalities system-wide.
Regarding device hw metadata (e.g. checksum offload) I can see some issues since on a system we can have multiple NICs with different capabilities. If we consider current codebase, stmmac driver supports only rx timestamp, while mlx5 supports all of them. In a theoretical system with these two NICs, since pkt_metadata_registry is global system-wide, we will end-up with quite a lot of holes for the stmmac, right? (I am not sure if this case is relevant or not). In other words, we will end-up with a fixed struct for device rx hw metadata (like xdp_rx_meta). So I am wondering if we really need all this complexity for xdp rx hw metadata?

Maybe we can start with a simple approach for xdp rx hw metadata putting the struct in xdp_frame as suggested by Jesper and covering the most common use-cases. We can then integrate this approach with Arthur/Jakub's solution without introducing any backward compatibility issue since these field are not visible to userspace.

Quoted 8 lines · Lorenzo Bianconi

Quoted 7 lines

I'm hinting at some complications here (with the EFAULT return) that needs to be resolved: there is no guarantee that a given packet will be in sync with the current status of the registered metadata, so we need explicit checks for this. If metadata entries are de-registered again this also means dealing with holes and/or reshuffling the metadata layout to reuse the released space (incidentally, this is the one place where a TLV format would have advantages).

Quoted 12 lines

I like this approach but it seems to me more suitable for 'sw' metadata (this is main Arthur and Jakub use case iiuc) where the userspace would enable/disable these functionalities system-wide.
Regarding device hw metadata (e.g. checksum offload) I can see some issues since on a system we can have multiple NICs with different capabilities. If we consider current codebase, stmmac driver supports only rx timestamp, while mlx5 supports all of them. In a theoretical system with these two NICs, since pkt_metadata_registry is global system-wide, we will end-up with quite a lot of holes for the stmmac, right? (I am not sure if this case is relevant or not). In other words, we will end-up with a fixed struct for device rx hw metadata (like xdp_rx_meta). So I am wondering if we really need all this complexity for xdp rx hw metadata?

Well, the "holes" will be there anyway (in your static struct approach). They would just correspond to parts of the "struct xdp_rx_meta" being unset.

What the "userspace can turn off the fields system wide" would accomplish is to *avoid* the holes if you know that you will never need them. E.g., say a system administrator know that they have no networks that use (offloaded) VLANs. They could then disable the vlan offload field system-wide, and thus reclaim the four bytes taken up by the "vlan" member of struct xdp_rx_meta, freeing that up for use by application metadata.

However, it may well be that the complexity of allowing fields to be turned off is not worth the gains. At least as long as there are only the couple of HW metadata fields that we have currently. Having the flexibility to change our minds later would be good, though, which is mostly a matter of making the API exposed to BPF and/or userspace flexible enough to allow us to move things around in memory in the future. Which was basically my thought with the API I sketched out in the previous email. I.e., you could go:

ret = bpf_get_packet_metadata_field(pkt, METADATA_ID_HW_HASH,

                                    &my_hash_vlaue, sizeof(u32))

...and the METADATA_ID_HW_HASH would be a constant defined by the kernel, for which the bpf_get_packet_metadata_field() kfunc just has a hardcoded lookup into struct xdp_rx_meta. And then, if we decide to move the field in the future, we just change the kfunc implementation, with no impact to the BPF programs calling it.

Quoted 5 lines

Maybe we can start with a simple approach for xdp rx hw metadata putting the struct in xdp_frame as suggested by Jesper and covering the most common use-cases. We can then integrate this approach with Arthur/Jakub's solution without introducing any backward compatibility issue since these field are not visible to userspace.

Yes, this is basically the gist of my suggestion (as I hopefully managed to clarify above): Expose the fields via an API that is flexible enough that we can move things around should the need arise, *and* which can co-exist with the user-defined application metadata.

-Toke

Quoted 23 lines

Quoted 8 lines · Lorenzo Bianconi

Quoted 7 lines

I'm hinting at some complications here (with the EFAULT return) that needs to be resolved: there is no guarantee that a given packet will be in sync with the current status of the registered metadata, so we need explicit checks for this. If metadata entries are de-registered again this also means dealing with holes and/or reshuffling the metadata layout to reuse the released space (incidentally, this is the one place where a TLV format would have advantages).

Quoted 12 lines

I like this approach but it seems to me more suitable for 'sw' metadata (this is main Arthur and Jakub use case iiuc) where the userspace would enable/disable these functionalities system-wide.
Regarding device hw metadata (e.g. checksum offload) I can see some issues since on a system we can have multiple NICs with different capabilities. If we consider current codebase, stmmac driver supports only rx timestamp, while mlx5 supports all of them. In a theoretical system with these two NICs, since pkt_metadata_registry is global system-wide, we will end-up with quite a lot of holes for the stmmac, right? (I am not sure if this case is relevant or not). In other words, we will end-up with a fixed struct for device rx hw metadata (like xdp_rx_meta). So I am wondering if we really need all this complexity for xdp rx hw metadata?

Quoted 3 lines

Well, the "holes" will be there anyway (in your static struct approach). They would just correspond to parts of the "struct xdp_rx_meta" being unset.

yes, what I wanted to say is I have the feeling we will end up 90% of the times in the same fields architecture and the cases where we can save some space seem very limited. Anyway, I am fine to discuss about a common architecture.

Quoted 8 lines

What the "userspace can turn off the fields system wide" would accomplish is to *avoid* the holes if you know that you will never need them. E.g., say a system administrator know that they have no networks that use (offloaded) VLANs. They could then disable the vlan offload field system-wide, and thus reclaim the four bytes taken up by the "vlan" member of struct xdp_rx_meta, freeing that up for use by application metadata.

Even if I like the idea of having a common approach for this kernel feature, hw metadata seems to me quite a corner case with respect of 'user-defined metadata', since:
- I do not think it is a common scenario to disable hw offload capabilities (e.g checksum offload in production)
- I guess it is not just enough to disable them via bpf, but the user/sysadmin will need even to configure the NIC via ethtool (so a 2-steps process).

I think we should pay attention to not overcomplicate something that is 99% enabled and just need to be fast. E.g I can see an issue of putting the hw rx metadata in metadata field since metadata grows backward and we will probably end up putting them in a different cacheline with respect to xdp_frame (xdp_headroom is usually 256B).

Quoted 10 lines

However, it may well be that the complexity of allowing fields to be turned off is not worth the gains. At least as long as there are only the couple of HW metadata fields that we have currently. Having the flexibility to change our minds later would be good, though, which is mostly a matter of making the API exposed to BPF and/or userspace flexible enough to allow us to move things around in memory in the future. Which was basically my thought with the API I sketched out in the previous email. I.e., you could go:

Quoted 2 lines

ret = bpf_get_packet_metadata_field(pkt, METADATA_ID_HW_HASH,

                                    &my_hash_vlaue, sizeof(u32))

yes, my plan is to add dedicated bpf kfuncs to store hw metadata in xdp_frame/xdp_buff

Quoted 8 lines

...and the METADATA_ID_HW_HASH would be a constant defined by the kernel, for which the bpf_get_packet_metadata_field() kfunc just has a hardcoded lookup into struct xdp_rx_meta. And then, if we decide to move the field in the future, we just change the kfunc implementation, with no impact to the BPF programs calling it.

maybe we can use what we Stanislav have already added (maybe removing xdp prefix):

enum xdp_rx_metadata {
XDP_METADATA_KFUNC_RX_TIMESTAMP,
XDP_METADATA_KFUNC_RX_HASH,
XDP_METADATA_KFUNC_RX_VLAN_TAG
};

FYI, we also had a discussion related to this at LPC on Friday, in this
session: https://lpc.events/event/18/contributions/1935/

The context here was that Arthur and Jakub want to also support extended rich metadata all the way through the SKB path, and are looking at the same area used for XDP metadata to store it. So there's a need to manage both the kernel's own usage of that area, and userspace/BPF usage of it.

I'll try to summarise some of the points of that discussion (all interpretations are my own, of course):

- We want something that can be carried with a frame all the way from the XDP layer, through all SKB layers and to userspace (to replace the use of skb->mark for this purpose).

- We want different applications running on the system (of which the kernel itself if one, cf this discussion) to be able to share this field, without having to have an out of band registry (like a Github repository where applications can agree on which bits to use). Which probably means that the kernel needs to be in the loop somehow to explicitly allocate space in the metadata area and track offsets.

- Having an explicit API to access this from userspace, without having to go through BPF (i.e., a socket- or CMSG-based API) would be useful.

The TLV format was one of the suggestions in Arthur and Jakub's talk, but AFAICT, there was not a lot of enthusiasm about this in the room (myself included), because of the parsing overhead and complexity. I believe the alternative that was seen as most favourable was a map lookup-style API, where applications can request a metadata area of arbitrary size and get an ID assigned that they can then use to set/get values in the data path.

So, sketching this out, this could be realised by something like:

/* could be called from BPF, or through netlink or sysfs; may fail, if
* there is no more space
*/
int metadata_id = register_packet_metadata_field(sizeof(struct my_meta));

The ID is just an opaque identifier that can then be passed to getter/setter functions (for both SKB and XDP), like:

ret = bpf_set_packet_metadata_field(pkt, metadata_id,
&my_meta_value, sizeof(my_meta_value))

ret = bpf_get_packet_metadata_field(pkt, metadata_id,
&my_meta_value, sizeof(my_meta_value))

On the kernel side, the implementation would track registered fields in a global structure somewhere, say:

struct pkt_metadata_entry {
int id;
u8 sz;
u8 offset;
u8 bit;
};

struct pkt_metadata_registry { /* allocated as a system-wide global */
u8 num_entries;
u8 total_size;
struct pkt_metadata_entry entries[MAX_ENTRIES];
};

struct xdp_rx_meta { /* at then end of xdp_frame */
u8 sz; /* set to pkt_metadata_registry->total_size on alloc */ u8 fields_set; /* bitmap of fields that have been set, see below */ u8 data[];
};

int register_packet_metadata_field(u8 size) {
struct pkt_metadata_registry *reg = get_global_registry(); struct pkt_metadata_entry *entry;

if (size + reg->total_size > MAX_METADATA_SIZE)

    return -ENOSPC;

  entry = &reg->entries[reg->num_entries++];
  entry->id = assign_id();
  entry->sz = size;
  entry->offset = reg->total_size;
  entry->bit = reg->num_entries - 1;
  reg->total_size += size;

return entry->id;
}

int bpf_set_packet_metadata_field(struct xdp_frame *frm, int id, void
*value, size_t sz)
{
struct pkt_metadata_entry *entry = get_metadata_entry_by_id(id);

if (!entry)

    return -ENOENT;

if (entry->sz != sz)
return -EINVAL; /* user error */

if (frm->rx_meta.sz < entry->offset + sz)
return -EFAULT; /* entry allocated after xdp_frame was initialised */

  memcpy(&frm->rx_meta.data + entry->offset, value, sz);
  frm->rx_meta.fields_set |= BIT(entry->bit);

return 0;
}

int bpf_get_packet_metadata_field(struct xdp_frame *frm, int id, void
*value, size_t sz)
{
struct pkt_metadata_entry *entry = get_metadata_entry_by_id(id);

if (!entry)

    return -ENOENT;

if (entry->sz != sz)

    return -EINVAL;

if (frm->rx_meta.sz < entry->offset + sz)
return -EFAULT; /* entry allocated after xdp_frame was initialised */

if (!(frm->rx_meta.fields_set & BIT(entry->bit)))

    return -ENOENT;

  memcpy(value, &frm->rx_meta.data + entry->offset, sz);

return 0;
}

I'm hinting at some complications here (with the EFAULT return) that needs to be resolved: there is no guarantee that a given packet will be in sync with the current status of the registered metadata, so we need explicit checks for this. If metadata entries are de-registered again this also means dealing with holes and/or reshuffling the metadata layout to reuse the released space (incidentally, this is the one place where a TLV format would have advantages).

The nice thing about an API like this, though, is that it's extensible, and the kernel itself can be just another consumer of it for the metadata fields Lorenzo is adding in this series. I.e., we could just pre-define some IDs for metadata vlan, timestamp etc, and use the same functions as above from within the kernel to set and get those values; using the registry, there could even be an option to turn those off if an application wants more space for its own usage. Or, alternatively, we could keep the kernel-internal IDs hardcoded and always allocated, and just use the getter/setter functions as the BPF API for accessing them.

-Toke

Quoted 22 lines · Toke Høiland-Jørgensen

FYI, we also had a discussion related to this at LPC on Friday, in this
session: https://lpc.events/event/18/contributions/1935/

The context here was that Arthur and Jakub want to also support extended rich metadata all the way through the SKB path, and are looking at the same area used for XDP metadata to store it. So there's a need to manage both the kernel's own usage of that area, and userspace/BPF usage of it.

I'll try to summarise some of the points of that discussion (all interpretations are my own, of course):

- We want something that can be carried with a frame all the way from the XDP layer, through all SKB layers and to userspace (to replace the use of skb->mark for this purpose).

- We want different applications running on the system (of which the kernel itself if one, cf this discussion) to be able to share this field, without having to have an out of band registry (like a Github repository where applications can agree on which bits to use). Which probably means that the kernel needs to be in the loop somehow to explicitly allocate space in the metadata area and track offsets.

Quoted 3 lines

- Having an explicit API to access this from userspace, without having to go through BPF (i.e., a socket- or CMSG-based API) would be useful.

Thanks for looping us in, and the great summary Toke!

Quoted 114 lines

The TLV format was one of the suggestions in Arthur and Jakub's talk, but AFAICT, there was not a lot of enthusiasm about this in the room (myself included), because of the parsing overhead and complexity. I believe the alternative that was seen as most favourable was a map lookup-style API, where applications can request a metadata area of arbitrary size and get an ID assigned that they can then use to set/get values in the data path.

So, sketching this out, this could be realised by something like:

/* could be called from BPF, or through netlink or sysfs; may fail, if
* there is no more space
*/
int metadata_id = register_packet_metadata_field(sizeof(struct my_meta));

The ID is just an opaque identifier that can then be passed to getter/setter functions (for both SKB and XDP), like:

ret = bpf_set_packet_metadata_field(pkt, metadata_id,
&my_meta_value, sizeof(my_meta_value))

ret = bpf_get_packet_metadata_field(pkt, metadata_id,
&my_meta_value, sizeof(my_meta_value))

On the kernel side, the implementation would track registered fields in a global structure somewhere, say:

struct pkt_metadata_entry {
int id;
u8 sz;
u8 offset;
u8 bit;
};

struct pkt_metadata_registry { /* allocated as a system-wide global */
u8 num_entries;
u8 total_size;
struct pkt_metadata_entry entries[MAX_ENTRIES];
};

struct xdp_rx_meta { /* at then end of xdp_frame */
u8 sz; /* set to pkt_metadata_registry->total_size on alloc */ u8 fields_set; /* bitmap of fields that have been set, see below */ u8 data[];
};

int register_packet_metadata_field(u8 size) {
struct pkt_metadata_registry *reg = get_global_registry(); struct pkt_metadata_entry *entry;

if (size + reg->total_size > MAX_METADATA_SIZE)

    return -ENOSPC;

  entry = &reg->entries[reg->num_entries++];
  entry->id = assign_id();
  entry->sz = size;
  entry->offset = reg->total_size;
  entry->bit = reg->num_entries - 1;
  reg->total_size += size;

return entry->id;
}

int bpf_set_packet_metadata_field(struct xdp_frame *frm, int id, void
*value, size_t sz)
{
struct pkt_metadata_entry *entry = get_metadata_entry_by_id(id);

if (!entry)

    return -ENOENT;

if (entry->sz != sz)
return -EINVAL; /* user error */

if (frm->rx_meta.sz < entry->offset + sz)
return -EFAULT; /* entry allocated after xdp_frame was initialised */

  memcpy(&frm->rx_meta.data + entry->offset, value, sz);
  frm->rx_meta.fields_set |= BIT(entry->bit);

return 0;
}

int bpf_get_packet_metadata_field(struct xdp_frame *frm, int id, void
*value, size_t sz)
{
struct pkt_metadata_entry *entry = get_metadata_entry_by_id(id);

if (!entry)

    return -ENOENT;

if (entry->sz != sz)

    return -EINVAL;

if (frm->rx_meta.sz < entry->offset + sz)
return -EFAULT; /* entry allocated after xdp_frame was initialised */

if (!(frm->rx_meta.fields_set & BIT(entry->bit)))

    return -ENOENT;

  memcpy(value, &frm->rx_meta.data + entry->offset, sz);

return 0;
}

I'm hinting at some complications here (with the EFAULT return) that needs to be resolved: there is no guarantee that a given packet will be in sync with the current status of the registered metadata, so we need explicit checks for this. If metadata entries are de-registered again this also means dealing with holes and/or reshuffling the metadata layout to reuse the released space (incidentally, this is the one place where a TLV format would have advantages).

Quoted 9 lines

The nice thing about an API like this, though, is that it's extensible, and the kernel itself can be just another consumer of it for the metadata fields Lorenzo is adding in this series. I.e., we could just pre-define some IDs for metadata vlan, timestamp etc, and use the same functions as above from within the kernel to set and get those values; using the registry, there could even be an option to turn those off if an application wants more space for its own usage. Or, alternatively, we could keep the kernel-internal IDs hardcoded and always allocated, and just use the getter/setter functions as the BPF API for accessing them.

That's exactly what I'm thinking of too, a simple API like:

get(u8 key, u8 len, void *val);
set(u8 key, u8 len, void *val);

With "well-known" keys like METADATA_ID_HW_HASH for hardware metadata.

If a NIC doesn't support a certain well-known metadata, the key wouldn't be set, and get() would return ENOENT.

I think this also lets us avoid having to "register" keys or bits of metadata with the kernel.
We'd reserve some number of keys for hardware metadata.

The remaining keys would be up to users. They'd have to allocate keys to services, and configure services to use those keys.
This is similar to the way listening on a certain port works: only one service can use port 80 or 443, and that can typically beconfigured in a service's config file.

This side-steps the whole question of how to change the registered metadata for in-flight packets, and how to deal with different NICs with different hardware metadata.

I think I've figured out a suitable encoding format, hopefully we'll have an RFC soon!

Quoted 23 lines · Arthur Fabre

Quoted 22 lines · Toke Høiland-Jørgensen

FYI, we also had a discussion related to this at LPC on Friday, in this
session: https://lpc.events/event/18/contributions/1935/

The context here was that Arthur and Jakub want to also support extended rich metadata all the way through the SKB path, and are looking at the same area used for XDP metadata to store it. So there's a need to manage both the kernel's own usage of that area, and userspace/BPF usage of it.

I'll try to summarise some of the points of that discussion (all interpretations are my own, of course):

- We want something that can be carried with a frame all the way from the XDP layer, through all SKB layers and to userspace (to replace the use of skb->mark for this purpose).

- We want different applications running on the system (of which the kernel itself if one, cf this discussion) to be able to share this field, without having to have an out of band registry (like a Github repository where applications can agree on which bits to use). Which probably means that the kernel needs to be in the loop somehow to explicitly allocate space in the metadata area and track offsets.

Quoted 5 lines

Quoted 3 lines

- Having an explicit API to access this from userspace, without having to go through BPF (i.e., a socket- or CMSG-based API) would be useful.

Thanks for looping us in, and the great summary Toke!

You're welcome :)

Quoted 129 lines

Quoted 114 lines

The TLV format was one of the suggestions in Arthur and Jakub's talk, but AFAICT, there was not a lot of enthusiasm about this in the room (myself included), because of the parsing overhead and complexity. I believe the alternative that was seen as most favourable was a map lookup-style API, where applications can request a metadata area of arbitrary size and get an ID assigned that they can then use to set/get values in the data path.

So, sketching this out, this could be realised by something like:

/* could be called from BPF, or through netlink or sysfs; may fail, if
* there is no more space
*/
int metadata_id = register_packet_metadata_field(sizeof(struct my_meta));

The ID is just an opaque identifier that can then be passed to getter/setter functions (for both SKB and XDP), like:

ret = bpf_set_packet_metadata_field(pkt, metadata_id,
&my_meta_value, sizeof(my_meta_value))

ret = bpf_get_packet_metadata_field(pkt, metadata_id,
&my_meta_value, sizeof(my_meta_value))

On the kernel side, the implementation would track registered fields in a global structure somewhere, say:

struct pkt_metadata_entry {
int id;
u8 sz;
u8 offset;
u8 bit;
};

struct pkt_metadata_registry { /* allocated as a system-wide global */
u8 num_entries;
u8 total_size;
struct pkt_metadata_entry entries[MAX_ENTRIES];
};

struct xdp_rx_meta { /* at then end of xdp_frame */
u8 sz; /* set to pkt_metadata_registry->total_size on alloc */ u8 fields_set; /* bitmap of fields that have been set, see below */ u8 data[];
};

int register_packet_metadata_field(u8 size) {
struct pkt_metadata_registry *reg = get_global_registry(); struct pkt_metadata_entry *entry;

if (size + reg->total_size > MAX_METADATA_SIZE)

    return -ENOSPC;

  entry = &reg->entries[reg->num_entries++];
  entry->id = assign_id();
  entry->sz = size;
  entry->offset = reg->total_size;
  entry->bit = reg->num_entries - 1;
  reg->total_size += size;

return entry->id;
}

int bpf_set_packet_metadata_field(struct xdp_frame *frm, int id, void
*value, size_t sz)
{
struct pkt_metadata_entry *entry = get_metadata_entry_by_id(id);

if (!entry)

    return -ENOENT;

if (entry->sz != sz)
return -EINVAL; /* user error */

if (frm->rx_meta.sz < entry->offset + sz)
return -EFAULT; /* entry allocated after xdp_frame was initialised */

  memcpy(&frm->rx_meta.data + entry->offset, value, sz);
  frm->rx_meta.fields_set |= BIT(entry->bit);

return 0;
}

int bpf_get_packet_metadata_field(struct xdp_frame *frm, int id, void
*value, size_t sz)
{
struct pkt_metadata_entry *entry = get_metadata_entry_by_id(id);

if (!entry)

    return -ENOENT;

if (entry->sz != sz)

    return -EINVAL;

if (frm->rx_meta.sz < entry->offset + sz)
return -EFAULT; /* entry allocated after xdp_frame was initialised */

if (!(frm->rx_meta.fields_set & BIT(entry->bit)))

    return -ENOENT;

  memcpy(value, &frm->rx_meta.data + entry->offset, sz);

return 0;
}

I'm hinting at some complications here (with the EFAULT return) that needs to be resolved: there is no guarantee that a given packet will be in sync with the current status of the registered metadata, so we need explicit checks for this. If metadata entries are de-registered again this also means dealing with holes and/or reshuffling the metadata layout to reuse the released space (incidentally, this is the one place where a TLV format would have advantages).

Quoted 9 lines

The nice thing about an API like this, though, is that it's extensible, and the kernel itself can be just another consumer of it for the metadata fields Lorenzo is adding in this series. I.e., we could just pre-define some IDs for metadata vlan, timestamp etc, and use the same functions as above from within the kernel to set and get those values; using the registry, there could even be an option to turn those off if an application wants more space for its own usage. Or, alternatively, we could keep the kernel-internal IDs hardcoded and always allocated, and just use the getter/setter functions as the BPF API for accessing them.

That's exactly what I'm thinking of too, a simple API like:

get(u8 key, u8 len, void *val);
set(u8 key, u8 len, void *val);

Quoted 8 lines

With "well-known" keys like METADATA_ID_HW_HASH for hardware metadata.

If a NIC doesn't support a certain well-known metadata, the key wouldn't be set, and get() would return ENOENT.

I think this also lets us avoid having to "register" keys or bits of metadata with the kernel.
We'd reserve some number of keys for hardware metadata.

Right, but how do you allocate space/offset for each key without an explicit allocation step? You'd basically have to encode the list of IDs in the metadata area itself, which implies a TLV format that you have to walk on every access? The registry idea in my example above was basically to avoid that...

Quoted 6 lines

The remaining keys would be up to users. They'd have to allocate keys to services, and configure services to use those keys.
This is similar to the way listening on a certain port works: only one service can use port 80 or 443, and that can typically beconfigured in a service's config file.

Right, well, port numbers *do* actually have an out of band service registry (IANA), which I thought was what we wanted to avoid? ;)

Quoted 7 lines

This side-steps the whole question of how to change the registered metadata for in-flight packets, and how to deal with different NICs with different hardware metadata.

I think I've figured out a suitable encoding format, hopefully we'll have an RFC soon!

Alright, cool!

-Toke

Quoted 24 lines

Quoted 15 lines

Quoted 9 lines

The nice thing about an API like this, though, is that it's extensible, and the kernel itself can be just another consumer of it for the metadata fields Lorenzo is adding in this series. I.e., we could just pre-define some IDs for metadata vlan, timestamp etc, and use the same functions as above from within the kernel to set and get those values; using the registry, there could even be an option to turn those off if an application wants more space for its own usage. Or, alternatively, we could keep the kernel-internal IDs hardcoded and always allocated, and just use the getter/setter functions as the BPF API for accessing them.

That's exactly what I'm thinking of too, a simple API like:

get(u8 key, u8 len, void *val);
set(u8 key, u8 len, void *val);

Quoted 8 lines

With "well-known" keys like METADATA_ID_HW_HASH for hardware metadata.

If a NIC doesn't support a certain well-known metadata, the key wouldn't be set, and get() would return ENOENT.

I think this also lets us avoid having to "register" keys or bits of metadata with the kernel.
We'd reserve some number of keys for hardware metadata.

Quoted 5 lines

Right, but how do you allocate space/offset for each key without an explicit allocation step? You'd basically have to encode the list of IDs in the metadata area itself, which implies a TLV format that you have to walk on every access? The registry idea in my example above was basically to avoid that...

I've been playing around with having a small fixed header at the front of the metadata itself, that lets you access values without walking them all.

Still WIP, and maybe this is too restrictive, but it lets you encode 64 2, 4, or 8 byte KVs with a single 16 byte header:

Quoted 8 lines

Quoted 5 lines

The remaining keys would be up to users. They'd have to allocate keys to services, and configure services to use those keys.
This is similar to the way listening on a certain port works: only one service can use port 80 or 443, and that can typically beconfigured in a service's config file.

Right, well, port numbers *do* actually have an out of band service registry (IANA), which I thought was what we wanted to avoid? ;)

Depends how you think about it ;)

I think we should avoid a global registry. But having a registry per deployment / server doesn't seem awful. Services that want to use a field would have a config file setting to set which index it corresponds to.
Admins would just have to pick a free one on their system, and set it in the config file of the service.

This is similar to what we do for non-IANA registered ports internally. For example each service needs a port on an internal interface to expose metrics, and we just track which ports are taken in our config management.

Dynamically registering fields means you have to share the returned ID with whoever is interested, which sounds tricky.
If an XDP program sets a field like packet_id, every tracing program that looks at it, and userspace service, would need to know what the ID of that field is.
Is there a way to easily share that ID with all of them?

Quoted 30 lines · Arthur Fabre

Quoted 24 lines

Quoted 15 lines

Quoted 9 lines

The nice thing about an API like this, though, is that it's extensible, and the kernel itself can be just another consumer of it for the metadata fields Lorenzo is adding in this series. I.e., we could just pre-define some IDs for metadata vlan, timestamp etc, and use the same functions as above from within the kernel to set and get those values; using the registry, there could even be an option to turn those off if an application wants more space for its own usage. Or, alternatively, we could keep the kernel-internal IDs hardcoded and always allocated, and just use the getter/setter functions as the BPF API for accessing them.

That's exactly what I'm thinking of too, a simple API like:

get(u8 key, u8 len, void *val);
set(u8 key, u8 len, void *val);

Quoted 8 lines

With "well-known" keys like METADATA_ID_HW_HASH for hardware metadata.

If a NIC doesn't support a certain well-known metadata, the key wouldn't be set, and get() would return ENOENT.

I think this also lets us avoid having to "register" keys or bits of metadata with the kernel.
We'd reserve some number of keys for hardware metadata.

Quoted 5 lines

Right, but how do you allocate space/offset for each key without an explicit allocation step? You'd basically have to encode the list of IDs in the metadata area itself, which implies a TLV format that you have to walk on every access? The registry idea in my example above was basically to avoid that...

Quoted 6 lines

I've been playing around with having a small fixed header at the front of the metadata itself, that lets you access values without walking them all.

Still WIP, and maybe this is too restrictive, but it lets you encode 64 2, 4, or 8 byte KVs with a single 16 byte header:

Ohh, that's clever, I like it! :)

It's also extensible in the sense that the internal representation can change without impacting the API, so if we end up needing more bits we can always add those.

Maybe it would be a good idea to make the 'key' parameter a larger integer type (function parameters are always 64-bit anyway, so might as well go all the way up to u64)? That way we can use higher values for the kernel-reserved types instead of reserving part of the already-small key space for applications (assuming the kernel-internal data is stored somewhere else, like in a static struct as in Lorenzo's patch)?

[...]

Quoted 18 lines

Quoted 8 lines

Quoted 5 lines

The remaining keys would be up to users. They'd have to allocate keys to services, and configure services to use those keys.
This is similar to the way listening on a certain port works: only one service can use port 80 or 443, and that can typically beconfigured in a service's config file.

Right, well, port numbers *do* actually have an out of band service registry (IANA), which I thought was what we wanted to avoid? ;)

Depends how you think about it ;)

I think we should avoid a global registry. But having a registry per deployment / server doesn't seem awful. Services that want to use a field would have a config file setting to set which index it corresponds to.
Admins would just have to pick a free one on their system, and set it in the config file of the service.

Quoted 4 lines

This is similar to what we do for non-IANA registered ports internally. For example each service needs a port on an internal interface to expose metrics, and we just track which ports are taken in our config management.

Right, this would work, but it assumes that applications are well-behaved and do this correctly. Which they probably do in a centrally-managed system like yours, but for random applications shipped by distros, I'm not sure if it's going to work.

In fact, it's more or less the situation we have with skb->mark today, isn't it? I.e., applications can co-exist as long as they don't use the same bits, so they have to coordinate on which bits to use. Sure, with this scheme there will be more total bits to use, which can lessen the pressure somewhat, but the basic problem remains. In other words, I worry that in practice we will end up with another github repository serving as a registry for metadata keys...

Quoted 7 lines

Dynamically registering fields means you have to share the returned ID with whoever is interested, which sounds tricky.
If an XDP program sets a field like packet_id, every tracing program that looks at it, and userspace service, would need to know what the ID of that field is.
Is there a way to easily share that ID with all of them?

Right, so I'll admit this was one of the handwavy bits of my original proposal, but I don't think it's unsolvable. You could do something like (once, on application initialisation):

__u64 my_key = bpf_register_metadata_field(my_size); /* maybe add a name for introspection? */ bpf_map_update(&shared_application_config, &my_key_index, &my_key);

and then just get the key out of that map from all programs that want to use it?

We could combine such a registration API with your header format, so that the registration just becomes a way of allocating one of the keys from 0-63 (and the registry just becomes a global copy of the header). This would basically amount to moving the "service config file" into the kernel, since that seems to be the only common denominator we can rely on between BPF applications (as all attempts to write a common daemon for BPF management have shown).

-Toke

Quoted 45 lines · Toke Høiland-Jørgensen

Quoted 30 lines · Arthur Fabre

Quoted 24 lines

Quoted 15 lines

Quoted 9 lines

The nice thing about an API like this, though, is that it's extensible, and the kernel itself can be just another consumer of it for the metadata fields Lorenzo is adding in this series. I.e., we could just pre-define some IDs for metadata vlan, timestamp etc, and use the same functions as above from within the kernel to set and get those values; using the registry, there could even be an option to turn those off if an application wants more space for its own usage. Or, alternatively, we could keep the kernel-internal IDs hardcoded and always allocated, and just use the getter/setter functions as the BPF API for accessing them.

That's exactly what I'm thinking of too, a simple API like:

get(u8 key, u8 len, void *val);
set(u8 key, u8 len, void *val);

Quoted 8 lines

With "well-known" keys like METADATA_ID_HW_HASH for hardware metadata.

If a NIC doesn't support a certain well-known metadata, the key wouldn't be set, and get() would return ENOENT.

I think this also lets us avoid having to "register" keys or bits of metadata with the kernel.
We'd reserve some number of keys for hardware metadata.

Quoted 5 lines

Right, but how do you allocate space/offset for each key without an explicit allocation step? You'd basically have to encode the list of IDs in the metadata area itself, which implies a TLV format that you have to walk on every access? The registry idea in my example above was basically to avoid that...

Quoted 6 lines

I've been playing around with having a small fixed header at the front of the metadata itself, that lets you access values without walking them all.

Still WIP, and maybe this is too restrictive, but it lets you encode 64 2, 4, or 8 byte KVs with a single 16 byte header:

Ohh, that's clever, I like it! :)

It's also extensible in the sense that the internal representation can change without impacting the API, so if we end up needing more bits we can always add those.

Quoted 6 lines

Maybe it would be a good idea to make the 'key' parameter a larger integer type (function parameters are always 64-bit anyway, so might as well go all the way up to u64)? That way we can use higher values for the kernel-reserved types instead of reserving part of the already-small key space for applications (assuming the kernel-internal data is stored somewhere else, like in a static struct as in Lorenzo's patch)?

Good idea! That makes it more extensible too if we ever support more keys or bigger lengths.

I'm not sure where the kernel-reserved types should live. Putting them in here uses up some the of KV IDs, but it uses less head room space than always reserving a static struct for them.
Maybe it doesn't matter much, as long as we use the same API to access them, we could internally switch between one and the other.

Quoted 31 lines

[...]

Quoted 18 lines

Quoted 8 lines

Quoted 5 lines

The remaining keys would be up to users. They'd have to allocate keys to services, and configure services to use those keys.
This is similar to the way listening on a certain port works: only one service can use port 80 or 443, and that can typically beconfigured in a service's config file.

Right, well, port numbers *do* actually have an out of band service registry (IANA), which I thought was what we wanted to avoid? ;)

Depends how you think about it ;)

I think we should avoid a global registry. But having a registry per deployment / server doesn't seem awful. Services that want to use a field would have a config file setting to set which index it corresponds to.
Admins would just have to pick a free one on their system, and set it in the config file of the service.

Quoted 4 lines

This is similar to what we do for non-IANA registered ports internally. For example each service needs a port on an internal interface to expose metrics, and we just track which ports are taken in our config management.

Right, this would work, but it assumes that applications are well-behaved and do this correctly. Which they probably do in a centrally-managed system like yours, but for random applications shipped by distros, I'm not sure if it's going to work.

Quoted 7 lines

In fact, it's more or less the situation we have with skb->mark today, isn't it? I.e., applications can co-exist as long as they don't use the same bits, so they have to coordinate on which bits to use. Sure, with this scheme there will be more total bits to use, which can lessen the pressure somewhat, but the basic problem remains. In other words, I worry that in practice we will end up with another github repository serving as a registry for metadata keys...

That's true. If applications hardcode keys, we'll be back to having conflicts. If someone creates a registry on github I'll be very sad.

(Maybe we can make the verifier enforce that the key passed to get() and set() isn't a constant? - only joking)

Applications don't tend to do this for ports though, I think most can be configured to listen on any port. Is that just because it's been instilled as "good practice" over time? Could we try to do the same with some very stern documentation and examples?

Thinking about it more, my only relectance for a registration API is how to communicate the ID back to other consumers (our discussion below).

Quoted 12 lines

Quoted 7 lines

Dynamically registering fields means you have to share the returned ID with whoever is interested, which sounds tricky.
If an XDP program sets a field like packet_id, every tracing program that looks at it, and userspace service, would need to know what the ID of that field is.
Is there a way to easily share that ID with all of them?

Right, so I'll admit this was one of the handwavy bits of my original proposal, but I don't think it's unsolvable. You could do something like (once, on application initialisation):

Quoted 5 lines

__u64 my_key = bpf_register_metadata_field(my_size); /* maybe add a name for introspection? */ bpf_map_update(&shared_application_config, &my_key_index, &my_key);

and then just get the key out of that map from all programs that want to use it?

Passing it out of band works (whether it's via a pinned map like you described, or through other means like a unix socket or some other API), it's just more complicated.

Every consumer also needs to know about that API. That won't work with standard tools. For example if we set a PACKET_ID KV, maybe we could give it to pwru so it could track packets using it?
Without registering keys, we could pass it as a cli flag. With registration, we'd have to have some helper to get the KV ID.

It also introduces ordering dependencies between the services on startup, eg packet tracing hooks could only be attached once our XDP service has registered a PACKET_ID KV, and they could query it's API.

Quoted 8 lines

We could combine such a registration API with your header format, so that the registration just becomes a way of allocating one of the keys from 0-63 (and the registry just becomes a global copy of the header). This would basically amount to moving the "service config file" into the kernel, since that seems to be the only common denominator we can rely on between BPF applications (as all attempts to write a common daemon for BPF management have shown).

That sounds reasonable. And I guess we'd have set() check the global registry to enforce that the key has been registered beforehand?

Quoted 2 lines

-Toke

Thanks for all the feedback!

Quoted 12 lines

Quoted 7 lines

We could combine such a registration API with your header format, so that the registration just becomes a way of allocating one of the keys from 0-63 (and the registry just becomes a global copy of the header). This would basically amount to moving the "service config file" into the kernel, since that seems to be the only common denominator we can rely on between BPF applications (as all attempts to write a common daemon for BPF management have shown).

That sounds reasonable. And I guess we'd have set() check the global registry to enforce that the key has been registered beforehand?

Quoted 1 line

Quoted 3 lines

Quoted 1 line

-Toke

Thanks for all the feedback!

I like this 'fast' KV approach but I guess we should really evaluate its impact on performances (especially for xdp) since, based on the kfunc calls order in the ebpf program, we can have one or multiple memmove/memcpy for each packet, right?

Moreover, I still think the metadata area in the xdp_frame/xdp_buff is not so suitable for nic hw metadata since:
- it grows backward
- it is probably in a different cacheline with respect to xdp_frame
- nic hw metadata will not start at fixed and immutable address, but it depends on the running ebpf program

What about having something like:
- fixed hw nic metadata: just after xdp_frame struct (or if you want at the end of the metadata area :)). Here he can reuse the same KV approach if it is fast
- user defined metadata: in the metadata area of the xdp_frame/xdp_buff

Quoted 12 lines · Lorenzo Bianconi

Quoted 12 lines

Quoted 7 lines

We could combine such a registration API with your header format, so that the registration just becomes a way of allocating one of the keys from 0-63 (and the registry just becomes a global copy of the header). This would basically amount to moving the "service config file" into the kernel, since that seems to be the only common denominator we can rely on between BPF applications (as all attempts to write a common daemon for BPF management have shown).

That sounds reasonable. And I guess we'd have set() check the global registry to enforce that the key has been registered beforehand?

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

-Toke

Thanks for all the feedback!

I like this 'fast' KV approach but I guess we should really evaluate its impact on performances (especially for xdp) since, based on the kfunc calls order in the ebpf program, we can have one or multiple memmove/memcpy for each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using a global registry for offsets would sidestep this, but have the synchronisation issues we discussed up-thread. So on balance, I think the memmove() suggestion will probably lead to the least pain.

For the HW metadata we could sidestep this by always having a fixed struct for it (but using the same set/get() API with reserved keys). The only drawback of doing that is that we statically reserve a bit of space, but I'm not sure that is such a big issue in practice (at least not until this becomes to popular that the space starts to be contended; but surely 256 bytes ought to be enough for everybody, right? :)).

Quoted 8 lines

Moreover, I still think the metadata area in the xdp_frame/xdp_buff is not so suitable for nic hw metadata since:
- it grows backward
- it is probably in a different cacheline with respect to xdp_frame
- nic hw metadata will not start at fixed and immutable address, but it depends on the running ebpf program

Quoted 4 lines

What about having something like:
- fixed hw nic metadata: just after xdp_frame struct (or if you want at the end of the metadata area :)). Here he can reuse the same KV approach if it is fast
- user defined metadata: in the metadata area of the xdp_frame/xdp_buff

AFAIU, none of this will live in the (current) XDP metadata area. It will all live just after the xdp_frame struct (so sharing the space with the metadata area in the sense that adding more metadata kv fields will decrease the amount of space that is usable by the current XDP metadata APIs).

-Toke

Quoted 28 lines

Quoted 12 lines · Lorenzo Bianconi

Quoted 12 lines

Quoted 7 lines

We could combine such a registration API with your header format, so that the registration just becomes a way of allocating one of the keys from 0-63 (and the registry just becomes a global copy of the header). This would basically amount to moving the "service config file" into the kernel, since that seems to be the only common denominator we can rely on between BPF applications (as all attempts to write a common daemon for BPF management have shown).

That sounds reasonable. And I guess we'd have set() check the global registry to enforce that the key has been registered beforehand?

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

-Toke

Thanks for all the feedback!

I like this 'fast' KV approach but I guess we should really evaluate its impact on performances (especially for xdp) since, based on the kfunc calls order in the ebpf program, we can have one or multiple memmove/memcpy for each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using a global registry for offsets would sidestep this, but have the synchronisation issues we discussed up-thread. So on balance, I think the memmove() suggestion will probably lead to the least pain.

Quoted 6 lines

For the HW metadata we could sidestep this by always having a fixed struct for it (but using the same set/get() API with reserved keys). The only drawback of doing that is that we statically reserve a bit of space, but I'm not sure that is such a big issue in practice (at least not until this becomes to popular that the space starts to be contended; but surely 256 bytes ought to be enough for everybody, right? :)).

I am fine with the proposed approach, but I think we need to verify what is the impact on performances (in the worst case??)

Quoted 18 lines

Quoted 11 lines

Moreover, I still think the metadata area in the xdp_frame/xdp_buff is not so suitable for nic hw metadata since:
- it grows backward
- it is probably in a different cacheline with respect to xdp_frame
- nic hw metadata will not start at fixed and immutable address, but it depends on the running ebpf program

What about having something like:
- fixed hw nic metadata: just after xdp_frame struct (or if you want at the end of the metadata area :)). Here he can reuse the same KV approach if it is fast
- user defined metadata: in the metadata area of the xdp_frame/xdp_buff

AFAIU, none of this will live in the (current) XDP metadata area. It will all live just after the xdp_frame struct (so sharing the space with the metadata area in the sense that adding more metadata kv fields will decrease the amount of space that is usable by the current XDP metadata APIs).

Quoted 2 lines

-Toke

ah, ok. I was thinking the proposed approach was to put them in the current metadata field.

Regards,
Lorenzo

Quoted 35 lines

Quoted 34 lines

Quoted 12 lines · Lorenzo Bianconi

Quoted 12 lines

Quoted 7 lines

We could combine such a registration API with your header format, so that the registration just becomes a way of allocating one of the keys from 0-63 (and the registry just becomes a global copy of the header). This would basically amount to moving the "service config file" into the kernel, since that seems to be the only common denominator we can rely on between BPF applications (as all attempts to write a common daemon for BPF management have shown).

That sounds reasonable. And I guess we'd have set() check the global registry to enforce that the key has been registered beforehand?

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

-Toke

Thanks for all the feedback!

I like this 'fast' KV approach but I guess we should really evaluate its impact on performances (especially for xdp) since, based on the kfunc calls order in the ebpf program, we can have one or multiple memmove/memcpy for each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using a global registry for offsets would sidestep this, but have the synchronisation issues we discussed up-thread. So on balance, I think the memmove() suggestion will probably lead to the least pain.

For the HW metadata we could sidestep this by always having a fixed struct for it (but using the same set/get() API with reserved keys). The only drawback of doing that is that we statically reserve a bit of space, but I'm not sure that is such a big issue in practice (at least not until this becomes to popular that the space starts to be contended; but surely 256 bytes ought to be enough for everybody, right? :)).

Quoted 2 lines

I am fine with the proposed approach, but I think we need to verify what is the impact on performances (in the worst case??)

If drivers are responsible for populating the hardware metadata before XDP, we could make sure drivers set the fields in order to avoid any memove() (and maybe even provide a helper to ensure this?).

Quoted 19 lines

Quoted 19 lines

Quoted 8 lines

Moreover, I still think the metadata area in the xdp_frame/xdp_buff is not so suitable for nic hw metadata since:
- it grows backward
- it is probably in a different cacheline with respect to xdp_frame
- nic hw metadata will not start at fixed and immutable address, but it depends on the running ebpf program

Quoted 4 lines

What about having something like:
- fixed hw nic metadata: just after xdp_frame struct (or if you want at the end of the metadata area :)). Here he can reuse the same KV approach if it is fast
- user defined metadata: in the metadata area of the xdp_frame/xdp_buff

AFAIU, none of this will live in the (current) XDP metadata area. It will all live just after the xdp_frame struct (so sharing the space with the metadata area in the sense that adding more metadata kv fields will decrease the amount of space that is usable by the current XDP metadata APIs).

Quoted 5 lines

Quoted 2 lines

-Toke

ah, ok. I was thinking the proposed approach was to put them in the current metadata field.

I've also been thinking of putting this new KV stuff at the start of the headroom (I think that's what you're saying Toke?). It has a few nice advantanges:

* It coexists nicely with the current XDP / TC metadata support. Those users won't be able to overwrite / corrupt the KV metadata. KV users won't need to call xdp_adjust_meta() (which would be awkward - how would they know how much space the KV implementation needs).

* We don't have to move all the metadata everytime we call xdp_adjust_head() (or the kernel equivalent).

Are there any performance implications of that, e.g. for caching?

This would also grow "upwards" which is more natural, but I think either way the KV API would hide whether it's downwards or upwards from users.

Quoted 3 lines

Regards,
Lorenzo

Quoted 36 lines · Lorenzo Bianconi

Quoted 35 lines

Quoted 34 lines

Quoted 12 lines · Lorenzo Bianconi

Quoted 12 lines

Quoted 7 lines

We could combine such a registration API with your header format, so that the registration just becomes a way of allocating one of the keys from 0-63 (and the registry just becomes a global copy of the header). This would basically amount to moving the "service config file" into the kernel, since that seems to be the only common denominator we can rely on between BPF applications (as all attempts to write a common daemon for BPF management have shown).

That sounds reasonable. And I guess we'd have set() check the global registry to enforce that the key has been registered beforehand?

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

-Toke

Thanks for all the feedback!

I like this 'fast' KV approach but I guess we should really evaluate its impact on performances (especially for xdp) since, based on the kfunc calls order in the ebpf program, we can have one or multiple memmove/memcpy for each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using a global registry for offsets would sidestep this, but have the synchronisation issues we discussed up-thread. So on balance, I think the memmove() suggestion will probably lead to the least pain.

For the HW metadata we could sidestep this by always having a fixed struct for it (but using the same set/get() API with reserved keys). The only drawback of doing that is that we statically reserve a bit of space, but I'm not sure that is such a big issue in practice (at least not until this becomes to popular that the space starts to be contended; but surely 256 bytes ought to be enough for everybody, right? :)).

Quoted 6 lines

Quoted 2 lines

I am fine with the proposed approach, but I think we need to verify what is the impact on performances (in the worst case??)

If drivers are responsible for populating the hardware metadata before XDP, we could make sure drivers set the fields in order to avoid any memove() (and maybe even provide a helper to ensure this?).

nope, since the current APIs introduced by Stanislav are consuming NIC metadata in kfuncs (mainly for af_xdp) and, according to my understanding, we want to add a kfunc to store the info for each NIC metadata (e.g rx-hash, timestamping, ..) into the packet (this is what Toke is proposing, right?). In this case kfunc calling order makes a difference.
We can think even to add single kfunc to store all the info for all the NIC metadata (maybe via a helping struct) but it seems not scalable to me and we are losing kfunc versatility.

Quoted 24 lines

Quoted 23 lines

Quoted 12 lines

Quoted 11 lines

Moreover, I still think the metadata area in the xdp_frame/xdp_buff is not so suitable for nic hw metadata since:
- it grows backward
- it is probably in a different cacheline with respect to xdp_frame
- nic hw metadata will not start at fixed and immutable address, but it depends on the running ebpf program

What about having something like:
- fixed hw nic metadata: just after xdp_frame struct (or if you want at the end of the metadata area :)). Here he can reuse the same KV approach if it is fast
- user defined metadata: in the metadata area of the xdp_frame/xdp_buff

Quoted 8 lines

AFAIU, none of this will live in the (current) XDP metadata area. It will all live just after the xdp_frame struct (so sharing the space with the metadata area in the sense that adding more metadata kv fields will decrease the amount of space that is usable by the current XDP metadata APIs).

-Toke

ah, ok. I was thinking the proposed approach was to put them in the current metadata field.

Quoted 8 lines

I've also been thinking of putting this new KV stuff at the start of the headroom (I think that's what you're saying Toke?). It has a few nice advantanges:

* It coexists nicely with the current XDP / TC metadata support. Those users won't be able to overwrite / corrupt the KV metadata. KV users won't need to call xdp_adjust_meta() (which would be awkward - how would they know how much space the KV implementation needs).

Quoted 4 lines

* We don't have to move all the metadata everytime we call xdp_adjust_head() (or the kernel equivalent).

Are there any performance implications of that, e.g. for caching?

Quoted 43 lines · Lorenzo Bianconi

Quoted 36 lines · Lorenzo Bianconi

Quoted 35 lines

Quoted 34 lines

Quoted 12 lines · Lorenzo Bianconi

Quoted 12 lines

Quoted 7 lines

We could combine such a registration API with your header format, so that the registration just becomes a way of allocating one of the keys from 0-63 (and the registry just becomes a global copy of the header). This would basically amount to moving the "service config file" into the kernel, since that seems to be the only common denominator we can rely on between BPF applications (as all attempts to write a common daemon for BPF management have shown).

That sounds reasonable. And I guess we'd have set() check the global registry to enforce that the key has been registered beforehand?

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

-Toke

Thanks for all the feedback!

I like this 'fast' KV approach but I guess we should really evaluate its impact on performances (especially for xdp) since, based on the kfunc calls order in the ebpf program, we can have one or multiple memmove/memcpy for each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using a global registry for offsets would sidestep this, but have the synchronisation issues we discussed up-thread. So on balance, I think the memmove() suggestion will probably lead to the least pain.

For the HW metadata we could sidestep this by always having a fixed struct for it (but using the same set/get() API with reserved keys). The only drawback of doing that is that we statically reserve a bit of space, but I'm not sure that is such a big issue in practice (at least not until this becomes to popular that the space starts to be contended; but surely 256 bytes ought to be enough for everybody, right? :)).

Quoted 6 lines

Quoted 2 lines

I am fine with the proposed approach, but I think we need to verify what is the impact on performances (in the worst case??)

If drivers are responsible for populating the hardware metadata before XDP, we could make sure drivers set the fields in order to avoid any memove() (and maybe even provide a helper to ensure this?).

Quoted 8 lines

nope, since the current APIs introduced by Stanislav are consuming NIC metadata in kfuncs (mainly for af_xdp) and, according to my understanding, we want to add a kfunc to store the info for each NIC metadata (e.g rx-hash, timestamping, ..) into the packet (this is what Toke is proposing, right?). In this case kfunc calling order makes a difference.
We can think even to add single kfunc to store all the info for all the NIC metadata (maybe via a helping struct) but it seems not scalable to me and we are losing kfunc versatility.

Yes, I agree we should have separate kfuncs for each metadata field. Which means it makes a lot of sense to just use the same setter API that we use for the user-registered metadata fields, but using reserved keys. So something like:

#define BPF_METADATA_HW_HASH BIT(60)
#define BPF_METADATA_HW_TIMESTAMP BIT(61)
#define BPF_METADATA_HW_VLAN BIT(62)
#define BPF_METADATA_RESERVED (0xffff << 48)

bpf_packet_metadata_set(pkt, BPF_METADATA_HW_HASH, hash_value);

As for the internal representation, we can just have the kfunc do something like:

int bpf_packet_metadata_set(field_id, value) {

  switch(field_id) {
    case BPF_METADATA_HW_HASH:
      pkt->xdp_hw_meta.hash = value;
      break;
    [...]
    default:
      /* do the key packing thing */

}
}

that way the order of setting the HW fields doesn't matter, only the user-defined metadata.

Quoted 82 lines · Toke Høiland-Jørgensen

Quoted 43 lines · Lorenzo Bianconi

Quoted 36 lines · Lorenzo Bianconi

Quoted 35 lines

Quoted 34 lines

Quoted 12 lines · Lorenzo Bianconi

Quoted 12 lines

Quoted 7 lines

We could combine such a registration API with your header format, so that the registration just becomes a way of allocating one of the keys from 0-63 (and the registry just becomes a global copy of the header). This would basically amount to moving the "service config file" into the kernel, since that seems to be the only common denominator we can rely on between BPF applications (as all attempts to write a common daemon for BPF management have shown).

That sounds reasonable. And I guess we'd have set() check the global registry to enforce that the key has been registered beforehand?

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

-Toke

Thanks for all the feedback!

I like this 'fast' KV approach but I guess we should really evaluate its impact on performances (especially for xdp) since, based on the kfunc calls order in the ebpf program, we can have one or multiple memmove/memcpy for each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using a global registry for offsets would sidestep this, but have the synchronisation issues we discussed up-thread. So on balance, I think the memmove() suggestion will probably lead to the least pain.

For the HW metadata we could sidestep this by always having a fixed struct for it (but using the same set/get() API with reserved keys). The only drawback of doing that is that we statically reserve a bit of space, but I'm not sure that is such a big issue in practice (at least not until this becomes to popular that the space starts to be contended; but surely 256 bytes ought to be enough for everybody, right? :)).

Quoted 6 lines

Quoted 2 lines

I am fine with the proposed approach, but I think we need to verify what is the impact on performances (in the worst case??)

If drivers are responsible for populating the hardware metadata before XDP, we could make sure drivers set the fields in order to avoid any memove() (and maybe even provide a helper to ensure this?).

Quoted 8 lines

nope, since the current APIs introduced by Stanislav are consuming NIC metadata in kfuncs (mainly for af_xdp) and, according to my understanding, we want to add a kfunc to store the info for each NIC metadata (e.g rx-hash, timestamping, ..) into the packet (this is what Toke is proposing, right?). In this case kfunc calling order makes a difference.
We can think even to add single kfunc to store all the info for all the NIC metadata (maybe via a helping struct) but it seems not scalable to me and we are losing kfunc versatility.

Yes, I agree we should have separate kfuncs for each metadata field. Which means it makes a lot of sense to just use the same setter API that we use for the user-registered metadata fields, but using reserved keys. So something like:

#define BPF_METADATA_HW_HASH BIT(60)
#define BPF_METADATA_HW_TIMESTAMP BIT(61)
#define BPF_METADATA_HW_VLAN BIT(62)
#define BPF_METADATA_RESERVED (0xffff << 48)

bpf_packet_metadata_set(pkt, BPF_METADATA_HW_HASH, hash_value);

As for the internal representation, we can just have the kfunc do something like:

int bpf_packet_metadata_set(field_id, value) {

  switch(field_id) {
    case BPF_METADATA_HW_HASH:
      pkt->xdp_hw_meta.hash = value;
      break;
    [...]
    default:
      /* do the key packing thing */

}
}

Quoted 2 lines

that way the order of setting the HW fields doesn't matter, only the user-defined metadata.

Can you expand on why we need the flexibility of picking the metadata fields here? Presumably we are talking about the use-cases where the XDP program is doing redirect/pass and it doesn't really know who's the final consumer is (might be another xdp program or might be the xdp->skb kernel case), so the only sensible option here seems to be store everything?

Quoted 83 lines · Stanislav Fomichev

Quoted 82 lines · Toke Høiland-Jørgensen

Quoted 43 lines · Lorenzo Bianconi

Quoted 36 lines · Lorenzo Bianconi

Quoted 35 lines

Quoted 34 lines

Quoted 12 lines · Lorenzo Bianconi

Quoted 12 lines

Quoted 7 lines

We could combine such a registration API with your header format, so that the registration just becomes a way of allocating one of the keys from 0-63 (and the registry just becomes a global copy of the header). This would basically amount to moving the "service config file" into the kernel, since that seems to be the only common denominator we can rely on between BPF applications (as all attempts to write a common daemon for BPF management have shown).

That sounds reasonable. And I guess we'd have set() check the global registry to enforce that the key has been registered beforehand?

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

-Toke

Thanks for all the feedback!

I like this 'fast' KV approach but I guess we should really evaluate its impact on performances (especially for xdp) since, based on the kfunc calls order in the ebpf program, we can have one or multiple memmove/memcpy for each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using a global registry for offsets would sidestep this, but have the synchronisation issues we discussed up-thread. So on balance, I think the memmove() suggestion will probably lead to the least pain.

For the HW metadata we could sidestep this by always having a fixed struct for it (but using the same set/get() API with reserved keys). The only drawback of doing that is that we statically reserve a bit of space, but I'm not sure that is such a big issue in practice (at least not until this becomes to popular that the space starts to be contended; but surely 256 bytes ought to be enough for everybody, right? :)).

Quoted 6 lines

Quoted 2 lines

I am fine with the proposed approach, but I think we need to verify what is the impact on performances (in the worst case??)

If drivers are responsible for populating the hardware metadata before XDP, we could make sure drivers set the fields in order to avoid any memove() (and maybe even provide a helper to ensure this?).

Quoted 8 lines

nope, since the current APIs introduced by Stanislav are consuming NIC metadata in kfuncs (mainly for af_xdp) and, according to my understanding, we want to add a kfunc to store the info for each NIC metadata (e.g rx-hash, timestamping, ..) into the packet (this is what Toke is proposing, right?). In this case kfunc calling order makes a difference.
We can think even to add single kfunc to store all the info for all the NIC metadata (maybe via a helping struct) but it seems not scalable to me and we are losing kfunc versatility.

Yes, I agree we should have separate kfuncs for each metadata field. Which means it makes a lot of sense to just use the same setter API that we use for the user-registered metadata fields, but using reserved keys. So something like:

#define BPF_METADATA_HW_HASH BIT(60)
#define BPF_METADATA_HW_TIMESTAMP BIT(61)
#define BPF_METADATA_HW_VLAN BIT(62)
#define BPF_METADATA_RESERVED (0xffff << 48)

bpf_packet_metadata_set(pkt, BPF_METADATA_HW_HASH, hash_value);

As for the internal representation, we can just have the kfunc do something like:

int bpf_packet_metadata_set(field_id, value) {

  switch(field_id) {
    case BPF_METADATA_HW_HASH:
      pkt->xdp_hw_meta.hash = value;
      break;
    [...]
    default:
      /* do the key packing thing */

}
}

Quoted 8 lines

Quoted 2 lines

that way the order of setting the HW fields doesn't matter, only the user-defined metadata.

Can you expand on why we need the flexibility of picking the metadata fields here? Presumably we are talking about the use-cases where the XDP program is doing redirect/pass and it doesn't really know who's the final consumer is (might be another xdp program or might be the xdp->skb kernel case), so the only sensible option here seems to be store everything?

For the same reason that we have separate kfuncs for each metadata field when getting it from the driver: XDP programs should have the flexibility to decide which pieces of metadata they need, and skip the overhead of stuff that is not needed.

For instance, say an XDP program knows that nothing in the system uses timestamps; in that case, it can skip both the getter and the setter call for timestamps.

I suppose we *could* support just a single call to set the skb meta, like:

bpf_set_skb_meta(struct xdp_md *pkt, struct xdp_hw_meta *data);

...but in that case, we'd need to support some fields being unset anyway, and the program would have to populate the struct on the stack before performing the call. So it seems simpler to just have symmetry between the get (from HW) and set side? :)

Quoted 94 lines · Toke Høiland-Jørgensen

Quoted 83 lines · Stanislav Fomichev

Quoted 82 lines · Toke Høiland-Jørgensen

Quoted 43 lines · Lorenzo Bianconi

Quoted 36 lines · Lorenzo Bianconi

Quoted 35 lines

Quoted 34 lines

Quoted 12 lines · Lorenzo Bianconi

Quoted 12 lines

Quoted 7 lines

We could combine such a registration API with your header format, so that the registration just becomes a way of allocating one of the keys from 0-63 (and the registry just becomes a global copy of the header). This would basically amount to moving the "service config file" into the kernel, since that seems to be the only common denominator we can rely on between BPF applications (as all attempts to write a common daemon for BPF management have shown).

That sounds reasonable. And I guess we'd have set() check the global registry to enforce that the key has been registered beforehand?

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

-Toke

Thanks for all the feedback!

I like this 'fast' KV approach but I guess we should really evaluate its impact on performances (especially for xdp) since, based on the kfunc calls order in the ebpf program, we can have one or multiple memmove/memcpy for each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using a global registry for offsets would sidestep this, but have the synchronisation issues we discussed up-thread. So on balance, I think the memmove() suggestion will probably lead to the least pain.

For the HW metadata we could sidestep this by always having a fixed struct for it (but using the same set/get() API with reserved keys). The only drawback of doing that is that we statically reserve a bit of space, but I'm not sure that is such a big issue in practice (at least not until this becomes to popular that the space starts to be contended; but surely 256 bytes ought to be enough for everybody, right? :)).

Quoted 6 lines

Quoted 2 lines

I am fine with the proposed approach, but I think we need to verify what is the impact on performances (in the worst case??)

If drivers are responsible for populating the hardware metadata before XDP, we could make sure drivers set the fields in order to avoid any memove() (and maybe even provide a helper to ensure this?).

Quoted 8 lines

nope, since the current APIs introduced by Stanislav are consuming NIC metadata in kfuncs (mainly for af_xdp) and, according to my understanding, we want to add a kfunc to store the info for each NIC metadata (e.g rx-hash, timestamping, ..) into the packet (this is what Toke is proposing, right?). In this case kfunc calling order makes a difference.
We can think even to add single kfunc to store all the info for all the NIC metadata (maybe via a helping struct) but it seems not scalable to me and we are losing kfunc versatility.

Yes, I agree we should have separate kfuncs for each metadata field. Which means it makes a lot of sense to just use the same setter API that we use for the user-registered metadata fields, but using reserved keys. So something like:

#define BPF_METADATA_HW_HASH BIT(60)
#define BPF_METADATA_HW_TIMESTAMP BIT(61)
#define BPF_METADATA_HW_VLAN BIT(62)
#define BPF_METADATA_RESERVED (0xffff << 48)

bpf_packet_metadata_set(pkt, BPF_METADATA_HW_HASH, hash_value);

As for the internal representation, we can just have the kfunc do something like:

int bpf_packet_metadata_set(field_id, value) {

  switch(field_id) {
    case BPF_METADATA_HW_HASH:
      pkt->xdp_hw_meta.hash = value;
      break;
    [...]
    default:
      /* do the key packing thing */

}
}

Quoted 8 lines

Quoted 2 lines

that way the order of setting the HW fields doesn't matter, only the user-defined metadata.

Can you expand on why we need the flexibility of picking the metadata fields here? Presumably we are talking about the use-cases where the XDP program is doing redirect/pass and it doesn't really know who's the final consumer is (might be another xdp program or might be the xdp->skb kernel case), so the only sensible option here seems to be store everything?

Quoted 8 lines

For the same reason that we have separate kfuncs for each metadata field when getting it from the driver: XDP programs should have the flexibility to decide which pieces of metadata they need, and skip the overhead of stuff that is not needed.

For instance, say an XDP program knows that nothing in the system uses timestamps; in that case, it can skip both the getter and the setter call for timestamps.

But doesn't it put us in the same place? Where the first (native) xdp program needs to know which metadata the final consumer wants. At this point why not propagate metadata layout as well?

(or maybe I'm still missing what exact use-case we are trying to solve)

Quoted 6 lines

I suppose we *could* support just a single call to set the skb meta, like:

bpf_set_skb_meta(struct xdp_md *pkt, struct xdp_hw_meta *data);

Quoted 4 lines

...but in that case, we'd need to support some fields being unset anyway, and the program would have to populate the struct on the stack before performing the call. So it seems simpler to just have symmetry between the get (from HW) and set side? :)

Why not simply bpf_set_skb_meta(struct xdp_md *pkt) and let it store the metadata somewhere in xdp_md directly? (also presumably by reusing most of the existing kfuncs/xmo_xxx helpers)

Quoted 95 lines · Stanislav Fomichev

Quoted 94 lines · Toke Høiland-Jørgensen

Quoted 91 lines · Stanislav Fomichev

Quoted 78 lines · Toke Høiland-Jørgensen

Quoted 43 lines · Lorenzo Bianconi

Quoted 36 lines · Lorenzo Bianconi

Quoted 35 lines

Quoted 34 lines

Quoted 12 lines · Lorenzo Bianconi

Quoted 12 lines

Quoted 7 lines

We could combine such a registration API with your header format, so that the registration just becomes a way of allocating one of the keys from 0-63 (and the registry just becomes a global copy of the header). This would basically amount to moving the "service config file" into the kernel, since that seems to be the only common denominator we can rely on between BPF applications (as all attempts to write a common daemon for BPF management have shown).

That sounds reasonable. And I guess we'd have set() check the global registry to enforce that the key has been registered beforehand?

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

-Toke

Thanks for all the feedback!

I like this 'fast' KV approach but I guess we should really evaluate its impact on performances (especially for xdp) since, based on the kfunc calls order in the ebpf program, we can have one or multiple memmove/memcpy for each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using a global registry for offsets would sidestep this, but have the synchronisation issues we discussed up-thread. So on balance, I think the memmove() suggestion will probably lead to the least pain.

For the HW metadata we could sidestep this by always having a fixed struct for it (but using the same set/get() API with reserved keys). The only drawback of doing that is that we statically reserve a bit of space, but I'm not sure that is such a big issue in practice (at least not until this becomes to popular that the space starts to be contended; but surely 256 bytes ought to be enough for everybody, right? :)).

Quoted 6 lines

Quoted 2 lines

I am fine with the proposed approach, but I think we need to verify what is the impact on performances (in the worst case??)

If drivers are responsible for populating the hardware metadata before XDP, we could make sure drivers set the fields in order to avoid any memove() (and maybe even provide a helper to ensure this?).

Quoted 8 lines

nope, since the current APIs introduced by Stanislav are consuming NIC metadata in kfuncs (mainly for af_xdp) and, according to my understanding, we want to add a kfunc to store the info for each NIC metadata (e.g rx-hash, timestamping, ..) into the packet (this is what Toke is proposing, right?). In this case kfunc calling order makes a difference.
We can think even to add single kfunc to store all the info for all the NIC metadata (maybe via a helping struct) but it seems not scalable to me and we are losing kfunc versatility.

Yes, I agree we should have separate kfuncs for each metadata field. Which means it makes a lot of sense to just use the same setter API that we use for the user-registered metadata fields, but using reserved keys. So something like:

#define BPF_METADATA_HW_HASH BIT(60)
#define BPF_METADATA_HW_TIMESTAMP BIT(61)
#define BPF_METADATA_HW_VLAN BIT(62)
#define BPF_METADATA_RESERVED (0xffff << 48)

bpf_packet_metadata_set(pkt, BPF_METADATA_HW_HASH, hash_value);

As for the internal representation, we can just have the kfunc do something like:

int bpf_packet_metadata_set(field_id, value) {

  switch(field_id) {
    case BPF_METADATA_HW_HASH:
      pkt->xdp_hw_meta.hash = value;
      break;
    [...]
    default:
      /* do the key packing thing */

Quoted 6 lines

}
}

that way the order of setting the HW fields doesn't matter, only the user-defined metadata.

Can you expand on why we need the flexibility of picking the metadata fields here? Presumably we are talking about the use-cases where the XDP program is doing redirect/pass and it doesn't really know who's the final consumer is (might be another xdp program or might be the xdp->skb kernel case), so the only sensible option here seems to be store everything?

Quoted 8 lines

Quoted 8 lines

For the same reason that we have separate kfuncs for each metadata field when getting it from the driver: XDP programs should have the flexibility to decide which pieces of metadata they need, and skip the overhead of stuff that is not needed.

For instance, say an XDP program knows that nothing in the system uses timestamps; in that case, it can skip both the getter and the setter call for timestamps.

Quoted 5 lines

But doesn't it put us in the same place? Where the first (native) xdp program needs to know which metadata the final consumer wants. At this point why not propagate metadata layout as well?

(or maybe I'm still missing what exact use-case we are trying to solve)

There are two different use-cases for the metadata:

* "Hardware" metadata (like the hash, rx_timestamp...). There are only a few well known fields, and only XDP can access them to set them as metadata, so storing them in a struct somewhere could make sense.

* Arbitrary metadata used by services. Eg a TC filter could set a field describing which service a packet is for, and that could be reused for iptables, routing, socket dispatch...
Similarly we could set a "packet_id" field that uniquely identifies a packet so we can trace it throughout the network stack (through clones, encap, decap, userspace services...).
The skb->mark, but with more room, and better support for sharing it.

We can only know the layout ahead of time for the first one. And they're similar enough in their requirements (need to be stored somewhere in the SKB, have a way of retrieving each one individually, that it seems to make sense to use a common API).

Quoted 10 lines

Quoted 9 lines

I suppose we *could* support just a single call to set the skb meta, like:

bpf_set_skb_meta(struct xdp_md *pkt, struct xdp_hw_meta *data);

...but in that case, we'd need to support some fields being unset anyway, and the program would have to populate the struct on the stack before performing the call. So it seems simpler to just have symmetry between the get (from HW) and set side? :)

Quoted 3 lines

Why not simply bpf_set_skb_meta(struct xdp_md *pkt) and let it store the metadata somewhere in xdp_md directly? (also presumably by reusing most of the existing kfuncs/xmo_xxx helpers)

If we store it in xdp_md, the metadata won't be available higher up the stack (or am I missing something?). I think one of the goals is to let things other than XDP access it (maybe even the network stack itself?).

Quoted 96 lines · Arthur Fabre

Quoted 95 lines · Stanislav Fomichev

Quoted 94 lines · Toke Høiland-Jørgensen

Quoted 91 lines · Stanislav Fomichev

Quoted 78 lines · Toke Høiland-Jørgensen

Quoted 43 lines · Lorenzo Bianconi

Quoted 36 lines · Lorenzo Bianconi

Quoted 35 lines

Quoted 34 lines

Quoted 12 lines · Lorenzo Bianconi

Quoted 12 lines

Quoted 7 lines

We could combine such a registration API with your header format, so
that the registration just becomes a way of allocating one of the keys
from 0-63 (and the registry just becomes a global copy of the header).
This would basically amount to moving the "service config file" into the
kernel, since that seems to be the only common denominator we can rely
on between BPF applications (as all attempts to write a common daemon
for BPF management have shown).

That sounds reasonable. And I guess we'd have set() check the global registry to enforce that the key has been registered beforehand?

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

-Toke

Thanks for all the feedback!

I like this 'fast' KV approach but I guess we should really evaluate its impact on performances (especially for xdp) since, based on the kfunc calls order in the ebpf program, we can have one or multiple memmove/memcpy for each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using a global registry for offsets would sidestep this, but have the synchronisation issues we discussed up-thread. So on balance, I think the memmove() suggestion will probably lead to the least pain.

For the HW metadata we could sidestep this by always having a fixed struct for it (but using the same set/get() API with reserved keys). The only drawback of doing that is that we statically reserve a bit of space, but I'm not sure that is such a big issue in practice (at least not until this becomes to popular that the space starts to be contended; but surely 256 bytes ought to be enough for everybody, right? :)).

Quoted 6 lines

Quoted 2 lines

I am fine with the proposed approach, but I think we need to verify what is the impact on performances (in the worst case??)

If drivers are responsible for populating the hardware metadata before XDP, we could make sure drivers set the fields in order to avoid any memove() (and maybe even provide a helper to ensure this?).

Quoted 8 lines

nope, since the current APIs introduced by Stanislav are consuming NIC metadata in kfuncs (mainly for af_xdp) and, according to my understanding, we want to add a kfunc to store the info for each NIC metadata (e.g rx-hash, timestamping, ..) into the packet (this is what Toke is proposing, right?). In this case kfunc calling order makes a difference.
We can think even to add single kfunc to store all the info for all the NIC metadata (maybe via a helping struct) but it seems not scalable to me and we are losing kfunc versatility.

Yes, I agree we should have separate kfuncs for each metadata field. Which means it makes a lot of sense to just use the same setter API that we use for the user-registered metadata fields, but using reserved keys. So something like:

#define BPF_METADATA_HW_HASH BIT(60)
#define BPF_METADATA_HW_TIMESTAMP BIT(61)
#define BPF_METADATA_HW_VLAN BIT(62)
#define BPF_METADATA_RESERVED (0xffff << 48)

bpf_packet_metadata_set(pkt, BPF_METADATA_HW_HASH, hash_value);

As for the internal representation, we can just have the kfunc do something like:

int bpf_packet_metadata_set(field_id, value) {

  switch(field_id) {
    case BPF_METADATA_HW_HASH:
      pkt->xdp_hw_meta.hash = value;
      break;
    [...]
    default:
      /* do the key packing thing */

Quoted 6 lines

}
}

that way the order of setting the HW fields doesn't matter, only the user-defined metadata.

Can you expand on why we need the flexibility of picking the metadata fields here? Presumably we are talking about the use-cases where the XDP program is doing redirect/pass and it doesn't really know who's the final consumer is (might be another xdp program or might be the xdp->skb kernel case), so the only sensible option here seems to be store everything?

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

For the same reason that we have separate kfuncs for each metadata field when getting it from the driver: XDP programs should have the flexibility to decide which pieces of metadata they need, and skip the overhead of stuff that is not needed.

For instance, say an XDP program knows that nothing in the system uses timestamps; in that case, it can skip both the getter and the setter call for timestamps.

Original RFC is talking about XDP -> XDP_REDIRECT -> skb use-case, right? For this we pretty much know what kind of metadata we want to preserve, so why not ship it in the existing metadata area and have a kfunc that the xdp program will call prior to doing xdp_redirect? This kfunc can do exactly what you're suggesting - skip the timestamp if we know that the timestamping is off.

Or have we moved to discussing some other use-cases? What am I missing about the need for some other new mechanism?

Quoted 20 lines

Quoted 5 lines

But doesn't it put us in the same place? Where the first (native) xdp program needs to know which metadata the final consumer wants. At this point why not propagate metadata layout as well?

(or maybe I'm still missing what exact use-case we are trying to solve)

There are two different use-cases for the metadata:

* "Hardware" metadata (like the hash, rx_timestamp...). There are only a few well known fields, and only XDP can access them to set them as metadata, so storing them in a struct somewhere could make sense.

* Arbitrary metadata used by services. Eg a TC filter could set a field describing which service a packet is for, and that could be reused for iptables, routing, socket dispatch...
Similarly we could set a "packet_id" field that uniquely identifies a packet so we can trace it throughout the network stack (through clones, encap, decap, userspace services...).
The skb->mark, but with more room, and better support for sharing it.

Quoted 4 lines

We can only know the layout ahead of time for the first one. And they're similar enough in their requirements (need to be stored somewhere in the SKB, have a way of retrieving each one individually, that it seems to make sense to use a common API).

Why not have the following layout then?

+---------------+-------------------+----------------------------------------+------+
| more headroom | user-defined meta | hw-meta (potentially fixed skb format) | data |
+---------------+-------------------+----------------------------------------+------+
                ^                                                            ^
            data_meta                                                      data

You obviously still have a problem of communicating the layout if you have some redirects in between, but you, in theory still have this problem with user-defined metadata anyway (unless I'm missing something).

Quoted 14 lines

Quoted 13 lines

Quoted 9 lines

I suppose we *could* support just a single call to set the skb meta, like:

bpf_set_skb_meta(struct xdp_md *pkt, struct xdp_hw_meta *data);

...but in that case, we'd need to support some fields being unset anyway, and the program would have to populate the struct on the stack before performing the call. So it seems simpler to just have symmetry between the get (from HW) and set side? :)

Why not simply bpf_set_skb_meta(struct xdp_md *pkt) and let it store the metadata somewhere in xdp_md directly? (also presumably by reusing most of the existing kfuncs/xmo_xxx helpers)

Quoted 3 lines

If we store it in xdp_md, the metadata won't be available higher up the stack (or am I missing something?). I think one of the goals is to let things other than XDP access it (maybe even the network stack itself?).

IIRC, xdp metadata gets copied to skb metadata, so it does propagate. Although, it might have a detrimental effect on the gro, but I'm assuming that is something that can be fixed separately.

Quoted 11 lines · Stanislav Fomichev

Quoted 10 lines · Arthur Fabre

Quoted 9 lines · Stanislav Fomichev

Quoted 8 lines · Toke Høiland-Jørgensen

Quoted 6 lines · Stanislav Fomichev

Quoted 5 lines · Toke Høiland-Jørgensen

Quoted 3 lines · Lorenzo Bianconi

Quoted 3 lines · Lorenzo Bianconi

Quoted 2 lines

Quoted 2 lines

Lorenzo Bianconi <[email protected]> writes:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

I like this 'fast' KV approach but I guess we should really evaluate its impact on performances (especially for xdp) since, based on the kfunc calls order in the ebpf program, we can have one or multiple memmove/memcpy for each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using

Quoted 27 lines

Quoted 14 lines

There are two different use-cases for the metadata:

* "Hardware" metadata (like the hash, rx_timestamp...). There are only a few well known fields, and only XDP can access them to set them as metadata, so storing them in a struct somewhere could make sense.

* Arbitrary metadata used by services. Eg a TC filter could set a field describing which service a packet is for, and that could be reused for iptables, routing, socket dispatch...
Similarly we could set a "packet_id" field that uniquely identifies a packet so we can trace it throughout the network stack (through clones, encap, decap, userspace services...).
The skb->mark, but with more room, and better support for sharing it.

Quoted 4 lines

We can only know the layout ahead of time for the first one. And they're similar enough in their requirements (need to be stored somewhere in the SKB, have a way of retrieving each one individually, that it seems to make sense to use a common API).

Why not have the following layout then?

+---------------+-------------------+----------------------------------------+------+
| more headroom | user-defined meta | hw-meta (potentially fixed skb format) | data |
+---------------+-------------------+----------------------------------------+------+
                 ^                                                            ^
             data_meta                                                      data

Quoted 5 lines

You obviously still have a problem of communicating the layout if you have some redirects in between, but you, in theory still have this problem with user-defined metadata anyway (unless I'm missing something).

[...]

Quoted 27 lines

Quoted 27 lines

Quoted 14 lines

There are two different use-cases for the metadata:

* "Hardware" metadata (like the hash, rx_timestamp...). There are only a few well known fields, and only XDP can access them to set them as metadata, so storing them in a struct somewhere could make sense.

* Arbitrary metadata used by services. Eg a TC filter could set a field describing which service a packet is for, and that could be reused for iptables, routing, socket dispatch...
Similarly we could set a "packet_id" field that uniquely identifies a packet so we can trace it throughout the network stack (through clones, encap, decap, userspace services...).
The skb->mark, but with more room, and better support for sharing it.

Quoted 4 lines

We can only know the layout ahead of time for the first one. And they're similar enough in their requirements (need to be stored somewhere in the SKB, have a way of retrieving each one individually, that it seems to make sense to use a common API).

Why not have the following layout then?

+---------------+-------------------+----------------------------------------+------+
| more headroom | user-defined meta | hw-meta (potentially fixed skb format) | data |
+---------------+-------------------+----------------------------------------+------+
                 ^                                                            ^
             data_meta                                                      data

Quoted 5 lines

Quoted 5 lines

You obviously still have a problem of communicating the layout if you have some redirects in between, but you, in theory still have this problem with user-defined metadata anyway (unless I'm missing something).

Hmm, I think you are missing something... As far as I'm concerned we are discussing placing the KV data after the xdp_frame, and not in the XDP data_meta area (as your drawing suggests). The xdp_frame is stored at the very top of the headroom. Lorenzo's patchset is extending struct xdp_frame and now we are discussing to we can make a more flexible API for extending this. I understand that Toke confirmed this here [3]. Let me know if I missed something :-)

[3] https://lore.kernel.org/all/[email protected]/

As part of designing this flexible API, we/Toke are trying hard not to tie this to a specific data area. This is a good API design, keeping it flexible enough that we can move things around should the need arise.

Quoted 42 lines · Jesper Dangaard Brouer

[...]

Quoted 27 lines

Quoted 27 lines

Quoted 14 lines

There are two different use-cases for the metadata:

* "Hardware" metadata (like the hash, rx_timestamp...). There are only a few well known fields, and only XDP can access them to set them as metadata, so storing them in a struct somewhere could make sense.

* Arbitrary metadata used by services. Eg a TC filter could set a field describing which service a packet is for, and that could be reused for iptables, routing, socket dispatch...
Similarly we could set a "packet_id" field that uniquely identifies a packet so we can trace it throughout the network stack (through clones, encap, decap, userspace services...).
The skb->mark, but with more room, and better support for sharing it.

Quoted 4 lines

We can only know the layout ahead of time for the first one. And they're similar enough in their requirements (need to be stored somewhere in the SKB, have a way of retrieving each one individually, that it seems to make sense to use a common API).

Why not have the following layout then?

+---------------+-------------------+----------------------------------------+------+
| more headroom | user-defined meta | hw-meta (potentially fixed skb format) | data |
+---------------+-------------------+----------------------------------------+------+
                  ^                                                            ^
              data_meta                                                      data

Quoted 5 lines

Quoted 5 lines

You obviously still have a problem of communicating the layout if you have some redirects in between, but you, in theory still have this problem with user-defined metadata anyway (unless I'm missing something).

Hmm, I think you are missing something... As far as I'm concerned we are discussing placing the KV data after the xdp_frame, and not in the XDP data_meta area (as your drawing suggests). The xdp_frame is stored at the very top of the headroom. Lorenzo's patchset is extending struct xdp_frame and now we are discussing to we can make a more flexible API for extending this. I understand that Toke confirmed this here [3]. Let me know if I missed something :-)

Quoted 5 lines

   [3] https://lore.kernel.org/all/[email protected]/

As part of designing this flexible API, we/Toke are trying hard not to tie this to a specific data area. This is a good API design, keeping it flexible enough that we can move things around should the need arise.

+1. And if we have an API for doing this for user-defined metadata, it seems like we might as well use it for hardware metadata too.

With something roughly like:

     *val get(id)

     set(id, *val)

with pre-defined ids for hardware metadata, consumers don't need to know the layout, or where / how the data is stored.

Under the hood we can implement it however we want, and change it in the future.

I was initially thinking we could store hardware metadata the same way as user defined metadata, but Toke and Lorenzo seem to prefer storing it in a fixed struct.

Quoted 64 lines · Arthur Fabre

Quoted 42 lines · Jesper Dangaard Brouer

[...]

Quoted 27 lines

Quoted 27 lines

Quoted 14 lines

There are two different use-cases for the metadata:

* "Hardware" metadata (like the hash, rx_timestamp...). There are only a few well known fields, and only XDP can access them to set them as metadata, so storing them in a struct somewhere could make sense.

* Arbitrary metadata used by services. Eg a TC filter could set a field describing which service a packet is for, and that could be reused for iptables, routing, socket dispatch...
Similarly we could set a "packet_id" field that uniquely identifies a packet so we can trace it throughout the network stack (through clones, encap, decap, userspace services...).
The skb->mark, but with more room, and better support for sharing it.

Quoted 4 lines

We can only know the layout ahead of time for the first one. And they're similar enough in their requirements (need to be stored somewhere in the SKB, have a way of retrieving each one individually, that it seems to make sense to use a common API).

Why not have the following layout then?

+---------------+-------------------+----------------------------------------+------+
| more headroom | user-defined meta | hw-meta (potentially fixed skb format) | data |
+---------------+-------------------+----------------------------------------+------+
                  ^                                                            ^
              data_meta                                                      data

Quoted 5 lines

Quoted 5 lines

You obviously still have a problem of communicating the layout if you have some redirects in between, but you, in theory still have this problem with user-defined metadata anyway (unless I'm missing something).

Hmm, I think you are missing something... As far as I'm concerned we are discussing placing the KV data after the xdp_frame, and not in the XDP data_meta area (as your drawing suggests). The xdp_frame is stored at the very top of the headroom. Lorenzo's patchset is extending struct xdp_frame and now we are discussing to we can make a more flexible API for extending this. I understand that Toke confirmed this here [3]. Let me know if I missed something :-)

Quoted 5 lines

   [3] https://lore.kernel.org/all/[email protected]/

As part of designing this flexible API, we/Toke are trying hard not to tie this to a specific data area. This is a good API design, keeping it flexible enough that we can move things around should the need arise.

+1. And if we have an API for doing this for user-defined metadata, it seems like we might as well use it for hardware metadata too.

With something roughly like:

     *val get(id)

     set(id, *val)

with pre-defined ids for hardware metadata, consumers don't need to know the layout, or where / how the data is stored.

Under the hood we can implement it however we want, and change it in the future.

Quoted 3 lines

I was initially thinking we could store hardware metadata the same way as user defined metadata, but Toke and Lorenzo seem to prefer storing it in a fixed struct.

If the API hide the actual location then we can always move things around, later. If your popcnt approach is fast enough, then IMO we don't need a fixed struct for hardware metadata.

--Jesper

Quoted 69 lines · Jesper Dangaard Brouer

Quoted 67 lines · Arthur Fabre

Quoted 42 lines · Jesper Dangaard Brouer

[...]

Quoted 27 lines

Quoted 27 lines

Quoted 14 lines

There are two different use-cases for the metadata:

* "Hardware" metadata (like the hash, rx_timestamp...). There are only a few well known fields, and only XDP can access them to set them as metadata, so storing them in a struct somewhere could make sense.

* Arbitrary metadata used by services. Eg a TC filter could set a field describing which service a packet is for, and that could be reused for iptables, routing, socket dispatch...
Similarly we could set a "packet_id" field that uniquely identifies a packet so we can trace it throughout the network stack (through clones, encap, decap, userspace services...).
The skb->mark, but with more room, and better support for sharing it.

Quoted 4 lines

We can only know the layout ahead of time for the first one. And they're similar enough in their requirements (need to be stored somewhere in the SKB, have a way of retrieving each one individually, that it seems to make sense to use a common API).

Why not have the following layout then?

+---------------+-------------------+----------------------------------------+------+
| more headroom | user-defined meta | hw-meta (potentially fixed skb format) | data |
+---------------+-------------------+----------------------------------------+------+
                  ^                                                            ^
              data_meta                                                      data

Quoted 5 lines

Quoted 5 lines

You obviously still have a problem of communicating the layout if you have some redirects in between, but you, in theory still have this problem with user-defined metadata anyway (unless I'm missing something).

Hmm, I think you are missing something... As far as I'm concerned we are discussing placing the KV data after the xdp_frame, and not in the XDP data_meta area (as your drawing suggests). The xdp_frame is stored at the very top of the headroom. Lorenzo's patchset is extending struct xdp_frame and now we are discussing to we can make a more flexible API for extending this. I understand that Toke confirmed this here [3]. Let me know if I missed something :-)

Quoted 5 lines

   [3] https://lore.kernel.org/all/[email protected]/

As part of designing this flexible API, we/Toke are trying hard not to tie this to a specific data area. This is a good API design, keeping it flexible enough that we can move things around should the need arise.

+1. And if we have an API for doing this for user-defined metadata, it seems like we might as well use it for hardware metadata too.

With something roughly like:

     *val get(id)

     set(id, *val)

with pre-defined ids for hardware metadata, consumers don't need to know the layout, or where / how the data is stored.

Under the hood we can implement it however we want, and change it in the future.

I was initially thinking we could store hardware metadata the same way as user defined metadata, but Toke and Lorenzo seem to prefer storing it in a fixed struct.

Quoted 3 lines

If the API hide the actual location then we can always move things around, later. If your popcnt approach is fast enough, then IMO we don't need a fixed struct for hardware metadata.

+1. I am fine with the KV approach for nic metadata as well if it is fast enough.

If you want I can modify my series to use kfunc sto store data after xdp_frame and then you can plug the KV encoding. What do you think? Up to you.

Quoted 12 lines · Daniel Xu

Quoted 11 lines · Stanislav Fomichev

Quoted 10 lines · Arthur Fabre

Quoted 9 lines · Stanislav Fomichev

Quoted 8 lines · Toke Høiland-Jørgensen

Quoted 6 lines · Stanislav Fomichev

Quoted 5 lines · Toke Høiland-Jørgensen

Quoted 3 lines · Lorenzo Bianconi

Quoted 3 lines · Lorenzo Bianconi

Quoted 2 lines

Quoted 2 lines

Lorenzo Bianconi <[email protected]> writes:

[...]

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

I like this 'fast' KV approach but I guess we should really evaluate its
impact on performances (especially for xdp) since, based on the kfunc calls
order in the ebpf program, we can have one or multiple memmove/memcpy for
each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using

I really like the *compact* Key-Value (KV) store idea from Arthur.
- The question is it is fast enough?

I've promised Arthur to XDP micro-benchmark this, if he codes this up to be usable in the XDP code path. Listening to the LPC recording I heard that Alexei also saw potential and other use-case for this kind of fast-and-compact KV approach.

I have high hopes for the performance, as Arthur uses POPCNT instruction which is *very* fast[1]. I checked[2] AMD Zen 3 and 4 have Ops/Latency=1 and Reciprocal throughput 0.25.

[1] https://www.agner.org/optimize/blog/read.php?i=853#848
[2] https://www.agner.org/optimize/instruction_tables.pdf

[...]

Quoted 27 lines

Quoted 27 lines

Quoted 14 lines

There are two different use-cases for the metadata:

* "Hardware" metadata (like the hash, rx_timestamp...). There are only a few well known fields, and only XDP can access them to set them as metadata, so storing them in a struct somewhere could make sense.

* Arbitrary metadata used by services. Eg a TC filter could set a field describing which service a packet is for, and that could be reused for iptables, routing, socket dispatch...
Similarly we could set a "packet_id" field that uniquely identifies a packet so we can trace it throughout the network stack (through clones, encap, decap, userspace services...).
The skb->mark, but with more room, and better support for sharing it.

Quoted 4 lines

We can only know the layout ahead of time for the first one. And they're similar enough in their requirements (need to be stored somewhere in the SKB, have a way of retrieving each one individually, that it seems to make sense to use a common API).

Why not have the following layout then?

+---------------+-------------------+----------------------------------------+------+
| more headroom | user-defined meta | hw-meta (potentially fixed skb format) | data |
+---------------+-------------------+----------------------------------------+------+
                 ^                                                            ^
             data_meta                                                      data

Quoted 5 lines

Quoted 5 lines

You obviously still have a problem of communicating the layout if you have some redirects in between, but you, in theory still have this problem with user-defined metadata anyway (unless I'm missing something).

Hmm, I think you are missing something... As far as I'm concerned we are discussing placing the KV data after the xdp_frame, and not in the XDP data_meta area (as your drawing suggests). The xdp_frame is stored at the very top of the headroom. Lorenzo's patchset is extending struct xdp_frame and now we are discussing to we can make a more flexible API for extending this. I understand that Toke confirmed this here [3]. Let me know if I missed something :-)

[3] https://lore.kernel.org/all/[email protected]/

As part of designing this flexible API, we/Toke are trying hard not to tie this to a specific data area. This is a good API design, keeping it flexible enough that we can move things around should the need arise.

I don't think it is viable to store this KV data in XDP data_meta area, because existing BPF-prog's already have direct memory (write) access and can change size of area, which creates too much headache with (existing) BPF-progs creating unintentional breakage for the KV store, which would then need extensive checks to handle random corruptions (slowing down KV-store code).

Quoted 87 lines · Jesper Dangaard Brouer

Quoted 12 lines · Daniel Xu

Quoted 11 lines · Stanislav Fomichev

Quoted 10 lines · Arthur Fabre

Quoted 9 lines · Stanislav Fomichev

Quoted 8 lines · Toke Høiland-Jørgensen

Quoted 6 lines · Stanislav Fomichev

Quoted 5 lines · Toke Høiland-Jørgensen

Quoted 3 lines · Lorenzo Bianconi

Quoted 3 lines · Lorenzo Bianconi

Quoted 2 lines

Quoted 2 lines

Lorenzo Bianconi <[email protected]> writes:

[...]

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

I like this 'fast' KV approach but I guess we should really evaluate its
impact on performances (especially for xdp) since, based on the kfunc calls
order in the ebpf program, we can have one or multiple memmove/memcpy for
each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using

I really like the *compact* Key-Value (KV) store idea from Arthur.
- The question is it is fast enough?

I've promised Arthur to XDP micro-benchmark this, if he codes this up to be usable in the XDP code path. Listening to the LPC recording I heard that Alexei also saw potential and other use-case for this kind of fast-and-compact KV approach.

I have high hopes for the performance, as Arthur uses POPCNT instruction which is *very* fast[1]. I checked[2] AMD Zen 3 and 4 have Ops/Latency=1 and Reciprocal throughput 0.25.

[1] https://www.agner.org/optimize/blog/read.php?i=853#848
[2] https://www.agner.org/optimize/instruction_tables.pdf

[...]

Quoted 27 lines

Quoted 27 lines

Quoted 14 lines

There are two different use-cases for the metadata:

* "Hardware" metadata (like the hash, rx_timestamp...). There are only a few well known fields, and only XDP can access them to set them as metadata, so storing them in a struct somewhere could make sense.

* Arbitrary metadata used by services. Eg a TC filter could set a field describing which service a packet is for, and that could be reused for iptables, routing, socket dispatch...
Similarly we could set a "packet_id" field that uniquely identifies a packet so we can trace it throughout the network stack (through clones, encap, decap, userspace services...).
The skb->mark, but with more room, and better support for sharing it.

Quoted 4 lines

We can only know the layout ahead of time for the first one. And they're similar enough in their requirements (need to be stored somewhere in the SKB, have a way of retrieving each one individually, that it seems to make sense to use a common API).

Why not have the following layout then?

+---------------+-------------------+----------------------------------------+------+
| more headroom | user-defined meta | hw-meta (potentially fixed skb format) | data |
+---------------+-------------------+----------------------------------------+------+
                 ^                                                            ^
             data_meta                                                      data

Quoted 5 lines

Quoted 5 lines

You obviously still have a problem of communicating the layout if you have some redirects in between, but you, in theory still have this problem with user-defined metadata anyway (unless I'm missing something).

Hmm, I think you are missing something... As far as I'm concerned we are discussing placing the KV data after the xdp_frame, and not in the XDP data_meta area (as your drawing suggests). The xdp_frame is stored at the very top of the headroom. Lorenzo's patchset is extending struct xdp_frame and now we are discussing to we can make a more flexible API for extending this. I understand that Toke confirmed this here [3]. Let me know if I missed something :-)

[3] https://lore.kernel.org/all/[email protected]/

As part of designing this flexible API, we/Toke are trying hard not to tie this to a specific data area. This is a good API design, keeping it flexible enough that we can move things around should the need arise.

Quoted 6 lines

I don't think it is viable to store this KV data in XDP data_meta area, because existing BPF-prog's already have direct memory (write) access and can change size of area, which creates too much headache with (existing) BPF-progs creating unintentional breakage for the KV store, which would then need extensive checks to handle random corruptions (slowing down KV-store code).

Yes, I'm definitely missing the bigger picture. If we want to have a global metadata registry in the kernel, why can't it be built on top of the existing area?

Quoted 95 lines · Stanislav Fomichev

Quoted 93 lines · Jesper Dangaard Brouer

Quoted 12 lines · Daniel Xu

Quoted 11 lines · Stanislav Fomichev

Quoted 10 lines · Arthur Fabre

Quoted 9 lines · Stanislav Fomichev

Quoted 8 lines · Toke Høiland-Jørgensen

Quoted 6 lines · Stanislav Fomichev

Quoted 5 lines · Toke Høiland-Jørgensen

Quoted 3 lines · Lorenzo Bianconi

Quoted 3 lines · Lorenzo Bianconi

Quoted 2 lines

Lorenzo Bianconi <[email protected]> writes:

[...]

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

I like this 'fast' KV approach but I guess we should really evaluate its
impact on performances (especially for xdp) since, based on the kfunc calls
order in the ebpf program, we can have one or multiple memmove/memcpy for
each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using

I really like the *compact* Key-Value (KV) store idea from Arthur.
- The question is it is fast enough?

I've promised Arthur to XDP micro-benchmark this, if he codes this up to be usable in the XDP code path. Listening to the LPC recording I heard that Alexei also saw potential and other use-case for this kind of fast-and-compact KV approach.

I have high hopes for the performance, as Arthur uses POPCNT instruction which is *very* fast[1]. I checked[2] AMD Zen 3 and 4 have Ops/Latency=1 and Reciprocal throughput 0.25.

[1] https://www.agner.org/optimize/blog/read.php?i=853#848
[2] https://www.agner.org/optimize/instruction_tables.pdf

[...]

Quoted 27 lines

Quoted 27 lines

Quoted 14 lines

There are two different use-cases for the metadata:

* "Hardware" metadata (like the hash, rx_timestamp...). There are only a few well known fields, and only XDP can access them to set them as metadata, so storing them in a struct somewhere could make sense.

* Arbitrary metadata used by services. Eg a TC filter could set a field describing which service a packet is for, and that could be reused for iptables, routing, socket dispatch...
Similarly we could set a "packet_id" field that uniquely identifies a packet so we can trace it throughout the network stack (through clones, encap, decap, userspace services...).
The skb->mark, but with more room, and better support for sharing it.

Quoted 4 lines

We can only know the layout ahead of time for the first one. And they're similar enough in their requirements (need to be stored somewhere in the SKB, have a way of retrieving each one individually, that it seems to make sense to use a common API).

Why not have the following layout then?

+---------------+-------------------+----------------------------------------+------+
| more headroom | user-defined meta | hw-meta (potentially fixed skb format) | data |
+---------------+-------------------+----------------------------------------+------+
                 ^                                                            ^
             data_meta                                                      data

Quoted 5 lines

Quoted 5 lines

You obviously still have a problem of communicating the layout if you have some redirects in between, but you, in theory still have this problem with user-defined metadata anyway (unless I'm missing something).

Hmm, I think you are missing something... As far as I'm concerned we are discussing placing the KV data after the xdp_frame, and not in the XDP data_meta area (as your drawing suggests). The xdp_frame is stored at the very top of the headroom. Lorenzo's patchset is extending struct xdp_frame and now we are discussing to we can make a more flexible API for extending this. I understand that Toke confirmed this here [3]. Let me know if I missed something :-)

[3] https://lore.kernel.org/all/[email protected]/

As part of designing this flexible API, we/Toke are trying hard not to tie this to a specific data area. This is a good API design, keeping it flexible enough that we can move things around should the need arise.

I don't think it is viable to store this KV data in XDP data_meta area, because existing BPF-prog's already have direct memory (write) access and can change size of area, which creates too much headache with (existing) BPF-progs creating unintentional breakage for the KV store, which would then need extensive checks to handle random corruptions (slowing down KV-store code).

Quoted 3 lines

Yes, I'm definitely missing the bigger picture. If we want to have a global metadata registry in the kernel, why can't it be built on top of the existing area?

Because we have no way of preventing existing XDP programs from overwriting (corrupting) the area using the xdp_adjust_meta() API and data_meta field.

But in a sense the *memory area* is shared between the two APIs, in the sense that they both use the headroom before the packet data, just from opposite ends. So if you store lots of data using the new KV API, that space will no longer be available for xdp_adjust_{head,meta}. But the kernel can enforce this so we don't get programs corrupting the KV format.

Quoted 97 lines · Toke Høiland-Jørgensen

Quoted 95 lines · Stanislav Fomichev

Quoted 93 lines · Jesper Dangaard Brouer

Quoted 12 lines · Daniel Xu

Quoted 11 lines · Stanislav Fomichev

Quoted 10 lines · Arthur Fabre

Quoted 9 lines · Stanislav Fomichev

Quoted 8 lines · Toke Høiland-Jørgensen

Quoted 6 lines · Stanislav Fomichev

Quoted 5 lines · Toke Høiland-Jørgensen

Quoted 3 lines · Lorenzo Bianconi

Quoted 3 lines · Lorenzo Bianconi

Lorenzo Bianconi <[email protected]> writes:

[...]

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

I like this 'fast' KV approach but I guess we should really evaluate its
impact on performances (especially for xdp) since, based on the kfunc calls
order in the ebpf program, we can have one or multiple memmove/memcpy for
each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using

I really like the *compact* Key-Value (KV) store idea from Arthur.
- The question is it is fast enough?

I've promised Arthur to XDP micro-benchmark this, if he codes this up to be usable in the XDP code path. Listening to the LPC recording I heard that Alexei also saw potential and other use-case for this kind of fast-and-compact KV approach.

I have high hopes for the performance, as Arthur uses POPCNT instruction which is *very* fast[1]. I checked[2] AMD Zen 3 and 4 have Ops/Latency=1 and Reciprocal throughput 0.25.

[1] https://www.agner.org/optimize/blog/read.php?i=853#848
[2] https://www.agner.org/optimize/instruction_tables.pdf

[...]

Quoted 27 lines

Quoted 27 lines

Quoted 14 lines

There are two different use-cases for the metadata:

* "Hardware" metadata (like the hash, rx_timestamp...). There are only a few well known fields, and only XDP can access them to set them as metadata, so storing them in a struct somewhere could make sense.

* Arbitrary metadata used by services. Eg a TC filter could set a field describing which service a packet is for, and that could be reused for iptables, routing, socket dispatch...
Similarly we could set a "packet_id" field that uniquely identifies a packet so we can trace it throughout the network stack (through clones, encap, decap, userspace services...).
The skb->mark, but with more room, and better support for sharing it.

Quoted 4 lines

We can only know the layout ahead of time for the first one. And they're similar enough in their requirements (need to be stored somewhere in the SKB, have a way of retrieving each one individually, that it seems to make sense to use a common API).

Why not have the following layout then?

+---------------+-------------------+----------------------------------------+------+
| more headroom | user-defined meta | hw-meta (potentially fixed skb format) | data |
+---------------+-------------------+----------------------------------------+------+
                 ^                                                            ^
             data_meta                                                      data

Quoted 5 lines

Quoted 5 lines

You obviously still have a problem of communicating the layout if you have some redirects in between, but you, in theory still have this problem with user-defined metadata anyway (unless I'm missing something).

Hmm, I think you are missing something... As far as I'm concerned we are discussing placing the KV data after the xdp_frame, and not in the XDP data_meta area (as your drawing suggests). The xdp_frame is stored at the very top of the headroom. Lorenzo's patchset is extending struct xdp_frame and now we are discussing to we can make a more flexible API for extending this. I understand that Toke confirmed this here [3]. Let me know if I missed something :-)

[3] https://lore.kernel.org/all/[email protected]/

As part of designing this flexible API, we/Toke are trying hard not to tie this to a specific data area. This is a good API design, keeping it flexible enough that we can move things around should the need arise.

I don't think it is viable to store this KV data in XDP data_meta area, because existing BPF-prog's already have direct memory (write) access and can change size of area, which creates too much headache with (existing) BPF-progs creating unintentional breakage for the KV store, which would then need extensive checks to handle random corruptions (slowing down KV-store code).

Quoted 7 lines

Quoted 3 lines

Yes, I'm definitely missing the bigger picture. If we want to have a global metadata registry in the kernel, why can't it be built on top of the existing area?

Because we have no way of preventing existing XDP programs from overwriting (corrupting) the area using the xdp_adjust_meta() API and data_meta field.

True, but this can be solved with some new BPF_F_XDP_HAS_FRAGS-like flag (which can reject loading if there is some incompatibility)? Even in the new KV-metadata world, 2+ programs still need to be aware of the new method to work correctly. But I do see your point that it's better to not apply any metadata than apply something that's corrupt/overridden.

Quoted 6 lines

But in a sense the *memory area* is shared between the two APIs, in the sense that they both use the headroom before the packet data, just from opposite ends. So if you store lots of data using the new KV API, that space will no longer be available for xdp_adjust_{head,meta}. But the kernel can enforce this so we don't get programs corrupting the KV format.

Ack, let's see how it shapes out. My main concern comes from the growing api surface where for af_xdp it's one mechanism, for xdp redirect it's another. And for Jakub's consumption from userspace it's gonna be another special case probably (to read it out from the headroom head)? Idk, maybe it's fine as long as each case is clearly documented.

Hi Stan,

Quoted 168 lines · Stanislav Fomichev

Quoted 96 lines · Arthur Fabre

Quoted 95 lines · Stanislav Fomichev

Quoted 94 lines · Toke Høiland-Jørgensen

Quoted 91 lines · Stanislav Fomichev

Quoted 78 lines · Toke Høiland-Jørgensen

Quoted 43 lines · Lorenzo Bianconi

Quoted 36 lines · Lorenzo Bianconi

Quoted 35 lines

Quoted 34 lines

Quoted 12 lines · Lorenzo Bianconi

Quoted 12 lines

We could combine such a registration API with your header format, so
that the registration just becomes a way of allocating one of the keys
from 0-63 (and the registry just becomes a global copy of the header).
This would basically amount to moving the "service config file" into the
kernel, since that seems to be the only common denominator we can rely
on between BPF applications (as all attempts to write a common daemon
for BPF management have shown).

That sounds reasonable. And I guess we'd have set() check the global
registry to enforce that the key has been registered beforehand?

Quoted 8 lines

Quoted 3 lines

-Toke

Thanks for all the feedback!

I like this 'fast' KV approach but I guess we should really evaluate its impact on performances (especially for xdp) since, based on the kfunc calls order in the ebpf program, we can have one or multiple memmove/memcpy for each packet, right?

Yes, with Arthur's scheme, performance will be ordering dependent. Using a global registry for offsets would sidestep this, but have the synchronisation issues we discussed up-thread. So on balance, I think the memmove() suggestion will probably lead to the least pain.

For the HW metadata we could sidestep this by always having a fixed struct for it (but using the same set/get() API with reserved keys). The only drawback of doing that is that we statically reserve a bit of space, but I'm not sure that is such a big issue in practice (at least not until this becomes to popular that the space starts to be contended; but surely 256 bytes ought to be enough for everybody, right? :)).

Quoted 6 lines

Quoted 2 lines

I am fine with the proposed approach, but I think we need to verify what is the impact on performances (in the worst case??)

If drivers are responsible for populating the hardware metadata before XDP, we could make sure drivers set the fields in order to avoid any memove() (and maybe even provide a helper to ensure this?).

Quoted 8 lines

nope, since the current APIs introduced by Stanislav are consuming NIC metadata in kfuncs (mainly for af_xdp) and, according to my understanding, we want to add a kfunc to store the info for each NIC metadata (e.g rx-hash, timestamping, ..) into the packet (this is what Toke is proposing, right?). In this case kfunc calling order makes a difference.
We can think even to add single kfunc to store all the info for all the NIC metadata (maybe via a helping struct) but it seems not scalable to me and we are losing kfunc versatility.

Yes, I agree we should have separate kfuncs for each metadata field. Which means it makes a lot of sense to just use the same setter API that we use for the user-registered metadata fields, but using reserved keys. So something like:

#define BPF_METADATA_HW_HASH BIT(60)
#define BPF_METADATA_HW_TIMESTAMP BIT(61)
#define BPF_METADATA_HW_VLAN BIT(62)
#define BPF_METADATA_RESERVED (0xffff << 48)

bpf_packet_metadata_set(pkt, BPF_METADATA_HW_HASH, hash_value);

As for the internal representation, we can just have the kfunc do something like:

int bpf_packet_metadata_set(field_id, value) {

  switch(field_id) {
    case BPF_METADATA_HW_HASH:
      pkt->xdp_hw_meta.hash = value;
      break;
    [...]
    default:
      /* do the key packing thing */

Quoted 6 lines

}
}

that way the order of setting the HW fields doesn't matter, only the user-defined metadata.

Can you expand on why we need the flexibility of picking the metadata fields here? Presumably we are talking about the use-cases where the XDP program is doing redirect/pass and it doesn't really know who's the final consumer is (might be another xdp program or might be the xdp->skb kernel case), so the only sensible option here seems to be store everything?

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

For the same reason that we have separate kfuncs for each metadata field when getting it from the driver: XDP programs should have the flexibility to decide which pieces of metadata they need, and skip the overhead of stuff that is not needed.

For instance, say an XDP program knows that nothing in the system uses timestamps; in that case, it can skip both the getter and the setter call for timestamps.

Original RFC is talking about XDP -> XDP_REDIRECT -> skb use-case, right? For this we pretty much know what kind of metadata we want to preserve, so why not ship it in the existing metadata area and have a kfunc that the xdp program will call prior to doing xdp_redirect? This kfunc can do exactly what you're suggesting - skip the timestamp if we know that the timestamping is off.

Or have we moved to discussing some other use-cases? What am I missing about the need for some other new mechanism?

Quoted 20 lines

Quoted 5 lines

But doesn't it put us in the same place? Where the first (native) xdp program needs to know which metadata the final consumer wants. At this point why not propagate metadata layout as well?

(or maybe I'm still missing what exact use-case we are trying to solve)

There are two different use-cases for the metadata:

* "Hardware" metadata (like the hash, rx_timestamp...). There are only a few well known fields, and only XDP can access them to set them as metadata, so storing them in a struct somewhere could make sense.

* Arbitrary metadata used by services. Eg a TC filter could set a field describing which service a packet is for, and that could be reused for iptables, routing, socket dispatch...
Similarly we could set a "packet_id" field that uniquely identifies a packet so we can trace it throughout the network stack (through clones, encap, decap, userspace services...).
The skb->mark, but with more room, and better support for sharing it.

Quoted 4 lines

We can only know the layout ahead of time for the first one. And they're similar enough in their requirements (need to be stored somewhere in the SKB, have a way of retrieving each one individually, that it seems to make sense to use a common API).

Why not have the following layout then?

+---------------+-------------------+----------------------------------------+------+
| more headroom | user-defined meta | hw-meta (potentially fixed skb format) | data |
+---------------+-------------------+----------------------------------------+------+
                ^                                                            ^
            data_meta                                                      data

You obviously still have a problem of communicating the layout if you have some redirects in between, but you, in theory still have this problem with user-defined metadata anyway (unless I'm missing something).

Quoted 14 lines

Quoted 13 lines

Quoted 9 lines

I suppose we *could* support just a single call to set the skb meta, like:

bpf_set_skb_meta(struct xdp_md *pkt, struct xdp_hw_meta *data);

...but in that case, we'd need to support some fields being unset anyway, and the program would have to populate the struct on the stack before performing the call. So it seems simpler to just have symmetry between the get (from HW) and set side? :)

Why not simply bpf_set_skb_meta(struct xdp_md *pkt) and let it store the metadata somewhere in xdp_md directly? (also presumably by reusing most of the existing kfuncs/xmo_xxx helpers)

Quoted 7 lines

Quoted 3 lines

If we store it in xdp_md, the metadata won't be available higher up the stack (or am I missing something?). I think one of the goals is to let things other than XDP access it (maybe even the network stack itself?).

IIRC, xdp metadata gets copied to skb metadata, so it does propagate. Although, it might have a detrimental effect on the gro, but I'm assuming that is something that can be fixed separately.

I was thinking about this today so I'm glad you brought it up.

IIUC putting unique data in the metadata area will prevent GRO from working. I wonder if as a part of this work there's a cleaner way to indicate to XDP or GRO engine that some metadata should be ignored for coalescing purposes. Otherwise the final XDP prog before GRO would need to memset() all the relevant bytes to 0 (assuming that even works).

Quoted 45 lines · Toke Høiland-Jørgensen

Quoted 30 lines · Arthur Fabre

Quoted 24 lines

Quoted 15 lines

Quoted 9 lines

The nice thing about an API like this, though, is that it's extensible, and the kernel itself can be just another consumer of it for the metadata fields Lorenzo is adding in this series. I.e., we could just pre-define some IDs for metadata vlan, timestamp etc, and use the same functions as above from within the kernel to set and get those values; using the registry, there could even be an option to turn those off if an application wants more space for its own usage. Or, alternatively, we could keep the kernel-internal IDs hardcoded and always allocated, and just use the getter/setter functions as the BPF API for accessing them.

That's exactly what I'm thinking of too, a simple API like:

get(u8 key, u8 len, void *val);
set(u8 key, u8 len, void *val);

Quoted 8 lines

With "well-known" keys like METADATA_ID_HW_HASH for hardware metadata.

If a NIC doesn't support a certain well-known metadata, the key wouldn't be set, and get() would return ENOENT.

I think this also lets us avoid having to "register" keys or bits of metadata with the kernel.
We'd reserve some number of keys for hardware metadata.

Quoted 5 lines

Right, but how do you allocate space/offset for each key without an explicit allocation step? You'd basically have to encode the list of IDs in the metadata area itself, which implies a TLV format that you have to walk on every access? The registry idea in my example above was basically to avoid that...

Quoted 6 lines

I've been playing around with having a small fixed header at the front of the metadata itself, that lets you access values without walking them all.

Still WIP, and maybe this is too restrictive, but it lets you encode 64 2, 4, or 8 byte KVs with a single 16 byte header:

Ohh, that's clever, I like it! :)

It's also extensible in the sense that the internal representation can change without impacting the API, so if we end up needing more bits we can always add those.

Quoted 6 lines

Maybe it would be a good idea to make the 'key' parameter a larger integer type (function parameters are always 64-bit anyway, so might as well go all the way up to u64)? That way we can use higher values for the kernel-reserved types instead of reserving part of the already-small key space for applications (assuming the kernel-internal data is stored somewhere else, like in a static struct as in Lorenzo's patch)?

Good idea! That makes it more extensible too if we ever support more keys or bigger lengths.

I'm not sure where the kernel-reserved types should live. Putting them in here uses up some the of KV IDs, but it uses less head room space than always reserving a static struct for them.
Maybe it doesn't matter much, as long as we use the same API to access them, we could internally switch between one and the other.

Quoted 31 lines

[...]

Quoted 18 lines

Quoted 8 lines

Quoted 5 lines

The remaining keys would be up to users. They'd have to allocate keys to services, and configure services to use those keys.
This is similar to the way listening on a certain port works: only one service can use port 80 or 443, and that can typically beconfigured in a service's config file.

Right, well, port numbers *do* actually have an out of band service registry (IANA), which I thought was what we wanted to avoid? ;)

Depends how you think about it ;)

I think we should avoid a global registry. But having a registry per deployment / server doesn't seem awful. Services that want to use a field would have a config file setting to set which index it corresponds to.
Admins would just have to pick a free one on their system, and set it in the config file of the service.

Quoted 4 lines

This is similar to what we do for non-IANA registered ports internally. For example each service needs a port on an internal interface to expose metrics, and we just track which ports are taken in our config management.

Right, this would work, but it assumes that applications are well-behaved and do this correctly. Which they probably do in a centrally-managed system like yours, but for random applications shipped by distros, I'm not sure if it's going to work.

Quoted 7 lines

In fact, it's more or less the situation we have with skb->mark today, isn't it? I.e., applications can co-exist as long as they don't use the same bits, so they have to coordinate on which bits to use. Sure, with this scheme there will be more total bits to use, which can lessen the pressure somewhat, but the basic problem remains. In other words, I worry that in practice we will end up with another github repository serving as a registry for metadata keys...

That's true. If applications hardcode keys, we'll be back to having conflicts. If someone creates a registry on github I'll be very sad.

(Maybe we can make the verifier enforce that the key passed to get() and set() isn't a constant? - only joking)

Applications don't tend to do this for ports though, I think most can be configured to listen on any port. Is that just because it's been instilled as "good practice" over time? Could we try to do the same with some very stern documentation and examples?

Thinking about it more, my only relectance for a registration API is how to communicate the ID back to other consumers (our discussion below).

Quoted 12 lines

Quoted 7 lines

Dynamically registering fields means you have to share the returned ID with whoever is interested, which sounds tricky.
If an XDP program sets a field like packet_id, every tracing program that looks at it, and userspace service, would need to know what the ID of that field is.
Is there a way to easily share that ID with all of them?

Right, so I'll admit this was one of the handwavy bits of my original proposal, but I don't think it's unsolvable. You could do something like (once, on application initialisation):

Quoted 5 lines

__u64 my_key = bpf_register_metadata_field(my_size); /* maybe add a name for introspection? */ bpf_map_update(&shared_application_config, &my_key_index, &my_key);

and then just get the key out of that map from all programs that want to use it?

Passing it out of band works (whether it's via a pinned map like you described, or through other means like a unix socket or some other API), it's just more complicated.

Every consumer also needs to know about that API. That won't work with standard tools. For example if we set a PACKET_ID KV, maybe we could give it to pwru so it could track packets using it?
Without registering keys, we could pass it as a cli flag. With registration, we'd have to have some helper to get the KV ID.

It also introduces ordering dependencies between the services on startup, eg packet tracing hooks could only be attached once our XDP service has registered a PACKET_ID KV, and they could query it's API.

Quoted 7 lines

We could combine such a registration API with your header format, so that the registration just becomes a way of allocating one of the keys from 0-63 (and the registry just becomes a global copy of the header). This would basically amount to moving the "service config file" into the kernel, since that seems to be the only common denominator we can rely on between BPF applications (as all attempts to write a common daemon for BPF management have shown).

That sounds reasonable. And I guess we'd have set() check the global registry to enforce that the key has been registered beforehand?

Thanks for all the feedback!

Quoted 31 lines

Thinking about it more, my only relectance for a registration API is how to communicate the ID back to other consumers (our discussion below).

Quoted 12 lines

Quoted 7 lines

Dynamically registering fields means you have to share the returned ID with whoever is interested, which sounds tricky.
If an XDP program sets a field like packet_id, every tracing program that looks at it, and userspace service, would need to know what the ID of that field is.
Is there a way to easily share that ID with all of them?

Right, so I'll admit this was one of the handwavy bits of my original proposal, but I don't think it's unsolvable. You could do something like (once, on application initialisation):

Quoted 5 lines

__u64 my_key = bpf_register_metadata_field(my_size); /* maybe add a name for introspection? */ bpf_map_update(&shared_application_config, &my_key_index, &my_key);

and then just get the key out of that map from all programs that want to use it?

Passing it out of band works (whether it's via a pinned map like you described, or through other means like a unix socket or some other API), it's just more complicated.

Every consumer also needs to know about that API. That won't work with standard tools. For example if we set a PACKET_ID KV, maybe we could give it to pwru so it could track packets using it?
Without registering keys, we could pass it as a cli flag. With registration, we'd have to have some helper to get the KV ID.

Quoted 3 lines

It also introduces ordering dependencies between the services on startup, eg packet tracing hooks could only be attached once our XDP service has registered a PACKET_ID KV, and they could query it's API.

Yeah, we definitely need a way to make that accessible and not too cumbersome.

I suppose what we really need is a way to map an application-specific identifier to an ID. And, well, those identifiers could just be (string) names? That's what we do for CO-RE, after all. So you'd get something like:

id = bpf_register_metadata_field("packet_id", 8, BPF_CREATE); /* register */

id = bpf_register_metadata_field("packet_id", 8, BPF_NO_CREATE); /* resolve */

and we make that idempotent, so that two callers using the same name and size will just get the same id back; and if called with BPF_NO_CREATE, you'll get -ENOENT if it hasn't already been registered by someone else?

We could even make this a sub-command of the bpf() syscall if we want it to be UAPI, but that's not strictly necessary, applications can also just call the registration from a syscall program at startup...

Quoted 10 lines

Quoted 7 lines

We could combine such a registration API with your header format, so that the registration just becomes a way of allocating one of the keys from 0-63 (and the registry just becomes a global copy of the header). This would basically amount to moving the "service config file" into the kernel, since that seems to be the only common denominator we can rely on between BPF applications (as all attempts to write a common daemon for BPF management have shown).

That sounds reasonable. And I guess we'd have set() check the global registry to enforce that the key has been registered beforehand?

Yes, exactly. And maybe check that the size matches as well just to remove the obvious footgun of accidentally stepping on each other's toes?

Quoted 1 line

Thanks for all the feedback!

You're welcome! Thanks for working on this :)

-Toke

Quoted 47 lines · Toke Høiland-Jørgensen

Quoted 31 lines

Thinking about it more, my only relectance for a registration API is how to communicate the ID back to other consumers (our discussion below).

Quoted 12 lines

Quoted 7 lines

Dynamically registering fields means you have to share the returned ID with whoever is interested, which sounds tricky.
If an XDP program sets a field like packet_id, every tracing program that looks at it, and userspace service, would need to know what the ID of that field is.
Is there a way to easily share that ID with all of them?

Right, so I'll admit this was one of the handwavy bits of my original proposal, but I don't think it's unsolvable. You could do something like (once, on application initialisation):

Quoted 5 lines

__u64 my_key = bpf_register_metadata_field(my_size); /* maybe add a name for introspection? */ bpf_map_update(&shared_application_config, &my_key_index, &my_key);

and then just get the key out of that map from all programs that want to use it?

Passing it out of band works (whether it's via a pinned map like you described, or through other means like a unix socket or some other API), it's just more complicated.

Every consumer also needs to know about that API. That won't work with standard tools. For example if we set a PACKET_ID KV, maybe we could give it to pwru so it could track packets using it?
Without registering keys, we could pass it as a cli flag. With registration, we'd have to have some helper to get the KV ID.

Quoted 3 lines

It also introduces ordering dependencies between the services on startup, eg packet tracing hooks could only be attached once our XDP service has registered a PACKET_ID KV, and they could query it's API.

Yeah, we definitely need a way to make that accessible and not too cumbersome.

I suppose what we really need is a way to map an application-specific identifier to an ID. And, well, those identifiers could just be (string) names? That's what we do for CO-RE, after all. So you'd get something like:

id = bpf_register_metadata_field("packet_id", 8, BPF_CREATE); /* register */

id = bpf_register_metadata_field("packet_id", 8, BPF_NO_CREATE); /* resolve */

Quoted 7 lines

and we make that idempotent, so that two callers using the same name and size will just get the same id back; and if called with BPF_NO_CREATE, you'll get -ENOENT if it hasn't already been registered by someone else?

We could even make this a sub-command of the bpf() syscall if we want it to be UAPI, but that's not strictly necessary, applications can also just call the registration from a syscall program at startup...

That's a nice API, it makes sharing the IDs much easier.

We still have to worry about collisions (what if two different things want to add their own "packet_id" field?). But at least:

* "Any string" has many more possibilities than 0-64 keys.

* bpf_register_metadata() could return an error if a field is already registered, instead of silently letting an application overwrite metadata

(although arguably we could have add a BPF_NOEXIST style flag to the KV set() to kind of do the same).

At least internally, it still feels like we'd maintain a registry of these string fields and make them configurable for each service to avoid collisions.

Quoted 56 lines · Arthur Fabre

Quoted 47 lines · Toke Høiland-Jørgensen

Quoted 31 lines

Thinking about it more, my only relectance for a registration API is how to communicate the ID back to other consumers (our discussion below).

Quoted 12 lines

Quoted 7 lines

Dynamically registering fields means you have to share the returned ID with whoever is interested, which sounds tricky.
If an XDP program sets a field like packet_id, every tracing program that looks at it, and userspace service, would need to know what the ID of that field is.
Is there a way to easily share that ID with all of them?

Right, so I'll admit this was one of the handwavy bits of my original proposal, but I don't think it's unsolvable. You could do something like (once, on application initialisation):

Quoted 5 lines

__u64 my_key = bpf_register_metadata_field(my_size); /* maybe add a name for introspection? */ bpf_map_update(&shared_application_config, &my_key_index, &my_key);

and then just get the key out of that map from all programs that want to use it?

Passing it out of band works (whether it's via a pinned map like you described, or through other means like a unix socket or some other API), it's just more complicated.

Every consumer also needs to know about that API. That won't work with standard tools. For example if we set a PACKET_ID KV, maybe we could give it to pwru so it could track packets using it?
Without registering keys, we could pass it as a cli flag. With registration, we'd have to have some helper to get the KV ID.

Quoted 3 lines

It also introduces ordering dependencies between the services on startup, eg packet tracing hooks could only be attached once our XDP service has registered a PACKET_ID KV, and they could query it's API.

Yeah, we definitely need a way to make that accessible and not too cumbersome.

I suppose what we really need is a way to map an application-specific identifier to an ID. And, well, those identifiers could just be (string) names? That's what we do for CO-RE, after all. So you'd get something like:

id = bpf_register_metadata_field("packet_id", 8, BPF_CREATE); /* register */

id = bpf_register_metadata_field("packet_id", 8, BPF_NO_CREATE); /* resolve */

Quoted 7 lines

and we make that idempotent, so that two callers using the same name and size will just get the same id back; and if called with BPF_NO_CREATE, you'll get -ENOENT if it hasn't already been registered by someone else?

We could even make this a sub-command of the bpf() syscall if we want it to be UAPI, but that's not strictly necessary, applications can also just call the registration from a syscall program at startup...

Quoted 6 lines

That's a nice API, it makes sharing the IDs much easier.

We still have to worry about collisions (what if two different things want to add their own "packet_id" field?). But at least:

* "Any string" has many more possibilities than 0-64 keys.

Yes, and it's easy to namespace (by prefixing, like APPNAME_my_metaname). But sure, if everyone uses very generic names that will be a problem.

Quoted 3 lines

* bpf_register_metadata() could return an error if a field is already registered, instead of silently letting an application overwrite metadata

I don't think we can fundamentally solve the collision problem if we also need to allow sharing keys (on purpose). I.e., we can't distinguish between "these two applications deliberately want to share the packet_id field" and "these two applications accidentally both picked packet_id as their internal key".

I suppose we could pre-define some extra reserved keys if there turns out to be widely used identifiers that many applications want. But I'm not sure if that should be there from the start, it quickly becomes very speculative(packet_id comes to mind as one that might be generally useful, but I'm really not sure, TBH).

Quoted 2 lines

(although arguably we could have add a BPF_NOEXIST style flag to the KV set() to kind of do the same).

A global registry will need locking, so not sure it's a good idea to support inserting into it in the fast path?

Quoted 4 lines

At least internally, it still feels like we'd maintain a registry of these string fields and make them configurable for each service to avoid collisions.

Yeah, see above. Some kind of coordination (like a registry) is impossible to avoid if you *want* to share data, but not sure how common that is?

-Toke

From: Lorenzo Bianconi <[email protected]>
Date: Sat, 21 Sep 2024 18:52:56 +0200

Quoted 1 line

This series introduces the xdp_rx_meta struct in the xdp_buff/xdp_frame

&xdp_buff is on the stack.
&xdp_frame consumes headroom.

IOW they're size-sensitive and putting metadata directly there might play bad; if not now, then later.

Our idea (me + Toke) was as follows:

- new BPF kfunc to build generic meta. If called, the driver builds a generic meta with hash, csum etc., in the data_meta area. Yes, this also consumes headroom, but only when the corresponding func is called. Introducing new fields like you're doing will consume it unconditionally;

- when &xdp_frame gets converted to sk_buff, the function checks whether data_meta contains a generic structure filled with hints.

We also thought about &skb_shared_info, but it's also size-sensitive as it consumes tailroom.

Quoted 5 lines

one as a container to store the already supported xdp rx hw hints (rx_hash and rx_vlan, rx_timestamp will be stored in skb_shared_info area) when the eBPF program running on the nic performs XDP_REDIRECT. Doing so, we are able to set the skb metadata converting the xdp_buff/xdp_frame to a skb.

Thanks,
Olek