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

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

On 9/2/26 7:14 PM, Mykyta Yatsenko wrote:
> 
> 
> 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.
>>

Discussed with Andrii, we'll scale for stdout and keep existing behaviour for the json.

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