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

On Tue, 2026-07-14 at 00:56 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [Medium] Arbitrary upper bound on UAPI memory region ID breaks
> forward compatibility for future memory tiering topologies.
> --
> 
> --- Patch [3]: [PATCH v2 3/6] perf tools: Show memory region in perf-
> c2c subcommand ---
> commit 06c5065732519d0c60d1739bb8a421f9120486b9
> Author: Dapeng Mi <[email protected]>
> 
> perf tools: Show memory region in perf-c2c subcommand
> 
> This commit adds a memory region field to the cacheline list view in
> perf-c2c. The new field helps identify the memory region to which the
> cacheline belongs.
> 
> Link:
> https://lore.kernel.org/all/[email protected]/
> 
> > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> > index c9584dbedf77a..cba2e9f21ae7c 100644
> > --- a/tools/perf/builtin-c2c.c
> > +++ b/tools/perf/builtin-c2c.c
> 
> [ ... ]
> 
> > @@ -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;
> 
> [Severity: Medium]
> Does this arbitrary upper bound break forward compatibility for
> future memory
> tiering topologies?
> 
> The UAPI field provides 5 bits, allowing for memory region IDs up to
> 31. By
> rejecting regions greater than PERF_MEM_REGION_MEM7, valid region IDs
> from
> future kernels or hardware architectures will be dropped and
> displayed as
> "N/A", while also triggering a spurious warning.
> 
> It looks like the new display logic in dcacheline_node_mem_region()
> can
> already mathematically scale to handle these higher values
> gracefully.

Currently, only eight memory regions are available on supported
architectures. These definitions can be expanded when future
architectures that support more regions become available.

Thanks,
Tom

> 
> > +
> > +	/* Update mem_region only if it really accesses memory */
> > +	if (mem_region >= PERF_MEM_REGION_MMIO)
> > +		c2c_he->mem_region = mem_region;
> > +}
> > +
>