Re: [PATCH v1 4/9] perf list: Generalize limiting to a PMU name

[Liang, Kan]

On 2022-11-14 2:51 a.m., Ian Rogers wrote:
> Deprecate the --cputype option and add a --unit option where '--unit
> cpu_atom' behaves like '--cputype atom'. The --unit option can be used
> with arbitrary PMUs, for example:
> 
> ```
> $ perf list --unit msr pmu
> 
> List of pre-defined events (to be used in -e or -M):
> 
>   msr/aperf/                                         [Kernel PMU event]
>   msr/cpu_thermal_margin/                            [Kernel PMU event]
>   msr/mperf/                                         [Kernel PMU event]
>   msr/pperf/                                         [Kernel PMU event]
>   msr/smi/                                           [Kernel PMU event]
>   msr/tsc/                                           [Kernel PMU event]
> ```
> 
> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
>  tools/perf/Documentation/perf-list.txt |  6 +++---
>  tools/perf/builtin-list.c              | 18 ++++++++++++------
>  tools/perf/util/metricgroup.c          |  3 ++-
>  tools/perf/util/pmu.c                  |  4 +---
>  4 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-list.txt b/tools/perf/Documentation/perf-list.txt
> index 57384a97c04f..44a819af573d 100644
> --- a/tools/perf/Documentation/perf-list.txt
> +++ b/tools/perf/Documentation/perf-list.txt
> @@ -39,9 +39,9 @@ any extra expressions computed by perf stat.
>  --deprecated::
>  Print deprecated events. By default the deprecated events are hidden.
>  
> ---cputype::
> -Print events applying cpu with this type for hybrid platform
> -(e.g. --cputype core or --cputype atom)

The "--cputype" is removed from the documentation, but the code is still
available. It sounds weird.

Can we still keep the "--cputype" in the documentation? Just say that
it's a deprecated option, please use the --unit cpu_atom instead. I
think even better if we can throw a warning and point to --unit when the
"--cputype" is used.

Thanks,
Kan
> +--unit::
> +Print PMU events and metrics limited to the specific PMU name.
> +(e.g. --unit cpu, --unit msr, --unit cpu_core, --unit cpu_atom)
>  
>  [[EVENT_MODIFIERS]]
>  EVENT MODIFIERS
> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> index 58e1ec1654ef..cc84ced6da26 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -21,7 +21,6 @@
>  
>  static bool desc_flag = true;
>  static bool details_flag;
> -static const char *hybrid_type;
>  
>  int cmd_list(int argc, const char **argv)
>  {
> @@ -30,6 +29,8 @@ int cmd_list(int argc, const char **argv)
>  	bool long_desc_flag = false;
>  	bool deprecated = false;
>  	char *pmu_name = NULL;
> +	const char *hybrid_name = NULL;
> +	const char *unit_name = NULL;
>  	struct option list_options[] = {
>  		OPT_BOOLEAN(0, "raw-dump", &raw_dump, "Dump raw events"),
>  		OPT_BOOLEAN('d', "desc", &desc_flag,
> @@ -40,9 +41,10 @@ int cmd_list(int argc, const char **argv)
>  			    "Print information on the perf event names and expressions used internally by events."),
>  		OPT_BOOLEAN(0, "deprecated", &deprecated,
>  			    "Print deprecated events."),
> -		OPT_STRING(0, "cputype", &hybrid_type, "hybrid cpu type",
> -			   "Print events applying cpu with this type for hybrid platform "
> -			   "(e.g. core or atom)"),
> +		OPT_STRING(0, "cputype", &hybrid_name, "hybrid cpu type",
> +			   "Limit PMU or metric printing to the given hybrid PMU (e.g. core or atom)."),
> +		OPT_STRING(0, "unit", &unit_name, "PMU name",
> +			   "Limit PMU or metric printing to the specified PMU."),
>  		OPT_INCR(0, "debug", &verbose,
>  			     "Enable debugging output"),
>  		OPT_END()
> @@ -53,6 +55,8 @@ int cmd_list(int argc, const char **argv)
>  	};
>  
>  	set_option_flag(list_options, 0, "raw-dump", PARSE_OPT_HIDDEN);
> +	/* Hide hybrid flag for the more generic 'unit' flag. */
> +	set_option_flag(list_options, 0, "cputype", PARSE_OPT_HIDDEN);
>  
>  	argc = parse_options(argc, argv, list_options, list_usage,
>  			     PARSE_OPT_STOP_AT_NON_OPTION);
> @@ -62,8 +66,10 @@ int cmd_list(int argc, const char **argv)
>  	if (!raw_dump && pager_in_use())
>  		printf("\nList of pre-defined events (to be used in -e or -M):\n\n");
>  
> -	if (hybrid_type) {
> -		pmu_name = perf_pmu__hybrid_type_to_pmu(hybrid_type);
> +	if (unit_name)
> +		pmu_name = strdup(unit_name);
> +	else if (hybrid_name) {
> +		pmu_name = perf_pmu__hybrid_type_to_pmu(hybrid_name);
>  		if (!pmu_name)
>  			pr_warning("WARNING: hybrid cputype is not supported!\n");
>  	}
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 4c98ac29ee13..1943fed9b6d9 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -556,11 +556,12 @@ static int metricgroup__print_callback(const struct pmu_event *pe,
>  				       void *vdata)
>  {
>  	struct metricgroup_print_data *data = vdata;
> +	const char *pmu = pe->pmu ?: "cpu";
>  
>  	if (!pe->metric_expr)
>  		return 0;
>  
> -	if (data->pmu_name && perf_pmu__is_hybrid(pe->pmu) && strcmp(data->pmu_name, pe->pmu))
> +	if (data->pmu_name && strcmp(data->pmu_name, pmu))
>  		return 0;
>  
>  	return metricgroup__print_pmu_event(pe, data->metricgroups, data->filter,
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index a8f9f47c6ed9..9c771f136b81 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1694,10 +1694,8 @@ void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
>  	pmu = NULL;
>  	j = 0;
>  	while ((pmu = perf_pmu__scan(pmu)) != NULL) {
> -		if (pmu_name && perf_pmu__is_hybrid(pmu->name) &&
> -		    strcmp(pmu_name, pmu->name)) {
> +		if (pmu_name && pmu->name && strcmp(pmu_name, pmu->name))
>  			continue;
> -		}
>  
>  		list_for_each_entry(alias, &pmu->aliases, list) {
>  			char *name = alias->desc ? alias->name :