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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Incorrect cycle estimation omits peer snoop cycles and misattributes them to generic loads.
- [Medium] Performance degradation via O(N) tree walk instead of utilizing the O(1) cached `child_stores` field.
- [Medium] Potential NULL pointer dereference in `symbol_width()` due to uninitialized `dim_symbol_view.se`.
--

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

perf c2c: add column format infrastructure for function view

This patch adds the column format plumbing functions used by all
function view dimensions. It introduces helper functions to calculate
column widths and render multi-line column headers.

> diff --git a/tools/perf/ui/browsers/c2c-function.c b/tools/perf/ui/browsers/c2c-function.c
> index dbc912a4a242b..d718cab6537d9 100644
> --- a/tools/perf/ui/browsers/c2c-function.c
> +++ b/tools/perf/ui/browsers/c2c-function.c
> @@ -66,6 +66,123 @@ static __maybe_unused inline u64 hist_entry__iaddr(struct hist_entry *he)

[ ... ]

> +static __maybe_unused int symbol_width(struct hists *hists, struct sort_entry *se)
> +{
> +	int width = hists__col_len(hists, se->se_width_idx);

[Severity: Medium]
Can this dereference a null pointer?

When c2c_width() is called for dim_symbol_view:

c2c_width() {
    ...
    if (dim == &dim_symbol_view)
        return symbol_width(hists, dim->se);
}

The statically declared dim_symbol_view is uninitialized, so its se member
is null. Passing it here will cause an immediate pointer dereference
when accessing se->se_width_idx.

> +
> +	if (!c2c.symbol_full && width > SYMBOL_WIDTH)
> +		width = SYMBOL_WIDTH;
> +
> +	return width;
> +}

[ ... ]

> +/*
> + * 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]
Is this cycle calculation omitting the peer snoop cycles?

The comment above this function states it includes rmt_peer and lcl_peer,
but the logic drops their averages. By only subtracting total_hitm from
stats.load, it leaves the peer snoop counts (tot_peer) in the other_load
remainder.

This appears to weight the peer counts with the generic cstats.load average
instead of multiplying rmt_peer and lcl_peer counts by their respective
latencies.

> +}

[ ... ]

> +/* Sum child entries' store counts under a level-1 hist_entry. */
> +static __maybe_unused u64 hist_entry__child_stores(struct hist_entry *he)
> +{
> +	struct rb_node *nd;
> +	u64 sum = 0;
> +
> +	for (nd = rb_first_cached(&he->hroot_out); nd; nd = rb_next(nd)) {
> +		struct hist_entry *child = rb_entry(nd, struct hist_entry, rb_node);
> +		struct c2c_hist_entry *c2c_child =
> +			container_of(child, struct c2c_hist_entry, he);
> +
> +		sum += (u64)c2c_child->stats.store;
> +	}
> +	return sum;

[Severity: Medium]
Could this use the cached child_stores field instead of walking the tree?

This loop introduces a tree walk to sum the store counts on every
render cycle. The c2c_hist_entry structure has a child_stores field that
could provide this count in constant time, avoiding the need to iterate
over all children in hroot_out.

> +}

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