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

[bot+bpf-ci@kernel.org]

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