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

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] Calling non-async-signal-safe POSIX functions in a signal handler creates a self-deadlock risk.
- [Medium] Missing state predicate for condition variable causes lost wakeups, defeating the patch's purpose.
- [Medium] Early benchmark completion passes negative sample counts to reporting functions, yielding NaN outputs.
--

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

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

The bench framework waits for duration_sec to elapse before collecting
results. Benchmarks that know exactly how many samples they need can
call bench_force_done() to signal completion early, avoiding wasted
wall-clock time.

> 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);
> +	pthread_cond_signal(&bench_done);
> +	pthread_mutex_unlock(&bench_done_mtx);
> +}
> +

Since bench_force_done() is now exported for concurrent benchmark worker
threads to call directly, could this lead to lost wakeups?

Condition variables are stateless. If a fast-running benchmark finishes and
signals the condition variable before main() reaches its wait statement:

tools/testing/selftests/bpf/bench.c:main() {
    ...
    pthread_cond_wait(&bench_done, &bench_done_mtx);
    ...
}

Will the signal be permanently lost, causing main() to sleep for the full
duration and defeating the goal of the patch?

Also, if a benchmark legitimately completes its target operations and calls
bench_force_done() before the warmup period is fully satisfied:

tools/testing/selftests/bpf/bench.c:main() {
    ...
    if (bench->report_final)
        bench->report_final(state.results + env.warmup_sec,
                            state.res_cnt - env.warmup_sec);
}

Could state.res_cnt - env.warmup_sec evaluate to a negative integer?

Passing a negative sample count to reporting functions might cause
reporter loops to fail bounds checks and yield nan outputs.

>  static void collect_measurements(long delta_ns) {
>  	int iter = state.res_cnt++;
>  	struct bench_res *res = &state.results[iter];
> @@ -750,11 +757,8 @@ static void collect_measurements(long delta_ns) {
>  	if (bench->report_progress)
>  		bench->report_progress(iter, res, delta_ns);
>  
> -	if (iter == env.duration_sec + env.warmup_sec) {
> -		pthread_mutex_lock(&bench_done_mtx);
> -		pthread_cond_signal(&bench_done);
> -		pthread_mutex_unlock(&bench_done_mtx);
> -	}
> +	if (iter == env.duration_sec + env.warmup_sec)
> +		bench_force_done();
>  }

collect_measurements() executes in the context of a timer signal handler.

Posix specifies that pthread_mutex_lock() and pthread_cond_signal() are
not async-signal-safe.

If the timer signal interrupts a thread exactly while it already holds
bench_done_mtx, will the signal handler attempt to acquire the same
non-recursive mutex and cause a self-deadlock?

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