On Fri, Jul 11, 2025 at 11:41 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Thu, Jul 10, 2025 at 2:00 PM Amery Hung <ameryhung@xxxxxxxxx> wrote:
> >
> > On Thu, Jul 10, 2025 at 12:47 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
> > >
> > > On 7/10/25 11:39 AM, Amery Hung wrote:
> > > >> On 7/8/25 4:08 PM, Amery Hung wrote:
> > > >>> @@ -906,6 +904,10 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
> > > >>> goto unlock;
> > > >>> }
> > > >>>
> > > >>> + err = bpf_struct_ops_prepare_attach(st_map, 0);
> > > >> A follow-up on the "using the map->id as the cookie" comment in the cover
> > > >> letter. I meant to use the map->id here instead of 0. If the cookie is intended
> > > >> to identify a particular struct_ops instance (i.e., the struct_ops map), then
> > > >> map->id should be a good fit, and it is automatically generated by the kernel
> > > >> during the map creation. As a result, I suspect that most of the changes in
> > > >> patch 1 and patch 2 will not be needed.
> > > >>
> > > > Do you mean keep using cookie as the mechanism to associate programs,
> > > > but for struct_ops the cookie will be map->id (i.e.,
> > > > bpf_get_attah_cookie() in struct_ops will return map->id)?
> > >
> > > I meant to use the map->id as the bpf_cookie stored in the bpf_tramp_run_ctx.
> > > Then there is no need for user space to generate a unique cookie during
> > > link_create. The kernel has already generated a unique ID in the map->id. The
> > > map->id is available during the bpf_struct_ops_map_update_elem(). Then there is
> > > also no need to distinguish between SEC(".struct_ops") vs
> > > SEC(".struct_ops.link"). Most of the patch 1 and patch 2 will not be needed.
> > >
> > > A minor detail: note that the same struct ops program can be used in different
> > > trampolines. Thus, to be specific, the bpf cookie is stored in the trampoline.
> > >
> > > If the question is about bpf global variable vs bpf cookie, yeah, I think using
> > > a bpf global variable should also work. The global variable can be initialized
> > > before libbpf's bpf_map__attach_struct_ops(). At that time, the map->id should
> > > be known already. I don't have a strong opinion on reusing the bpf cookie in the
> > > struct ops trampoline. No one is using it now, so it is available to be used.
> > > Exposing BPF_FUNC_get_attach_cookie for struct ops programs is pretty cheap
> > > also. Using bpf cookie to allow the struct ops program to tell which struct_ops
> > > map is calling it seems to fit well also after sleeping on it a bit. bpf global
> > > variable will also break if a bpf_prog.o has more than one SEC(".struct_ops").
> > >
> >
> > While both of them work, using cookie instead of global variable is
> > one less thing for the user to take care of (i.e., slightly better
> > usability).
> >
> > With the approach you suggested, to not mix the existing semantics of
> > bpf cookie, I think a new struct_ops kfuncs is needed to retrieve the
>
> yes, if absolutely necessary, sure, let's reuse the spot that is
> reserved for cookie inside the trampoline, but let's not expose this
> as real BPF cookie (i.e., let's not allow bpf_get_attach_cookie()
> helper for struct_ops), because BPF cookie is meant to be fully user
> controllable and used for whatever they deem necessary. Not
> necessarily to just identify the struct_ops map. So it will be a huge
> violation to just pre-define what BPF cookie value is for struct_ops.
>
We had some offline discussions and figured out this will not work well.
sched_ext users already call scx kfuncs in global subprograms. If we
choose to add bpf_get_struct_ops_id() to get the id to be passed to
scx kfuncs, it will force the user to create two sets of the same
global subprog. The one called by struct_ops that calls
bpf_get_struct_ops_id() and tracing programs that calls
bpf_get_attach_cookie().
> > map->id stored in bpf_tramp_run_ctx::bpf_cookie. Maybe
> > bpf_struct_ops_get_map_id()?
> >
> > Another approach is to complete the plumbing of this patchset by
> > moving trampoline and ksyms from map to link. Right now it is broken
> > when creating multiple links from the same map as can be seen in the
> > CI. I think this is better as we don't create another unique thing for
> > struct_ops.
> >
> > WDYT?
>
> I think that is a logical thing to do, because BPF link represents
> attachment, and trampoline should conceptually correspond to an
> attachment, not to the thing that is being attached (and might be
> attached to multiple places, potentially). We have this approach with
> the fentry/fexit program's trampoline, so it would be nice to move
> struct_ops to the same model.
>
> >
> > > For tracing program, the bpf cookie seems to be an existing mechanism that can
> > > have any value (?). Thus, user space is free to store the map->id in it also. It
> > > can also choose to store the map->id in a bpf global variable if it has other
> > > uses for the bpf cookie. I think both should work similarly.
> >
