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

[sashiko-bot@kernel.org]

Sashiko AI review found 6 potential issue(s):
- [Medium] The BPF timing macros rely on undeclared kfuncs `bpf_get_cpu_time_counter()` and `bpf_cpu_time_counter_to_ns()` which do not exist in the kernel.
- [Medium] Integer division truncation in `compute_batch_iters` completely breaks calibration for fast operations (< 1 ns/op).
- [Medium] The `BENCH_BPF_LOOP` macro leaks state across BPF program executions by omitting the `reset` step on the final untimed iteration.
- [Medium] The `machine_readable` field in `struct bpf_bench_timing` is left uninitialized by the `BENCH_TIMING_INIT` macro.
- [Medium] The BPF program updates the per-CPU `timing_idx` array using a non-atomic read-modify-write pattern, causing TOCTOU races.
- [Medium] Userspace lacks memory barriers when toggling `timing_enabled` and clearing the BSS arrays, allowing store reordering on weakly-ordered architectures.
--

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

selftests/bpf: Add BPF batch-timing library

This commit adds a reusable timing library for BPF benchmarks to allow precise
per-operation measurements inside BPF programs.

> diff --git a/tools/testing/selftests/bpf/bench_bpf_timing.h b/tools/testing/selftests/bpf/bench_bpf_timing.h
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/bench_bpf_timing.h

[ ... ]

> +#define BENCH_TIMING_INIT(t, skel, iters) do {				\
> +	(t)->samples = (skel)->bss->timing_samples;			\
> +	(t)->idx = (skel)->bss->timing_idx;				\
> +	(t)->timing_enabled = &(skel)->bss->timing_enabled;		\
> +	(t)->batch_iters_bss = &(skel)->bss->batch_iters;		\
> +	(t)->batch_iters = (iters);					\
> +	(t)->target_samples = 200;					\
> +	(t)->nr_cpus = env.nr_cpus;					\
> +	(t)->warmup_ticks = 0;						\
> +	(t)->done = false;						\
> +} while (0)

Is there a reason machine_readable is omitted from this initialization? If
the struct is allocated on the stack by a benchmark driver, could this result
in unpredictable output formatting due to reading uninitialized memory?

> diff --git a/tools/testing/selftests/bpf/benchs/bench_bpf_timing.c b/tools/testing/selftests/bpf/benchs/bench_bpf_timing.c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/benchs/bench_bpf_timing.c

[ ... ]

> +static void reset_timing(struct bpf_bench_timing *t)
> +{
> +	*t->timing_enabled = 0;
> +	memset(t->samples, 0,
> +	       sizeof(__u64) * BENCH_NR_CPUS * BENCH_NR_SAMPLES);
> +	memset(t->idx, 0, sizeof(__u32) * BENCH_NR_CPUS);
> +}

Does this need CPU memory barriers to prevent hardware reordering? While
timing_enabled is volatile, weakly-ordered architectures like arm64 might
reorder the memory stores across the flag toggle. This could allow background
BPF executions to write samples while the arrays are being zeroed.

[ ... ]

> +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);

[ ... ]

> +	per_op_ns = elapsed / CALIBRATE_SEED_BATCH;
> +	t->batch_iters = compute_batch_iters(per_op_ns);

Will this truncation break calibration for operations taking less than 1 ns?
If the total elapsed time for 100 iterations is under 100 ns, per_op_ns
evaluates to 0.

This would force compute_batch_iters() to return CALIBRATE_MIN_BATCH (100)
instead of scaling up to reach the 10 ms target.

> diff --git a/tools/testing/selftests/bpf/progs/bench_bpf_timing.bpf.h b/tools/testing/selftests/bpf/progs/bench_bpf_timing.bpf.h
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/bench_bpf_timing.bpf.h

[ ... ]

> +extern __u64 bpf_get_cpu_time_counter(void) __weak __ksym;
> +extern __u64 bpf_cpu_time_counter_to_ns(__u64 counter) __weak __ksym;

Do these kfuncs exist in the kernel source? It does not appear they are
introduced in this patch series.

If they are missing, won't libbpf resolve them to 0, causing the verifier to
reject the program or causing executions to record 0 elapsed time?

[ ... ]

> +static __always_inline void bench_record_sample(__u64 elapsed_ns)
> +{
> +	__u32 cpu, idx;
> +
> +	if (!timing_enabled)
> +		return;
> +
> +	cpu = bpf_get_smp_processor_id() & BENCH_CPU_MASK;
> +	idx = timing_idx[cpu];
> +
> +	if (idx >= BENCH_NR_SAMPLES)
> +		return;
> +
> +	timing_samples[cpu][idx] = elapsed_ns;
> +	timing_idx[cpu] = idx + 1;
> +}

Can this read-modify-write pattern cause lost samples or out-of-bounds writes?
If machines have more than 256 logical CPUs, BENCH_CPU_MASK will alias
multiple CPUs to the same index.

Concurrent thread executions could race and overwrite the index, potentially
corrupting the timing statistics.

[ ... ]

> +/*
> + * @body:  expression to time; return value (int) stored in __bench_result.
> + * @reset: undo body's side-effects so each iteration starts identically.
> + *         May reference __bench_result.  Use ({}) for empty reset.
> + *
> + * Runs batch_iters timed iterations, then one untimed iteration whose
> + * return value the macro evaluates to (for validation).
> + */
> +#define BENCH_BPF_LOOP(body, reset) ({					\
> +	__u64 __bench_start = bpf_get_cpu_time_counter();		\
> +	int __bench_result;						\
> +									\
> +	bpf_repeat(batch_iters) {					\
> +		__bench_result = (body);				\
> +		reset;							\
> +	}								\
> +									\
> +	bench_record_sample(bpf_cpu_time_counter_to_ns(			\
> +		bpf_get_cpu_time_counter() - __bench_start));		\
> +									\
> +	__bench_result = (body);					\
> +	__bench_result;							\
> +})

Should the final untimed iteration also evaluate the reset expression?

Without calling reset here, any side effects from the last body execution
(like memory allocations or map modifications) will leak into the next
program invocation and could violate benchmark isolation.

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