Re: [PATCH bpf-next 2/7] selftests/bpf: Add BPF batch-timing library

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [Medium] Integer division truncation in `bpf_bench_calibrate()` breaks auto-calibration for very fast BPF programs.
- [Medium] Zero warmup time (`-w 0`) causes the benchmark to silently collect no samples.
- [Medium] BPF `can_loop` (`bpf_may_goto`) can prematurely terminate the timed loop, silently invalidating the userspace `ns/op` calculation.
- [Low] The BPF header includes a non-existent file `<bpf_may_goto.h>`.
--

commit ecbc1ddd2ab895fa706970f55f150bb87ba2790e
Author: Puranjay Mohan <puranjay@kernel.org>

selftests/bpf: Add BPF batch-timing library
    
This commit adds a reusable timing library for BPF benchmarks that need to
measure BPF program execution time. It provides a BPF macro to record elapsed
time and a userspace library to auto-calibrate batch iterations and compute
statistics.

> diff --git a/tools/testing/selftests/bpf/benchs/bench_bpf_timing.c b/tools/testing/selftests/bpf/benchs/bench_bpf_timing.c
> new file mode 100644
> index 000000000000..75a39da69655
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/benchs/bench_bpf_timing.c
[ ... ]
> +static int collect_samples(struct bpf_bench_timing *t,
> +			   double *out, int max_out)
> +{
> +	unsigned int nr_cpus = bpf_num_possible_cpus();
> +	__u32 timed_iters = t->batch_iters;
> +	int total = 0;
[ ... ]
> +		for (__u32 i = 0; i < count && total < max_out; i++) {
> +			__u64 sample = t->samples[cpu][i];
> +
> +			if (sample == 0)
> +				continue;
> +			out[total++] = (double)sample / timed_iters;

If the loop in BENCH_BPF_LOOP is terminated early by the can_loop condition
(e.g., due to instruction limits or yielding), the actual number of completed
iterations will be less than timed_iters.

Since the BPF program only records the elapsed time, will dividing by
timed_iters produce artificially low ns/op calculations?

Should the BPF program explicitly record and export the actual number of
completed iterations for this calculation?

> +		}
> +	}
> +
> +	qsort(out, total, sizeof(double), cmp_double);
> +	return total;
> +}
[ ... ]
> +void bpf_bench_timing_measure(struct bpf_bench_timing *t, struct bench_res *res)
> +{
> +	unsigned int nr_cpus;
> +	__u32 total_samples;
> +	int i;
> +
> +	t->warmup_ticks++;
> +
> +	if (t->warmup_ticks < env.warmup_sec)
> +		return;
> +
> +	if (t->warmup_ticks == env.warmup_sec) {
> +		*t->timing_enabled = 1;
> +		return;
> +	}

If the benchmark is run with zero warmup (env.warmup_sec == 0),
t->warmup_ticks becomes 1 on the first tick.

Does this mean t->warmup_ticks == env.warmup_sec will never be true, bypassing
the assignment and preventing timing_enabled from ever being set to 1?

[ ... ]
> +void bpf_bench_calibrate(struct bpf_bench_timing *t, bpf_bench_run_fn run_fn, void *run_ctx)
> +{
> +	__u64 elapsed, per_op_ns;
> +	__u64 time_n, time_2n;
> +	double ratio;
> +
> +	elapsed = measure_elapsed(t, run_fn, run_ctx, CALIBRATE_SEED_BATCH, CALIBRATE_RUNS);
> +	if (elapsed == 0) {
> +		fprintf(stderr, "calibration: no timing samples, using default\n");
> +		t->batch_iters = 10000;
> +		*t->batch_iters_bss = t->batch_iters;
> +		reset_timing(t);
> +		return;
> +	}
> +
> +	per_op_ns = elapsed / CALIBRATE_SEED_BATCH;

If the BPF operation executes in less than 1 ns (e.g., an empty loop or
bpf-nop), elapsed could be less than 100 since CALIBRATE_SEED_BATCH is 100.

Will the integer division truncate per_op_ns to 0 here, causing
compute_batch_iters() to incorrectly fall back to CALIBRATE_MIN_BATCH instead
of scaling up the iterations?

> +	t->batch_iters = compute_batch_iters(per_op_ns);
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/bench_bpf_timing.bpf.h b/tools/testing/selftests/bpf/progs/bench_bpf_timing.bpf.h
> new file mode 100644
> index 000000000000..6a1ad75f1fd7
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/bench_bpf_timing.bpf.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2026 Meta Platforms, Inc. and affiliates. */
> +
> +#ifndef __BENCH_BPF_TIMING_BPF_H__
> +#define __BENCH_BPF_TIMING_BPF_H__
> +
> +#include <stdbool.h>
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf_may_goto.h>

Does bpf_may_goto.h exist in the kernel source tree?

I noticed that can_loop is typically defined in bpf_experimental.h. Will
including this cause a No such file or directory build error?

> +
> +#ifndef BENCH_NR_SAMPLES

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260427232313.1582588-1-puranjay@kernel.org?part=2