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
