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

[PATCH 1/1 v2] 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"

Also fix the -C usage, i.e. allow for profiling all the tasks in one
CPU.

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 |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 63136d0..2654253 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;
@@ -411,6 +411,7 @@ static int __cmd_record(int argc __used, const char **argv)
 	int err;
 	unsigned long waking = 0;
 	int child_ready_pipe[2], go_pipe[2];
+	const bool forks = target_pid == -1 && argc > 0;
 	char buf;
 
 	page_size = sysconf(_SC_PAGE_SIZE);
@@ -422,7 +423,7 @@ 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 (forks && (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 (forks) {
 		pid = fork();
 		if (pid < 0) {
 			perror("failed to fork");
@@ -550,7 +551,7 @@ static int __cmd_record(int argc __used, const char **argv)
 			return err;
 	}
 
-	if (!system_wide)
+	if (!system_wide && profile_cpu == -1)
 		event__synthesize_thread(pid, process_synthesized_event,
 					 session);
 	else
@@ -569,7 +570,8 @@ static int __cmd_record(int argc __used, const char **argv)
 	/*
 	 * Let the child rip
 	 */
-	close(go_pipe[1]);
+	if (forks)
+		close(go_pipe[1]);
 
 	for (;;) {
 		int hits = samples;
@@ -667,7 +669,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 && profile_cpu == -1)
 		usage_with_options(record_usage, options);
 
 	symbol__init();
-- 
1.6.2.5

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

[Peter Zijlstra]

On Sat, 2009-12-19 at 19:22 -0200, Arnaldo Carvalho de Melo wrote:
> @@ -550,7 +551,7 @@ static int __cmd_record(int argc __used, const char **argv)
>                         return err;
>         }
>  
> -       if (!system_wide)
> +       if (!system_wide && profile_cpu == -1)
>                 event__synthesize_thread(pid, process_synthesized_event,
>                                          session); 

Not actually sure about this bit though, since we now have
per-task-per-cpu things, the profile_cpu is also used to !system_wide.

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

[Arnaldo Carvalho de Melo]

Em Sat, Dec 19, 2009 at 10:39:01PM +0100, Peter Zijlstra escreveu:
> On Sat, 2009-12-19 at 19:22 -0200, Arnaldo Carvalho de Melo wrote:
> > @@ -550,7 +551,7 @@ static int __cmd_record(int argc __used, const char **argv)
> >                         return err;
> >         }
> >  
> > -       if (!system_wide)
> > +       if (!system_wide && profile_cpu == -1)
> >                 event__synthesize_thread(pid, process_synthesized_event,
> >                                          session); 
> 
> Not actually sure about this bit though, since we now have
> per-task-per-cpu things, the profile_cpu is also used to !system_wide.

this actually means: if it is not system wide and not for an specific
cpu, it must be for an specific pid.

Not clear, right, you may want to profile an specific pid but only when
it is running on an specific CPU... But at least this patch allows for
specifiying either an specific CPU or an specific pid or systemwide.

- Arnaldo

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

[Peter Zijlstra]

On Sat, 2009-12-19 at 19:22 -0200, Arnaldo Carvalho de Melo wrote:
> @@ -422,7 +423,7 @@ 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 (forks && (pipe(child_ready_pipe) < 0 || pipe(go_pipe) < 0)) {
>                 perror("failed to create pipes");
>                 exit(-1);
>         } 

you can avoid this by simply putting the whole thing into the if (forks)
thing below.

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

[Peter Zijlstra]

On Sat, 2009-12-19 at 19:58 -0200, Arnaldo Carvalho de Melo wrote:
> Em Sat, Dec 19, 2009 at 10:39:01PM +0100, Peter Zijlstra escreveu:
> > On Sat, 2009-12-19 at 19:22 -0200, Arnaldo Carvalho de Melo wrote:
> > > @@ -550,7 +551,7 @@ static int __cmd_record(int argc __used, const char **argv)
> > >                         return err;
> > >         }
> > >  
> > > -       if (!system_wide)
> > > +       if (!system_wide && profile_cpu == -1)
> > >                 event__synthesize_thread(pid, process_synthesized_event,
> > >                                          session); 
> > 
> > Not actually sure about this bit though, since we now have
> > per-task-per-cpu things, the profile_cpu is also used to !system_wide.
> 
> this actually means: if it is not system wide and not for an specific
> cpu, it must be for an specific pid.

But it can be for a specific pid and specific cpu.

> Not clear, right, you may want to profile an specific pid but only when
> it is running on an specific CPU... But at least this patch allows for
> specifiying either an specific CPU or an specific pid or systemwide.

Ah, right, not enough state to tell what we meant, fair enough.