On Fri Oct 4, 2024 at 12:38 PM CEST, Jesper Dangaard Brouer wrote: > [...] > >>> 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). > >> > >> 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). > >> > > 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/874j62u1lb.fsf@xxxxxxx/ > > 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.
