Re: [PATCH v3 2/3] perf sched stats: Fix SIGCHLD vs pause() race in schedstat_live()

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Overwriting `done` with `0` defeats the intended fix for the lost-wakeup race, causing an indefinite hang.
- [High] The blocking `waitpid()` ignores `SIGTERM` and `SIGINT` because of `SA_RESTART` semantics, causing the tool to hang unresponsively.
--

commit 5047a8fcd3c2b9c566153aee1d997f695b5b0543
Author: Swapnil Sapkal <swapnil.sapkal@amd.com>

perf sched stats: Fix SIGCHLD vs pause() race in schedstat_live()

This commit addresses a lost-wakeup race in perf_sched__schedstat_live()
by replacing pause() with waitpid() for workloads, and using a while (!done)
sleep loop for system-wide mode.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index cfd93bf11c2ee..36fbc20dbebd8 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -4695,11 +4695,15 @@ static int perf_sched__schedstat_live(struct perf_sched *sched,
>  	if (err < 0)
>  		goto out;
>  
> -	if (argc)
> -		evlist__start_workload(evlist);
> +	done = 0;

Could setting done = 0 here inadvertently overwrite the state if a signal
was received during the setup phase?

If a termination signal like SIGINT or SIGTERM is delivered during the
earlier perf_event__synthesize_schedstat() call, the signal handler
sets done = 1. This assignment unconditionally resets it back to 0, which
seems to bypass the intended fix and could cause the subsequent loop to
hang indefinitely.

>  
> -	/* wait for signal */
> -	pause();
> +	if (argc) {
> +		evlist__start_workload(evlist);
> +		waitpid(evlist->workload.pid, NULL, 0);

Could this blocking waitpid() cause the tool to become unresponsive to
SIGINT or SIGTERM?

Since the tool uses signal() for handler registration, glibc provides BSD
semantics which sets the SA_RESTART flag. If a signal arrives while
waitpid() is blocked, the signal handler executes and sets done = 1, but
the waitpid() syscall is automatically restarted by the kernel instead of
returning EINTR.

Because the code is blocked inside waitpid() and doesn't check the done
flag, the tool might ignore termination signals and hang until the workload
completes.

Would a non-blocking wait combined with sleep prevent this issue?
For example:

while (!done) {
        if (waitpid(evlist->workload.pid, NULL, WNOHANG) != 0)
                break;
        sleep(1);
}

> +	} else {
> +		while (!done)
> +			sleep(1);
> +	}
>  
>  	if (reset) {
>  		err = disable_sched_schedstat();

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