Re: [PATCH bpf-next v3 1/3] bpftool: Track perf counter snapshot state

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

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
>>