lkml.org 
[lkml]   [2009]   [Dec]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/1] perf record: We should fork only if a program was specified to run
From
Date
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!



\
 
 \ /
  Last update: 2009-12-18 22:17    [W:0.080 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site