Re: [PATCH 2/4] perf tests c2c: Report skip when the workload fails

Re: [PATCH 2/4] perf tests c2c: Report skip when the workload fails

[sashiko-bot]

> The workload path leaves err at 0 and returns, so the test exits 0 and is
> reported as a pass even though nothing was recorded and neither report
> invocation ran. The check_c2c_support() path above it already sets err=2.
> 
> Set err=2 there as well so a failing workload is reported as a skip.
> 
> Fixes: ac881007c4bf ("perf tests c2c: Add a basic c2c")
> Signed-off-by: Jiebin Sun <jiebin.sun@intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787283281.git.jiebin.sun@intel.com?part=2

[PATCH 0/4] perf c2c: Add function-view stdio support and coverage

[Jiebin Sun]

The recently merged function-view series added an interactive TUI for
examining the c2c data as:

  read-side function -> contending writer -> shared cacheline

This follow-up makes the same view available to builds without SLANG and
to pipes and scripts. `perf c2c report --function` now implies stdio and
prints the three-level hierarchy fully expanded. It requires `iaddr` in
the coalescing fields and is rejected together with `--stats`.

The first two patches fix adjacent pre-existing issues: the documented
default coalesce fields are stale, and a workload recording failure in
the existing c2c shell test is incorrectly reported as a pass. The third
patch adds the stdio function view, and the final patch adds automated
coverage using a separate contended futex workload. Systems that record
no contended samples report a skip for the hierarchy checks rather than a
spurious failure.

This series follows the function-view work merged from:

  https://lore.kernel.org/linux-perf-users/20260817094623.3288456-1-jiebin.sun@intel.com/

