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

Puranjay Mohan
2026-04-28T09:23:45Z

On Tue, Apr 28, 2026 at 1:18 AM <bot+bpf-ci@kernel.org> wrote:
>
> > 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 __u32 compute_batch_iters(__u64 per_op_ns)
> > +{
> > + __u64 target_ns = (__u64)CALIBRATE_TARGET_MS * 1000000ULL;
> > + __u32 iters;
> > +
> > + if (per_op_ns == 0)
> > + return CALIBRATE_MIN_BATCH;
> > +
> > + iters = target_ns / per_op_ns;
> > +
> > + if (iters < CALIBRATE_MIN_BATCH)
> > + iters = CALIBRATE_MIN_BATCH;
> > + if (iters > CALIBRATE_MAX_BATCH)
> > + iters = CALIBRATE_MAX_BATCH;
> > +
> > + return iters;
> > +}
> > +
> > +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;
> > + t->batch_iters = compute_batch_iters(per_op_ns);
>
> Will this integer division 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, forcing compute_batch_iters() to return
> CALIBRATE_MIN_BATCH (100) instead of scaling up to reach the 10 ms target.

If individual operations take less than 1 ns then we can't meaninfully
measure them, so returning 100 here is good enough.

>
> > +
> > + time_n = measure_elapsed(t, run_fn, run_ctx, t->batch_iters, CALIBRATE_RUNS);
> > + time_2n = measure_elapsed(t, run_fn, run_ctx, t->batch_iters * 2, CALIBRATE_RUNS);
>
> [ ... ]
>
> > +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.

reset_timing() is only called during calibration, which runs in
setup() — before any producer threads are started. There's no
concurrent BPF execution to race with.

> [ ... ]
>
> > 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
>
> [ ... ]
>
> > +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.

xdp_lb_validate() checks bpf_num_possible_cpus() > BENCH_NR_CPUS and
exits with an error.

>
> > +
> > +/*
> > + * @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_ktime_get_ns(); \
> > + __u32 __bench_i; \
> > + int __bench_result; \
> > + \
> > + for (__bench_i = 0; \
> > + __bench_i < batch_iters && can_loop; \
> > + __bench_i++) { \
> > + __bench_result = (body); \
> > + reset; \
> > + } \
> > + \
> > + bench_record_sample(bpf_ktime_get_ns() - __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.

This is by design, we want to have the last untimed iteration to have
side effects so we can validate output.

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