[PATCH 1/1] perf record: We should fork only if a program was specified to run

[PATCH 1/1] perf record: We should fork only if a program was specified to run

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

IOW: Now 'perf record -a' works, this was a bug introduced in:

856e96608a72412d319e498a3a7c557571f811bd
"perf record: Properly synchronize child creation"

Reported-by: Pekka Enberg <penberg@cs.helsinki.fi>
Cc: Frédéric Weisbecker <fweisbec@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 63136d0..90eedd9 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -402,7 +402,7 @@ static void atexit_header(void)
 	perf_header__write(&session->header, output, true);
 }
 
-static int __cmd_record(int argc __used, const char **argv)
+static int __cmd_record(int argc, const char **argv)
 {
 	int i, counter;
 	struct stat st;
@@ -422,7 +422,8 @@ static int __cmd_record(int argc __used, const char **argv)
 	signal(SIGCHLD, sig_handler);
 	signal(SIGINT, sig_handler);
 
-	if (pipe(child_ready_pipe) < 0 || pipe(go_pipe) < 0) {
+	if (target_pid == -1 && argc > 0 &&
+	    (pipe(child_ready_pipe) < 0 || pipe(go_pipe) < 0)) {
 		perror("failed to create pipes");
 		exit(-1);
 	}
@@ -483,7 +484,7 @@ static int __cmd_record(int argc __used, const char **argv)
 
 	atexit(atexit_header);
 
-	if (target_pid == -1) {
+	if (target_pid == -1 && argc > 0) {
 		pid = fork();
 		if (pid < 0) {
 			perror("failed to fork");
@@ -667,7 +668,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 
 	argc = parse_options(argc, argv, options, record_usage,
 			    PARSE_OPT_STOP_AT_NON_OPTION);
-	if (!argc && target_pid == -1 && (!system_wide || profile_cpu == -1))
+	if (!argc && target_pid == -1 && !system_wide)
 		usage_with_options(record_usage, options);
 
 	symbol__init();
-- 
1.6.2.5

Re: [PATCH 1/1] perf record: We should fork only if a program was specified to run

[Peter Zijlstra]

On Fri, 2009-12-18 at 18:42 -0200, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> IOW: Now 'perf record -a' works, this was a bug introduced in:
> 
> 856e96608a72412d319e498a3a7c557571f811bd
> "perf record: Properly synchronize child creation"
> 
> Reported-by: Pekka Enberg <penberg@cs.helsinki.fi>
> Cc: Frédéric Weisbecker <fweisbec@gmail.com>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/builtin-record.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 63136d0..90eedd9 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -402,7 +402,7 @@ static void atexit_header(void)
>  	perf_header__write(&session->header, output, true);
>  }
>  
> -static int __cmd_record(int argc __used, const char **argv)
> +static int __cmd_record(int argc, const char **argv)
>  {
>  	int i, counter;
>  	struct stat st;
> @@ -422,7 +422,8 @@ static int __cmd_record(int argc __used, const char **argv)
>  	signal(SIGCHLD, sig_handler);
>  	signal(SIGINT, sig_handler);
>  
> -	if (pipe(child_ready_pipe) < 0 || pipe(go_pipe) < 0) {
> +	if (target_pid == -1 && argc > 0 &&
> +	    (pipe(child_ready_pipe) < 0 || pipe(go_pipe) < 0)) {
>  		perror("failed to create pipes");
>  		exit(-1);
>  	}

Why bother?

> @@ -483,7 +484,7 @@ static int __cmd_record(int argc __used, const char **argv)
>  
>  	atexit(atexit_header);
>  
> -	if (target_pid == -1) {
> +	if (target_pid == -1 && argc > 0) {
>  		pid = fork();
>  		if (pid < 0) {
>  			perror("failed to fork");

I'd write:

target_pid == -1 && !system_wide

> @@ -667,7 +668,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
>  
>  	argc = parse_options(argc, argv, options, record_usage,
>  			    PARSE_OPT_STOP_AT_NON_OPTION);
> -	if (!argc && target_pid == -1 && (!system_wide || profile_cpu == -1))
> +	if (!argc && target_pid == -1 && !system_wide)
>  		usage_with_options(record_usage, options);
>  
>  	symbol__init();

Right, so I was thinking -C would be enough to also start profiling..
clearly messed up the logic though :/

Re: [PATCH 1/1] perf record: We should fork only if a program was specified to run

[Arnaldo Carvalho de Melo]

Em Fri, Dec 18, 2009 at 09:52:41PM +0100, Peter Zijlstra escreveu:
> On Fri, 2009-12-18 at 18:42 -0200, Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > @@ -422,7 +422,8 @@ static int __cmd_record(int argc __used, const char **argv)
> >  	signal(SIGCHLD, sig_handler);
> >  	signal(SIGINT, sig_handler);
> >  
> > -	if (pipe(child_ready_pipe) < 0 || pipe(go_pipe) < 0) {
> > +	if (target_pid == -1 && argc > 0 &&
> > +	    (pipe(child_ready_pipe) < 0 || pipe(go_pipe) < 0)) {
> >  		perror("failed to create pipes");
> >  		exit(-1);
> >  	}
> 
> Why bother?

Because we don't need it?

> > @@ -483,7 +484,7 @@ static int __cmd_record(int argc __used, const char **argv)
> >  
> >  	atexit(atexit_header);
> >  
> > -	if (target_pid == -1) {
> > +	if (target_pid == -1 && argc > 0) {
> >  		pid = fork();
> >  		if (pid < 0) {
> >  			perror("failed to fork");
> 
> I'd write:
> 
> target_pid == -1 && !system_wide

Because we want this to work:

[root@doppio linux-2.6-tip]# perf record -a -f sleep 2
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.243 MB perf.data (~10599 samples) ]
[root@doppio linux-2.6-tip]#

See, no hands mother! I.e. it is system wide, but just for 2 seconds,
its clever, but I like it, and it always worked :-)
 
> > @@ -667,7 +668,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
> >  
> >  	argc = parse_options(argc, argv, options, record_usage,
> >  			    PARSE_OPT_STOP_AT_NON_OPTION);
> > -	if (!argc && target_pid == -1 && (!system_wide || profile_cpu == -1))
> > +	if (!argc && target_pid == -1 && !system_wide)
> >  		usage_with_options(record_usage, options);
> >  
> >  	symbol__init();
> 
> Right, so I was thinking -C would be enough to also start profiling..
> clearly messed up the logic though :/

Yeah, it should, meaning "profile everything on CPU N"

- Arnaldo

Re: [PATCH 1/1] perf record: We should fork only if a program was specified to run

[Peter Zijlstra]

On Fri, 2009-12-18 at 18:58 -0200, Arnaldo Carvalho de Melo wrote:
> Em Fri, Dec 18, 2009 at 09:52:41PM +0100, Peter Zijlstra escreveu:
> > On Fri, 2009-12-18 at 18:42 -0200, Arnaldo Carvalho de Melo wrote:
> > > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > @@ -422,7 +422,8 @@ static int __cmd_record(int argc __used, const char **argv)
> > >  	signal(SIGCHLD, sig_handler);
> > >  	signal(SIGINT, sig_handler);
> > >  
> > > -	if (pipe(child_ready_pipe) < 0 || pipe(go_pipe) < 0) {
> > > +	if (target_pid == -1 && argc > 0 &&
> > > +	    (pipe(child_ready_pipe) < 0 || pipe(go_pipe) < 0)) {
> > >  		perror("failed to create pipes");
> > >  		exit(-1);
> > >  	}
> > 
> > Why bother?
> 
> Because we don't need it?

Well its a rare error, and the paranoid in me says bailing asap avoids
funny cases later.

> > > @@ -483,7 +484,7 @@ static int __cmd_record(int argc __used, const char **argv)
> > >  
> > >  	atexit(atexit_header);
> > >  
> > > -	if (target_pid == -1) {
> > > +	if (target_pid == -1 && argc > 0) {
> > >  		pid = fork();
> > >  		if (pid < 0) {
> > >  			perror("failed to fork");
> > 
> > I'd write:
> > 
> > target_pid == -1 && !system_wide
> 
> Because we want this to work:
> 
> [root@doppio linux-2.6-tip]# perf record -a -f sleep 2
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.243 MB perf.data (~10599 samples) ]
> [root@doppio linux-2.6-tip]#
> 
> See, no hands mother! I.e. it is system wide, but just for 2 seconds,
> its clever, but I like it, and it always worked :-)

Ah, right.. yes, totally forgot about that.

> > > @@ -667,7 +668,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
> > >  
> > >  	argc = parse_options(argc, argv, options, record_usage,
> > >  			    PARSE_OPT_STOP_AT_NON_OPTION);
> > > -	if (!argc && target_pid == -1 && (!system_wide || profile_cpu == -1))
> > > +	if (!argc && target_pid == -1 && !system_wide)
> > >  		usage_with_options(record_usage, options);
> > >  
> > >  	symbol__init();
> > 
> > Right, so I was thinking -C would be enough to also start profiling..
> > clearly messed up the logic though :/
> 
> Yeah, it should, meaning "profile everything on CPU N"

Right, so I tried folding that in and goofed up.

Anyway, patch looks ok, thanks!

Re: [PATCH 1/1] perf record: We should fork only if a program was specified to run

[Peter Zijlstra]

On Fri, 2009-12-18 at 18:58 -0200, Arnaldo Carvalho de Melo wrote:
> > > -   if (pipe(child_ready_pipe) < 0 || pipe(go_pipe) < 0) {
> > > +   if (target_pid == -1 && argc > 0 &&
> > > +       (pipe(child_ready_pipe) < 0 || pipe(go_pipe) < 0)) {
> > >             perror("failed to create pipes");
> > >             exit(-1);
> > >     }

Also, there's an unconditional close() a bit further down.

If not for that, I'd have put the whole mkpipe thing in the if
(target_pid == -1) branch.

Maybe I shouldn't have skimped on the extra state for that close

Re: [PATCH 1/1] perf record: We should fork only if a program was specified to run

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> > Because we want this to work:
> > 
> > [root@doppio linux-2.6-tip]# perf record -a -f sleep 2
> > [ perf record: Woken up 1 times to write data ]
> > [ perf record: Captured and wrote 0.243 MB perf.data (~10599 samples) ]
> > [root@doppio linux-2.6-tip]#
> > 
> > See, no hands mother! I.e. it is system wide, but just for 2 seconds,
> > its clever, but I like it, and it always worked :-)
> 
> Ah, right.. yes, totally forgot about that.

I use the 'perf record -a -f sleep 1' trick all the time to capture a 
system-wide snippet of a particular workload.

	Ingo

Re: [PATCH 1/1] perf record: We should fork only if a program was specified to run

[Xiao Guangrong]

Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> IOW: Now 'perf record -a' works, this was a bug introduced in:
> 
> 856e96608a72412d319e498a3a7c557571f811bd
> "perf record: Properly synchronize child creation"
> 

Sorry, i not noticed this patch and also send a patch to fix this bug,
See:
	http://lkml.org/lkml/2009/12/20/7

please ignore my patch.

Thanks,
Xiao