* [PATCH 1/2] perf/bpf: Allow a bpf program to suppress I/O signals. [not found] <20231204201406.341074-1-khuey@kylehuey.com> @ 2023-12-04 20:14 ` Kyle Huey 2023-12-04 22:18 ` Andrii Nakryiko 0 siblings, 1 reply; 8+ messages in thread From: Kyle Huey @ 2023-12-04 20:14 UTC (permalink / raw) To: Kyle Huey, linux-kernel Cc: Robert O'Callahan, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter, linux-perf-users, bpf Returning zero from a bpf program attached to a perf event already suppresses any data output. This allows it to suppress I/O availability signals too. Signed-off-by: Kyle Huey <khuey@kylehuey.com> --- kernel/events/core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/kernel/events/core.c b/kernel/events/core.c index b704d83a28b2..34d7b19d45eb 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -10417,8 +10417,10 @@ static void bpf_overflow_handler(struct perf_event *event, rcu_read_unlock(); out: __this_cpu_dec(bpf_prog_active); - if (!ret) + if (!ret) { + event->pending_kill = 0; return; + } event->orig_overflow_handler(event, data, regs); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] perf/bpf: Allow a bpf program to suppress I/O signals. 2023-12-04 20:14 ` [PATCH 1/2] perf/bpf: Allow a bpf program to suppress I/O signals Kyle Huey @ 2023-12-04 22:18 ` Andrii Nakryiko 2023-12-05 11:16 ` Jiri Olsa 0 siblings, 1 reply; 8+ messages in thread From: Andrii Nakryiko @ 2023-12-04 22:18 UTC (permalink / raw) To: Kyle Huey Cc: Kyle Huey, linux-kernel, Robert O'Callahan, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter, linux-perf-users, bpf On Mon, Dec 4, 2023 at 12:14 PM Kyle Huey <me@kylehuey.com> wrote: > > Returning zero from a bpf program attached to a perf event already > suppresses any data output. This allows it to suppress I/O availability > signals too. make sense, just one question below > > Signed-off-by: Kyle Huey <khuey@kylehuey.com> > --- > kernel/events/core.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index b704d83a28b2..34d7b19d45eb 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -10417,8 +10417,10 @@ static void bpf_overflow_handler(struct perf_event *event, > rcu_read_unlock(); > out: > __this_cpu_dec(bpf_prog_active); > - if (!ret) > + if (!ret) { > + event->pending_kill = 0; > return; > + } What's the distinction between event->pending_kill and event->pending_wakeup? Should we do something about pending_wakeup? Asking out of complete ignorance of all these perf specifics. > > event->orig_overflow_handler(event, data, regs); > } > -- > 2.34.1 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] perf/bpf: Allow a bpf program to suppress I/O signals. 2023-12-04 22:18 ` Andrii Nakryiko @ 2023-12-05 11:16 ` Jiri Olsa 2023-12-05 18:07 ` Namhyung Kim 0 siblings, 1 reply; 8+ messages in thread From: Jiri Olsa @ 2023-12-05 11:16 UTC (permalink / raw) To: Andrii Nakryiko Cc: Kyle Huey, Kyle Huey, linux-kernel, Robert O'Callahan, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Ian Rogers, Adrian Hunter, linux-perf-users, bpf On Mon, Dec 04, 2023 at 02:18:49PM -0800, Andrii Nakryiko wrote: > On Mon, Dec 4, 2023 at 12:14 PM Kyle Huey <me@kylehuey.com> wrote: > > > > Returning zero from a bpf program attached to a perf event already > > suppresses any data output. This allows it to suppress I/O availability > > signals too. > > make sense, just one question below > > > > > Signed-off-by: Kyle Huey <khuey@kylehuey.com> Acked-by: Jiri Olsa <jolsa@kernel.org> > > --- > > kernel/events/core.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > index b704d83a28b2..34d7b19d45eb 100644 > > --- a/kernel/events/core.c > > +++ b/kernel/events/core.c > > @@ -10417,8 +10417,10 @@ static void bpf_overflow_handler(struct perf_event *event, > > rcu_read_unlock(); > > out: > > __this_cpu_dec(bpf_prog_active); > > - if (!ret) > > + if (!ret) { > > + event->pending_kill = 0; > > return; > > + } > > What's the distinction between event->pending_kill and > event->pending_wakeup? Should we do something about pending_wakeup? > Asking out of complete ignorance of all these perf specifics. > I think zeroing pending_kill is enough.. when it's set the perf code sets pending_wakeup to call perf_event_wakeup in irq code that wakes up event's ring buffer readers and sends sigio if pending_kill is set jirka > > > > > event->orig_overflow_handler(event, data, regs); > > } > > -- > > 2.34.1 > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] perf/bpf: Allow a bpf program to suppress I/O signals. 2023-12-05 11:16 ` Jiri Olsa @ 2023-12-05 18:07 ` Namhyung Kim 2023-12-05 18:16 ` Marco Elver 2023-12-05 19:19 ` Kyle Huey 0 siblings, 2 replies; 8+ messages in thread From: Namhyung Kim @ 2023-12-05 18:07 UTC (permalink / raw) To: Jiri Olsa Cc: Andrii Nakryiko, Kyle Huey, Kyle Huey, linux-kernel, Robert O'Callahan, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Ian Rogers, Adrian Hunter, linux-perf-users, bpf, Marco Elver Hello, Add Marco Elver to CC. On Tue, Dec 5, 2023 at 3:16 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Mon, Dec 04, 2023 at 02:18:49PM -0800, Andrii Nakryiko wrote: > > On Mon, Dec 4, 2023 at 12:14 PM Kyle Huey <me@kylehuey.com> wrote: > > > > > > Returning zero from a bpf program attached to a perf event already > > > suppresses any data output. This allows it to suppress I/O availability > > > signals too. > > > > make sense, just one question below > > > > > > > > Signed-off-by: Kyle Huey <khuey@kylehuey.com> > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > > > --- > > > kernel/events/core.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > > index b704d83a28b2..34d7b19d45eb 100644 > > > --- a/kernel/events/core.c > > > +++ b/kernel/events/core.c > > > @@ -10417,8 +10417,10 @@ static void bpf_overflow_handler(struct perf_event *event, > > > rcu_read_unlock(); > > > out: > > > __this_cpu_dec(bpf_prog_active); > > > - if (!ret) > > > + if (!ret) { > > > + event->pending_kill = 0; > > > return; > > > + } > > > > What's the distinction between event->pending_kill and > > event->pending_wakeup? Should we do something about pending_wakeup? > > Asking out of complete ignorance of all these perf specifics. > > > > I think zeroing pending_kill is enough.. when it's set the perf code > sets pending_wakeup to call perf_event_wakeup in irq code that wakes > up event's ring buffer readers and sends sigio if pending_kill is set Right, IIUC pending_wakeup is set by the ring buffer code when a task is waiting for events and it gets enough events (watermark). So I think it's good for ring buffer to manage the pending_wakeup. And pending_kill is set when a task wants a signal delivery even without getting enough events. Clearing pending_kill looks ok to suppress normal signals but I'm not sure if it's ok for SIGTRAP. If we want to handle returning 0 from bpf as if the event didn't happen, I think SIGTRAP and event_limit logic should be done after the overflow handler depending on pending_kill or something. Thanks, Namhyung ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] perf/bpf: Allow a bpf program to suppress I/O signals. 2023-12-05 18:07 ` Namhyung Kim @ 2023-12-05 18:16 ` Marco Elver 2023-12-05 18:23 ` Kyle Huey 2023-12-05 18:26 ` Namhyung Kim 2023-12-05 19:19 ` Kyle Huey 1 sibling, 2 replies; 8+ messages in thread From: Marco Elver @ 2023-12-05 18:16 UTC (permalink / raw) To: Namhyung Kim Cc: Jiri Olsa, Andrii Nakryiko, Kyle Huey, Kyle Huey, linux-kernel, Robert O'Callahan, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Ian Rogers, Adrian Hunter, linux-perf-users, bpf On Tue, 5 Dec 2023 at 19:07, Namhyung Kim <namhyung@kernel.org> wrote: > > Hello, > > Add Marco Elver to CC. > > On Tue, Dec 5, 2023 at 3:16 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > On Mon, Dec 04, 2023 at 02:18:49PM -0800, Andrii Nakryiko wrote: > > > On Mon, Dec 4, 2023 at 12:14 PM Kyle Huey <me@kylehuey.com> wrote: > > > > > > > > Returning zero from a bpf program attached to a perf event already > > > > suppresses any data output. This allows it to suppress I/O availability > > > > signals too. > > > > > > make sense, just one question below > > > > > > > > > > > Signed-off-by: Kyle Huey <khuey@kylehuey.com> > > > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > > > > > --- > > > > kernel/events/core.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > > > index b704d83a28b2..34d7b19d45eb 100644 > > > > --- a/kernel/events/core.c > > > > +++ b/kernel/events/core.c > > > > @@ -10417,8 +10417,10 @@ static void bpf_overflow_handler(struct perf_event *event, > > > > rcu_read_unlock(); > > > > out: > > > > __this_cpu_dec(bpf_prog_active); > > > > - if (!ret) > > > > + if (!ret) { > > > > + event->pending_kill = 0; > > > > return; > > > > + } > > > > > > What's the distinction between event->pending_kill and > > > event->pending_wakeup? Should we do something about pending_wakeup? > > > Asking out of complete ignorance of all these perf specifics. > > > > > > > I think zeroing pending_kill is enough.. when it's set the perf code > > sets pending_wakeup to call perf_event_wakeup in irq code that wakes > > up event's ring buffer readers and sends sigio if pending_kill is set > > Right, IIUC pending_wakeup is set by the ring buffer code when > a task is waiting for events and it gets enough events (watermark). > So I think it's good for ring buffer to manage the pending_wakeup. > > And pending_kill is set when a task wants a signal delivery even > without getting enough events. Clearing pending_kill looks ok > to suppress normal signals but I'm not sure if it's ok for SIGTRAP. > > If we want to handle returning 0 from bpf as if the event didn't > happen, I think SIGTRAP and event_limit logic should be done > after the overflow handler depending on pending_kill or something. I'm not sure which kernel version this is for, but in recent kernels, the SIGTRAP logic was changed to no longer "abuse" event_limit, and uses its own "pending_sigtrap" + "pending_work" (on reschedule transitions). Thanks, -- Marco ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] perf/bpf: Allow a bpf program to suppress I/O signals. 2023-12-05 18:16 ` Marco Elver @ 2023-12-05 18:23 ` Kyle Huey 2023-12-05 18:26 ` Namhyung Kim 1 sibling, 0 replies; 8+ messages in thread From: Kyle Huey @ 2023-12-05 18:23 UTC (permalink / raw) To: Marco Elver Cc: Namhyung Kim, Jiri Olsa, Andrii Nakryiko, Kyle Huey, linux-kernel, Robert O'Callahan, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Ian Rogers, Adrian Hunter, linux-perf-users, bpf On Tue, Dec 5, 2023 at 10:17 AM Marco Elver <elver@google.com> wrote: > > On Tue, 5 Dec 2023 at 19:07, Namhyung Kim <namhyung@kernel.org> wrote: > > > > Hello, > > > > Add Marco Elver to CC. > > > > On Tue, Dec 5, 2023 at 3:16 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > > > On Mon, Dec 04, 2023 at 02:18:49PM -0800, Andrii Nakryiko wrote: > > > > On Mon, Dec 4, 2023 at 12:14 PM Kyle Huey <me@kylehuey.com> wrote: > > > > > > > > > > Returning zero from a bpf program attached to a perf event already > > > > > suppresses any data output. This allows it to suppress I/O availability > > > > > signals too. > > > > > > > > make sense, just one question below > > > > > > > > > > > > > > Signed-off-by: Kyle Huey <khuey@kylehuey.com> > > > > > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > > > > > > > --- > > > > > kernel/events/core.c | 4 +++- > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > > > > index b704d83a28b2..34d7b19d45eb 100644 > > > > > --- a/kernel/events/core.c > > > > > +++ b/kernel/events/core.c > > > > > @@ -10417,8 +10417,10 @@ static void bpf_overflow_handler(struct perf_event *event, > > > > > rcu_read_unlock(); > > > > > out: > > > > > __this_cpu_dec(bpf_prog_active); > > > > > - if (!ret) > > > > > + if (!ret) { > > > > > + event->pending_kill = 0; > > > > > return; > > > > > + } > > > > > > > > What's the distinction between event->pending_kill and > > > > event->pending_wakeup? Should we do something about pending_wakeup? > > > > Asking out of complete ignorance of all these perf specifics. > > > > > > > > > > I think zeroing pending_kill is enough.. when it's set the perf code > > > sets pending_wakeup to call perf_event_wakeup in irq code that wakes > > > up event's ring buffer readers and sends sigio if pending_kill is set > > > > Right, IIUC pending_wakeup is set by the ring buffer code when > > a task is waiting for events and it gets enough events (watermark). > > So I think it's good for ring buffer to manage the pending_wakeup. > > > > And pending_kill is set when a task wants a signal delivery even > > without getting enough events. Clearing pending_kill looks ok > > to suppress normal signals but I'm not sure if it's ok for SIGTRAP. > > > > If we want to handle returning 0 from bpf as if the event didn't > > happen, I think SIGTRAP and event_limit logic should be done > > after the overflow handler depending on pending_kill or something. > > I'm not sure which kernel version this is for, but in recent kernels, > the SIGTRAP logic was changed to no longer "abuse" event_limit, and > uses its own "pending_sigtrap" + "pending_work" (on reschedule > transitions). > > Thanks, > -- Marco The patch was prepared against a 6.7 release candidate. - Kyle ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] perf/bpf: Allow a bpf program to suppress I/O signals. 2023-12-05 18:16 ` Marco Elver 2023-12-05 18:23 ` Kyle Huey @ 2023-12-05 18:26 ` Namhyung Kim 1 sibling, 0 replies; 8+ messages in thread From: Namhyung Kim @ 2023-12-05 18:26 UTC (permalink / raw) To: Marco Elver Cc: Jiri Olsa, Andrii Nakryiko, Kyle Huey, Kyle Huey, linux-kernel, Robert O'Callahan, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Ian Rogers, Adrian Hunter, linux-perf-users, bpf On Tue, Dec 5, 2023 at 10:17 AM Marco Elver <elver@google.com> wrote: > > On Tue, 5 Dec 2023 at 19:07, Namhyung Kim <namhyung@kernel.org> wrote: > > If we want to handle returning 0 from bpf as if the event didn't > > happen, I think SIGTRAP and event_limit logic should be done > > after the overflow handler depending on pending_kill or something. > > I'm not sure which kernel version this is for, but in recent kernels, > the SIGTRAP logic was changed to no longer "abuse" event_limit, and > uses its own "pending_sigtrap" + "pending_work" (on reschedule > transitions). Oh, I didn't mean SIGTRAP and event_limit together. Maybe they have an issue separately. Thanks, Namhyung ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] perf/bpf: Allow a bpf program to suppress I/O signals. 2023-12-05 18:07 ` Namhyung Kim 2023-12-05 18:16 ` Marco Elver @ 2023-12-05 19:19 ` Kyle Huey 1 sibling, 0 replies; 8+ messages in thread From: Kyle Huey @ 2023-12-05 19:19 UTC (permalink / raw) To: Namhyung Kim Cc: Jiri Olsa, Andrii Nakryiko, Kyle Huey, linux-kernel, Robert O'Callahan, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Ian Rogers, Adrian Hunter, linux-perf-users, bpf, Marco Elver On Tue, Dec 5, 2023 at 10:07 AM Namhyung Kim <namhyung@kernel.org> wrote: > > Hello, > > Add Marco Elver to CC. > > On Tue, Dec 5, 2023 at 3:16 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > On Mon, Dec 04, 2023 at 02:18:49PM -0800, Andrii Nakryiko wrote: > > > On Mon, Dec 4, 2023 at 12:14 PM Kyle Huey <me@kylehuey.com> wrote: > > > > > > > > Returning zero from a bpf program attached to a perf event already > > > > suppresses any data output. This allows it to suppress I/O availability > > > > signals too. > > > > > > make sense, just one question below > > > > > > > > > > > Signed-off-by: Kyle Huey <khuey@kylehuey.com> > > > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > > > > > --- > > > > kernel/events/core.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > > > index b704d83a28b2..34d7b19d45eb 100644 > > > > --- a/kernel/events/core.c > > > > +++ b/kernel/events/core.c > > > > @@ -10417,8 +10417,10 @@ static void bpf_overflow_handler(struct perf_event *event, > > > > rcu_read_unlock(); > > > > out: > > > > __this_cpu_dec(bpf_prog_active); > > > > - if (!ret) > > > > + if (!ret) { > > > > + event->pending_kill = 0; > > > > return; > > > > + } > > > > > > What's the distinction between event->pending_kill and > > > event->pending_wakeup? Should we do something about pending_wakeup? > > > Asking out of complete ignorance of all these perf specifics. > > > > > > > I think zeroing pending_kill is enough.. when it's set the perf code > > sets pending_wakeup to call perf_event_wakeup in irq code that wakes > > up event's ring buffer readers and sends sigio if pending_kill is set > > Right, IIUC pending_wakeup is set by the ring buffer code when > a task is waiting for events and it gets enough events (watermark). > So I think it's good for ring buffer to manage the pending_wakeup. > > And pending_kill is set when a task wants a signal delivery even > without getting enough events. Clearing pending_kill looks ok > to suppress normal signals but I'm not sure if it's ok for SIGTRAP. > > If we want to handle returning 0 from bpf as if the event didn't > happen, I think SIGTRAP and event_limit logic should be done > after the overflow handler depending on pending_kill or something. Hmm, yes, perhaps. The SIGTRAP thing (which I was previously unaware of) would actually be more useful to us than an I/O signal. I am slightly wary that event_limit appears to have no tests in the kernel tree. - Kyle > Thanks, > Namhyung ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-12-05 19:19 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20231204201406.341074-1-khuey@kylehuey.com>
2023-12-04 20:14 ` [PATCH 1/2] perf/bpf: Allow a bpf program to suppress I/O signals Kyle Huey
2023-12-04 22:18 ` Andrii Nakryiko
2023-12-05 11:16 ` Jiri Olsa
2023-12-05 18:07 ` Namhyung Kim
2023-12-05 18:16 ` Marco Elver
2023-12-05 18:23 ` Kyle Huey
2023-12-05 18:26 ` Namhyung Kim
2023-12-05 19:19 ` Kyle Huey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).