* [PATCH] perf stat: Poll for monitored tasks being alive in fork mode
@ 2019-01-04 2:28 Jin Yao
2019-01-04 12:54 ` Jiri Olsa
0 siblings, 1 reply; 6+ messages in thread
From: Jin Yao @ 2019-01-04 2:28 UTC (permalink / raw)
To: acme, jolsa, peterz, mingo, alexander.shishkin
Cc: Linux-kernel, ak, kan.liang, yao.jin, Jin Yao
Following test shows the stat keeps running even if no longer
task to monitor (mgen exits at ~5s).
perf stat -e cycles -p `pgrep mgen` -I1000 -- sleep 10
time counts unit events
1.000148916 1,308,365,864 cycles
2.000379171 1,297,269,875 cycles
3.000556719 1,297,187,078 cycles
4.000914241 761,261,827 cycles
5.001306091 <not counted> cycles
6.001676881 <not counted> cycles
7.002046336 <not counted> cycles
8.002405651 <not counted> cycles
9.002766625 <not counted> cycles
10.001395827 <not counted> cycles
We'd better finish stat immediately if there's no longer task to
monitor.
After:
perf stat -e cycles -p `pgrep mgen` -I1000 -- sleep 10
time counts unit events
1.000180062 1,236,592,661 cycles
2.000421539 1,223,733,572 cycles
3.000609910 1,297,047,663 cycles
4.000807545 1,297,215,816 cycles
5.001001578 1,297,208,032 cycles
6.001390345 582,343,659 cycles
sleep: Terminated
Now the stat exits immediately when the monitored tasks ends.
Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
tools/perf/builtin-stat.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 63a3afc..71f3bc8 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -553,6 +553,13 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
if (interval || timeout) {
while (!waitpid(child_pid, &status, WNOHANG)) {
+ if (!is_target_alive(&target,
+ evsel_list->threads) &&
+ (child_pid != -1)) {
+ kill(child_pid, SIGTERM);
+ break;
+ }
+
nanosleep(&ts, NULL);
if (timeout)
break;
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] perf stat: Poll for monitored tasks being alive in fork mode 2019-01-04 2:28 [PATCH] perf stat: Poll for monitored tasks being alive in fork mode Jin Yao @ 2019-01-04 12:54 ` Jiri Olsa 2019-01-05 3:16 ` Jin, Yao 0 siblings, 1 reply; 6+ messages in thread From: Jiri Olsa @ 2019-01-04 12:54 UTC (permalink / raw) To: Jin Yao Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak, kan.liang, yao.jin On Fri, Jan 04, 2019 at 10:28:17AM +0800, Jin Yao wrote: > Following test shows the stat keeps running even if no longer > task to monitor (mgen exits at ~5s). > > perf stat -e cycles -p `pgrep mgen` -I1000 -- sleep 10 > time counts unit events > 1.000148916 1,308,365,864 cycles > 2.000379171 1,297,269,875 cycles > 3.000556719 1,297,187,078 cycles > 4.000914241 761,261,827 cycles > 5.001306091 <not counted> cycles > 6.001676881 <not counted> cycles > 7.002046336 <not counted> cycles > 8.002405651 <not counted> cycles > 9.002766625 <not counted> cycles > 10.001395827 <not counted> cycles > > We'd better finish stat immediately if there's no longer task to > monitor. > > After: > > perf stat -e cycles -p `pgrep mgen` -I1000 -- sleep 10 > time counts unit events > 1.000180062 1,236,592,661 cycles > 2.000421539 1,223,733,572 cycles > 3.000609910 1,297,047,663 cycles > 4.000807545 1,297,215,816 cycles > 5.001001578 1,297,208,032 cycles > 6.001390345 582,343,659 cycles > sleep: Terminated > > Now the stat exits immediately when the monitored tasks ends. > > Signed-off-by: Jin Yao <yao.jin@linux.intel.com> > --- > tools/perf/builtin-stat.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > index 63a3afc..71f3bc8 100644 > --- a/tools/perf/builtin-stat.c > +++ b/tools/perf/builtin-stat.c > @@ -553,6 +553,13 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) > > if (interval || timeout) { > while (!waitpid(child_pid, &status, WNOHANG)) { > + if (!is_target_alive(&target, > + evsel_list->threads) && > + (child_pid != -1)) { do we need that child_pid check? we just returned from waitpid so we should be ok.. we just make the race window smaller could we just do: if (!is_target_alive(&target, evsel_list->threads)) { kill(child_pid, SIGTERM); break; } also I'm not sure we should do this only under new option, as it might break people's scripts.. thoughts? jirka > + kill(child_pid, SIGTERM); > + break; > + } > + > nanosleep(&ts, NULL); > if (timeout) > break; > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf stat: Poll for monitored tasks being alive in fork mode 2019-01-04 12:54 ` Jiri Olsa @ 2019-01-05 3:16 ` Jin, Yao 2019-01-06 13:25 ` Jiri Olsa 0 siblings, 1 reply; 6+ messages in thread From: Jin, Yao @ 2019-01-05 3:16 UTC (permalink / raw) To: Jiri Olsa Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak, kan.liang, yao.jin On 1/4/2019 8:54 PM, Jiri Olsa wrote: > On Fri, Jan 04, 2019 at 10:28:17AM +0800, Jin Yao wrote: >> Following test shows the stat keeps running even if no longer >> task to monitor (mgen exits at ~5s). >> >> perf stat -e cycles -p `pgrep mgen` -I1000 -- sleep 10 >> time counts unit events >> 1.000148916 1,308,365,864 cycles >> 2.000379171 1,297,269,875 cycles >> 3.000556719 1,297,187,078 cycles >> 4.000914241 761,261,827 cycles >> 5.001306091 <not counted> cycles >> 6.001676881 <not counted> cycles >> 7.002046336 <not counted> cycles >> 8.002405651 <not counted> cycles >> 9.002766625 <not counted> cycles >> 10.001395827 <not counted> cycles >> >> We'd better finish stat immediately if there's no longer task to >> monitor. >> >> After: >> >> perf stat -e cycles -p `pgrep mgen` -I1000 -- sleep 10 >> time counts unit events >> 1.000180062 1,236,592,661 cycles >> 2.000421539 1,223,733,572 cycles >> 3.000609910 1,297,047,663 cycles >> 4.000807545 1,297,215,816 cycles >> 5.001001578 1,297,208,032 cycles >> 6.001390345 582,343,659 cycles >> sleep: Terminated >> >> Now the stat exits immediately when the monitored tasks ends. >> >> Signed-off-by: Jin Yao <yao.jin@linux.intel.com> >> --- >> tools/perf/builtin-stat.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c >> index 63a3afc..71f3bc8 100644 >> --- a/tools/perf/builtin-stat.c >> +++ b/tools/perf/builtin-stat.c >> @@ -553,6 +553,13 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) >> >> if (interval || timeout) { >> while (!waitpid(child_pid, &status, WNOHANG)) { >> + if (!is_target_alive(&target, >> + evsel_list->threads) && >> + (child_pid != -1)) { > > do we need that child_pid check? we just returned from waitpid > so we should be ok.. we just make the race window smaller > > could we just do: > > if (!is_target_alive(&target, evsel_list->threads)) { > kill(child_pid, SIGTERM); > break; > } > I think this code should be OK and I have tested yet. I have a question about the race condition, we really don't need a lock to protect the child_pid? skip_signal() { /* * render child_pid harmless * won't send SIGTERM to a random * process in case of race condition * and fast PID recycling */ child_pid = -1; } __run_perf_stat() { .... kill(child_pid, SIGTERM); } If child_pid is set by -1 in a small window between checking of child_pid and kill(), then kill(-1, SIGTERM) may happen. All processes except the kill process itself and init would receive SIGTERM. Is this case possible? > also I'm not sure we should do this only under new option, > as it might break people's scripts.. thoughts? > > jirka > In current behavior, for non fork mode, if we terminate the monitored task, the perf stat would return immediately. So I think this patch should be OK. Thanks Jin Yao >> + kill(child_pid, SIGTERM); >> + break; >> + } >> + >> nanosleep(&ts, NULL); >> if (timeout) >> break; >> -- >> 2.7.4 >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf stat: Poll for monitored tasks being alive in fork mode 2019-01-05 3:16 ` Jin, Yao @ 2019-01-06 13:25 ` Jiri Olsa 2019-01-06 14:02 ` Jin, Yao 0 siblings, 1 reply; 6+ messages in thread From: Jiri Olsa @ 2019-01-06 13:25 UTC (permalink / raw) To: Jin, Yao Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak, kan.liang, yao.jin On Sat, Jan 05, 2019 at 11:16:40AM +0800, Jin, Yao wrote: > > > On 1/4/2019 8:54 PM, Jiri Olsa wrote: > > On Fri, Jan 04, 2019 at 10:28:17AM +0800, Jin Yao wrote: > > > Following test shows the stat keeps running even if no longer > > > task to monitor (mgen exits at ~5s). > > > > > > perf stat -e cycles -p `pgrep mgen` -I1000 -- sleep 10 > > > time counts unit events > > > 1.000148916 1,308,365,864 cycles > > > 2.000379171 1,297,269,875 cycles > > > 3.000556719 1,297,187,078 cycles > > > 4.000914241 761,261,827 cycles > > > 5.001306091 <not counted> cycles > > > 6.001676881 <not counted> cycles > > > 7.002046336 <not counted> cycles > > > 8.002405651 <not counted> cycles > > > 9.002766625 <not counted> cycles > > > 10.001395827 <not counted> cycles > > > > > > We'd better finish stat immediately if there's no longer task to > > > monitor. > > > > > > After: > > > > > > perf stat -e cycles -p `pgrep mgen` -I1000 -- sleep 10 > > > time counts unit events > > > 1.000180062 1,236,592,661 cycles > > > 2.000421539 1,223,733,572 cycles > > > 3.000609910 1,297,047,663 cycles > > > 4.000807545 1,297,215,816 cycles > > > 5.001001578 1,297,208,032 cycles > > > 6.001390345 582,343,659 cycles > > > sleep: Terminated > > > > > > Now the stat exits immediately when the monitored tasks ends. > > > > > > Signed-off-by: Jin Yao <yao.jin@linux.intel.com> > > > --- > > > tools/perf/builtin-stat.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > > > index 63a3afc..71f3bc8 100644 > > > --- a/tools/perf/builtin-stat.c > > > +++ b/tools/perf/builtin-stat.c > > > @@ -553,6 +553,13 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) > > > if (interval || timeout) { > > > while (!waitpid(child_pid, &status, WNOHANG)) { > > > + if (!is_target_alive(&target, > > > + evsel_list->threads) && > > > + (child_pid != -1)) { > > > > do we need that child_pid check? we just returned from waitpid > > so we should be ok.. we just make the race window smaller > > > > could we just do: > > > > if (!is_target_alive(&target, evsel_list->threads)) { > > kill(child_pid, SIGTERM); > > break; > > } > > > > I think this code should be OK and I have tested yet. I have a question > about the race condition, we really don't need a lock to protect the > child_pid? > > skip_signal() > { > /* > * render child_pid harmless > * won't send SIGTERM to a random > * process in case of race condition > * and fast PID recycling > */ > child_pid = -1; > } > > __run_perf_stat() > { > .... > kill(child_pid, SIGTERM); > } > > If child_pid is set by -1 in a small window between checking of child_pid > and kill(), then kill(-1, SIGTERM) may happen. All processes except the kill > process itself and init would receive SIGTERM. ah right, -1 is special.. however that can still happen also in the orginal patch.. how about we do something like below jirka --- diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c index acfd48db52dd..c322cb271180 100644 --- a/tools/perf/builtin-stat.c +++ b/tools/perf/builtin-stat.c @@ -583,6 +583,14 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) if (interval || timeout) { while (!waitpid(child_pid, &status, WNOHANG)) { + if (!is_target_alive(&target, evsel_list->threads)) { + int pid = child_pid; + + if (pid != -1) + kill(pid, SIGTERM); + break; + } + nanosleep(&ts, NULL); if (timeout) break; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] perf stat: Poll for monitored tasks being alive in fork mode 2019-01-06 13:25 ` Jiri Olsa @ 2019-01-06 14:02 ` Jin, Yao 2019-01-06 15:57 ` Jiri Olsa 0 siblings, 1 reply; 6+ messages in thread From: Jin, Yao @ 2019-01-06 14:02 UTC (permalink / raw) To: Jiri Olsa Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak, kan.liang, yao.jin On 1/6/2019 9:25 PM, Jiri Olsa wrote: > On Sat, Jan 05, 2019 at 11:16:40AM +0800, Jin, Yao wrote: >> >> >> On 1/4/2019 8:54 PM, Jiri Olsa wrote: >>> On Fri, Jan 04, 2019 at 10:28:17AM +0800, Jin Yao wrote: >>>> Following test shows the stat keeps running even if no longer >>>> task to monitor (mgen exits at ~5s). >>>> >>>> perf stat -e cycles -p `pgrep mgen` -I1000 -- sleep 10 >>>> time counts unit events >>>> 1.000148916 1,308,365,864 cycles >>>> 2.000379171 1,297,269,875 cycles >>>> 3.000556719 1,297,187,078 cycles >>>> 4.000914241 761,261,827 cycles >>>> 5.001306091 <not counted> cycles >>>> 6.001676881 <not counted> cycles >>>> 7.002046336 <not counted> cycles >>>> 8.002405651 <not counted> cycles >>>> 9.002766625 <not counted> cycles >>>> 10.001395827 <not counted> cycles >>>> >>>> We'd better finish stat immediately if there's no longer task to >>>> monitor. >>>> >>>> After: >>>> >>>> perf stat -e cycles -p `pgrep mgen` -I1000 -- sleep 10 >>>> time counts unit events >>>> 1.000180062 1,236,592,661 cycles >>>> 2.000421539 1,223,733,572 cycles >>>> 3.000609910 1,297,047,663 cycles >>>> 4.000807545 1,297,215,816 cycles >>>> 5.001001578 1,297,208,032 cycles >>>> 6.001390345 582,343,659 cycles >>>> sleep: Terminated >>>> >>>> Now the stat exits immediately when the monitored tasks ends. >>>> >>>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com> >>>> --- >>>> tools/perf/builtin-stat.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c >>>> index 63a3afc..71f3bc8 100644 >>>> --- a/tools/perf/builtin-stat.c >>>> +++ b/tools/perf/builtin-stat.c >>>> @@ -553,6 +553,13 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) >>>> if (interval || timeout) { >>>> while (!waitpid(child_pid, &status, WNOHANG)) { >>>> + if (!is_target_alive(&target, >>>> + evsel_list->threads) && >>>> + (child_pid != -1)) { >>> >>> do we need that child_pid check? we just returned from waitpid >>> so we should be ok.. we just make the race window smaller >>> >>> could we just do: >>> >>> if (!is_target_alive(&target, evsel_list->threads)) { >>> kill(child_pid, SIGTERM); >>> break; >>> } >>> >> >> I think this code should be OK and I have tested yet. I have a question >> about the race condition, we really don't need a lock to protect the >> child_pid? >> >> skip_signal() >> { >> /* >> * render child_pid harmless >> * won't send SIGTERM to a random >> * process in case of race condition >> * and fast PID recycling >> */ >> child_pid = -1; >> } >> >> __run_perf_stat() >> { >> .... >> kill(child_pid, SIGTERM); >> } >> >> If child_pid is set by -1 in a small window between checking of child_pid >> and kill(), then kill(-1, SIGTERM) may happen. All processes except the kill >> process itself and init would receive SIGTERM. > > ah right, -1 is special.. however that can still happen also > in the orginal patch.. how about we do something like below > > jirka > > > --- > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > index acfd48db52dd..c322cb271180 100644 > --- a/tools/perf/builtin-stat.c > +++ b/tools/perf/builtin-stat.c > @@ -583,6 +583,14 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) > > if (interval || timeout) { > while (!waitpid(child_pid, &status, WNOHANG)) { > + if (!is_target_alive(&target, evsel_list->threads)) { > + int pid = child_pid; > + > + if (pid != -1) > + kill(pid, SIGTERM); > + break; > + } > + > nanosleep(&ts, NULL); > if (timeout) > break; > Hi Jiri, I think your patch is good. At least, we can avoid the case of kill(-1, SIGTERM). BTW, you post this patch or I re-post it, both fine for me. :) Thanks Jin Yao ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf stat: Poll for monitored tasks being alive in fork mode 2019-01-06 14:02 ` Jin, Yao @ 2019-01-06 15:57 ` Jiri Olsa 0 siblings, 0 replies; 6+ messages in thread From: Jiri Olsa @ 2019-01-06 15:57 UTC (permalink / raw) To: Jin, Yao Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak, kan.liang, yao.jin On Sun, Jan 06, 2019 at 10:02:18PM +0800, Jin, Yao wrote: SNIP > > --- > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > > index acfd48db52dd..c322cb271180 100644 > > --- a/tools/perf/builtin-stat.c > > +++ b/tools/perf/builtin-stat.c > > @@ -583,6 +583,14 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx) > > if (interval || timeout) { > > while (!waitpid(child_pid, &status, WNOHANG)) { > > + if (!is_target_alive(&target, evsel_list->threads)) { > > + int pid = child_pid; > > + > > + if (pid != -1) > > + kill(pid, SIGTERM); > > + break; > > + } > > + > > nanosleep(&ts, NULL); > > if (timeout) > > break; > > > > Hi Jiri, > > I think your patch is good. At least, we can avoid the case of kill(-1, > SIGTERM). > > BTW, you post this patch or I re-post it, both fine for me. :) please post it thanks, jirka ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-06 15:57 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-01-04 2:28 [PATCH] perf stat: Poll for monitored tasks being alive in fork mode Jin Yao 2019-01-04 12:54 ` Jiri Olsa 2019-01-05 3:16 ` Jin, Yao 2019-01-06 13:25 ` Jiri Olsa 2019-01-06 14:02 ` Jin, Yao 2019-01-06 15:57 ` Jiri Olsa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox