All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] perf record: handle death by SIGTERM
@ 2013-05-06 18:24 David Ahern
  2013-05-06 18:45 ` David Ahern
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: David Ahern @ 2013-05-06 18:24 UTC (permalink / raw)
  To: acme, linux-kernel
  Cc: David Ahern, Mike Galbraith, Ingo Molnar, Frederic Weisbecker,
	Peter Zijlstra, Jiri Olsa, Namhyung Kim, Stephane Eranian

perf data files cannot be processed until the header file is update
which is done via an on_exit handler. If perf is killed due to a SIGTERM
it does not run the on_exit hooks leaving the perf.data file in a
random state which perf-report will happily spin on trying to read. As
noted by Mike an easy reproducer is:
  perf record -a -g & sleep 1; killall perf

Fix by catching SIGTERM like it does SIGINT.  Also need to remove the
kill which was added via commit f7b7c26e.

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
---
 tools/perf/builtin-record.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index cdf58ec..fff985c 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -198,7 +198,6 @@ static void perf_record__sig_exit(int exit_status __maybe_unused, void *arg)
 		return;
 
 	signal(signr, SIG_DFL);
-	kill(getpid(), signr);
 }
 
 static bool perf_evlist__equal(struct perf_evlist *evlist,
@@ -404,6 +403,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
 	signal(SIGCHLD, sig_handler);
 	signal(SIGINT, sig_handler);
 	signal(SIGUSR1, sig_handler);
+	signal(SIGTERM, sig_handler);
 
 	if (!output_name) {
 		if (!fstat(STDOUT_FILENO, &st) && S_ISFIFO(st.st_mode))
-- 
1.7.10.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] perf record: handle death by SIGTERM
  2013-05-06 18:24 [PATCH] perf record: handle death by SIGTERM David Ahern
@ 2013-05-06 18:45 ` David Ahern
  2013-05-06 22:40 ` Stephane Eranian
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: David Ahern @ 2013-05-06 18:45 UTC (permalink / raw)
  To: acme
  Cc: linux-kernel, Mike Galbraith, Ingo Molnar, Frederic Weisbecker,
	Peter Zijlstra, Jiri Olsa, Namhyung Kim, Stephane Eranian

On 5/6/13 12:24 PM, David Ahern wrote:
> perf data files cannot be processed until the header file is update

yuk, that is supposed to say "until the header is updated"

David

> which is done via an on_exit handler. If perf is killed due to a SIGTERM
> it does not run the on_exit hooks leaving the perf.data file in a
> random state which perf-report will happily spin on trying to read. As
> noted by Mike an easy reproducer is:
>    perf record -a -g & sleep 1; killall perf
>
> Fix by catching SIGTERM like it does SIGINT.  Also need to remove the
> kill which was added via commit f7b7c26e.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Stephane Eranian <eranian@google.com>
> ---
>   tools/perf/builtin-record.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index cdf58ec..fff985c 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -198,7 +198,6 @@ static void perf_record__sig_exit(int exit_status __maybe_unused, void *arg)
>   		return;
>
>   	signal(signr, SIG_DFL);
> -	kill(getpid(), signr);
>   }
>
>   static bool perf_evlist__equal(struct perf_evlist *evlist,
> @@ -404,6 +403,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
>   	signal(SIGCHLD, sig_handler);
>   	signal(SIGINT, sig_handler);
>   	signal(SIGUSR1, sig_handler);
> +	signal(SIGTERM, sig_handler);
>
>   	if (!output_name) {
>   		if (!fstat(STDOUT_FILENO, &st) && S_ISFIFO(st.st_mode))
>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] perf record: handle death by SIGTERM
  2013-05-06 18:24 [PATCH] perf record: handle death by SIGTERM David Ahern
  2013-05-06 18:45 ` David Ahern
