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

Hello,

On Wed, Apr 22, 2026 at 05:29:52AM +0000, [email protected] wrote:
> Sashiko AI review found 2 potential issue(s):
> - [High] Blocking waitpid() ignores the cancellation flag, causing a hang or premature exit.
> - [Medium] Unconditional `done = 0;` assignment discards termination signals received during initialization.
> --
> 
> commit 1c95f818d983ad8f7f5ed28e4a0036553b2d2758
> Author: Swapnil Sapkal <[email protected]>
> 
> perf sched stats: Fix SIGCHLD vs pause() race in schedstat_record()
> 
> This commit replaces pause() with waitpid() or a sleep() loop to fix an
> indefinite hang if SIGCHLD is delivered before the process enters pause().
> It also introduces a sig_atomic_t flag set by the signal handler.
> 
> > diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> > index 3f509cfdd58c4..cfd93bf11c2ee 100644
> > --- a/tools/perf/builtin-sched.c
> > +++ b/tools/perf/builtin-sched.c
> [ ... ]
> > @@ -3899,11 +3903,15 @@ static int perf_sched__schedstat_record(struct perf_sched *sched,
> >  	if (err < 0)
> >  		goto out;
> >  
> > -	if (argc)
> > -		evlist__start_workload(evlist);
> > +	done = 0;
> 
> Is it possible that this unconditional assignment discards termination signals
> received during initialization?
> 
> Since the signal handlers are registered at the beginning of the function,
> if a user sends SIGINT during the setup phase, the handler will set done to 1.
> This assignment would then erase the early cancellation request, requiring the
> user to send the signal again.

Right, as it's initialized to 0 you don't need to set it here.

> 
> >  
> > -	/* wait for signal */
> > -	pause();
> > +	if (argc) {
> > +		evlist__start_workload(evlist);
> > +		waitpid(evlist->workload.pid, NULL, 0);
> 
> Could this blocking waitpid() cause the tool to ignore the cancellation flag?
> 
> Because perf is compiled with _GNU_SOURCE, glibc's signal() uses the
> SA_RESTART flag by default. This causes waitpid() to automatically restart
> after the signal handler returns.
> 
> If the profiled workload ignores or delays handling SIGINT, waitpid() will
> continue to block indefinitely and ignore the done flag.
> 
> Conversely, if SA_RESTART were disabled, waitpid() would return -1 (EINTR).
> Since the return value is not checked, the program would exit prematurely
> and leave the workload orphaned in the background.
> 
> Would it be safer to use waitpid() with WNOHANG inside a loop that checks
> the done flag?

Right, something like this? (not tested)

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

Thanks,
Namhyung

> 
> > +	} else {
> > +		while (!done)
> > +			sleep(1);
> > +	}
> >  
> >  	if (reset) {
> >  		err = disable_sched_schedstat();
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1