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

[sashiko-bot@kernel.org]

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