On Wed, Apr 10, 2024 at 12:32 AM Ingo Molnar <mingo@xxxxxxxxxx> wrote: > > > * Kyle Huey <me@xxxxxxxxxxxx> wrote: > > > To ultimately allow bpf programs attached to perf events to completely > > suppress all of the effects of a perf event overflow (rather than just the > > sample output, as they do today), call bpf_overflow_handler() from > > __perf_event_overflow() directly rather than modifying struct perf_event's > > overflow_handler. Return the bpf program's return value from > > bpf_overflow_handler() so that __perf_event_overflow() knows how to > > proceed. Remove the now unnecessary orig_overflow_handler from struct > > perf_event. > > > > This patch is solely a refactoring and results in no behavior change. > > > > Signed-off-by: Kyle Huey <khuey@xxxxxxxxxxxx> > > Suggested-by: Namhyung Kim <namhyung@xxxxxxxxxx> > > Acked-by: Song Liu <song@xxxxxxxxxx> > > Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx> > > --- > > include/linux/perf_event.h | 6 +----- > > kernel/events/core.c | 28 +++++++++++++++------------- > > 2 files changed, 16 insertions(+), 18 deletions(-) > > > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > > index d2a15c0c6f8a..c7f54fd74d89 100644 > > --- a/include/linux/perf_event.h > > +++ b/include/linux/perf_event.h > > @@ -810,7 +810,6 @@ struct perf_event { > > perf_overflow_handler_t overflow_handler; > > void *overflow_handler_context; > > #ifdef CONFIG_BPF_SYSCALL > > - perf_overflow_handler_t orig_overflow_handler; > > struct bpf_prog *prog; > > u64 bpf_cookie; > > #endif > > Could we reduce the #ifdeffery please? Not easily. > On distros CONFIG_BPF_SYSCALL is almost always enabled, so it's not like > this truly saves anything on real systems. > > I'd suggest making the perf_event::prog and perf_event::bpf_cookie fields > unconditional. That's not sufficient. See below. > > +#ifdef CONFIG_BPF_SYSCALL > > +static int bpf_overflow_handler(struct perf_event *event, > > + struct perf_sample_data *data, > > + struct pt_regs *regs); > > +#endif > > If the function definitions are misordered then first do a patch that moves > the function earlier in the file, instead of slapping a random prototype > into a random place. Ok. > > - READ_ONCE(event->overflow_handler)(event, data, regs); > > +#ifdef CONFIG_BPF_SYSCALL > > + if (!(event->prog && !bpf_overflow_handler(event, data, regs))) > > +#endif > > + READ_ONCE(event->overflow_handler)(event, data, regs); > > This #ifdef would go away too - on !CONFIG_BPF_SYSCALL event->prog should > always be NULL. bpf_overflow_handler() is also #ifdef CONFIG_BPF_SYSCALL. It uses bpf_prog_active, so that would need to be moved out of the ifdef, which would require moving the DEFINE_PER_CPU out of bpf/syscall.c ... or I'd have to add a !CONFIG_BPF_SYSCALL definition of bpf_overflow_handler() that only returns 1 and never actually gets called because the condition short-circuits on event->prog. Neither seems like it makes my patch or the code simpler, especially since this weird ifdef-that-applies-only-to-the-condition goes away in Part 3 where I actually change the behavior. It feels like the root of your objection is that CONFIG_BPF_SYSCALL exists at all. I could remove it in a separate patch if there's consensus about that. > Please keep the #ifdeffery reduction and function-moving patches separate > from these other changes. > > Thanks, > > Ingo - Kyle