All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jiri Olsa <jolsa@redhat.com>
To: Leo Yan <leo.yan@linaro.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Kan Liang <kan.liang@linux.intel.com>,
	Andi Kleen <ak@linux.intel.com>, Ian Rogers <irogers@google.com>,
	Joe Mario <jmario@redhat.com>, David Ahern <dsahern@gmail.com>,
	Don Zickus <dzickus@redhat.com>, Al Grant <Al.Grant@arm.com>,
	James Clark <james.clark@arm.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 7/8] perf c2c: Add option '-d llc' for sorting with LLC load
Date: Tue, 20 Oct 2020 09:25:53 +0200	[thread overview]
Message-ID: <20201020072553.GC2084117@krava> (raw)
In-Reply-To: <20201015145041.10953-8-leo.yan@linaro.org>

On Thu, Oct 15, 2020 at 03:50:40PM +0100, Leo Yan wrote:

SNIP

> @@ -1533,6 +1539,7 @@ static struct c2c_header percent_hitm_header[] = {
>  	[DISPLAY_LCL] = HEADER_BOTH("Lcl", "Hitm"),
>  	[DISPLAY_RMT] = HEADER_BOTH("Rmt", "Hitm"),
>  	[DISPLAY_TOT] = HEADER_BOTH("Tot", "Hitm"),
> +	[DISPLAY_LLC] = HEADER_BOTH("LLC", "Hit"),
>  };
>  
>  static struct c2c_dimension dim_percent_hitm = {
> @@ -2002,6 +2009,10 @@ static bool he__display(struct hist_entry *he, struct c2c_stats *stats)
>  		break;
>  	case DISPLAY_TOT:
>  		FILTER_ENTRY(tot_hitm);
> +		break;
> +	case DISPLAY_LLC:
> +		FILTER_ENTRY(tot_llchit);
> +		break;
>  	default:
>  		break;
>  	}
> @@ -2032,6 +2043,9 @@ static inline bool is_valid_hist_entry(struct hist_entry *he)
>  	case DISPLAY_TOT:
>  		has_record = !!c2c_he->stats.tot_hitm;
>  		break;
> +	case DISPLAY_LLC:
> +		has_record = !!c2c_he->stats.tot_llchit;
> +		break;
>  	default:
>  		break;
>  	}

there's one more switch with c2c.display in percent_hitm function,
where you did not add DISPLAY_LLC case.. I guess it should not ever
because we will not use that column in llc display mode, but we
should add at least some warning or that

SNIP

