Messages in this thread | | | Date | Tue, 20 Oct 2020 16:08:39 +0800 | From | Leo Yan <> | Subject | Re: [PATCH v1 7/8] perf c2c: Add option '-d llc' for sorting with LLC load |
| |
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
| |