Message-ID: <20151022080204.GA18916@gmail.com>
Date:	Thu, 22 Oct 2015 10:02:04 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Namhyung Kim <namhyung@...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


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

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.

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.

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

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.

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

Thanks,

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