* [RFC PATCH bpf-next v2 0/2] Pass external callchain entry to get_perf_callchain @ 2025-10-14 10:01 Tao Chen 2025-10-14 10:01 ` [RFC PATCH bpf-next v2 1/2] perf: Use extern perf_callchain_entry for get_perf_callchain Tao Chen 2025-10-14 10:01 ` [RFC PATCH bpf-next v2 2/2] bpf: Pass external callchain entry to get_perf_callchain Tao Chen 0 siblings, 2 replies; 6+ messages in thread From: Tao Chen @ 2025-10-14 10:01 UTC (permalink / raw) To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang, song, ast, daniel, andrii, martin.lau, eddyz87, yonghong.song, john.fastabend, kpsingh, sdf, haoluo Cc: linux-perf-users, linux-kernel, bpf, Tao Chen Background ========== Alexei noted we should use preempt_disable to protect get_perf_callchain in bpf stackmap. https://lore.kernel.org/bpf/CAADnVQ+s8B7-fvR1TNO-bniSyKv57cH_ihRszmZV7pQDyV=VDQ@mail.gmail.com A previous patch was submitted to attempt fixing this issue. And Andrii suggested teach get_perf_callchain to let us pass that buffer directly to avoid that unnecessary copy. https://lore.kernel.org/bpf/20250926153952.1661146-1-chen.dylane@linux.dev Proposed Solution ================= Add external perf_callchain_entry parameter for get_perf_callchain to allow us to use external buffer from BPF side. The biggest advantage is that it can reduce unnecessary copies. Todo ==== If the above changes are reasonable, it seems that get_callchain_entry_for_task could also use an external perf_callchain_entry. But I'm not sure if this modification is appropriate. After all, the implementation of get_callchain_entry in the perf subsystem seems much more complex than directly using an external buffer. Comments and suggestions are always welcome. Change list: - v1 -> v2 From Jiri - rebase code, fix confict - v1: https://lore.kernel.org/bpf/20251013174721.2681091-1-chen.dylane@linux.dev Tao Chen (2): perf: Use extern perf_callchain_entry for get_perf_callchain bpf: Pass external callchain entry to get_perf_callchain include/linux/perf_event.h | 4 ++-- kernel/bpf/stackmap.c | 19 +++++++++++-------- kernel/events/callchain.c | 13 +++++++++---- kernel/events/core.c | 2 +- 4 files changed, 23 insertions(+), 15 deletions(-) -- 2.48.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC PATCH bpf-next v2 1/2] perf: Use extern perf_callchain_entry for get_perf_callchain 2025-10-14 10:01 [RFC PATCH bpf-next v2 0/2] Pass external callchain entry to get_perf_callchain Tao Chen @ 2025-10-14 10:01 ` Tao Chen 2025-10-14 10:01 ` [RFC PATCH bpf-next v2 2/2] bpf: Pass external callchain entry to get_perf_callchain Tao Chen 1 sibling, 0 replies; 6+ messages in thread From: Tao Chen @ 2025-10-14 10:01 UTC (permalink / raw) To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang, song, ast, daniel, andrii, martin.lau, eddyz87, yonghong.song, john.fastabend, kpsingh, sdf, haoluo Cc: linux-perf-users, linux-kernel, bpf, Tao Chen From bpf stack map, we want to use our own buffers to avoid unnecessary copy, so let us pass it directly. BPF will use this in the next patch. Signed-off-by: Tao Chen <chen.dylane@linux.dev> --- include/linux/perf_event.h | 4 ++-- kernel/bpf/stackmap.c | 4 ++-- kernel/events/callchain.c | 13 +++++++++---- kernel/events/core.c | 2 +- 4 files changed, 14 insertions(+), 9 deletions(-) diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index fd1d91017b9..b144da7d803 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -1719,8 +1719,8 @@ DECLARE_PER_CPU(struct perf_callchain_entry, perf_callchain_entry); extern void perf_callchain_user(struct perf_callchain_entry_ctx *entry, struct pt_regs *regs); extern void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *regs); extern struct perf_callchain_entry * -get_perf_callchain(struct pt_regs *regs, bool kernel, bool user, - u32 max_stack, bool crosstask, bool add_mark); +get_perf_callchain(struct pt_regs *regs, struct perf_callchain_entry *external_entry, + bool kernel, bool user, u32 max_stack, bool crosstask, bool add_mark); extern int get_callchain_buffers(int max_stack); extern void put_callchain_buffers(void); extern struct perf_callchain_entry *get_callchain_entry(int *rctx); diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c index 4d53cdd1374..94e46b7f340 100644 --- a/kernel/bpf/stackmap.c +++ b/kernel/bpf/stackmap.c @@ -314,7 +314,7 @@ BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map, if (max_depth > sysctl_perf_event_max_stack) max_depth = sysctl_perf_event_max_stack; - trace = get_perf_callchain(regs, kernel, user, max_depth, + trace = get_perf_callchain(regs, NULL, kernel, user, max_depth, false, false); if (unlikely(!trace)) @@ -451,7 +451,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, else if (kernel && task) trace = get_callchain_entry_for_task(task, max_depth); else - trace = get_perf_callchain(regs, kernel, user, max_depth, + trace = get_perf_callchain(regs, NULL, kernel, user, max_depth, crosstask, false); if (unlikely(!trace) || trace->nr < skip) { diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c index 808c0d7a31f..851e8f9d026 100644 --- a/kernel/events/callchain.c +++ b/kernel/events/callchain.c @@ -217,8 +217,8 @@ static void fixup_uretprobe_trampoline_entries(struct perf_callchain_entry *entr } struct perf_callchain_entry * -get_perf_callchain(struct pt_regs *regs, bool kernel, bool user, - u32 max_stack, bool crosstask, bool add_mark) +get_perf_callchain(struct pt_regs *regs, struct perf_callchain_entry *external_entry, + bool kernel, bool user, u32 max_stack, bool crosstask, bool add_mark) { struct perf_callchain_entry *entry; struct perf_callchain_entry_ctx ctx; @@ -228,7 +228,11 @@ get_perf_callchain(struct pt_regs *regs, bool kernel, bool user, if (crosstask && user && !kernel) return NULL; - entry = get_callchain_entry(&rctx); + if (external_entry) + entry = external_entry; + else + entry = get_callchain_entry(&rctx); + if (!entry) return NULL; @@ -260,7 +264,8 @@ get_perf_callchain(struct pt_regs *regs, bool kernel, bool user, } exit_put: - put_callchain_entry(rctx); + if (!external_entry) + put_callchain_entry(rctx); return entry; } diff --git a/kernel/events/core.c b/kernel/events/core.c index 7541f6f85fc..5d8e146003a 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -8217,7 +8217,7 @@ perf_callchain(struct perf_event *event, struct pt_regs *regs) if (!kernel && !user) return &__empty_callchain; - callchain = get_perf_callchain(regs, kernel, user, + callchain = get_perf_callchain(regs, NULL, kernel, user, max_stack, crosstask, true); return callchain ?: &__empty_callchain; } -- 2.48.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFC PATCH bpf-next v2 2/2] bpf: Pass external callchain entry to get_perf_callchain 2025-10-14 10:01 [RFC PATCH bpf-next v2 0/2] Pass external callchain entry to get_perf_callchain Tao Chen 2025-10-14 10:01 ` [RFC PATCH bpf-next v2 1/2] perf: Use extern perf_callchain_entry for get_perf_callchain Tao Chen @ 2025-10-14 10:01 ` Tao Chen 2025-10-14 12:14 ` Jiri Olsa 1 sibling, 1 reply; 6+ messages in thread From: Tao Chen @ 2025-10-14 10:01 UTC (permalink / raw) To: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, kan.liang, song, ast, daniel, andrii, martin.lau, eddyz87, yonghong.song, john.fastabend, kpsingh, sdf, haoluo Cc: linux-perf-users, linux-kernel, bpf, Tao Chen As Alexei noted, get_perf_callchain() return values may be reused if a task is preempted after the BPF program enters migrate disable mode. Drawing on the per-cpu design of bpf_perf_callchain_entries, stack-allocated memory of bpf_perf_callchain_entry is used here. Signed-off-by: Tao Chen <chen.dylane@linux.dev> --- kernel/bpf/stackmap.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c index 94e46b7f340..acd72c021c0 100644 --- a/kernel/bpf/stackmap.c +++ b/kernel/bpf/stackmap.c @@ -31,6 +31,11 @@ struct bpf_stack_map { struct stack_map_bucket *buckets[] __counted_by(n_buckets); }; +struct bpf_perf_callchain_entry { + u64 nr; + u64 ip[PERF_MAX_STACK_DEPTH]; +}; + static inline bool stack_map_use_build_id(struct bpf_map *map) { return (map->map_flags & BPF_F_STACK_BUILD_ID); @@ -305,6 +310,7 @@ BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map, bool user = flags & BPF_F_USER_STACK; struct perf_callchain_entry *trace; bool kernel = !user; + struct bpf_perf_callchain_entry entry = { 0 }; if (unlikely(flags & ~(BPF_F_SKIP_FIELD_MASK | BPF_F_USER_STACK | BPF_F_FAST_STACK_CMP | BPF_F_REUSE_STACKID))) @@ -314,12 +320,8 @@ BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map, if (max_depth > sysctl_perf_event_max_stack) max_depth = sysctl_perf_event_max_stack; - trace = get_perf_callchain(regs, NULL, kernel, user, max_depth, - false, false); - - if (unlikely(!trace)) - /* couldn't fetch the stack trace */ - return -EFAULT; + trace = get_perf_callchain(regs, (struct perf_callchain_entry *)&entry, + kernel, user, max_depth, false, false); return __bpf_get_stackid(map, trace, flags); } @@ -412,6 +414,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, u32 skip = flags & BPF_F_SKIP_FIELD_MASK; bool user = flags & BPF_F_USER_STACK; struct perf_callchain_entry *trace; + struct bpf_perf_callchain_entry entry = { 0 }; bool kernel = !user; int err = -EINVAL; u64 *ips; @@ -451,8 +454,8 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, else if (kernel && task) trace = get_callchain_entry_for_task(task, max_depth); else - trace = get_perf_callchain(regs, NULL, kernel, user, max_depth, - crosstask, false); + trace = get_perf_callchain(regs, (struct perf_callchain_entry *)&entry, + kernel, user, max_depth, crosstask, false); if (unlikely(!trace) || trace->nr < skip) { if (may_fault) -- 2.48.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH bpf-next v2 2/2] bpf: Pass external callchain entry to get_perf_callchain 2025-10-14 10:01 ` [RFC PATCH bpf-next v2 2/2] bpf: Pass external callchain entry to get_perf_callchain Tao Chen @ 2025-10-14 12:14 ` Jiri Olsa 2025-10-14 12:34 ` Tao Chen 2025-10-14 15:02 ` Alexei Starovoitov 0 siblings, 2 replies; 6+ messages in thread From: Jiri Olsa @ 2025-10-14 12:14 UTC (permalink / raw) To: Tao Chen Cc: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin, irogers, adrian.hunter, kan.liang, song, ast, daniel, andrii, martin.lau, eddyz87, yonghong.song, john.fastabend, kpsingh, sdf, haoluo, linux-perf-users, linux-kernel, bpf On Tue, Oct 14, 2025 at 06:01:28PM +0800, 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. Drawing on the per-cpu design of bpf_perf_callchain_entries, > stack-allocated memory of bpf_perf_callchain_entry is used here. > > Signed-off-by: Tao Chen <chen.dylane@linux.dev> > --- > kernel/bpf/stackmap.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c > index 94e46b7f340..acd72c021c0 100644 > --- a/kernel/bpf/stackmap.c > +++ b/kernel/bpf/stackmap.c > @@ -31,6 +31,11 @@ struct bpf_stack_map { > struct stack_map_bucket *buckets[] __counted_by(n_buckets); > }; > > +struct bpf_perf_callchain_entry { > + u64 nr; > + u64 ip[PERF_MAX_STACK_DEPTH]; > +}; > + > static inline bool stack_map_use_build_id(struct bpf_map *map) > { > return (map->map_flags & BPF_F_STACK_BUILD_ID); > @@ -305,6 +310,7 @@ BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map, > bool user = flags & BPF_F_USER_STACK; > struct perf_callchain_entry *trace; > bool kernel = !user; > + struct bpf_perf_callchain_entry entry = { 0 }; so IIUC having entries on stack we do not need to do preempt_disable you had in the previous version, right? I saw Andrii's justification to have this on the stack, I think it's fine, but does it have to be initialized? it seems that only used entries are copied to map jirka > > if (unlikely(flags & ~(BPF_F_SKIP_FIELD_MASK | BPF_F_USER_STACK | > BPF_F_FAST_STACK_CMP | BPF_F_REUSE_STACKID))) > @@ -314,12 +320,8 @@ BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map, > if (max_depth > sysctl_perf_event_max_stack) > max_depth = sysctl_perf_event_max_stack; > > - trace = get_perf_callchain(regs, NULL, kernel, user, max_depth, > - false, false); > - > - if (unlikely(!trace)) > - /* couldn't fetch the stack trace */ > - return -EFAULT; > + trace = get_perf_callchain(regs, (struct perf_callchain_entry *)&entry, > + kernel, user, max_depth, false, false); > > return __bpf_get_stackid(map, trace, flags); > } > @@ -412,6 +414,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, > u32 skip = flags & BPF_F_SKIP_FIELD_MASK; > bool user = flags & BPF_F_USER_STACK; > struct perf_callchain_entry *trace; > + struct bpf_perf_callchain_entry entry = { 0 }; > bool kernel = !user; > int err = -EINVAL; > u64 *ips; > @@ -451,8 +454,8 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, > else if (kernel && task) > trace = get_callchain_entry_for_task(task, max_depth); > else > - trace = get_perf_callchain(regs, NULL, kernel, user, max_depth, > - crosstask, false); > + trace = get_perf_callchain(regs, (struct perf_callchain_entry *)&entry, > + kernel, user, max_depth, crosstask, false); > > if (unlikely(!trace) || trace->nr < skip) { > if (may_fault) > -- > 2.48.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH bpf-next v2 2/2] bpf: Pass external callchain entry to get_perf_callchain 2025-10-14 12:14 ` Jiri Olsa @ 2025-10-14 12:34 ` Tao Chen 2025-10-14 15:02 ` Alexei Starovoitov 1 sibling, 0 replies; 6+ messages in thread From: Tao Chen @ 2025-10-14 12:34 UTC (permalink / raw) To: Jiri Olsa Cc: peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin, irogers, adrian.hunter, kan.liang, song, ast, daniel, andrii, martin.lau, eddyz87, yonghong.song, john.fastabend, kpsingh, sdf, haoluo, linux-perf-users, linux-kernel, bpf 在 2025/10/14 20:14, Jiri Olsa 写道: >> + struct bpf_perf_callchain_entry entry = { 0 }; > so IIUC having entries on stack we do not need to do preempt_disable > you had in the previous version, right? > Yes, i think so, preempt_disable seems unnecessary. > I saw Andrii's justification to have this on the stack, I think it's > fine, but does it have to be initialized? it seems that only used > entries are copied to map That makes sense. Removing it definitely looks better. -- Best Regards Tao Chen ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH bpf-next v2 2/2] bpf: Pass external callchain entry to get_perf_callchain 2025-10-14 12:14 ` Jiri Olsa 2025-10-14 12:34 ` Tao Chen @ 2025-10-14 15:02 ` Alexei Starovoitov 1 sibling, 0 replies; 6+ messages in thread From: Alexei Starovoitov @ 2025-10-14 15:02 UTC (permalink / raw) To: Jiri Olsa Cc: Tao Chen, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland, Alexander Shishkin, Ian Rogers, Adrian Hunter, Kan Liang, Song Liu, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Eduard, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, linux-perf-use., LKML, bpf On Tue, Oct 14, 2025 at 5:14 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Tue, Oct 14, 2025 at 06:01:28PM +0800, 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. Drawing on the per-cpu design of bpf_perf_callchain_entries, > > stack-allocated memory of bpf_perf_callchain_entry is used here. > > > > Signed-off-by: Tao Chen <chen.dylane@linux.dev> > > --- > > kernel/bpf/stackmap.c | 19 +++++++++++-------- > > 1 file changed, 11 insertions(+), 8 deletions(-) > > > > diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c > > index 94e46b7f340..acd72c021c0 100644 > > --- a/kernel/bpf/stackmap.c > > +++ b/kernel/bpf/stackmap.c > > @@ -31,6 +31,11 @@ struct bpf_stack_map { > > struct stack_map_bucket *buckets[] __counted_by(n_buckets); > > }; > > > > +struct bpf_perf_callchain_entry { > > + u64 nr; > > + u64 ip[PERF_MAX_STACK_DEPTH]; > > +}; > > + > > static inline bool stack_map_use_build_id(struct bpf_map *map) > > { > > return (map->map_flags & BPF_F_STACK_BUILD_ID); > > @@ -305,6 +310,7 @@ BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map, > > bool user = flags & BPF_F_USER_STACK; > > struct perf_callchain_entry *trace; > > bool kernel = !user; > > + struct bpf_perf_callchain_entry entry = { 0 }; > > so IIUC having entries on stack we do not need to do preempt_disable > you had in the previous version, right? > > I saw Andrii's justification to have this on the stack, I think it's > fine, but does it have to be initialized? it seems that only used > entries are copied to map No. We're not adding 1k stack consumption. pw-bot: cr ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-10-14 15:02 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-14 10:01 [RFC PATCH bpf-next v2 0/2] Pass external callchain entry to get_perf_callchain Tao Chen 2025-10-14 10:01 ` [RFC PATCH bpf-next v2 1/2] perf: Use extern perf_callchain_entry for get_perf_callchain Tao Chen 2025-10-14 10:01 ` [RFC PATCH bpf-next v2 2/2] bpf: Pass external callchain entry to get_perf_callchain Tao Chen 2025-10-14 12:14 ` Jiri Olsa 2025-10-14 12:34 ` Tao Chen 2025-10-14 15:02 ` Alexei Starovoitov
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).