Re: [PATCH v4 4/6] perf tools: Show memory region in perf-c2c subcommand

On Wed, 2026-08-19 at 11:29 -0700, Namhyung Kim wrote:
> Hello,
> 
> On Mon, Aug 17, 2026 at 02:35:44PM -0700, Ian Rogers wrote:
> > On Tue, Aug 11, 2026 at 10:34 AM Thomas Falcon
> > <[email protected]> wrote:
> > > 
> > > From: Dapeng Mi <[email protected]>
> > > 
> > > Add memory region field to the cacheline list view to help users
> > > identify the memory region to which the cacheline belongs. The
> > > memory
> > > region field was included with the introduction of support for
> > > the
> > > Off-module Response facility (OMR) [1] in Intel's Diamond Rapids
> > > and
> > > Nova Lake architectures.
> > > 
> > > An example of the new perf c2c output including the memory region
> > > field is shown below:
> > > 
> > > Shared Data Cache Line Table     (112 entries, sorted on Total
> > > HITMs)
> > >        --------------- Cacheline --------------      Tot  -------
> > > Load Hitm -------    Total    Total    Total
> > > Index             Address  Region  Node  PA cnt     Hitm   
> > > Total  LclHitm  RmtHitm  records    Loads   Stores
> > >     0  0xff1b7032a1e8c5c0  N/A        1      47    2.25%      
> > > 50       44        6     1024     1023        1
> > >     1  0xff1b6ff3255bb880  N/A        0      11    1.62%      
> > > 36       34        2       96       93        4
> > >     2  0xff1b70328e3b9880  N/A        1      13    1.49%      
> > > 33       33        0      100       91        9
> > >     3  0xff1b70328b023800  N/A        1       1    1.40%      
> > > 31       14       17       52       48        4
> > >     4  0xff1b70328e3b9c00  N/A        1       1    1.31%      
> > > 29       27        2       67       33       34
> > >     5  0xff1b70328b0237c0  N/A        1       1    1.26%      
> > > 28       10       18      154      150        4
> > >     6  0xff1b6ff3255bbc00  N/A        0       1    1.13%      
> > > 25       25        0       48       25       23
> > >     7  0xff1b6ff3255bba40  N/A        0       1    0.99%      
> > > 22       22        0       46       23       23
> > >     8  0xff3ab9ba50255c80  N/A      N/A       0    0.77%      
> > > 17       15        2       35       35        0
> > >     9  0xff3ab9ba503e3040  N/A      N/A       0    0.77%      
> > > 17       11        6       37       37        1
> > >    10  0xff1b703289e88f40  N/A        1      33    0.72%      
> > > 16        9        7       69       63        6
> > >    11  0xff1b70328e3b9a40  N/A        1       1    0.68%      
> > > 15       15        0       43       17       26
> > >    12  0xff1b7032c9fd6a40  N/A        1      15    0.68%      
> > > 15       15        0       57       54        4
> > >    13  0xff1b7032a1e8c980  N/A        1      27    0.54%      
> > > 12       11        1      761      761        0
> > >    14  0xff1b70727ffd57c0  N/A        1       7    0.54%      
> > > 12       12        0      219      218        1
> > >    15  0xffffffffaefe2380  N/A        1       1    0.50%      
> > > 11        8        3       14       14        0
> > > 
> > > Note: DMR simics does not support memory regions. Since the
> > > output is
> > > captured on SPR, the memory region field shows "N/A" for all
> > > cachelines.
> > > 
> > > [1]:
> > > https://lore.kernel.org/all/[email protected]/
> > > 
> > > Assisted-by: Sashiko:gemini-3.1-pro-preview
> > > Assisted-by: GitHub-Copilot:claude-opus-4-8
> > > Signed-off-by: Dapeng Mi <[email protected]>
> > > Signed-off-by: Thomas Falcon <[email protected]>
> > > Link:
> > > https://lore.kernel.org/all/[email protected]/
> > 
> > Reviewed-by: Ian Rogers <[email protected]>
> > 
> > Thanks,
> > Ian
> > 
> > > ---
> > > v4: correctly handle output_str memory allocation failure
> > > 
> > > v3: make memory region reporting conditional on feature bit
> > > ---
> > >  tools/perf/builtin-c2c.c | 93
> > > +++++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 83 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> > > index c9584dbedf77..8cd90524fe49 100644
> > > --- a/tools/perf/builtin-c2c.c
> > > +++ b/tools/perf/builtin-c2c.c
> > > @@ -74,6 +74,7 @@ struct c2c_hist_entry {
> > >         unsigned long           *nodeset;
> > >         struct c2c_stats        *node_stats;
> > >         unsigned int             cacheline_idx;
> > > +       unsigned int             mem_region;
> > > 
> > >         struct compute_stats     cstats;
> > > 
> > > @@ -281,6 +282,18 @@ static void c2c_he__set_node(struct
> > > c2c_hist_entry *c2c_he,
> > >         }
> > >  }
> > > 
> > > +static void c2c_he__set_mem_region(struct c2c_hist_entry
> > > *c2c_he,
> > > +                                  unsigned int mem_region)
> > > +{
> > > +       if (WARN_ONCE(mem_region > PERF_MEM_REGION_MEM7,
> > > +                     "WARNING: invalid memory region ID\n"))
> > > +               return;
> > > +
> > > +       /* Update mem_region only if it really accesses memory */
> > > +       if (mem_region >= PERF_MEM_REGION_MMIO)
> > > +               c2c_he->mem_region = mem_region;
> > > +}
> > > +
> > >  static void compute_stats(struct c2c_hist_entry *c2c_he,
> > >                           struct c2c_stats *stats,
> > >                           u64 weight)
> > > @@ -339,6 +352,7 @@ static int process_sample_event(const struct
> > > perf_tool *tool __maybe_unused,
> > >         struct addr_location al;
> > >         struct mem_info *mi = NULL;
> > >         struct callchain_cursor *cursor;
> > > +       unsigned int mem_region;
> > >         int ret;
> > > 
> > >         addr_location__init(&al);
> > > @@ -366,6 +380,7 @@ static int process_sample_event(const struct
> > > perf_tool *tool __maybe_unused,
> > >         }
> > > 
> > >         c2c_decode_stats(&stats, mi);
> > > +       mem_region = mem_info__data_src(mi)->mem_region;
> > > 
> > >         he = hists__add_entry_ops(&c2c_hists->hists,
> > > &c2c_entry_ops,
> > >                                   &al, NULL, NULL, mi, NULL,
> > > @@ -382,6 +397,7 @@ static int process_sample_event(const struct
> > > perf_tool *tool __maybe_unused,
> > >         c2c_he__set_cpu(c2c_he, sample);
> > >         c2c_he__set_node(c2c_he, sample);
> > >         c2c_he__set_evsel(c2c_he, evsel);
> > > +       c2c_he__set_mem_region(c2c_he, mem_region);
> > > 
> > >         hists__inc_nr_samples(&c2c_hists->hists, he->filtered);
> > > 
> > > @@ -435,6 +451,7 @@ static int process_sample_event(const struct
> > > perf_tool *tool __maybe_unused,
> > >                 c2c_he__set_cpu(c2c_he, sample);
> > >                 c2c_he__set_node(c2c_he, sample);
> > >                 c2c_he__set_evsel(c2c_he, evsel);
> > > +               c2c_he__set_mem_region(c2c_he, mem_region);
> > > 
> > >                 hists__inc_nr_samples(&c2c_hists->hists, he-
> > > >filtered);
> > >                 ret = hist_entry__append_callchain(he, sample);
> > > @@ -603,6 +620,30 @@ dcacheline_node_count(struct perf_hpp_fmt
> > > *fmt, struct perf_hpp *hpp,
> > >         return scnprintf(hpp->buf, hpp->size, "%*lu", width,
> > > c2c_he->paddr_cnt);
> > >  }
> > > 
> > > +static int
> > > +dcacheline_node_mem_region(struct perf_hpp_fmt *fmt, struct
> > > perf_hpp *hpp,
> > > +                          struct hist_entry *he)
> > > +{
> > > +       int width = c2c_width(fmt, hpp, he->hists);
> > > +       struct c2c_hist_entry *c2c_he;
> > > +       unsigned int mem_region;
> > > +       char buf[20];
> > > +
> > > +       c2c_he = container_of(he, struct c2c_hist_entry, he);
> > > +       mem_region = c2c_he->mem_region;
> > > +
> > > +       if (mem_region == PERF_MEM_REGION_NA)
> > > +               scnprintf(buf, sizeof(buf),  "N/A");
> > > +       /* mem_region could only be >= PERF_MEM_REGION_MMIO */
> > > +       else if (mem_region == PERF_MEM_REGION_MMIO)
> > > +               scnprintf(buf, sizeof(buf), "MMIO");
> > > +       else
> > > +               scnprintf(buf, sizeof(buf), "0x%x",
> > > +                         mem_region - PERF_MEM_REGION_MEM0);
> > > +
> > > +       return scnprintf(hpp->buf, hpp->size, "%*s", width, buf);
> > > +}
> > > +
> > >  static int offset_entry(struct perf_hpp_fmt *fmt, struct
> > > perf_hpp *hpp,
> > >                         struct hist_entry *he)
> > >  {
> > > @@ -1425,7 +1466,7 @@ cl_idx_empty_entry(struct perf_hpp_fmt
> > > *fmt, struct perf_hpp *hpp,
> > >         }
> > > 
> > >  static struct c2c_dimension dim_dcacheline = {
> > > -       .header         = HEADER_SPAN("--- Cacheline ----",
> > > "Address", 2),
> > > +       .header         = HEADER_SPAN("--- Cacheline ----",
> > > "Address", 3),
> 
> Doesn't this also need to be handled dynamically depending on the
> availability of memory regions?

Thanks, I will update it to handle memory regions dynamically.

> 
> Thanks,
> Namhyung
> 
> 
> > >         .name           = "dcacheline",
> > >         .cmp            = dcacheline_cmp,
> > >         .entry          = dcacheline_entry,
> > > @@ -1440,6 +1481,14 @@ static struct c2c_dimension
> > > dim_dcacheline_node = {
> > >         .width          = 4,
> > >  };
> > > 
> > > +static struct c2c_dimension dim_dcacheline_mem_region = {
> > > +       .header         = HEADER_LOW("Region"),
> > > +       .name           = "dcacheline_mem_region",
> > > +
> > >        .cmp            = empty_cmp,
> > > +       .entry          = dcacheline_node_mem_region,
> > > +       .width          = 6,
> > > +};
> > > +
> > >  static struct c2c_dimension dim_dcacheline_count = {
> > >         .header         = HEADER_LOW("PA cnt"),
> > >         .name           = "dcacheline_count",
> > > @@ -1871,6 +1920,7 @@ static struct c2c_dimension
> > > dim_dcacheline_num_empty = {
> > > 
> > >  static struct c2c_dimension *dimensions[] = {
> > >         &dim_dcacheline,
> > > +       &dim_dcacheline_mem_region,
> > >         &dim_dcacheline_node,
> > >         &dim_dcacheline_count,
> > >         &dim_offset,
> > > @@ -2898,8 +2948,9 @@ static int ui_quirks(void)
> > >         /* Fix the zero line for dcacheline column. */
> > >         buf = fill_line(chk_double_cl ? "Double-Cacheline" :
> > > "Cacheline",
> > >                                 dim_dcacheline.width +
> > > +                               dim_dcacheline_mem_region.width +
> > >                                 dim_dcacheline_node.width +
> > > -                               dim_dcacheline_count.width + 4);
> > > +                               dim_dcacheline_count.width + 6);

Do you think this needs to be handled dynamically too?
Thanks,
Tom

> > >         if (!buf)
> > >                 return -ENOMEM;
> > > 
> > > @@ -3151,8 +3202,10 @@ static int perf_c2c__report(int argc,
> > > const char **argv)
> > >         OPT_END()
> > >         };
> > >         int err = 0;
> > > -       const char *output_str, *sort_str = NULL;
> > > +       const char *sort_str = NULL;
> > > +       char *output_str = NULL;
> > >         struct perf_env *env;
> > > +       bool has_mem_regions;
> > > 
> > >         annotation_options__init();
> > > 
> > > @@ -3316,9 +3369,14 @@ static int perf_c2c__report(int argc,
> > > const char **argv)
> > >                 goto out_mem2node;
> > >         }
> > > 
> > > -       if (c2c.display != DISPLAY_SNP_PEER)
> > > -               output_str = "cl_idx,"
> > > +       has_mem_regions = perf_header__has_feat(&session->header,
> > > +                                              
> > > HEADER_MEMORY_RANGES);
> > > +
> > > +       if (c2c.display != DISPLAY_SNP_PEER) {
> > > +               if (asprintf(&output_str,
> > > +                            "cl_idx,"
> > >                              "dcacheline,"
> > > +                            "%s"
> > >                              "dcacheline_node,"
> > >                              "dcacheline_count,"
> > >                              "percent_costly_snoop,"
> > > @@ -3330,10 +3388,17 @@ static int perf_c2c__report(int argc,
> > > const char **argv)
> > >                              "ld_fbhit,ld_l1hit,ld_l2hit,"
> > >                              "ld_lclhit,lcl_hitm,"
> > >                              "ld_rmthit,rmt_hitm,"
> > > -                            "dram_lcl,dram_rmt";
> > > -       else
> > > -               output_str = "cl_idx,"
> > > +                            "dram_lcl,dram_rmt",
> > > +                            has_mem_regions ?
> > > +                            "dcacheline_mem_region," : "") < 0)
> > > {
> > > +                       err = -ENOMEM;
> > > +                       goto out_mem2node;
> > > +               }
> > > +       } else {
> > > +               if (asprintf(&output_str,
> > > +                            "cl_idx,"
> > >                              "dcacheline,"
> > > +                            "%s"
> > >                              "dcacheline_node,"
> > >                              "dcacheline_count,"
> > >                              "percent_costly_snoop,"
> > > @@ -3345,7 +3410,13 @@ static int perf_c2c__report(int argc,
> > > const char **argv)
> > >                              "ld_fbhit,ld_l1hit,ld_l2hit,"
> > >                              "ld_lclhit,lcl_hitm,"
> > >                              "ld_rmthit,rmt_hitm,"
> > > -                            "dram_lcl,dram_rmt";
> > > +                            "dram_lcl,dram_rmt",
> > > +                            has_mem_regions ?
> > > +                            "dcacheline_mem_region," : "") < 0)
> > > {
> > > +                       err = -ENOMEM;
> > > +                       goto out_mem2node;
> > > +               }
> > > +       }
> > > 
> > >         if (c2c.display == DISPLAY_TOT_HITM)
> > >                 sort_str = "tot_hitm";
> > > @@ -3368,11 +3439,13 @@ static int perf_c2c__report(int argc,
> > > const char **argv)
> > > 
> > >         if (ui_quirks()) {
> > >                 pr_err("failed to setup UI\n");
> > > -               goto out_mem2node;
> > > +               goto out_str;
> > >         }
> > > 
> > >         perf_c2c_display(session);
> > > 
> > > +out_str:
> > > +       free(output_str);
> > >  out_mem2node:
> > >         mem2node__exit(&c2c.mem2node);
> > >  out_session:
> > > --
> > > 2.55.0
> > >