On Fri, Jul 11, 2025 at 1:21 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Fri, Jul 11, 2025 at 12:29 PM Amery Hung <ameryhung@xxxxxxxxx> wrote:
> >
> > 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().
>
> Can we put cookie into map_extra during st_ops map creation time and
> later copy it into actual cookie place in a trampoline?
It should work. Currently, it makes sense to have cookie in struct_ops
map as all struct_ops implementers (hid, tcp cc, qdisc, sched_ext) do
not allow multi-attachment of the same map. A struct_ops map is
effectively an unique attachment for now.
Additionally, we will be able to support cookie for non-link
struct_ops with this way.
This approach will not block future effort to support link-specific
cookie if there is such a use case. We can revisit this patchset then.