The series is based on perf-tools-next commit 9453bc6a69ef ("perf vendor
events arm64: Fix Tegra410 Olympus event 0x0197").

Tested with:

  - SLANG and NO_SLANG=1 builds
  - perf test c2c
  - perf test 'import perf'
  - checkpatch.pl --strict on each patch
  - unresolved-symbol checks on the Python perf module

Jiebin Sun (4):
  perf c2c: Fix documented default coalesce fields
  perf tests c2c: Report skip when the workload fails
  perf c2c: Add stdio support for the function view
  perf tests c2c: Add function view stdio coverage

 tools/perf/Documentation/perf-c2c.txt |  14 +++-
 tools/perf/builtin-c2c.c              |  85 +++++++++++++++++--
 tools/perf/tests/shell/c2c.sh         | 114 ++++++++++++++++++++++++++
 tools/perf/util/c2c-function.c        |   9 +-
 tools/perf/util/c2c.h                 |   1 +
 5 files changed, 205 insertions(+), 18 deletions(-)


base-commit: 9453bc6a69ef43755f1c28d5688cacdd69fa16bd
-- 
2.52.0

[PATCH 1/4] perf c2c: Fix documented default coalesce fields

[Jiebin Sun]

The default coalesce fields dropped pid in favor of iaddr, but the man
page still documents the old pid,iaddr default. Update it to match the
command.

Fixes: 423701a0c8d7 ("perf c2c: Change the default coalesce setup")
Signed-off-by: Jiebin Sun <jiebin.sun@intel.com>
Cc: Dapeng Mi <dapeng1.mi@linux.intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: James Clark <james.clark@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Thomas Falcon <thomas.falcon@intel.com>
Reviewed-by: Tianyou Li <tianyou.li@intel.com>
Reviewed-by: Wangyang Guo <wangyang.guo@intel.com>
---
 tools/perf/Documentation/perf-c2c.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/Documentation/perf-c2c.txt b/tools/perf/Documentation/perf-c2c.txt
index 8775889bc0a3..9e58a51c55de 100644
--- a/tools/perf/Documentation/perf-c2c.txt
+++ b/tools/perf/Documentation/perf-c2c.txt
@@ -341,7 +341,7 @@ output fields set for cacheline offsets output:
              Code address, Code symbol, Shared Object, Source line
   dso   - coalesced by shared object
 
-By default the coalescing is setup with 'pid,iaddr'.
+By default, coalescing uses `iaddr`.
 
 STDIO OUTPUT
 ------------
-- 
2.52.0

[PATCH 2/4] perf tests c2c: Report skip when the workload fails

[Jiebin Sun]

The workload path leaves err at 0 and returns, so the test exits 0 and is
reported as a pass even though nothing was recorded and neither report
invocation ran. The check_c2c_support() path above it already sets err=2.

Set err=2 there as well so a failing workload is reported as a skip.

Fixes: ac881007c4bf ("perf tests c2c: Add a basic c2c")
Signed-off-by: Jiebin Sun <jiebin.sun@intel.com>
Cc: Dapeng Mi <dapeng1.mi@linux.intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: James Clark <james.clark@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Thomas Falcon <thomas.falcon@intel.com>
Reviewed-by: Tianyou Li <tianyou.li@intel.com>
Reviewed-by: Wangyang Guo <wangyang.guo@intel.com>
---
 tools/perf/tests/shell/c2c.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/tests/shell/c2c.sh b/tools/perf/tests/shell/c2c.sh
index 2471d44595c3..f5f223cbf9cc 100755
--- a/tools/perf/tests/shell/c2c.sh
+++ b/tools/perf/tests/shell/c2c.sh
@@ -39,6 +39,7 @@ test_c2c_record_report() {
 	# Run a workload that does some memory operations.
 	if ! perf c2c record -o "${perfdata}" -- perf test -w datasym 1 > /dev/null 2>&1 ; then
 		echo "c2c record and report test [Skipped: perf c2c record failed during workload]"
+		err=2
 		return
 	fi
 
-- 
2.52.0

[PATCH 3/4] perf c2c: Add stdio support for the function view

[Jiebin Sun]

The function view is currently TUI-only, so it cannot be used by builds
without SLANG support, when output is piped, or from a script. Add a
--function option that prints the fully expanded three-level hierarchy to
stdout.

Keep the stdio renderer in builtin-c2c.c and reuse the common function-view
model introduced by the merged series. Export only the coalescing-field
capability check from the model, preserving the util/UI boundary and
leaving the TUI object in libperf-ui.a.

Stop padding the final identity column in symbol_view_entry(). The generic
formatter pads non-final columns but deliberately leaves the final column
unpadded, avoiding trailing whitespace in function-view table rows. The TUI
remains unchanged because its browser clears the rest of each rendered row.

--function implies --stdio and is rejected together with --stats. Validate
the iaddr requirement before processing events. Return function-view build
failures from the report command, and preserve TUI browser errors when
converting the display helpers to return a status.

Signed-off-by: Jiebin Sun <jiebin.sun@intel.com>
Cc: Dapeng Mi <dapeng1.mi@linux.intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: James Clark <james.clark@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Thomas Falcon <thomas.falcon@intel.com>
Reviewed-by: Tianyou Li <tianyou.li@intel.com>
Reviewed-by: Wangyang Guo <wangyang.guo@intel.com>
---
 tools/perf/Documentation/perf-c2c.txt | 12 +++-
 tools/perf/builtin-c2c.c              | 85 ++++++++++++++++++++++++---
 tools/perf/util/c2c-function.c        |  9 +--
 tools/perf/util/c2c.h                 |  1 +
 4 files changed, 90 insertions(+), 17 deletions(-)

diff --git a/tools/perf/Documentation/perf-c2c.txt b/tools/perf/Documentation/perf-c2c.txt
index 9e58a51c55de..7a0cf31be7ed 100644
--- a/tools/perf/Documentation/perf-c2c.txt
+++ b/tools/perf/Documentation/perf-c2c.txt
@@ -107,6 +107,10 @@ REPORT OPTIONS
 --stats::
 	Display only statistic tables and force stdio mode.
 
+--function::
+	Display the function view and force stdio mode.  This requires `iaddr`
+	in the cacheline coalescing fields and cannot be used with `--stats`.
+
 --full-symbols::
 	Display full length of symbols.
 
@@ -360,6 +364,10 @@ Following tables are displayed:
   Shared Cache Line Distribution Pareto
   - list of all accessed offsets for each cacheline
 
+With `--function`, the cacheline and Pareto tables are replaced by a fully
+expanded Shared Data Functions Table.  Its three levels are the read-side
+function, contending writer, and shared cacheline, as detailed below.
+
 TUI OUTPUT
 ----------
 The TUI output provides interactive interface to navigate
@@ -374,8 +382,8 @@ Verbose mode also includes code addresses in function rows, and code addresses
 remain available in the per-cacheline detail view ('d').
 
 The function view requires `iaddr` in the cacheline coalescing fields.  If
-`--coalesce` omits it, TAB reports that the view is unavailable rather than
-attributing already-coalesced samples to an arbitrary function.
+`--coalesce` omits it, TAB or `--function` reports that the view is unavailable
+rather than attributing already-coalesced samples to an arbitrary function.
 
   Level 1: the read-side function, sorted by Cycles % (estimated load
            cycles: HITM, peer-snoop and other-load cycles)
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 715b75d42f2a..cce76a1e2ff5 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -73,6 +73,7 @@ struct perf_c2c {
 	bool			 show_all;
 	bool			 use_stdio;
 	bool			 stats_only;
+	bool			 function_view;
 	bool			 symbol_full;
 	bool			 stitch_lbr;
 
@@ -2530,7 +2531,48 @@ static void print_c2c_info(FILE *out, struct perf_session *session)
 	fprintf(out, "  Cacheline data grouping           : %s\n", c2c.cl_sort);
 }
 
-static void perf_c2c__hists_fprintf(FILE *out, struct perf_session *session)
+static void c2c_function__unfold_all(struct rb_root_cached *root)
+{
+	struct rb_node *nd;
+
+	for (nd = rb_first_cached(root); nd; nd = rb_next(nd)) {
+		struct hist_entry *he = rb_entry(nd, struct hist_entry, rb_node);
+
+		if (!he->has_children)
+			continue;
+		he->unfolded = true;
+		c2c_function__unfold_all(&he->hroot_out);
+	}
+}
+
+static int perf_c2c__function_fprintf(FILE *out)
+{
+	bool saved_use_callchain = symbol_conf.use_callchain;
+	struct hists *hists;
+	int ret;
+
+	/* Function-view entries aggregate samples and never display callchains. */
+	symbol_conf.use_callchain = false;
+	ret = c2c_function__build(&c2c.hists, c2c.cl_sort, c2c.symbol_full,
+				  &hists);
+	if (ret) {
+		if (ret == -EOPNOTSUPP)
+			pr_err("The function view requires iaddr in --coalesce.\n");
+		else
+			pr_err("Failed to build function view hierarchy (ret=%d)\n", ret);
+		goto out;
+	}
+
+	/* Match fold signs to hists__fprintf()'s forced child traversal. */
+	c2c_function__unfold_all(&hists->entries);
+	hists__fprintf(hists, true, 0, 0, 0, out, true);
+	c2c_function__reset();
+out:
+	symbol_conf.use_callchain = saved_use_callchain;
+	return ret;
+}
+
+static int perf_c2c__hists_fprintf(FILE *out, struct perf_session *session)
 {
 	setup_pager();
 
@@ -2541,7 +2583,17 @@ static void perf_c2c__hists_fprintf(FILE *out, struct perf_session *session)
 	print_c2c_info(out, session);
 
 	if (c2c.stats_only)
-		return;
+		return 0;
+
+	if (c2c.function_view) {
+		fprintf(out, "\n");
+		fprintf(out, "=================================================\n");
+		fprintf(out, "           Shared Data Functions Table\n");
+		fprintf(out, "=================================================\n");
+		fprintf(out, "#\n");
+
+		return perf_c2c__function_fprintf(out);
+	}
 
 	fprintf(out, "\n");
 	fprintf(out, "=================================================\n");
@@ -2558,6 +2610,7 @@ static void perf_c2c__hists_fprintf(FILE *out, struct perf_session *session)
 	fprintf(out, "#\n");
 
 	print_pareto(out, perf_session__env(session));
+	return 0;
 }
 
 #ifdef HAVE_SLANG_SUPPORT
@@ -2794,18 +2847,18 @@ static int perf_c2c__hists_browse(struct hists *hists)
 	return 0;
 }
 
-static void perf_c2c_display(struct perf_session *session)
+static int perf_c2c_display(struct perf_session *session)
 {
 	if (use_browser == 0)
-		perf_c2c__hists_fprintf(stdout, session);
-	else
-		perf_c2c__hists_browse(&c2c.hists.hists);
+		return perf_c2c__hists_fprintf(stdout, session);
+
+	return perf_c2c__hists_browse(&c2c.hists.hists);
 }
 #else
-static void perf_c2c_display(struct perf_session *session)
+static int perf_c2c_display(struct perf_session *session)
 {
 	use_browser = 0;
-	perf_c2c__hists_fprintf(stdout, session);
+	return perf_c2c__hists_fprintf(stdout, session);
 }
 #endif /* HAVE_SLANG_SUPPORT */
 
@@ -3081,6 +3134,8 @@ static int perf_c2c__report(int argc, const char **argv)
 	OPT_BOOLEAN(0, "stdio", &c2c.use_stdio, "Use the stdio interface"),
 	OPT_BOOLEAN(0, "stats", &c2c.stats_only,
 		    "Display only statistic tables (implies --stdio)"),
+	OPT_BOOLEAN(0, "function", &c2c.function_view,
+		    "Display the function view (implies --stdio)"),
 	OPT_BOOLEAN(0, "full-symbols", &c2c.symbol_full,
 		    "Display full length of symbols"),
 	OPT_BOOLEAN(0, "no-source", &no_source,
@@ -3119,6 +3174,11 @@ static int perf_c2c__report(int argc, const char **argv)
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	if (argc)
 		usage_with_options(report_c2c_usage, options);
+	if (c2c.stats_only && c2c.function_view) {
+		pr_err("--stats and --function cannot be used together.\n");
+		err = -EINVAL;
+		goto out;
+	}
 
 #ifndef HAVE_SLANG_SUPPORT
 	c2c.use_stdio = true;
@@ -3126,6 +3186,8 @@ static int perf_c2c__report(int argc, const char **argv)
 
 	if (c2c.stats_only)
 		c2c.use_stdio = true;
+	if (c2c.function_view)
+		c2c.use_stdio = true;
 
 	/**
 	 * Annotation related options disassembler_style, objdump_path are set
@@ -3199,6 +3261,11 @@ static int perf_c2c__report(int argc, const char **argv)
 		pr_debug("Failed to initialize hists\n");
 		goto out_session;
 	}
+	if (c2c.function_view && !c2c_function__has_iaddr(c2c.cl_sort)) {
+		pr_err("The function view requires iaddr in --coalesce.\n");
+		err = -EINVAL;
+		goto out_session;
+	}
 
 	err = c2c_hists__init(&c2c.hists, "dcacheline", 2, perf_session__env(session));
 	if (err) {
@@ -3332,7 +3399,7 @@ static int perf_c2c__report(int argc, const char **argv)
 		goto out_mem2node;
 	}
 
-	perf_c2c_display(session);
+	err = perf_c2c_display(session);
 
 out_mem2node:
 	mem2node__exit(&c2c.mem2node);
diff --git a/tools/perf/util/c2c-function.c b/tools/perf/util/c2c-function.c
index 5b6a06a5a067..d410196d0429 100644
--- a/tools/perf/util/c2c-function.c
+++ b/tools/perf/util/c2c-function.c
@@ -305,8 +305,8 @@ symbol_view_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
 		char symbuf[32];
 
 		scnprintf(symbuf, sizeof(symbuf), "0x%" PRIx64, addr);
-		ret += scnprintf(hpp->buf + ret, hpp->size - ret, "%-*.*s",
-				 text_width, text_width, symbuf);
+		ret += scnprintf(hpp->buf + ret, hpp->size - ret, "%.*s",
+				 text_width, symbuf);
 	} else {
 		/* Level 1 and level 2 are both functions. */
 		size_t cell_size;
@@ -331,9 +331,6 @@ symbol_view_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
 			len = min_t(size_t, len, cell_size - 1);
 
 		ret += len;
-		if (len < text_width)
-			ret += scnprintf(hpp->buf + ret, hpp->size - ret, "%*s",
-					 text_width - len, "");
 	}
 
 	return ret;
@@ -1519,7 +1516,7 @@ void c2c_function__reset(void)
 	symbol_conf.use_callchain = saved_use_callchain;
 }
 
