[PATCH v1 0/4] Fixes/improvements to metric output

[PATCH v1 0/4] Fixes/improvements to metric output

[Ian Rogers]

A small addition to allow NaNs to be encoded in metrics and then 3
fixes to various metric related issues.

Ian Rogers (4):
  perf expr: Allow NaN to be a valid number
  perf expr: Fix "has_event" function for metric style events
  perf stat: Avoid metric-only segv
  perf metric: Don't remove scale from counts

 tools/perf/util/expr.c         | 20 +++++++++++++++++++-
 tools/perf/util/expr.l         |  9 +++++++++
 tools/perf/util/stat-display.c |  2 +-
 tools/perf/util/stat-shadow.c  |  7 +------
 4 files changed, 30 insertions(+), 8 deletions(-)

-- 
2.43.0.687.g38aa6559b0-goog

[PATCH v1 1/4] perf expr: Allow NaN to be a valid number

[Ian Rogers]

Currently only floating point numbers can be parsed, add a special
case for NaN.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/expr.l | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
index 0feef0726c48..a2fc43159ee9 100644
--- a/tools/perf/util/expr.l
+++ b/tools/perf/util/expr.l
@@ -94,6 +94,14 @@ static int literal(yyscan_t scanner, const struct expr_scanner_ctx *sctx)
 	}
 	return LITERAL;
 }
+
+static int nan_value(yyscan_t scanner)
+{
+	YYSTYPE *yylval = expr_get_lval(scanner);
+
+	yylval->num = NAN;
+	return NUMBER;
+}
 %}
 
 number		([0-9]+\.?[0-9]*|[0-9]*\.?[0-9]+)(e-?[0-9]+)?
@@ -115,6 +123,7 @@ else		{ return ELSE; }
 source_count	{ return SOURCE_COUNT; }
 has_event	{ return HAS_EVENT; }
 strcmp_cpuid_str	{ return STRCMP_CPUID_STR; }
+NaN		{ return nan_value(yyscanner); }
 {literal}	{ return literal(yyscanner, sctx); }
 {number}	{ return value(yyscanner); }
 {symbol}	{ return str(yyscanner, ID, sctx->runtime); }
-- 
2.43.0.687.g38aa6559b0-goog

[PATCH v1 2/4] perf expr: Fix "has_event" function for metric style events

[Ian Rogers]

Events in metrics cannot use '/' as a separator, it would be
recognized as a divide, so they use '@'. The '@' is recognized in the
metricgroups code and changed to '/', do the same in the has_event
function so that the parsing is only tried without the @s.

Fixes: 4a4a9bf9075f ("perf expr: Add has_event function")
Signed-off-by: Ian Rogers <irogers@google.com>
---
Note, this fixes a has_event issue but as no metrics currently use
metric style events like cpu@inst_retired.any@ no current metrics are
broken.
---
 tools/perf/util/expr.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 7be23b3ac082..b8875aac8f87 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -500,7 +500,25 @@ double expr__has_event(const struct expr_parse_ctx *ctx, bool compute_ids, const
 	tmp = evlist__new();
 	if (!tmp)
 		return NAN;
-	ret = parse_event(tmp, id) ? 0 : 1;
+
+	if (strchr(id, '@')) {
+		char *tmp_id, *p;
+
+		tmp_id = strdup(id);
+		if (!tmp_id) {
+			ret = NAN;
+			goto out;
+		}
+		p = strchr(tmp_id, '@');
+		*p = '/';
+		p = strrchr(tmp_id, '@');
+		*p = '/';
+		ret = parse_event(tmp, tmp_id) ? 0 : 1;
+		free(tmp_id);
+	} else {
+		ret = parse_event(tmp, id) ? 0 : 1;
+	}
+out:
 	evlist__delete(tmp);
 	return ret;
 }
-- 
2.43.0.687.g38aa6559b0-goog

[PATCH v1 3/4] perf stat: Avoid metric-only segv

[Ian Rogers]

Cycles is recognized as part of a hard coded metric in stat-shadow.c,
it may call print_metric_only with a NULL fmt string leading to a
segfault. Handle the NULL fmt explicitly.

