[Patch v2 0/5] perf: fix test error and improve c2c memory-region display

[Patch v2 0/5] perf: fix test error and improve c2c memory-region display

[Dapeng Mi]

This patch series fixes a failing object-code reading test and improves
the c2c memory-region display in perf tools. In particular, it adds
memory-region information to the per-cacheline view and ensures that all
sampled memory regions are displayed, rather than only the last one.
These changes improve correctness and make the c2c output more
informative for debugging and analysis.

History:
v1: https://lore.kernel.org/all/20261106073018.903126-1-dapeng1.mi@linux.intel.com/

Dapeng Mi (5):
  perf tests: Skip zero sample IP in object-code reading test
  perf c2c: Fix top-level histogram ownership check
  perf c2c: Show memory region in the per-cacheline view
  perf c2c: Show all memory regions in the region column
  perf c2c: Rename "Region" column to "Range" for consistency

 tools/perf/builtin-c2c.c        | 139 ++++++++++++++++++++++----------
 tools/perf/tests/code-reading.c |   5 ++
 tools/perf/util/c2c.h           |   3 +-
 3 files changed, 105 insertions(+), 42 deletions(-)


base-commit: 73adad359f45f67e81f71c016f49abd7dbf57405
-- 
2.34.1

[Patch v2 1/5] perf tests: Skip zero sample IP in object-code reading test

[Dapeng Mi]

The kernel may clear the sample IP to 0 when a user-space address is
expected but a kernel address is sampled due to hardware skid. A zero
sample IP causes the object-code reading test to fail incorrectly.

Skip zero sample IP values in the object-code reading test to avoid
false-positive failures.

Reported-by: kernel test robot <yi1.lai@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202609151503.cb02f753-lkp@intel.com
Fixes: 166f10836a65 ("perf/core: Fix kernel register info leak via hardware skid")
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/tests/code-reading.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
index 130cd6021f93..7b80c4ab6916 100644
--- a/tools/perf/tests/code-reading.c
+++ b/tools/perf/tests/code-reading.c
@@ -546,6 +546,11 @@ static int process_sample_event(struct machine *machine, struct evlist *evlist,
 		goto out;
 	}
 
+	if (!sample.ip) {
+		pr_debug("kernel may clear IP to 0 in invalid cases, skip it\n");
+		goto out;
+	}
+
 	thread = machine__findnew_thread(machine, sample.pid, sample.tid);
 	if (!thread) {
 		pr_debug("machine__findnew_thread failed\n");
-- 
2.34.1

[Patch v2 2/5] perf c2c: Fix top-level histogram ownership check

[Dapeng Mi]

The function set_node_width() incorrectly determined whether a c2c
histogram entry belonged to the top-level cacheline view by checking
if &c2c.hists equals c2c_he->hists.

This comparison is always false for top-level entries because
c2c_he->hists is only assigned for lazily-created sub-histograms in
he__get_c2c_hists(). Top-level entries are instead stored under
he->hists, which points to the embedded struct hists inside c2c.hists.

Add a dedicated helper, c2c_he__is_main_hist(), to check whether a
c2c_hist_entry belongs to the top-level histogram tree and use it to
update the node width accordingly for top-level cachelines view or
per-cacheline view.

Fixes: 7f834c2e84bb ("perf c2c report: Display node for cacheline address")
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/builtin-c2c.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index f51fa23f3bf1..a0c9afddbe98 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2233,11 +2233,19 @@ static inline bool is_valid_hist_entry(struct hist_entry *he)
 	return has_record;
 }
 
