Messages in this thread | | | Date | Tue, 20 Oct 2020 09:25:53 +0200 | From | Jiri Olsa <> | Subject | Re: [PATCH v1 7/8] perf c2c: Add option '-d llc' for sorting with LLC load |
| |
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 >
| |