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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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

> - [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