Message-Id: <D4KIZY73DAJJ.EVUPLH612IV6@bobby>
Date: Tue, 01 Oct 2024 16:06:20 +0200
From: "Arthur Fabre" <afabre@...udflare.com>
To: Toke Høiland-Jørgensen <toke@...hat.com>
Cc: "Lorenzo Bianconi" <lorenzo.bianconi@...hat.com>, "Jesper Dangaard
 Brouer" <hawk@...nel.org>, "Jakub Sitnicki" <jakub@...udflare.com>,
 "Alexander Lobakin" <aleksander.lobakin@...el.com>, "Lorenzo Bianconi"
 <lorenzo@...nel.org>, <bpf@...r.kernel.org>, <netdev@...r.kernel.org>,
 <ast@...nel.org>, <daniel@...earbox.net>, <davem@...emloft.net>,
 <kuba@...nel.org>, <john.fastabend@...il.com>, <edumazet@...gle.com>,
 <pabeni@...hat.com>, <sdf@...ichev.me>, <tariqt@...dia.com>,
 <saeedm@...dia.com>, <anthony.l.nguyen@...el.com>,
 <przemyslaw.kitszel@...el.com>, <intel-wired-lan@...ts.osuosl.org>,
 <mst@...hat.com>, <jasowang@...hat.com>, <mcoquelin.stm32@...il.com>,
 <alexandre.torgue@...s.st.com>, "kernel-team" <kernel-team@...udflare.com>,
 "Yan Zhai" <yan@...udflare.com>
Subject: Re: [RFC bpf-next 0/4] Add XDP rx hw hints support performing
 XDP_REDIRECT

On Mon Sep 30, 2024 at 12:52 PM CEST, Toke Høiland-Jørgensen wrote:
> > Thinking about it more, my only relectance for a registration API is how
> > to communicate the ID back to other consumers (our discussion below).
> >
> >>
> >> > 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?
> >
> > 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.
>
> 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...

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.

> >> 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?
>
> > Thanks for all the feedback!
>
> You're welcome! Thanks for working on this :)
>
> -Toke

Powered by blists - more mailing lists