-static bool c2c_function__has_iaddr(const char *cl_sort)
+bool c2c_function__has_iaddr(const char *cl_sort)
 {
 	const char *field = cl_sort;
 
diff --git a/tools/perf/util/c2c.h b/tools/perf/util/c2c.h
index 53f024e25d99..198032ec7c87 100644
--- a/tools/perf/util/c2c.h
+++ b/tools/perf/util/c2c.h
@@ -107,6 +107,7 @@ bool c2c_fmt_equal(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b);
 int c2c_function__build(struct c2c_hists *cl_hists, const char *cl_sort,
 			bool symbol_full, struct hists **hists);
 void c2c_function__reset(void);
+bool c2c_function__has_iaddr(const char *cl_sort);
 /* Valid only between a successful build and c2c_function__reset(). */
 struct hist_entry *c2c_function__find_cacheline(struct hist_entry *he);
 
-- 
2.52.0

[PATCH 4/4] perf tests c2c: Add function view stdio coverage

[Jiebin Sun]

Exercise the function view without driving a terminal now that it has
a stdio path. Keep the existing datasym record/report coverage and make a
separate recording of the contended locks used by the futex hash benchmark.

Always check the function table headers, table replacement, missing-iaddr
diagnostic, and conflicting options. A machine can support c2c recording
without capturing a contended sample, so report a skip when the hierarchy
is empty rather than treating hardware sampling variance as a failure.

When samples are available, check generic row shapes for all three
hierarchy levels, their expanded fold signs, cacheline addresses, and the
absence of trailing whitespace in the table body.

Signed-off-by: Jiebin Sun <jiebin.sun@intel.com>
Cc: Dapeng Mi <dapeng1.mi@linux.intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: James Clark <james.clark@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Thomas Falcon <thomas.falcon@intel.com>
Reviewed-by: Tianyou Li <tianyou.li@intel.com>
Reviewed-by: Wangyang Guo <wangyang.guo@intel.com>
---
 tools/perf/tests/shell/c2c.sh | 113 ++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/tools/perf/tests/shell/c2c.sh b/tools/perf/tests/shell/c2c.sh
index f5f223cbf9cc..cf76359d19cc 100755
--- a/tools/perf/tests/shell/c2c.sh
+++ b/tools/perf/tests/shell/c2c.sh
@@ -6,10 +6,15 @@ set -e
 
 err=0
 perfdata=$(mktemp /tmp/__perf_c2c_test.perf.data.XXXXX)
+funcdata=$(mktemp /tmp/__perf_c2c_function_test.perf.data.XXXXX)
+perfout=$(mktemp /tmp/__perf_c2c_test.output.XXXXX)
 
 cleanup() {
 	rm -f "${perfdata}"
 	rm -f "${perfdata}".old
+	rm -f "${funcdata}"
+	rm -f "${funcdata}".old
+	rm -f "${perfout}"
 	trap - EXIT TERM INT
 }
 
@@ -58,6 +63,114 @@ test_c2c_record_report() {
 	echo "c2c record and report test [Success]"
 }
 
+test_c2c_function_report() {
+	echo "c2c function stdio report test"
+
+	if perf c2c report -i "${perfdata}" --function -c pid > "${perfout}" 2>&1 ; then
+		echo "c2c function stdio report test [Failed: report accepted missing iaddr]"
+		err=1
+		return
+	fi
+	if ! grep -Fq "The function view requires iaddr in --coalesce." "${perfout}" ; then
+		echo "c2c function stdio report test [Failed: missing iaddr diagnostic]"
+		cat "${perfout}"
+		err=1
+		return
+	fi
+	if grep -Fq "Shared Data Functions Table" "${perfout}" ; then
+		echo "c2c function stdio report test [Failed: partial report on missing iaddr]"
+		cat "${perfout}"
+		err=1
+		return
+	fi
+
+	if perf c2c report -i "${perfdata}" --function --stats > "${perfout}" 2>&1 ; then
+		echo "c2c function stdio report test [Failed: accepted conflicting options]"
+		err=1
+		return
+	fi
+	if ! grep -Fq -- "--stats and --function cannot be used together." "${perfout}" ; then
+		echo "c2c function stdio report test [Failed: missing conflict diagnostic]"
+		cat "${perfout}"
+		err=1
+		return
+	fi
+
+	# Exercise contended futex hash-bucket locks so the function view can get
+	# reader, writer, and cacheline rows without changing the original test.
+	if ! perf c2c record -o "${funcdata}" -- \
+		perf bench futex hash -t 4 -r 1 -s > /dev/null 2>&1 ; then
+		echo "c2c function stdio report test [Skipped: recording failed]"
+		err=2
+		return
+	fi
+
+	if ! perf c2c report -i "${funcdata}" --function > "${perfout}" 2>&1 ; then
+		echo "c2c function stdio report test [Failed: report failed]"
+		cat "${perfout}"
+		err=1
+		return
+	fi
+
+	for expected in "Shared Data Functions Table" \
+		"#    Cycles    Store" \
+		"#         %    count  Function / Contending function / Cacheline" \
+		"# .........  ......." ; do
+		if ! grep -Fq "${expected}" "${perfout}" ; then
+			echo "c2c function stdio report test [Failed: missing '${expected}']"
+			cat "${perfout}"
+			err=1
+			return
+		fi
+	done
+	for unexpected in "Shared Data Cache Line Table" \
+		"Shared Cache Line Distribution Pareto" ; do
+		if grep -Fq "${unexpected}" "${perfout}" ; then
+			echo "c2c function stdio report test [Failed: found '${unexpected}']"
+			cat "${perfout}"
+			err=1
+			return
+		fi
+	done
+
+	if ! awk '/^# \.+/ { body = 1; next }
+		body && !/^#/ && NF { found = 1 }
+		END { exit !found }' "${perfout}" ; then
+		echo "c2c function stdio report test [Skipped: no contended samples]"
+		err=2
+		return
+	fi
+
+	# The spaces intentionally verify per-level indentation and expanded
+	# fold-sign placement without depending on symbol names.
+	for expected in \
+		'^  - +[0-9]+\.[0-9]+% +[0-9]+  - ' \
+		'^ +[0-9]+    - ' \
+		'^ +[0-9]+        0x[[:xdigit:]]+$' ; do
+		if ! grep -Eq "${expected}" "${perfout}" ; then
+			echo "c2c function stdio report test [Failed: missing hierarchy row]"
+			echo "  ${expected}"
+			cat "${perfout}"
+			err=1
+			return
+		fi
+	done
+
+	if awk '/^# \.+/ { body = 1; next }
+		body && /[[:blank:]]$/ { found = 1 }
+		END { exit !found }' "${perfout}" ; then
+		echo "c2c function stdio report test [Failed: trailing whitespace in table body]"
+		cat "${perfout}"
+		err=1
+		return
+	fi
+
+	echo "c2c function stdio report test [Success]"
+}
+
 test_c2c_record_report
+if [ "${err}" -eq 0 ]; then
+	test_c2c_function_report
+fi
 cleanup
 exit $err
-- 
2.52.0

Re: [PATCH 1/4] perf c2c: Fix documented default coalesce fields

[sashiko-bot]

> The default coalesce fields dropped pid in favor of iaddr, but the man
> page still documents the old pid,iaddr default. Update it to match the
> command.
> 
> Fixes: 423701a0c8d7 ("perf c2c: Change the default coalesce setup")
> Signed-off-by: Jiebin Sun <jiebin.sun@intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787283281.git.jiebin.sun@intel.com?part=1

Re: [PATCH 4/4] perf tests c2c: Add function view stdio coverage

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added `test_c2c_function_report` test fails spuriously because it incorrectly assumes `perfdata` is valid when `err` is 0. If the workload in `test_c2c_record_report` fails, it returns early but leaves `err=0`. This causes `test_c2c_function_report` to process an invalid or missing `perfdata` file, failing its diagnostic checks and setting `err=1` (test failure).
--

--- Patch [4]: [PATCH 4/4] perf tests c2c: Add function view stdio coverage ---
commit 32c977aa30c2c6186f39077056298ef3b53307dd
Author: Jiebin Sun <jiebin.sun@intel.com>

perf tests c2c: Add function view stdio coverage

Exercise the function view without driving a terminal now that it has
a stdio path. Keep the existing datasym record/report coverage and make a
separate recording of the contended locks used by the futex hash benchmark.

> diff --git a/tools/perf/tests/shell/c2c.sh b/tools/perf/tests/shell/c2c.sh
> index f5f223cbf9cc7..cf76359d19ccd 100755
> --- a/tools/perf/tests/shell/c2c.sh
> +++ b/tools/perf/tests/shell/c2c.sh
[ ... ]
> @@ -58,6 +63,114 @@ test_c2c_record_report() {
>  	echo "c2c record and report test [Success]"
>  }
>  
> +test_c2c_function_report() {
> +	echo "c2c function stdio report test"
> +
> +	if perf c2c report -i "${perfdata}" --function -c pid > "${perfout}" 2>&1 ; then
> +		echo "c2c function stdio report test [Failed: report accepted missing iaddr]"
> +		err=1
> +		return
> +	fi
[ ... ]
>  test_c2c_record_report
> +if [ "${err}" -eq 0 ]; then
> +	test_c2c_function_report
> +fi
>  cleanup
>  exit $err

[Severity: Medium]
Will this lead to a spurious test failure if the record workload fails?

In test_c2c_record_report(), if the 'perf test -w datasym 1' workload fails,
it returns early without modifying err, leaving err=0:

	if ! perf c2c record -o "${perfdata}" -- perf test -w datasym 1 > /dev/null 2>&1 ; then
		echo "c2c record and report test [Skipped: perf c2c record failed during workload]"
		return
	fi

Because err remains 0, test_c2c_function_report() will conditionally execute
and attempt to use the missing or invalid perfdata file. This will cause
test_c2c_function_report() to fail its diagnostic checks and set err=1.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787283281.git.jiebin.sun@intel.com?part=4

Re: [PATCH 3/4] perf c2c: Add stdio support for the function view

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] Memory leak: L2 and L3 function-view child entries are not freed on cleanup because `perf c2c` does not set `symbol_conf.report_hierarchy`, bypassing the recursive deletion in `hist_entry__delete()`.
--

--- Patch [3]: [PATCH 3/4] perf c2c: Add stdio support for the function view ---
commit e1f0ed49bafb9da749fbfa14e02ab8497ae9d5d4
Author: Jiebin Sun <jiebin.sun@intel.com>

perf c2c: Add stdio support for the function view

This commit adds a --function option to perf c2c to print the fully expanded
three-level hierarchy to stdout, allowing its use without SLANG support,
when piped, or in scripts. It reuses the common function-view model while
preserving the boundary between utilities and the UI.

> diff --git a/tools/perf/util/c2c-function.c b/tools/perf/util/c2c-function.c
> --- a/tools/perf/util/c2c-function.c
> +++ b/tools/perf/util/c2c-function.c

[ ... ]

> @@ -1519,7 +1516,7 @@ void c2c_function__reset(void)
>  	symbol_conf.use_callchain = saved_use_callchain;
>  }

[Severity: Medium]
This is a pre-existing issue, but does this teardown logic in
c2c_function__reset() leak the L2 and L3 function-view child entries?

When cleaning up the hierarchy, hists__delete_all_entries() relies on
hist_entry__delete() for each node. However, in tools/perf/util/hist.c,
hist_entry__delete() only recursively deletes the child entries in hroot_out
if the hierarchy flag is set:

tools/perf/util/hist.c:hist_entry__delete() {
    ...
    if (symbol_conf.report_hierarchy) {
        struct rb_root *root = &he->hroot_out.rb_root;
        struct hist_entry *child, *tmp;

        rbtree_postorder_for_each_entry_safe(child, tmp, root, rb_node)
            hist_entry__delete(child);

        *root = RB_ROOT;
    }
    ...
}

Since perf c2c does not appear to set symbol_conf.report_hierarchy, won't
these child entries be orphaned and skipped during cleanup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787283281.git.jiebin.sun@intel.com?part=3