On Fri Oct 4, 2024 at 12:38 PM CEST, Jesper Dangaard Brouer wrote:

[...]

quoted

quoted

quoted

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@toke.dk/ (local)

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.