perf stat: Expand metric+unit buffer size

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

Posted by Ian Rogers 2 weeks, 4 days ago

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];
 
 			if (metric_threshold &&
 			    expr__parse(&threshold, pctx, metric_threshold) == 0 &&
-- 
2.47.0.199.ga7371fff76-goog

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

Posted by Namhyung Kim 2 weeks, 1 day ago

On Tue, 05 Nov 2024 16:48:18 -0800, 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.
> 
> 

Applied to perf-tools-next, thanks!

Best regards,
Namhyung

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

Posted by Liang, Kan 2 weeks, 3 days ago


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?

Thanks,
Kan
>  
>  			if (metric_threshold &&
>  			    expr__parse(&threshold, pctx, metric_threshold) == 0 &&

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

Posted by Ian Rogers 2 weeks, 3 days ago

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

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

Posted by Liang, Kan 2 weeks, 3 days ago


On 2024-11-06 11:04 a.m., Ian Rogers wrote:
> 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.

OK

> 
> 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.
> 

Somehow I missed your last reply. I will take a look.

Thanks,
Kan

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

Posted by Namhyung Kim 2 weeks, 2 days ago

Hi Kan,

On Wed, Nov 06, 2024 at 11:23:15AM -0500, Liang, Kan wrote:
> 
> 
> On 2024-11-06 11:04 a.m., Ian Rogers wrote:
> > 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.
> 
> OK

Can I assume it as Acked-by?

Thanks,
Namhyung

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

Posted by Liang, Kan 2 weeks, 2 days ago


On 2024-11-07 2:07 p.m., Namhyung Kim wrote:
> Hi Kan,
> 
> On Wed, Nov 06, 2024 at 11:23:15AM -0500, Liang, Kan wrote:
>>
>>
>> On 2024-11-06 11:04 a.m., Ian Rogers wrote:
>>> 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.
>>
>> OK
> 
> Can I assume it as Acked-by?
> 

Yes.

Acked-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan
> Thanks,
> Namhyung
>