@ 2013-05-06 22:40 ` Stephane Eranian
  2013-05-07  0:05   ` David Ahern
  2013-05-07  6:29   ` Ingo Molnar
  2013-05-24  9:08 ` Jiri Olsa
  2013-05-31 11:33 ` [tip:perf/core] " tip-bot for David Ahern
  3 siblings, 2 replies; 12+ messages in thread
From: Stephane Eranian @ 2013-05-06 22:40 UTC (permalink / raw)
  To: David Ahern
  Cc: Arnaldo Carvalho de Melo, LKML, Mike Galbraith, Ingo Molnar,
	Frederic Weisbecker, Peter Zijlstra, Jiri Olsa, Namhyung Kim

This is a good fix. I have run into this infinite loop in perf report
many times.
Thanks David.

On Mon, May 6, 2013 at 8:24 PM, David Ahern <dsahern@gmail.com> wrote:
> perf data files cannot be processed until the header file is update
> which is done via an on_exit handler. If perf is killed due to a SIGTERM
> it does not run the on_exit hooks leaving the perf.data file in a
> random state which perf-report will happily spin on trying to read. As
> noted by Mike an easy reproducer is:
>   perf record -a -g & sleep 1; killall perf
>
> Fix by catching SIGTERM like it does SIGINT.  Also need to remove the
> kill which was added via commit f7b7c26e.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Stephane Eranian <eranian@google.com>
> ---
>  tools/perf/builtin-record.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index cdf58ec..fff985c 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -198,7 +198,6 @@ static void perf_record__sig_exit(int exit_status __maybe_unused, void *arg)
>                 return;
>
>         signal(signr, SIG_DFL);
> -       kill(getpid(), signr);
>  }
>
>  static bool perf_evlist__equal(struct perf_evlist *evlist,
> @@ -404,6 +403,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
>         signal(SIGCHLD, sig_handler);
>         signal(SIGINT, sig_handler);
>         signal(SIGUSR1, sig_handler);
> +       signal(SIGTERM, sig_handler);
>
>         if (!output_name) {
>                 if (!fstat(STDOUT_FILENO, &st) && S_ISFIFO(st.st_mode))
> --
> 1.7.10.1
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] perf record: handle death by SIGTERM
  2013-05-06 22:40 ` Stephane Eranian
@ 2013-05-07  0:05   ` David Ahern
  2013-05-07  6:29   ` Ingo Molnar
  1 sibling, 0 replies; 12+ messages in thread
From: David Ahern @ 2013-05-07  0:05 UTC (permalink / raw)
  To: Stephane Eranian
  Cc: Arnaldo Carvalho de Melo, LKML, Mike Galbraith, Ingo Molnar,
	Frederic Weisbecker, Peter Zijlstra, Jiri Olsa, Namhyung Kim

On 5/6/13 4:40 PM, Stephane Eranian wrote:
> This is a good fix. I have run into this infinite loop in perf report
> many times.

The perf_file_header could use an 'I am sane' bit which is only set when 
the file is closed properly. Perhaps we could overload the magic field 
like this:

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 326068a..cd9fad6 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2364,7 +2364,6 @@ out_err_write:
     }

     f_header = (struct perf_file_header){
-       .magic     = PERF_MAGIC,
         .size      = sizeof(f_header),
         .attr_size = sizeof(f_attr),
         .attrs = {
@@ -2382,6 +2381,8 @@ out_err_write:
     };

     memcpy(&f_header.adds_features, &header->adds_features, 
sizeof(header->adds_features));
+   if (at_exit)
+       f_header.magic     = PERF_MAGIC,

     lseek(fd, 0, SEEK_SET);
     err = do_write(fd, &f_header, sizeof(f_header));

The perf magic is only written when the file is closed properly. In 
Mike's case you end up with the message (which can be enhanced)

   magic/endian check failed
   incompatible file format (rerun with -v to learn more)

which is better than an infinite loop.

David


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] perf record: handle death by SIGTERM
  2013-05-06 22:40 ` Stephane Eranian
  2013-05-07  0:05   ` David Ahern
