[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20151022141722.GB2455@kernel.org>
Date: Thu, 22 Oct 2015 11:17:22 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Ingo Molnar <mingo@...nel.org>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Jiri Olsa <jolsa@...hat.com>,
LKML <linux-kernel@...r.kernel.org>,
David Ahern <dsahern@...il.com>,
Adrian Hunter <adrian.hunter@...el.com>,
Borislav Petkov <bp@...e.de>,
Chandler Carruth <chandlerc@...il.com>,
Frederic Weisbecker <fweisbec@...il.com>,
Stephane Eranian <eranian@...gle.com>,
Wang Nan <wangnan0@...wei.com>,
Brendan Gregg <brendan.d.gregg@...il.com>
Subject: Re: [PATCH 1/3] perf tools: Move callchain help messages to
callchain.h
Em Thu, Oct 22, 2015 at 09:13:49PM +0900, Namhyung Kim escreveu:
> On Thu, Oct 22, 2015 at 5:02 PM, Ingo Molnar <mingo@...nel.org> wrote:
> >
> > * Namhyung Kim <namhyung@...nel.org> wrote:
> >
> >> +#define CALLCHAIN_HELP "setup and enables call-graph (stack chain/backtrace) recording: "
> >> +
> >> +#ifdef HAVE_DWARF_UNWIND_SUPPORT
> >> +#define CALLCHAIN_RECORD_HELP CALLCHAIN_HELP "fp dwarf lbr"
> >> +#else
> >> +#define CALLCHAIN_RECORD_HELP CALLCHAIN_HELP "fp lbr"
> >> +#endif
> >
> > nano-nit, could we structure such balanced #ifdefs the following way:
> >
> > #ifdef HAVE_DWARF_UNWIND_SUPPORT
> > # define CALLCHAIN_RECORD_HELP CALLCHAIN_HELP "fp dwarf lbr"
> > #else
> > # define CALLCHAIN_RECORD_HELP CALLCHAIN_HELP "fp lbr"
> > #endif
> >
> > makes the construct stand out a lot better visually.
>
> OK
Done.
> >
> > I also had another look at the help text:
> >
> >> output_type,min_percent[,print_limit],call_order[,branch]
> >
> >> +#define CALLCHAIN_REPORT_HELP "output_type (graph, flat, fractal, or none), " \
> >> + "min percent threshold, optional print limit, callchain order, " \
> >> + "key (function or address), add branches"
> >
> > Btw., when I first read this message in the help text yesterday, I had to read the
> > 'min percent threshold' twice, to realize that the default 0.5 is in units of
> > percentage - the wording wasn't entirely clear about that.
>
> OK
>
> >
> > Also, I had to go into the code to decode the real meaning of all the other
> > parameters. I'd have expected them to be more obvious from reading the help text.
>
> Did you check the man page also? I think we have (short) explanation
> for each parameter and users should read it first to understand the
> meaning. But I agree that the help text should also be improved to
> provide quick reference.
>
>
> >
> > Wording them the following way would have made things a lot more apparent to me:
> >
> > print_style,min_percent[,print_percent],call_order[,key]
> >
> > call chain tree printing style (graph|flat|fractal|none)
> > minimum tree inclusion threshold (percent)
> > printing threshold (percent)
>
> Note that this 'printing threshold' is not percent. It's to limit
> number of callchain entries printed for each hist entry. However it
> works for --stdio only probably since it lacks interactive
> collapse/expand feature.
>
>
> > call chain order (caller|callee)
> > key (function|address|branch)
> >
> > Note that I extended the help text with new options not mentioned in the help text
> > but present in the current code - such as the 'branch' key.
> >
> > Also note that in the code I did not find any trace of the '[,branch]' and
> > 'add branches' part present in the help text. What we have is a 'branch'
> > option in the (optional) key parameter.
>
> Looking at the document, it seems branch is not a key:
>
> branch can be:
> - branch: include last branch information in callgraph
> when available. Usually more convenient to use --branch-history
> for this.
>
> Confusingly, it was checked in parse_callchain_sort_key() but does
> nothing with the sort key IIUC.
>
> >
> > I also made various edits to the help text to make it more consistent and more
> > self-explanatory. I think we should also put the various options into a new line
> > in the help screen, not the single line dump of text it is currently.
>
> OK
These can come on a followup patch, improving the situation, right?
> >
> > Btw., we also have a grammar problem with all things call chains: there's 800+
> > occurances of 'callchain' in the perf code, and less than 20 spellings of 'call
> > chain'. But the latter is the correct variant: Google won't even let you search
> > for 'callchain' by default and corrects it to 'call chain' automatically.
> >
> > If you insist on searching for 'callchain', Google finds this number of hits:
> >
> > 'code callchain': 54,200
> > 'code call chain': 141,000,000
> >
> > I think it's pretty obvious what the dominant spelling is in the industry! ;-)
> >
> > So we should probably rename all occurances of 'callchain' to 'call chain' or
> > 'call_chain'.
>
> Not sure about this part. Do you really think it's worth changing?
>
> Thanks,
> Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists