* [PATCH] perf trace: Avoid early exit due SIGCHLD from non-workload processes
@ 2022-02-08 14:07 Changbin Du
2022-02-09 15:01 ` Jiri Olsa
0 siblings, 1 reply; 5+ messages in thread
From: Changbin Du @ 2022-02-08 14:07 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar
Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
linux-perf-users, linux-kernel, Changbin Du
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>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] perf trace: Avoid early exit due SIGCHLD from non-workload processes
2022-02-08 14:07 [PATCH] perf trace: Avoid early exit due SIGCHLD from non-workload processes Changbin Du
@ 2022-02-09 15:01 ` Jiri Olsa
2022-02-09 15:50 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 5+ messages in thread
From: Jiri Olsa @ 2022-02-09 15:01 UTC (permalink / raw)
To: Changbin Du
Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
linux-perf-users, linux-kernel
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf trace: Avoid early exit due SIGCHLD from non-workload processes
2022-02-09 15:01 ` Jiri Olsa
@ 2022-02-09 15:50 ` Arnaldo Carvalho de Melo
2022-02-09 18:53 ` Jiri Olsa
0 siblings, 1 reply; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-02-09 15:50 UTC (permalink / raw)
To: Jiri Olsa
Cc: Changbin Du, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-perf-users,
linux-kernel
Em Wed, Feb 09, 2022 at 04:01:33PM +0100, Jiri Olsa escreveu:
> 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>
So can I replace this with the stronger:
Tested-by: Jiri Olsa <jolsa@kernel.org>
?
- Arnaldo
> 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
> >
--
- Arnaldo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf trace: Avoid early exit due SIGCHLD from non-workload processes
2022-02-09 15:50 ` Arnaldo Carvalho de Melo
@ 2022-02-09 18:53 ` Jiri Olsa
2022-02-09 19:08 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 5+ messages in thread
From: Jiri Olsa @ 2022-02-09 18:53 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Changbin Du, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-perf-users,
linux-kernel
On Wed, Feb 09, 2022 at 12:50:07PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Feb 09, 2022 at 04:01:33PM +0100, Jiri Olsa escreveu:
> > 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>
>
> So can I replace this with the stronger:
>
> Tested-by: Jiri Olsa <jolsa@kernel.org>
I did not see the case described in the changelog,
just checked the other one was fine..
jirka
>
> ?
>
> - Arnaldo
>
> > 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
> > >
>
> --
>
> - Arnaldo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] perf trace: Avoid early exit due SIGCHLD from non-workload processes
2022-02-09 18:53 ` Jiri Olsa
@ 2022-02-09 19:08 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-02-09 19:08 UTC (permalink / raw)
To: Jiri Olsa
Cc: Changbin Du, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-perf-users,
linux-kernel
On February 9, 2022 3:53:55 PM GMT-03:00, Jiri Olsa <olsajiri@gmail.com> wrote:
>On Wed, Feb 09, 2022 at 12:50:07PM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Wed, Feb 09, 2022 at 04:01:33PM +0100, Jiri Olsa escreveu:
>> > 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>
>>
>> So can I replace this with the stronger:
>>
>> Tested-by: Jiri Olsa <jolsa@kernel.org>
>
>I did not see the case described in the changelog,
>just checked the other one was fine..
Ok, I'll keep it as an acked-by then.
- Arnaldo
>
>jirka
>
>>
>> ?
>>
>> - Arnaldo
>>
>> > 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
>> > >
>>
>> --
>>
>> - Arnaldo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-02-09 19:10 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-08 14:07 [PATCH] perf trace: Avoid early exit due SIGCHLD from non-workload processes Changbin Du
2022-02-09 15:01 ` Jiri Olsa
2022-02-09 15:50 ` Arnaldo Carvalho de Melo
2022-02-09 18:53 ` Jiri Olsa
2022-02-09 19:08 ` Arnaldo Carvalho de Melo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.