@ 2013-05-07  6:29   ` Ingo Molnar
  2013-05-07 20:56     ` David Ahern
  1 sibling, 1 reply; 12+ messages in thread
From: Ingo Molnar @ 2013-05-07  6:29 UTC (permalink / raw)
  To: Stephane Eranian
  Cc: David Ahern, Arnaldo Carvalho de Melo, LKML, Mike Galbraith,
	Frederic Weisbecker, Peter Zijlstra, Jiri Olsa, Namhyung Kim


* Stephane Eranian <eranian@google.com> wrote:

> This is a good fix. I have run into this infinite loop in perf report 
> many times.

Hm, perf record should really not assume much about the perf.data and 
should avoid infinite loops ...

So while making perf.data more consistent on SIGTERM is a nice fix, perf 
report should be fixed as well to detect loops and such.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] perf record: handle death by SIGTERM
  2013-05-07  6:29   ` Ingo Molnar
@ 2013-05-07 20:56     ` David Ahern
  2013-05-08  6:17       ` Namhyung Kim
  2013-05-08  6:54       ` Ingo Molnar
  0 siblings, 2 replies; 12+ messages in thread
From: David Ahern @ 2013-05-07 20:56 UTC (permalink / raw)
  To: Ingo Molnar, Stephane Eranian
  Cc: Arnaldo Carvalho de Melo, LKML, Mike Galbraith,
	Frederic Weisbecker, Peter Zijlstra, Jiri Olsa, Namhyung Kim

On 5/7/13 12:29 AM, Ingo Molnar wrote:
>
> * Stephane Eranian <eranian@google.com> wrote:
>
>> This is a good fix. I have run into this infinite loop in perf report
>> many times.
>
> Hm, perf record should really not assume much about the perf.data and
> should avoid infinite loops ...
>
> So while making perf.data more consistent on SIGTERM is a nice fix, perf
> report should be fixed as well to detect loops and such.
>
> Thanks,
>
> 	Ingo
>

This seems to do the trick:

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 326068a..e82646f 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2802,6 +2802,17 @@ int perf_session__read_header(struct perf_session 
*session, int fd)
     if (perf_file_header__read(&f_header, header, fd) < 0)
         return -EINVAL;

