Re: [PATCH v1] perf stat: Expand metric+unit buffer size

[Ian Rogers <irogers@google.com>]

On Wed, Nov 6, 2024 at 7:27 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2024-11-05 7:48 p.m., Ian Rogers wrote:
> > Long metric names combined with units may exceed the metric_bf and
> > lead to truncation. Double metric_bf in size to avoid this.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/stat-shadow.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> > index 8c9292aa61d3..6b531d4f58a3 100644
> > --- a/tools/perf/util/stat-shadow.c
> > +++ b/tools/perf/util/stat-shadow.c
> > @@ -507,7 +507,7 @@ static void generic_metric(struct perf_stat_config *config,
> >       if (!metric_events[i]) {
> >               if (expr__parse(&ratio, pctx, metric_expr) == 0) {
> >                       char *unit;
> > -                     char metric_bf[64];
> > +                     char metric_bf[128];
>
> I thin there is already a MAX_EVENT_NAME.
> Can we similarly define a MAX_METRIC_NAME for it?

So in this case the buffer needs to be big enough to hold the metric
name, the unit from the scaleunit (e.g. the "%" from "100%"). I'd
prefer we used dynamic memory allocation to having hard coded limits,
just to avoid a "640K ought to be enough for anybody," moment.
Although this change is implicitly a hard coded limit, sigh. There is
also the metric only name length:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-stat.c?h=perf-tools-next#n160
That looked like it was planned to be dynamically computed but then
the patch adding that never materialized - meaning "standard"
metric-only output has its own cut-off rules at 20 characters. I'd
rather wait on doing a larger cleanup and do this quick fix for now,
mainly as I have enough to do.

While we're talking metrics, I'm still looking for feedback on the
python generation of metrics. v1 was sent back in January, if v4 looks
okay some acknowledgement would be nice:
https://lore.kernel.org/lkml/CAP-5=fVX5wypmAAhR8LsE4nSWp5BmN_qhGf9+WCh2bebNcGYTg@mail.gmail.com/
Leo sent some feedback to improve some of the ARM metrics, so I can
resend the whole series. Would be nice to gather some acked-by or
other tags.

Thanks,
Ian