* [PATCH bpf-next v3 0/3] bpftool: Improve perf counter reporting
@ 2026-09-08 14:27 Mykyta Yatsenko
2026-09-08 14:27 ` [PATCH bpf-next v3 1/3] bpftool: Track perf counter snapshot state Mykyta Yatsenko
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Mykyta Yatsenko @ 2026-09-08 14:27 UTC (permalink / raw)
To: bpf, ast, andrii, daniel, kernel-team, eddyz87, memxor, qmo,
linux-perf-users, acme, namhyung
Cc: Mykyta Yatsenko
Track valid snapshots that have a zero counter value. Scale each per-CPU
counter before aggregation. Report cycles per included program run.
Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
---
Changes in v3:
- Keep the JSON value field raw. Add value_scaled and run_cnt_valid.
- Use the same CPU set for all metrics.
- Move snapshot state tracking to a separate patch.
- Link to v2: https://patch.msgid.link/20260902-bpftool_cyles_per_run-v2-1-bc5d14ad0ce9@meta.com
Changes in v2:
- Scale all counters.
- Use an enum for metric indexes.
- Link to v1: https://patch.msgid.link/20260827-bpftool_cyles_per_run-v1-1-77d7bfc3c065@meta.com
---
Mykyta Yatsenko (3):
bpftool: Track perf counter snapshot state
perf bpf_counter: Track valid BPF counter snapshots
bpftool: Scale counters and report cycles per run
tools/bpf/bpftool/Documentation/bpftool-prog.rst | 13 +-
tools/bpf/bpftool/prog.c | 159 +++++++++++++++++------
tools/bpf/bpftool/skeleton/profiler.bpf.c | 25 ++--
tools/perf/util/bpf_skel/bpf_prog_profiler.bpf.c | 26 ++--
4 files changed, 168 insertions(+), 55 deletions(-)
---
base-commit: 3fa3128887a4df6057c543aae7c5c7fd2ed41f12
change-id: 20260827-bpftool_cyles_per_run-93169fcb30b7
Best regards,
--
Mykyta Yatsenko <yatsenko@meta.com>
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH bpf-next v3 1/3] bpftool: Track perf counter snapshot state 2026-09-08 14:27 [PATCH bpf-next v3 0/3] bpftool: Improve perf counter reporting Mykyta Yatsenko @ 2026-09-08 14:27 ` Mykyta Yatsenko 2026-09-08 14:35 ` sashiko-bot ` (2 more replies) 2026-09-08 14:27 ` [PATCH bpf-next v3 2/3] perf bpf_counter: Track valid BPF counter snapshots Mykyta Yatsenko ` (2 subsequent siblings) 3 siblings, 3 replies; 16+ messages in thread From: Mykyta Yatsenko @ 2026-09-08 14:27 UTC (permalink / raw) To: bpf, ast, andrii, daniel, kernel-team, eddyz87, memxor, qmo, linux-perf-users, acme, namhyung Cc: Mykyta Yatsenko From: Mykyta Yatsenko <yatsenko@meta.com> A perf counter can be zero at fentry. PMU multiplexing can schedule the event during the BPF program. The old counter check then drops a valid sample. Use an armed flag to track each successful fentry snapshot. Reset all flags before new reads. The fexit path clears each flag when it consumes the snapshot. Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command") Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> --- tools/bpf/bpftool/skeleton/profiler.bpf.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/tools/bpf/bpftool/skeleton/profiler.bpf.c b/tools/bpf/bpftool/skeleton/profiler.bpf.c index f48c783cb9f7..6c654bd9b346 100644 --- a/tools/bpf/bpftool/skeleton/profiler.bpf.c +++ b/tools/bpf/bpftool/skeleton/profiler.bpf.c @@ -10,6 +10,11 @@ struct bpf_perf_event_value___local { __u64 running; } __attribute__((preserve_access_index)); +struct profile_reading { + struct bpf_perf_event_value___local value; + bool armed; +}; + /* map of perf event fds, num_cpu * num_metric entries */ struct { __uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY); @@ -21,7 +26,7 @@ struct { struct { __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY); __uint(key_size, sizeof(u32)); - __uint(value_size, sizeof(struct bpf_perf_event_value___local)); + __uint(value_size, sizeof(struct profile_reading)); } fentry_readings SEC(".maps"); /* accumulated readings */ @@ -45,7 +50,7 @@ const volatile __u32 num_metric = 1; SEC("fentry/XXX") int BPF_PROG(fentry_XXX) { - struct bpf_perf_event_value___local *ptrs[MAX_NUM_METRICS]; + struct profile_reading *ptrs[MAX_NUM_METRICS]; u32 key = bpf_get_smp_processor_id(); u32 i; @@ -56,6 +61,7 @@ int BPF_PROG(fentry_XXX) ptrs[i] = bpf_map_lookup_elem(&fentry_readings, &flag); if (!ptrs[i]) return 0; + ptrs[i]->armed = false; } for (i = 0; i < num_metric && i < MAX_NUM_METRICS; i++) { @@ -66,7 +72,8 @@ int BPF_PROG(fentry_XXX) sizeof(reading)); if (err) return 0; - *(ptrs[i]) = reading; + ptrs[i]->value = reading; + ptrs[i]->armed = true; key += num_cpu; } @@ -76,16 +83,18 @@ int BPF_PROG(fentry_XXX) static inline void fexit_update_maps(u32 id, struct bpf_perf_event_value___local *after) { - struct bpf_perf_event_value___local *before, diff; + struct profile_reading *before; + struct bpf_perf_event_value___local diff; before = bpf_map_lookup_elem(&fentry_readings, &id); /* only account samples with a valid fentry_reading */ - if (before && before->counter) { + if (before && before->armed) { struct bpf_perf_event_value___local *accum; - diff.counter = after->counter - before->counter; - diff.enabled = after->enabled - before->enabled; - diff.running = after->running - before->running; + before->armed = false; + diff.counter = after->counter - before->value.counter; + diff.enabled = after->enabled - before->value.enabled; + diff.running = after->running - before->value.running; accum = bpf_map_lookup_elem(&accum_readings, &id); if (accum) { -- 2.53.0-Meta ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next v3 1/3] bpftool: Track perf counter snapshot state 2026-09-08 14:27 ` [PATCH bpf-next v3 1/3] bpftool: Track perf counter snapshot state Mykyta Yatsenko @ 2026-09-08 14:35 ` sashiko-bot 2026-09-08 22:08 ` Quentin Monnet 2026-09-08 23:56 ` Andrii Nakryiko 2 siblings, 0 replies; 16+ messages in thread From: sashiko-bot @ 2026-09-08 14:35 UTC (permalink / raw) To: Mykyta Yatsenko; +Cc: linux-perf-users, bpf > From: Mykyta Yatsenko <yatsenko@meta.com> > > A perf counter can be zero at fentry. PMU multiplexing can schedule the > event during the BPF program. The old counter check then drops a valid > sample. > > Use an armed flag to track each successful fentry snapshot. Reset all > flags before new reads. The fexit path clears each flag when it consumes > the snapshot. > > Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command") > Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> Sashiko has reviewed this patch and found no issues. It looks great! -- Sashiko AI review · https://sashiko.dev/#/patchset/20260908-bpftool_cyles_per_run-v3-0-60e86f325c35@meta.com?part=1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next v3 1/3] bpftool: Track perf counter snapshot state 2026-09-08 14:27 ` [PATCH bpf-next v3 1/3] bpftool: Track perf counter snapshot state Mykyta Yatsenko 2026-09-08 14:35 ` sashiko-bot @ 2026-09-08 22:08 ` Quentin Monnet 2026-09-08 23:56 ` Andrii Nakryiko 2 siblings, 0 replies; 16+ messages in thread From: Quentin Monnet @ 2026-09-08 22:08 UTC (permalink / raw) To: Mykyta Yatsenko, bpf, ast, andrii, daniel, kernel-team, eddyz87, memxor, linux-perf-users, acme, namhyung Cc: Mykyta Yatsenko 2026-09-08 07:27 UTC-0700 ~ Mykyta Yatsenko <mykyta.yatsenko5@gmail.com> > From: Mykyta Yatsenko <yatsenko@meta.com> > > A perf counter can be zero at fentry. PMU multiplexing can schedule the > event during the BPF program. The old counter check then drops a valid > sample. > > Use an armed flag to track each successful fentry snapshot. Reset all > flags before new reads. The fexit path clears each flag when it consumes > the snapshot. > > Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command") > Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> Acked-by: Quentin Monnet <qmo@kernel.org> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next v3 1/3] bpftool: Track perf counter snapshot state 2026-09-08 14:27 ` [PATCH bpf-next v3 1/3] bpftool: Track perf counter snapshot state Mykyta Yatsenko 2026-09-08 14:35 ` sashiko-bot 2026-09-08 22:08 ` Quentin Monnet @ 2026-09-08 23:56 ` Andrii Nakryiko 2026-09-09 9:19 ` Mykyta Yatsenko 2 siblings, 1 reply; 16+ messages in thread From: Andrii Nakryiko @ 2026-09-08 23:56 UTC (permalink / raw) To: Mykyta Yatsenko Cc: bpf, ast, andrii, daniel, kernel-team, eddyz87, memxor, qmo, linux-perf-users, acme, namhyung, Mykyta Yatsenko On Tue, Sep 8, 2026 at 7:27 AM Mykyta Yatsenko <mykyta.yatsenko5@gmail.com> wrote: > > From: Mykyta Yatsenko <yatsenko@meta.com> > > A perf counter can be zero at fentry. PMU multiplexing can schedule the > event during the BPF program. The old counter check then drops a valid > sample. > > Use an armed flag to track each successful fentry snapshot. Reset all > flags before new reads. The fexit path clears each flag when it consumes > the snapshot. > > Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command") > Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> > --- > tools/bpf/bpftool/skeleton/profiler.bpf.c | 25 +++++++++++++++++-------- > 1 file changed, 17 insertions(+), 8 deletions(-) > > diff --git a/tools/bpf/bpftool/skeleton/profiler.bpf.c b/tools/bpf/bpftool/skeleton/profiler.bpf.c > index f48c783cb9f7..6c654bd9b346 100644 > --- a/tools/bpf/bpftool/skeleton/profiler.bpf.c > +++ b/tools/bpf/bpftool/skeleton/profiler.bpf.c > @@ -10,6 +10,11 @@ struct bpf_perf_event_value___local { > __u64 running; > } __attribute__((preserve_access_index)); > > +struct profile_reading { > + struct bpf_perf_event_value___local value; > + bool armed; > +}; > + > /* map of perf event fds, num_cpu * num_metric entries */ > struct { > __uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY); > @@ -21,7 +26,7 @@ struct { > struct { > __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY); > __uint(key_size, sizeof(u32)); > - __uint(value_size, sizeof(struct bpf_perf_event_value___local)); this is not a bug, but... ;) struct bpf_perf_event_value is UAPI, why do we need CO-RE-relocatable ___local variant?... > + __uint(value_size, sizeof(struct profile_reading)); > } fentry_readings SEC(".maps"); > > /* accumulated readings */ > @@ -45,7 +50,7 @@ const volatile __u32 num_metric = 1; > SEC("fentry/XXX") > int BPF_PROG(fentry_XXX) > { > - struct bpf_perf_event_value___local *ptrs[MAX_NUM_METRICS]; > + struct profile_reading *ptrs[MAX_NUM_METRICS]; > u32 key = bpf_get_smp_processor_id(); > u32 i; > > @@ -56,6 +61,7 @@ int BPF_PROG(fentry_XXX) > ptrs[i] = bpf_map_lookup_elem(&fentry_readings, &flag); > if (!ptrs[i]) > return 0; > + ptrs[i]->armed = false; > } this is preexisting, but why do we have two separate loops: first lookup up fentry_readings pointers, and then separately a) reading perf counters into local variable just to b) immediately copy it into map_value. can you try simplifying this and doing bpf_perf_event_read_value() into ptrs[i] directly? all within the same loop? it might have been some verifier issue, not sure, but I think this should work just fine > > for (i = 0; i < num_metric && i < MAX_NUM_METRICS; i++) { > @@ -66,7 +72,8 @@ int BPF_PROG(fentry_XXX) > sizeof(reading)); > if (err) > return 0; > - *(ptrs[i]) = reading; > + ptrs[i]->value = reading; > + ptrs[i]->armed = true; > key += num_cpu; > } > > @@ -76,16 +83,18 @@ int BPF_PROG(fentry_XXX) > static inline void > fexit_update_maps(u32 id, struct bpf_perf_event_value___local *after) > { > - struct bpf_perf_event_value___local *before, diff; > + struct profile_reading *before; > + struct bpf_perf_event_value___local diff; > > before = bpf_map_lookup_elem(&fentry_readings, &id); > /* only account samples with a valid fentry_reading */ > - if (before && before->counter) { > + if (before && before->armed) { this is such an unlikely situation that I wouldn't even bother "fixing" it, tbh. alternatively we can check enabled or running for zero, I don't think realistically enabled can be zero if we actually captured it an fentry pw-bot: cr > struct bpf_perf_event_value___local *accum; > > - diff.counter = after->counter - before->counter; > - diff.enabled = after->enabled - before->enabled; > - diff.running = after->running - before->running; > + before->armed = false; > + diff.counter = after->counter - before->value.counter; > + diff.enabled = after->enabled - before->value.enabled; > + diff.running = after->running - before->value.running; > > accum = bpf_map_lookup_elem(&accum_readings, &id); > if (accum) { > > -- > 2.53.0-Meta > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next v3 1/3] bpftool: Track perf counter snapshot state 2026-09-08 23:56 ` Andrii Nakryiko @ 2026-09-09 9:19 ` Mykyta Yatsenko 0 siblings, 0 replies; 16+ messages in thread From: Mykyta Yatsenko @ 2026-09-09 9:19 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, ast, andrii, daniel, kernel-team, eddyz87, memxor, qmo, linux-perf-users, acme, namhyung, Mykyta Yatsenko On 9/9/26 12:56 AM, Andrii Nakryiko wrote: > On Tue, Sep 8, 2026 at 7:27 AM Mykyta Yatsenko > <mykyta.yatsenko5@gmail.com> wrote: >> >> From: Mykyta Yatsenko <yatsenko@meta.com> >> >> A perf counter can be zero at fentry. PMU multiplexing can schedule the >> event during the BPF program. The old counter check then drops a valid >> sample. >> >> Use an armed flag to track each successful fentry snapshot. Reset all >> flags before new reads. The fexit path clears each flag when it consumes >> the snapshot. >> >> Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command") >> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> >> --- >> tools/bpf/bpftool/skeleton/profiler.bpf.c | 25 +++++++++++++++++-------- >> 1 file changed, 17 insertions(+), 8 deletions(-) >> >> diff --git a/tools/bpf/bpftool/skeleton/profiler.bpf.c b/tools/bpf/bpftool/skeleton/profiler.bpf.c >> index f48c783cb9f7..6c654bd9b346 100644 >> --- a/tools/bpf/bpftool/skeleton/profiler.bpf.c >> +++ b/tools/bpf/bpftool/skeleton/profiler.bpf.c >> @@ -10,6 +10,11 @@ struct bpf_perf_event_value___local { >> __u64 running; >> } __attribute__((preserve_access_index)); >> >> +struct profile_reading { >> + struct bpf_perf_event_value___local value; >> + bool armed; >> +}; >> + >> /* map of perf event fds, num_cpu * num_metric entries */ >> struct { >> __uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY); >> @@ -21,7 +26,7 @@ struct { >> struct { >> __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY); >> __uint(key_size, sizeof(u32)); >> - __uint(value_size, sizeof(struct bpf_perf_event_value___local)); > > this is not a bug, but... ;) > > > struct bpf_perf_event_value is UAPI, why do we need CO-RE-relocatable > ___local variant?... > > >> + __uint(value_size, sizeof(struct profile_reading)); >> } fentry_readings SEC(".maps"); >> >> /* accumulated readings */ >> @@ -45,7 +50,7 @@ const volatile __u32 num_metric = 1; >> SEC("fentry/XXX") >> int BPF_PROG(fentry_XXX) >> { >> - struct bpf_perf_event_value___local *ptrs[MAX_NUM_METRICS]; >> + struct profile_reading *ptrs[MAX_NUM_METRICS]; >> u32 key = bpf_get_smp_processor_id(); >> u32 i; >> >> @@ -56,6 +61,7 @@ int BPF_PROG(fentry_XXX) >> ptrs[i] = bpf_map_lookup_elem(&fentry_readings, &flag); >> if (!ptrs[i]) >> return 0; >> + ptrs[i]->armed = false; >> } > > this is preexisting, but why do we have two separate loops: first > lookup up fentry_readings pointers, and then separately a) reading > perf counters into local variable just to b) immediately copy it into > map_value. > > can you try simplifying this and doing bpf_perf_event_read_value() > into ptrs[i] directly? all within the same loop? > > it might have been some verifier issue, not sure, but I think this > should work just fine > >> >> for (i = 0; i < num_metric && i < MAX_NUM_METRICS; i++) { >> @@ -66,7 +72,8 @@ int BPF_PROG(fentry_XXX) >> sizeof(reading)); >> if (err) >> return 0; >> - *(ptrs[i]) = reading; >> + ptrs[i]->value = reading; >> + ptrs[i]->armed = true; >> key += num_cpu; >> } >> >> @@ -76,16 +83,18 @@ int BPF_PROG(fentry_XXX) >> static inline void >> fexit_update_maps(u32 id, struct bpf_perf_event_value___local *after) >> { >> - struct bpf_perf_event_value___local *before, diff; >> + struct profile_reading *before; >> + struct bpf_perf_event_value___local diff; >> >> before = bpf_map_lookup_elem(&fentry_readings, &id); >> /* only account samples with a valid fentry_reading */ >> - if (before && before->counter) { >> + if (before && before->armed) { > > this is such an unlikely situation that I wouldn't even bother > "fixing" it, tbh. alternatively we can check enabled or running for > zero, I don't think realistically enabled can be zero if we actually > captured it an fentry Here we check before->counter for 0, substituting by enabled or running will still have the same risk of dropping the first sample. I could reproduce it by: 1. make PMU busy with 4 events to force multiplexing 2. then quick profile for 1 second Result: first sample gets dropped, because measurement at fentry is 0. This is not a huge deal by itself, but the fix is simple enough, in my opinion, to make it worth. For a pocket change we get clearer flow: fentry arms the counter, fexit disarms it. We have the same code in perf, I thought it would be nice to have this change there (more choice of sparse events, subsecond timeouts possible) > > pw-bot: cr > > >> struct bpf_perf_event_value___local *accum; >> >> - diff.counter = after->counter - before->counter; >> - diff.enabled = after->enabled - before->enabled; >> - diff.running = after->running - before->running; >> + before->armed = false; >> + diff.counter = after->counter - before->value.counter; >> + diff.enabled = after->enabled - before->value.enabled; >> + diff.running = after->running - before->value.running; >> >> accum = bpf_map_lookup_elem(&accum_readings, &id); >> if (accum) { >> >> -- >> 2.53.0-Meta >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH bpf-next v3 2/3] perf bpf_counter: Track valid BPF counter snapshots 2026-09-08 14:27 [PATCH bpf-next v3 0/3] bpftool: Improve perf counter reporting Mykyta Yatsenko 2026-09-08 14:27 ` [PATCH bpf-next v3 1/3] bpftool: Track perf counter snapshot state Mykyta Yatsenko @ 2026-09-08 14:27 ` Mykyta Yatsenko 2026-09-08 14:39 ` sashiko-bot 2026-09-08 14:27 ` [PATCH bpf-next v3 3/3] bpftool: Scale counters and report cycles per run Mykyta Yatsenko 2026-09-08 18:01 ` [PATCH bpf-next v3 0/3] bpftool: Improve perf counter reporting Ihor Solodrai 3 siblings, 1 reply; 16+ messages in thread From: Mykyta Yatsenko @ 2026-09-08 14:27 UTC (permalink / raw) To: bpf, ast, andrii, daniel, kernel-team, eddyz87, memxor, qmo, linux-perf-users, acme, namhyung Cc: Mykyta Yatsenko From: Mykyta Yatsenko <yatsenko@meta.com> The perf profiler uses a zero counter value to reject an invalid entry snapshot. However, zero is valid when the PMU has not scheduled the event. To reproduce the issue, fill the PMU with other events and profile a long-running BPF program for iTLB misses. In this test, the entry read returned 0. Use an armed flag to track successful entry reads. Reset the flag before each new read. The fexit path clears the flag when it consumes the snapshot. Fixes: fa853c4b839e ("perf stat: Enable counting events for BPF programs") Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> --- tools/perf/util/bpf_skel/bpf_prog_profiler.bpf.c | 26 ++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/tools/perf/util/bpf_skel/bpf_prog_profiler.bpf.c b/tools/perf/util/bpf_skel/bpf_prog_profiler.bpf.c index 97037d3b3d9f..7bc0db26e005 100644 --- a/tools/perf/util/bpf_skel/bpf_prog_profiler.bpf.c +++ b/tools/perf/util/bpf_skel/bpf_prog_profiler.bpf.c @@ -4,6 +4,11 @@ #include <bpf/bpf_helpers.h> #include <bpf/bpf_tracing.h> +struct profile_reading { + struct bpf_perf_event_value value; + bool armed; +}; + /* map of perf event fds, num_cpu * num_metric entries */ struct { __uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY); @@ -15,7 +20,7 @@ struct { struct { __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY); __uint(key_size, sizeof(__u32)); - __uint(value_size, sizeof(struct bpf_perf_event_value)); + __uint(value_size, sizeof(struct profile_reading)); __uint(max_entries, 1); } fentry_readings SEC(".maps"); @@ -33,7 +38,7 @@ SEC("fentry/XXX") int BPF_PROG(fentry_XXX) { __u32 key = bpf_get_smp_processor_id(); - struct bpf_perf_event_value *ptr; + struct profile_reading *ptr; __u32 zero = 0; long err; @@ -42,9 +47,12 @@ int BPF_PROG(fentry_XXX) if (!ptr) return 0; - err = bpf_perf_event_read_value(&events, key, ptr, sizeof(*ptr)); + ptr->armed = false; + err = bpf_perf_event_read_value(&events, key, &ptr->value, + sizeof(ptr->value)); if (err) return 0; + ptr->armed = true; return 0; } @@ -52,17 +60,19 @@ int BPF_PROG(fentry_XXX) static inline void fexit_update_maps(struct bpf_perf_event_value *after) { - struct bpf_perf_event_value *before, diff; + struct profile_reading *before; + struct bpf_perf_event_value diff; __u32 zero = 0; before = bpf_map_lookup_elem(&fentry_readings, &zero); /* only account samples with a valid fentry_reading */ - if (before && before->counter) { + if (before && before->armed) { struct bpf_perf_event_value *accum; - diff.counter = after->counter - before->counter; - diff.enabled = after->enabled - before->enabled; - diff.running = after->running - before->running; + before->armed = false; + diff.counter = after->counter - before->value.counter; + diff.enabled = after->enabled - before->value.enabled; + diff.running = after->running - before->value.running; accum = bpf_map_lookup_elem(&accum_readings, &zero); if (accum) { -- 2.53.0-Meta ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next v3 2/3] perf bpf_counter: Track valid BPF counter snapshots 2026-09-08 14:27 ` [PATCH bpf-next v3 2/3] perf bpf_counter: Track valid BPF counter snapshots Mykyta Yatsenko @ 2026-09-08 14:39 ` sashiko-bot 0 siblings, 0 replies; 16+ messages in thread From: sashiko-bot @ 2026-09-08 14:39 UTC (permalink / raw) To: Mykyta Yatsenko; +Cc: linux-perf-users, bpf > From: Mykyta Yatsenko <yatsenko@meta.com> > > The perf profiler uses a zero counter value to reject an invalid entry > snapshot. However, zero is valid when the PMU has not scheduled the event. > > To reproduce the issue, fill the PMU with other events and profile a > long-running BPF program for iTLB misses. In this test, the entry read > returned 0. > > Use an armed flag to track successful entry reads. Reset the flag before > each new read. The fexit path clears the flag when it consumes the > snapshot. > > Fixes: fa853c4b839e ("perf stat: Enable counting events for BPF programs") > Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> Sashiko has reviewed this patch and found no issues. It looks great! -- Sashiko AI review · https://sashiko.dev/#/patchset/20260908-bpftool_cyles_per_run-v3-0-60e86f325c35@meta.com?part=2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH bpf-next v3 3/3] bpftool: Scale counters and report cycles per run 2026-09-08 14:27 [PATCH bpf-next v3 0/3] bpftool: Improve perf counter reporting Mykyta Yatsenko 2026-09-08 14:27 ` [PATCH bpf-next v3 1/3] bpftool: Track perf counter snapshot state Mykyta Yatsenko 2026-09-08 14:27 ` [PATCH bpf-next v3 2/3] perf bpf_counter: Track valid BPF counter snapshots Mykyta Yatsenko @ 2026-09-08 14:27 ` Mykyta Yatsenko 2026-09-08 14:42 ` sashiko-bot ` (3 more replies) 2026-09-08 18:01 ` [PATCH bpf-next v3 0/3] bpftool: Improve perf counter reporting Ihor Solodrai 3 siblings, 4 replies; 16+ messages in thread From: Mykyta Yatsenko @ 2026-09-08 14:27 UTC (permalink / raw) To: bpf, ast, andrii, daniel, kernel-team, eddyz87, memxor, qmo, linux-perf-users, acme, namhyung Cc: Mykyta Yatsenko From: Mykyta Yatsenko <yatsenko@meta.com> Perf counters can report too few events when the PMU multiplexes them. Scale each per-CPU value before aggregation. Use the same CPU set for derived ratios. Report cycles per included program run, and preserve the total run count in JSON. Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command") Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> --- tools/bpf/bpftool/Documentation/bpftool-prog.rst | 13 +- tools/bpf/bpftool/prog.c | 159 +++++++++++++++++------ 2 files changed, 133 insertions(+), 39 deletions(-) diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst index 90fa2a48cc26..0108a1c8be4d 100644 --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst @@ -217,7 +217,16 @@ bpftool prog run *PROG* data_in *FILE* [data_out *FILE* [data_size_out *L*]] [ct bpftool prog profile *PROG* [duration *DURATION*] *METRICs* Profile *METRICs* for bpf program *PROG* for *DURATION* seconds or until user hits <Ctrl+C>. *DURATION* is optional. If *DURATION* is not specified, - the profiling will run up to **UINT_MAX** seconds. + the profiling will run up to **UINT_MAX** seconds. Plain output scales each + per-CPU metric value to correct for perf event multiplexing. When + **cycles** is selected, it also reports cycles per included program run. If + a selected metric was not scheduled on a CPU, all metric values exclude + that CPU so that ratios use a consistent CPU set. The **run_cnt** value + still includes all recorded runs. + + In JSON output, **value** is raw, **value_scaled** is scaled, **run_cnt** + includes all runs, and **run_cnt_valid** includes only runs used for metric + values. bpftool prog help Print short help message. @@ -360,7 +369,7 @@ EXAMPLES :: 51397 run_cnt - 40176203 cycles (83.05%) + 40176203 cycles # 781.68 cycles per run (83.05%) 42518139 instructions # 1.06 insns per cycle (83.39%) 123 llc_misses # 2.89 LLC misses per million insns (83.15%) diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c index 8c2f9255b36d..9e126f3823ad 100644 --- a/tools/bpf/bpftool/prog.c +++ b/tools/bpf/bpftool/prog.c @@ -2062,37 +2062,52 @@ static int do_profile(int argc, char **argv) #include "profiler.skel.h" +enum ratio_metric { + METRIC_NONE = -2, + METRIC_RUN_CNT = -1, + METRIC_CYCLES = 0, + METRIC_INSTRUCTIONS = 1, + METRIC_L1D_LOADS = 2, + METRIC_LLC_MISSES = 3, + METRIC_ITLB_MISSES = 4, + METRIC_DTLB_MISSES = 5, +}; + struct profile_metric { const char *name; struct bpf_perf_event_value val; + __u64 scaled_val; struct perf_event_attr attr; bool selected; /* calculate ratios like instructions per cycle */ - const int ratio_metric; /* 0 for N/A, 1 for index 0 (cycles) */ + const enum ratio_metric ratio_metric; const char *ratio_desc; const float ratio_mul; } metrics[] = { - { + [METRIC_CYCLES] = { .name = "cycles", .attr = { .type = PERF_TYPE_HARDWARE, .config = PERF_COUNT_HW_CPU_CYCLES, .exclude_user = 1, }, + .ratio_metric = METRIC_RUN_CNT, + .ratio_desc = "cycles per run", + .ratio_mul = 1.0, }, - { + [METRIC_INSTRUCTIONS] = { .name = "instructions", .attr = { .type = PERF_TYPE_HARDWARE, .config = PERF_COUNT_HW_INSTRUCTIONS, .exclude_user = 1, }, - .ratio_metric = 1, + .ratio_metric = METRIC_CYCLES, .ratio_desc = "insns per cycle", .ratio_mul = 1.0, }, - { + [METRIC_L1D_LOADS] = { .name = "l1d_loads", .attr = { .type = PERF_TYPE_HW_CACHE, @@ -2102,8 +2117,9 @@ struct profile_metric { (PERF_COUNT_HW_CACHE_RESULT_ACCESS << 16), .exclude_user = 1, }, + .ratio_metric = METRIC_NONE, }, - { + [METRIC_LLC_MISSES] = { .name = "llc_misses", .attr = { .type = PERF_TYPE_HW_CACHE, @@ -2113,11 +2129,11 @@ struct profile_metric { (PERF_COUNT_HW_CACHE_RESULT_MISS << 16), .exclude_user = 1 }, - .ratio_metric = 2, + .ratio_metric = METRIC_INSTRUCTIONS, .ratio_desc = "LLC misses per million insns", .ratio_mul = 1e6, }, - { + [METRIC_ITLB_MISSES] = { .name = "itlb_misses", .attr = { .type = PERF_TYPE_HW_CACHE, @@ -2127,11 +2143,11 @@ struct profile_metric { (PERF_COUNT_HW_CACHE_RESULT_MISS << 16), .exclude_user = 1 }, - .ratio_metric = 2, + .ratio_metric = METRIC_INSTRUCTIONS, .ratio_desc = "itlb misses per million insns", .ratio_mul = 1e6, }, - { + [METRIC_DTLB_MISSES] = { .name = "dtlb_misses", .attr = { .type = PERF_TYPE_HW_CACHE, @@ -2141,13 +2157,14 @@ struct profile_metric { (PERF_COUNT_HW_CACHE_RESULT_MISS << 16), .exclude_user = 1 }, - .ratio_metric = 2, + .ratio_metric = METRIC_INSTRUCTIONS, .ratio_desc = "dtlb misses per million insns", .ratio_mul = 1e6, }, }; static __u64 profile_total_count; +static __u64 profile_valid_count; #define MAX_NUM_PROFILE_METRICS 4 @@ -2182,9 +2199,39 @@ static int profile_parse_metrics(int argc, char **argv) return selected_cnt; } -static void profile_read_values(struct profiler_bpf *obj) +/* + * Filter out CPUs that have any selected metric not scheduled for them. This makes sure all + * metrics are using the same CPU set, as a result ratio metrics are consistent. + */ +static void profile_filter_cpus(__u32 num_cpu, + struct bpf_perf_event_value vals[MAX_NUM_PROFILE_METRICS][num_cpu], + __u64 *counts) +{ + __u32 m, cpu, key = 0; + + for (m = 0; m < ARRAY_SIZE(metrics); m++) { + if (!metrics[m].selected) + continue; + + for (cpu = 0; cpu < num_cpu; cpu++) { + /* + * CPU has hits, but this metric never scheduled, set counts[cpu] to 0 + * so other metrics ignore this CPU too + */ + if (counts[cpu] && !vals[key][cpu].running) { + p_info("%s not scheduled on CPU %u; excluding %llu runs from all metrics", + metrics[m].name, cpu, counts[cpu]); + counts[cpu] = 0; + } + } + key++; + } +} + +static int profile_read_values(struct profiler_bpf *obj) { __u32 m, cpu, num_cpu = obj->rodata->num_cpu; + struct bpf_perf_event_value values[MAX_NUM_PROFILE_METRICS][num_cpu], *val; int reading_map_fd, count_map_fd; __u64 counts[num_cpu]; __u32 key = 0; @@ -2194,38 +2241,61 @@ static void profile_read_values(struct profiler_bpf *obj) count_map_fd = bpf_map__fd(obj->maps.counts); if (reading_map_fd < 0 || count_map_fd < 0) { p_err("failed to get fd for map"); - return; + return min(reading_map_fd, count_map_fd); } err = bpf_map_lookup_elem(count_map_fd, &key, counts); if (err) { p_err("failed to read count_map: %s", strerror(errno)); - return; + return err; + } + + for (m = 0; m < ARRAY_SIZE(metrics); m++) { + if (!metrics[m].selected) + continue; + + err = bpf_map_lookup_elem(reading_map_fd, &key, values[key]); + if (err) { + p_err("failed to read reading_map: %s", strerror(errno)); + return err; + } + key++; } profile_total_count = 0; for (cpu = 0; cpu < num_cpu; cpu++) profile_total_count += counts[cpu]; + profile_filter_cpus(num_cpu, values, counts); + + profile_valid_count = 0; + for (cpu = 0; cpu < num_cpu; cpu++) + profile_valid_count += counts[cpu]; + + key = 0; for (m = 0; m < ARRAY_SIZE(metrics); m++) { - struct bpf_perf_event_value values[num_cpu]; + double scale; if (!metrics[m].selected) continue; - err = bpf_map_lookup_elem(reading_map_fd, &key, values); - if (err) { - p_err("failed to read reading_map: %s", - strerror(errno)); - return; - } for (cpu = 0; cpu < num_cpu; cpu++) { - metrics[m].val.counter += values[cpu].counter; - metrics[m].val.enabled += values[cpu].enabled; - metrics[m].val.running += values[cpu].running; + /* Skip CPUs with no runs or with an unscheduled metric. */ + if (!counts[cpu]) + continue; + + val = &values[key][cpu]; + + metrics[m].val.enabled += val->enabled; + metrics[m].val.running += val->running; + metrics[m].val.counter += val->counter; + /* Scale counter values to account for perf event multiplexing. */ + scale = (double)val->enabled / val->running; + metrics[m].scaled_val += val->counter * scale; } key++; } + return 0; } static void profile_print_readings_json(void) @@ -2239,9 +2309,11 @@ static void profile_print_readings_json(void) jsonw_start_object(json_wtr); jsonw_string_field(json_wtr, "metric", metrics[m].name); jsonw_lluint_field(json_wtr, "run_cnt", profile_total_count); + jsonw_lluint_field(json_wtr, "run_cnt_valid", profile_valid_count); jsonw_lluint_field(json_wtr, "value", metrics[m].val.counter); jsonw_lluint_field(json_wtr, "enabled", metrics[m].val.enabled); jsonw_lluint_field(json_wtr, "running", metrics[m].val.running); + jsonw_lluint_field(json_wtr, "value_scaled", metrics[m].scaled_val); jsonw_end_object(json_wtr); } @@ -2250,24 +2322,34 @@ static void profile_print_readings_json(void) static void profile_print_readings_plain(void) { - __u32 m; + __u32 i; printf("\n%18llu %-20s\n", profile_total_count, "run_cnt"); - for (m = 0; m < ARRAY_SIZE(metrics); m++) { - struct bpf_perf_event_value *val = &metrics[m].val; + for (i = 0; i < ARRAY_SIZE(metrics); i++) { + struct profile_metric *m = &metrics[i]; + struct bpf_perf_event_value *val = &m->val; int r; + __u64 ratio; - if (!metrics[m].selected) + if (!m->selected) continue; - printf("%18llu %-20s", val->counter, metrics[m].name); + printf("%18llu %-20s", m->scaled_val, m->name); - r = metrics[m].ratio_metric - 1; - if (r >= 0 && metrics[r].selected && - metrics[r].val.counter > 0) { + r = m->ratio_metric; + switch (r) { + case METRIC_RUN_CNT: + ratio = profile_valid_count; + break; + case METRIC_NONE: + ratio = 0; + break; + default: + ratio = metrics[r].scaled_val; + } + if (ratio) { printf("# %8.2f %-30s", - val->counter * metrics[m].ratio_mul / - metrics[r].val.counter, - metrics[m].ratio_desc); + m->scaled_val * m->ratio_mul / ratio, + m->ratio_desc); } else { printf("%-41s", ""); } @@ -2423,9 +2505,12 @@ static int profile_open_perf_events(struct profiler_bpf *obj) static void profile_print_and_cleanup(void) { + int err; + profile_close_perf_events(profile_obj); - profile_read_values(profile_obj); - profile_print_readings(); + err = profile_read_values(profile_obj); + if (!err) + profile_print_readings(); profiler_bpf__destroy(profile_obj); close(profile_tgt_fd); -- 2.53.0-Meta ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next v3 3/3] bpftool: Scale counters and report cycles per run 2026-09-08 14:27 ` [PATCH bpf-next v3 3/3] bpftool: Scale counters and report cycles per run Mykyta Yatsenko @ 2026-09-08 14:42 ` sashiko-bot 2026-09-08 16:21 ` bot+bpf-ci ` (2 subsequent siblings) 3 siblings, 0 replies; 16+ messages in thread From: sashiko-bot @ 2026-09-08 14:42 UTC (permalink / raw) To: Mykyta Yatsenko; +Cc: bpf, linux-perf-users > From: Mykyta Yatsenko <yatsenko@meta.com> > > Perf counters can report too few events when the PMU multiplexes them. > Scale each per-CPU value before aggregation. > > Use the same CPU set for derived ratios. Report cycles per included > program run, and preserve the total run count in JSON. > > Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command") > Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> Sashiko has reviewed this patch and found no issues. It looks great! -- Sashiko AI review · https://sashiko.dev/#/patchset/20260908-bpftool_cyles_per_run-v3-0-60e86f325c35@meta.com?part=3 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next v3 3/3] bpftool: Scale counters and report cycles per run 2026-09-08 14:27 ` [PATCH bpf-next v3 3/3] bpftool: Scale counters and report cycles per run Mykyta Yatsenko 2026-09-08 14:42 ` sashiko-bot @ 2026-09-08 16:21 ` bot+bpf-ci 2026-09-08 17:37 ` Mykyta Yatsenko 2026-09-08 22:08 ` Quentin Monnet 2026-09-09 0:02 ` Andrii Nakryiko 3 siblings, 1 reply; 16+ messages in thread From: bot+bpf-ci @ 2026-09-08 16:21 UTC (permalink / raw) To: mykyta.yatsenko5, bpf, ast, andrii, daniel, kernel-team, eddyz87, memxor, qmo, linux-perf-users, acme, namhyung Cc: yatsenko, ast, andrii, daniel, martin.lau, eddyz87, yonghong.song, mason, ihor.solodrai [-- Attachment #1: Type: text/plain, Size: 7699 bytes --] > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > index 8c2f9255b36d..9e126f3823ad 100644 > --- a/tools/bpf/bpftool/prog.c > +++ b/tools/bpf/bpftool/prog.c [ ... ] > @@ -2182,9 +2199,39 @@ static int profile_parse_metrics(int argc, char **argv) > return selected_cnt; > } > > +/* > + * Filter out CPUs that have any selected metric not scheduled for them. This makes sure all > + * metrics are using the same CPU set, as a result ratio metrics are consistent. > + */ > +static void profile_filter_cpus(__u32 num_cpu, > + struct bpf_perf_event_value vals[MAX_NUM_PROFILE_METRICS][num_cpu], > + __u64 *counts) > +{ > + __u32 m, cpu, key = 0; > + > + for (m = 0; m < ARRAY_SIZE(metrics); m++) { > + if (!metrics[m].selected) > + continue; > + > + for (cpu = 0; cpu < num_cpu; cpu++) { > + /* > + * CPU has hits, but this metric never scheduled, set counts[cpu] to 0 > + * so other metrics ignore this CPU too > + */ > + if (counts[cpu] && !vals[key][cpu].running) { > + p_info("%s not scheduled on CPU %u; excluding %llu runs from all metrics", > + metrics[m].name, cpu, counts[cpu]); > + counts[cpu] = 0; > + } > + } > + key++; > + } > +} [ ... ] > @@ -2194,38 +2241,61 @@ static void profile_read_values(struct profiler_bpf *obj) > count_map_fd = bpf_map__fd(obj->maps.counts); > if (reading_map_fd < 0 || count_map_fd < 0) { > p_err("failed to get fd for map"); > - return; > + return min(reading_map_fd, count_map_fd); > } > > err = bpf_map_lookup_elem(count_map_fd, &key, counts); > if (err) { > p_err("failed to read count_map: %s", strerror(errno)); > - return; > + return err; > + } > + > + for (m = 0; m < ARRAY_SIZE(metrics); m++) { > + if (!metrics[m].selected) > + continue; > + > + err = bpf_map_lookup_elem(reading_map_fd, &key, values[key]); > + if (err) { > + p_err("failed to read reading_map: %s", strerror(errno)); > + return err; > + } > + key++; > } > > profile_total_count = 0; > for (cpu = 0; cpu < num_cpu; cpu++) > profile_total_count += counts[cpu]; > > + profile_filter_cpus(num_cpu, values, counts); > + > + profile_valid_count = 0; > + for (cpu = 0; cpu < num_cpu; cpu++) > + profile_valid_count += counts[cpu]; > + > + key = 0; > for (m = 0; m < ARRAY_SIZE(metrics); m++) { > - struct bpf_perf_event_value values[num_cpu]; > + double scale; > > if (!metrics[m].selected) > continue; > > - err = bpf_map_lookup_elem(reading_map_fd, &key, values); > - if (err) { > - p_err("failed to read reading_map: %s", > - strerror(errno)); > - return; > - } > for (cpu = 0; cpu < num_cpu; cpu++) { > - metrics[m].val.counter += values[cpu].counter; > - metrics[m].val.enabled += values[cpu].enabled; > - metrics[m].val.running += values[cpu].running; > + /* Skip CPUs with no runs or with an unscheduled metric. */ > + if (!counts[cpu]) > + continue; > + > + val = &values[key][cpu]; > + > + metrics[m].val.enabled += val->enabled; > + metrics[m].val.running += val->running; > + metrics[m].val.counter += val->counter; > + /* Scale counter values to account for perf event multiplexing. */ > + scale = (double)val->enabled / val->running; > + metrics[m].scaled_val += val->counter * scale; > } > key++; > } > + return 0; > } Does this correctly handle the case where one metric is never scheduled on any CPU? The CPU-set filter in profile_filter_cpus() zeroes counts[cpu] for every CPU where any selected metric has running == 0: if (counts[cpu] && !vals[key][cpu].running) { p_info("%s not scheduled on CPU %u; ..."); counts[cpu] = 0; } The aggregation loop then skips those CPUs for all metrics: for (cpu = 0; cpu < num_cpu; cpu++) { if (!counts[cpu]) continue; ... metrics[m].val.counter += val->counter; ... metrics[m].scaled_val += val->counter * scale; } If a selected metric is never scheduled on any CPU (the scenario this series targets - commit 44f919dd1fbc describes profiling iTLB misses on a long-running program with a full PMU), then every CPU with counts[cpu] != 0 gets zeroed. For example, `bpftool prog profile <prog> cycles itlb_misses`: * profile_valid_count becomes 0 * cycles' val.counter and scaled_val stay 0 even though the cycles event was measured correctly on every CPU * plain output prints "run_cnt 51397" followed by "0 cycles" and "0 itlb_misses" with no ratio column Before this patch the same run reported the real cycles count and 0 for itlb_misses, so the healthy metric's data is lost. In JSON mode there is no diagnostic at all. The p_info() call in profile_filter_cpus() returns immediately when json_output is set, so a consumer sees run_cnt=51397 with value=0, value_scaled=0, enabled=0, running=0, and only run_cnt_valid=0 as a hint. The commit message scopes the change to ratios ("Use the same CPU set for derived ratios"), and the .rst says "all metric values exclude that CPU so that ratios use a consistent CPU set" but neither documents that a single unscheduled metric collapses the whole report to zeros, and JSON gives no warning. Would it make sense to exclude only the affected metric, or keep the raw per-metric aggregation intact and restrict only the ratio denominators? > @@ -2250,24 +2322,34 @@ static void profile_print_readings_json(void) > > static void profile_print_readings_plain(void) > { > - __u32 m; > + __u32 i; > > printf("\n%18llu %-20s\n", profile_total_count, "run_cnt"); > - for (m = 0; m < ARRAY_SIZE(metrics); m++) { > - struct bpf_perf_event_value *val = &metrics[m].val; > + for (i = 0; i < ARRAY_SIZE(metrics); i++) { > + struct profile_metric *m = &metrics[i]; > + struct bpf_perf_event_value *val = &m->val; > int r; > + __u64 ratio; > > - if (!metrics[m].selected) > + if (!m->selected) > continue; > - printf("%18llu %-20s", val->counter, metrics[m].name); > + printf("%18llu %-20s", m->scaled_val, m->name); > > - r = metrics[m].ratio_metric - 1; > - if (r >= 0 && metrics[r].selected && > - metrics[r].val.counter > 0) { > + r = m->ratio_metric; > + switch (r) { > + case METRIC_RUN_CNT: > + ratio = profile_valid_count; > + break; > + case METRIC_NONE: > + ratio = 0; > + break; > + default: > + ratio = metrics[r].scaled_val; > + } > + if (ratio) { > printf("# %8.2f %-30s", > - val->counter * metrics[m].ratio_mul / > - metrics[r].val.counter, > - metrics[m].ratio_desc); > + m->scaled_val * m->ratio_mul / ratio, > + m->ratio_desc); > } else { > printf("%-41s", ""); > } Plain output prints profile_total_count as "run_cnt" but the new "cycles per run" column divides by profile_valid_count, and profile_valid_count is never printed in plain mode (it is only exposed as the JSON run_cnt_valid field). Whenever profile_filter_cpus() excludes at least one CPU the two differ, so the displayed numbers cannot be reconciled. The user sees run_cnt and a scaled cycles value, and "# <x> cycles per run" where x != cycles/run_cnt, with no way to see the denominator that was actually used. The only signal is the p_info() lines on stderr, which are easy to miss when stdout is redirected and are suppressed entirely under --json. Would it make sense to print the valid run count in plain mode (as JSON now does) or label the ratio with the CPU set it was computed over? --- AI reviewed your patch. Please fix the bug or email reply why it's not a bug. See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34246064212 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next v3 3/3] bpftool: Scale counters and report cycles per run 2026-09-08 16:21 ` bot+bpf-ci @ 2026-09-08 17:37 ` Mykyta Yatsenko 0 siblings, 0 replies; 16+ messages in thread From: Mykyta Yatsenko @ 2026-09-08 17:37 UTC (permalink / raw) To: bot+bpf-ci, bpf, ast, andrii, daniel, kernel-team, eddyz87, memxor, qmo, linux-perf-users, acme, namhyung Cc: yatsenko, martin.lau, yonghong.song, mason, ihor.solodrai On 9/8/26 5:21 PM, bot+bpf-ci@kernel.org wrote: >> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c >> index 8c2f9255b36d..9e126f3823ad 100644 >> --- a/tools/bpf/bpftool/prog.c >> +++ b/tools/bpf/bpftool/prog.c > > [ ... ] > >> @@ -2182,9 +2199,39 @@ static int profile_parse_metrics(int argc, char **argv) >> return selected_cnt; >> } >> >> +/* >> + * Filter out CPUs that have any selected metric not scheduled for them. This makes sure all >> + * metrics are using the same CPU set, as a result ratio metrics are consistent. >> + */ >> +static void profile_filter_cpus(__u32 num_cpu, >> + struct bpf_perf_event_value vals[MAX_NUM_PROFILE_METRICS][num_cpu], >> + __u64 *counts) >> +{ >> + __u32 m, cpu, key = 0; >> + >> + for (m = 0; m < ARRAY_SIZE(metrics); m++) { >> + if (!metrics[m].selected) >> + continue; >> + >> + for (cpu = 0; cpu < num_cpu; cpu++) { >> + /* >> + * CPU has hits, but this metric never scheduled, set counts[cpu] to 0 >> + * so other metrics ignore this CPU too >> + */ >> + if (counts[cpu] && !vals[key][cpu].running) { >> + p_info("%s not scheduled on CPU %u; excluding %llu runs from all metrics", >> + metrics[m].name, cpu, counts[cpu]); >> + counts[cpu] = 0; >> + } >> + } >> + key++; >> + } >> +} > > [ ... ] > >> @@ -2194,38 +2241,61 @@ static void profile_read_values(struct profiler_bpf *obj) >> count_map_fd = bpf_map__fd(obj->maps.counts); >> if (reading_map_fd < 0 || count_map_fd < 0) { >> p_err("failed to get fd for map"); >> - return; >> + return min(reading_map_fd, count_map_fd); >> } >> >> err = bpf_map_lookup_elem(count_map_fd, &key, counts); >> if (err) { >> p_err("failed to read count_map: %s", strerror(errno)); >> - return; >> + return err; >> + } >> + >> + for (m = 0; m < ARRAY_SIZE(metrics); m++) { >> + if (!metrics[m].selected) >> + continue; >> + >> + err = bpf_map_lookup_elem(reading_map_fd, &key, values[key]); >> + if (err) { >> + p_err("failed to read reading_map: %s", strerror(errno)); >> + return err; >> + } >> + key++; >> } >> >> profile_total_count = 0; >> for (cpu = 0; cpu < num_cpu; cpu++) >> profile_total_count += counts[cpu]; >> >> + profile_filter_cpus(num_cpu, values, counts); >> + >> + profile_valid_count = 0; >> + for (cpu = 0; cpu < num_cpu; cpu++) >> + profile_valid_count += counts[cpu]; >> + >> + key = 0; >> for (m = 0; m < ARRAY_SIZE(metrics); m++) { >> - struct bpf_perf_event_value values[num_cpu]; >> + double scale; >> >> if (!metrics[m].selected) >> continue; >> >> - err = bpf_map_lookup_elem(reading_map_fd, &key, values); >> - if (err) { >> - p_err("failed to read reading_map: %s", >> - strerror(errno)); >> - return; >> - } >> for (cpu = 0; cpu < num_cpu; cpu++) { >> - metrics[m].val.counter += values[cpu].counter; >> - metrics[m].val.enabled += values[cpu].enabled; >> - metrics[m].val.running += values[cpu].running; >> + /* Skip CPUs with no runs or with an unscheduled metric. */ >> + if (!counts[cpu]) >> + continue; >> + >> + val = &values[key][cpu]; >> + >> + metrics[m].val.enabled += val->enabled; >> + metrics[m].val.running += val->running; >> + metrics[m].val.counter += val->counter; >> + /* Scale counter values to account for perf event multiplexing. */ >> + scale = (double)val->enabled / val->running; >> + metrics[m].scaled_val += val->counter * scale; >> } >> key++; >> } >> + return 0; >> } > > Does this correctly handle the case where one metric is never scheduled > on any CPU? > > The CPU-set filter in profile_filter_cpus() zeroes counts[cpu] for every > CPU where any selected metric has running == 0: This would be an unusual edge case, perf counter normally get scheduled for some fraction of time, even when multiplexed. > if (counts[cpu] && !vals[key][cpu].running) { > p_info("%s not scheduled on CPU %u; ..."); > counts[cpu] = 0; > } > > The aggregation loop then skips those CPUs for all metrics: > > for (cpu = 0; cpu < num_cpu; cpu++) { > if (!counts[cpu]) > continue; > ... > metrics[m].val.counter += val->counter; > ... > metrics[m].scaled_val += val->counter * scale; > } > > If a selected metric is never scheduled on any CPU (the scenario this > series targets - commit 44f919dd1fbc describes profiling iTLB misses on > a long-running program with a full PMU), then every CPU with counts[cpu] > != 0 gets zeroed. > > For example, `bpftool prog profile <prog> cycles itlb_misses`: > > * profile_valid_count becomes 0 > * cycles' val.counter and scaled_val stay 0 even though the cycles > event was measured correctly on every CPU > * plain output prints "run_cnt 51397" followed by "0 cycles" and > "0 itlb_misses" with no ratio column > > Before this patch the same run reported the real cycles count and 0 for > itlb_misses, so the healthy metric's data is lost. > > In JSON mode there is no diagnostic at all. The p_info() call in > profile_filter_cpus() returns immediately when json_output is set, so > a consumer sees run_cnt=51397 with value=0, value_scaled=0, enabled=0, > running=0, and only run_cnt_valid=0 as a hint. > > The commit message scopes the change to ratios ("Use the same CPU set > for derived ratios"), and the .rst says "all metric values exclude that > CPU so that ratios use a consistent CPU set" but neither documents that > a single unscheduled metric collapses the whole report to zeros, and > JSON gives no warning. > > Would it make sense to exclude only the affected metric, or keep the raw > per-metric aggregation intact and restrict only the ratio denominators? > >> @@ -2250,24 +2322,34 @@ static void profile_print_readings_json(void) >> >> static void profile_print_readings_plain(void) >> { >> - __u32 m; >> + __u32 i; >> >> printf("\n%18llu %-20s\n", profile_total_count, "run_cnt"); >> - for (m = 0; m < ARRAY_SIZE(metrics); m++) { >> - struct bpf_perf_event_value *val = &metrics[m].val; >> + for (i = 0; i < ARRAY_SIZE(metrics); i++) { >> + struct profile_metric *m = &metrics[i]; >> + struct bpf_perf_event_value *val = &m->val; >> int r; >> + __u64 ratio; >> >> - if (!metrics[m].selected) >> + if (!m->selected) >> continue; >> - printf("%18llu %-20s", val->counter, metrics[m].name); >> + printf("%18llu %-20s", m->scaled_val, m->name); >> >> - r = metrics[m].ratio_metric - 1; >> - if (r >= 0 && metrics[r].selected && >> - metrics[r].val.counter > 0) { >> + r = m->ratio_metric; >> + switch (r) { >> + case METRIC_RUN_CNT: >> + ratio = profile_valid_count; >> + break; >> + case METRIC_NONE: >> + ratio = 0; >> + break; >> + default: >> + ratio = metrics[r].scaled_val; >> + } >> + if (ratio) { >> printf("# %8.2f %-30s", >> - val->counter * metrics[m].ratio_mul / >> - metrics[r].val.counter, >> - metrics[m].ratio_desc); >> + m->scaled_val * m->ratio_mul / ratio, >> + m->ratio_desc); >> } else { >> printf("%-41s", ""); >> } > > Plain output prints profile_total_count as "run_cnt" but the new "cycles > per run" column divides by profile_valid_count, and profile_valid_count > is never printed in plain mode (it is only exposed as the JSON > run_cnt_valid field). > > Whenever profile_filter_cpus() excludes at least one CPU the two differ, > so the displayed numbers cannot be reconciled. The user sees run_cnt and > a scaled cycles value, and "# <x> cycles per run" where > x != cycles/run_cnt, with no way to see the denominator that was > actually used. > > The only signal is the p_info() lines on stderr, which are easy to miss > when stdout is redirected and are suppressed entirely under --json. > > Would it make sense to print the valid run count in plain mode (as JSON > now does) or label the ratio with the CPU set it was computed over? > > > --- > AI reviewed your patch. Please fix the bug or email reply why it's not a bug. > See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md > > CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34246064212 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next v3 3/3] bpftool: Scale counters and report cycles per run 2026-09-08 14:27 ` [PATCH bpf-next v3 3/3] bpftool: Scale counters and report cycles per run Mykyta Yatsenko 2026-09-08 14:42 ` sashiko-bot 2026-09-08 16:21 ` bot+bpf-ci @ 2026-09-08 22:08 ` Quentin Monnet 2026-09-09 0:02 ` Andrii Nakryiko 3 siblings, 0 replies; 16+ messages in thread From: Quentin Monnet @ 2026-09-08 22:08 UTC (permalink / raw) To: Mykyta Yatsenko, bpf, ast, andrii, daniel, kernel-team, eddyz87, memxor, linux-perf-users, acme, namhyung Cc: Mykyta Yatsenko 2026-09-08 07:27 UTC-0700 ~ Mykyta Yatsenko <mykyta.yatsenko5@gmail.com> > From: Mykyta Yatsenko <yatsenko@meta.com> > > Perf counters can report too few events when the PMU multiplexes them. > Scale each per-CPU value before aggregation. > > Use the same CPU set for derived ratios. Report cycles per included > program run, and preserve the total run count in JSON. > > Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command") > Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> Thank you for this! Acked-by: Quentin Monnet <qmo@kernel.org> Apologies for missing the review (and your question) on the previous versions of the patchset. Quentin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next v3 3/3] bpftool: Scale counters and report cycles per run 2026-09-08 14:27 ` [PATCH bpf-next v3 3/3] bpftool: Scale counters and report cycles per run Mykyta Yatsenko ` (2 preceding siblings ...) 2026-09-08 22:08 ` Quentin Monnet @ 2026-09-09 0:02 ` Andrii Nakryiko 2026-09-09 9:49 ` Mykyta Yatsenko 3 siblings, 1 reply; 16+ messages in thread From: Andrii Nakryiko @ 2026-09-09 0:02 UTC (permalink / raw) To: Mykyta Yatsenko Cc: bpf, ast, andrii, daniel, kernel-team, eddyz87, memxor, qmo, linux-perf-users, acme, namhyung, Mykyta Yatsenko On Tue, Sep 8, 2026 at 7:27 AM Mykyta Yatsenko <mykyta.yatsenko5@gmail.com> wrote: > > From: Mykyta Yatsenko <yatsenko@meta.com> > > Perf counters can report too few events when the PMU multiplexes them. > Scale each per-CPU value before aggregation. > > Use the same CPU set for derived ratios. Report cycles per included > program run, and preserve the total run count in JSON. > > Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command") > Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> > --- > tools/bpf/bpftool/Documentation/bpftool-prog.rst | 13 +- > tools/bpf/bpftool/prog.c | 159 +++++++++++++++++------ > 2 files changed, 133 insertions(+), 39 deletions(-) > > diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst > index 90fa2a48cc26..0108a1c8be4d 100644 > --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst > +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst > @@ -217,7 +217,16 @@ bpftool prog run *PROG* data_in *FILE* [data_out *FILE* [data_size_out *L*]] [ct > bpftool prog profile *PROG* [duration *DURATION*] *METRICs* > Profile *METRICs* for bpf program *PROG* for *DURATION* seconds or until > user hits <Ctrl+C>. *DURATION* is optional. If *DURATION* is not specified, > - the profiling will run up to **UINT_MAX** seconds. > + the profiling will run up to **UINT_MAX** seconds. Plain output scales each > + per-CPU metric value to correct for perf event multiplexing. When > + **cycles** is selected, it also reports cycles per included program run. If > + a selected metric was not scheduled on a CPU, all metric values exclude > + that CPU so that ratios use a consistent CPU set. The **run_cnt** value > + still includes all recorded runs. > + > + In JSON output, **value** is raw, **value_scaled** is scaled, **run_cnt** > + includes all runs, and **run_cnt_valid** includes only runs used for metric > + values. > > bpftool prog help > Print short help message. > @@ -360,7 +369,7 @@ EXAMPLES > :: > > 51397 run_cnt > - 40176203 cycles (83.05%) > + 40176203 cycles # 781.68 cycles per run (83.05%) > 42518139 instructions # 1.06 insns per cycle (83.39%) > 123 llc_misses # 2.89 LLC misses per million insns (83.15%) > > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > index 8c2f9255b36d..9e126f3823ad 100644 > --- a/tools/bpf/bpftool/prog.c > +++ b/tools/bpf/bpftool/prog.c > @@ -2062,37 +2062,52 @@ static int do_profile(int argc, char **argv) > > #include "profiler.skel.h" > > +enum ratio_metric { > + METRIC_NONE = -2, > + METRIC_RUN_CNT = -1, > + METRIC_CYCLES = 0, > + METRIC_INSTRUCTIONS = 1, > + METRIC_L1D_LOADS = 2, > + METRIC_LLC_MISSES = 3, > + METRIC_ITLB_MISSES = 4, > + METRIC_DTLB_MISSES = 5, > +}; > + > struct profile_metric { > const char *name; > struct bpf_perf_event_value val; > + __u64 scaled_val; > struct perf_event_attr attr; > bool selected; > > /* calculate ratios like instructions per cycle */ > - const int ratio_metric; /* 0 for N/A, 1 for index 0 (cycles) */ > + const enum ratio_metric ratio_metric; > const char *ratio_desc; > const float ratio_mul; > } metrics[] = { > - { > + [METRIC_CYCLES] = { > .name = "cycles", > .attr = { > .type = PERF_TYPE_HARDWARE, > .config = PERF_COUNT_HW_CPU_CYCLES, > .exclude_user = 1, > }, > + .ratio_metric = METRIC_RUN_CNT, > + .ratio_desc = "cycles per run", > + .ratio_mul = 1.0, > }, > - { > + [METRIC_INSTRUCTIONS] = { > .name = "instructions", > .attr = { > .type = PERF_TYPE_HARDWARE, > .config = PERF_COUNT_HW_INSTRUCTIONS, > .exclude_user = 1, > }, > - .ratio_metric = 1, > + .ratio_metric = METRIC_CYCLES, > .ratio_desc = "insns per cycle", > .ratio_mul = 1.0, > }, > - { > + [METRIC_L1D_LOADS] = { > .name = "l1d_loads", > .attr = { > .type = PERF_TYPE_HW_CACHE, > @@ -2102,8 +2117,9 @@ struct profile_metric { > (PERF_COUNT_HW_CACHE_RESULT_ACCESS << 16), > .exclude_user = 1, > }, > + .ratio_metric = METRIC_NONE, > }, > - { > + [METRIC_LLC_MISSES] = { > .name = "llc_misses", > .attr = { > .type = PERF_TYPE_HW_CACHE, > @@ -2113,11 +2129,11 @@ struct profile_metric { > (PERF_COUNT_HW_CACHE_RESULT_MISS << 16), > .exclude_user = 1 > }, > - .ratio_metric = 2, > + .ratio_metric = METRIC_INSTRUCTIONS, > .ratio_desc = "LLC misses per million insns", > .ratio_mul = 1e6, > }, > - { > + [METRIC_ITLB_MISSES] = { > .name = "itlb_misses", > .attr = { > .type = PERF_TYPE_HW_CACHE, > @@ -2127,11 +2143,11 @@ struct profile_metric { > (PERF_COUNT_HW_CACHE_RESULT_MISS << 16), > .exclude_user = 1 > }, > - .ratio_metric = 2, > + .ratio_metric = METRIC_INSTRUCTIONS, > .ratio_desc = "itlb misses per million insns", > .ratio_mul = 1e6, > }, > - { > + [METRIC_DTLB_MISSES] = { > .name = "dtlb_misses", > .attr = { > .type = PERF_TYPE_HW_CACHE, > @@ -2141,13 +2157,14 @@ struct profile_metric { > (PERF_COUNT_HW_CACHE_RESULT_MISS << 16), > .exclude_user = 1 > }, > - .ratio_metric = 2, > + .ratio_metric = METRIC_INSTRUCTIONS, > .ratio_desc = "dtlb misses per million insns", > .ratio_mul = 1e6, > }, > }; > > static __u64 profile_total_count; > +static __u64 profile_valid_count; > > #define MAX_NUM_PROFILE_METRICS 4 > > @@ -2182,9 +2199,39 @@ static int profile_parse_metrics(int argc, char **argv) > return selected_cnt; > } > > -static void profile_read_values(struct profiler_bpf *obj) > +/* > + * Filter out CPUs that have any selected metric not scheduled for them. This makes sure all > + * metrics are using the same CPU set, as a result ratio metrics are consistent. > + */ > +static void profile_filter_cpus(__u32 num_cpu, > + struct bpf_perf_event_value vals[MAX_NUM_PROFILE_METRICS][num_cpu], > + __u64 *counts) > +{ > + __u32 m, cpu, key = 0; > + > + for (m = 0; m < ARRAY_SIZE(metrics); m++) { > + if (!metrics[m].selected) > + continue; > + > + for (cpu = 0; cpu < num_cpu; cpu++) { > + /* > + * CPU has hits, but this metric never scheduled, set counts[cpu] to 0 > + * so other metrics ignore this CPU too > + */ > + if (counts[cpu] && !vals[key][cpu].running) { > + p_info("%s not scheduled on CPU %u; excluding %llu runs from all metrics", > + metrics[m].name, cpu, counts[cpu]); > + counts[cpu] = 0; > + } > + } > + key++; > + } > +} was this suggested by AI or we actually ran into issues due to this in practice? this looks perculiar > + > +static int profile_read_values(struct profiler_bpf *obj) > { > __u32 m, cpu, num_cpu = obj->rodata->num_cpu; > + struct bpf_perf_event_value values[MAX_NUM_PROFILE_METRICS][num_cpu], *val; > int reading_map_fd, count_map_fd; > __u64 counts[num_cpu]; > __u32 key = 0; > @@ -2194,38 +2241,61 @@ static void profile_read_values(struct profiler_bpf *obj) > count_map_fd = bpf_map__fd(obj->maps.counts); > if (reading_map_fd < 0 || count_map_fd < 0) { this can't happen if skeleton loaded successfully, just remove this check instead of weird min() over fds/errors > p_err("failed to get fd for map"); > - return; > + return min(reading_map_fd, count_map_fd); > } > > err = bpf_map_lookup_elem(count_map_fd, &key, counts); > if (err) { > p_err("failed to read count_map: %s", strerror(errno)); > - return; > + return err; > + } > + > + for (m = 0; m < ARRAY_SIZE(metrics); m++) { > + if (!metrics[m].selected) > + continue; > + > + err = bpf_map_lookup_elem(reading_map_fd, &key, values[key]); > + if (err) { > + p_err("failed to read reading_map: %s", strerror(errno)); > + return err; > + } > + key++; > } > > profile_total_count = 0; > for (cpu = 0; cpu < num_cpu; cpu++) > profile_total_count += counts[cpu]; > > + profile_filter_cpus(num_cpu, values, counts); > + > + profile_valid_count = 0; > + for (cpu = 0; cpu < num_cpu; cpu++) > + profile_valid_count += counts[cpu]; > + > + key = 0; > for (m = 0; m < ARRAY_SIZE(metrics); m++) { > - struct bpf_perf_event_value values[num_cpu]; > + double scale; > > if (!metrics[m].selected) > continue; > > - err = bpf_map_lookup_elem(reading_map_fd, &key, values); > - if (err) { > - p_err("failed to read reading_map: %s", > - strerror(errno)); > - return; > - } > for (cpu = 0; cpu < num_cpu; cpu++) { > - metrics[m].val.counter += values[cpu].counter; > - metrics[m].val.enabled += values[cpu].enabled; > - metrics[m].val.running += values[cpu].running; > + /* Skip CPUs with no runs or with an unscheduled metric. */ > + if (!counts[cpu]) > + continue; > + > + val = &values[key][cpu]; > + > + metrics[m].val.enabled += val->enabled; > + metrics[m].val.running += val->running; > + metrics[m].val.counter += val->counter; > + /* Scale counter values to account for perf event multiplexing. */ > + scale = (double)val->enabled / val->running; > + metrics[m].scaled_val += val->counter * scale; > } > key++; > } > + return 0; > } > > static void profile_print_readings_json(void) > @@ -2239,9 +2309,11 @@ static void profile_print_readings_json(void) > jsonw_start_object(json_wtr); > jsonw_string_field(json_wtr, "metric", metrics[m].name); > jsonw_lluint_field(json_wtr, "run_cnt", profile_total_count); > + jsonw_lluint_field(json_wtr, "run_cnt_valid", profile_valid_count); aren't we just overcomplicating things for no good reason?.. > jsonw_lluint_field(json_wtr, "value", metrics[m].val.counter); > jsonw_lluint_field(json_wtr, "enabled", metrics[m].val.enabled); > jsonw_lluint_field(json_wtr, "running", metrics[m].val.running); > + jsonw_lluint_field(json_wtr, "value_scaled", metrics[m].scaled_val); > > jsonw_end_object(json_wtr); > } > @@ -2250,24 +2322,34 @@ static void profile_print_readings_json(void) > > static void profile_print_readings_plain(void) > { > - __u32 m; > + __u32 i; > > printf("\n%18llu %-20s\n", profile_total_count, "run_cnt"); > - for (m = 0; m < ARRAY_SIZE(metrics); m++) { > - struct bpf_perf_event_value *val = &metrics[m].val; > + for (i = 0; i < ARRAY_SIZE(metrics); i++) { > + struct profile_metric *m = &metrics[i]; > + struct bpf_perf_event_value *val = &m->val; > int r; > + __u64 ratio; > > - if (!metrics[m].selected) > + if (!m->selected) > continue; > - printf("%18llu %-20s", val->counter, metrics[m].name); > + printf("%18llu %-20s", m->scaled_val, m->name); > > - r = metrics[m].ratio_metric - 1; > - if (r >= 0 && metrics[r].selected && > - metrics[r].val.counter > 0) { > + r = m->ratio_metric; > + switch (r) { > + case METRIC_RUN_CNT: > + ratio = profile_valid_count; > + break; > + case METRIC_NONE: > + ratio = 0; > + break; > + default: > + ratio = metrics[r].scaled_val; > + } > + if (ratio) { > printf("# %8.2f %-30s", > - val->counter * metrics[m].ratio_mul / > - metrics[r].val.counter, > - metrics[m].ratio_desc); > + m->scaled_val * m->ratio_mul / ratio, > + m->ratio_desc); > } else { > printf("%-41s", ""); > } > @@ -2423,9 +2505,12 @@ static int profile_open_perf_events(struct profiler_bpf *obj) > > static void profile_print_and_cleanup(void) > { > + int err; > + > profile_close_perf_events(profile_obj); > - profile_read_values(profile_obj); > - profile_print_readings(); > + err = profile_read_values(profile_obj); > + if (!err) > + profile_print_readings(); > profiler_bpf__destroy(profile_obj); > > close(profile_tgt_fd); > > -- > 2.53.0-Meta > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next v3 3/3] bpftool: Scale counters and report cycles per run 2026-09-09 0:02 ` Andrii Nakryiko @ 2026-09-09 9:49 ` Mykyta Yatsenko 0 siblings, 0 replies; 16+ messages in thread From: Mykyta Yatsenko @ 2026-09-09 9:49 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, ast, andrii, daniel, kernel-team, eddyz87, memxor, qmo, linux-perf-users, acme, namhyung, Mykyta Yatsenko On 9/9/26 1:02 AM, Andrii Nakryiko wrote: > On Tue, Sep 8, 2026 at 7:27 AM Mykyta Yatsenko > <mykyta.yatsenko5@gmail.com> wrote: >> >> From: Mykyta Yatsenko <yatsenko@meta.com> >> >> Perf counters can report too few events when the PMU multiplexes them. >> Scale each per-CPU value before aggregation. >> >> Use the same CPU set for derived ratios. Report cycles per included >> program run, and preserve the total run count in JSON. >> >> Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command") >> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> >> --- >> tools/bpf/bpftool/Documentation/bpftool-prog.rst | 13 +- >> tools/bpf/bpftool/prog.c | 159 +++++++++++++++++------ >> 2 files changed, 133 insertions(+), 39 deletions(-) >> >> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst >> index 90fa2a48cc26..0108a1c8be4d 100644 >> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst >> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst >> @@ -217,7 +217,16 @@ bpftool prog run *PROG* data_in *FILE* [data_out *FILE* [data_size_out *L*]] [ct >> bpftool prog profile *PROG* [duration *DURATION*] *METRICs* >> Profile *METRICs* for bpf program *PROG* for *DURATION* seconds or until >> user hits <Ctrl+C>. *DURATION* is optional. If *DURATION* is not specified, >> - the profiling will run up to **UINT_MAX** seconds. >> + the profiling will run up to **UINT_MAX** seconds. Plain output scales each >> + per-CPU metric value to correct for perf event multiplexing. When >> + **cycles** is selected, it also reports cycles per included program run. If >> + a selected metric was not scheduled on a CPU, all metric values exclude >> + that CPU so that ratios use a consistent CPU set. The **run_cnt** value >> + still includes all recorded runs. >> + >> + In JSON output, **value** is raw, **value_scaled** is scaled, **run_cnt** >> + includes all runs, and **run_cnt_valid** includes only runs used for metric >> + values. >> >> bpftool prog help >> Print short help message. >> @@ -360,7 +369,7 @@ EXAMPLES >> :: >> >> 51397 run_cnt >> - 40176203 cycles (83.05%) >> + 40176203 cycles # 781.68 cycles per run (83.05%) >> 42518139 instructions # 1.06 insns per cycle (83.39%) >> 123 llc_misses # 2.89 LLC misses per million insns (83.15%) >> >> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c >> index 8c2f9255b36d..9e126f3823ad 100644 >> --- a/tools/bpf/bpftool/prog.c >> +++ b/tools/bpf/bpftool/prog.c >> @@ -2062,37 +2062,52 @@ static int do_profile(int argc, char **argv) >> >> #include "profiler.skel.h" >> >> +enum ratio_metric { >> + METRIC_NONE = -2, >> + METRIC_RUN_CNT = -1, >> + METRIC_CYCLES = 0, >> + METRIC_INSTRUCTIONS = 1, >> + METRIC_L1D_LOADS = 2, >> + METRIC_LLC_MISSES = 3, >> + METRIC_ITLB_MISSES = 4, >> + METRIC_DTLB_MISSES = 5, >> +}; >> + >> struct profile_metric { >> const char *name; >> struct bpf_perf_event_value val; >> + __u64 scaled_val; >> struct perf_event_attr attr; >> bool selected; >> >> /* calculate ratios like instructions per cycle */ >> - const int ratio_metric; /* 0 for N/A, 1 for index 0 (cycles) */ >> + const enum ratio_metric ratio_metric; >> const char *ratio_desc; >> const float ratio_mul; >> } metrics[] = { >> - { >> + [METRIC_CYCLES] = { >> .name = "cycles", >> .attr = { >> .type = PERF_TYPE_HARDWARE, >> .config = PERF_COUNT_HW_CPU_CYCLES, >> .exclude_user = 1, >> }, >> + .ratio_metric = METRIC_RUN_CNT, >> + .ratio_desc = "cycles per run", >> + .ratio_mul = 1.0, >> }, >> - { >> + [METRIC_INSTRUCTIONS] = { >> .name = "instructions", >> .attr = { >> .type = PERF_TYPE_HARDWARE, >> .config = PERF_COUNT_HW_INSTRUCTIONS, >> .exclude_user = 1, >> }, >> - .ratio_metric = 1, >> + .ratio_metric = METRIC_CYCLES, >> .ratio_desc = "insns per cycle", >> .ratio_mul = 1.0, >> }, >> - { >> + [METRIC_L1D_LOADS] = { >> .name = "l1d_loads", >> .attr = { >> .type = PERF_TYPE_HW_CACHE, >> @@ -2102,8 +2117,9 @@ struct profile_metric { >> (PERF_COUNT_HW_CACHE_RESULT_ACCESS << 16), >> .exclude_user = 1, >> }, >> + .ratio_metric = METRIC_NONE, >> }, >> - { >> + [METRIC_LLC_MISSES] = { >> .name = "llc_misses", >> .attr = { >> .type = PERF_TYPE_HW_CACHE, >> @@ -2113,11 +2129,11 @@ struct profile_metric { >> (PERF_COUNT_HW_CACHE_RESULT_MISS << 16), >> .exclude_user = 1 >> }, >> - .ratio_metric = 2, >> + .ratio_metric = METRIC_INSTRUCTIONS, >> .ratio_desc = "LLC misses per million insns", >> .ratio_mul = 1e6, >> }, >> - { >> + [METRIC_ITLB_MISSES] = { >> .name = "itlb_misses", >> .attr = { >> .type = PERF_TYPE_HW_CACHE, >> @@ -2127,11 +2143,11 @@ struct profile_metric { >> (PERF_COUNT_HW_CACHE_RESULT_MISS << 16), >> .exclude_user = 1 >> }, >> - .ratio_metric = 2, >> + .ratio_metric = METRIC_INSTRUCTIONS, >> .ratio_desc = "itlb misses per million insns", >> .ratio_mul = 1e6, >> }, >> - { >> + [METRIC_DTLB_MISSES] = { >> .name = "dtlb_misses", >> .attr = { >> .type = PERF_TYPE_HW_CACHE, >> @@ -2141,13 +2157,14 @@ struct profile_metric { >> (PERF_COUNT_HW_CACHE_RESULT_MISS << 16), >> .exclude_user = 1 >> }, >> - .ratio_metric = 2, >> + .ratio_metric = METRIC_INSTRUCTIONS, >> .ratio_desc = "dtlb misses per million insns", >> .ratio_mul = 1e6, >> }, >> }; >> >> static __u64 profile_total_count; >> +static __u64 profile_valid_count; >> >> #define MAX_NUM_PROFILE_METRICS 4 >> >> @@ -2182,9 +2199,39 @@ static int profile_parse_metrics(int argc, char **argv) >> return selected_cnt; >> } >> >> -static void profile_read_values(struct profiler_bpf *obj) >> +/* >> + * Filter out CPUs that have any selected metric not scheduled for them. This makes sure all >> + * metrics are using the same CPU set, as a result ratio metrics are consistent. >> + */ >> +static void profile_filter_cpus(__u32 num_cpu, >> + struct bpf_perf_event_value vals[MAX_NUM_PROFILE_METRICS][num_cpu], >> + __u64 *counts) >> +{ >> + __u32 m, cpu, key = 0; >> + >> + for (m = 0; m < ARRAY_SIZE(metrics); m++) { >> + if (!metrics[m].selected) >> + continue; >> + >> + for (cpu = 0; cpu < num_cpu; cpu++) { >> + /* >> + * CPU has hits, but this metric never scheduled, set counts[cpu] to 0 >> + * so other metrics ignore this CPU too >> + */ >> + if (counts[cpu] && !vals[key][cpu].running) { >> + p_info("%s not scheduled on CPU %u; excluding %llu runs from all metrics", >> + metrics[m].name, cpu, counts[cpu]); >> + counts[cpu] = 0; >> + } >> + } >> + key++; >> + } >> +} > > was this suggested by AI or we actually ran into issues due to this in > practice? this looks perculiar > Suggested by AI is a bit of a simplification: The initial problem was scaling counters, the counters are collected and accounted per CPU, so it sounds like a right thing to scale per CPU as well(?) Because we divide by running, we need to check it for 0. Now it is possible that some counters have running 0 on some CPU, but others dont, will it make ratios biased? this is easy to repro: in tab 1 run: ``` sudo ./build/tools/bpf/bpftool/bpftool prog profile name myprog duration 80 itlb_misses dtlb_misses cycles instructions ``` in tab 2 run: ``` sudo ./build/tools/bpf/bpftool/bpftool prog profile name myprog duration 1 itlb_misses dtlb_misses cycles instructions cycles not scheduled on CPU 36; excluding 1 runs from all metrics instructions not scheduled on CPU 6; excluding 4 runs from all metrics instructions not scheduled on CPU 39; excluding 1 runs from all metrics instructions not scheduled on CPU 72; excluding 1 runs from all metrics itlb_misses not scheduled on CPU 50; excluding 1 runs from all metrics 330 run_cnt 3601259 cycles # 11184.03 cycles per run (73.57%) 682448 instructions # 0.19 insns per cycle (80.85%) 132 itlb_misses # 193.42 itlb misses per million insns (73.66%) 2189 dtlb_misses # 3207.57 dtlb misses per million insns (72.24%) ``` >> + >> +static int profile_read_values(struct profiler_bpf *obj) >> { >> __u32 m, cpu, num_cpu = obj->rodata->num_cpu; >> + struct bpf_perf_event_value values[MAX_NUM_PROFILE_METRICS][num_cpu], *val; >> int reading_map_fd, count_map_fd; >> __u64 counts[num_cpu]; >> __u32 key = 0; >> @@ -2194,38 +2241,61 @@ static void profile_read_values(struct profiler_bpf *obj) >> count_map_fd = bpf_map__fd(obj->maps.counts); >> if (reading_map_fd < 0 || count_map_fd < 0) { > > this can't happen if skeleton loaded successfully, just remove this > check instead of weird min() over fds/errors > >> p_err("failed to get fd for map"); >> - return; >> + return min(reading_map_fd, count_map_fd); >> } >> >> err = bpf_map_lookup_elem(count_map_fd, &key, counts); >> if (err) { >> p_err("failed to read count_map: %s", strerror(errno)); >> - return; >> + return err; >> + } >> + >> + for (m = 0; m < ARRAY_SIZE(metrics); m++) { >> + if (!metrics[m].selected) >> + continue; >> + >> + err = bpf_map_lookup_elem(reading_map_fd, &key, values[key]); >> + if (err) { >> + p_err("failed to read reading_map: %s", strerror(errno)); >> + return err; >> + } >> + key++; >> } >> >> profile_total_count = 0; >> for (cpu = 0; cpu < num_cpu; cpu++) >> profile_total_count += counts[cpu]; >> >> + profile_filter_cpus(num_cpu, values, counts); >> + >> + profile_valid_count = 0; >> + for (cpu = 0; cpu < num_cpu; cpu++) >> + profile_valid_count += counts[cpu]; >> + >> + key = 0; >> for (m = 0; m < ARRAY_SIZE(metrics); m++) { >> - struct bpf_perf_event_value values[num_cpu]; >> + double scale; >> >> if (!metrics[m].selected) >> continue; >> >> - err = bpf_map_lookup_elem(reading_map_fd, &key, values); >> - if (err) { >> - p_err("failed to read reading_map: %s", >> - strerror(errno)); >> - return; >> - } >> for (cpu = 0; cpu < num_cpu; cpu++) { >> - metrics[m].val.counter += values[cpu].counter; >> - metrics[m].val.enabled += values[cpu].enabled; >> - metrics[m].val.running += values[cpu].running; >> + /* Skip CPUs with no runs or with an unscheduled metric. */ >> + if (!counts[cpu]) >> + continue; >> + >> + val = &values[key][cpu]; >> + >> + metrics[m].val.enabled += val->enabled; >> + metrics[m].val.running += val->running; >> + metrics[m].val.counter += val->counter; >> + /* Scale counter values to account for perf event multiplexing. */ >> + scale = (double)val->enabled / val->running; >> + metrics[m].scaled_val += val->counter * scale; >> } >> key++; >> } >> + return 0; >> } >> >> static void profile_print_readings_json(void) >> @@ -2239,9 +2309,11 @@ static void profile_print_readings_json(void) >> jsonw_start_object(json_wtr); >> jsonw_string_field(json_wtr, "metric", metrics[m].name); >> jsonw_lluint_field(json_wtr, "run_cnt", profile_total_count); >> + jsonw_lluint_field(json_wtr, "run_cnt_valid", profile_valid_count); > > aren't we just overcomplicating things for no good reason?.. > >> jsonw_lluint_field(json_wtr, "value", metrics[m].val.counter); >> jsonw_lluint_field(json_wtr, "enabled", metrics[m].val.enabled); >> jsonw_lluint_field(json_wtr, "running", metrics[m].val.running); >> + jsonw_lluint_field(json_wtr, "value_scaled", metrics[m].scaled_val); >> >> jsonw_end_object(json_wtr); >> } >> @@ -2250,24 +2322,34 @@ static void profile_print_readings_json(void) >> >> static void profile_print_readings_plain(void) >> { >> - __u32 m; >> + __u32 i; >> >> printf("\n%18llu %-20s\n", profile_total_count, "run_cnt"); >> - for (m = 0; m < ARRAY_SIZE(metrics); m++) { >> - struct bpf_perf_event_value *val = &metrics[m].val; >> + for (i = 0; i < ARRAY_SIZE(metrics); i++) { >> + struct profile_metric *m = &metrics[i]; >> + struct bpf_perf_event_value *val = &m->val; >> int r; >> + __u64 ratio; >> >> - if (!metrics[m].selected) >> + if (!m->selected) >> continue; >> - printf("%18llu %-20s", val->counter, metrics[m].name); >> + printf("%18llu %-20s", m->scaled_val, m->name); >> >> - r = metrics[m].ratio_metric - 1; >> - if (r >= 0 && metrics[r].selected && >> - metrics[r].val.counter > 0) { >> + r = m->ratio_metric; >> + switch (r) { >> + case METRIC_RUN_CNT: >> + ratio = profile_valid_count; >> + break; >> + case METRIC_NONE: >> + ratio = 0; >> + break; >> + default: >> + ratio = metrics[r].scaled_val; >> + } >> + if (ratio) { >> printf("# %8.2f %-30s", >> - val->counter * metrics[m].ratio_mul / >> - metrics[r].val.counter, >> - metrics[m].ratio_desc); >> + m->scaled_val * m->ratio_mul / ratio, >> + m->ratio_desc); >> } else { >> printf("%-41s", ""); >> } >> @@ -2423,9 +2505,12 @@ static int profile_open_perf_events(struct profiler_bpf *obj) >> >> static void profile_print_and_cleanup(void) >> { >> + int err; >> + >> profile_close_perf_events(profile_obj); >> - profile_read_values(profile_obj); >> - profile_print_readings(); >> + err = profile_read_values(profile_obj); >> + if (!err) >> + profile_print_readings(); >> profiler_bpf__destroy(profile_obj); >> >> close(profile_tgt_fd); >> >> -- >> 2.53.0-Meta >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH bpf-next v3 0/3] bpftool: Improve perf counter reporting 2026-09-08 14:27 [PATCH bpf-next v3 0/3] bpftool: Improve perf counter reporting Mykyta Yatsenko ` (2 preceding siblings ...) 2026-09-08 14:27 ` [PATCH bpf-next v3 3/3] bpftool: Scale counters and report cycles per run Mykyta Yatsenko @ 2026-09-08 18:01 ` Ihor Solodrai 3 siblings, 0 replies; 16+ messages in thread From: Ihor Solodrai @ 2026-09-08 18:01 UTC (permalink / raw) To: Mykyta Yatsenko, bpf, ast, andrii, daniel, kernel-team, eddyz87, memxor, qmo, linux-perf-users, acme, namhyung Cc: Mykyta Yatsenko On 9/8/26 7:27 AM, Mykyta Yatsenko wrote: > Track valid snapshots that have a zero counter value. Scale each per-CPU > counter before aggregation. Report cycles per included program run. > > Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> > --- > Changes in v3: > - Keep the JSON value field raw. Add value_scaled and run_cnt_valid. > - Use the same CPU set for all metrics. > - Move snapshot state tracking to a separate patch. > - Link to v2: https://patch.msgid.link/20260902-bpftool_cyles_per_run-v2-1-bc5d14ad0ce9@meta.com > > Changes in v2: > - Scale all counters. > - Use an enum for metric indexes. > - Link to v1: https://patch.msgid.link/20260827-bpftool_cyles_per_run-v1-1-77d7bfc3c065@meta.com > > --- > Mykyta Yatsenko (3): > bpftool: Track perf counter snapshot state > perf bpf_counter: Track valid BPF counter snapshots > bpftool: Scale counters and report cycles per run For the series: Acked-by: Ihor Solodrai <ihor.solodrai@linux.dev> > > tools/bpf/bpftool/Documentation/bpftool-prog.rst | 13 +- > tools/bpf/bpftool/prog.c | 159 +++++++++++++++++------ > tools/bpf/bpftool/skeleton/profiler.bpf.c | 25 ++-- > tools/perf/util/bpf_skel/bpf_prog_profiler.bpf.c | 26 ++-- > 4 files changed, 168 insertions(+), 55 deletions(-) > --- > base-commit: 3fa3128887a4df6057c543aae7c5c7fd2ed41f12 > change-id: 20260827-bpftool_cyles_per_run-93169fcb30b7 > > Best regards, > -- > Mykyta Yatsenko <yatsenko@meta.com> > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2026-09-09 9:49 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-09-08 14:27 [PATCH bpf-next v3 0/3] bpftool: Improve perf counter reporting Mykyta Yatsenko 2026-09-08 14:27 ` [PATCH bpf-next v3 1/3] bpftool: Track perf counter snapshot state Mykyta Yatsenko 2026-09-08 14:35 ` sashiko-bot 2026-09-08 22:08 ` Quentin Monnet 2026-09-08 23:56 ` Andrii Nakryiko 2026-09-09 9:19 ` Mykyta Yatsenko 2026-09-08 14:27 ` [PATCH bpf-next v3 2/3] perf bpf_counter: Track valid BPF counter snapshots Mykyta Yatsenko 2026-09-08 14:39 ` sashiko-bot 2026-09-08 14:27 ` [PATCH bpf-next v3 3/3] bpftool: Scale counters and report cycles per run Mykyta Yatsenko 2026-09-08 14:42 ` sashiko-bot 2026-09-08 16:21 ` bot+bpf-ci 2026-09-08 17:37 ` Mykyta Yatsenko 2026-09-08 22:08 ` Quentin Monnet 2026-09-09 0:02 ` Andrii Nakryiko 2026-09-09 9:49 ` Mykyta Yatsenko 2026-09-08 18:01 ` [PATCH bpf-next v3 0/3] bpftool: Improve perf counter reporting Ihor Solodrai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox