Re: [PATCHv2 bpf-next 09/11] bpf: Remove trace_in argument from __bpf_get_stack

On Wed, Jul 29, 2026 at 1:39 AM Jiri Olsa <[email protected]> wrote:
>
> Now with the new callchain_* helper functions we can process trace_in
> case directly in bpf_get_stack_pe function and remove it from
> __bpf_get_stack which makes things easier for preemption fix in
> following change.
>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
>  kernel/bpf/stackmap.c | 46 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index 875f530f3597..bdf48a7ad2e8 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -783,7 +783,6 @@ static long callchain_finalize(void *buf, u32 size, u32 trace_nr, u32 elem_size,
>  }
>
>  static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task,
> -                           struct perf_callchain_entry *trace_in,
>                             void *buf, u32 size, u64 flags, bool may_fault)
>  {
>         bool user_build_id = flags & BPF_F_USER_BUILD_ID;
> @@ -822,10 +821,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task,
>         if (may_fault)
>                 rcu_read_lock(); /* need RCU for perf's callchain below */
>
> -       if (trace_in) {
> -               trace = trace_in;
> -               trace->nr = min_t(u32, trace->nr, max_depth);
> -       } else if (kernel && task) {
> +       if (kernel && task) {
>                 trace = get_callchain_entry_for_task(task, max_depth);
>         } else {
>                 trace = get_perf_callchain(regs, kernel, user, max_depth,
> @@ -856,7 +852,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task,
>  BPF_CALL_4(bpf_get_stack, struct pt_regs *, regs, void *, buf, u32, size,
>            u64, flags)
>  {
> -       return __bpf_get_stack(regs, NULL, NULL, buf, size, flags, false /* !may_fault */);
> +       return __bpf_get_stack(regs, NULL, buf, size, flags, false /* !may_fault */);
>  }
>
>  const struct bpf_func_proto bpf_get_stack_proto = {
> @@ -872,7 +868,7 @@ const struct bpf_func_proto bpf_get_stack_proto = {
>  BPF_CALL_4(bpf_get_stack_sleepable, struct pt_regs *, regs, void *, buf, u32, size,
>            u64, flags)
>  {
> -       return __bpf_get_stack(regs, NULL, NULL, buf, size, flags, true /* may_fault */);
> +       return __bpf_get_stack(regs, NULL, buf, size, flags, true /* may_fault */);
>  }
>
>  const struct bpf_func_proto bpf_get_stack_sleepable_proto = {
> @@ -896,7 +892,7 @@ static long __bpf_get_task_stack(struct task_struct *task, void *buf, u32 size,
>
>         regs = task_pt_regs(task);
>         if (regs)
> -               res = __bpf_get_stack(regs, task, NULL, buf, size, flags, may_fault);
> +               res = __bpf_get_stack(regs, task, buf, size, flags, may_fault);
>         put_task_stack(task);
>
>         return res;
> @@ -936,6 +932,33 @@ const struct bpf_func_proto bpf_get_task_stack_sleepable_proto = {
>         .arg4_type      = ARG_ANYTHING,
>  };
>
> +static int __bpf_get_stack_pe(struct perf_callchain_entry *trace, void *buf, u32 size,
> +                             u64 flags)
> +{
> +       bool user_build_id = flags & BPF_F_USER_BUILD_ID;
> +       u64 skip = flags & BPF_F_SKIP_FIELD_MASK;
> +       bool user = flags & BPF_F_USER_STACK;
> +       u32 elem_size, max_depth, nr_trace;
> +       bool kernel = !user;
> +
> +       if (kernel && user_build_id)
> +               return -EINVAL;
> +
> +       elem_size = user_build_id ? sizeof(struct bpf_stack_build_id) : sizeof(u64);
> +       if (unlikely(size % elem_size))
> +               return -EINVAL;
> +
> +       max_depth = stack_map_calculate_max_depth(size, elem_size, flags);
> +       trace->nr = min_t(u32, trace->nr, max_depth);
> +
> +       if (trace->nr < skip)
> +               return -EFAULT;
> +
> +       nr_trace = callchain_store(trace, buf, elem_size, flags);
> +       return callchain_finalize(buf, size, nr_trace, elem_size, user_build_id,
> +                                 user, false /* !may_fault */);
> +}
> +
>  BPF_CALL_4(bpf_get_stack_pe, struct bpf_perf_event_data_kern *, ctx,
>            void *, buf, u32, size, u64, flags)
>  {
> @@ -947,7 +970,7 @@ BPF_CALL_4(bpf_get_stack_pe, struct bpf_perf_event_data_kern *, ctx,
>         int err = -EINVAL;
>
>         if (!(event->attr.sample_type & PERF_SAMPLE_CALLCHAIN))
> -               return __bpf_get_stack(regs, NULL, NULL, buf, size, flags, false /* !may_fault */);
> +               return __bpf_get_stack(regs, NULL, buf, size, flags, false /* !may_fault */);
>
>         if (unlikely(flags & ~(BPF_F_SKIP_FIELD_MASK | BPF_F_USER_STACK |
>                                BPF_F_USER_BUILD_ID)))
> @@ -966,7 +989,7 @@ BPF_CALL_4(bpf_get_stack_pe, struct bpf_perf_event_data_kern *, ctx,
>
>         if (kernel) {
>                 trace->nr = nr_kernel;

this whole count_kernel_ip() logic above, why do we have it? I don't
think we can have combined user and kernel stack trace, so how can we
end up with PERF_CONTEXT_USER "ip" at all? I might be missing
something subtle here, but can you please check again if this whole
trace->nr calculation/override is even necessary.

> -               err = __bpf_get_stack(regs, NULL, trace, buf, size, flags, false /* !may_fault */);
> +               err = __bpf_get_stack_pe(trace, buf, size, flags);
>
>         } else { /* user */
>                 u64 skip = flags & BPF_F_SKIP_FIELD_MASK;
> @@ -974,9 +997,8 @@ BPF_CALL_4(bpf_get_stack_pe, struct bpf_perf_event_data_kern *, ctx,
>                 skip += nr_kernel;
>                 if (skip > BPF_F_SKIP_FIELD_MASK)
>                         goto clear;
> -
>                 flags = (flags & ~BPF_F_SKIP_FIELD_MASK) | skip;
> -               err = __bpf_get_stack(regs, NULL, trace, buf, size, flags, false /* !may_fault */);
> +               err = __bpf_get_stack_pe(trace, buf, size, flags);
>         }
>
>         /* restore nr */
> --
> 2.54.0
>