Re: [PATCH] perf trace: Avoid early exit due SIGCHLD from non-workload processes

[Jiri Olsa]

On Tue, Feb 08, 2022 at 10:07:25PM +0800, Changbin Du wrote:
> The function trace__symbols_init() runs "perf-read-vdso32" and that ends up
> with a SIGCHLD delivered to 'perf'. And this SIGCHLD make perf exit early.
> 
> 'perf trace' should exit only if the SIGCHLD is from our workload process.
> So let's use sigaction() instead of signal() to match such condition.
> 
> Signed-off-by: Changbin Du <changbin.du@xxxxxxxxx>

good idea, I tested with the reproducer for:
  f06a82f9d31a perf trace: Avoid early exit due to running SIGCHLD handler before it makes sense to

and it's working properly

Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>

thanks,
jirka

> ---
>  tools/perf/builtin-trace.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 32844d8a0ea5..d03556c14b0a 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -1536,13 +1536,20 @@ static size_t trace__fprintf_tstamp(struct trace *trace, u64 tstamp, FILE *fp)
>  	return fprintf(fp, "         ? ");
>  }
>  
> +static pid_t workload_pid = -1;
>  static bool done = false;
>  static bool interrupted = false;
>  
> -static void sig_handler(int sig)
> +static void sighandler_interrupt(int sig __maybe_unused)
>  {
> -	done = true;
> -	interrupted = sig == SIGINT;
> +	done = interrupted = true;
> +}
> +
> +static void sighandler_chld(int sig __maybe_unused, siginfo_t *info,
> +			    void *context __maybe_unused)
> +{
> +	if (info->si_pid == workload_pid)
> +		done = true;
>  }
>  
>  static size_t trace__fprintf_comm_tid(struct trace *trace, struct thread *thread, FILE *fp)
> @@ -3938,7 +3945,6 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
>  	bool draining = false;
>  
>  	trace->live = true;
> -	signal(SIGCHLD, sig_handler);
>  
>  	if (!trace->raw_augmented_syscalls) {
>  		if (trace->trace_syscalls && trace__add_syscall_newtp(trace))
> @@ -4018,6 +4024,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
>  			fprintf(trace->output, "Couldn't run the workload!\n");
>  			goto out_delete_evlist;
>  		}
> +		workload_pid = evlist->workload.pid;
>  	}
>  
>  	err = evlist__open(evlist);
> @@ -4887,10 +4894,14 @@ int cmd_trace(int argc, const char **argv)
>  	const char * const trace_subcommands[] = { "record", NULL };
>  	int err = -1;
>  	char bf[BUFSIZ];
> +	struct sigaction sigchld_act = { 0 };
>  
>  	signal(SIGSEGV, sighandler_dump_stack);
>  	signal(SIGFPE, sighandler_dump_stack);
> -	signal(SIGINT, sig_handler);
> +	signal(SIGINT, sighandler_interrupt);
> +	sigchld_act.sa_flags = SA_SIGINFO;
> +	sigchld_act.sa_sigaction = sighandler_chld;
> +	sigaction(SIGCHLD, &sigchld_act, NULL);
>  
>  	trace.evlist = evlist__new();
>  	trace.sctbl = syscalltbl__new();
> -- 
> 2.25.1
>