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

Puranjay Mohan
2026-04-28T09:15:14Z

On Tue, Apr 28, 2026 at 1:05 AM <bot+bpf-ci@kernel.org> wrote:
>
> > 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]

bench_force_done() is expected to be called from SIGALARM handlers
only, it is never called in Thread context.

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

t->warmup_ticks++;
if (t->warmup_ticks < env.warmup_sec)
return;
if (t->warmup_ticks == env.warmup_sec) {
*t->timing_enabled = 1;
return;
}
// ... only here can bench_force_done() is called

So bench_force_done() fires at earliest on tick warmup_sec + 1,
meaning state.res_cnt >= warmup_sec + 1 when it fires. The subtraction
is always positive

> 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