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
>
