Re: [PATCH v2 bpf-next 05/14] bpf: allow to specify user-provided context value for BPF perf links

From: Peter Zijlstra <peterz@infradead.org>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v2 bpf-next 05/14] bpf: allow to specify user-provided context value for BPF perf links
Date: Wed, 28 Jul 2021 10:58:24 +0200	[thread overview]
Message-ID: <YQEcMEHm+Ww1m4WO@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <CAEf4BzZCOj_rQrUjLnvBNYTDCg6A_5mC7rBuBJxm0Lzr8F5-pg@mail.gmail.com>

On Tue, Jul 27, 2021 at 02:09:08PM -0700, Andrii Nakryiko wrote:
> On Tue, Jul 27, 2021 at 2:14 AM Peter Zijlstra <peterz@infradead.org> 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).

OK, fair enough I suppose.

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

y/w :-)

Thanks!