+   /*
+    * sanity check that perf.data was written cleanly: data size
+    * is initialized to 0 and updated only if the on_exit function
+    * is run. If data size is still 0 then the file cannot be
+    * processed.
+    */
+   if (f_header.data.size == 0) {
+       pr_err("data size is 0. Was record properly terminated?\n");
+       return -1;
+   }
+
     nr_attrs = f_header.attrs.size / f_header.attr_size;
     lseek(fd, f_header.attrs.offset, SEEK_SET);


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] perf record: handle death by SIGTERM
  2013-05-07 20:56     ` David Ahern
@ 2013-05-08  6:17       ` Namhyung Kim
  2013-05-08  6:54       ` Ingo Molnar
  1 sibling, 0 replies; 12+ messages in thread
From: Namhyung Kim @ 2013-05-08  6:17 UTC (permalink / raw)
  To: David Ahern
  Cc: Ingo Molnar, Stephane Eranian, Arnaldo Carvalho de Melo, LKML,
	Mike Galbraith, Frederic Weisbecker, Peter Zijlstra, Jiri Olsa

Hi David,

On Tue, 07 May 2013 14:56:17 -0600, David Ahern wrote:
> On 5/7/13 12:29 AM, Ingo Molnar wrote:
>>
>> * Stephane Eranian <eranian@google.com> wrote:
>>
>>> This is a good fix. I have run into this infinite loop in perf report
>>> many times.
>>
>> Hm, perf record should really not assume much about the perf.data and
>> should avoid infinite loops ...
>>
>> So while making perf.data more consistent on SIGTERM is a nice fix, perf
>> report should be fixed as well to detect loops and such.
>>
>> Thanks,
>>
>> 	Ingo
>>
>
> This seems to do the trick:

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

>
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 326068a..e82646f 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2802,6 +2802,17 @@ int perf_session__read_header(struct
> perf_session *session, int fd)
>     if (perf_file_header__read(&f_header, header, fd) < 0)
>         return -EINVAL;
>
> +   /*
> +    * sanity check that perf.data was written cleanly: data size
> +    * is initialized to 0 and updated only if the on_exit function
> +    * is run. If data size is still 0 then the file cannot be
> +    * processed.
> +    */
> +   if (f_header.data.size == 0) {
> +       pr_err("data size is 0. Was record properly terminated?\n");
> +       return -1;
> +   }
> +
>     nr_attrs = f_header.attrs.size / f_header.attr_size;
>     lseek(fd, f_header.attrs.offset, SEEK_SET);

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] perf record: handle death by SIGTERM
  2013-05-07 20:56     ` David Ahern
  2013-05-08  6:17       ` Namhyung Kim
@ 2013-05-08  6:54       ` Ingo Molnar
  2013-05-08 13:48         ` David Ahern
  1 sibling, 1 reply; 12+ messages in thread
From: Ingo Molnar @ 2013-05-08  6:54 UTC (permalink / raw)
  To: David Ahern
  Cc: Stephane Eranian, Arnaldo Carvalho de Melo, LKML, Mike Galbraith,
	Frederic Weisbecker, Peter Zijlstra, Jiri Olsa, Namhyung Kim


* David Ahern <dsahern@gmail.com> wrote:

> On 5/7/13 12:29 AM, Ingo Molnar wrote:
> >
> >* Stephane Eranian <eranian@google.com> wrote:
> >
> >>This is a good fix. I have run into this infinite loop in perf report
> >>many times.
> >
> >Hm, perf record should really not assume much about the perf.data and
> >should avoid infinite loops ...
> >
> >So while making perf.data more consistent on SIGTERM is a nice fix, perf
> >report should be fixed as well to detect loops and such.
> >
> >Thanks,
> >
> >	Ingo
> >
> 
> This seems to do the trick:
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 326068a..e82646f 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2802,6 +2802,17 @@ int perf_session__read_header(struct
> perf_session *session, int fd)
>     if (perf_file_header__read(&f_header, header, fd) < 0)
>         return -EINVAL;
> 
> +   /*
> +    * sanity check that perf.data was written cleanly: data size
> +    * is initialized to 0 and updated only if the on_exit function
> +    * is run. If data size is still 0 then the file cannot be
> +    * processed.
> +    */
> +   if (f_header.data.size == 0) {
> +       pr_err("data size is 0. Was record properly terminated?\n");
> +       return -1;
> +   }

Hm, this detects the condition - but where does the looping come from?

Can it happen with a perf.data that 'seems' clean but is corrupted 
(because not fully written, buggy kernel just crashed, etc.).

In essence it would be _very_ nice if someone reproduced the looping and 
checked what to do to fix the looping itself. Or does the above
data.size == 0 check fully fix the looping under every possible state of a 
perf.data?

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] perf record: handle death by SIGTERM
  2013-05-08  6:54       ` Ingo Molnar