+static inline bool c2c_he__is_main_hist(struct c2c_hist_entry *c2c_he)
+{
+	struct hists *hists = c2c_he->he.hists;
+
+	return hists &&
+	       container_of(hists, struct c2c_hists, hists) == &c2c.hists;
+}
+
 static void set_node_width(struct c2c_hist_entry *c2c_he, int len)
 {
 	struct c2c_dimension *dim;
 
-	dim = &c2c.hists == c2c_he->hists ?
+	dim = c2c_he__is_main_hist(c2c_he) ?
 	      &dim_dcacheline_node : &dim_offset_node;
 
 	if (len > dim->width)
-- 
2.34.1

[Patch v2 3/5] perf c2c: Show memory region in the per-cacheline view

[Dapeng Mi]

The memory-region column is currently displayed only in the top-level
cacheline view; it is missing from the child per-cacheline view.

Show the memory region in the per-cacheline view as well so the display
is complete and consistent.

For example, the per-cacheline view now shows entries such as:

Cacheline 0xff11002f7ef47a00
RmtHitm  LclHitm   L1 Hit  L1 Miss      N/A    Off  Region  Node  PA cnt
100.00%    0.00%    0.00%    0.00%    0.00%   0x10   Mem-0     0       1

Also move the c2c.show_mem_region assignment before setup_coalesce() so
the per-cacheline header is generated with the correct region layout.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/builtin-c2c.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index a0c9afddbe98..8ed4a4704d2a 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -1452,6 +1452,14 @@ static struct c2c_dimension dim_offset_node = {
 	.width		= 4,
 };
 
+static struct c2c_dimension dim_offset_mem_region = {
+	.header		= HEADER_LOW("Region"),
+	.name		= "offset_mem_region",
+	.cmp		= empty_cmp,
+	.entry		= dcacheline_node_mem_region,
+	.width		= 6,
+};
+
 static struct c2c_dimension dim_iaddr = {
 	.header		= HEADER_LOW("Code address"),
 	.name		= "iaddr",
@@ -1861,6 +1869,7 @@ static struct c2c_dimension *dimensions[] = {
 	&dim_dcacheline_node,
 	&dim_dcacheline_count,
 	&dim_offset,
+	&dim_offset_mem_region,
 	&dim_offset_node,
 	&dim_iaddr,
 	&dim_tot_hitm,
@@ -3013,13 +3022,24 @@ static int ui_quirks(void)
 		nodestr = chk_double_cl ? "Double-CL" : "CL";
 	}
 
+	/*
+	 * With the extra "Region" column, the zero line of the
+	 * dcacheline/offset header must span one more column.  Set
+	 * this after the TUI header assignment above, which would
+	 * otherwise overwrite the span with the default value.
+	 */
+	if (c2c.show_mem_region) {
+		dim_dcacheline.header.line[0].span = 3;
+		dim_offset.header.line[0].span = 3;
+	}
+
 	dim_percent_costly_snoop.header = percent_costly_snoop_header[c2c.display];
 
 	/* Fix the zero line for dcacheline column. */
 	buf = fill_line(chk_double_cl ? "Double-Cacheline" : "Cacheline",
 				dim_dcacheline.width +
 				(c2c.show_mem_region ?
-					dim_dcacheline_mem_region.width : 0) +
+				 dim_dcacheline_mem_region.width : 0) +
 				dim_dcacheline_node.width +
 				dim_dcacheline_count.width +
 				(c2c.show_mem_region ? 6 : 4));
@@ -3030,8 +3050,11 @@ static int ui_quirks(void)
 
 	/* Fix the zero line for offset column. */
 	buf = fill_line(nodestr, dim_offset.width +
+				 (c2c.show_mem_region ?
+				  dim_offset_mem_region.width : 0) +
 			         dim_offset_node.width +
-				 dim_dcacheline_count.width + 4);
+				 dim_dcacheline_count.width +
+				 (c2c.show_mem_region ? 6 : 4));
 	if (!buf)
 		return -ENOMEM;
 
