linux-kernel - Re: [PATCH 1/3] perf tools: Move callchain help messages to callchain.h

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 22 Oct 2015 21:13:49 +0900
From:	Namhyung Kim <namhyung@...nel.org>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Arnaldo Carvalho de Melo <acme@...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

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

>
> 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

>
> 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/