On Tue, Jul 27, 2021 at 2:14 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> On Mon, Jul 26, 2021 at 09:12:02AM -0700, Andrii Nakryiko wrote:
> > Add ability for users to specify custom u64 value when creating BPF link for
> > perf_event-backed BPF programs (kprobe/uprobe, perf_event, tracepoints).
>
> If I read this right, the value is dependent on the link, not the
> program. In which case:
You can see it both ways. BPF link in this (and at least few other
cases) is just this invisible orchestrator of BPF program
attachment/detachment. The underlying perf_event subsystem doesn't
know about the existence of the BPF link at all. In the end, it's
actually struct bpf_prog that is added to perf_event or into tp's
bpf_prog_array list, and this user-provided value (bpf cookie per
below) is associated with that particular attachment. So when we call
trace_call_bpf() from tracepoint or kprobe/uprobe, there is no BPF
link anywhere, it's just a list of bpf_prog_array_items, with bpf_prog
pointer and associated user value. Note, exactly the same bpf_prog can
be attached to another perf_event with a completely different cookie
and that's expected and is fine.
So in short, perf_event just needs to know about attaching/detaching
bpf_prog pointer (and this cookie), it doesn't need to know about
bpf_link. Everything is handled the same regardless if bpf_link is
used to attach or ioctl(PERF_EVENT_IOC_SET_BPF).
>
> > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> > index 2d510ad750ed..97ab46802800 100644
> > --- a/include/linux/perf_event.h
> > +++ b/include/linux/perf_event.h
> > @@ -762,6 +762,7 @@ struct perf_event {
> > #ifdef CONFIG_BPF_SYSCALL
> > perf_overflow_handler_t orig_overflow_handler;
> > struct bpf_prog *prog;
> > + u64 user_ctx;
> > #endif
> >
> > #ifdef CONFIG_EVENT_TRACING
> > diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> > index 8ac92560d3a3..4543852f1480 100644
> > --- a/include/linux/trace_events.h
> > +++ b/include/linux/trace_events.h
> > @@ -675,7 +675,7 @@ trace_trigger_soft_disabled(struct trace_event_file *file)
> >
> > #ifdef CONFIG_BPF_EVENTS
> > unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx);
> > -int perf_event_attach_bpf_prog(struct perf_event *event, struct bpf_prog *prog);
> > +int perf_event_attach_bpf_prog(struct perf_event *event, struct bpf_prog *prog, u64 user_ctx);
>
> This API would be misleading, because it is about setting the program.
Answered above, here perf_event just provides a low-level internal API
for attaching bpf_prog with associated value. BPF link is a
higher-level invisible concept as far as perf_event is concerned.
>
> > void perf_event_detach_bpf_prog(struct perf_event *event);
> > int perf_event_query_prog_array(struct perf_event *event, void __user *info);
> > int bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_prog *prog);
>
> > @@ -9966,6 +9968,7 @@ static int perf_event_set_bpf_handler(struct perf_event *event, struct bpf_prog
> > }
> >
> > event->prog = prog;
> > + event->user_ctx = user_ctx;
> > event->orig_overflow_handler = READ_ONCE(event->overflow_handler);
> > WRITE_ONCE(event->overflow_handler, bpf_overflow_handler);
> > return 0;
>
> Also, the name @user_ctx is a bit confusing. Would something like
> @bpf_cookie or somesuch not be a better name?
I struggled to come up with a good name, user_ctx was the best I could
do. But I do like bpf_cookie for this, thank you! I'll switch the
terminology in the next revision.
>
> Combined would it not make more sense to add something like:
>
> extern int perf_event_set_bpf_cookie(struct perf_event *event, u64 cookie);
Passing that user_ctx along the bpf_prog makes it clear that they go
together and user_ctx is immutable once set. I don't actually plan to
allow updating this cookie value.
>
>