@ 2013-05-08 13:48         ` David Ahern
  0 siblings, 0 replies; 12+ messages in thread
From: David Ahern @ 2013-05-08 13:48 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Stephane Eranian, Arnaldo Carvalho de Melo, LKML, Mike Galbraith,
	Frederic Weisbecker, Peter Zijlstra, Jiri Olsa, Namhyung Kim

On 5/8/13 12:54 AM, Ingo Molnar wrote:
>
> * David Ahern <dsahern@gmail.com> wrote:
>
>> On 5/7/13 12:29 AM, Ingo Molnar wrote:
>>>
>>> * Stephane Eranian <eranian@google.com> wrote:
>>>
>>>> This is a good fix. I have run into this infinite loop in perf report
>>>> many times.
>>>
>>> Hm, perf record should really not assume much about the perf.data and
>>> should avoid infinite loops ...
>>>
>>> So while making perf.data more consistent on SIGTERM is a nice fix, perf
>>> report should be fixed as well to detect loops and such.
>>>
>>> Thanks,
>>>
>>> 	Ingo
>>>
>>
>> This seems to do the trick:
>>
>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
>> index 326068a..e82646f 100644
>> --- a/tools/perf/util/header.c
>> +++ b/tools/perf/util/header.c
>> @@ -2802,6 +2802,17 @@ int perf_session__read_header(struct
>> perf_session *session, int fd)
>>      if (perf_file_header__read(&f_header, header, fd) < 0)
>>          return -EINVAL;
>>
>> +   /*
>> +    * sanity check that perf.data was written cleanly: data size
>> +    * is initialized to 0 and updated only if the on_exit function
>> +    * is run. If data size is still 0 then the file cannot be
>> +    * processed.
>> +    */
>> +   if (f_header.data.size == 0) {
>> +       pr_err("data size is 0. Was record properly terminated?\n");
>> +       return -1;
>> +   }
>
> Hm, this detects the condition - but where does the looping come from?
>
> Can it happen with a perf.data that 'seems' clean but is corrupted
> (because not fully written, buggy kernel just crashed, etc.).
>
> In essence it would be _very_ nice if someone reproduced the looping and
> checked what to do to fix the looping itself. Or does the above
> data.size == 0 check fully fix the looping under every possible state of a
> perf.data?

I think so. If you want the dirty details here you go.

The looping is in __perf_session__process_events. When the data file is 
not closed properly data_size is 0 and n my case data_offset is 288. 
Dropping into this function:

     page_offset = page_size * (data_offset / page_size);
     file_offset = page_offset;
     head = data_offset - page_offset;

which means

   page_offset = 0
   file_offset = 0
   head = 288

The looping is here:

remap:
     buf = mmap(NULL, mmap_size, mmap_prot, mmap_flags, session->fd,
            file_offset);
     if (buf == MAP_FAILED) {
         pr_err("failed to mmap file\n");
         err = -errno;
         goto out_err;
     }
     mmaps[map_idx] = buf;
     map_idx = (map_idx + 1) & (ARRAY_SIZE(mmaps) - 1);
     file_pos = file_offset + head;

more:
     event = fetch_mmaped_event(session, head, mmap_size, buf);

--> returned event is NULL

     if (!event) {
         if (mmaps[map_idx]) {
             munmap(mmaps[map_idx], mmap_size);
             mmaps[map_idx] = NULL;
         }

         page_offset = page_size * (head / page_size);
         file_offset += page_offset;
         head -= page_offset;

--> head is 288 which means the new page_offset is 0 and the new 
file_offset is 0. head never changes. and then we go back to remap.

         goto remap;
     }

So, if you want to handle the looping then seeing that page_offset new 
in the above is 0 would suffice. A 0 value means file_offset does not 
change and the jump to remap means the mmap does not change. ie., in a 
loop where no values are changing.

David

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] perf record: handle death by SIGTERM
  2013-05-06 18:24 [PATCH] perf record: handle death by SIGTERM David Ahern
  2013-05-06 18:45 ` David Ahern
  2013-05-06 22:40 ` Stephane Eranian
@ 2013-05-24  9:08 ` Jiri Olsa
  2013-05-24 14:11   ` David Ahern
  2013-05-31 11:33 ` [tip:perf/core] " tip-bot for David Ahern
  3 siblings, 1 reply; 12+ messages in thread
From: Jiri Olsa @ 2013-05-24  9:08 UTC (permalink / raw)
  To: David Ahern
  Cc: acme, linux-kernel, Mike Galbraith, Ingo Molnar,
	Frederic Weisbecker, Peter Zijlstra, Namhyung Kim,
	Stephane Eranian

