Re: [PATCH bpf-next v3 3/3] bpftool: Scale counters and report cycles per run

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

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