Re: [PATCH bpf-next v2] bpftool: Scale multiplexed perf counters

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

On 9/2/26 4:20 PM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
>> index 90fa2a48cc26a..2280dc4492c06 100644
>> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
>> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
>> @@ -217,7 +217,9 @@ 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. When **cycles** is
>> +    selected, plain output also reports the average number of cycles per
>> +    program run.
> 
> [ ... ]
> 
>> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
>> index a9f730d407a92..ad6d33f838e3c 100644
>> --- a/tools/bpf/bpftool/prog.c
>> +++ b/tools/bpf/bpftool/prog.c
> 
> [ ... ]
> 
>> @@ -2185,6 +2200,7 @@ static int profile_parse_metrics(int argc, char **argv)
>>  static void profile_read_values(struct profiler_bpf *obj)
>>  {
>>  	__u32 m, cpu, num_cpu = obj->rodata->num_cpu;
>> +	struct bpf_perf_event_value *val;
>>  	int reading_map_fd, count_map_fd;
>>  	__u64 counts[num_cpu];
>>  	__u32 key = 0;
>> @@ -2209,6 +2225,7 @@ static void profile_read_values(struct profiler_bpf *obj)
>>
>>  	for (m = 0; m < ARRAY_SIZE(metrics); m++) {
>>  		struct bpf_perf_event_value values[num_cpu];
>> +		double scale;
>>
>>  		if (!metrics[m].selected)
>>  			continue;
>> @@ -2220,9 +2237,22 @@ static void profile_read_values(struct profiler_bpf *obj)
>>  			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;
>> +			val = &values[cpu];
>> +
>> +			metrics[m].val.enabled += val->enabled;
>> +			metrics[m].val.running += val->running;
>> +
>> +			/* This counter has never been scheduled */
>> +			if (!val->running) {
>> +				if (counts[cpu])
>> +					p_info("%s not scheduled on CPU %u; results may be inaccurate",
>> +					       metrics[m].name, cpu);
>> +				continue;
>> +			}
>> +
>> +			/* Scale counter values to account for perf event multiplexing. */
>> +			scale = (double)val->enabled / val->running;
>> +			metrics[m].val.counter += val->counter * scale;
>>  		}
>>  		key++;
>>  	}
> 
> Does this change the semantics of the JSON output? Looking at
> profile_print_readings_json() in prog.c:
> 
> profile_print_readings_json() {
>     ...
>     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);
>     ...
> }
> 
> Before this commit "value" was the raw counter sum and "enabled"/"running"
> let a consumer apply the correction value * enabled / running itself.
> 
> After this commit "value" is already scaled (multiplied by enabled/running)
> while "enabled" and "running" remain as raw sums. If an existing consumer
> of bpftool -j prog profile applies the documented formula - value * enabled
> / running - it would now double-scale the result.
> 
> There is no new field and no version marker letting a consumer distinguish
> between the two output formats. The changelog says only "Scale resulting
> values similar to how perf does it" without mentioning the JSON output,
> and the .rst hunk explicitly scopes its wording to plain output ("When
> **cycles** is selected, plain output also reports ...").
> 
> Should the JSON "value" field stay raw to keep the interface stable, or
> should the scaled/raw split be made explicit with a new "scaled_value"
> field?
> 
> The same asymmetry appears in plain output. profile_print_readings_plain()
> still prints the raw multiplex percentage next to a counter that has
> already been corrected for that percentage:
> 
> if (val->enabled > val->running)
>     printf("(%4.2f%%)", val->running * 100.0 / val->enabled);
> 
> which reads as if the correction still has to be applied by the reader.
> 

This is a good point. I don't think we should scale for plain output, but not json.
But otherwise we potentially break users.
> 
> ---
> 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/33641892894