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.
> 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.
>