> -				"cl_rmt_hitm,"
> -				"cl_lcl_hitm,"
> -				"cl_stores_l1hit,"
> -				"cl_stores_l1miss,"
> -				"dcacheline",
> -				NULL);
> +	ret = hpp_list__parse(&hpp_list, cl_output, NULL);
>  
>  	if (WARN_ONCE(ret, "failed to setup sort entries\n"))
>  		return;
> @@ -2357,7 +2384,7 @@ static void print_c2c_info(FILE *out, struct perf_session *session)
>  		fprintf(out, "%-36s: %s\n", first ? "  Events" : "", evsel__name(evsel));
>  		first = false;
>  	}
> -	fprintf(out, "  Cachelines sort on                : %s HITMs\n",
> +	fprintf(out, "  Cachelines sort on                : %s\n",
>  		display_str[c2c.display]);
>  	fprintf(out, "  Cacheline data grouping           : %s\n", c2c.cl_sort);
>  }
> @@ -2514,7 +2541,7 @@ static int perf_c2c_browser__title(struct hist_browser *browser,
>  {
>  	scnprintf(bf, size,
>  		  "Shared Data Cache Line Table     "
> -		  "(%lu entries, sorted on %s HITMs)",
> +		  "(%lu entries, sorted on %s)",
>  		  browser->nr_non_filtered_entries,
>  		  display_str[c2c.display]);
>  	return 0;
> @@ -2720,6 +2747,8 @@ static int setup_display(const char *str)
>  		c2c.display = DISPLAY_RMT;
>  	else if (!strcmp(display, "lcl"))
>  		c2c.display = DISPLAY_LCL;
> +	else if (!strcmp(display, "llc"))
> +		c2c.display = DISPLAY_LLC;

please update man page with this

>  	else {
>  		pr_err("failed: unknown display type: %s\n", str);
>  		return -1;
> @@ -2766,9 +2795,10 @@ static int build_cl_output(char *cl_sort, bool no_source)
>  	}
>  
>  	if (asprintf(&c2c.cl_output,
> -		"%s%s%s%s%s%s%s%s%s%s",
> +		"%s%s%s%s%s%s%s%s%s%s%s",

why is there extra '%s' when we did not add new argument.. ?

SNIP

> +			     "ld_fbhit,ld_l1hit,ld_l2hit,"
> +			     "ld_lclhit,lcl_hitm,"
> +			     "ld_rmthit,rmt_hitm,"
> +			     "dram_lcl,dram_rmt";
> +	else /* c2c.display == DISPLAY_LLC */
> +		output_str = "cl_idx,"
> +			     "dcacheline,"
> +			     "dcacheline_node,"
> +			     "dcacheline_count,"
> +			     "percent_llchit,"
> +			     "tot_llchit,"
> +			     "tot_recs,"
> +			     "tot_loads,"
> +			     "tot_stores,"
> +			     "stores_l1hit,stores_l1miss,"
> +			     "ld_fbhit,ld_l1hit,ld_l2hit,"
> +			     "ld_lclhit,lcl_hitm,"
> +			     "ld_rmthit,rmt_hitm,"
> +			     "dram_lcl,dram_rmt";
> +
> +	if (c2c.display == DISPLAY_TOT)
> +		sort_str = "tot_hitm";
> +	else if (c2c.display == DISPLAY_RMT)
> +		sort_str = "rmt_hitm";
> +	else if (c2c.display == DISPLAY_LCL)
> +		sort_str = "lcl_hitm";
> +	else if (c2c.display == DISPLAY_LLC)
> +		sort_str = "tot_llchit";
> +

could you please split addition of output_str/sort_str into separate
patch and then add DISPLAY_LLC support? it'd ease up review 

perhaps include also print_pareto changes in it 

thanks,
jirka

> +	c2c_hists__reinit(&c2c.hists, output_str, sort_str);
>  
>  	ui_progress__init(&prog, c2c.hists.hists.nr_entries, "Sorting...");
>  
> -- 
> 2.17.1
> 


  reply	other threads:[~2020-10-20  7:26 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-15 14:50 [PATCH v1 0/8] perf c2c: Sort cacheline with LLC load Leo Yan
2020-10-15 14:50 ` [PATCH v1 1/8] perf mem: Add structure field c2c_stats::tot_llchit Leo Yan
2020-10-15 14:50 ` [PATCH v1 2/8] perf c2c: Add dimensions for total LLC hit Leo Yan
2020-10-15 14:50 ` [PATCH v1 3/8] perf c2c: Add dimensions for LLC load hit Leo Yan
2020-10-15 14:50 ` [PATCH v1 4/8] perf c2c: Change to general naming for macros Leo Yan
2020-10-15 14:50 ` [PATCH v1 5/8] perf c2c: Rename for shared cache line stats Leo Yan
2020-10-15 14:50 ` [PATCH v1 6/8] perf c2c: Refactor hist entry validation Leo Yan
2020-10-15 14:50 ` [PATCH v1 7/8] perf c2c: Add option '-d llc' for sorting with LLC load Leo Yan
2020-10-20  7:25   ` Jiri Olsa [this message]
2020-10-20  8:08     ` Leo Yan
2020-10-22  8:43       ` Jiri Olsa
2020-10-20  7:26   ` Jiri Olsa
2020-10-20  8:14     ` Leo Yan
2020-10-15 14:50 ` [PATCH v1 8/8] perf c2c: Update documentation for display option 'llc' Leo Yan
2020-10-20  7:26   ` Jiri Olsa
2020-10-15 15:05 ` [PATCH v1 0/8] perf c2c: Sort cacheline with LLC load Arnaldo Carvalho de Melo
2020-10-15 15:14   ` Leo Yan
2020-10-20  8:13 ` Namhyung Kim
2020-10-20  8:18   ` Leo Yan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201020072553.GC2084117@krava \
    --to=jolsa@redhat.com \
    --cc=Al.Grant@arm.com \
    --cc=acme@kernel.org \
    --cc=ak@linux.intel.com \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=dsahern@gmail.com \
    --cc=dzickus@redhat.com \
    --cc=irogers@google.com \
    --cc=james.clark@arm.com \
    --cc=jmario@redhat.com \
    --cc=kan.liang@linux.intel.com \
    --cc=leo.yan@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.