diff mbox series

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

Message ID	20221114075127.2650315-5-irogers@google.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Date: Sun, 13 Nov 2022 23:51:22 -0800 In-Reply-To: <20221114075127.2650315-1-irogers@google.com> Message-Id: <20221114075127.2650315-5-irogers@google.com> Mime-Version: 1.0 References: <20221114075127.2650315-1-irogers@google.com> Subject: [PATCH v1 4/9] perf list: Generalize limiting to a PMU name From: Ian Rogers <irogers@google.com> To: Weilin Wang <weilin.wang@intel.com>, Perry Taylor <perry.taylor@intel.com>, Caleb Biggers <caleb.biggers@intel.com>, Leo Yan <leo.yan@linaro.org>, Adrian Hunter <adrian.hunter@intel.com>, Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, Arnaldo Carvalho de Melo <acme@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>, Sandipan Das <sandipan.das@amd.com>, Kajol Jain <kjain@linux.ibm.com>, Zhengjun Xing <zhengjun.xing@linux.intel.com>, Kan Liang <kan.liang@linux.intel.com>, Ravi Bangoria <ravi.bangoria@amd.com>, Xin Gao <gaoxin@cdjrlc.com>, Rob Herring <robh@kernel.org>, linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org Cc: Stephane Eranian <eranian@google.com>, Ian Rogers <irogers@google.com> Content-Type: text/plain; charset="UTF-8" Precedence: bulk
Series	Restructure perf list and add json output \| [v1,0/9] Restructure perf list and add json output [v1,1/9] perf pmu: Add documentation [v1,2/9] tools lib api fs tracing_path: Add scandir alphasort [v1,3/9] perf tracepoint: Sort events in iterator [v1,4/9] perf list: Generalize limiting to a PMU name [v1,5/9] perf list: Simplify cache event printing [v1,6/9] perf list: Simplify symbol event printing [v1,7/9] perf pmu: Restructure print_pmu_events [v1,8/9] perf list: Reorganize to use callbacks [v1,9/9] perf list: Add json output option

Commit Message

Ian Rogers Nov. 14, 2022, 7:51 a.m. UTC

  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@google.com>
---
 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(-)

Comments

Xing Zhengjun Nov. 14, 2022, 8:51 a.m. UTC | #1

On 11/14/2022 3:51 PM, 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@google.com>
> ---
>   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)
> +--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))

Why remove perf_pmu__is_hybrid check?

>   			continue;
> -		}
>   
>   		list_for_each_entry(alias, &pmu->aliases, list) {
>   			char *name = alias->desc ? alias->name :

Liang, Kan Nov. 14, 2022, 1:57 p.m. UTC | #2

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@google.com>
> ---
>  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 :

Ian Rogers Nov. 14, 2022, 1:58 p.m. UTC | #3

On Mon, Nov 14, 2022 at 12:51 AM Xing Zhengjun
<zhengjun.xing@linux.intel.com> wrote:
>
>
>
> On 11/14/2022 3:51 PM, 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@google.com>
> > ---
> >   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)
> > +--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))
>
> Why remove perf_pmu__is_hybrid check?

Thanks for checking! If you have the perf_pmu__is_hybrid check then
the filter only applies PMUs that are hybrid. Firstly, why have
is_hybrid in struct perf_pmu if you use this function that searches a
list?  Secondly, the point of this change is to make the PMU name
filtering a generic feature rather than one limited to specifically
PMUs called cpu_core and cpu_atom.

Thanks,
Ian

>
> >                       continue;
> > -             }
> >
> >               list_for_each_entry(alias, &pmu->aliases, list) {
> >                       char *name = alias->desc ? alias->name :
>
> --
> Zhengjun Xing

Ian Rogers Nov. 14, 2022, 2:02 p.m. UTC | #4

On Mon, Nov 14, 2022 at 5:58 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> 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@google.com>
> > ---
> >  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.

I think we want to remove --cputype widely in the code and replace
what it does with specifying a PMU name. Advertising a flag but then
warning seems strange and is a behavioral change from what is
currently done. For raw-dump we don't document it in the man page and
hide the flag, this is the pattern being followed here.

Thanks,
Ian

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

Liang, Kan Nov. 14, 2022, 2:53 p.m. UTC | #5

On 2022-11-14 9:02 a.m., Ian Rogers wrote:
> On Mon, Nov 14, 2022 at 5:58 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>
>>
>> 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@google.com>
>>> ---
>>>  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.
> 
> I think we want to remove --cputype widely in the code and replace
> what it does with specifying a PMU name. Advertising a flag but then
> warning seems strange and is a behavioral change from what is
> currently done. For raw-dump we don't document it in the man page and
> hide the flag, this is the pattern being followed here.

I see. So the --cputype is still supported, but only be hidden in the
default. Sure, we can follow the pattern.

> 
> Thanks,
> Ian
> 
>> 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");
>>>       }

