[PATCH v2] perf list: For metricgroup only list include description

[PATCH v2] perf list: For metricgroup only list include description

[Ian Rogers]

If perf list is invoked with 'metricgroups' include the description
unless it is invoked with flags to exclude it. Make the description of
metricgroup dumping dependent on the desc flag in print_state as with
metrics.

Before:
```
$ perf list metricgroups
List of pre-defined events (to be used in -e or -M):

Metric Groups:

Backend
Bad
BadSpec
...
```

After:
```
$ perf list metricgroups
List of pre-defined events (to be used in -e or -M):

Metric Groups:

Backend [Grouping from Top-down Microarchitecture Analysis Metrics spreadsheet]
Bad [Grouping from Top-down Microarchitecture Analysis Metrics spreadsheet]
BadSpec
...
```

Signed-off-by: Ian Rogers <irogers@google.com>
---
v2 rebases on top of perf-tools-next patch:
79bacb6ad73c perf list: Add output file option
https://lore.kernel.org/r/20240124043015.1388867-3-irogers@google.com
---
 tools/perf/builtin-list.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index e27a1b1288c2..02bf608d585e 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -208,17 +208,24 @@ static void default_print_metric(void *ps,
 	if (!print_state->last_metricgroups ||
 	    strcmp(print_state->last_metricgroups, group ?: "")) {
 		if (group && print_state->metricgroups) {
-			if (print_state->name_only)
+			if (print_state->name_only) {
 				fprintf(fp, "%s ", group);
-			else if (print_state->metrics) {
-				const char *gdesc = describe_metricgroup(group);
+			} else {
+				const char *gdesc = print_state->desc
+					? describe_metricgroup(group)
+					: NULL;
+				const char *print_colon = "";
+
+				if (print_state->metrics) {
+					print_colon = ":";
+					fputc('\n', fp);
+				}
 
 				if (gdesc)
-					fprintf(fp, "\n%s: [%s]\n", group, gdesc);
+					fprintf(fp, "%s%s [%s]\n", group, print_colon, gdesc);
 				else
-					fprintf(fp, "\n%s:\n", group);
-			} else
-				fprintf(fp, "%s\n", group);
+					fprintf(fp, "%s%s\n", group, print_colon);
+			}
 		}
 		zfree(&print_state->last_metricgroups);
 		print_state->last_metricgroups = strdup(group ?: "");
-- 
2.44.0.rc0.258.g7320e95886-goog

Re: [PATCH v2] perf list: For metricgroup only list include description

[Namhyung Kim]

On Fri, Feb 16, 2024 at 11:20:44AM -0800, Ian Rogers wrote:
> If perf list is invoked with 'metricgroups' include the description
> unless it is invoked with flags to exclude it. Make the description of
> metricgroup dumping dependent on the desc flag in print_state as with
> metrics.
> 
> Before:
> ```
> $ perf list metricgroups
> List of pre-defined events (to be used in -e or -M):
> 
> Metric Groups:
> 
> Backend
> Bad
> BadSpec
> ...
> ```
> 
> After:
> ```
> $ perf list metricgroups
> List of pre-defined events (to be used in -e or -M):
> 
> Metric Groups:
> 
> Backend [Grouping from Top-down Microarchitecture Analysis Metrics spreadsheet]
> Bad [Grouping from Top-down Microarchitecture Analysis Metrics spreadsheet]
> BadSpec
> ...
> ```

When I run `perf list` and look at the Metric Groups:

  Metric Groups:
  
  Backend: [Grouping from Top-down Microarchitecture Analysis Metrics spreadsheet]
    tma_core_bound
         [This metric represents fraction of slots where Core non-memory issues were of a bottleneck]
    tma_info_core_ilp
         [Instruction-Level-Parallelism (average number of uops executed when there is execution) per-core]
    tma_info_memory_l2mpki
         [L2 cache true misses per kilo instruction for retired demand loads]
    tma_memory_bound
         [This metric represents fraction of slots the Memory subsystem within the Backend was a bottleneck]
  
  Bad: [Grouping from Top-down Microarchitecture Analysis Metrics spreadsheet]
    tma_info_bad_spec_branch_misprediction_cost
         [Branch Misprediction Cost: Fraction of TMA slots wasted per non-speculative branch misprediction (retired JEClear)]
    tma_info_bad_spec_ipmisp_cond_ntaken
         [Instructions per retired mispredicts for conditional non-taken branches (lower number means higher occurrence rate)]
    tma_info_bad_spec_ipmisp_cond_taken
         [Instructions per retired mispredicts for conditional taken branches (lower number means higher occurrence rate)]
    ...
 
So this change aligns with the default behavior, so

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> v2 rebases on top of perf-tools-next patch:
> 79bacb6ad73c perf list: Add output file option
> https://lore.kernel.org/r/20240124043015.1388867-3-irogers@google.com
> ---
>  tools/perf/builtin-list.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> index e27a1b1288c2..02bf608d585e 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -208,17 +208,24 @@ static void default_print_metric(void *ps,
>  	if (!print_state->last_metricgroups ||
>  	    strcmp(print_state->last_metricgroups, group ?: "")) {
>  		if (group && print_state->metricgroups) {
> -			if (print_state->name_only)
> +			if (print_state->name_only) {
>  				fprintf(fp, "%s ", group);
> -			else if (print_state->metrics) {
> -				const char *gdesc = describe_metricgroup(group);
> +			} else {
> +				const char *gdesc = print_state->desc
> +					? describe_metricgroup(group)
> +					: NULL;
> +				const char *print_colon = "";
> +
> +				if (print_state->metrics) {
> +					print_colon = ":";
> +					fputc('\n', fp);
> +				}
>  
>  				if (gdesc)
> -					fprintf(fp, "\n%s: [%s]\n", group, gdesc);
> +					fprintf(fp, "%s%s [%s]\n", group, print_colon, gdesc);
>  				else
> -					fprintf(fp, "\n%s:\n", group);
> -			} else
> -				fprintf(fp, "%s\n", group);
> +					fprintf(fp, "%s%s\n", group, print_colon);
> +			}
>  		}
>  		zfree(&print_state->last_metricgroups);
>  		print_state->last_metricgroups = strdup(group ?: "");
> -- 
> 2.44.0.rc0.258.g7320e95886-goog
> 

Re: [PATCH v2] perf list: For metricgroup only list include description

[Namhyung Kim]

On Fri, 16 Feb 2024 11:20:44 -0800, Ian Rogers wrote:
> If perf list is invoked with 'metricgroups' include the description
> unless it is invoked with flags to exclude it. Make the description of
> metricgroup dumping dependent on the desc flag in print_state as with
> metrics.
> 
> Before:
> ```
> $ perf list metricgroups
> List of pre-defined events (to be used in -e or -M):
> 
> [...]

Applied to perf-tools-next, thanks!

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