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

[sashiko-bot@kernel.org]

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.

> +
> +	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}").

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106073018.903126-1-dapeng1.mi@linux.intel.com?part=2