Re: [PATCH 15/25] perf report: Make max_stack value allow for synthesized callchains

[Adrian Hunter <adrian.hunter@intel.com>]

On 29/09/15 18:51, Arnaldo Carvalho de Melo wrote:
> Em Tue, Sep 29, 2015 at 11:52:37AM +0300, Adrian Hunter escreveu:
>> On 28/09/15 23:03, Arnaldo Carvalho de Melo wrote:
>>> Em Fri, Sep 25, 2015 at 04:15:46PM +0300, Adrian Hunter escreveu:
>>>> perf report has an option (--max-stack) to set the maximum stack depth
>>>> when processing callchains.  The option defaults to the hard-coded
>>>> maximum definition PERF_MAX_STACK_DEPTH which is 127.  The intention of
>>>> the option is to allow the user to reduce the processing time by
>>>> reducing the amount of the callchain that is processed.
>>>>
>>>> It is also possible, when processing instruction traces, to synthesize
>>>> callchains.  Synthesized callchains do not have the kernel size
>>>> limitation and are whatever size the user requests, although validation
>>>> presently prevents the user requested a value greater that 1024.  The
>>>> default value is 16.
>>>
>>> So, haven't checked the options, but one can possibly use both the way
>>> itrace has to ask for a max stack size and also via --max-stack, right?
>>
>> Possibly, but it would not be a common paradigm.
>>
>>>
>>> In that case we better emit a warning or plain state that one either
>>> uses one way of setting the max stack or the other?
>>
>> max_stack was added as an optimization to reduce processing time, so
>> people specifying --max-stack might get a increased processing time
>> if combined with synthesized callchains, but otherwise no real harm.
>>
>> A warning seems like overkill.  Could amend the documenation e.g.
> 
> Adding the doc part helps, but actually telling that what they are
> trying to do is not possible, even for unlikely scenarios like this,
> seems cleaner, but no biggie.
> 
> I'll add the patch below with your s-o-b, ack?

Yes thank you.

> 
> - Arnaldo
>  
>>
>> diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
>> index b941d5e07e28..ce499035e6d8 100644
>> --- a/tools/perf/Documentation/perf-report.txt
>> +++ b/tools/perf/Documentation/perf-report.txt
>> @@ -205,6 +205,8 @@ OPTIONS
>>  	beyond the specified depth will be ignored. This is a trade-off
>>  	between information loss and faster processing especially for
>>  	workloads that can have a very long callchain stack.
>> +	Note that when using the --itrace option the synthesized callchain size
>> +	will override this value if the synthesized callchain size is bigger.
>>  
>>  	Default: 127
>>  
>>
>>
>>
>>>
>>> I'm applying the patch, because it is unlikely that this gets specified,
>>> but would be good to close this gap.
>>>
>>> - Arnaldo
>>>  
>>>> To allow for synthesized callchains, make the max_stack value at least
>>>> the same size as the synthesized callchain size.
>>>>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>  tools/perf/builtin-report.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
>>>> index e94e5c7155af..37c9f5125887 100644
>>>> --- a/tools/perf/builtin-report.c
>>>> +++ b/tools/perf/builtin-report.c
>>>> @@ -809,6 +809,10 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
>>>>  	if (report.inverted_callchain)
>>>>  		callchain_param.order = ORDER_CALLER;
>>>>  
>>>> +	if (itrace_synth_opts.callchain &&
>>>> +	    (int)itrace_synth_opts.callchain_sz > report.max_stack)
>>>> +		report.max_stack = itrace_synth_opts.callchain_sz;
>>>> +
>>>>  	if (!input_name || !strlen(input_name)) {
>>>>  		if (!fstat(STDIN_FILENO, &st) && S_ISFIFO(st.st_mode))
>>>>  			input_name = "-";
>>>> -- 
>>>> 1.9.1
>>>
>