Re: [PATCH 2/2] perf c2c: Show all memory regions in the region column

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 11/6/2026 3:51 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issues to consider:
> - [Low] perf c2c: memory leak of c2c_he->regionstr in c2c_he_free()
> - [Low] perf c2c: calc_width() populates regionstr and width on child
>   entries
> --
>
> commit bee454704ac015d02d879348350c49da07d10fb5
> Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
>
> perf c2c: Show all memory regions in the region column
>
> This commit updates perf c2c to record a bitmap of all sampled memory
> regions per histogram entry and format them into a heap-allocated string
> displayed in the memory region column.
>
>> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
>> index f51fa23f3bf18..9c638fbfc2eaa 100644
>> --- a/tools/perf/builtin-c2c.c
>> +++ b/tools/perf/builtin-c2c.c
> [ ... ]
>
>> @@ -2264,6 +2239,52 @@ static int set_nodestr(struct c2c_hist_entry *c2c_he)
>>  	return c2c_he->nodestr ? 0 : -ENOMEM;
>>  }
>>  
>> +#define PERF_MEM_REGION_MASK	\
>> +	GENMASK(PERF_MEM_REGION_MEM7, PERF_MEM_REGION_MEM0)
>> +#define PERF_MEM_REGION_NUM	\
>> +	(PERF_MEM_REGION_MEM7 - PERF_MEM_REGION_MEM0 + 1)
>> +
>> +static int set_mem_region_str(struct c2c_hist_entry *c2c_he)
>> +{
>> +	char buf[32];
>> +	int len = 0;
>> +
>> +	if (c2c_he->regionstr)
>> +		return 0;
>> +
>> +	if (!c2c_he->regionset) {
>> +		len = scnprintf(buf, sizeof(buf), "N/A");
>> +		goto out;
>> +	}
>> +
>> +	if (c2c_he->regionset & BIT(PERF_MEM_REGION_MMIO)) {
>> +		len = scnprintf(buf, sizeof(buf), "MMIO");
>> +		if (c2c_he->regionset & PERF_MEM_REGION_MASK)
>> +			len += scnprintf(buf + len, sizeof(buf) - len, ",");
>> +	}
>> +
>> +	if (c2c_he->regionset & PERF_MEM_REGION_MASK) {
>> +		unsigned long regionset =
>> +			c2c_he->regionset >> PERF_MEM_REGION_MEM0;
>> +		int num = hweight_long(regionset);
>> +
>> +		len += scnprintf(buf + len, sizeof(buf) - len, "Mem-");
>> +		if (num > 1)
>> +			len += scnprintf(buf + len, sizeof(buf) - len, "{");
>> +		len += bitmap_scnprintf(&regionset, PERF_MEM_REGION_NUM,
>> +					buf + len, sizeof(buf) - len);
>> +		if (num > 1)
>> +			len += scnprintf(buf + len, sizeof(buf) - len, "}");
>> +	}
>> +
>> +out:
>> +	if (len > dim_dcacheline_mem_region.width)
>> +		dim_dcacheline_mem_region.width = len;\
>> +	c2c_he->regionstr = strdup(buf);
> [Severity: Low]
> Does c2c_he_free() leak c2c_he->regionstr?  c2c_he_free() frees
> c2c_he->nodestr and the other heap fields with zfree(), but does not free
> c2c_he->regionstr.

Yes, it's missed, would be added in the next version.


>
>> +
>> +	return c2c_he->regionstr ? 0 : -ENOMEM;
>> +}
>> +
>>  static void calc_width(struct c2c_hist_entry *c2c_he)
>>  {
>>  	struct c2c_hists *c2c_hists;
>> @@ -2271,6 +2292,7 @@ static void calc_width(struct c2c_hist_entry *c2c_he)
>>  	c2c_hists = container_of(c2c_he->he.hists, struct c2c_hists, hists);
>>  	hists__calc_col_len(&c2c_hists->hists, &c2c_he->he);
>>  	set_nodestr(c2c_he);
>> +	set_mem_region_str(c2c_he);
>>  }
> [Severity: Low]
> Should set_mem_region_str() only run for top-level cacheline entries (or
> when c2c_he->hists is non-NULL)?
>
> calc_width() is also called from filter_cb() for per-cacheline child
> entries, which do not display the dcacheline_mem_region column.  Running
> set_mem_region_str() on child entries allocates an unused regionstr for
> every child entry (including filtered ones) and can inflate
> dim_dcacheline_mem_region.width when a child's non-contiguous subset of
> regions (such as "Mem-{0,2,4}") formats wider than the parent cacheline's
> contiguous range (such as "Mem-{0-4}").

although currently the region column is only supported on the top-level
cacheline entries, but it would be extended the per-cacheline child view.
So let's keep what it is now.


>