@@ -3158,7 +3181,7 @@ static int build_cl_output(char *cl_sort, bool no_source)
 	}
 
 	if (asprintf(&c2c.cl_output,
-		"%s%s%s%s%s%s%s%s%s%s%s%s",
+		"%s%s%s%s%s%s%s%s%s%s%s%s%s",
 		c2c.use_stdio ? "cl_num_empty," : "",
 		c2c.display == DISPLAY_SNP_PEER ? "percent_rmt_peer,"
 						  "percent_lcl_peer," :
@@ -3166,7 +3189,9 @@ static int build_cl_output(char *cl_sort, bool no_source)
 						  "percent_lcl_hitm,",
 		"percent_stores_l1hit,"
 		"percent_stores_l1miss,"
-		"percent_stores_na,"
+		"percent_stores_na,",
+		c2c.show_mem_region ?
+		"offset,offset_mem_region,offset_node,dcacheline_count," :
 		"offset,offset_node,dcacheline_count,",
 		add_pid   ? "pid," : "",
 		add_tid   ? "tid," : "",
@@ -3356,6 +3381,8 @@ static int perf_c2c__report(int argc, const char **argv)
 		goto out;
 	}
 	env = perf_session__env(session);
+	c2c.show_mem_region = perf_header__has_feat(&session->header,
+						HEADER_MEMORY_RANGES);
 	/*
 	 * Use the 'tot' as default display type if user doesn't specify it;
 	 * since Arm64 platform doesn't support HITMs flag, use 'peer' as the
@@ -3454,11 +3481,6 @@ static int perf_c2c__report(int argc, const char **argv)
 		goto out_mem2node;
 	}
 
-	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,"
-- 
2.34.1

[Patch v2 4/5] perf c2c: Show all memory regions in the region column

[Dapeng Mi]

The c2c memory-region column currently records only the last memory
region seen for an address. However, a single address can map to
multiple memory regions over the sampling window, so reporting only the
final one loses useful information.

Update the memory-region tracking to accumulate all sampled regions for
the address and display them together in the region column.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/builtin-c2c.c | 85 ++++++++++++++++++++++++++--------------
 tools/perf/util/c2c.h    |  3 +-
 2 files changed, 58 insertions(+), 30 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 8ed4a4704d2a..a2dcc65d6b3c 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -164,6 +164,7 @@ static void c2c_he_free(void *he)
 	zfree(&c2c_he->nodeset);
 	zfree(&c2c_he->nodestr);
 	zfree(&c2c_he->node_stats);
+	zfree(&c2c_he->regionstr);
 	free(c2c_he);
 }
 
@@ -260,7 +261,7 @@ static void c2c_he__set_mem_region(struct c2c_hist_entry *c2c_he,
 
 	/* Update mem_region only if it really accesses memory */
 	if (mem_region >= PERF_MEM_REGION_MMIO)
-		c2c_he->mem_region = mem_region;
+		c2c_he->regionset |= BIT(mem_region);
 }
 
 static void compute_stats(struct c2c_hist_entry *c2c_he,
@@ -565,37 +566,12 @@ dcacheline_node_mem_region(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
 {
 	int width = c2c_width(fmt, hpp, he->hists);
 	struct c2c_hist_entry *c2c_he;
-	unsigned int mem_region;
-	char buf[20];
 
 	c2c_he = container_of(he, struct c2c_hist_entry, he);
-	mem_region = c2c_he->mem_region;
-
-	switch (mem_region) {
-	case PERF_MEM_REGION_NA:
-	case PERF_MEM_REGION_RSVD:
-		scnprintf(buf, sizeof(buf), "N/A");
-		break;
-	case PERF_MEM_REGION_MMIO:
-		scnprintf(buf, sizeof(buf), "MMIO");
-		break;
-	case PERF_MEM_REGION_MEM0:
-	case PERF_MEM_REGION_MEM1:
-	case PERF_MEM_REGION_MEM2:
-	case PERF_MEM_REGION_MEM3:
-	case PERF_MEM_REGION_MEM4:
-	case PERF_MEM_REGION_MEM5:
-	case PERF_MEM_REGION_MEM6:
-	case PERF_MEM_REGION_MEM7:
-		scnprintf(buf, sizeof(buf), "Mem-%d",
-			  mem_region - PERF_MEM_REGION_MEM0);
-		break;
-	default:
-		scnprintf(buf, sizeof(buf), "N/A");
-		break;
-	}
+	if (WARN_ON_ONCE(!c2c_he->regionstr))
+		return 0;
 
-	return scnprintf(hpp->buf, hpp->size, "%*s", width, buf);
+	return scnprintf(hpp->buf, hpp->size, "%*s", width, c2c_he->regionstr);
 }
 
 static int offset_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
@@ -2281,6 +2257,56 @@ 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 void set_mem_region_width(struct c2c_hist_entry *c2c_he, int len)
+{
+	struct c2c_dimension *dim;
+
+	dim = c2c_he__is_main_hist(c2c_he) ?
+	      &dim_dcacheline_mem_region : &dim_offset_mem_region;
+
+	if (len > dim->width)
+		dim->width = len;
+}
+
+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;
+
+		len += bitmap_scnprintf(&regionset, PERF_MEM_REGION_NUM,
+					buf + len, sizeof(buf) - len);
+	}
+
+out:
+	set_mem_region_width(c2c_he, len);
+	c2c_he->regionstr = strdup(buf);
+
+	return c2c_he->regionstr ? 0 : -ENOMEM;
+}
+
 static void calc_width(struct c2c_hist_entry *c2c_he)
 {
 	struct c2c_hists *c2c_hists;
@@ -2288,6 +2314,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);
 }
 
 static int filter_cb(struct hist_entry *he, void *arg __maybe_unused)
