Re: [PATCH v3 3/3] perf lock contention: Fix SIGCHLD vs pause() race in __cmd_contention()

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Blocking `waitpid()` without checking the `done` flag prevents clean teardown and ignores signals in workload mode.
- [Medium] The `done = 0` assignment introduces a race condition that causes early `SIGINT`/`SIGTERM` signals to be completely ignored.
--

commit 8ef94afdd78ecb3553bc6a801fe79f49d3d5680d
Author: Swapnil Sapkal <swapnil.sapkal@amd.com>

perf lock contention: Fix SIGCHLD vs pause() race in __cmd_contention()

This commit replaces the pause() call with waitpid() for workload mode
and a sleep loop for system-wide mode to fix a lost-wakeup race.

> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index e8962c985d34a..3c165d632941f 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
[ ... ]
> @@ -2124,11 +2128,16 @@ static int __cmd_contention(int argc, const char **argv)
>  
>  	if (use_bpf) {
>  		lock_contention_start();
> -		if (argc)
> -			evlist__start_workload(con.evlist);
>  
> -		/* wait for signal */
> -		pause();
> +		done = 0;

Is it possible for this assignment to overwrite a signal received earlier?

The signal handlers are registered before BPF loading, which can take a
noticeable amount of time:

__cmd_contention() {
    ...
    signal(SIGINT, sighandler);
    ...
    err = lock_contention_prepare(&con);
    ...
}

If a user sends SIGINT during BPF loading, the handler correctly sets
done = 1, but this assignment then erases that state. Since done is
statically zero-initialized, could this assignment be safely removed?

> +
> +		if (argc) {
> +			evlist__start_workload(con.evlist);
> +			waitpid(con.evlist->workload.pid, NULL, 0);

Will this blocking waitpid() hang if the process receives a signal?

Signal handlers registered with glibc's signal() typically have the
SA_RESTART flag set by default. If perf receives a signal like SIGTERM
during this waitpid(), the handler sets done = 1, but the kernel will
automatically restart the interrupted waitpid().

If the child process ignores the signal or the signal is only sent to perf,
waitpid() resumes without checking the done flag. This prevents the process
from gracefully stopping the trace and printing the results.

Should this use a polling loop with WNOHANG to allow checking the done flag?

> +		} else {
> +			while (!done)
> +				sleep(1);
> +		}
>  
>  		lock_contention_stop();
>  		lock_contention_read(&con);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260422050545.129448-1-swapnil.sapkal@amd.com?part=3