Re: [RFC PATCH bpf-next 1/6] selftests/bpf: Add bench_force_done() for early benchmark completion

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] bench_force_done() introduces lost wakeups and spurious wakeups because it signals a condition variable without a shared state predicate.
- [High] Early termination via bench_force_done() can pass a negative sample count to report_final(), leading to division by zero.
- [High] Potential deadlock and undefined behavior due to locking a non-async-signal-safe mutex inside a signal handler.
--

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

selftests/bpf: Add bench_force_done() for early benchmark completion

This commit introduces bench_force_done() to allow benchmarks to signal
early completion. It avoids waiting for the full duration_sec when a
benchmark has already collected the necessary samples.

> diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
> --- a/tools/testing/selftests/bpf/bench.c
> +++ b/tools/testing/selftests/bpf/bench.c

[ ... ]

> @@ -741,6 +741,13 @@ static void setup_benchmark(void)
>  static pthread_mutex_t bench_done_mtx = PTHREAD_MUTEX_INITIALIZER;
>  static pthread_cond_t bench_done = PTHREAD_COND_INITIALIZER;
>  
> +void bench_force_done(void)
> +{
> +	pthread_mutex_lock(&bench_done_mtx);

Is it safe to lock this mutex here considering collect_measurements() is
called from a SIGALRM handler?

If the signal interrupts a thread that is already holding bench_done_mtx
(like a worker thread executing bench_force_done()), the signal handler
will attempt to lock the same mutex and cause a self-deadlock.

Since pthread_mutex_lock() and pthread_cond_signal() are not
async-signal-safe, should an async-signal-safe mechanism like an atomic
flag with sem_post() be used instead?

> +	pthread_cond_signal(&bench_done);

Could this introduce lost wakeups or spurious wakeups because the condition
variable is signaled without a shared state predicate?

If a worker thread completes quickly and calls bench_force_done() before
main() reaches pthread_cond_wait(), the signal will be lost because POSIX
condition variables do not retain state. This would cause main() to block
for the full timer duration.

Also, if main() waits without a while loop checking a condition flag, could
a POSIX spurious wakeup arbitrarily terminate the benchmark prematurely?

> +	pthread_mutex_unlock(&bench_done_mtx);
> +}

What happens if bench_force_done() is called before the warmup period fully
elapses?

In main(), the final sample count passed to bench->report_final() is
calculated as:

state.res_cnt - env.warmup_sec

If state.res_cnt is strictly less than env.warmup_sec, the resulting
negative count will bypass the iteration loops in the report_final
callbacks (like ops_report_final()), leaving aggregate variables at zero.

Would subsequent calculations divide by zero and corrupt the benchmark
summary?

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