Re: [PATCH v3 04/14] perf c2c: add column format infrastructure for function view

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] c2c_hist_entry__cycles() incorrectly calculates cycle counts by omitting peer snoop latencies and misattributing peer load counts to the latency of regular loads.
--

--- Patch [4]: [PATCH v3 04/14] perf c2c: add column format infrastructure for function view ---
commit 989667f800a57839109dc11e25255b1d88273729
Author: Jiebin Sun <jiebin.sun@intel.com>

perf c2c: add column format infrastructure for function view

This commit adds the column format plumbing functions used by all function
view dimensions. It introduces helper functions for calculating symbol widths,
dispatching column widths, and rendering multi-line column headers.

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

[ ... ]

> +/*
> + * Return the estimated total cycles for a c2c_hist_entry
> + * (rmt_hitm + lcl_hitm + rmt_peer + lcl_peer + other loads).
> + */
> +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);
> +}

[Severity: Medium]
Does this calculation correctly account for peer snoop latencies?

The function comment explicitly mentions including rmt_peer and lcl_peer,
but the implementation appears to omit cycles_rmt_peer and cycles_lcl_peer
entirely from the cycles aggregation.

Additionally, by only subtracting tot_hitm from stats.load, does this
inadvertently group peer load counts into the other_load bucket? This
would cause peer load events to be multiplied by the generic load latency
instead of their actual peer latencies, potentially skewing the performance
metrics shown in the function view.

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