Re: [PATCH 4/4] perf report: Treat an argument as a symbol filter

[Ingo Molnar]

* Namhyung Kim <namhyung.kim@xxxxxxx> wrote:

> As Ingo requested, it'd be better off treating first (and the only)
> argument as a symbol filter, so that user doesn't need to input the
> symbol on the dialog window.
> 
> Signed-off-by: Namhyung Kim <namhyung.kim@xxxxxxx>
> ---
>  tools/perf/builtin-report.c |   19 ++++++++++++++-----
>  1 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 25d34d483e49..62c4a11bcf3e 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -50,6 +50,7 @@ struct perf_report {
>  	const char		*pretty_printing_style;
>  	symbol_filter_t		annotate_init;
>  	const char		*cpu_list;
> +	const char		*symbol_filter_str;
>  	DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
>  };
>  
> @@ -315,6 +316,9 @@ static int __cmd_report(struct perf_report *rep)
>  	list_for_each_entry(pos, &session->evlist->entries, node) {
>  		struct hists *hists = &pos->hists;
>  
> +		if (pos->idx == 0)
> +			hists->symbol_filter_str = rep->symbol_filter_str;
> +
>  		hists__collapse_resort(hists);
>  		hists__output_resort(hists);
>  		nr_samples += hists->stats.nr_events[PERF_RECORD_SAMPLE];
> @@ -586,11 +590,16 @@ int cmd_report(int argc, const char **argv, const char *prefix __used)
>  	} else
>  		symbol_conf.exclude_other = false;
>  
> -	/*
> -	 * Any (unrecognized) arguments left?
> -	 */
> -	if (argc)
> -		usage_with_options(report_usage, options);
> +	if (argc) {
> +		/*
> +		 * Special case: if there's an argument left then assume tha

s/tha/that

> +		 * it's a symbol filter:
> +		 */
> +		if (argc > 1)
> +			usage_with_options(report_usage, options);
> +
> +		report.symbol_filter_str = argv[0];
> +	}

Are people fine with this special case?

To play the devil's advocate it uses up valuable perf report 
command line real estate and we probably cannot change this 
workflow in the future, once people get used to it.

For example doing this would preclude us from adding subsystem 
subcommands to perf report in the future, such as:

 $ perf report sched
 $ perf report kvm
 ...

OTOH have 'perf sched' and 'perf kvm' and maybe those are better 
provided as:

 $ perf kvm report
 $ perf sched report

:-)

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/