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
Currently only floating point numbers can be parsed, add a special
case for NaN.
Signed-off-by: Ian Rogers <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
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(-)
>
On Fri, Feb 9, 2024 at 12:50 PM Ian Rogers <[email protected]> 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 <[email protected]>
> ---
> 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
>
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 <[email protected]>