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

bot+bpf-ci
2026-04-28T00:05:11Z

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

> diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
> index 029b3e21f438..47a4e72208d6 100644
> --- 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);
> +}
> +

Is it safe to use pthread_mutex_lock() and pthread_cond_signal() here
given that collect_measurements() calls bench_force_done() from a
SIGALRM signal handler (sigalarm_handler)?

If the signal interrupts a thread that is already holding bench_done_mtx
(for example, a worker thread executing bench_force_done()), the signal
handler will attempt to lock the same mutex:

Thread context:
bench_force_done()
pthread_mutex_lock(&bench_done_mtx); // lock acquired
[SIGALRM arrives]

Signal handler context:
sigalarm_handler()
collect_measurements()
bench_force_done()
pthread_mutex_lock(&bench_done_mtx); // deadlock

This causes a self-deadlock because pthread_mutex_lock() and
pthread_cond_signal() are not async-signal-safe according to POSIX.

Could an async-signal-safe mechanism like an atomic flag with sem_post()
be used instead?

This concern was raised in the v1 and v2 reviews by Sashiko AI on lore:
https://lore.kernel.org/bpf/20260420124120.13970C19425@smtp.kernel.org/

Could this code lose 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:

Worker thread:
bench_force_done()
pthread_cond_signal(&bench_done); // signal sent

Main thread (not waiting yet):
[still in setup]
pthread_cond_wait(&bench_done, &bench_done_mtx); // misses signal

This would cause main() to block for the full timer duration, defeating
the purpose of bench_force_done(). Should a boolean flag be added to
track the done state?

This concern was raised in the v1 and v2 reviews by Sashiko AI on lore:
https://lore.kernel.org/bpf/20260420124120.13970C19425@smtp.kernel.org/

> 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();
> }

[ ... ]

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?

This concern was raised in the v1 and v2 reviews by Sashiko AI on lore:
https://lore.kernel.org/bpf/20260420124120.13970C19425@smtp.kernel.org/

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