On Mon, May 06, 2013 at 12:24:23PM -0600, David Ahern wrote:
> perf data files cannot be processed until the header file is update
> which is done via an on_exit handler. If perf is killed due to a SIGTERM
> it does not run the on_exit hooks leaving the perf.data file in a
> random state which perf-report will happily spin on trying to read. As
> noted by Mike an easy reproducer is:
>   perf record -a -g & sleep 1; killall perf
> 
> Fix by catching SIGTERM like it does SIGINT.  Also need to remove the
> kill which was added via commit f7b7c26e.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Stephane Eranian <eranian@google.com>
> ---
>  tools/perf/builtin-record.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index cdf58ec..fff985c 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -198,7 +198,6 @@ static void perf_record__sig_exit(int exit_status __maybe_unused, void *arg)
>  		return;
>  
>  	signal(signr, SIG_DFL);
> -	kill(getpid(), signr);
>  }
>  
>  static bool perf_evlist__equal(struct perf_evlist *evlist,
> @@ -404,6 +403,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
>  	signal(SIGCHLD, sig_handler);
>  	signal(SIGINT, sig_handler);
>  	signal(SIGUSR1, sig_handler);
> +	signal(SIGTERM, sig_handler);
>  
>  	if (!output_name) {
>  		if (!fstat(STDOUT_FILENO, &st) && S_ISFIFO(st.st_mode))
> -- 
> 1.7.10.1
> 

hi,
got a perf test hanging on me on latest acme's perf/core and bisected
it to this one. I separated reproducer so far but probably wont get to
it this week:

[jolsa@krava2 perf]$ cat /proc/sys/kernel/perf_event_paranoid
1
[jolsa@krava2 perf]$ ./perf record -C 0 kill
Error:
You may not have permission to collect %sstats.
Consider tweaking /proc/sys/kernel/perf_event_paranoid:
 -1 - Not paranoid at all
  0 - Disallow raw tracepoint access for unpriv
  1 - Disallow cpu events for unpriv
  2 - Disallow kernel profiling for unpriv

jirka

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] perf record: handle death by SIGTERM
  2013-05-24  9:08 ` Jiri Olsa
@ 2013-05-24 14:11   ` David Ahern
  0 siblings, 0 replies; 12+ messages in thread
From: David Ahern @ 2013-05-24 14:11 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: acme, linux-kernel, Mike Galbraith, Ingo Molnar,
	Frederic Weisbecker, Peter Zijlstra, Namhyung Kim,
	Stephane Eranian

On 5/24/13 3:08 AM, Jiri Olsa wrote:
> On Mon, May 06, 2013 at 12:24:23PM -0600, David Ahern wrote:
>> perf data files cannot be processed until the header file is update
>> which is done via an on_exit handler. If perf is killed due to a SIGTERM
>> it does not run the on_exit hooks leaving the perf.data file in a
>> random state which perf-report will happily spin on trying to read. As
>> noted by Mike an easy reproducer is:
>>    perf record -a -g & sleep 1; killall perf
>>
>> Fix by catching SIGTERM like it does SIGINT.  Also need to remove the
>> kill which was added via commit f7b7c26e.
>>
>> Signed-off-by: David Ahern <dsahern@gmail.com>
>> Cc: Mike Galbraith <efault@gmx.de>
>> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Stephane Eranian <eranian@google.com>
>> ---
>>   tools/perf/builtin-record.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index cdf58ec..fff985c 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -198,7 +198,6 @@ static void perf_record__sig_exit(int exit_status __maybe_unused, void *arg)
>>   		return;
>>
>>   	signal(signr, SIG_DFL);
>> -	kill(getpid(), signr);
>>   }
>>
>>   static bool perf_evlist__equal(struct perf_evlist *evlist,
>> @@ -404,6 +403,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
>>   	signal(SIGCHLD, sig_handler);
>>   	signal(SIGINT, sig_handler);
>>   	signal(SIGUSR1, sig_handler);
>> +	signal(SIGTERM, sig_handler);
>>
>>   	if (!output_name) {
>>   		if (!fstat(STDOUT_FILENO, &st) && S_ISFIFO(st.st_mode))
>> --
>> 1.7.10.1
>>
>
> hi,
> got a perf test hanging on me on latest acme's perf/core and bisected
> it to this one. I separated reproducer so far but probably wont get to
> it this week:

Ok, I should have some time to look into it this weekend.

David

>
> [jolsa@krava2 perf]$ cat /proc/sys/kernel/perf_event_paranoid
> 1
> [jolsa@krava2 perf]$ ./perf record -C 0 kill
> Error:
> You may not have permission to collect %sstats.
> Consider tweaking /proc/sys/kernel/perf_event_paranoid:
>   -1 - Not paranoid at all
>    0 - Disallow raw tracepoint access for unpriv
>    1 - Disallow cpu events for unpriv
>    2 - Disallow kernel profiling for unpriv
>
> jirka
>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [tip:perf/core] perf record: handle death by SIGTERM
  2013-05-06 18:24 [PATCH] perf record: handle death by SIGTERM David Ahern
                   ` (2 preceding siblings ...)
  2013-05-24  9:08 ` Jiri Olsa
@ 2013-05-31 11:33 ` tip-bot for David Ahern
  3 siblings, 0 replies; 12+ messages in thread
From: tip-bot for David Ahern @ 2013-05-31 11:33 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: acme, linux-kernel, eranian, hpa, mingo, peterz, efault,
	namhyung, jolsa, fweisbec, dsahern, tglx

Commit-ID:  804f7ac78803ed095bb0402d540f859ecb1be9f1
Gitweb:     http://git.kernel.org/tip/804f7ac78803ed095bb0402d540f859ecb1be9f1
Author:     David Ahern <dsahern@gmail.com>
AuthorDate: Mon, 6 May 2013 12:24:23 -0600
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 28 May 2013 16:23:58 +0300

perf record: handle death by SIGTERM

Perf data files cannot be processed until the header is updated which is
done via an on_exit handler.

If perf is killed due to a SIGTERM it does not run the on_exit hooks
leaving the perf.data file in a random state which perf-report will
happily spin on trying to read.

As noted by Mike an easy reproducer is:

perf record -a -g & sleep 1; killall perf

Fix by catching SIGTERM like it does SIGINT.

Also need to remove the kill which was added via commit f7b7c26e.

Acked-by: Stephane Eranian <eranian@google.com>
Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1367864663-1309-1-git-send-email-dsahern@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index cdf58ec..fff985c 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -198,7 +198,6 @@ static void perf_record__sig_exit(int exit_status __maybe_unused, void *arg)
 		return;
 
 	signal(signr, SIG_DFL);
-	kill(getpid(), signr);
 }
 
 static bool perf_evlist__equal(struct perf_evlist *evlist,
@@ -404,6 +403,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
 	signal(SIGCHLD, sig_handler);
 	signal(SIGINT, sig_handler);
 	signal(SIGUSR1, sig_handler);
+	signal(SIGTERM, sig_handler);
 
 	if (!output_name) {
 		if (!fstat(STDOUT_FILENO, &st) && S_ISFIFO(st.st_mode))

^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2013-05-31 11:33 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-06 18:24 [PATCH] perf record: handle death by SIGTERM David Ahern
2013-05-06 18:45 ` David Ahern
2013-05-06 22:40 ` Stephane Eranian
2013-05-07  0:05   ` David Ahern
2013-05-07  6:29   ` Ingo Molnar
2013-05-07 20:56     ` David Ahern
2013-05-08  6:17       ` Namhyung Kim
2013-05-08  6:54       ` Ingo Molnar
2013-05-08 13:48         ` David Ahern
2013-05-24  9:08 ` Jiri Olsa
2013-05-24 14:11   ` David Ahern
2013-05-31 11:33 ` [tip:perf/core] " tip-bot for David Ahern

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.