Re: [RFC PATCH v1] perf trace: Mitigate failures in parallel perf trace instances

Re: [RFC PATCH v1] perf trace: Mitigate failures in parallel perf trace instances

[Steven Rostedt]

On Fri, 30 May 2025 17:00:38 -0700
Howard Chu <howardchu95@gmail.com> wrote:

> Hello Namhyung,
> 
> On Fri, May 30, 2025 at 4:37 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hello,
> >
> > (Adding tracing folks)  
> 
> (That's so convenient wow)

Shouldn't the BPF folks be more relevant. I don't see any of the tracing
code involved here.

> 
> >
> > On Wed, May 28, 2025 at 11:55:36PM -0700, Howard Chu wrote:  
> > > perf trace utilizes the tracepoint utility, the only filter in perf
> > > trace is a filter on syscall type. For example, if perf traces only
> > > openat, then it filters all the other syscalls, such as readlinkat,
> > > readv, etc.
> > >
> > > This filtering is flawed. Consider this case: two perf trace
> > > instances are running at the same time, trace instance A tracing
> > > readlinkat, trace instance B tracing openat. When an openat syscall
> > > enters, it triggers both BPF programs (sys_enter) in both perf trace
> > > instances, these kernel functions will be executed:
> > >
> > > perf_syscall_enter
> > >   perf_call_bpf_enter
> > >     trace_call_bpf

This is in bpf_trace.c (BPF related, not tracing related).

-- Steve


> > >       bpf_prog_run_array
> > >
> > > In bpf_prog_run_array:
> > > ~~~
> > > while ((prog = READ_ONCE(item->prog))) {
> > >       run_ctx.bpf_cookie = item->bpf_cookie;
> > >       ret &= run_prog(prog, ctx);
> > >       item++;
> > > }
> > > ~~~
> > >
> > > I'm not a BPF expert, but by tinkering I found that if one of the BPF
> > > programs returns 0, there will be no tracepoint sample. That is,
> > >
> > > (Is there a sample?) = ProgRetA & ProgRetB & ProgRetC
> > >
> > > Where ProgRetA is the return value of one of the BPF programs in the BPF
> > > program array.
> > >
> > > Go back to the case, when two perf trace instances are tracing two
> > > different syscalls, again, A is tracing readlinkat, B is tracing openat,
> > > when an openat syscall enters, it triggers the sys_enter program in
> > > instance A, call it ProgA, and the sys_enter program in instance B,
> > > ProgB, now ProgA will return 0 because ProgA cares about readlinkat only,
> > > even though ProgB returns 1; (Is there a sample?) = ProgRetA (0) &
> > > ProgRetB (1) = 0. So there won't be a tracepoint sample in B's output,
> > > when there really should be one.  
> >
> > Sounds like a bug.  I think it should run bpf programs attached to the
> > current perf_event only.  Isn't it the case for tracepoint + perf + bpf?  
> 
> I really can't answer that question.
> 
> >  
> > >
> > > I also want to point out that openat and readlinkat have augmented
> > > output, so my example might not be accurate, but it does explain the
> > > current perf-trace-in-parallel dilemma.
> > >
> > > Now for augmented output, it is different. When it calls
> > > bpf_perf_event_output, there is a sample. There won't be no ProgRetA &
> > > ProgRetB... thing. So I will send another RFC patch to enable
> > > parallelism using this feature. Also, augmented_output creates a sample
> > > on it's own, so returning 1 will create a duplicated sample, when
> > > augmented, just return 0 instead.  
> >
> > Yes, it's bpf-output and tracepoint respectively.  Maybe we should
> > always return 1 not to drop syscalls unintentionally and perf can
> > discard duplicated samples.  
> 
> I like this.
> 
> >
> > Another approach would be return 0 always and use bpf-output for
> > unaugmented syscalls too.  But I'm afraid it'd affect other perf tools
> > using tracepoints.  
> 
> Yep.
> 
> >  
> > >
> > > Is this approach perfect? Absolutely not, there will likely be some
> > > performance overhead on the kernel side. It is just a quick dirty fix
> > > that makes perf trace run in parallel without failing. This patch is an
> > > explanation on the reason of failures and possibly, a link used in a
> > > nack comment.  
> >
> > Thanks for your work, but I'm afraid it'd still miss some syscalls as it
> > returns 0 sometimes.  
> 
> My bad... For example this:
> 
> if (pid_filter__has(&pids_filtered, getpid()))
>    return 0;
> 
> This patch is practically meaningless, but it passes the parallel tests.
> 
> Thanks,
> Howard

Re: [RFC PATCH v1] perf trace: Mitigate failures in parallel perf trace instances

[Namhyung Kim]

Hi Steve,

On Mon, Jun 02, 2025 at 06:17:43PM -0400, Steven Rostedt wrote:
> On Fri, 30 May 2025 17:00:38 -0700
> Howard Chu <howardchu95@gmail.com> wrote:
> 
> > Hello Namhyung,
> > 
> > On Fri, May 30, 2025 at 4:37 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > Hello,
> > >
> > > (Adding tracing folks)  
> > 
> > (That's so convenient wow)
> 
> Shouldn't the BPF folks be more relevant. I don't see any of the tracing
> code involved here.

Yep, they are CC'ed too.  And sorry I thought you are involved in this
part of the code.

> 
> > 
> > >
> > > On Wed, May 28, 2025 at 11:55:36PM -0700, Howard Chu wrote:  
> > > > perf trace utilizes the tracepoint utility, the only filter in perf
> > > > trace is a filter on syscall type. For example, if perf traces only
> > > > openat, then it filters all the other syscalls, such as readlinkat,
> > > > readv, etc.
> > > >
> > > > This filtering is flawed. Consider this case: two perf trace
> > > > instances are running at the same time, trace instance A tracing
> > > > readlinkat, trace instance B tracing openat. When an openat syscall
> > > > enters, it triggers both BPF programs (sys_enter) in both perf trace
> > > > instances, these kernel functions will be executed:
> > > >
> > > > perf_syscall_enter
> > > >   perf_call_bpf_enter
> > > >     trace_call_bpf
> 
> This is in bpf_trace.c (BPF related, not tracing related).
 
Ok, noted.

Thanks,
Namhyung

> 
> > > >       bpf_prog_run_array
> > > >
> > > > In bpf_prog_run_array:
> > > > ~~~
> > > > while ((prog = READ_ONCE(item->prog))) {
> > > >       run_ctx.bpf_cookie = item->bpf_cookie;
> > > >       ret &= run_prog(prog, ctx);
> > > >       item++;
> > > > }
> > > > ~~~
> > > >
> > > > I'm not a BPF expert, but by tinkering I found that if one of the BPF
> > > > programs returns 0, there will be no tracepoint sample. That is,
> > > >
> > > > (Is there a sample?) = ProgRetA & ProgRetB & ProgRetC
> > > >
> > > > Where ProgRetA is the return value of one of the BPF programs in the BPF
> > > > program array.
> > > >
> > > > Go back to the case, when two perf trace instances are tracing two
> > > > different syscalls, again, A is tracing readlinkat, B is tracing openat,
> > > > when an openat syscall enters, it triggers the sys_enter program in
> > > > instance A, call it ProgA, and the sys_enter program in instance B,
> > > > ProgB, now ProgA will return 0 because ProgA cares about readlinkat only,
> > > > even though ProgB returns 1; (Is there a sample?) = ProgRetA (0) &
> > > > ProgRetB (1) = 0. So there won't be a tracepoint sample in B's output,
> > > > when there really should be one.  
> > >
> > > Sounds like a bug.  I think it should run bpf programs attached to the
> > > current perf_event only.  Isn't it the case for tracepoint + perf + bpf?  
> > 
> > I really can't answer that question.
> > 
> > >  
> > > >
> > > > I also want to point out that openat and readlinkat have augmented
> > > > output, so my example might not be accurate, but it does explain the
> > > > current perf-trace-in-parallel dilemma.
> > > >
> > > > Now for augmented output, it is different. When it calls
> > > > bpf_perf_event_output, there is a sample. There won't be no ProgRetA &
> > > > ProgRetB... thing. So I will send another RFC patch to enable
> > > > parallelism using this feature. Also, augmented_output creates a sample
> > > > on it's own, so returning 1 will create a duplicated sample, when
> > > > augmented, just return 0 instead.  
> > >
> > > Yes, it's bpf-output and tracepoint respectively.  Maybe we should
> > > always return 1 not to drop syscalls unintentionally and perf can
> > > discard duplicated samples.  
> > 
> > I like this.
> > 
> > >
> > > Another approach would be return 0 always and use bpf-output for
> > > unaugmented syscalls too.  But I'm afraid it'd affect other perf tools
> > > using tracepoints.  
> > 
> > Yep.
> > 
> > >  
> > > >
> > > > Is this approach perfect? Absolutely not, there will likely be some
> > > > performance overhead on the kernel side. It is just a quick dirty fix
> > > > that makes perf trace run in parallel without failing. This patch is an
> > > > explanation on the reason of failures and possibly, a link used in a
> > > > nack comment.  
> > >
> > > Thanks for your work, but I'm afraid it'd still miss some syscalls as it
> > > returns 0 sometimes.  
> > 
> > My bad... For example this:
> > 
> > if (pid_filter__has(&pids_filtered, getpid()))
> >    return 0;
> > 
> > This patch is practically meaningless, but it passes the parallel tests.
> > 
> > Thanks,
> > Howard
> 

Re: [RFC PATCH v1] perf trace: Mitigate failures in parallel perf trace instances

[Jiri Olsa]

On Mon, Jun 02, 2025 at 06:17:43PM -0400, Steven Rostedt wrote:
> On Fri, 30 May 2025 17:00:38 -0700
> Howard Chu <howardchu95@gmail.com> wrote:
> 
> > Hello Namhyung,
> > 
> > On Fri, May 30, 2025 at 4:37 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > Hello,
> > >
> > > (Adding tracing folks)  
> > 
> > (That's so convenient wow)
> 
> Shouldn't the BPF folks be more relevant. I don't see any of the tracing
> code involved here.
> 
> > 
> > >
> > > On Wed, May 28, 2025 at 11:55:36PM -0700, Howard Chu wrote:  
> > > > perf trace utilizes the tracepoint utility, the only filter in perf
> > > > trace is a filter on syscall type. For example, if perf traces only
> > > > openat, then it filters all the other syscalls, such as readlinkat,
> > > > readv, etc.
> > > >
> > > > This filtering is flawed. Consider this case: two perf trace
> > > > instances are running at the same time, trace instance A tracing
> > > > readlinkat, trace instance B tracing openat. When an openat syscall
> > > > enters, it triggers both BPF programs (sys_enter) in both perf trace
> > > > instances, these kernel functions will be executed:
> > > >
> > > > perf_syscall_enter
> > > >   perf_call_bpf_enter
> > > >     trace_call_bpf
> 
> This is in bpf_trace.c (BPF related, not tracing related).
> 
> -- Steve
> 
> 
> > > >       bpf_prog_run_array
> > > >
> > > > In bpf_prog_run_array:
> > > > ~~~
> > > > while ((prog = READ_ONCE(item->prog))) {
> > > >       run_ctx.bpf_cookie = item->bpf_cookie;
> > > >       ret &= run_prog(prog, ctx);
> > > >       item++;
> > > > }
> > > > ~~~
> > > >
> > > > I'm not a BPF expert, but by tinkering I found that if one of the BPF
> > > > programs returns 0, there will be no tracepoint sample. That is,
> > > >
> > > > (Is there a sample?) = ProgRetA & ProgRetB & ProgRetC
> > > >
> > > > Where ProgRetA is the return value of one of the BPF programs in the BPF
> > > > program array.
> > > >
> > > > Go back to the case, when two perf trace instances are tracing two
> > > > different syscalls, again, A is tracing readlinkat, B is tracing openat,
> > > > when an openat syscall enters, it triggers the sys_enter program in
> > > > instance A, call it ProgA, and the sys_enter program in instance B,
> > > > ProgB, now ProgA will return 0 because ProgA cares about readlinkat only,
> > > > even though ProgB returns 1; (Is there a sample?) = ProgRetA (0) &
> > > > ProgRetB (1) = 0. So there won't be a tracepoint sample in B's output,
> > > > when there really should be one.  
> > >
> > > Sounds like a bug.  I think it should run bpf programs attached to the
> > > current perf_event only.  Isn't it the case for tracepoint + perf + bpf?  
> > 
> > I really can't answer that question.

bpf programs for tracepoint are executed before the perf event specific
check/trigger in perf_trace_run_bpf_submit

bpf programs array is part of struct trace_event_call so it's global per
tracepoint, not per perf event

IIRC perf trace needs the perf event sample and the bpf program is there
to do the filter and some other related stuff?

if that's the case I wonder we could switch bpf_prog_run_array logic
to be permissive like below, and perhaps make that as tracepoint specific
change, because bpf_prog_run_array is used in other place

or make it somehow configurable.. otherwise I fear that might break existing
use cases.. FWIW bpf ci tests [1] survived the change below

jirka


[1] https://github.com/kernel-patches/bpf/pull/9044

---
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 5b25d278409b..4ca8afe0217c 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2218,12 +2218,12 @@ bpf_prog_run_array(const struct bpf_prog_array *array,
 	const struct bpf_prog *prog;
 	struct bpf_run_ctx *old_run_ctx;
 	struct bpf_trace_run_ctx run_ctx;
-	u32 ret = 1;
+	u32 ret = 0;
 
 	RCU_LOCKDEP_WARN(!rcu_read_lock_held(), "no rcu lock held");
 
 	if (unlikely(!array))
-		return ret;
+		return 1;
 
 	run_ctx.is_uprobe = false;
 
@@ -2232,7 +2232,7 @@ bpf_prog_run_array(const struct bpf_prog_array *array,
 	item = &array->items[0];
 	while ((prog = READ_ONCE(item->prog))) {
 		run_ctx.bpf_cookie = item->bpf_cookie;
-		ret &= run_prog(prog, ctx);
+		ret |= run_prog(prog, ctx);
 		item++;
 	}
 	bpf_reset_run_ctx(old_run_ctx);

Re: [RFC PATCH v1] perf trace: Mitigate failures in parallel perf trace instances

[Alexei Starovoitov]

On Wed, Jun 4, 2025 at 3:25 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Mon, Jun 02, 2025 at 06:17:43PM -0400, Steven Rostedt wrote:
> > On Fri, 30 May 2025 17:00:38 -0700
> > Howard Chu <howardchu95@gmail.com> wrote:
> >
> > > Hello Namhyung,
> > >
> > > On Fri, May 30, 2025 at 4:37 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > Hello,
> > > >
> > > > (Adding tracing folks)
> > >
> > > (That's so convenient wow)
> >
> > Shouldn't the BPF folks be more relevant. I don't see any of the tracing
> > code involved here.
> >
> > >
> > > >
> > > > On Wed, May 28, 2025 at 11:55:36PM -0700, Howard Chu wrote:
> > > > > perf trace utilizes the tracepoint utility, the only filter in perf
> > > > > trace is a filter on syscall type. For example, if perf traces only
> > > > > openat, then it filters all the other syscalls, such as readlinkat,
> > > > > readv, etc.
> > > > >
> > > > > This filtering is flawed. Consider this case: two perf trace
> > > > > instances are running at the same time, trace instance A tracing
> > > > > readlinkat, trace instance B tracing openat. When an openat syscall
> > > > > enters, it triggers both BPF programs (sys_enter) in both perf trace
> > > > > instances, these kernel functions will be executed:
> > > > >
> > > > > perf_syscall_enter
> > > > >   perf_call_bpf_enter
> > > > >     trace_call_bpf
> >
> > This is in bpf_trace.c (BPF related, not tracing related).
> >
> > -- Steve
> >
> >
> > > > >       bpf_prog_run_array
> > > > >
> > > > > In bpf_prog_run_array:
> > > > > ~~~
> > > > > while ((prog = READ_ONCE(item->prog))) {
> > > > >       run_ctx.bpf_cookie = item->bpf_cookie;
> > > > >       ret &= run_prog(prog, ctx);
> > > > >       item++;
> > > > > }
> > > > > ~~~
> > > > >
> > > > > I'm not a BPF expert, but by tinkering I found that if one of the BPF
> > > > > programs returns 0, there will be no tracepoint sample. That is,
> > > > >
> > > > > (Is there a sample?) = ProgRetA & ProgRetB & ProgRetC
> > > > >
> > > > > Where ProgRetA is the return value of one of the BPF programs in the BPF
> > > > > program array.
> > > > >
> > > > > Go back to the case, when two perf trace instances are tracing two
> > > > > different syscalls, again, A is tracing readlinkat, B is tracing openat,
> > > > > when an openat syscall enters, it triggers the sys_enter program in
> > > > > instance A, call it ProgA, and the sys_enter program in instance B,
> > > > > ProgB, now ProgA will return 0 because ProgA cares about readlinkat only,
> > > > > even though ProgB returns 1; (Is there a sample?) = ProgRetA (0) &
> > > > > ProgRetB (1) = 0. So there won't be a tracepoint sample in B's output,
> > > > > when there really should be one.
> > > >
> > > > Sounds like a bug.  I think it should run bpf programs attached to the
> > > > current perf_event only.  Isn't it the case for tracepoint + perf + bpf?
> > >
> > > I really can't answer that question.
>
> bpf programs for tracepoint are executed before the perf event specific
> check/trigger in perf_trace_run_bpf_submit
>
> bpf programs array is part of struct trace_event_call so it's global per
> tracepoint, not per perf event

right.
looks like perf is attaching two different progs to the same sys_enter
tracepoint and one of them returns 0.
That's expected behavior.
The rule is all-yes-is-yes, any-no-is-no.
We apply this logic to majority (if not all) bpf prog return values.

> IIRC perf trace needs the perf event sample and the bpf program is there
> to do the filter and some other related stuff?
>
> if that's the case I wonder we could switch bpf_prog_run_array logic
> to be permissive like below, and perhaps make that as tracepoint specific
> change, because bpf_prog_run_array is used in other place

No. That might break somebody and we don't want to deviate from the rule.

> or make it somehow configurable.. otherwise I fear that might break existing
> use cases.. FWIW bpf ci tests [1] survived the change below
>
> jirka
>
>
> [1] https://github.com/kernel-patches/bpf/pull/9044
>
> ---
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 5b25d278409b..4ca8afe0217c 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2218,12 +2218,12 @@ bpf_prog_run_array(const struct bpf_prog_array *array,
>         const struct bpf_prog *prog;
>         struct bpf_run_ctx *old_run_ctx;
>         struct bpf_trace_run_ctx run_ctx;
> -       u32 ret = 1;
> +       u32 ret = 0;
>
>         RCU_LOCKDEP_WARN(!rcu_read_lock_held(), "no rcu lock held");
>
>         if (unlikely(!array))
> -               return ret;
> +               return 1;
>
>         run_ctx.is_uprobe = false;
>
> @@ -2232,7 +2232,7 @@ bpf_prog_run_array(const struct bpf_prog_array *array,
>         item = &array->items[0];
>         while ((prog = READ_ONCE(item->prog))) {
>                 run_ctx.bpf_cookie = item->bpf_cookie;
> -               ret &= run_prog(prog, ctx);
> +               ret |= run_prog(prog, ctx);
>                 item++;
>         }
>         bpf_reset_run_ctx(old_run_ctx);
>

Re: [RFC PATCH v1] perf trace: Mitigate failures in parallel perf trace instances

[Howard Chu]

Hi Alexei,

On Fri, Jun 6, 2025 at 11:27 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Jun 4, 2025 at 3:25 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Mon, Jun 02, 2025 at 06:17:43PM -0400, Steven Rostedt wrote:
> > > On Fri, 30 May 2025 17:00:38 -0700
> > > Howard Chu <howardchu95@gmail.com> wrote:
> > >
> > > > Hello Namhyung,
> > > >
> > > > On Fri, May 30, 2025 at 4:37 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > (Adding tracing folks)
> > > >
> > > > (That's so convenient wow)
> > >
> > > Shouldn't the BPF folks be more relevant. I don't see any of the tracing
> > > code involved here.
> > >
> > > >
> > > > >
> > > > > On Wed, May 28, 2025 at 11:55:36PM -0700, Howard Chu wrote:
> > > > > > perf trace utilizes the tracepoint utility, the only filter in perf
> > > > > > trace is a filter on syscall type. For example, if perf traces only
> > > > > > openat, then it filters all the other syscalls, such as readlinkat,
> > > > > > readv, etc.
> > > > > >
> > > > > > This filtering is flawed. Consider this case: two perf trace
> > > > > > instances are running at the same time, trace instance A tracing
> > > > > > readlinkat, trace instance B tracing openat. When an openat syscall
> > > > > > enters, it triggers both BPF programs (sys_enter) in both perf trace
> > > > > > instances, these kernel functions will be executed:
> > > > > >
> > > > > > perf_syscall_enter
> > > > > >   perf_call_bpf_enter
> > > > > >     trace_call_bpf
> > >
> > > This is in bpf_trace.c (BPF related, not tracing related).
> > >
> > > -- Steve
> > >
> > >
> > > > > >       bpf_prog_run_array
> > > > > >
> > > > > > In bpf_prog_run_array:
> > > > > > ~~~
> > > > > > while ((prog = READ_ONCE(item->prog))) {
> > > > > >       run_ctx.bpf_cookie = item->bpf_cookie;
> > > > > >       ret &= run_prog(prog, ctx);
> > > > > >       item++;
> > > > > > }
> > > > > > ~~~
> > > > > >
> > > > > > I'm not a BPF expert, but by tinkering I found that if one of the BPF
> > > > > > programs returns 0, there will be no tracepoint sample. That is,
> > > > > >
> > > > > > (Is there a sample?) = ProgRetA & ProgRetB & ProgRetC
> > > > > >
> > > > > > Where ProgRetA is the return value of one of the BPF programs in the BPF
> > > > > > program array.
> > > > > >
> > > > > > Go back to the case, when two perf trace instances are tracing two
> > > > > > different syscalls, again, A is tracing readlinkat, B is tracing openat,
> > > > > > when an openat syscall enters, it triggers the sys_enter program in
> > > > > > instance A, call it ProgA, and the sys_enter program in instance B,
> > > > > > ProgB, now ProgA will return 0 because ProgA cares about readlinkat only,
> > > > > > even though ProgB returns 1; (Is there a sample?) = ProgRetA (0) &
> > > > > > ProgRetB (1) = 0. So there won't be a tracepoint sample in B's output,
> > > > > > when there really should be one.
> > > > >
> > > > > Sounds like a bug.  I think it should run bpf programs attached to the
> > > > > current perf_event only.  Isn't it the case for tracepoint + perf + bpf?
> > > >
> > > > I really can't answer that question.
> >
> > bpf programs for tracepoint are executed before the perf event specific
> > check/trigger in perf_trace_run_bpf_submit
> >
> > bpf programs array is part of struct trace_event_call so it's global per
> > tracepoint, not per perf event
>
> right.
> looks like perf is attaching two different progs to the same sys_enter
> tracepoint and one of them returns 0.
> That's expected behavior.
> The rule is all-yes-is-yes, any-no-is-no.
> We apply this logic to majority (if not all) bpf prog return values.
>
> > IIRC perf trace needs the perf event sample and the bpf program is there
> > to do the filter and some other related stuff?
> >
> > if that's the case I wonder we could switch bpf_prog_run_array logic
> > to be permissive like below, and perhaps make that as tracepoint specific
> > change, because bpf_prog_run_array is used in other place
>
> No. That might break somebody and we don't want to deviate from the rule.

Makes sense. Thanks.

Thanks,
Howard

Re: [RFC PATCH v1] perf trace: Mitigate failures in parallel perf trace instances

[Howard Chu]

Hi Jiri,

On Wed, Jun 4, 2025 at 3:25 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Mon, Jun 02, 2025 at 06:17:43PM -0400, Steven Rostedt wrote:
> > On Fri, 30 May 2025 17:00:38 -0700
> > Howard Chu <howardchu95@gmail.com> wrote:
> >
> > > Hello Namhyung,
> > >
> > > On Fri, May 30, 2025 at 4:37 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > Hello,
> > > >
> > > > (Adding tracing folks)
> > >
> > > (That's so convenient wow)
> >
> > Shouldn't the BPF folks be more relevant. I don't see any of the tracing
> > code involved here.
> >
> > >
> > > >
> > > > On Wed, May 28, 2025 at 11:55:36PM -0700, Howard Chu wrote:
> > > > > perf trace utilizes the tracepoint utility, the only filter in perf
> > > > > trace is a filter on syscall type. For example, if perf traces only
> > > > > openat, then it filters all the other syscalls, such as readlinkat,
> > > > > readv, etc.
> > > > >
> > > > > This filtering is flawed. Consider this case: two perf trace
> > > > > instances are running at the same time, trace instance A tracing
> > > > > readlinkat, trace instance B tracing openat. When an openat syscall
> > > > > enters, it triggers both BPF programs (sys_enter) in both perf trace
> > > > > instances, these kernel functions will be executed:
> > > > >
> > > > > perf_syscall_enter
> > > > >   perf_call_bpf_enter
> > > > >     trace_call_bpf
> >
> > This is in bpf_trace.c (BPF related, not tracing related).
> >
> > -- Steve
> >
> >
> > > > >       bpf_prog_run_array
> > > > >
> > > > > In bpf_prog_run_array:
> > > > > ~~~
> > > > > while ((prog = READ_ONCE(item->prog))) {
> > > > >       run_ctx.bpf_cookie = item->bpf_cookie;
> > > > >       ret &= run_prog(prog, ctx);
> > > > >       item++;
> > > > > }
> > > > > ~~~
> > > > >
> > > > > I'm not a BPF expert, but by tinkering I found that if one of the BPF
> > > > > programs returns 0, there will be no tracepoint sample. That is,
> > > > >
> > > > > (Is there a sample?) = ProgRetA & ProgRetB & ProgRetC
> > > > >
> > > > > Where ProgRetA is the return value of one of the BPF programs in the BPF
> > > > > program array.
> > > > >
> > > > > Go back to the case, when two perf trace instances are tracing two
> > > > > different syscalls, again, A is tracing readlinkat, B is tracing openat,
> > > > > when an openat syscall enters, it triggers the sys_enter program in
> > > > > instance A, call it ProgA, and the sys_enter program in instance B,
> > > > > ProgB, now ProgA will return 0 because ProgA cares about readlinkat only,
> > > > > even though ProgB returns 1; (Is there a sample?) = ProgRetA (0) &
> > > > > ProgRetB (1) = 0. So there won't be a tracepoint sample in B's output,
> > > > > when there really should be one.
> > > >
> > > > Sounds like a bug.  I think it should run bpf programs attached to the
> > > > current perf_event only.  Isn't it the case for tracepoint + perf + bpf?
> > >
> > > I really can't answer that question.
>
> bpf programs for tracepoint are executed before the perf event specific
> check/trigger in perf_trace_run_bpf_submit
>
> bpf programs array is part of struct trace_event_call so it's global per
> tracepoint, not per perf event
>
> IIRC perf trace needs the perf event sample and the bpf program is there
> to do the filter and some other related stuff?

Actually perf trace relies on BPF programs to produce samples and do
filtering, but it can also turn BPF off and use tracepoints only.

>
> if that's the case I wonder we could switch bpf_prog_run_array logic
> to be permissive like below, and perhaps make that as tracepoint specific
> change, because bpf_prog_run_array is used in other place

Thanks but as Alexei said this may break those who depend on it.

>
> or make it somehow configurable.. otherwise I fear that might break existing
> use cases.. FWIW bpf ci tests [1] survived the change below
>
> jirka
>
>
> [1] https://github.com/kernel-patches/bpf/pull/9044
>
> ---
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 5b25d278409b..4ca8afe0217c 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2218,12 +2218,12 @@ bpf_prog_run_array(const struct bpf_prog_array *array,
>         const struct bpf_prog *prog;
>         struct bpf_run_ctx *old_run_ctx;
>         struct bpf_trace_run_ctx run_ctx;
> -       u32 ret = 1;
> +       u32 ret = 0;
>
>         RCU_LOCKDEP_WARN(!rcu_read_lock_held(), "no rcu lock held");
>
>         if (unlikely(!array))
> -               return ret;
> +               return 1;
>
>         run_ctx.is_uprobe = false;
>
> @@ -2232,7 +2232,7 @@ bpf_prog_run_array(const struct bpf_prog_array *array,
>         item = &array->items[0];
>         while ((prog = READ_ONCE(item->prog))) {
>                 run_ctx.bpf_cookie = item->bpf_cookie;
> -               ret &= run_prog(prog, ctx);
> +               ret |= run_prog(prog, ctx);
>                 item++;
>         }
>         bpf_reset_run_ctx(old_run_ctx);

Thank you so much for looking into this.

Thanks,
Howard

Re: [RFC PATCH v1] perf trace: Mitigate failures in parallel perf trace instances

[Namhyung Kim]

Hello,

Sorry for the late reply.

On Mon, Jun 09, 2025 at 11:38:00AM -0700, Howard Chu wrote:
> Hi Jiri,
> 
> On Wed, Jun 4, 2025 at 3:25 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Mon, Jun 02, 2025 at 06:17:43PM -0400, Steven Rostedt wrote:
> > > On Fri, 30 May 2025 17:00:38 -0700
> > > Howard Chu <howardchu95@gmail.com> wrote:
> > >
> > > > Hello Namhyung,
> > > >
> > > > On Fri, May 30, 2025 at 4:37 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > > > On Wed, May 28, 2025 at 11:55:36PM -0700, Howard Chu wrote:
> > > > > > perf trace utilizes the tracepoint utility, the only filter in perf
> > > > > > trace is a filter on syscall type. For example, if perf traces only
> > > > > > openat, then it filters all the other syscalls, such as readlinkat,
> > > > > > readv, etc.
> > > > > >
> > > > > > This filtering is flawed. Consider this case: two perf trace
> > > > > > instances are running at the same time, trace instance A tracing
> > > > > > readlinkat, trace instance B tracing openat. When an openat syscall
> > > > > > enters, it triggers both BPF programs (sys_enter) in both perf trace
> > > > > > instances, these kernel functions will be executed:
> > > > > >
> > > > > > perf_syscall_enter
> > > > > >   perf_call_bpf_enter
> > > > > >     trace_call_bpf
> > > > > >       bpf_prog_run_array
> > > > > >
> > > > > > In bpf_prog_run_array:
> > > > > > ~~~
> > > > > > while ((prog = READ_ONCE(item->prog))) {
> > > > > >       run_ctx.bpf_cookie = item->bpf_cookie;
> > > > > >       ret &= run_prog(prog, ctx);
> > > > > >       item++;
> > > > > > }
> > > > > > ~~~
> > > > > >
> > > > > > I'm not a BPF expert, but by tinkering I found that if one of the BPF
> > > > > > programs returns 0, there will be no tracepoint sample. That is,
> > > > > >
> > > > > > (Is there a sample?) = ProgRetA & ProgRetB & ProgRetC
> > > > > >
> > > > > > Where ProgRetA is the return value of one of the BPF programs in the BPF
> > > > > > program array.
> > > > > >
> > > > > > Go back to the case, when two perf trace instances are tracing two
> > > > > > different syscalls, again, A is tracing readlinkat, B is tracing openat,
> > > > > > when an openat syscall enters, it triggers the sys_enter program in
> > > > > > instance A, call it ProgA, and the sys_enter program in instance B,
> > > > > > ProgB, now ProgA will return 0 because ProgA cares about readlinkat only,
> > > > > > even though ProgB returns 1; (Is there a sample?) = ProgRetA (0) &
> > > > > > ProgRetB (1) = 0. So there won't be a tracepoint sample in B's output,
> > > > > > when there really should be one.
> > > > >
> > > > > Sounds like a bug.  I think it should run bpf programs attached to the
> > > > > current perf_event only.  Isn't it the case for tracepoint + perf + bpf?
> > > >
> > > > I really can't answer that question.
> >
> > bpf programs for tracepoint are executed before the perf event specific
> > check/trigger in perf_trace_run_bpf_submit
> >
> > bpf programs array is part of struct trace_event_call so it's global per
> > tracepoint, not per perf event

Right, I think we need a way to attach a BPF program to perf_event (as
an overflow handler), not to the trace_event_call, when it comes to a
tracepoint event.  So that it can only affect behaviors of the calling
thread.  It would access the trace data as sample raw data from ctx.

Maybe it needs new link_create flags and requires BPF_PROG_TYPE_PERF_EVENT.

Wdyt?

Thanks,
Namhyung