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

bot+bpf-ci
2026-04-28T00:18:21Z

[-- Attachment #1: Type: text/plain, Size: 4624 bytes --]

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

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

[ ... ]

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

> +
> +/*
> + * @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.

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