Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 9 Feb 2022 16:01:33 +0100
From:   Jiri Olsa <olsajiri@gmail.com>
To:     Changbin Du <changbin.du@gmail.com>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] perf trace: Avoid early exit due SIGCHLD from
 non-workload processes
Message-ID: <YgPXTe7LlTfh+z4S@krava>
References: <20220208140725.3947-1-changbin.du@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220208140725.3947-1-changbin.du@gmail.com>
Precedence: bulk

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@gmail.com>

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@kernel.org>

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
>