On Tue, Jun 24, 2025 at 8:38 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Wed, Jun 18, 2025 at 3:19 PM Amery Hung <ameryhung@xxxxxxxxx> wrote:
> >
> > On Thu, Jun 12, 2025 at 4:08 PM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> > >
> > > On Mon, Jun 9, 2025 at 4:27 PM Amery Hung <ameryhung@xxxxxxxxx> wrote:
> > > >
> > > > Allows struct_ops implementors to infer the calling struct_ops instance
> > > > inside a kfunc through prog->aux->this_st_ops. A new field, flags, is
> > > > added to bpf_struct_ops. If BPF_STRUCT_OPS_F_THIS_PTR is set in flags,
> > > > a pointer to the struct_ops structure registered to the kernel (i.e.,
> > > > kvalue->data) will be saved to prog->aux->this_st_ops. To access it in
> > > > a kfunc, use BPF_STRUCT_OPS_F_THIS_PTR with __prog argument [0]. The
> > > > verifier will fixup the argument with a pointer to prog->aux. this_st_ops
> > > > is protected by rcu and is valid until a struct_ops map is unregistered
> > > > updated.
> > > >
> > > > For a struct_ops map with BPF_STRUCT_OPS_F_THIS_PTR, to make sure all
> > > > programs in it have the same this_st_ops, cmpxchg is used. Only if a
> > > > program is not already used in another struct_ops map also with
> > > > BPF_STRUCT_OPS_F_THIS_PTR can it be assigned to the current struct_ops
> > > > map.
> > > >
> > >
> > > Have you considered an alternative to storing this_st_ops in
> > > bpf_prog_aux by setting it at runtime (in struct_ops trampoline) into
> > > bpf_run_ctx (which I think all struct ops programs have set), and then
> > > letting any struct_ops kfunc just access it through current (see other
> > > uses of bpf_run_ctx, if you are unfamiliar). This would avoid all this
> > > business with extra flags and passing bpf_prog_aux as an extra
> > > argument.
> > >
> >
> > I didn't know this. Thanks for suggesting an alternative!
> >
> > > There will be no "only one struct_ops for this BPF program" limitation
> > > either: technically, you could have the same BPF program used from two
> > > struct_ops maps just fine (even at the same time). Then depending on
> > > which struct_ops is currently active, you'd have a corresponding
> > > bpf_run_ctx's struct_ops pointer. It feels like a cleaner approach to
> > > me.
> > >
> >
> > This is a cleaner approach for struct_ops operators. To make it work
> > for kfuncs called in timer callback, I think prog->aux->st_ops is
> > still needed, but at least we can unify how to get this_st_ops in
> > kfunc, in a way that does not requires adding __prog to every kfuncs.
> >
> > +enum bpf_run_ctx_type {
> > + BPF_CG_RUN_CTX = 0,
> > + BPF_TRACE_RUN_CTX,
> > + BPF_TRAMP_RUN_CTX,
> > + BPF_TIMER_RUN_CTX,
> > +};
> >
> > struct bpf_run_ctx {
> > + enum bpf_run_ctx_type type;
> > };
> >
> > +struct bpf_timer_run_ctx {
> > + struct bpf_prog_aux *aux;
> > +};
> >
> > struct bpf_tramp_run_ctx {
> > ...
> > + void *st_ops;
> > };
> >
> > In bpf_struct_ops_prepare_trampoline(), the st_ops assignment will be
> > emitted to the trampoline.
> >
> > In bpf_timer_cb(), prepare bpf_timer_run_ctx, where st_ops comes from
> > prog->aux->this_st_ops and set current->bpf_ctx.
> >
> > Finally, in kfuncs that want to know the current struct_ops, call this
> > new function below:
> >
> > +void *bpf_struct_ops_current_st_ops(void)
> > +{
> > + struct bpf_prog_aux aux;
> > +
> > + if (!current->bpf_ctx)
> > + return NULL;
> > +
> > + switch(current->bpf_ctx->type) {
> > + case BPF_TRAMP_RUN_CTX:
> > + return (struct bpf_tramp_run_ctx *)(current->bpf_ctx)->st_ops;
> > + case BPF_TIMER_RUN_CTX:
> > + aux = (struct bpf_timer_run_ctx *)(current->bpf_ctx)->aux;
> > + return rcu_dereference(aux->this_st_ops);
> > + }
> > + return NULL;
> > +}
> >
> > What do you think?
>
> I'm not sure I particularly like different ways to get this st_ops
> pointer depending whether we are in an async callback or not. So if
> bpf_prog_aux is inevitable, I'd just stick to that. As far as
> bpf_run_ctx, though, instead of bpf_run_ctx_type, shall we just put
> `struct bpf_prog_aux *` pointer into common struct bpf_run_ctx and
> always set it for all program types that support run_ctx?
No. This is death by a thousand cuts.
bpf trampoline is getting slower and slower.
scx's 'this' case is not a common pattern.
