On Fri Oct 4, 2024 at 4:18 PM CEST, Lorenzo Bianconi wrote: > On Oct 04, Jesper Dangaard Brouer wrote: > > On 04/10/2024 15.55, Arthur Fabre wrote: > > > 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. > > > > 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. Great! That's simpler. I should have something for Jesper to benchmark on Monday. > 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. Thanks for the offer! That works for me :)
