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

["Falcon, Thomas" <thomas.falcon@intel.com>]

On Mon, 2026-09-14 at 21:07 -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Sep 14, 2026 at 10:57:16PM +0000, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> > - [Medium] perf c2c: valid memory region overwritten by N/A
> 
> All the other are minor stuff that I would fix up myself, but there this
> one, can you take a look at it, address the other minor ones and post
> v10?
> 
> Thanks,
> 
> - Arnaldo

Thanks, I will do that and send a new version soon

Tom

> 
> > - [Low] perf c2c: missing stdio.h inclusion for asprintf
> > --
> > 
> > commit d7af7398254a585e98330957faf348c7603375be
> > Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
> > 
> > perf tools: Show memory region in perf-c2c subcommand
> > 
> > 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) in Intel's Diamond Rapids and
> > Nova Lake architectures.
> > 
> > Link: https://lore.kernel.org/all/20260114011750.350569-1-dapeng1.mi@linux.intel.com/
> > 
> > > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> > > index 715b75d42f2a8..4272f5f22c01f 100644
> > > --- a/tools/perf/builtin-c2c.c
> > > +++ b/tools/perf/builtin-c2c.c
> > > @@ -247,6 +248,16 @@ 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;
> > > +
> > > +	c2c_he->mem_region = mem_region;
> > 
> > [Severity: Medium]
> > Does this code unconditionally overwrite a previously recorded valid memory
> > region with a new value?
> > 
> > In process_sample_event(), this is called for every sample grouped into the
> > cacheline hist_entry:
> > 
> > process_sample_event()
> >   c2c_he__set_mem_region(c2c_he, mem_region);
> > 
> > If multiple samples hit the same cacheline, could an earlier sample that
> > provides valid OMR region data be overwritten by a later standard sample
> > (such as an L1 hit) lacking it? This could result in the cacheline's
> > memory region being incorrectly displayed as 'N/A' in the perf c2c output.
> > 
> > [ ... ]
> > 
> > > @@ -3271,9 +3353,16 @@ static int perf_c2c__report(int argc, const char **argv)
> > >  		goto out_mem2node;
> > >  	}
> > >  
> > > -	if (c2c.display != DISPLAY_SNP_PEER)
> > > -		output_str = "cl_idx,"
> > > +	c2c.show_mem_region = perf_header__has_feat(&session->header,
> > > +						HEADER_MEMORY_RANGES);
> > > +	if (c2c.show_mem_region)
> > > +		dim_dcacheline.header.line[0].span = 3;
> > > +
> > > +	if (c2c.display != DISPLAY_SNP_PEER) {
> > > +		if (asprintf(&output_str,
> > > +			     "cl_idx,"
> > 
> > [Severity: Low]
> > Does this file explicitly include <stdio.h> for asprintf?
> > 
> > Since musl libc enforces strict POSIX header separation and doesn't
> > implicitly include headers, omitting <stdio.h> here could lead to build
> > failures when compiling perf with a musl libc toolchain.
> > 
> > >  			     "dcacheline,"
> > > +			     "%s"
> > 
> > -- 
> > Sashiko AI review · https://sashiko.dev/#/patchset/20260914224650.52982-1-thomas.falcon@intel.com?part=4