diff --git a/tools/perf/util/c2c.h b/tools/perf/util/c2c.h
index 65117108ddb1..e09edfa04710 100644
--- a/tools/perf/util/c2c.h
+++ b/tools/perf/util/c2c.h
@@ -33,7 +33,7 @@ struct c2c_hist_entry {
 	unsigned long		*nodeset;
 	struct c2c_stats	*node_stats;
 	unsigned int		 cacheline_idx;
-	unsigned int		 mem_region;
+	unsigned long		 regionset;
 
 	struct compute_stats	 cstats;
 
@@ -41,6 +41,7 @@ struct c2c_hist_entry {
 	unsigned long		 paddr_cnt;
 	bool			 paddr_zero;
 	char			*nodestr;
+	char			*regionstr;
 
 	/*
 	 * must be at the end,
-- 
2.34.1

[Patch v2 5/5] perf c2c: Rename "Region" column to "Range" for consistency

[Dapeng Mi]

Currently the "Memory Ranges" header uses the word "range" to indicate
a memory region, however the top-level cachelines and per-cacheline pages
label the memory-region column as "Region". This naming mismatch could
confuse the users.

Rename the column label to "Range" to match the existing "Memory Ranges"
terminology and keep the output clearer for users.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/builtin-c2c.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index a2dcc65d6b3c..94fca3e4fc0a 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -1395,11 +1395,11 @@ static struct c2c_dimension dim_dcacheline_node = {
 };
 
 static struct c2c_dimension dim_dcacheline_mem_region = {
-	.header		= HEADER_LOW("Region"),
+	.header		= HEADER_LOW("Range"),
 	.name		= "dcacheline_mem_region",
 	.cmp		= empty_cmp,
 	.entry		= dcacheline_node_mem_region,
-	.width		= 6,
+	.width		= 5,
 };
 
 static struct c2c_dimension dim_dcacheline_count = {
@@ -1429,11 +1429,11 @@ static struct c2c_dimension dim_offset_node = {
 };
 
 static struct c2c_dimension dim_offset_mem_region = {
-	.header		= HEADER_LOW("Region"),
+	.header		= HEADER_LOW("Range"),
 	.name		= "offset_mem_region",
 	.cmp		= empty_cmp,
 	.entry		= dcacheline_node_mem_region,
-	.width		= 6,
+	.width		= 5,
 };
 
 static struct c2c_dimension dim_iaddr = {
-- 
2.34.1