Can the tool implicitly convert the --cputype to --unit at the very
beginning? (Just need to append a prefix "cpu_".). Here we only need to
handle the unit_name.
The same logic may be applied for other tools if someone implements the
--unit for the stat or record later.

BTW: we may want to check the existence of a PMU here, just like what we
did for the hybrid. If a user perf list a nonexistence PMU, we can warn
here.

Thanks,
Kan

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

Ian Rogers Nov. 14, 2022, 5:10 p.m. UTC | #6

On Mon, Nov 14, 2022 at 6:55 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2022-11-14 9:02 a.m., Ian Rogers wrote:
> > On Mon, Nov 14, 2022 at 5:58 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >>
> >>
> >>
> >> 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@google.com>
> >>> ---
> >>>  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.
> >
> > I think we want to remove --cputype widely in the code and replace
> > what it does with specifying a PMU name. Advertising a flag but then
> > warning seems strange and is a behavioral change from what is
> > currently done. For raw-dump we don't document it in the man page and
> > hide the flag, this is the pattern being followed here.
>
> I see. So the --cputype is still supported, but only be hidden in the
> default. Sure, we can follow the pattern.
>
> >
> > Thanks,
> > Ian
> >
> >> 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");
> >>>       }
>
> Can the tool implicitly convert the --cputype to --unit at the very
> beginning? (Just need to append a prefix "cpu_".). Here we only need to
> handle the unit_name.
> The same logic may be applied for other tools if someone implements the
> --unit for the stat or record later.
>
> BTW: we may want to check the existence of a PMU here, just like what we
> did for the hybrid. If a user perf list a nonexistence PMU, we can warn
> here.

That'd be a fine follow up. I think it would be problematic as it
would warn because of a lack of permissions.

Thanks,
Ian

> Thanks,
> Kan
>
> >>> 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 :

Liang, Kan Nov. 14, 2022, 7 p.m. UTC | #7

On 2022-11-14 12:10 p.m., Ian Rogers wrote:
> On Mon, Nov 14, 2022 at 6:55 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>
>>
>> On 2022-11-14 9:02 a.m., Ian Rogers wrote:
>>> On Mon, Nov 14, 2022 at 5:58 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>>>
>>>>
>>>>
>>>> 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@google.com>
>>>>> ---
>>>>>  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.
>>>
>>> I think we want to remove --cputype widely in the code and replace
>>> what it does with specifying a PMU name. Advertising a flag but then
>>> warning seems strange and is a behavioral change from what is
>>> currently done. For raw-dump we don't document it in the man page and
>>> hide the flag, this is the pattern being followed here.
>>
>> I see. So the --cputype is still supported, but only be hidden in the
>> default. Sure, we can follow the pattern.
>>
>>>
>>> Thanks,
>>> Ian
>>>
>>>> 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");
>>>>>       }
>>
>> Can the tool implicitly convert the --cputype to --unit at the very
>> beginning? (Just need to append a prefix "cpu_".). Here we only need to
>> handle the unit_name.
>> The same logic may be applied for other tools if someone implements the
>> --unit for the stat or record later.
>>
>> BTW: we may want to check the existence of a PMU here, just like what we
>> did for the hybrid. If a user perf list a nonexistence PMU, we can warn
>> here.
> 
> That'd be a fine follow up. I think it would be problematic as it
> would warn because of a lack of permissions.
>

Just check whether the PMU folder exists in the sysfs. I don't think it
has a permission issue.

Thanks,
Kan

> Thanks,
> Ian
> 
>> Thanks,
>> Kan
>>
>>>>> 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 :

diff mbox series

Patch

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)
+--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 :