Re: [PATCH v3 05/14] perf c2c: add column entry functions for function view

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Cycles percentage estimation incorrectly calculates peer-snoop cycles by grouping them with generic loads.
--

--- Patch [5]: [PATCH v3 05/14] perf c2c: add column entry functions for function view ---
commit 4bb0adbd5755e4c2cab2297cfb8207a0361395f1
Author: Jiebin Sun <jiebin.sun@intel.com>

perf c2c: add column entry functions for function view

This commit adds rendering functions for each column in the perf c2c function
view. This includes entry functions for total stores, cacheline symbols,
instruction addresses, symbol names, and estimated load-cycle percentages.

> diff --git a/tools/perf/ui/browsers/c2c-function.c b/tools/perf/ui/browsers/c2c-function.c
> index ace7e3f27b55c..a9add0ba4cdaf 100644
> --- a/tools/perf/ui/browsers/c2c-function.c
> +++ b/tools/perf/ui/browsers/c2c-function.c

[ ... ]

> +/*
> + * cycles_percent_entry - Render cycles percentage column
> + */
> +static __maybe_unused int
> +cycles_percent_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
> +		     struct hist_entry *he)
> +{
> +	struct c2c_hist_entry *c2c_he;
> +	int width = c2c_width(fmt, hpp, he->hists);
> +	u64 fn_cycles, total_cycles;
> +	char folded_sign;
> +	double pct;
> +	int ret, pct_width;
> +
> +	/* Hide Cycles Percent for child functions and cachelines. */
> +	if (he->parent_he)
> +		return scnprintf(hpp->buf, hpp->size, "%*s", width, "");
> +
> +	c2c_he = container_of(he, struct c2c_hist_entry, he);
> +	fn_cycles = c2c_hist_entry__cycles(c2c_he);

[Severity: Medium]
The commit message mentions this function renders an estimated load-cycle
percentage for "(HITM, peer-snoop and other-load cycles)". However, looking
at the implementation of c2c_hist_entry__cycles():

static __maybe_unused u64 c2c_hist_entry__cycles(struct c2c_hist_entry *c2c_he)
{
	double cycles_rmt, cycles_lcl, cycles_load;
	u64 other_load, total_hitm;

	cycles_rmt = avg_stats(&c2c_he->cstats.rmt_hitm) * c2c_he->stats.rmt_hitm;
	cycles_lcl = avg_stats(&c2c_he->cstats.lcl_hitm) * c2c_he->stats.lcl_hitm;
	total_hitm = c2c_he->stats.tot_hitm;
	other_load = (c2c_he->stats.load >= total_hitm) ? c2c_he->stats.load - total_hitm : 0;
	cycles_load = avg_stats(&c2c_he->cstats.load) * other_load;

	return (u64)(cycles_rmt + cycles_lcl + cycles_load);
}

This estimation logic ignores cstats.rmt_peer and cstats.lcl_peer latencies.

Because total_peer is not subtracted alongside total_hitm, do the peer-snoop
counts remain in other_load and get incorrectly multiplied by the non-snoop
generic load average (cstats.load) rather than their actual tracked
peer-snoop latencies?

> +	/* Populated by build_function_view_hierarchy() once the L1 tree is built. */
> +	total_cycles = c2c_ext.total_cycles;
> +	pct = total_cycles > 0 ? (double)fn_cycles / total_cycles * 100.0 : 0.0;

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