On 10/03, Arthur Fabre wrote:
> On Thu Oct 3, 2024 at 12:49 AM CEST, Stanislav Fomichev wrote:
> > On 10/02, Toke Høiland-Jørgensen wrote:
> > > Stanislav Fomichev <stfomic...@gmail.com> writes:
> > >
> > > > On 10/01, Toke Høiland-Jørgensen wrote:
> > > >> Lorenzo Bianconi <lore...@kernel.org> writes:
> > > >>
> > > >> >> On Mon Sep 30, 2024 at 1:49 PM CEST, Lorenzo Bianconi wrote:
> > > >> >> > > Lorenzo Bianconi <lore...@kernel.org> writes:
> > > >> >> > >
> > > >> >> > > >> > 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?
> > > >> >> > > >>
> > > >> >> > > >> >
> > > >> >> > > >> > -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?
> > > >> >> > > :)).
> > > >> >> >
> > > >> >> > 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.
> > > >>
> > > >> 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.
> > > >
> > > > 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.
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?
> > 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).
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).
> > > 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)
>
> 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.
