Re: [PATCH V2 05/12] perf mem: Factor out a function to generate sort order

[Jiri Olsa <jolsa@redhat.com>]

On Mon, Nov 30, 2020 at 09:27:56AM -0800, kan.liang@linux.intel.com wrote:
> From: Kan Liang <kan.liang@linux.intel.com>
> 
> Now, "--phys-data" is the only option which impacts the sort order.
> A simple "if else" is enough to handle the option. But there will be
> more options added, e.g. "--data-page-size", which also impact the sort
> order. The code will become too complex to be maintained.
> 
> Divide the sort order string into several small pieces.
> The first piece is always the default sort string for LOAD/STORE.
> Appends the specific sort string if related option is applied.
> 
> No functional change.
> 
> Acked-by: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> ---
>  tools/perf/builtin-mem.c | 41 ++++++++++++++++++++++++++--------------
>  1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
> index fdfbff7592f4..823742036ddb 100644
> --- a/tools/perf/builtin-mem.c
> +++ b/tools/perf/builtin-mem.c
> @@ -298,11 +298,35 @@ static int report_raw_events(struct perf_mem *mem)
>  	perf_session__delete(session);
>  	return ret;
>  }
> +static char *get_sort_order(struct perf_mem *mem)
> +{
> +	bool has_extra_options = mem->phys_addr ? true : false;

hum, would simple assignment do? ;-)

how about to do this like in c2c with extra %s:

       if (!(mem->operation & MEM_OPERATION_LOAD)) {
               strcpy(sort, "--sort=mem,sym,dso,symbol_daddr,"
                            "dso_daddr,tlb,locked%s",
                            mem->phys_addr ? ",phys_daddr" : "");
       } else if (mem->phys_addr) {
               strcpy(sort, "--sort=local_weight,mem,sym,dso,symbol_daddr,"
                            "dso_daddr,snoop,tlb,locked,phys_daddr");
       } else
               return NULL;

jirka

> +	char sort[128];
> +
> +	/*
> +	 * there is no weight (cost) associated with stores, so don't print
> +	 * the column
> +	 */
> +	if (!(mem->operation & MEM_OPERATION_LOAD)) {
> +		strcpy(sort, "--sort=mem,sym,dso,symbol_daddr,"
> +			     "dso_daddr,tlb,locked");
> +	} else if (has_extra_options) {
> +		strcpy(sort, "--sort=local_weight,mem,sym,dso,symbol_daddr,"
> +			     "dso_daddr,snoop,tlb,locked");
> +	} else
> +		return NULL;
> +
> +	if (mem->phys_addr)
> +		strcat(sort, ",phys_daddr");
> +
> +	return strdup(sort);
> +}
>  
>  static int report_events(int argc, const char **argv, struct perf_mem *mem)
>  {
>  	const char **rep_argv;
>  	int ret, i = 0, j, rep_argc;
> +	char *new_sort_order;
>  
>  	if (mem->dump_raw)
>  		return report_raw_events(mem);
> @@ -316,20 +340,9 @@ static int report_events(int argc, const char **argv, struct perf_mem *mem)
>  	rep_argv[i++] = "--mem-mode";
>  	rep_argv[i++] = "-n"; /* display number of samples */
>  
> -	/*
> -	 * there is no weight (cost) associated with stores, so don't print
> -	 * the column
> -	 */
> -	if (!(mem->operation & MEM_OPERATION_LOAD)) {
> -		if (mem->phys_addr)
> -			rep_argv[i++] = "--sort=mem,sym,dso,symbol_daddr,"
> -					"dso_daddr,tlb,locked,phys_daddr";
> -		else
> -			rep_argv[i++] = "--sort=mem,sym,dso,symbol_daddr,"
> -					"dso_daddr,tlb,locked";
> -	} else if (mem->phys_addr)
> -		rep_argv[i++] = "--sort=local_weight,mem,sym,dso,symbol_daddr,"
> -				"dso_daddr,snoop,tlb,locked,phys_daddr";
> +	new_sort_order = get_sort_order(mem);
> +	if (new_sort_order)
> +		rep_argv[i++] = new_sort_order;
>  
>  	for (j = 1; j < argc; j++, i++)
>  		rep_argv[i] = argv[j];
> -- 
> 2.17.1
>