Re: [PATCH bpf-next v4 2/2] bpf: Hold the perf callchain entry until used completely

[Tao Chen]

在 2025/11/6 14:20, Yonghong Song 写道:
> On 11/5/25 9:12 PM, Tao Chen wrote:
> > 在 2025/11/6 06:16, Yonghong Song 写道:
> > > On 10/28/25 9:25 AM, Tao Chen wrote:
> > > > As Alexei noted, get_perf_callchain() return values may be reused
> > > > if a task is preempted after the BPF program enters migrate disable
> > > > mode. The perf_callchain_entres has a small stack of entries, and
> > > > we can reuse it as follows:
> > > >
> > > > 1. get the perf callchain entry
> > > > 2. BPF use...
> > > > 3. put the perf callchain entry
> > > >
> > > > Signed-off-by: Tao Chen <chen.dylane@xxxxxxxxx>
> > > > ---
> > > >   kernel/bpf/stackmap.c | 61 +++++++++++++++++++++++++++++++++ 
> > > > +---------
> > > >   1 file changed, 48 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> > > > index e28b35c7e0b..70d38249083 100644
> > > > --- a/kernel/bpf/stackmap.c
> > > > +++ b/kernel/bpf/stackmap.c
> > > > @@ -188,13 +188,12 @@ static void 
> > > > stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
> > > >   }
> > > >   static struct perf_callchain_entry *
> > > > -get_callchain_entry_for_task(struct task_struct *task, u32 max_depth)
> > > > +get_callchain_entry_for_task(int *rctx, struct task_struct *task, 
> > > > u32 max_depth)
> > > >   {
> > > >   #ifdef CONFIG_STACKTRACE
> > > >       struct perf_callchain_entry *entry;
> > > > -    int rctx;
> > > > -    entry = get_callchain_entry(&rctx);
> > > > +    entry = get_callchain_entry(rctx);
> > > >       if (!entry)
> > > >           return NULL;
> > > > @@ -216,8 +215,6 @@ get_callchain_entry_for_task(struct task_struct 
> > > > *task, u32 max_depth)
> > > >               to[i] = (u64)(from[i]);
> > > >       }
> > > > -    put_callchain_entry(rctx);
> > > > -
> > > >       return entry;
> > > >   #else /* CONFIG_STACKTRACE */
> > > >       return NULL;
> > > > @@ -297,6 +294,31 @@ static long __bpf_get_stackid(struct bpf_map *map,
> > > >       return id;
> > > >   }
> > > > +static struct perf_callchain_entry *
> > > > +bpf_get_perf_callchain(int *rctx, struct pt_regs *regs, bool 
> > > > kernel, bool user,
> > > > +               int max_stack, bool crosstask)
> > > > +{
> > > > +    struct perf_callchain_entry_ctx ctx;
> > > > +    struct perf_callchain_entry *entry;
> > > > +
> > > > +    entry = get_callchain_entry(rctx);
> > >
> > > I think this may not work. Let us say we have two bpf programs
> > > both pinned to a particular cpu (migrate disabled but preempt enabled).
> > > get_callchain_entry() calls get_recursion_context() to get the
> > > buffer for a particulart level.
> > >
> > > static inline int get_recursion_context(u8 *recursion)
> > > {
> > >          unsigned char rctx = interrupt_context_level();
> > >          if (recursion[rctx])
> > >                  return -1;
> > >          recursion[rctx]++;
> > >          barrier();
> > >          return rctx;
> > > }
> > >
> > > It is possible that both tasks (at process level) may
> > > reach right before "recursion[rctx]++;".
> > > In such cases, both tasks will be able to get
> > > buffer and this is not right.
> > >
> > > To fix this, we either need to have preempt disable
> > > in bpf side, or maybe we have some kind of atomic
> > > operation (cmpxchg or similar things), or maybe
> > > has a preempt disable between if statement and recursion[rctx]++,
> > > so only one task can get buffer?
> >
> > Thanks to your reminder, can we add preempt disable before and after 
> > get_callchain_entry, avoid affecting the original functions of perf.
>
> Yes, we get two get_callchain_entry() call site:
>    bpf/stackmap.c: entry = get_callchain_entry(&rctx);
>    events/callchain.c:     entry = get_callchain_entry(&rctx);
> We need to have preempt_disable()/preempt_enable() around them.
>
> Another choice maybe adds preempt_disable/enable() for
> get_callchain_entry() and get_perf_callchain() in stackmap.c,
> assuming these two function usage in other places are for
> interrupts (softirq, hardirq and nmi) so they are okay.
>
> But maybe the following is better?
>
> diff --git a/kernel/events/internal.h b/kernel/events/internal.h
> index d9cc57083091..0ccf94315954 100644
> --- a/kernel/events/internal.h
> +++ b/kernel/events/internal.h
> @@ -214,12 +214,9 @@ static inline int get_recursion_context(u8 *recursion)
>   {
>          unsigned char rctx = interrupt_context_level();
>
> -       if (recursion[rctx])
> +       if (cmpxchg(&recursion[rctx], 0, 1) != 0)
>                  return -1;
>
> -       recursion[rctx]++;
> -       barrier();
> -
>          return rctx;
>   }

Agree, this seems to have fewer side effects, thanks.

> > Regarding multiple task preemption: if the entry is not released via 
> > put_callchain_entry, it appears that perf's buffer does not support 
> > recording the second task, so it returns directly here.
> >
> >           if (recursion[rctx])
> >                   return -1;
> >
> > > > +    if (unlikely(!entry))
> > > > +        return NULL;
> > > > +
> > > > +    __init_perf_callchain_ctx(&ctx, entry, max_stack, false);
> > > > +    if (kernel)
> > > > +        __get_perf_callchain_kernel(&ctx, regs);
> > > > +    if (user && !crosstask)
> > > > +        __get_perf_callchain_user(&ctx, regs);
> > > > +
> > > > +    return entry;
> > > > +}
> > > > +
> > > > +static void bpf_put_callchain_entry(int rctx)
> > >
> > > we have bpf_get_perf_callchain(), maybe rename the above
> > > to bpf_put_perf_callchain()?
> >
> > Ack, thanks.
> >
> > > > +{
> > > > +    put_callchain_entry(rctx);
> > > > +}
> > > > +
> > >
> > > [...]


--
Best Regards
Tao Chen