Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 20 Oct 2020 16:08:39 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Jiri Olsa <jolsa@redhat.com>
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
Message-ID: <20201020080839.GC13630@leoy-ThinkPad-X240s>
References: <20201015145041.10953-1-leo.yan@linaro.org>
 <20201015145041.10953-8-leo.yan@linaro.org>
 <20201020072553.GC2084117@krava>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201020072553.GC2084117@krava>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

Hi Jiri,

On Tue, Oct 20, 2020 at 09:25:53AM +0200, Jiri Olsa wrote:
> 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

Exactly, for "DISPLAY_LLC" case, it will not run in the function
percent_hitm(); will add warning for 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.. ?

This is deliberate.  The change is as below:

        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",
                c2c.use_stdio ? "cl_num_empty," : "",
-               "percent_rmt_hitm,"
+               c2c.display == DISPLAY_LLC ? "percent_llc_hit," :
+                                            "percent_rmt_hitm,",
                "percent_lcl_hitm,"

In the old code the string "percent_rmt_hitm," is merged with later
lines (the second string is "percent_lcl_hitm,") into single string.

In this patch, it needs to check condition c2c.display and pass
different string ("percent_llc_hit," vs "percent_rmt_hitm,"), thus
the string ("percent_llc_hit," or "percent_rmt_hitm,") is passed
independently, it's _NOT_ jointed with sequnetial lines.

> 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 

Will do.

Thanks a lot for reviewing,
Leo