From: Kyle Huey <me@kylehuey.com>
To: Marco Elver <elver@google.com>
Cc: Kyle Huey <khuey@kylehuey.com>,
linux-kernel@vger.kernel.org,
Andrii Nakryiko <andrii.nakryiko@gmail.com>,
Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
Yonghong Song <yonghong.song@linux.dev>,
"Robert O'Callahan" <robert@ocallahan.org>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Ian Rogers <irogers@google.com>,
Adrian Hunter <adrian.hunter@intel.com>,
linux-perf-users@vger.kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH v3 1/4] perf/bpf: Call bpf handler directly, not through overflow machinery
Date: Mon, 11 Dec 2023 07:20:22 -0800 [thread overview]
Message-ID: <CAP045ApSQWLouDd65-M4+TFiKFjDF1aMPnPW-Ue+e63NhQnUwQ@mail.gmail.com> (raw)
In-Reply-To: <CANpmjNPvZ=S5Afn9DR7nG2UFstqz5t1gBznTh4yezVv7k1+m9w@mail.gmail.com>
On Mon, Dec 11, 2023 at 6:20 AM Marco Elver <elver@google.com> wrote:
>
> On Mon, 11 Dec 2023 at 05:55, Kyle Huey <me@kylehuey.com> 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@kylehuey.com>
> > Suggested-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> > 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 5547ba68e6e4..312b9f31442c 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
> > @@ -1337,10 +1336,7 @@ __is_default_overflow_handler(perf_overflow_handler_t overflow_handler)
> > #ifdef CONFIG_BPF_SYSCALL
> > static inline bool uses_default_overflow_handler(struct perf_event *event)
> > {
> > - if (likely(is_default_overflow_handler(event)))
> > - return true;
> > -
> > - return __is_default_overflow_handler(event->orig_overflow_handler);
> > + return is_default_overflow_handler(event);
> > }
> > #else
> > #define uses_default_overflow_handler(event) \
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index b704d83a28b2..54f6372d2634 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -9515,6 +9515,12 @@ static inline bool sample_is_allowed(struct perf_event *event, struct pt_regs *r
> > return true;
> > }
> >
> > +#ifdef CONFIG_BPF_SYSCALL
> > +static int bpf_overflow_handler(struct perf_event *event,
> > + struct perf_sample_data *data,
> > + struct pt_regs *regs);
> > +#endif
>
> To avoid more #ifdefs we usually add a stub, something like:
>
> #ifdef ...
> static int bpf_overflow_handler(...);
> #else
> static inline int bpf_overflow_handler(...) { return 0; }
> #endif
>
> Then you can avoid more #ifdefs below, esp. when it surrounds an
> if-statement it easily leads to confusion or subtle bugs in future
> changes. The compiler will optimize out the constants and the
> generated code will be the same.
This would not allow removing any #ifdefs because event->prog is only
present if CONFIG_BPF_SYSCALL is defined.
- Kyle
> > /*
> > * Generic event overflow handling, sampling.
> > */
> > @@ -9584,7 +9590,10 @@ static int __perf_event_overflow(struct perf_event *event,
> > irq_work_queue(&event->pending_irq);
> > }
> >
> > - 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);
> >
> > if (*perf_event_fasync(event) && event->pending_kill) {
> > event->pending_wakeup = 1;
> > @@ -10394,9 +10403,9 @@ static void perf_event_free_filter(struct perf_event *event)
> > }
> >
> > #ifdef CONFIG_BPF_SYSCALL
> > -static void bpf_overflow_handler(struct perf_event *event,
> > - struct perf_sample_data *data,
> > - struct pt_regs *regs)
> > +static int bpf_overflow_handler(struct perf_event *event,
> > + struct perf_sample_data *data,
> > + struct pt_regs *regs)
> > {
> > struct bpf_perf_event_data_kern ctx = {
> > .data = data,
> > @@ -10417,10 +10426,8 @@ static void bpf_overflow_handler(struct perf_event *event,
> > rcu_read_unlock();
> > out:
> > __this_cpu_dec(bpf_prog_active);
> > - if (!ret)
> > - return;
> >
> > - event->orig_overflow_handler(event, data, regs);
> > + return ret;
> > }
> >
> > static int perf_event_set_bpf_handler(struct perf_event *event,
> > @@ -10456,8 +10463,6 @@ static int perf_event_set_bpf_handler(struct perf_event *event,
> >
> > event->prog = prog;
> > event->bpf_cookie = bpf_cookie;
> > - event->orig_overflow_handler = READ_ONCE(event->overflow_handler);
> > - WRITE_ONCE(event->overflow_handler, bpf_overflow_handler);
> > return 0;
> > }
> >
> > @@ -10468,7 +10473,6 @@ static void perf_event_free_bpf_handler(struct perf_event *event)
> > if (!prog)
> > return;
> >
> > - WRITE_ONCE(event->overflow_handler, event->orig_overflow_handler);
> > event->prog = NULL;
> > bpf_prog_put(prog);
> > }
> > @@ -11928,13 +11932,11 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
> > overflow_handler = parent_event->overflow_handler;
> > context = parent_event->overflow_handler_context;
> > #if defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_EVENT_TRACING)
> > - if (overflow_handler == bpf_overflow_handler) {
> > + if (parent_event->prog) {
> > struct bpf_prog *prog = parent_event->prog;
> >
> > bpf_prog_inc(prog);
> > event->prog = prog;
> > - event->orig_overflow_handler =
> > - parent_event->orig_overflow_handler;
> > }
> > #endif
> > }
> > --
> > 2.34.1
> >
next prev parent reply other threads:[~2023-12-11 15:20 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-11 4:55 [PATCH v3 0/4] Combine perf and bpf for fast eval of hw breakpoint conditions Kyle Huey
2023-12-11 4:55 ` [PATCH v3 1/4] perf/bpf: Call bpf handler directly, not through overflow machinery Kyle Huey
2023-12-11 14:20 ` Marco Elver
2023-12-11 15:20 ` Kyle Huey [this message]
2024-01-02 22:56 ` Song Liu
2024-01-02 23:05 ` Song Liu
2024-01-19 0:07 ` Kyle Huey
2023-12-11 4:55 ` [PATCH v3 2/4] perf/bpf: Remove unneeded uses_default_overflow_handler Kyle Huey
2023-12-11 4:55 ` Kyle Huey
2023-12-12 9:22 ` Will Deacon
2023-12-12 9:22 ` Will Deacon
2024-01-02 22:56 ` Song Liu
2024-01-02 22:56 ` Song Liu
2023-12-11 4:55 ` [PATCH v3 3/4] perf/bpf: Allow a bpf program to suppress all sample side effects Kyle Huey
2024-01-02 23:10 ` Song Liu
2023-12-11 4:55 ` [PATCH v3 4/4] selftest/bpf: Test a perf bpf program that suppresses " Kyle Huey
2024-01-02 22:49 ` Song Liu
2024-01-19 0:08 ` Kyle Huey
2023-12-13 1:38 ` [PATCH v3 0/4] Combine perf and bpf for fast eval of hw breakpoint conditions Namhyung Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAP045ApSQWLouDd65-M4+TFiKFjDF1aMPnPW-Ue+e63NhQnUwQ@mail.gmail.com \
--to=me@kylehuey.com \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=andrii.nakryiko@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=elver@google.com \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=khuey@kylehuey.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=robert@ocallahan.org \
--cc=yonghong.song@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.