[BUG] perf stat: hangs with -p and process completes

[BUG] perf stat: hangs with -p and process completes

[Stephane Eranian]

Hi,

I am running into a perf stat issue with the -p option which allows you
to attach to a running process. If that process happens to terminate
while under monitoring
perf hangs in there and never terminates. The proper behavior would be to stop.
I can see the issue in that the attached process is not a child, so
wait() would not work.

To reproduce:
  $ sleep 10 &
  $ perf stat -p $!

doing the same with perf record works, so there is a solution to this problem.

Re: [BUG] perf stat: hangs with -p and process completes

[Jiri Olsa]

On Fri, Oct 12, 2018 at 02:26:09PM -0700, Stephane Eranian wrote:
> Hi,
> 
> I am running into a perf stat issue with the -p option which allows you
> to attach to a running process. If that process happens to terminate
> while under monitoring
> perf hangs in there and never terminates. The proper behavior would be to stop.
> I can see the issue in that the attached process is not a child, so
> wait() would not work.
> 
> To reproduce:
>   $ sleep 10 &
>   $ perf stat -p $!
> 
> doing the same with perf record works, so there is a solution to this problem.

yea, we don't poll for the event state change in perf stat,
but we do that in perf record.. also because the perf poll
code in kernel is originaly meant for tracking the ring
buffer state

maybe we could return EPOLLIN for alive events without ring
buffer.. like below (totaly untested) and add polling for 
event state into perf stat 

cc-ing perf folks

jirka


---
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 5a97f34bc14c..b9ee7e7803bf 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4865,12 +4865,12 @@ static __poll_t perf_poll(struct file *file, poll_table *wait)
 {
 	struct perf_event *event = file->private_data;
 	struct ring_buffer *rb;
-	__poll_t events = EPOLLHUP;
+	__poll_t events = EPOLLIN;
 
 	poll_wait(file, &event->waitq, wait);
 
 	if (is_event_hup(event))
-		return events;
+		return EPOLLHUP;
 
 	/*
 	 * Pin the event->rb by taking event->mmap_mutex; otherwise

Re: [BUG] perf stat: hangs with -p and process completes

[Jiri Olsa]

On Tue, Oct 16, 2018 at 01:03:41PM +0200, Jiri Olsa wrote:
> On Fri, Oct 12, 2018 at 02:26:09PM -0700, Stephane Eranian wrote:
> > Hi,
> > 
> > I am running into a perf stat issue with the -p option which allows you
> > to attach to a running process. If that process happens to terminate
> > while under monitoring
> > perf hangs in there and never terminates. The proper behavior would be to stop.
> > I can see the issue in that the attached process is not a child, so
> > wait() would not work.
> > 
> > To reproduce:
> >   $ sleep 10 &
> >   $ perf stat -p $!
> > 
> > doing the same with perf record works, so there is a solution to this problem.
> 
> yea, we don't poll for the event state change in perf stat,
> but we do that in perf record.. also because the perf poll
> code in kernel is originaly meant for tracking the ring
> buffer state
> 
> maybe we could return EPOLLIN for alive events without ring
> buffer.. like below (totaly untested) and add polling for 
> event state into perf stat 
> 
> cc-ing perf folks
> 

on the second thought attached patch works as well
without kernel change

jirka


---
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index b86aba1c8028..d1028d7755bb 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -409,6 +409,28 @@ static struct perf_evsel *perf_evsel__reset_weak_group(struct perf_evsel *evsel)
 	return leader;
 }
 
+static bool is_target_alive(struct target *_target,
+			    struct thread_map *threads)
+{
+	struct stat st;
+	int i;
+
+	if (!target__has_task(_target))
+		return true;
+
+	for (i = 0; i < threads->nr; i++) {
+		char path[PATH_MAX];
+
+		scnprintf(path, PATH_MAX, "%s/%d", procfs__mountpoint(),
+			  threads->map[i].pid);
+
+		if (!stat(path, &st))
+			return true;
+	}
+
+	return false;
+}
+
 static int __run_perf_stat(int argc, const char **argv, int run_idx)
 {
 	int interval = stat_config.interval;
@@ -579,6 +601,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 		enable_counters();
 		while (!done) {
 			nanosleep(&ts, NULL);
+			if (!is_target_alive(&target, evsel_list->threads))
+				break;
 			if (timeout)
 				break;
 			if (interval) {

Re: [BUG] perf stat: hangs with -p and process completes

[Stephane Eranian]

Jiri,

Thanks for looking into this.
Yeah, I don't think you need a kernel patch to make this work.
You can poll in the app.
Let me try this out.


On Tue, Oct 16, 2018 at 9:25 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Oct 16, 2018 at 01:03:41PM +0200, Jiri Olsa wrote:
> > On Fri, Oct 12, 2018 at 02:26:09PM -0700, Stephane Eranian wrote:
> > > Hi,
> > >
> > > I am running into a perf stat issue with the -p option which allows you
> > > to attach to a running process. If that process happens to terminate
> > > while under monitoring
> > > perf hangs in there and never terminates. The proper behavior would be to stop.
> > > I can see the issue in that the attached process is not a child, so
> > > wait() would not work.
> > >
> > > To reproduce:
> > >   $ sleep 10 &
> > >   $ perf stat -p $!
> > >
> > > doing the same with perf record works, so there is a solution to this problem.
> >
> > yea, we don't poll for the event state change in perf stat,
> > but we do that in perf record.. also because the perf poll
> > code in kernel is originaly meant for tracking the ring
> > buffer state
> >
> > maybe we could return EPOLLIN for alive events without ring
> > buffer.. like below (totaly untested) and add polling for
> > event state into perf stat
> >
> > cc-ing perf folks
> >
>
> on the second thought attached patch works as well
> without kernel change
>
> jirka
>
>
> ---
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index b86aba1c8028..d1028d7755bb 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -409,6 +409,28 @@ static struct perf_evsel *perf_evsel__reset_weak_group(struct perf_evsel *evsel)
>         return leader;
>  }
>
> +static bool is_target_alive(struct target *_target,
> +                           struct thread_map *threads)
> +{
> +       struct stat st;
> +       int i;
> +
> +       if (!target__has_task(_target))
> +               return true;
> +
> +       for (i = 0; i < threads->nr; i++) {
> +               char path[PATH_MAX];
> +
> +               scnprintf(path, PATH_MAX, "%s/%d", procfs__mountpoint(),
> +                         threads->map[i].pid);
> +
> +               if (!stat(path, &st))
> +                       return true;
> +       }
> +
> +       return false;
> +}
> +
>  static int __run_perf_stat(int argc, const char **argv, int run_idx)
>  {
>         int interval = stat_config.interval;
> @@ -579,6 +601,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>                 enable_counters();
>                 while (!done) {
>                         nanosleep(&ts, NULL);
> +                       if (!is_target_alive(&target, evsel_list->threads))
> +                               break;
>                         if (timeout)
>                                 break;
>                         if (interval) {