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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fV3RuvU0N_bt7R-ZMs2nX1_wfKh30PA59u3MW0TF4nzBg@mail.gmail.com>
Date: Wed, 6 Nov 2024 08:04:51 -0800
From: Ian Rogers <irogers@...gle.com>
To: "Liang, Kan" <kan.liang@...ux.intel.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] perf stat: Expand metric+unit buffer size

On Wed, Nov 6, 2024 at 7:27 AM Liang, Kan <kan.liang@...ux.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@...gle.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