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

[Namhyung Kim <namhyung@kernel.org>]

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
>