Fixes: 088519f318be ("perf stat: Move the display functions to stat-display.c")
Signed-off-by: Ian Rogers <irogers@google.com>
---
Note, the fixes tag is to a refactor that moved the function. The bug
existed before this.
---
 tools/perf/util/stat-display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 8c61f8627ebc..b7d00a538d70 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -560,7 +560,7 @@ static void print_metric_only(struct perf_stat_config *config,
 	if (color)
 		mlen += strlen(color) + sizeof(PERF_COLOR_RESET) - 1;
 
-	color_snprintf(str, sizeof(str), color ?: "", fmt, val);
+	color_snprintf(str, sizeof(str), color ?: "", fmt ?: "", val);
 	fprintf(out, "%*s ", mlen, str);
 	os->first = false;
 }
-- 
2.43.0.687.g38aa6559b0-goog

[PATCH v1 4/4] perf metric: Don't remove scale from counts

[Ian Rogers]

Counts were switched from the scaled saved value form to the
aggregated count to avoid double accounting. When this happened the
removing of scaling for a count should have been removed, however, it
wasn't and this wasn't observed as it normally doesn't matter because
a counter's scale is 1. A problem was observed with RAPL events that
are scaled.

Fixes: 37cc8ad77cf8 ("perf metric: Directly use counts rather than saved_value")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/stat-shadow.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index e31426167852..cf573ff3fa84 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -414,12 +414,7 @@ static int prepare_metric(struct evsel **metric_events,
 				val = NAN;
 				source_count = 0;
 			} else {
-				/*
-				 * If an event was scaled during stat gathering,
-				 * reverse the scale before computing the
-				 * metric.
-				 */
-				val = aggr->counts.val * (1.0 / metric_events[i]->scale);
+				val = aggr->counts.val;
 				source_count = evsel__source_count(metric_events[i]);
 			}
 		}
-- 
2.43.0.687.g38aa6559b0-goog

Re: [PATCH v1 0/4] Fixes/improvements to metric output

[Liang, Kan]

On 2024-02-09 3:49 p.m., Ian Rogers wrote:
> A small addition to allow NaNs to be encoded in metrics and then 3
> fixes to various metric related issues.
> 
> Ian Rogers (4):
>   perf expr: Allow NaN to be a valid number
>   perf expr: Fix "has_event" function for metric style events
>   perf stat: Avoid metric-only segv
>   perf metric: Don't remove scale from counts
>

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

Thanks,
Kan


>  tools/perf/util/expr.c         | 20 +++++++++++++++++++-
>  tools/perf/util/expr.l         |  9 +++++++++
>  tools/perf/util/stat-display.c |  2 +-
>  tools/perf/util/stat-shadow.c  |  7 +------
>  4 files changed, 30 insertions(+), 8 deletions(-)
> 

Re: [PATCH v1 3/4] perf stat: Avoid metric-only segv

[Namhyung Kim]

On Fri, Feb 9, 2024 at 12:50 PM Ian Rogers <irogers@google.com> wrote:
>
> Cycles is recognized as part of a hard coded metric in stat-shadow.c,
> it may call print_metric_only with a NULL fmt string leading to a
> segfault. Handle the NULL fmt explicitly.
>
> Fixes: 088519f318be ("perf stat: Move the display functions to stat-display.c")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> Note, the fixes tag is to a refactor that moved the function. The bug
> existed before this.

Yeah I noticed that.

> ---
>  tools/perf/util/stat-display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 8c61f8627ebc..b7d00a538d70 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -560,7 +560,7 @@ static void print_metric_only(struct perf_stat_config *config,
>         if (color)
>                 mlen += strlen(color) + sizeof(PERF_COLOR_RESET) - 1;
>
> -       color_snprintf(str, sizeof(str), color ?: "", fmt, val);
> +       color_snprintf(str, sizeof(str), color ?: "", fmt ?: "", val);

I was thinking about fixing the callers to pass valid format strings
but it seems they don't care about the output string anyway so
I think it's fine.

Thanks,
Namhyung


>         fprintf(out, "%*s ", mlen, str);
>         os->first = false;
>  }
> --
> 2.43.0.687.g38aa6559b0-goog
>

Re: [PATCH v1 0/4] Fixes/improvements to metric output

[Namhyung Kim]

On Fri, 9 Feb 2024 12:49:43 -0800, Ian Rogers wrote:
> A small addition to allow NaNs to be encoded in metrics and then 3
> fixes to various metric related issues.
> 
> Ian Rogers (4):
>   perf expr: Allow NaN to be a valid number
>   perf expr: Fix "has_event" function for metric style events
>   perf stat: Avoid metric-only segv
>   perf metric: Don't remove scale from counts
> 
> [...]

Applied to perf-tools-next, thanks!

Best regards,
-- 
Namhyung Kim <namhyung@kernel.org>