[PATCH v1 0/9] Restructure perf list and add json output

[PATCH v1 0/9] Restructure perf list and add json output

[Ian Rogers]

Restructure perf list so that it uses callbacks to print events and
metrics. Use the callbacks to implement json output. In the process
add documentation to pmu.h, avoid some sorting of events, fix some
command line and output bugs.

Ian Rogers (9):
  perf pmu: Add documentation
  tools lib api fs tracing_path: Add scandir alphasort
  perf tracepoint: Sort events in iterator
  perf list: Generalize limiting to a PMU name
  perf list: Simplify cache event printing
  perf list: Simplify symbol event printing
  perf pmu: Restructure print_pmu_events
  perf list: Reorganize to use callbacks
  perf list: Add json output option

 tools/lib/api/fs/tracing_path.c        |  16 +
 tools/lib/api/fs/tracing_path.h        |   1 +
 tools/perf/Documentation/perf-list.txt |  10 +-
 tools/perf/builtin-list.c              | 503 +++++++++++++++++---
 tools/perf/util/metricgroup.c          | 238 +++-------
 tools/perf/util/metricgroup.h          |   4 +-
 tools/perf/util/pmu.c                  | 277 +++++------
 tools/perf/util/pmu.h                  | 110 ++++-
 tools/perf/util/print-events.c         | 630 ++++++++++---------------
 tools/perf/util/print-events.h         |  40 +-
 10 files changed, 1034 insertions(+), 795 deletions(-)

-- 
2.38.1.431.g37b22c650d-goog

[PATCH v1 1/9] perf pmu: Add documentation

[Ian Rogers]

Add documentation to struct perf_pmu and the associated structs of
perf_pmu_alias and perf_pmu_format.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/pmu.c |  14 ++++++
 tools/perf/util/pmu.h | 105 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 113 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 6a86e6af0903..a8f9f47c6ed9 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -31,10 +31,24 @@
 
 struct perf_pmu perf_pmu__fake;
 
+/**
+ * Values from a format file read from <sysfs>/devices/cpu/format/ held in
+ * struct perf_pmu. For example, the contents of
+ * <sysfs>/devices/cpu/format/event may be "config:0-7" and will be represented
+ * here as name="event", value=PERF_PMU_FORMAT_VALUE_CONFIG and bits 0 to 7 will
+ * be set.
+ */
 struct perf_pmu_format {
+	/** The modifier/file name. */
 	char *name;
+	/**
+	 * Which config value the format relates to. Supported values are from
+	 * PERF_PMU_FORMAT_VALUE_CONFIG to PERF_PMU_FORMAT_VALUE_CONFIG_END.
+	 */
 	int value;
+	/** Which config bits are set by this format value. */
 	DECLARE_BITMAP(bits, PERF_PMU_FORMAT_BITS);
+	/** Element on list within struct perf_pmu. */
 	struct list_head list;
 };
 
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 68e15c38ae71..29571c0f9d15 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -34,30 +34,91 @@ struct perf_pmu_caps {
 };
 
 struct perf_pmu {
+	/** The name of the PMU such as "cpu". */
 	char *name;
+	/**
+	 * Optional alternate name for the PMU determined in architecture
+	 * specific code.
+	 */
 	char *alias_name;
+	/**
+	 * Optional PMU identifier read from
+	 * <sysfs>/bus/event_source/devices/<name>/identifier.
+	 */
 	char *id;
+	/**
+	 * Perf event attributed type value, read from
+	 * <sysfs>/bus/event_source/devices/<name>/type.
+	 */
 	__u32 type;
+	/**
+	 * Can the PMU name be selected as if it were an event?
+	 */
 	bool selectable;
+	/**
+	 * Is the PMU not within the CPU core? Determined by the presence of
+	 * <sysfs>/bus/event_source/devices/<name>/cpumask.
+	 */
 	bool is_uncore;
+	/** Is the PMU name either cpu_core or cpu_atom. */
 	bool is_hybrid;
+	/**
+	 * Are events auxiliary events? Determined in architecture specific
+	 * code.
+	 */
 	bool auxtrace;
+	/**
+	 * Number of levels of :ppp precision supported by the PMU, read from
+	 * <sysfs>/bus/event_source/devices/<name>/caps/max_precise.
+	 */
 	int max_precise;
+	/**
+	 * Optional default perf_event_attr determined in architecture specific
+	 * code.
+	 */
 	struct perf_event_attr *default_config;
+	/**
+	 * Empty or the contents of either of:
+	 * <sysfs>/bus/event_source/devices/<name>/cpumask.
+	 * <sysfs>/bus/event_source/devices/<cpu>/cpus.
+	 */
 	struct perf_cpu_map *cpus;
-	struct list_head format;  /* HEAD struct perf_pmu_format -> list */
-	struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
+	/**
+	 * Holds the contents of files read from
+	 * <sysfs>/bus/event_source/devices/<name>/format/. The contents specify
+	 * which event parameter changes what config, config1 or config2 bits.
+	 */
+	struct list_head format;
+	/**
+	 * List of struct perf_pmu_alias. Each alias corresponds to an event
+	 * read from <sysfs>/bus/event_source/devices/<name>/events/ or from
+	 * json events in pmu-events.c.
+	 */
+	struct list_head aliases;
+	/** Has the list caps been initialized? */
 	bool caps_initialized;
+	/** The length of the list caps. */
 	u32 nr_caps;
-	struct list_head caps;    /* HEAD struct perf_pmu_caps -> list */
-	struct list_head list;    /* ELEM */
+	/**
+	 * Holds the contents of files read from
+	 * <sysfs>/bus/event_source/devices/<name>/caps/. The contents are pairs
+	 * of the filename with the value of its contents, for example,
+	 * max_precise (see above) may have a value of 3.
+	 */
+	struct list_head caps;
+	/** Element on pmus list in pmu.c. */
+	struct list_head list;
+	/** Element on perf_pmu__hybrid_pmus. */
 	struct list_head hybrid_list;
 
+	/** Features to inhibit when events on this PMU are opened. */
 	struct {
+		/** Disables perf_event_attr exclude_guest and exclude_host. */
 		bool exclude_guest;
 	} missing_features;
 };
 
+/** A special global PMU used for testing. */
 extern struct perf_pmu perf_pmu__fake;
 
 struct perf_pmu_info {
@@ -71,21 +132,53 @@ struct perf_pmu_info {
 
 #define UNIT_MAX_LEN	31 /* max length for event unit name */
 
+/**
+ * An event either read from sysfs or builtin in pmu-events.c, created by
+ * parsing the pmu-events json files.
+ */
 struct perf_pmu_alias {
 	char *name;
+	/** Optional short description of the event. */
 	char *desc;
+	/** Optional long description. */
 	char *long_desc;
+	/**
+	 * Optional topic such as cache or pipeline, particularly for json
+	 * events.
+	 */
 	char *topic;
+	/** Comma separated parameter list. */
 	char *str;
-	struct list_head terms; /* HEAD struct parse_events_term -> list */
-	struct list_head list;  /* ELEM */
+	/** Owned list of the original parsed parameters. */
+	struct list_head terms;
+	/** List element of struct perf_pmu aliases. */
+	struct list_head list;
+	/** Units for the event, such as bytes or cache lines. */
 	char unit[UNIT_MAX_LEN+1];
+	/** Value to scale read counter values by. */
 	double scale;
+	/**
+	 * Does the file
+	 * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.per-pkg or
+	 * equivalent json value exist and have the value 1.
+	 */
 	bool per_pkg;
+	/**
+	 * Does the file
+	 * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.snapshot
+	 * exist and have the value 1.
+	 */
 	bool snapshot;
+	/** Is the event hidden and so not shown in perf list by default. */
 	bool deprecated;
+	/**
+	 * A metric expression associated with an event. Doing this makes little
+	 * sense due to scale and unit applying to both.
+	 */
 	char *metric_expr;
+	/** A name for the metric. unit applying to both. */
 	char *metric_name;
+	/** The name copied from struct perf_pmu. */
 	char *pmu_name;
 };
 
-- 
2.38.1.431.g37b22c650d-goog

Re: [PATCH v1 1/9] perf pmu: Add documentation

[Adrian Hunter]

On 14/11/22 09:51, Ian Rogers wrote:
> Add documentation to struct perf_pmu and the associated structs of
> perf_pmu_alias and perf_pmu_format.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>

Should this be kernel-doc format?

$ ./scripts/kernel-doc -man tools/perf/util/pmu.* > /tmp/manout
tools/perf/util/pmu.c:35: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
 * Values from a format file read from <sysfs>/devices/cpu/format/ held in
tools/perf/util/pmu.c:1: warning: no structured comments found
tools/perf/util/pmu.h:140: warning: cannot understand function prototype: 'struct perf_pmu_alias '
tools/perf/util/pmu.h:1: warning: no structured comments found
tools/perf/util/pmu.l:1: warning: no structured comments found
tools/perf/util/pmu.o:1: warning: no structured comments found
tools/perf/util/pmu.y:1: warning: no structured comments found
$ man -l /tmp/manout | cat
$

> ---
>  tools/perf/util/pmu.c |  14 ++++++
>  tools/perf/util/pmu.h | 105 +++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 113 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 6a86e6af0903..a8f9f47c6ed9 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -31,10 +31,24 @@
>  
>  struct perf_pmu perf_pmu__fake;
>  
> +/**
> + * Values from a format file read from <sysfs>/devices/cpu/format/ held in
> + * struct perf_pmu. For example, the contents of
> + * <sysfs>/devices/cpu/format/event may be "config:0-7" and will be represented
> + * here as name="event", value=PERF_PMU_FORMAT_VALUE_CONFIG and bits 0 to 7 will
> + * be set.
> + */
>  struct perf_pmu_format {
> +	/** The modifier/file name. */
>  	char *name;
> +	/**
> +	 * Which config value the format relates to. Supported values are from
> +	 * PERF_PMU_FORMAT_VALUE_CONFIG to PERF_PMU_FORMAT_VALUE_CONFIG_END.
> +	 */
>  	int value;
> +	/** Which config bits are set by this format value. */
>  	DECLARE_BITMAP(bits, PERF_PMU_FORMAT_BITS);
> +	/** Element on list within struct perf_pmu. */
>  	struct list_head list;
>  };
>  
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 68e15c38ae71..29571c0f9d15 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -34,30 +34,91 @@ struct perf_pmu_caps {
>  };
>  
>  struct perf_pmu {
> +	/** The name of the PMU such as "cpu". */
>  	char *name;
> +	/**
> +	 * Optional alternate name for the PMU determined in architecture
> +	 * specific code.
> +	 */
>  	char *alias_name;
> +	/**
> +	 * Optional PMU identifier read from
> +	 * <sysfs>/bus/event_source/devices/<name>/identifier.
> +	 */
>  	char *id;
> +	/**
> +	 * Perf event attributed type value, read from
> +	 * <sysfs>/bus/event_source/devices/<name>/type.
> +	 */
>  	__u32 type;
> +	/**
> +	 * Can the PMU name be selected as if it were an event?
> +	 */
>  	bool selectable;
> +	/**
> +	 * Is the PMU not within the CPU core? Determined by the presence of
> +	 * <sysfs>/bus/event_source/devices/<name>/cpumask.
> +	 */
>  	bool is_uncore;
> +	/** Is the PMU name either cpu_core or cpu_atom. */
>  	bool is_hybrid;
> +	/**
> +	 * Are events auxiliary events? Determined in architecture specific
> +	 * code.
> +	 */
>  	bool auxtrace;
> +	/**
> +	 * Number of levels of :ppp precision supported by the PMU, read from
> +	 * <sysfs>/bus/event_source/devices/<name>/caps/max_precise.
> +	 */
>  	int max_precise;
> +	/**
> +	 * Optional default perf_event_attr determined in architecture specific
> +	 * code.
> +	 */
>  	struct perf_event_attr *default_config;
> +	/**
> +	 * Empty or the contents of either of:
> +	 * <sysfs>/bus/event_source/devices/<name>/cpumask.
> +	 * <sysfs>/bus/event_source/devices/<cpu>/cpus.
> +	 */
>  	struct perf_cpu_map *cpus;
> -	struct list_head format;  /* HEAD struct perf_pmu_format -> list */
> -	struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
> +	/**
> +	 * Holds the contents of files read from
> +	 * <sysfs>/bus/event_source/devices/<name>/format/. The contents specify
> +	 * which event parameter changes what config, config1 or config2 bits.
> +	 */
> +	struct list_head format;
> +	/**
> +	 * List of struct perf_pmu_alias. Each alias corresponds to an event
> +	 * read from <sysfs>/bus/event_source/devices/<name>/events/ or from
> +	 * json events in pmu-events.c.
> +	 */
> +	struct list_head aliases;
> +	/** Has the list caps been initialized? */
>  	bool caps_initialized;
> +	/** The length of the list caps. */
>  	u32 nr_caps;
> -	struct list_head caps;    /* HEAD struct perf_pmu_caps -> list */
> -	struct list_head list;    /* ELEM */
> +	/**
> +	 * Holds the contents of files read from
> +	 * <sysfs>/bus/event_source/devices/<name>/caps/. The contents are pairs
> +	 * of the filename with the value of its contents, for example,
> +	 * max_precise (see above) may have a value of 3.
> +	 */
> +	struct list_head caps;
> +	/** Element on pmus list in pmu.c. */
> +	struct list_head list;
> +	/** Element on perf_pmu__hybrid_pmus. */
>  	struct list_head hybrid_list;
>  
> +	/** Features to inhibit when events on this PMU are opened. */
>  	struct {
> +		/** Disables perf_event_attr exclude_guest and exclude_host. */
>  		bool exclude_guest;
>  	} missing_features;
>  };
>  
> +/** A special global PMU used for testing. */
>  extern struct perf_pmu perf_pmu__fake;
>  
>  struct perf_pmu_info {
> @@ -71,21 +132,53 @@ struct perf_pmu_info {
>  
>  #define UNIT_MAX_LEN	31 /* max length for event unit name */
>  
> +/**
> + * An event either read from sysfs or builtin in pmu-events.c, created by
> + * parsing the pmu-events json files.
> + */
>  struct perf_pmu_alias {
>  	char *name;
> +	/** Optional short description of the event. */
>  	char *desc;
> +	/** Optional long description. */
>  	char *long_desc;
> +	/**
> +	 * Optional topic such as cache or pipeline, particularly for json
> +	 * events.
> +	 */
>  	char *topic;
> +	/** Comma separated parameter list. */
>  	char *str;
> -	struct list_head terms; /* HEAD struct parse_events_term -> list */
> -	struct list_head list;  /* ELEM */
> +	/** Owned list of the original parsed parameters. */
> +	struct list_head terms;
> +	/** List element of struct perf_pmu aliases. */
> +	struct list_head list;
> +	/** Units for the event, such as bytes or cache lines. */
>  	char unit[UNIT_MAX_LEN+1];
> +	/** Value to scale read counter values by. */
>  	double scale;
> +	/**
> +	 * Does the file
> +	 * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.per-pkg or
> +	 * equivalent json value exist and have the value 1.
> +	 */
>  	bool per_pkg;
> +	/**
> +	 * Does the file
> +	 * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.snapshot
> +	 * exist and have the value 1.
> +	 */
>  	bool snapshot;
> +	/** Is the event hidden and so not shown in perf list by default. */
>  	bool deprecated;
> +	/**
> +	 * A metric expression associated with an event. Doing this makes little
> +	 * sense due to scale and unit applying to both.
> +	 */
>  	char *metric_expr;
> +	/** A name for the metric. unit applying to both. */
>  	char *metric_name;
> +	/** The name copied from struct perf_pmu. */
>  	char *pmu_name;
>  };
>  

Re: [PATCH v1 1/9] perf pmu: Add documentation

[Ian Rogers]

On Mon, Nov 14, 2022 at 12:56 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 14/11/22 09:51, Ian Rogers wrote:
> > Add documentation to struct perf_pmu and the associated structs of
> > perf_pmu_alias and perf_pmu_format.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
>
> Should this be kernel-doc format?
>
> $ ./scripts/kernel-doc -man tools/perf/util/pmu.* > /tmp/manout
> tools/perf/util/pmu.c:35: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
>  * Values from a format file read from <sysfs>/devices/cpu/format/ held in
> tools/perf/util/pmu.c:1: warning: no structured comments found
> tools/perf/util/pmu.h:140: warning: cannot understand function prototype: 'struct perf_pmu_alias '
> tools/perf/util/pmu.h:1: warning: no structured comments found
> tools/perf/util/pmu.l:1: warning: no structured comments found
> tools/perf/util/pmu.o:1: warning: no structured comments found
> tools/perf/util/pmu.y:1: warning: no structured comments found
> $ man -l /tmp/manout | cat
> $

Thanks, I'll take a look into fixing this.

Ian

> > ---
> >  tools/perf/util/pmu.c |  14 ++++++
> >  tools/perf/util/pmu.h | 105 +++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 113 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > index 6a86e6af0903..a8f9f47c6ed9 100644
> > --- a/tools/perf/util/pmu.c
> > +++ b/tools/perf/util/pmu.c
> > @@ -31,10 +31,24 @@
> >
> >  struct perf_pmu perf_pmu__fake;
> >
> > +/**
> > + * Values from a format file read from <sysfs>/devices/cpu/format/ held in
> > + * struct perf_pmu. For example, the contents of
> > + * <sysfs>/devices/cpu/format/event may be "config:0-7" and will be represented
> > + * here as name="event", value=PERF_PMU_FORMAT_VALUE_CONFIG and bits 0 to 7 will
> > + * be set.
> > + */
> >  struct perf_pmu_format {
> > +     /** The modifier/file name. */
> >       char *name;
> > +     /**
> > +      * Which config value the format relates to. Supported values are from
> > +      * PERF_PMU_FORMAT_VALUE_CONFIG to PERF_PMU_FORMAT_VALUE_CONFIG_END.
> > +      */
> >       int value;
> > +     /** Which config bits are set by this format value. */
> >       DECLARE_BITMAP(bits, PERF_PMU_FORMAT_BITS);
> > +     /** Element on list within struct perf_pmu. */
> >       struct list_head list;
> >  };
> >
> > diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> > index 68e15c38ae71..29571c0f9d15 100644
> > --- a/tools/perf/util/pmu.h
> > +++ b/tools/perf/util/pmu.h
> > @@ -34,30 +34,91 @@ struct perf_pmu_caps {
> >  };
> >
> >  struct perf_pmu {
> > +     /** The name of the PMU such as "cpu". */
> >       char *name;
> > +     /**
> > +      * Optional alternate name for the PMU determined in architecture
> > +      * specific code.
> > +      */
> >       char *alias_name;
> > +     /**
> > +      * Optional PMU identifier read from
> > +      * <sysfs>/bus/event_source/devices/<name>/identifier.
> > +      */
> >       char *id;
> > +     /**
> > +      * Perf event attributed type value, read from
> > +      * <sysfs>/bus/event_source/devices/<name>/type.
> > +      */
> >       __u32 type;
> > +     /**
> > +      * Can the PMU name be selected as if it were an event?
> > +      */
> >       bool selectable;
> > +     /**
> > +      * Is the PMU not within the CPU core? Determined by the presence of
> > +      * <sysfs>/bus/event_source/devices/<name>/cpumask.
> > +      */
> >       bool is_uncore;
> > +     /** Is the PMU name either cpu_core or cpu_atom. */
> >       bool is_hybrid;
> > +     /**
> > +      * Are events auxiliary events? Determined in architecture specific
> > +      * code.
> > +      */
> >       bool auxtrace;
> > +     /**
> > +      * Number of levels of :ppp precision supported by the PMU, read from
> > +      * <sysfs>/bus/event_source/devices/<name>/caps/max_precise.
> > +      */
> >       int max_precise;
> > +     /**
> > +      * Optional default perf_event_attr determined in architecture specific
> > +      * code.
> > +      */
> >       struct perf_event_attr *default_config;
> > +     /**
> > +      * Empty or the contents of either of:
> > +      * <sysfs>/bus/event_source/devices/<name>/cpumask.
> > +      * <sysfs>/bus/event_source/devices/<cpu>/cpus.
> > +      */
> >       struct perf_cpu_map *cpus;
> > -     struct list_head format;  /* HEAD struct perf_pmu_format -> list */
> > -     struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
> > +     /**
> > +      * Holds the contents of files read from
> > +      * <sysfs>/bus/event_source/devices/<name>/format/. The contents specify
> > +      * which event parameter changes what config, config1 or config2 bits.
> > +      */
> > +     struct list_head format;
> > +     /**
> > +      * List of struct perf_pmu_alias. Each alias corresponds to an event
> > +      * read from <sysfs>/bus/event_source/devices/<name>/events/ or from
> > +      * json events in pmu-events.c.
> > +      */
> > +     struct list_head aliases;
> > +     /** Has the list caps been initialized? */
> >       bool caps_initialized;
> > +     /** The length of the list caps. */
> >       u32 nr_caps;
> > -     struct list_head caps;    /* HEAD struct perf_pmu_caps -> list */
> > -     struct list_head list;    /* ELEM */
> > +     /**
> > +      * Holds the contents of files read from
> > +      * <sysfs>/bus/event_source/devices/<name>/caps/. The contents are pairs
> > +      * of the filename with the value of its contents, for example,
> > +      * max_precise (see above) may have a value of 3.
> > +      */
> > +     struct list_head caps;
> > +     /** Element on pmus list in pmu.c. */
> > +     struct list_head list;
> > +     /** Element on perf_pmu__hybrid_pmus. */
> >       struct list_head hybrid_list;
> >
> > +     /** Features to inhibit when events on this PMU are opened. */
> >       struct {
> > +             /** Disables perf_event_attr exclude_guest and exclude_host. */
> >               bool exclude_guest;
> >       } missing_features;
> >  };
> >
> > +/** A special global PMU used for testing. */
> >  extern struct perf_pmu perf_pmu__fake;
> >
> >  struct perf_pmu_info {
> > @@ -71,21 +132,53 @@ struct perf_pmu_info {
> >
> >  #define UNIT_MAX_LEN 31 /* max length for event unit name */
> >
> > +/**
> > + * An event either read from sysfs or builtin in pmu-events.c, created by
> > + * parsing the pmu-events json files.
> > + */
> >  struct perf_pmu_alias {
> >       char *name;
> > +     /** Optional short description of the event. */
> >       char *desc;
> > +     /** Optional long description. */
> >       char *long_desc;
> > +     /**
> > +      * Optional topic such as cache or pipeline, particularly for json
> > +      * events.
> > +      */
> >       char *topic;
> > +     /** Comma separated parameter list. */
> >       char *str;
> > -     struct list_head terms; /* HEAD struct parse_events_term -> list */
> > -     struct list_head list;  /* ELEM */
> > +     /** Owned list of the original parsed parameters. */
> > +     struct list_head terms;
> > +     /** List element of struct perf_pmu aliases. */
> > +     struct list_head list;
> > +     /** Units for the event, such as bytes or cache lines. */
> >       char unit[UNIT_MAX_LEN+1];
> > +     /** Value to scale read counter values by. */
> >       double scale;
> > +     /**
> > +      * Does the file
> > +      * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.per-pkg or
> > +      * equivalent json value exist and have the value 1.
> > +      */
> >       bool per_pkg;
> > +     /**
> > +      * Does the file
> > +      * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.snapshot
> > +      * exist and have the value 1.
> > +      */
> >       bool snapshot;
> > +     /** Is the event hidden and so not shown in perf list by default. */
> >       bool deprecated;
> > +     /**
> > +      * A metric expression associated with an event. Doing this makes little
> > +      * sense due to scale and unit applying to both.
> > +      */
> >       char *metric_expr;
> > +     /** A name for the metric. unit applying to both. */
> >       char *metric_name;
> > +     /** The name copied from struct perf_pmu. */
> >       char *pmu_name;
> >  };
> >
>

Re: [PATCH v1 1/9] perf pmu: Add documentation

[Liang, Kan]

On 2022-11-14 2:51 a.m., Ian Rogers wrote:
> Add documentation to struct perf_pmu and the associated structs of
> perf_pmu_alias and perf_pmu_format.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/pmu.c |  14 ++++++
>  tools/perf/util/pmu.h | 105 +++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 113 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 6a86e6af0903..a8f9f47c6ed9 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -31,10 +31,24 @@
>  
>  struct perf_pmu perf_pmu__fake;
>  
> +/**
> + * Values from a format file read from <sysfs>/devices/cpu/format/ held in
> + * struct perf_pmu. For example, the contents of
> + * <sysfs>/devices/cpu/format/event may be "config:0-7" and will be represented
> + * here as name="event", value=PERF_PMU_FORMAT_VALUE_CONFIG and bits 0 to 7 will
> + * be set.
> + */
>  struct perf_pmu_format {
> +	/** The modifier/file name. */
>  	char *name;
> +	/**
> +	 * Which config value the format relates to. Supported values are from
> +	 * PERF_PMU_FORMAT_VALUE_CONFIG to PERF_PMU_FORMAT_VALUE_CONFIG_END.
> +	 */
>  	int value;
> +	/** Which config bits are set by this format value. */
>  	DECLARE_BITMAP(bits, PERF_PMU_FORMAT_BITS);
> +	/** Element on list within struct perf_pmu. */
>  	struct list_head list;
>  };
>  
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 68e15c38ae71..29571c0f9d15 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -34,30 +34,91 @@ struct perf_pmu_caps {
>  };
>  
>  struct perf_pmu {
> +	/** The name of the PMU such as "cpu". */
>  	char *name;
> +	/**
> +	 * Optional alternate name for the PMU determined in architecture
> +	 * specific code.
> +	 */
>  	char *alias_name;
> +	/**
> +	 * Optional PMU identifier read from
> +	 * <sysfs>/bus/event_source/devices/<name>/identifier.
> +	 */
>  	char *id;
> +	/**
> +	 * Perf event attributed type value, read from
> +	 * <sysfs>/bus/event_source/devices/<name>/type.
> +	 */
>  	__u32 type;
> +	/**
> +	 * Can the PMU name be selected as if it were an event?
> +	 */
>  	bool selectable;
> +	/**
> +	 * Is the PMU not within the CPU core? Determined by the presence of
> +	 * <sysfs>/bus/event_source/devices/<name>/cpumask.
> +	 */
>  	bool is_uncore;
> +	/** Is the PMU name either cpu_core or cpu_atom. */

I don't think we want to limit the hybrid names only to cpu_core or
cpu_atom. Maybe something as below?
/* Is a hybrid CPU PMU, e.g., cpu_core, cpu_atom. */

Thanks,
Kan

>  	bool is_hybrid;
> +	/**
> +	 * Are events auxiliary events? Determined in architecture specific
> +	 * code.
> +	 */
>  	bool auxtrace;
> +	/**
> +	 * Number of levels of :ppp precision supported by the PMU, read from
> +	 * <sysfs>/bus/event_source/devices/<name>/caps/max_precise.
> +	 */
>  	int max_precise;
> +	/**
> +	 * Optional default perf_event_attr determined in architecture specific
> +	 * code.
> +	 */
>  	struct perf_event_attr *default_config;
> +	/**
> +	 * Empty or the contents of either of:
> +	 * <sysfs>/bus/event_source/devices/<name>/cpumask.
> +	 * <sysfs>/bus/event_source/devices/<cpu>/cpus.
> +	 */
>  	struct perf_cpu_map *cpus;
> -	struct list_head format;  /* HEAD struct perf_pmu_format -> list */
> -	struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
> +	/**
> +	 * Holds the contents of files read from
> +	 * <sysfs>/bus/event_source/devices/<name>/format/. The contents specify
> +	 * which event parameter changes what config, config1 or config2 bits.
> +	 */
> +	struct list_head format;
> +	/**
> +	 * List of struct perf_pmu_alias. Each alias corresponds to an event
> +	 * read from <sysfs>/bus/event_source/devices/<name>/events/ or from
> +	 * json events in pmu-events.c.
> +	 */
> +	struct list_head aliases;
> +	/** Has the list caps been initialized? */
>  	bool caps_initialized;
> +	/** The length of the list caps. */
>  	u32 nr_caps;
> -	struct list_head caps;    /* HEAD struct perf_pmu_caps -> list */
> -	struct list_head list;    /* ELEM */
> +	/**
> +	 * Holds the contents of files read from
> +	 * <sysfs>/bus/event_source/devices/<name>/caps/. The contents are pairs
> +	 * of the filename with the value of its contents, for example,
> +	 * max_precise (see above) may have a value of 3.
> +	 */
> +	struct list_head caps;
> +	/** Element on pmus list in pmu.c. */
> +	struct list_head list;
> +	/** Element on perf_pmu__hybrid_pmus. */
>  	struct list_head hybrid_list;
>  
> +	/** Features to inhibit when events on this PMU are opened. */
>  	struct {
> +		/** Disables perf_event_attr exclude_guest and exclude_host. */
>  		bool exclude_guest;
>  	} missing_features;
>  };
>  
> +/** A special global PMU used for testing. */
>  extern struct perf_pmu perf_pmu__fake;
>  
>  struct perf_pmu_info {
> @@ -71,21 +132,53 @@ struct perf_pmu_info {
>  
>  #define UNIT_MAX_LEN	31 /* max length for event unit name */
>  
> +/**
> + * An event either read from sysfs or builtin in pmu-events.c, created by
> + * parsing the pmu-events json files.
> + */
>  struct perf_pmu_alias {
>  	char *name;
> +	/** Optional short description of the event. */
>  	char *desc;
> +	/** Optional long description. */
>  	char *long_desc;
> +	/**
> +	 * Optional topic such as cache or pipeline, particularly for json
> +	 * events.
> +	 */
>  	char *topic;
> +	/** Comma separated parameter list. */
>  	char *str;
> -	struct list_head terms; /* HEAD struct parse_events_term -> list */
> -	struct list_head list;  /* ELEM */
> +	/** Owned list of the original parsed parameters. */
> +	struct list_head terms;
> +	/** List element of struct perf_pmu aliases. */
> +	struct list_head list;
> +	/** Units for the event, such as bytes or cache lines. */
>  	char unit[UNIT_MAX_LEN+1];
> +	/** Value to scale read counter values by. */
>  	double scale;
> +	/**
> +	 * Does the file
> +	 * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.per-pkg or
> +	 * equivalent json value exist and have the value 1.
> +	 */
>  	bool per_pkg;
> +	/**
> +	 * Does the file
> +	 * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.snapshot
> +	 * exist and have the value 1.
> +	 */
>  	bool snapshot;
> +	/** Is the event hidden and so not shown in perf list by default. */
>  	bool deprecated;
> +	/**
> +	 * A metric expression associated with an event. Doing this makes little
> +	 * sense due to scale and unit applying to both.
> +	 */
>  	char *metric_expr;
> +	/** A name for the metric. unit applying to both. */
>  	char *metric_name;
> +	/** The name copied from struct perf_pmu. */
>  	char *pmu_name;
>  };
>  

Re: [PATCH v1 1/9] perf pmu: Add documentation

[Ian Rogers]

On Mon, Nov 14, 2022 at 5:40 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2022-11-14 2:51 a.m., Ian Rogers wrote:
> > Add documentation to struct perf_pmu and the associated structs of
> > perf_pmu_alias and perf_pmu_format.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/pmu.c |  14 ++++++
> >  tools/perf/util/pmu.h | 105 +++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 113 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > index 6a86e6af0903..a8f9f47c6ed9 100644
> > --- a/tools/perf/util/pmu.c
> > +++ b/tools/perf/util/pmu.c
> > @@ -31,10 +31,24 @@
> >
> >  struct perf_pmu perf_pmu__fake;
> >
> > +/**
> > + * Values from a format file read from <sysfs>/devices/cpu/format/ held in
> > + * struct perf_pmu. For example, the contents of
> > + * <sysfs>/devices/cpu/format/event may be "config:0-7" and will be represented
> > + * here as name="event", value=PERF_PMU_FORMAT_VALUE_CONFIG and bits 0 to 7 will
> > + * be set.
> > + */
> >  struct perf_pmu_format {
> > +     /** The modifier/file name. */
> >       char *name;
> > +     /**
> > +      * Which config value the format relates to. Supported values are from
> > +      * PERF_PMU_FORMAT_VALUE_CONFIG to PERF_PMU_FORMAT_VALUE_CONFIG_END.
> > +      */
> >       int value;
> > +     /** Which config bits are set by this format value. */
> >       DECLARE_BITMAP(bits, PERF_PMU_FORMAT_BITS);
> > +     /** Element on list within struct perf_pmu. */
> >       struct list_head list;
> >  };
> >
> > diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> > index 68e15c38ae71..29571c0f9d15 100644
> > --- a/tools/perf/util/pmu.h
> > +++ b/tools/perf/util/pmu.h
> > @@ -34,30 +34,91 @@ struct perf_pmu_caps {
> >  };
> >
> >  struct perf_pmu {
> > +     /** The name of the PMU such as "cpu". */
> >       char *name;
> > +     /**
> > +      * Optional alternate name for the PMU determined in architecture
> > +      * specific code.
> > +      */
> >       char *alias_name;
> > +     /**
> > +      * Optional PMU identifier read from
> > +      * <sysfs>/bus/event_source/devices/<name>/identifier.
> > +      */
> >       char *id;
> > +     /**
> > +      * Perf event attributed type value, read from
> > +      * <sysfs>/bus/event_source/devices/<name>/type.
> > +      */
> >       __u32 type;
> > +     /**
> > +      * Can the PMU name be selected as if it were an event?
> > +      */
> >       bool selectable;
> > +     /**
> > +      * Is the PMU not within the CPU core? Determined by the presence of
> > +      * <sysfs>/bus/event_source/devices/<name>/cpumask.
> > +      */
> >       bool is_uncore;
> > +     /** Is the PMU name either cpu_core or cpu_atom. */
>
> I don't think we want to limit the hybrid names only to cpu_core or
> cpu_atom. Maybe something as below?
> /* Is a hybrid CPU PMU, e.g., cpu_core, cpu_atom. */

Currently the hybrid code only works for cpu_core or cpu_atom, a
limitation of its implementation. As pointed out in a later patch,
this bool isn't being used when it could be and I think we can work to
remove it. It would be possible to remove all uses of this with
perf_pmu__is_hybrid. As such I think it may be useful to mark the
hybrid variables in struct perf_pmu as deprecated while we work to
replace their use with more generic just any PMU code.

Thanks,
Ian

> Thanks,
> Kan
>
> >       bool is_hybrid;
> > +     /**
> > +      * Are events auxiliary events? Determined in architecture specific
> > +      * code.
> > +      */
> >       bool auxtrace;
> > +     /**
> > +      * Number of levels of :ppp precision supported by the PMU, read from
> > +      * <sysfs>/bus/event_source/devices/<name>/caps/max_precise.
> > +      */
> >       int max_precise;
> > +     /**
> > +      * Optional default perf_event_attr determined in architecture specific
> > +      * code.
> > +      */
> >       struct perf_event_attr *default_config;
> > +     /**
> > +      * Empty or the contents of either of:
> > +      * <sysfs>/bus/event_source/devices/<name>/cpumask.
> > +      * <sysfs>/bus/event_source/devices/<cpu>/cpus.
> > +      */
> >       struct perf_cpu_map *cpus;
> > -     struct list_head format;  /* HEAD struct perf_pmu_format -> list */
> > -     struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
> > +     /**
> > +      * Holds the contents of files read from
> > +      * <sysfs>/bus/event_source/devices/<name>/format/. The contents specify
> > +      * which event parameter changes what config, config1 or config2 bits.
> > +      */
> > +     struct list_head format;
> > +     /**
> > +      * List of struct perf_pmu_alias. Each alias corresponds to an event
> > +      * read from <sysfs>/bus/event_source/devices/<name>/events/ or from
> > +      * json events in pmu-events.c.
> > +      */
> > +     struct list_head aliases;
> > +     /** Has the list caps been initialized? */
> >       bool caps_initialized;
> > +     /** The length of the list caps. */
> >       u32 nr_caps;
> > -     struct list_head caps;    /* HEAD struct perf_pmu_caps -> list */
> > -     struct list_head list;    /* ELEM */
> > +     /**
> > +      * Holds the contents of files read from
> > +      * <sysfs>/bus/event_source/devices/<name>/caps/. The contents are pairs
> > +      * of the filename with the value of its contents, for example,
> > +      * max_precise (see above) may have a value of 3.
> > +      */
> > +     struct list_head caps;
> > +     /** Element on pmus list in pmu.c. */
> > +     struct list_head list;
> > +     /** Element on perf_pmu__hybrid_pmus. */
> >       struct list_head hybrid_list;
> >
> > +     /** Features to inhibit when events on this PMU are opened. */
> >       struct {
> > +             /** Disables perf_event_attr exclude_guest and exclude_host. */
> >               bool exclude_guest;
> >       } missing_features;
> >  };
> >
> > +/** A special global PMU used for testing. */
> >  extern struct perf_pmu perf_pmu__fake;
> >
> >  struct perf_pmu_info {
> > @@ -71,21 +132,53 @@ struct perf_pmu_info {
> >
> >  #define UNIT_MAX_LEN 31 /* max length for event unit name */
> >
> > +/**
> > + * An event either read from sysfs or builtin in pmu-events.c, created by
> > + * parsing the pmu-events json files.
> > + */
> >  struct perf_pmu_alias {
> >       char *name;
> > +     /** Optional short description of the event. */
> >       char *desc;
> > +     /** Optional long description. */
> >       char *long_desc;
> > +     /**
> > +      * Optional topic such as cache or pipeline, particularly for json
> > +      * events.
> > +      */
> >       char *topic;
> > +     /** Comma separated parameter list. */
> >       char *str;
> > -     struct list_head terms; /* HEAD struct parse_events_term -> list */
> > -     struct list_head list;  /* ELEM */
> > +     /** Owned list of the original parsed parameters. */
> > +     struct list_head terms;
> > +     /** List element of struct perf_pmu aliases. */
> > +     struct list_head list;
> > +     /** Units for the event, such as bytes or cache lines. */
> >       char unit[UNIT_MAX_LEN+1];
> > +     /** Value to scale read counter values by. */
> >       double scale;
> > +     /**
> > +      * Does the file
> > +      * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.per-pkg or
> > +      * equivalent json value exist and have the value 1.
> > +      */
> >       bool per_pkg;
> > +     /**
> > +      * Does the file
> > +      * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.snapshot
> > +      * exist and have the value 1.
> > +      */
> >       bool snapshot;
> > +     /** Is the event hidden and so not shown in perf list by default. */
> >       bool deprecated;
> > +     /**
> > +      * A metric expression associated with an event. Doing this makes little
> > +      * sense due to scale and unit applying to both.
> > +      */
> >       char *metric_expr;
> > +     /** A name for the metric. unit applying to both. */
> >       char *metric_name;
> > +     /** The name copied from struct perf_pmu. */
> >       char *pmu_name;
> >  };
> >

Re: [PATCH v1 1/9] perf pmu: Add documentation

[Liang, Kan]

On 2022-11-14 9:09 a.m., Ian Rogers wrote:
> On Mon, Nov 14, 2022 at 5:40 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>
>>
>> On 2022-11-14 2:51 a.m., Ian Rogers wrote:
>>> Add documentation to struct perf_pmu and the associated structs of
>>> perf_pmu_alias and perf_pmu_format.
>>>
>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>> ---
>>>  tools/perf/util/pmu.c |  14 ++++++
>>>  tools/perf/util/pmu.h | 105 +++++++++++++++++++++++++++++++++++++++---
>>>  2 files changed, 113 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>>> index 6a86e6af0903..a8f9f47c6ed9 100644
>>> --- a/tools/perf/util/pmu.c
>>> +++ b/tools/perf/util/pmu.c
>>> @@ -31,10 +31,24 @@
>>>
>>>  struct perf_pmu perf_pmu__fake;
>>>
>>> +/**
>>> + * Values from a format file read from <sysfs>/devices/cpu/format/ held in
>>> + * struct perf_pmu. For example, the contents of
>>> + * <sysfs>/devices/cpu/format/event may be "config:0-7" and will be represented
>>> + * here as name="event", value=PERF_PMU_FORMAT_VALUE_CONFIG and bits 0 to 7 will
>>> + * be set.
>>> + */
>>>  struct perf_pmu_format {
>>> +     /** The modifier/file name. */
>>>       char *name;
>>> +     /**
>>> +      * Which config value the format relates to. Supported values are from
>>> +      * PERF_PMU_FORMAT_VALUE_CONFIG to PERF_PMU_FORMAT_VALUE_CONFIG_END.
>>> +      */
>>>       int value;
>>> +     /** Which config bits are set by this format value. */
>>>       DECLARE_BITMAP(bits, PERF_PMU_FORMAT_BITS);
>>> +     /** Element on list within struct perf_pmu. */
>>>       struct list_head list;
>>>  };
>>>
>>> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
>>> index 68e15c38ae71..29571c0f9d15 100644
>>> --- a/tools/perf/util/pmu.h
>>> +++ b/tools/perf/util/pmu.h
>>> @@ -34,30 +34,91 @@ struct perf_pmu_caps {
>>>  };
>>>
>>>  struct perf_pmu {
>>> +     /** The name of the PMU such as "cpu". */
>>>       char *name;
>>> +     /**
>>> +      * Optional alternate name for the PMU determined in architecture
>>> +      * specific code.
>>> +      */
>>>       char *alias_name;
>>> +     /**
>>> +      * Optional PMU identifier read from
>>> +      * <sysfs>/bus/event_source/devices/<name>/identifier.
>>> +      */
>>>       char *id;
>>> +     /**
>>> +      * Perf event attributed type value, read from
>>> +      * <sysfs>/bus/event_source/devices/<name>/type.
>>> +      */
>>>       __u32 type;
>>> +     /**
>>> +      * Can the PMU name be selected as if it were an event?
>>> +      */
>>>       bool selectable;
>>> +     /**
>>> +      * Is the PMU not within the CPU core? Determined by the presence of
>>> +      * <sysfs>/bus/event_source/devices/<name>/cpumask.
>>> +      */
>>>       bool is_uncore;
>>> +     /** Is the PMU name either cpu_core or cpu_atom. */
>>
>> I don't think we want to limit the hybrid names only to cpu_core or
>> cpu_atom. Maybe something as below?
>> /* Is a hybrid CPU PMU, e.g., cpu_core, cpu_atom. */
> 
> Currently the hybrid code only works for cpu_core or cpu_atom, a
> limitation of its implementation. 

I don't think so. See perf_pmu__hybrid_mounted(). If a PMU is named as
"cpu_", we treat it as a hybrid CPU PMU. The cpu_core or cpu_atom should
only be hard coded to specially handle some model-specific cases, e.g.,
mem-loads-aux event.

Thanks,
Kan

> As pointed out in a later patch,
> this bool isn't being used when it could be and I think we can work to
> remove it. It would be possible to remove all uses of this with
> perf_pmu__is_hybrid. As such I think it may be useful to mark the
> hybrid variables in struct perf_pmu as deprecated while we work to
> replace their use with more generic just any PMU code.
> 
> Thanks,
> Ian
> 
>> Thanks,
>> Kan
>>
>>>       bool is_hybrid;
>>> +     /**
>>> +      * Are events auxiliary events? Determined in architecture specific
>>> +      * code.
>>> +      */
>>>       bool auxtrace;
>>> +     /**
>>> +      * Number of levels of :ppp precision supported by the PMU, read from
>>> +      * <sysfs>/bus/event_source/devices/<name>/caps/max_precise.
>>> +      */
>>>       int max_precise;
>>> +     /**
>>> +      * Optional default perf_event_attr determined in architecture specific
>>> +      * code.
>>> +      */
>>>       struct perf_event_attr *default_config;
>>> +     /**
>>> +      * Empty or the contents of either of:
>>> +      * <sysfs>/bus/event_source/devices/<name>/cpumask.
>>> +      * <sysfs>/bus/event_source/devices/<cpu>/cpus.
>>> +      */
>>>       struct perf_cpu_map *cpus;
>>> -     struct list_head format;  /* HEAD struct perf_pmu_format -> list */
>>> -     struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
>>> +     /**
>>> +      * Holds the contents of files read from
>>> +      * <sysfs>/bus/event_source/devices/<name>/format/. The contents specify
>>> +      * which event parameter changes what config, config1 or config2 bits.
>>> +      */
>>> +     struct list_head format;
>>> +     /**
>>> +      * List of struct perf_pmu_alias. Each alias corresponds to an event
>>> +      * read from <sysfs>/bus/event_source/devices/<name>/events/ or from
>>> +      * json events in pmu-events.c.
>>> +      */
>>> +     struct list_head aliases;
>>> +     /** Has the list caps been initialized? */
>>>       bool caps_initialized;
>>> +     /** The length of the list caps. */
>>>       u32 nr_caps;
>>> -     struct list_head caps;    /* HEAD struct perf_pmu_caps -> list */
>>> -     struct list_head list;    /* ELEM */
>>> +     /**
>>> +      * Holds the contents of files read from
>>> +      * <sysfs>/bus/event_source/devices/<name>/caps/. The contents are pairs
>>> +      * of the filename with the value of its contents, for example,
>>> +      * max_precise (see above) may have a value of 3.
>>> +      */
>>> +     struct list_head caps;
>>> +     /** Element on pmus list in pmu.c. */
>>> +     struct list_head list;
>>> +     /** Element on perf_pmu__hybrid_pmus. */
>>>       struct list_head hybrid_list;
>>>
>>> +     /** Features to inhibit when events on this PMU are opened. */
>>>       struct {
>>> +             /** Disables perf_event_attr exclude_guest and exclude_host. */
>>>               bool exclude_guest;
>>>       } missing_features;
>>>  };
>>>
>>> +/** A special global PMU used for testing. */
>>>  extern struct perf_pmu perf_pmu__fake;
>>>
>>>  struct perf_pmu_info {
>>> @@ -71,21 +132,53 @@ struct perf_pmu_info {
>>>
>>>  #define UNIT_MAX_LEN 31 /* max length for event unit name */
>>>
>>> +/**
>>> + * An event either read from sysfs or builtin in pmu-events.c, created by
>>> + * parsing the pmu-events json files.
>>> + */
>>>  struct perf_pmu_alias {
>>>       char *name;
>>> +     /** Optional short description of the event. */
>>>       char *desc;
>>> +     /** Optional long description. */
>>>       char *long_desc;
>>> +     /**
>>> +      * Optional topic such as cache or pipeline, particularly for json
>>> +      * events.
>>> +      */
>>>       char *topic;
>>> +     /** Comma separated parameter list. */
>>>       char *str;
>>> -     struct list_head terms; /* HEAD struct parse_events_term -> list */
>>> -     struct list_head list;  /* ELEM */
>>> +     /** Owned list of the original parsed parameters. */
>>> +     struct list_head terms;
>>> +     /** List element of struct perf_pmu aliases. */
>>> +     struct list_head list;
>>> +     /** Units for the event, such as bytes or cache lines. */
>>>       char unit[UNIT_MAX_LEN+1];
>>> +     /** Value to scale read counter values by. */
>>>       double scale;
>>> +     /**
>>> +      * Does the file
>>> +      * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.per-pkg or
>>> +      * equivalent json value exist and have the value 1.
>>> +      */
>>>       bool per_pkg;
>>> +     /**
>>> +      * Does the file
>>> +      * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.snapshot
>>> +      * exist and have the value 1.
>>> +      */
>>>       bool snapshot;
>>> +     /** Is the event hidden and so not shown in perf list by default. */
>>>       bool deprecated;
>>> +     /**
>>> +      * A metric expression associated with an event. Doing this makes little
>>> +      * sense due to scale and unit applying to both.
>>> +      */
>>>       char *metric_expr;
>>> +     /** A name for the metric. unit applying to both. */
>>>       char *metric_name;
>>> +     /** The name copied from struct perf_pmu. */
>>>       char *pmu_name;
>>>  };
>>>

Re: [PATCH v1 1/9] perf pmu: Add documentation

[Ian Rogers]

On Mon, Nov 14, 2022 at 7:26 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2022-11-14 9:09 a.m., Ian Rogers wrote:
> > On Mon, Nov 14, 2022 at 5:40 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >>
> >>
> >>
> >> On 2022-11-14 2:51 a.m., Ian Rogers wrote:
> >>> Add documentation to struct perf_pmu and the associated structs of
> >>> perf_pmu_alias and perf_pmu_format.
> >>>
> >>> Signed-off-by: Ian Rogers <irogers@google.com>
> >>> ---
> >>>  tools/perf/util/pmu.c |  14 ++++++
> >>>  tools/perf/util/pmu.h | 105 +++++++++++++++++++++++++++++++++++++++---
> >>>  2 files changed, 113 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> >>> index 6a86e6af0903..a8f9f47c6ed9 100644
> >>> --- a/tools/perf/util/pmu.c
> >>> +++ b/tools/perf/util/pmu.c
> >>> @@ -31,10 +31,24 @@
> >>>
> >>>  struct perf_pmu perf_pmu__fake;
> >>>
> >>> +/**
> >>> + * Values from a format file read from <sysfs>/devices/cpu/format/ held in
> >>> + * struct perf_pmu. For example, the contents of
> >>> + * <sysfs>/devices/cpu/format/event may be "config:0-7" and will be represented
> >>> + * here as name="event", value=PERF_PMU_FORMAT_VALUE_CONFIG and bits 0 to 7 will
> >>> + * be set.
> >>> + */
> >>>  struct perf_pmu_format {
> >>> +     /** The modifier/file name. */
> >>>       char *name;
> >>> +     /**
> >>> +      * Which config value the format relates to. Supported values are from
> >>> +      * PERF_PMU_FORMAT_VALUE_CONFIG to PERF_PMU_FORMAT_VALUE_CONFIG_END.
> >>> +      */
> >>>       int value;
> >>> +     /** Which config bits are set by this format value. */
> >>>       DECLARE_BITMAP(bits, PERF_PMU_FORMAT_BITS);
> >>> +     /** Element on list within struct perf_pmu. */
> >>>       struct list_head list;
> >>>  };
> >>>
> >>> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> >>> index 68e15c38ae71..29571c0f9d15 100644
> >>> --- a/tools/perf/util/pmu.h
> >>> +++ b/tools/perf/util/pmu.h
> >>> @@ -34,30 +34,91 @@ struct perf_pmu_caps {
> >>>  };
> >>>
> >>>  struct perf_pmu {
> >>> +     /** The name of the PMU such as "cpu". */
> >>>       char *name;
> >>> +     /**
> >>> +      * Optional alternate name for the PMU determined in architecture
> >>> +      * specific code.
> >>> +      */
> >>>       char *alias_name;
> >>> +     /**
> >>> +      * Optional PMU identifier read from
> >>> +      * <sysfs>/bus/event_source/devices/<name>/identifier.
> >>> +      */
> >>>       char *id;
> >>> +     /**
> >>> +      * Perf event attributed type value, read from
> >>> +      * <sysfs>/bus/event_source/devices/<name>/type.
> >>> +      */
> >>>       __u32 type;
> >>> +     /**
> >>> +      * Can the PMU name be selected as if it were an event?
> >>> +      */
> >>>       bool selectable;
> >>> +     /**
> >>> +      * Is the PMU not within the CPU core? Determined by the presence of
> >>> +      * <sysfs>/bus/event_source/devices/<name>/cpumask.
> >>> +      */
> >>>       bool is_uncore;
> >>> +     /** Is the PMU name either cpu_core or cpu_atom. */
> >>
> >> I don't think we want to limit the hybrid names only to cpu_core or
> >> cpu_atom. Maybe something as below?
> >> /* Is a hybrid CPU PMU, e.g., cpu_core, cpu_atom. */
> >
> > Currently the hybrid code only works for cpu_core or cpu_atom, a
> > limitation of its implementation.
>
> I don't think so. See perf_pmu__hybrid_mounted(). If a PMU is named as
> "cpu_", we treat it as a hybrid CPU PMU. The cpu_core or cpu_atom should
> only be hard coded to specially handle some model-specific cases, e.g.,
> mem-loads-aux event.

Ugh.. Why is a property about something being a CPU named in an Intel
specific term for big little? Why wasn't this generalized? Why are CPU
PMUs assumed to be prefixed cpu_* .. ? The comment is factually
correct, we can hold off deprecating it on the assumption that it'd be
renamed to something more appropriate like is_cpu.

Thanks,
Ian

> Thanks,
> Kan
>
> > As pointed out in a later patch,
> > this bool isn't being used when it could be and I think we can work to
> > remove it. It would be possible to remove all uses of this with
> > perf_pmu__is_hybrid. As such I think it may be useful to mark the
> > hybrid variables in struct perf_pmu as deprecated while we work to
> > replace their use with more generic just any PMU code.
> >
> > Thanks,
> > Ian
> >
> >> Thanks,
> >> Kan
> >>
> >>>       bool is_hybrid;
> >>> +     /**
> >>> +      * Are events auxiliary events? Determined in architecture specific
> >>> +      * code.
> >>> +      */
> >>>       bool auxtrace;
> >>> +     /**
> >>> +      * Number of levels of :ppp precision supported by the PMU, read from
> >>> +      * <sysfs>/bus/event_source/devices/<name>/caps/max_precise.
> >>> +      */
> >>>       int max_precise;
> >>> +     /**
> >>> +      * Optional default perf_event_attr determined in architecture specific
> >>> +      * code.
> >>> +      */
> >>>       struct perf_event_attr *default_config;
> >>> +     /**
> >>> +      * Empty or the contents of either of:
> >>> +      * <sysfs>/bus/event_source/devices/<name>/cpumask.
> >>> +      * <sysfs>/bus/event_source/devices/<cpu>/cpus.
> >>> +      */
> >>>       struct perf_cpu_map *cpus;
> >>> -     struct list_head format;  /* HEAD struct perf_pmu_format -> list */
> >>> -     struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
> >>> +     /**
> >>> +      * Holds the contents of files read from
> >>> +      * <sysfs>/bus/event_source/devices/<name>/format/. The contents specify
> >>> +      * which event parameter changes what config, config1 or config2 bits.
> >>> +      */
> >>> +     struct list_head format;
> >>> +     /**
> >>> +      * List of struct perf_pmu_alias. Each alias corresponds to an event
> >>> +      * read from <sysfs>/bus/event_source/devices/<name>/events/ or from
> >>> +      * json events in pmu-events.c.
> >>> +      */
> >>> +     struct list_head aliases;
> >>> +     /** Has the list caps been initialized? */
> >>>       bool caps_initialized;
> >>> +     /** The length of the list caps. */
> >>>       u32 nr_caps;
> >>> -     struct list_head caps;    /* HEAD struct perf_pmu_caps -> list */
> >>> -     struct list_head list;    /* ELEM */
> >>> +     /**
> >>> +      * Holds the contents of files read from
> >>> +      * <sysfs>/bus/event_source/devices/<name>/caps/. The contents are pairs
> >>> +      * of the filename with the value of its contents, for example,
> >>> +      * max_precise (see above) may have a value of 3.
> >>> +      */
> >>> +     struct list_head caps;
> >>> +     /** Element on pmus list in pmu.c. */
> >>> +     struct list_head list;
> >>> +     /** Element on perf_pmu__hybrid_pmus. */
> >>>       struct list_head hybrid_list;
> >>>
> >>> +     /** Features to inhibit when events on this PMU are opened. */
> >>>       struct {
> >>> +             /** Disables perf_event_attr exclude_guest and exclude_host. */
> >>>               bool exclude_guest;
> >>>       } missing_features;
> >>>  };
> >>>
> >>> +/** A special global PMU used for testing. */
> >>>  extern struct perf_pmu perf_pmu__fake;
> >>>
> >>>  struct perf_pmu_info {
> >>> @@ -71,21 +132,53 @@ struct perf_pmu_info {
> >>>
> >>>  #define UNIT_MAX_LEN 31 /* max length for event unit name */
> >>>
> >>> +/**
> >>> + * An event either read from sysfs or builtin in pmu-events.c, created by
> >>> + * parsing the pmu-events json files.
> >>> + */
> >>>  struct perf_pmu_alias {
> >>>       char *name;
> >>> +     /** Optional short description of the event. */
> >>>       char *desc;
> >>> +     /** Optional long description. */
> >>>       char *long_desc;
> >>> +     /**
> >>> +      * Optional topic such as cache or pipeline, particularly for json
> >>> +      * events.
> >>> +      */
> >>>       char *topic;
> >>> +     /** Comma separated parameter list. */
> >>>       char *str;
> >>> -     struct list_head terms; /* HEAD struct parse_events_term -> list */
> >>> -     struct list_head list;  /* ELEM */
> >>> +     /** Owned list of the original parsed parameters. */
> >>> +     struct list_head terms;
> >>> +     /** List element of struct perf_pmu aliases. */
> >>> +     struct list_head list;
> >>> +     /** Units for the event, such as bytes or cache lines. */
> >>>       char unit[UNIT_MAX_LEN+1];
> >>> +     /** Value to scale read counter values by. */
> >>>       double scale;
> >>> +     /**
> >>> +      * Does the file
> >>> +      * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.per-pkg or
> >>> +      * equivalent json value exist and have the value 1.
> >>> +      */
> >>>       bool per_pkg;
> >>> +     /**
> >>> +      * Does the file
> >>> +      * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.snapshot
> >>> +      * exist and have the value 1.
> >>> +      */
> >>>       bool snapshot;
> >>> +     /** Is the event hidden and so not shown in perf list by default. */
> >>>       bool deprecated;
> >>> +     /**
> >>> +      * A metric expression associated with an event. Doing this makes little
> >>> +      * sense due to scale and unit applying to both.
> >>> +      */
> >>>       char *metric_expr;
> >>> +     /** A name for the metric. unit applying to both. */
> >>>       char *metric_name;
> >>> +     /** The name copied from struct perf_pmu. */
> >>>       char *pmu_name;
> >>>  };
> >>>

Re: [PATCH v1 1/9] perf pmu: Add documentation

[Liang, Kan]

On 2022-11-14 12:04 p.m., Ian Rogers wrote:
> On Mon, Nov 14, 2022 at 7:26 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>
>>
>> On 2022-11-14 9:09 a.m., Ian Rogers wrote:
>>> On Mon, Nov 14, 2022 at 5:40 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2022-11-14 2:51 a.m., Ian Rogers wrote:
>>>>> Add documentation to struct perf_pmu and the associated structs of
>>>>> perf_pmu_alias and perf_pmu_format.
>>>>>
>>>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>>>> ---
>>>>>  tools/perf/util/pmu.c |  14 ++++++
>>>>>  tools/perf/util/pmu.h | 105 +++++++++++++++++++++++++++++++++++++++---
>>>>>  2 files changed, 113 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>>>>> index 6a86e6af0903..a8f9f47c6ed9 100644
>>>>> --- a/tools/perf/util/pmu.c
>>>>> +++ b/tools/perf/util/pmu.c
>>>>> @@ -31,10 +31,24 @@
>>>>>
>>>>>  struct perf_pmu perf_pmu__fake;
>>>>>
>>>>> +/**
>>>>> + * Values from a format file read from <sysfs>/devices/cpu/format/ held in
>>>>> + * struct perf_pmu. For example, the contents of
>>>>> + * <sysfs>/devices/cpu/format/event may be "config:0-7" and will be represented
>>>>> + * here as name="event", value=PERF_PMU_FORMAT_VALUE_CONFIG and bits 0 to 7 will
>>>>> + * be set.
>>>>> + */
>>>>>  struct perf_pmu_format {
>>>>> +     /** The modifier/file name. */
>>>>>       char *name;
>>>>> +     /**
>>>>> +      * Which config value the format relates to. Supported values are from
>>>>> +      * PERF_PMU_FORMAT_VALUE_CONFIG to PERF_PMU_FORMAT_VALUE_CONFIG_END.
>>>>> +      */
>>>>>       int value;
>>>>> +     /** Which config bits are set by this format value. */
>>>>>       DECLARE_BITMAP(bits, PERF_PMU_FORMAT_BITS);
>>>>> +     /** Element on list within struct perf_pmu. */
>>>>>       struct list_head list;
>>>>>  };
>>>>>
>>>>> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
>>>>> index 68e15c38ae71..29571c0f9d15 100644
>>>>> --- a/tools/perf/util/pmu.h
>>>>> +++ b/tools/perf/util/pmu.h
>>>>> @@ -34,30 +34,91 @@ struct perf_pmu_caps {
>>>>>  };
>>>>>
>>>>>  struct perf_pmu {
>>>>> +     /** The name of the PMU such as "cpu". */
>>>>>       char *name;
>>>>> +     /**
>>>>> +      * Optional alternate name for the PMU determined in architecture
>>>>> +      * specific code.
>>>>> +      */
>>>>>       char *alias_name;
>>>>> +     /**
>>>>> +      * Optional PMU identifier read from
>>>>> +      * <sysfs>/bus/event_source/devices/<name>/identifier.
>>>>> +      */
>>>>>       char *id;
>>>>> +     /**
>>>>> +      * Perf event attributed type value, read from
>>>>> +      * <sysfs>/bus/event_source/devices/<name>/type.
>>>>> +      */
>>>>>       __u32 type;
>>>>> +     /**
>>>>> +      * Can the PMU name be selected as if it were an event?
>>>>> +      */
>>>>>       bool selectable;
>>>>> +     /**
>>>>> +      * Is the PMU not within the CPU core? Determined by the presence of
>>>>> +      * <sysfs>/bus/event_source/devices/<name>/cpumask.
>>>>> +      */
>>>>>       bool is_uncore;
>>>>> +     /** Is the PMU name either cpu_core or cpu_atom. */
>>>>
>>>> I don't think we want to limit the hybrid names only to cpu_core or
>>>> cpu_atom. Maybe something as below?
>>>> /* Is a hybrid CPU PMU, e.g., cpu_core, cpu_atom. */
>>>
>>> Currently the hybrid code only works for cpu_core or cpu_atom, a
>>> limitation of its implementation.
>>
>> I don't think so. See perf_pmu__hybrid_mounted(). If a PMU is named as
>> "cpu_", we treat it as a hybrid CPU PMU. The cpu_core or cpu_atom should
>> only be hard coded to specially handle some model-specific cases, e.g.,
>> mem-loads-aux event.
> 
> Ugh.. Why is a property about something being a CPU named in an Intel
> specific term for big little? Why wasn't this generalized? Why are CPU
> PMUs assumed to be prefixed cpu_* .. ? The comment is factually
> correct, we can hold off deprecating it on the assumption that it'd be
> renamed to something more appropriate like is_cpu.

Does any code really use the pmu->is_hybrid?
From what I read, it's only used in the pmu_lookup(), where it's
assigned. It looks like we don't need it in the struct perf_pmu. If so,
let's remove it completely from struct perf_pmu.

Thanks,
Kan
> 
> Thanks,
> Ian
> 
>> Thanks,
>> Kan
>>
>>> As pointed out in a later patch,
>>> this bool isn't being used when it could be and I think we can work to
>>> remove it. It would be possible to remove all uses of this with
>>> perf_pmu__is_hybrid. As such I think it may be useful to mark the
>>> hybrid variables in struct perf_pmu as deprecated while we work to
>>> replace their use with more generic just any PMU code.
>>>
>>> Thanks,
>>> Ian
>>>
>>>> Thanks,
>>>> Kan
>>>>
>>>>>       bool is_hybrid;
>>>>> +     /**
>>>>> +      * Are events auxiliary events? Determined in architecture specific
>>>>> +      * code.
>>>>> +      */
>>>>>       bool auxtrace;
>>>>> +     /**
>>>>> +      * Number of levels of :ppp precision supported by the PMU, read from
>>>>> +      * <sysfs>/bus/event_source/devices/<name>/caps/max_precise.
>>>>> +      */
>>>>>       int max_precise;
>>>>> +     /**
>>>>> +      * Optional default perf_event_attr determined in architecture specific
>>>>> +      * code.
>>>>> +      */
>>>>>       struct perf_event_attr *default_config;
>>>>> +     /**
>>>>> +      * Empty or the contents of either of:
>>>>> +      * <sysfs>/bus/event_source/devices/<name>/cpumask.
>>>>> +      * <sysfs>/bus/event_source/devices/<cpu>/cpus.
>>>>> +      */
>>>>>       struct perf_cpu_map *cpus;
>>>>> -     struct list_head format;  /* HEAD struct perf_pmu_format -> list */
>>>>> -     struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
>>>>> +     /**
>>>>> +      * Holds the contents of files read from
>>>>> +      * <sysfs>/bus/event_source/devices/<name>/format/. The contents specify
>>>>> +      * which event parameter changes what config, config1 or config2 bits.
>>>>> +      */
>>>>> +     struct list_head format;
>>>>> +     /**
>>>>> +      * List of struct perf_pmu_alias. Each alias corresponds to an event
>>>>> +      * read from <sysfs>/bus/event_source/devices/<name>/events/ or from
>>>>> +      * json events in pmu-events.c.
>>>>> +      */
>>>>> +     struct list_head aliases;
>>>>> +     /** Has the list caps been initialized? */
>>>>>       bool caps_initialized;
>>>>> +     /** The length of the list caps. */
>>>>>       u32 nr_caps;
>>>>> -     struct list_head caps;    /* HEAD struct perf_pmu_caps -> list */
>>>>> -     struct list_head list;    /* ELEM */
>>>>> +     /**
>>>>> +      * Holds the contents of files read from
>>>>> +      * <sysfs>/bus/event_source/devices/<name>/caps/. The contents are pairs
>>>>> +      * of the filename with the value of its contents, for example,
>>>>> +      * max_precise (see above) may have a value of 3.
>>>>> +      */
>>>>> +     struct list_head caps;
>>>>> +     /** Element on pmus list in pmu.c. */
>>>>> +     struct list_head list;
>>>>> +     /** Element on perf_pmu__hybrid_pmus. */
>>>>>       struct list_head hybrid_list;
>>>>>
>>>>> +     /** Features to inhibit when events on this PMU are opened. */
>>>>>       struct {
>>>>> +             /** Disables perf_event_attr exclude_guest and exclude_host. */
>>>>>               bool exclude_guest;
>>>>>       } missing_features;
>>>>>  };
>>>>>
>>>>> +/** A special global PMU used for testing. */
>>>>>  extern struct perf_pmu perf_pmu__fake;
>>>>>
>>>>>  struct perf_pmu_info {
>>>>> @@ -71,21 +132,53 @@ struct perf_pmu_info {
>>>>>
>>>>>  #define UNIT_MAX_LEN 31 /* max length for event unit name */
>>>>>
>>>>> +/**
>>>>> + * An event either read from sysfs or builtin in pmu-events.c, created by
>>>>> + * parsing the pmu-events json files.
>>>>> + */
>>>>>  struct perf_pmu_alias {
>>>>>       char *name;
>>>>> +     /** Optional short description of the event. */
>>>>>       char *desc;
>>>>> +     /** Optional long description. */
>>>>>       char *long_desc;
>>>>> +     /**
>>>>> +      * Optional topic such as cache or pipeline, particularly for json
>>>>> +      * events.
>>>>> +      */
>>>>>       char *topic;
>>>>> +     /** Comma separated parameter list. */
>>>>>       char *str;
>>>>> -     struct list_head terms; /* HEAD struct parse_events_term -> list */
>>>>> -     struct list_head list;  /* ELEM */
>>>>> +     /** Owned list of the original parsed parameters. */
>>>>> +     struct list_head terms;
>>>>> +     /** List element of struct perf_pmu aliases. */
>>>>> +     struct list_head list;
>>>>> +     /** Units for the event, such as bytes or cache lines. */
>>>>>       char unit[UNIT_MAX_LEN+1];
>>>>> +     /** Value to scale read counter values by. */
>>>>>       double scale;
>>>>> +     /**
>>>>> +      * Does the file
>>>>> +      * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.per-pkg or
>>>>> +      * equivalent json value exist and have the value 1.
>>>>> +      */
>>>>>       bool per_pkg;
>>>>> +     /**
>>>>> +      * Does the file
>>>>> +      * <sysfs>/bus/event_source/devices/<pmu_name>/events/<name>.snapshot
>>>>> +      * exist and have the value 1.
>>>>> +      */
>>>>>       bool snapshot;
>>>>> +     /** Is the event hidden and so not shown in perf list by default. */
>>>>>       bool deprecated;
>>>>> +     /**
>>>>> +      * A metric expression associated with an event. Doing this makes little
>>>>> +      * sense due to scale and unit applying to both.
>>>>> +      */
>>>>>       char *metric_expr;
>>>>> +     /** A name for the metric. unit applying to both. */
>>>>>       char *metric_name;
>>>>> +     /** The name copied from struct perf_pmu. */
>>>>>       char *pmu_name;
>>>>>  };
>>>>>

[PATCH v1 2/9] tools lib api fs tracing_path: Add scandir alphasort

[Ian Rogers]

tracing_events__opendir allows iteration over files in
<debugfs>/tracing/events but with an arbitrary sort order. Add a
scandir alternative where the results are alphabetically sorted.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/api/fs/tracing_path.c | 16 ++++++++++++++++
 tools/lib/api/fs/tracing_path.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/tools/lib/api/fs/tracing_path.c b/tools/lib/api/fs/tracing_path.c
index 5afb11b30fca..b8e457c841ab 100644
--- a/tools/lib/api/fs/tracing_path.c
+++ b/tools/lib/api/fs/tracing_path.c
@@ -113,6 +113,22 @@ DIR *tracing_events__opendir(void)
 	return dir;
 }
 
+int tracing_events__scandir_alphasort(struct dirent ***namelist)
+{
+	char *path = get_tracing_file("events");
+	int ret;
+
+	if (!path) {
+		*namelist = NULL;
+		return 0;
+	}
+
+	ret = scandir(path, namelist, NULL, alphasort);
+	put_events_file(path);
+
+	return ret;
+}
+
 int tracing_path__strerror_open_tp(int err, char *buf, size_t size,
 				   const char *sys, const char *name)
 {
diff --git a/tools/lib/api/fs/tracing_path.h b/tools/lib/api/fs/tracing_path.h
index a19136b086dc..fc6347c11deb 100644
--- a/tools/lib/api/fs/tracing_path.h
+++ b/tools/lib/api/fs/tracing_path.h
@@ -6,6 +6,7 @@
 #include <dirent.h>
 
 DIR *tracing_events__opendir(void);
+int tracing_events__scandir_alphasort(struct dirent ***namelist);
 
 void tracing_path_set(const char *mountpoint);
 const char *tracing_path_mount(void);
-- 
2.38.1.431.g37b22c650d-goog

[PATCH v1 3/9] perf tracepoint: Sort events in iterator

[Ian Rogers]

In print_tracepoint_events use tracing_events__scandir_alphasort and
scandir alphasort so that the subsystem and events are sorted and
don't need a secondary qsort. Locally this results in the following
change:

...
   ext4:ext4_zero_range                               [Tracepoint event]
-  fib6:fib6_table_lookup                             [Tracepoint event]
   fib:fib_table_lookup                               [Tracepoint event]
+  fib6:fib6_table_lookup                             [Tracepoint event]
   filelock:break_lease_block                         [Tracepoint event]
...

ie fib6 now is after fib and not before it. This is more consistent
with how numbers are more generally sorted, such as:

...
  syscalls:sys_enter_renameat                        [Tracepoint event]
  syscalls:sys_enter_renameat2                       [Tracepoint event]
...

and so an improvement over the qsort approach.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/print-events.c | 108 +++++++++++----------------------
 1 file changed, 37 insertions(+), 71 deletions(-)

diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
index c4d5d87fae2f..fefc025bc259 100644
--- a/tools/perf/util/print-events.c
+++ b/tools/perf/util/print-events.c
@@ -66,26 +66,21 @@ static int cmp_string(const void *a, const void *b)
 void print_tracepoint_events(const char *subsys_glob,
 			     const char *event_glob, bool name_only)
 {
-	DIR *sys_dir, *evt_dir;
-	struct dirent *sys_dirent, *evt_dirent;
-	char evt_path[MAXPATHLEN];
-	char *dir_path;
-	char **evt_list = NULL;
-	unsigned int evt_i = 0, evt_num = 0;
-	bool evt_num_known = false;
-
-restart:
-	sys_dir = tracing_events__opendir();
-	if (!sys_dir)
-		return;
-
-	if (evt_num_known) {
-		evt_list = zalloc(sizeof(char *) * evt_num);
-		if (!evt_list)
-			goto out_close_sys_dir;
-	}
+	struct dirent **sys_namelist = NULL;
+	bool printed = false;
+	int sys_items = tracing_events__scandir_alphasort(&sys_namelist);
+
+	for (int i = 0; i < sys_items; i++) {
+		struct dirent *sys_dirent = sys_namelist[i];
+		struct dirent **evt_namelist = NULL;
+		char *dir_path;
+		int evt_items;
+
+		if (sys_dirent->d_type != DT_DIR ||
+		    !strcmp(sys_dirent->d_name, ".") ||
+		    !strcmp(sys_dirent->d_name, ".."))
+			continue;
 
-	for_each_subsystem(sys_dir, sys_dirent) {
 		if (subsys_glob != NULL &&
 		    !strglobmatch(sys_dirent->d_name, subsys_glob))
 			continue;
@@ -93,69 +88,40 @@ void print_tracepoint_events(const char *subsys_glob,
 		dir_path = get_events_file(sys_dirent->d_name);
 		if (!dir_path)
 			continue;
-		evt_dir = opendir(dir_path);
-		if (!evt_dir)
-			goto next;
 
-		for_each_event(dir_path, evt_dir, evt_dirent) {
-			if (event_glob != NULL &&
-			    !strglobmatch(evt_dirent->d_name, event_glob))
+		evt_items = scandir(dir_path, &evt_namelist, NULL, alphasort);
+		for (int j = 0; j < evt_items; j++) {
+			struct dirent *evt_dirent = evt_namelist[j];
+			char evt_path[MAXPATHLEN];
+
+			if (evt_dirent->d_type != DT_DIR ||
+			    !strcmp(evt_dirent->d_name, ".") ||
+			    !strcmp(evt_dirent->d_name, ".."))
 				continue;
 
-			if (!evt_num_known) {
-				evt_num++;
+			if (tp_event_has_id(dir_path, evt_dirent) != 0)
+				continue;
+
+			if (event_glob != NULL &&
+			    !strglobmatch(evt_dirent->d_name, event_glob))
 				continue;
-			}
 
 			snprintf(evt_path, MAXPATHLEN, "%s:%s",
 				 sys_dirent->d_name, evt_dirent->d_name);
-
-			evt_list[evt_i] = strdup(evt_path);
-			if (evt_list[evt_i] == NULL) {
-				put_events_file(dir_path);
-				goto out_close_evt_dir;
+			if (name_only)
+				printf("%s ", evt_path);
+			else {
+				printf("  %-50s [%s]\n", evt_path,
+				       event_type_descriptors[PERF_TYPE_TRACEPOINT]);
 			}
-			evt_i++;
+			printed = true;
 		}
-		closedir(evt_dir);
-next:
-		put_events_file(dir_path);
+		free(dir_path);
+		free(evt_namelist);
 	}
-	closedir(sys_dir);
-
-	if (!evt_num_known) {
-		evt_num_known = true;
-		goto restart;
-	}
-	qsort(evt_list, evt_num, sizeof(char *), cmp_string);
-	evt_i = 0;
-	while (evt_i < evt_num) {
-		if (name_only) {
-			printf("%s ", evt_list[evt_i++]);
-			continue;
-		}
-		printf("  %-50s [%s]\n", evt_list[evt_i++],
-				event_type_descriptors[PERF_TYPE_TRACEPOINT]);
-	}
-	if (evt_num && pager_in_use())
+	free(sys_namelist);
+	if (printed && pager_in_use())
 		printf("\n");
-
-out_free:
-	evt_num = evt_i;
-	for (evt_i = 0; evt_i < evt_num; evt_i++)
-		zfree(&evt_list[evt_i]);
-	zfree(&evt_list);
-	return;
-
-out_close_evt_dir:
-	closedir(evt_dir);
-out_close_sys_dir:
-	closedir(sys_dir);
-
-	printf("FATAL: not enough memory to print %s\n",
-			event_type_descriptors[PERF_TYPE_TRACEPOINT]);
-	if (evt_list)
-		goto out_free;
 }
 
 void print_sdt_events(const char *subsys_glob, const char *event_glob,
-- 
2.38.1.431.g37b22c650d-goog

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

[Ian Rogers]

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))
 			continue;
-		}
 
 		list_for_each_entry(alias, &pmu->aliases, list) {
 			char *name = alias->desc ? alias->name :
-- 
2.38.1.431.g37b22c650d-goog

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

[Xing Zhengjun]

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 :

-- 
Zhengjun Xing

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

[Ian Rogers]

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

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

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

[Ian Rogers]

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 :

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

[Liang, Kan]

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 :

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

[Ian Rogers]

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 :

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

[Liang, Kan]

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 :

[PATCH v1 5/9] perf list: Simplify cache event printing

[Ian Rogers]

The current code computes an array of cache names then sorts and
prints them. Use a strlist to create a list of names that is
sorted. Keep the hybrid names, it is unclear how to generalize it, but
drop the computation of evt_pmus that is never used.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/print-events.c | 132 +++++++--------------------------
 1 file changed, 27 insertions(+), 105 deletions(-)

diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
index fefc025bc259..ff7793944246 100644
--- a/tools/perf/util/print-events.c
+++ b/tools/perf/util/print-events.c
@@ -206,137 +206,59 @@ void print_sdt_events(const char *subsys_glob, const char *event_glob,
 
 int print_hwcache_events(const char *event_glob, bool name_only)
 {
-	unsigned int type, op, i, evt_i = 0, evt_num = 0, npmus = 0;
-	char name[64], new_name[128];
-	char **evt_list = NULL, **evt_pmus = NULL;
-	bool evt_num_known = false;
-	struct perf_pmu *pmu = NULL;
-
-	if (perf_pmu__has_hybrid()) {
-		npmus = perf_pmu__hybrid_pmu_num();
-		evt_pmus = zalloc(sizeof(char *) * npmus);
-		if (!evt_pmus)
-			goto out_enomem;
-	}
+	struct strlist *evt_name_list = strlist__new(NULL, NULL);
+	struct str_node *nd;
 
-restart:
-	if (evt_num_known) {
-		evt_list = zalloc(sizeof(char *) * evt_num);
-		if (!evt_list)
-			goto out_enomem;
+	if (!evt_name_list) {
+		pr_debug("Failed to allocate new strlist for hwcache events\n");
+		return -ENOMEM;
 	}
-
-	for (type = 0; type < PERF_COUNT_HW_CACHE_MAX; type++) {
-		for (op = 0; op < PERF_COUNT_HW_CACHE_OP_MAX; op++) {
+	for (int type = 0; type < PERF_COUNT_HW_CACHE_MAX; type++) {
+		for (int op = 0; op < PERF_COUNT_HW_CACHE_OP_MAX; op++) {
 			/* skip invalid cache type */
 			if (!evsel__is_cache_op_valid(type, op))
 				continue;
 
-			for (i = 0; i < PERF_COUNT_HW_CACHE_RESULT_MAX; i++) {
-				unsigned int hybrid_supported = 0, j;
-				bool supported;
+			for (int i = 0; i < PERF_COUNT_HW_CACHE_RESULT_MAX; i++) {
+				struct perf_pmu *pmu = NULL;
+				char name[64];
 
 				__evsel__hw_cache_type_op_res_name(type, op, i, name, sizeof(name));
 				if (event_glob != NULL && !strglobmatch(name, event_glob))
 					continue;
 
 				if (!perf_pmu__has_hybrid()) {
-					if (!is_event_supported(PERF_TYPE_HW_CACHE,
-								type | (op << 8) | (i << 16))) {
-						continue;
-					}
-				} else {
-					perf_pmu__for_each_hybrid_pmu(pmu) {
-						if (!evt_num_known) {
-							evt_num++;
-							continue;
-						}
-
-						supported = is_event_supported(
-							PERF_TYPE_HW_CACHE,
-							type | (op << 8) | (i << 16) |
-							((__u64)pmu->type << PERF_PMU_TYPE_SHIFT));
-						if (supported) {
-							snprintf(new_name, sizeof(new_name),
-								 "%s/%s/", pmu->name, name);
-							evt_pmus[hybrid_supported] =
-								strdup(new_name);
-							hybrid_supported++;
-						}
-					}
-
-					if (hybrid_supported == 0)
-						continue;
-				}
-
-				if (!evt_num_known) {
-					evt_num++;
+					if (is_event_supported(PERF_TYPE_HW_CACHE,
+							       type | (op << 8) | (i << 16)))
+						strlist__add(evt_name_list, name);
 					continue;
 				}
-
-				if ((hybrid_supported == 0) ||
-				    (hybrid_supported == npmus)) {
-					evt_list[evt_i] = strdup(name);
-					if (npmus > 0) {
-						for (j = 0; j < npmus; j++)
-							zfree(&evt_pmus[j]);
-					}
-				} else {
-					for (j = 0; j < hybrid_supported; j++) {
-						evt_list[evt_i++] = evt_pmus[j];
-						evt_pmus[j] = NULL;
+				perf_pmu__for_each_hybrid_pmu(pmu) {
+					if (is_event_supported(PERF_TYPE_HW_CACHE,
+					    type | (op << 8) | (i << 16) |
+					    ((__u64)pmu->type << PERF_PMU_TYPE_SHIFT))) {
+						char new_name[128];
+							snprintf(new_name, sizeof(new_name),
+								 "%s/%s/", pmu->name, name);
+							strlist__add(evt_name_list, new_name);
 					}
-					continue;
 				}
-
-				if (evt_list[evt_i] == NULL)
-					goto out_enomem;
-				evt_i++;
 			}
 		}
 	}
 
-	if (!evt_num_known) {
-		evt_num_known = true;
-		goto restart;
-	}
-
-	for (evt_i = 0; evt_i < evt_num; evt_i++) {
-		if (!evt_list[evt_i])
-			break;
-	}
-
-	evt_num = evt_i;
-	qsort(evt_list, evt_num, sizeof(char *), cmp_string);
-	evt_i = 0;
-	while (evt_i < evt_num) {
+	strlist__for_each_entry(nd, evt_name_list) {
 		if (name_only) {
-			printf("%s ", evt_list[evt_i++]);
+			printf("%s ", nd->s);
 			continue;
 		}
-		printf("  %-50s [%s]\n", evt_list[evt_i++],
-				event_type_descriptors[PERF_TYPE_HW_CACHE]);
+		printf("  %-50s [%s]\n", nd->s, event_type_descriptors[PERF_TYPE_HW_CACHE]);
 	}
-	if (evt_num && pager_in_use())
+	if (!strlist__empty(evt_name_list) && pager_in_use())
 		printf("\n");
 
-out_free:
-	evt_num = evt_i;
-	for (evt_i = 0; evt_i < evt_num; evt_i++)
-		zfree(&evt_list[evt_i]);
-	zfree(&evt_list);
-
-	for (evt_i = 0; evt_i < npmus; evt_i++)
-		zfree(&evt_pmus[evt_i]);
-	zfree(&evt_pmus);
-	return evt_num;
-
-out_enomem:
-	printf("FATAL: not enough memory to print %s\n",
-		event_type_descriptors[PERF_TYPE_HW_CACHE]);
-	if (evt_list)
-		goto out_free;
-	return evt_num;
+	strlist__delete(evt_name_list);
+	return 0;
 }
 
 static void print_tool_event(const struct event_symbol *syms, const char *event_glob,
-- 
2.38.1.431.g37b22c650d-goog

[PATCH v1 6/9] perf list: Simplify symbol event printing

[Ian Rogers]

The current code computes an array of symbol names then sorts and
prints them. Use a strlist to create a list of names that is
sorted and then print it.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/print-events.c | 79 +++++++++-------------------------
 1 file changed, 21 insertions(+), 58 deletions(-)

diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
index ff7793944246..d53dba033597 100644
--- a/tools/perf/util/print-events.c
+++ b/tools/perf/util/print-events.c
@@ -52,14 +52,6 @@ static const struct event_symbol event_symbols_tool[PERF_TOOL_MAX] = {
 	},
 };
 
-static int cmp_string(const void *a, const void *b)
-{
-	const char * const *as = a;
-	const char * const *bs = b;
-
-	return strcmp(*as, *bs);
-}
-
 /*
  * Print the events from <debugfs_mount_point>/tracing/events
  */
@@ -298,77 +290,48 @@ void print_symbol_events(const char *event_glob, unsigned int type,
 			 struct event_symbol *syms, unsigned int max,
 			 bool name_only)
 {
-	unsigned int i, evt_i = 0, evt_num = 0;
-	char name[MAX_NAME_LEN];
-	char **evt_list = NULL;
-	bool evt_num_known = false;
-
-restart:
-	if (evt_num_known) {
-		evt_list = zalloc(sizeof(char *) * evt_num);
-		if (!evt_list)
-			goto out_enomem;
-		syms -= max;
-	}
+	struct strlist *evt_name_list = strlist__new(NULL, NULL);
+	struct str_node *nd;
 
-	for (i = 0; i < max; i++, syms++) {
+	if (!evt_name_list) {
+		pr_debug("Failed to allocate new strlist for symbol events\n");
+		return;
+	}
+	for (unsigned int i = 0; i < max; i++) {
 		/*
 		 * New attr.config still not supported here, the latest
 		 * example was PERF_COUNT_SW_CGROUP_SWITCHES
 		 */
-		if (syms->symbol == NULL)
+		if (syms[i].symbol == NULL)
 			continue;
 
-		if (event_glob != NULL && !(strglobmatch(syms->symbol, event_glob) ||
-		      (syms->alias && strglobmatch(syms->alias, event_glob))))
+		if (event_glob != NULL && !(strglobmatch(syms[i].symbol, event_glob) ||
+		      (syms[i].alias && strglobmatch(syms[i].alias, event_glob))))
 			continue;
 
 		if (!is_event_supported(type, i))
 			continue;
 
-		if (!evt_num_known) {
-			evt_num++;
-			continue;
-		}
-
-		if (!name_only && strlen(syms->alias))
-			snprintf(name, MAX_NAME_LEN, "%s OR %s", syms->symbol, syms->alias);
-		else
-			strlcpy(name, syms->symbol, MAX_NAME_LEN);
+		if (strlen(syms[i].alias)) {
+			char name[MAX_NAME_LEN];
 
-		evt_list[evt_i] = strdup(name);
-		if (evt_list[evt_i] == NULL)
-			goto out_enomem;
-		evt_i++;
+			snprintf(name, MAX_NAME_LEN, "%s OR %s", syms[i].symbol, syms[i].alias);
+			strlist__add(evt_name_list, name);
+		} else
+			strlist__add(evt_name_list, syms[i].symbol);
 	}
 
-	if (!evt_num_known) {
-		evt_num_known = true;
-		goto restart;
-	}
-	qsort(evt_list, evt_num, sizeof(char *), cmp_string);
-	evt_i = 0;
-	while (evt_i < evt_num) {
+	strlist__for_each_entry(nd, evt_name_list) {
 		if (name_only) {
-			printf("%s ", evt_list[evt_i++]);
+			printf("%s ", nd->s);
 			continue;
 		}
-		printf("  %-50s [%s]\n", evt_list[evt_i++], event_type_descriptors[type]);
+		printf("  %-50s [%s]\n", nd->s, event_type_descriptors[type]);
 	}
-	if (evt_num && pager_in_use())
+	if (!strlist__empty(evt_name_list) && pager_in_use())
 		printf("\n");
 
-out_free:
-	evt_num = evt_i;
-	for (evt_i = 0; evt_i < evt_num; evt_i++)
-		zfree(&evt_list[evt_i]);
-	zfree(&evt_list);
-	return;
-
-out_enomem:
-	printf("FATAL: not enough memory to print %s\n", event_type_descriptors[type]);
-	if (evt_list)
-		goto out_free;
+	strlist__delete(evt_name_list);
 }
 
 /*
-- 
2.38.1.431.g37b22c650d-goog

[PATCH v1 7/9] perf pmu: Restructure print_pmu_events

[Ian Rogers]

Previously print_pmu_events would compute the values to be printed,
place them in struct sevent, sort them and then print them. Modify the
code so that struct sevent holds just the PMU and event, sort these
and then in the main print loop calculate aliases for names, etc. This
avoids memory allocations for copied values as they are computed then
printed.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/pmu.c | 208 ++++++++++++++++++++++--------------------
 1 file changed, 110 insertions(+), 98 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 9c771f136b81..8322395c9cf7 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1552,8 +1552,8 @@ static int sub_non_neg(int a, int b)
 	return a - b;
 }
 
-static char *format_alias(char *buf, int len, struct perf_pmu *pmu,
-			  struct perf_pmu_alias *alias)
+static char *format_alias(char *buf, int len, const struct perf_pmu *pmu,
+			  const struct perf_pmu_alias *alias)
 {
 	struct parse_events_term *term;
 	int used = snprintf(buf, len, "%s/%s", pmu->name, alias->name);
@@ -1578,51 +1578,67 @@ static char *format_alias(char *buf, int len, struct perf_pmu *pmu,
 	return buf;
 }
 
-static char *format_alias_or(char *buf, int len, struct perf_pmu *pmu,
-			     struct perf_pmu_alias *alias)
+static char *format_alias_or(char *buf, int len, const struct perf_pmu *pmu,
+			     const struct perf_pmu_alias *alias)
 {
 	snprintf(buf, len, "%s OR %s/%s/", alias->name, pmu->name, alias->name);
 	return buf;
 }
 
+/** Struct for ordering events as output in perf list. */
 struct sevent {
-	char *name;
-	char *desc;
-	char *topic;
-	char *str;
-	char *pmu;
-	char *metric_expr;
-	char *metric_name;
-	int is_cpu;
+	/** PMU for event. */
+	const struct perf_pmu *pmu;
+	/**
+	 * Optional event for name, desc, etc. If not present then this is a
+	 * selectable PMU and the event name is shown as "//".
+	 */
+	const struct perf_pmu_alias *event;
+	/** Is the PMU for the CPU? */
+	bool is_cpu;
 };
 
 static int cmp_sevent(const void *a, const void *b)
 {
 	const struct sevent *as = a;
 	const struct sevent *bs = b;
+	const char *a_pmu_name, *b_pmu_name;
+	const char *a_name = "//", *a_desc = NULL, *a_topic = "";
+	const char *b_name = "//", *b_desc = NULL, *b_topic = "";
 	int ret;
 
-	/* Put extra events last */
-	if (!!as->desc != !!bs->desc)
-		return !!as->desc - !!bs->desc;
-	if (as->topic && bs->topic) {
-		int n = strcmp(as->topic, bs->topic);
-
-		if (n)
-			return n;
+	if (as->event) {
+		a_name = as->event->name;
+		a_desc = as->event->desc;
+		a_topic = as->event->topic ?: "";
 	}
+	if (bs->event) {
+		b_name = bs->event->name;
+		b_desc = bs->event->desc;
+		b_topic = bs->event->topic ?: "";
+	}
+	/* Put extra events last. */
+	if (!!a_desc != !!b_desc)
+		return !!a_desc - !!b_desc;
+
+	/* Order by topics. */
+	ret = strcmp(a_topic, b_topic);
+	if (ret)
+		return ret;
 
 	/* Order CPU core events to be first */
 	if (as->is_cpu != bs->is_cpu)
 		return bs->is_cpu - as->is_cpu;
 
-	ret = strcmp(as->name, bs->name);
-	if (!ret) {
-		if (as->pmu && bs->pmu)
-			return strcmp(as->pmu, bs->pmu);
-	}
+	/* Order by PMU name. */
+	a_pmu_name = as->pmu->name ?: "";
+	b_pmu_name = bs->pmu->name ?: "";
+	ret = strcmp(a_pmu_name, b_pmu_name);
+	if (ret)
+		return ret;
 
-	return ret;
+	/* Order by event name. */
+	return strcmp(a_name, b_name);
 }
 
 static void wordwrap(char *s, int start, int max, int corr)
@@ -1654,16 +1670,18 @@ bool is_pmu_core(const char *name)
 static bool pmu_alias_is_duplicate(struct sevent *alias_a,
 				   struct sevent *alias_b)
 {
-	/* Different names -> never duplicates */
-	if (strcmp(alias_a->name, alias_b->name))
-		return false;
+	const char *a_pmu_name, *b_pmu_name;
+	const char *a_name = alias_a->event ? alias_a->event->name : "//";
+	const char *b_name = alias_b->event ? alias_b->event->name : "//";
 
-	/* Don't remove duplicates for hybrid PMUs */
-	if (perf_pmu__is_hybrid(alias_a->pmu) &&
-	    perf_pmu__is_hybrid(alias_b->pmu))
+	/* Different names -> never duplicates */
+	if (strcmp(a_name, b_name))
 		return false;
 
-	return true;
+	/* Don't remove duplicates for different PMUs */
+	a_pmu_name = alias_a->pmu->name ?: "";
+	b_pmu_name = alias_b->pmu->name ?: "";
+	return strcmp(a_pmu_name, b_pmu_name) == 0;
 }
 
 void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
@@ -1689,110 +1707,104 @@ void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
 			len++;
 	}
 	aliases = zalloc(sizeof(struct sevent) * len);
-	if (!aliases)
-		goto out_enomem;
+	if (!aliases) {
+		pr_err("FATAL: not enough memory to print PMU events\n");
+		return;
+	}
 	pmu = NULL;
 	j = 0;
 	while ((pmu = perf_pmu__scan(pmu)) != NULL) {
+		bool is_cpu;
+
 		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 :
-				format_alias(buf, sizeof(buf), pmu, alias);
-			bool is_cpu = is_pmu_core(pmu->name) ||
-				      perf_pmu__is_hybrid(pmu->name);
+		is_cpu = is_pmu_core(pmu->name) || pmu->is_hybrid;
 
+		list_for_each_entry(alias, &pmu->aliases, list) {
 			if (alias->deprecated && !deprecated)
 				continue;
 
 			if (event_glob != NULL &&
-			    !(strglobmatch_nocase(name, event_glob) ||
-			      (!is_cpu && strglobmatch_nocase(alias->name,
-						       event_glob)) ||
+			    !(strglobmatch_nocase(alias->name, event_glob) ||
+			      (!is_cpu &&
+			       strglobmatch_nocase(alias->name, event_glob)) ||
 			      (alias->topic &&
 			       strglobmatch_nocase(alias->topic, event_glob))))
 				continue;
 
-			if (is_cpu && !name_only && !alias->desc)
-				name = format_alias_or(buf, sizeof(buf), pmu, alias);
-
-			aliases[j].name = name;
-			if (is_cpu && !name_only && !alias->desc)
-				aliases[j].name = format_alias_or(buf,
-								  sizeof(buf),
-								  pmu, alias);
-			aliases[j].name = strdup(aliases[j].name);
-			if (!aliases[j].name)
-				goto out_enomem;
-
-			aliases[j].desc = long_desc ? alias->long_desc :
-						alias->desc;
-			aliases[j].topic = alias->topic;
-			aliases[j].str = alias->str;
-			aliases[j].pmu = pmu->name;
-			aliases[j].metric_expr = alias->metric_expr;
-			aliases[j].metric_name = alias->metric_name;
+			aliases[j].event = alias;
+			aliases[j].pmu = pmu;
 			aliases[j].is_cpu = is_cpu;
 			j++;
 		}
 		if (pmu->selectable &&
 		    (event_glob == NULL || strglobmatch(pmu->name, event_glob))) {
-			char *s;
-			if (asprintf(&s, "%s//", pmu->name) < 0)
-				goto out_enomem;
-			aliases[j].name = s;
+			aliases[j].event = NULL;
+			aliases[j].pmu = pmu;
+			aliases[j].is_cpu = is_cpu;
 			j++;
 		}
 	}
 	len = j;
 	qsort(aliases, len, sizeof(struct sevent), cmp_sevent);
 	for (j = 0; j < len; j++) {
+		char *name, *desc;
+
 		/* Skip duplicates */
 		if (j > 0 && pmu_alias_is_duplicate(&aliases[j], &aliases[j - 1]))
 			continue;
 
+		if (!aliases[j].event) {
+			/* A selectable event. */
+			snprintf(buf, sizeof(buf), "%s//", aliases[j].pmu->name);
+			name = buf;
+		} else if (aliases[j].event->desc) {
+			name = aliases[j].event->name;
+		} else {
+			if (!name_only && aliases[j].is_cpu) {
+				name = format_alias_or(buf, sizeof(buf), aliases[j].pmu,
+						       aliases[j].event);
+			} else {
+				name = format_alias(buf, sizeof(buf), aliases[j].pmu,
+						    aliases[j].event);
+			}
+		}
 		if (name_only) {
-			printf("%s ", aliases[j].name);
+			printf("%s ", name);
 			continue;
 		}
-		if (aliases[j].desc && !quiet_flag) {
-			if (numdesc++ == 0)
-				printf("\n");
-			if (aliases[j].topic && (!topic ||
-					strcmp(topic, aliases[j].topic))) {
-				printf("%s%s:\n", topic ? "\n" : "",
-						aliases[j].topic);
-				topic = aliases[j].topic;
-			}
-			printf("  %-50s\n", aliases[j].name);
-			printf("%*s", 8, "[");
-			wordwrap(aliases[j].desc, 8, columns, 0);
-			printf("]\n");
-			if (details_flag) {
-				printf("%*s%s/%s/ ", 8, "", aliases[j].pmu, aliases[j].str);
-				if (aliases[j].metric_name)
-					printf(" MetricName: %s", aliases[j].metric_name);
-				if (aliases[j].metric_expr)
-					printf(" MetricExpr: %s", aliases[j].metric_expr);
-				putchar('\n');
-			}
-		} else
-			printf("  %-50s [Kernel PMU event]\n", aliases[j].name);
 		printed++;
+		if (!aliases[j].event || !aliases[j].event->desc || quiet_flag) {
+			printf("  %-50s [Kernel PMU event]\n", name);
+			continue;
+		}
+		if (numdesc++ == 0)
+			printf("\n");
+		if (aliases[j].event->topic && (!topic ||
+						strcmp(topic, aliases[j].event->topic))) {
+			printf("%s%s:\n", topic ? "\n" : "", aliases[j].event->topic);
+			topic = aliases[j].event->topic;
+		}
+		printf("  %-50s\n", name);
+		printf("%*s", 8, "[");
+		desc = long_desc ? aliases[j].event->long_desc : aliases[j].event->desc;
+		wordwrap(desc, 8, columns, 0);
+		printf("]\n");
+		if (details_flag) {
+			printf("%*s%s/%s/ ", 8, "", aliases[j].pmu->name, aliases[j].event->str);
+			if (aliases[j].event->metric_name)
+				printf(" MetricName: %s", aliases[j].event->metric_name);
+			if (aliases[j].event->metric_expr)
+				printf(" MetricExpr: %s", aliases[j].event->metric_expr);
+			putchar('\n');
+		}
 	}
 	if (printed && pager_in_use())
 		printf("\n");
-out_free:
-	for (j = 0; j < len; j++)
-		zfree(&aliases[j].name);
+
 	zfree(&aliases);
 	return;
-
-out_enomem:
-	printf("FATAL: not enough memory to print PMU events\n");
-	if (aliases)
-		goto out_free;
 }
 
 bool pmu_have_event(const char *pname, const char *name)
-- 
2.38.1.431.g37b22c650d-goog

[PATCH v1 8/9] perf list: Reorganize to use callbacks

[Ian Rogers]

Rather than controlling the list output with passed flags, add
callbacks that are called when an event or metric are
encountered. State is passed to the callback so that command line
options can be respected, alternatively the callbacks can be changed.

Fix a few bugs:
 - wordwrap to columns metric descriptions and expressions;
 - remove unnecessary whitespace after PMU event names;
 - the metric filter is a glob but matched using strstr which will
   always fail, switch to using a proper globmatch,
 - the detail flag gives details for extra kernel PMU events like
   branch-instructions.

In metricgroup.c switch from struct mep being a rbtree of metricgroups
containing a list of metrics, to the tree directly containing all the
metrics. In general the alias for a name is passed to the print
routine rather than being contained in the name with OR.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-list.c      | 320 ++++++++++++++++++++++++-----
 tools/perf/util/metricgroup.c  | 239 ++++++----------------
 tools/perf/util/metricgroup.h  |   4 +-
 tools/perf/util/pmu.c          | 137 ++++---------
 tools/perf/util/pmu.h          |   5 +-
 tools/perf/util/print-events.c | 355 +++++++++++++++++----------------
 tools/perf/util/print-events.h |  40 ++--
 7 files changed, 588 insertions(+), 512 deletions(-)

diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index cc84ced6da26..91e2b6f52548 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -15,31 +15,229 @@
 #include "util/pmu-hybrid.h"
 #include "util/debug.h"
 #include "util/metricgroup.h"
+#include "util/string2.h"
+#include "util/strlist.h"
 #include <subcmd/pager.h>
 #include <subcmd/parse-options.h>
 #include <stdio.h>
 
-static bool desc_flag = true;
-static bool details_flag;
+struct print_state {
+	/**
+	 * Optionally restrict PMU and metric matching to PMU or debugfs
+	 * subsystem name.
+	 */
+	char *pmu_glob;
+	/** Optional pattern matching glob. */
+	char *event_glob;
+	/** Print event or metric names only. */
+	bool name_only;
+	/** Print the event or metric description. */
+	bool desc;
+	/** Print longer event or metric description. */
+	bool long_desc;
+	/** Print deprecated events or metrics. */
+	bool deprecated;
+	/**
+	 * Print extra information on the perf event such as names and
+	 * expressions used internally by events.
+	 */
+	bool detailed;
+	bool metrics;
+	bool metricgroups;
+	char *last_topic;
+	char *last_metricgroups;
+	struct strlist *visited_metrics;
+};
+
+static void default_print_start(void *ps)
+{
+	struct print_state *print_state = ps;
+
+	if (!print_state->name_only && pager_in_use())
+		printf("\nList of pre-defined events (to be used in -e or -M):\n\n");
+}
+
+static void default_print_end(void *print_state __maybe_unused) {}
+
+static void wordwrap(const char *s, int start, int max, int corr)
+{
+	int column = start;
+	int n;
+
+	while (*s) {
+		int wlen = strcspn(s, " \t");
+
+		if (column + wlen >= max && column > start) {
+			printf("\n%*s", start, "");
+			column = start + corr;
+		}
+		n = printf("%s%.*s", column > start ? " " : "", wlen, s);
+		if (n <= 0)
+			break;
+		s += wlen;
+		column += n;
+		s = skip_spaces(s);
+	}
+}
+
+static void default_print_event(void *ps, const char *pmu_name, const char *topic,
+				const char *event_name, const char *event_alias,
+				bool deprecated, const char *event_type_desc,
+				const char *desc, const char *long_desc,
+				const char *encoding_desc,
+				const char *metric_name, const char *metric_expr)
+{
+	struct print_state *print_state = ps;
+	int pos;
+
+	if (deprecated && !print_state->deprecated)
+		return;
+
+	if (print_state->pmu_glob && !strglobmatch(pmu_name, print_state->pmu_glob))
+		return;
+
+	if (print_state->event_glob &&
+	    (!event_name || !strglobmatch(event_name, print_state->event_glob)) &&
+	    (!event_alias || !strglobmatch(event_alias, print_state->event_glob)) &&
+	    (!topic || !strglobmatch_nocase(topic, print_state->event_glob)))
+		return;
+
+	if (print_state->name_only) {
+		if (event_alias && strlen(event_alias))
+			printf("%s ", event_alias);
+		else
+			printf("%s ", event_name);
+		return;
+	}
+
+	if (strcmp(print_state->last_topic, topic ?: "")) {
+		if (topic)
+			printf("\n%s:\n", topic);
+		free(print_state->last_topic);
+		print_state->last_topic = strdup(topic ?: "");
+	}
+
+	if (event_alias && strlen(event_alias))
+		pos = printf("  %s OR %s", event_name, event_alias);
+	else
+		pos = printf("  %s", event_name);
+
+	if (!topic && event_type_desc) {
+		for (; pos < 53; pos++)
+			putchar(' ');
+		printf("[%s]\n", event_type_desc);
+	} else
+		putchar('\n');
+
+	if (desc && print_state->desc) {
+		printf("%*s", 8, "[");
+		wordwrap(desc, 8, pager_get_columns(), 0);
+		printf("]\n");
+	}
+
+	if (long_desc && print_state->long_desc) {
+		printf("%*s", 8, "[");
+		wordwrap(long_desc, 8, pager_get_columns(), 0);
+		printf("]\n");
+	}
+
+	if (print_state->detailed && encoding_desc) {
+		printf("%*s%s", 8, "", encoding_desc);
+		if (metric_name)
+			printf(" MetricName: %s", metric_name);
+		if (metric_expr)
+			printf(" MetricExpr: %s", metric_expr);
+		putchar('\n');
+	}
+}
+
+static void default_print_metric(void *ps,
+				const char *group,
+				const char *name,
+				const char *desc,
+				const char *long_desc,
+				const char *expr)
+{
+	struct print_state *print_state = ps;
+
+	if (print_state->event_glob &&
+	    (!print_state->metrics || !name || !strglobmatch(name, print_state->event_glob)) &&
+	    (!print_state->metricgroups || !group || !strglobmatch(group, print_state->event_glob)))
+		return;
+
+	if (!print_state->name_only && !print_state->last_metricgroups) {
+		if (print_state->metricgroups) {
+			printf("\nMetric Groups:\n");
+			if (!print_state->metrics)
+				putchar('\n');
+		} else {
+			printf("\nMetrics:\n\n");
+		}
+	}
+	if (!print_state->last_metricgroups ||
+	    strcmp(print_state->last_metricgroups, group ?: "")) {
+		if (group && print_state->metricgroups) {
+			if (print_state->name_only)
+				printf("%s ", group);
+			else if (print_state->metrics)
+				printf("\n%s:\n", group);
+			else
+				printf("%s\n", group);
+		}
+		free(print_state->last_metricgroups);
+		print_state->last_metricgroups = strdup(group ?: "");
+	}
+	if (!print_state->metrics)
+		return;
+
+	if (print_state->name_only) {
+		if (print_state->metrics &&
+		    !strlist__has_entry(print_state->visited_metrics, name)) {
+			printf("%s ", name);
+			strlist__add(print_state->visited_metrics, name);
+		}
+		return;
+	}
+	printf("  %s\n", name);
+
+	if (desc && print_state->desc) {
+		printf("%*s", 8, "[");
+		wordwrap(desc, 8, pager_get_columns(), 0);
+		printf("]\n");
+	}
+	if (long_desc && print_state->long_desc) {
+		printf("%*s", 8, "[");
+		wordwrap(long_desc, 8, pager_get_columns(), 0);
+		printf("]\n");
+	}
+	if (expr && print_state->detailed) {
+		printf("%*s", 8, "[");
+		wordwrap(expr, 8, pager_get_columns(), 0);
+		printf("]\n");
+	}
+}
 
 int cmd_list(int argc, const char **argv)
 {
 	int i, ret = 0;
-	bool raw_dump = false;
-	bool long_desc_flag = false;
-	bool deprecated = false;
-	char *pmu_name = NULL;
+	struct print_state ps = {};
+	struct print_callbacks print_cb = {
+		.print_start = default_print_start,
+		.print_end = default_print_end,
+		.print_event = default_print_event,
+		.print_metric = default_print_metric,
+	};
 	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,
+		OPT_BOOLEAN(0, "raw-dump", &ps.name_only, "Dump raw events"),
+		OPT_BOOLEAN('d', "desc", &ps.desc,
 			    "Print extra event descriptions. --no-desc to not print."),
-		OPT_BOOLEAN('v', "long-desc", &long_desc_flag,
+		OPT_BOOLEAN('v', "long-desc", &ps.long_desc,
 			    "Print longer event descriptions."),
-		OPT_BOOLEAN(0, "details", &details_flag,
+		OPT_BOOLEAN(0, "details", &ps.detailed,
 			    "Print information on the perf event names and expressions used internally by events."),
-		OPT_BOOLEAN(0, "deprecated", &deprecated,
+		OPT_BOOLEAN(0, "deprecated", &ps.deprecated,
 			    "Print deprecated events."),
 		OPT_STRING(0, "cputype", &hybrid_name, "hybrid cpu type",
 			   "Limit PMU or metric printing to the given hybrid PMU (e.g. core or atom)."),
@@ -63,20 +261,28 @@ int cmd_list(int argc, const char **argv)
 
 	setup_pager();
 
-	if (!raw_dump && pager_in_use())
-		printf("\nList of pre-defined events (to be used in -e or -M):\n\n");
+	if (!ps.name_only)
+		setup_pager();
 
+	ps.desc = !ps.long_desc;
+	ps.last_topic = strdup("");
+	assert(ps.last_topic);
+	ps.visited_metrics = strlist__new(NULL, NULL);
+	assert(ps.visited_metrics);
 	if (unit_name)
-		pmu_name = strdup(unit_name);
+		ps.pmu_glob = strdup(unit_name);
 	else if (hybrid_name) {
-		pmu_name = perf_pmu__hybrid_type_to_pmu(hybrid_name);
-		if (!pmu_name)
+		ps.pmu_glob = perf_pmu__hybrid_type_to_pmu(hybrid_name);
+		if (!ps.pmu_glob)
 			pr_warning("WARNING: hybrid cputype is not supported!\n");
 	}
 
+	print_cb.print_start(&ps);
+
 	if (argc == 0) {
-		print_events(NULL, raw_dump, !desc_flag, long_desc_flag,
-				details_flag, deprecated, pmu_name);
+		ps.metrics = true;
+		ps.metricgroups = true;
+		print_events(&print_cb, &ps);
 		goto out;
 	}
 
@@ -84,30 +290,33 @@ int cmd_list(int argc, const char **argv)
 		char *sep, *s;
 
 		if (strcmp(argv[i], "tracepoint") == 0)
-			print_tracepoint_events(NULL, NULL, raw_dump);
+			print_tracepoint_events(&print_cb, &ps);
 		else if (strcmp(argv[i], "hw") == 0 ||
 			 strcmp(argv[i], "hardware") == 0)
-			print_symbol_events(NULL, PERF_TYPE_HARDWARE,
-					event_symbols_hw, PERF_COUNT_HW_MAX, raw_dump);
+			print_symbol_events(&print_cb, &ps, PERF_TYPE_HARDWARE,
+					event_symbols_hw, PERF_COUNT_HW_MAX);
 		else if (strcmp(argv[i], "sw") == 0 ||
 			 strcmp(argv[i], "software") == 0) {
-			print_symbol_events(NULL, PERF_TYPE_SOFTWARE,
-					event_symbols_sw, PERF_COUNT_SW_MAX, raw_dump);
-			print_tool_events(NULL, raw_dump);
+			print_symbol_events(&print_cb, &ps, PERF_TYPE_SOFTWARE,
+					event_symbols_sw, PERF_COUNT_SW_MAX);
+			print_tool_events(&print_cb, &ps);
 		} else if (strcmp(argv[i], "cache") == 0 ||
 			 strcmp(argv[i], "hwcache") == 0)
-			print_hwcache_events(NULL, raw_dump);
+			print_hwcache_events(&print_cb, &ps);
 		else if (strcmp(argv[i], "pmu") == 0)
-			print_pmu_events(NULL, raw_dump, !desc_flag,
-						long_desc_flag, details_flag,
-						deprecated, pmu_name);
+			print_pmu_events(&print_cb, &ps);
 		else if (strcmp(argv[i], "sdt") == 0)
-			print_sdt_events(NULL, NULL, raw_dump);
-		else if (strcmp(argv[i], "metric") == 0 || strcmp(argv[i], "metrics") == 0)
-			metricgroup__print(true, false, NULL, raw_dump, details_flag, pmu_name);
-		else if (strcmp(argv[i], "metricgroup") == 0 || strcmp(argv[i], "metricgroups") == 0)
-			metricgroup__print(false, true, NULL, raw_dump, details_flag, pmu_name);
-		else if ((sep = strchr(argv[i], ':')) != NULL) {
+			print_sdt_events(&print_cb, &ps);
+		else if (strcmp(argv[i], "metric") == 0 || strcmp(argv[i], "metrics") == 0) {
+			ps.metricgroups = false;
+			ps.metrics = true;
+			metricgroup__print(&print_cb, &ps);
+		} else if (strcmp(argv[i], "metricgroup") == 0 ||
+			   strcmp(argv[i], "metricgroups") == 0) {
+			ps.metricgroups = true;
+			ps.metrics = false;
+			metricgroup__print(&print_cb, &ps);
+		} else if ((sep = strchr(argv[i], ':')) != NULL) {
 			int sep_idx;
 
 			sep_idx = sep - argv[i];
@@ -118,34 +327,41 @@ int cmd_list(int argc, const char **argv)
 			}
 
 			s[sep_idx] = '\0';
-			print_tracepoint_events(s, s + sep_idx + 1, raw_dump);
-			print_sdt_events(s, s + sep_idx + 1, raw_dump);
-			metricgroup__print(true, true, s, raw_dump, details_flag, pmu_name);
+			ps.pmu_glob = s;
+			ps.event_glob = s + sep_idx + 1;
+			print_tracepoint_events(&print_cb, &ps);
+			print_sdt_events(&print_cb, &ps);
+			ps.metrics = true;
+			ps.metricgroups = true;
+			metricgroup__print(&print_cb, &ps);
 			free(s);
 		} else {
 			if (asprintf(&s, "*%s*", argv[i]) < 0) {
 				printf("Critical: Not enough memory! Trying to continue...\n");
 				continue;
 			}
-			print_symbol_events(s, PERF_TYPE_HARDWARE,
-					    event_symbols_hw, PERF_COUNT_HW_MAX, raw_dump);
-			print_symbol_events(s, PERF_TYPE_SOFTWARE,
-					    event_symbols_sw, PERF_COUNT_SW_MAX, raw_dump);
-			print_tool_events(s, raw_dump);
-			print_hwcache_events(s, raw_dump);
-			print_pmu_events(s, raw_dump, !desc_flag,
-						long_desc_flag,
-						details_flag,
-						deprecated,
-						pmu_name);
-			print_tracepoint_events(NULL, s, raw_dump);
-			print_sdt_events(NULL, s, raw_dump);
-			metricgroup__print(true, true, s, raw_dump, details_flag, pmu_name);
+			ps.event_glob = s;
+			print_symbol_events(&print_cb, &ps, PERF_TYPE_HARDWARE,
+					event_symbols_hw, PERF_COUNT_HW_MAX);
+			print_symbol_events(&print_cb, &ps, PERF_TYPE_SOFTWARE,
+					event_symbols_sw, PERF_COUNT_SW_MAX);
+			print_tool_events(&print_cb, &ps);
+			print_hwcache_events(&print_cb, &ps);
+			print_pmu_events(&print_cb, &ps);
+			print_tracepoint_events(&print_cb, &ps);
+			print_sdt_events(&print_cb, &ps);
+			ps.metrics = true;
+			ps.metricgroups = true;
+			metricgroup__print(&print_cb, &ps);
 			free(s);
 		}
 	}
 
 out:
-	free(pmu_name);
+	print_cb.print_end(&ps);
+	free(ps.pmu_glob);
+	free(ps.last_topic);
+	free(ps.last_metricgroups);
+	strlist__delete(ps.visited_metrics);
 	return ret;
 }
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 1943fed9b6d9..4cb2a193c0f2 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -12,6 +12,7 @@
 #include "strbuf.h"
 #include "pmu.h"
 #include "pmu-hybrid.h"
+#include "print-events.h"
 #include "expr.h"
 #include "rblist.h"
 #include <string.h>
@@ -352,51 +353,63 @@ static bool match_pe_metric(const struct pmu_event *pe, const char *metric)
 	       match_metric(pe->metric_name, metric);
 }
 
+/** RB-tree node for building printing information. */
 struct mep {
 	struct rb_node nd;
-	const char *name;
-	struct strlist *metrics;
+	/** Owned metric group name, separated others with ';'. */
+	char *metric_group;
+	const char *metric_name;
+	const char *metric_desc;
+	const char *metric_long_desc;
+	const char *metric_expr;
 };
 
 static int mep_cmp(struct rb_node *rb_node, const void *entry)
 {
 	struct mep *a = container_of(rb_node, struct mep, nd);
 	struct mep *b = (struct mep *)entry;
+	int ret;
 
-	return strcmp(a->name, b->name);
+	ret = strcmp(a->metric_group, b->metric_group);
+	if (ret)
+		return ret;
+
+	return strcmp(a->metric_name, b->metric_name);
 }
 
-static struct rb_node *mep_new(struct rblist *rl __maybe_unused,
-					const void *entry)
+static struct rb_node *mep_new(struct rblist *rl __maybe_unused, const void *entry)
 {
 	struct mep *me = malloc(sizeof(struct mep));
 
 	if (!me)
 		return NULL;
+
 	memcpy(me, entry, sizeof(struct mep));
-	me->name = strdup(me->name);
-	if (!me->name)
-		goto out_me;
-	me->metrics = strlist__new(NULL, NULL);
-	if (!me->metrics)
-		goto out_name;
 	return &me->nd;
-out_name:
-	zfree(&me->name);
-out_me:
+}
+
+static void mep_delete(struct rblist *rl __maybe_unused,
+		       struct rb_node *nd)
+{
+	struct mep *me = container_of(nd, struct mep, nd);
+
+	zfree(&me->metric_group);
 	free(me);
-	return NULL;
 }
 
-static struct mep *mep_lookup(struct rblist *groups, const char *name)
+static struct mep *mep_lookup(struct rblist *groups, const char *metric_group,
+			      const char *metric_name)
 {
 	struct rb_node *nd;
 	struct mep me = {
-		.name = name
+		.metric_group = strdup(metric_group),
+		.metric_name = metric_name,
 	};
 	nd = rblist__find(groups, &me);
-	if (nd)
+	if (nd) {
+		free(me.metric_group);
 		return container_of(nd, struct mep, nd);
+	}
 	rblist__add_node(groups, &me);
 	nd = rblist__find(groups, &me);
 	if (nd)
@@ -404,107 +417,36 @@ static struct mep *mep_lookup(struct rblist *groups, const char *name)
 	return NULL;
 }
 
-static void mep_delete(struct rblist *rl __maybe_unused,
-		       struct rb_node *nd)
-{
-	struct mep *me = container_of(nd, struct mep, nd);
-
-	strlist__delete(me->metrics);
-	zfree(&me->name);
-	free(me);
-}
-
-static void metricgroup__print_strlist(struct strlist *metrics, bool raw)
-{
-	struct str_node *sn;
-	int n = 0;
-
-	strlist__for_each_entry (sn, metrics) {
-		if (raw)
-			printf("%s%s", n > 0 ? " " : "", sn->s);
-		else
-			printf("  %s\n", sn->s);
-		n++;
-	}
-	if (raw)
-		putchar('\n');
-}
-
-static int metricgroup__print_pmu_event(const struct pmu_event *pe,
-					bool metricgroups, char *filter,
-					bool raw, bool details,
-					struct rblist *groups,
-					struct strlist *metriclist)
+static int metricgroup__add_to_mep_groups(const struct pmu_event *pe,
+					struct rblist *groups)
 {
 	const char *g;
 	char *omg, *mg;
 
-	g = pe->metric_group;
-	if (!g && pe->metric_name) {
-		if (pe->name)
-			return 0;
-		g = "No_group";
-	}
-
-	if (!g)
-		return 0;
-
-	mg = strdup(g);
-
+	mg = strdup(pe->metric_group ?: "No_group");
 	if (!mg)
 		return -ENOMEM;
 	omg = mg;
 	while ((g = strsep(&mg, ";")) != NULL) {
 		struct mep *me;
-		char *s;
 
 		g = skip_spaces(g);
-		if (*g == 0)
-			g = "No_group";
-		if (filter && !strstr(g, filter))
-			continue;
-		if (raw)
-			s = (char *)pe->metric_name;
-		else {
-			if (asprintf(&s, "%s\n%*s%s]",
-				     pe->metric_name, 8, "[", pe->desc) < 0)
-				return -1;
-			if (details) {
-				if (asprintf(&s, "%s\n%*s%s]",
-					     s, 8, "[", pe->metric_expr) < 0)
-					return -1;
-			}
-		}
-
-		if (!s)
-			continue;
+		if (strlen(g))
+			me = mep_lookup(groups, g, pe->metric_name);
+		else
+			me = mep_lookup(groups, "No_group", pe->metric_name);
 
-		if (!metricgroups) {
-			strlist__add(metriclist, s);
-		} else {
-			me = mep_lookup(groups, g);
-			if (!me)
-				continue;
-			strlist__add(me->metrics, s);
+		if (me) {
+			me->metric_desc = pe->desc;
+			me->metric_long_desc = pe->long_desc;
+			me->metric_expr = pe->metric_expr;
 		}
-
-		if (!raw)
-			free(s);
 	}
 	free(omg);
 
 	return 0;
 }
 
-struct metricgroup_print_sys_idata {
-	struct strlist *metriclist;
-	char *filter;
-	struct rblist *groups;
-	bool metricgroups;
-	bool raw;
-	bool details;
-};
-
 struct metricgroup_iter_data {
 	pmu_event_iter_fn fn;
 	void *data;
@@ -527,61 +469,26 @@ static int metricgroup__sys_event_iter(const struct pmu_event *pe,
 
 		return d->fn(pe, table, d->data);
 	}
-
 	return 0;
 }
 
-static int metricgroup__print_sys_event_iter(const struct pmu_event *pe,
-					     const struct pmu_events_table *table __maybe_unused,
-					     void *data)
-{
-	struct metricgroup_print_sys_idata *d = data;
-
-	return metricgroup__print_pmu_event(pe, d->metricgroups, d->filter, d->raw,
-				     d->details, d->groups, d->metriclist);
-}
-
-struct metricgroup_print_data {
-	const char *pmu_name;
-	struct strlist *metriclist;
-	char *filter;
-	struct rblist *groups;
-	bool metricgroups;
-	bool raw;
-	bool details;
-};
-
-static int metricgroup__print_callback(const struct pmu_event *pe,
-				       const struct pmu_events_table *table __maybe_unused,
-				       void *vdata)
+static int metricgroup__add_to_mep_groups_callback(const struct pmu_event *pe,
+						const struct pmu_events_table *table __maybe_unused,
+						void *vdata)
 {
-	struct metricgroup_print_data *data = vdata;
-	const char *pmu = pe->pmu ?: "cpu";
+	struct rblist *groups = vdata;
 
-	if (!pe->metric_expr)
+	if (!pe->metric_name)
 		return 0;
 
-	if (data->pmu_name && strcmp(data->pmu_name, pmu))
-		return 0;
-
-	return metricgroup__print_pmu_event(pe, data->metricgroups, data->filter,
-					    data->raw, data->details, data->groups,
-					    data->metriclist);
+	return metricgroup__add_to_mep_groups(pe, groups);
 }
 
-void metricgroup__print(bool metrics, bool metricgroups, char *filter,
-			bool raw, bool details, const char *pmu_name)
+void metricgroup__print(const struct print_callbacks *print_cb, void *print_state)
 {
 	struct rblist groups;
-	struct rb_node *node, *next;
-	struct strlist *metriclist = NULL;
 	const struct pmu_events_table *table;
-
-	if (!metricgroups) {
-		metriclist = strlist__new(NULL, NULL);
-		if (!metriclist)
-			return;
-	}
+	struct rb_node *node, *next;
 
 	rblist__init(&groups);
 	groups.node_new = mep_new;
@@ -589,56 +496,30 @@ void metricgroup__print(bool metrics, bool metricgroups, char *filter,
 	groups.node_delete = mep_delete;
 	table = pmu_events_table__find();
 	if (table) {
-		struct metricgroup_print_data data = {
-			.pmu_name = pmu_name,
-			.metriclist = metriclist,
-			.metricgroups = metricgroups,
-			.filter = filter,
-			.raw = raw,
-			.details = details,
-			.groups = &groups,
-		};
-
 		pmu_events_table_for_each_event(table,
-						metricgroup__print_callback,
-						&data);
+						metricgroup__add_to_mep_groups_callback,
+						&groups);
 	}
 	{
 		struct metricgroup_iter_data data = {
-			.fn = metricgroup__print_sys_event_iter,
-			.data = (void *) &(struct metricgroup_print_sys_idata){
-				.metriclist = metriclist,
-				.metricgroups = metricgroups,
-				.filter = filter,
-				.raw = raw,
-				.details = details,
-				.groups = &groups,
-			},
+			.fn = metricgroup__add_to_mep_groups_callback,
+			.data = &groups,
 		};
-
 		pmu_for_each_sys_event(metricgroup__sys_event_iter, &data);
 	}
 
-	if (!filter || !rblist__empty(&groups)) {
-		if (metricgroups && !raw)
-			printf("\nMetric Groups:\n\n");
-		else if (metrics && !raw)
-			printf("\nMetrics:\n\n");
-	}
-
 	for (node = rb_first_cached(&groups.entries); node; node = next) {
 		struct mep *me = container_of(node, struct mep, nd);
 
-		if (metricgroups)
-			printf("%s%s%s", me->name, metrics && !raw ? ":" : "", raw ? " " : "\n");
-		if (metrics)
-			metricgroup__print_strlist(me->metrics, raw);
+		print_cb->print_metric(print_state,
+				me->metric_group,
+				me->metric_name,
+				me->metric_desc,
+				me->metric_long_desc,
+				me->metric_expr);
 		next = rb_next(node);
 		rblist__remove_node(&groups, node);
 	}
-	if (!metricgroups)
-		metricgroup__print_strlist(metriclist, raw);
-	strlist__delete(metriclist);
 }
 
 static const char *code_characters = ",-=@";
diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
index 732d3a0d3334..0013cf582173 100644
--- a/tools/perf/util/metricgroup.h
+++ b/tools/perf/util/metricgroup.h
@@ -10,6 +10,7 @@
 struct evlist;
 struct evsel;
 struct option;
+struct print_callbacks;
 struct rblist;
 struct cgroup;
 
@@ -78,8 +79,7 @@ int metricgroup__parse_groups_test(struct evlist *evlist,
 				   bool metric_no_merge,
 				   struct rblist *metric_events);
 
-void metricgroup__print(bool metrics, bool groups, char *filter,
-			bool raw, bool details, const char *pmu_name);
+void metricgroup__print(const struct print_callbacks *print_cb, void *print_state);
 bool metricgroup__has_metric(const char *metric);
 int arch_get_runtimeparam(const struct pmu_event *pe __maybe_unused);
 void metricgroup__rblist_exit(struct rblist *metric_events);
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 8322395c9cf7..b7a34dd28875 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -23,6 +23,7 @@
 #include "evsel.h"
 #include "pmu.h"
 #include "parse-events.h"
+#include "print-events.h"
 #include "header.h"
 #include "string2.h"
 #include "strbuf.h"
@@ -1578,13 +1579,6 @@ static char *format_alias(char *buf, int len, const struct perf_pmu *pmu,
 	return buf;
 }
 
-static char *format_alias_or(char *buf, int len, const struct perf_pmu *pmu,
-			     const struct perf_pmu_alias *alias)
-{
-	snprintf(buf, len, "%s OR %s/%s/", alias->name, pmu->name, alias->name);
-	return buf;
-}
-
 /** Struct for ordering events as output in perf list. */
 struct sevent {
 	/** PMU for event. */
@@ -1628,7 +1622,7 @@ static int cmp_sevent(const void *a, const void *b)
 
 	/* Order CPU core events to be first */
 	if (as->is_cpu != bs->is_cpu)
-		return bs->is_cpu - as->is_cpu;
+		return as->is_cpu ? -1 : 1;
 
 	/* Order by PMU name. */
 	a_pmu_name = as->pmu->name ?: "";
@@ -1641,27 +1635,6 @@ static int cmp_sevent(const void *a, const void *b)
 	return strcmp(a_name, b_name);
 }
 
-static void wordwrap(char *s, int start, int max, int corr)
-{
-	int column = start;
-	int n;
-
-	while (*s) {
-		int wlen = strcspn(s, " \t");
-
-		if (column + wlen >= max && column > start) {
-			printf("\n%*s", start, "");
-			column = start + corr;
-		}
-		n = printf("%s%.*s", column > start ? " " : "", wlen, s);
-		if (n <= 0)
-			break;
-		s += wlen;
-		column += n;
-		s = skip_spaces(s);
-	}
-}
-
 bool is_pmu_core(const char *name)
 {
 	return !strcmp(name, "cpu") || is_arm_pmu_core(name);
@@ -1684,24 +1657,19 @@ static bool pmu_alias_is_duplicate(struct sevent *alias_a,
 	return strcmp(a_pmu_name, b_pmu_name) == 0;
 }
 
-void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
-			bool long_desc, bool details_flag, bool deprecated,
-			const char *pmu_name)
+void print_pmu_events(const struct print_callbacks *print_cb, void *print_state)
 {
 	struct perf_pmu *pmu;
-	struct perf_pmu_alias *alias;
+	struct perf_pmu_alias *event;
 	char buf[1024];
 	int printed = 0;
 	int len, j;
 	struct sevent *aliases;
-	int numdesc = 0;
-	int columns = pager_get_columns();
-	char *topic = NULL;
 
 	pmu = NULL;
 	len = 0;
 	while ((pmu = perf_pmu__scan(pmu)) != NULL) {
-		list_for_each_entry(alias, &pmu->aliases, list)
+		list_for_each_entry(event, &pmu->aliases, list)
 			len++;
 		if (pmu->selectable)
 			len++;
@@ -1714,32 +1682,15 @@ 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) {
-		bool is_cpu;
+		bool is_cpu = is_pmu_core(pmu->name) || pmu->is_hybrid;
 
-		if (pmu_name && pmu->name && strcmp(pmu_name, pmu->name))
-			continue;
-
-		is_cpu = is_pmu_core(pmu->name) || pmu->is_hybrid;
-
-		list_for_each_entry(alias, &pmu->aliases, list) {
-			if (alias->deprecated && !deprecated)
-				continue;
-
-			if (event_glob != NULL &&
-			    !(strglobmatch_nocase(alias->name, event_glob) ||
-			      (!is_cpu &&
-			       strglobmatch_nocase(alias->name, event_glob)) ||
-			      (alias->topic &&
-			       strglobmatch_nocase(alias->topic, event_glob))))
-				continue;
-
-			aliases[j].event = alias;
+		list_for_each_entry(event, &pmu->aliases, list) {
+			aliases[j].event = event;
 			aliases[j].pmu = pmu;
 			aliases[j].is_cpu = is_cpu;
 			j++;
 		}
-		if (pmu->selectable &&
-		    (event_glob == NULL || strglobmatch(pmu->name, event_glob))) {
+		if (pmu->selectable) {
 			aliases[j].event = NULL;
 			aliases[j].pmu = pmu;
 			aliases[j].is_cpu = is_cpu;
@@ -1749,7 +1700,11 @@ void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
 	len = j;
 	qsort(aliases, len, sizeof(struct sevent), cmp_sevent);
 	for (j = 0; j < len; j++) {
-		char *name, *desc;
+		const char *name, *alias = NULL, *desc = NULL, *long_desc = NULL,
+			*encoding_desc = NULL, *topic = NULL,
+			*metric_name = NULL, *metric_expr = NULL;
+		bool deprecated = false;
+		size_t buf_used;
 
 		/* Skip duplicates */
 		if (j > 0 && pmu_alias_is_duplicate(&aliases[j], &aliases[j - 1]))
@@ -1757,48 +1712,44 @@ void print_pmu_events(const char *event_glob, bool name_only, bool quiet_flag,
 
 		if (!aliases[j].event) {
 			/* A selectable event. */
-			snprintf(buf, sizeof(buf), "%s//", aliases[j].pmu->name);
+			buf_used = snprintf(buf, sizeof(buf), "%s//", aliases[j].pmu->name) + 1;
 			name = buf;
-		} else if (aliases[j].event->desc) {
-			name = aliases[j].event->name;
 		} else {
-			if (!name_only && aliases[j].is_cpu) {
-				name = format_alias_or(buf, sizeof(buf), aliases[j].pmu,
-						       aliases[j].event);
+			if (aliases[j].event->desc) {
+				name = aliases[j].event->name;
+				buf_used = 0;
 			} else {
 				name = format_alias(buf, sizeof(buf), aliases[j].pmu,
 						    aliases[j].event);
+				if (aliases[j].is_cpu) {
+					alias = name;
+					name = aliases[j].event->name;
+				}
+				buf_used = strlen(buf) + 1;
 			}
-		}
-		if (name_only) {
-			printf("%s ", name);
-			continue;
-		}
-		printed++;
-		if (!aliases[j].event || !aliases[j].event->desc || quiet_flag) {
-			printf("  %-50s [Kernel PMU event]\n", name);
-			continue;
-		}
-		if (numdesc++ == 0)
-			printf("\n");
-		if (aliases[j].event->topic && (!topic ||
-						strcmp(topic, aliases[j].event->topic))) {
-			printf("%s%s:\n", topic ? "\n" : "", aliases[j].event->topic);
+			desc = aliases[j].event->desc;
+			long_desc = aliases[j].event->long_desc;
 			topic = aliases[j].event->topic;
+			encoding_desc = buf + buf_used;
+			buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
+					"%s/%s/", aliases[j].pmu->name,
+					aliases[j].event->str) + 1;
+			metric_name = aliases[j].event->metric_name;
+			metric_expr = aliases[j].event->metric_expr;
+			deprecated = aliases[j].event->deprecated;
 		}
-		printf("  %-50s\n", name);
-		printf("%*s", 8, "[");
-		desc = long_desc ? aliases[j].event->long_desc : aliases[j].event->desc;
-		wordwrap(desc, 8, columns, 0);
-		printf("]\n");
-		if (details_flag) {
-			printf("%*s%s/%s/ ", 8, "", aliases[j].pmu->name, aliases[j].event->str);
-			if (aliases[j].event->metric_name)
-				printf(" MetricName: %s", aliases[j].event->metric_name);
-			if (aliases[j].event->metric_expr)
-				printf(" MetricExpr: %s", aliases[j].event->metric_expr);
-			putchar('\n');
-		}
+		print_cb->print_event(print_state,
+				aliases[j].pmu->name,
+				topic,
+				name,
+				alias,
+				deprecated,
+				"Kernel PMU event",
+				desc,
+				long_desc,
+				encoding_desc,
+				metric_name,
+				metric_expr);
 	}
 	if (printed && pager_in_use())
 		printf("\n");
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 29571c0f9d15..4f9b0202be2d 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -12,6 +12,7 @@
 
 struct evsel_config_term;
 struct perf_cpu_map;
+struct print_callbacks;
 
 enum {
 	PERF_PMU_FORMAT_VALUE_CONFIG,
@@ -209,9 +210,7 @@ void perf_pmu__del_formats(struct list_head *formats);
 struct perf_pmu *perf_pmu__scan(struct perf_pmu *pmu);
 
 bool is_pmu_core(const char *name);
-void print_pmu_events(const char *event_glob, bool name_only, bool quiet,
-		      bool long_desc, bool details_flag,
-		      bool deprecated, const char *pmu_name);
+void print_pmu_events(const struct print_callbacks *print_cb, void *print_state);
 bool pmu_have_event(const char *pname, const char *name);
 
 int perf_pmu__scan_file(struct perf_pmu *pmu, const char *name, const char *fmt, ...) __scanf(3, 4);
diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
index d53dba033597..9aa53e43bda0 100644
--- a/tools/perf/util/print-events.c
+++ b/tools/perf/util/print-events.c
@@ -28,6 +28,7 @@
 
 #define MAX_NAME_LEN 100
 
+/** Strings corresponding to enum perf_type_id. */
 static const char * const event_type_descriptors[] = {
 	"Hardware event",
 	"Software event",
@@ -55,11 +56,9 @@ static const struct event_symbol event_symbols_tool[PERF_TOOL_MAX] = {
 /*
  * Print the events from <debugfs_mount_point>/tracing/events
  */
-void print_tracepoint_events(const char *subsys_glob,
-			     const char *event_glob, bool name_only)
+void print_tracepoint_events(const struct print_callbacks *print_cb, void *print_state)
 {
 	struct dirent **sys_namelist = NULL;
-	bool printed = false;
 	int sys_items = tracing_events__scandir_alphasort(&sys_namelist);
 
 	for (int i = 0; i < sys_items; i++) {
@@ -73,10 +72,6 @@ void print_tracepoint_events(const char *subsys_glob,
 		    !strcmp(sys_dirent->d_name, ".."))
 			continue;
 
-		if (subsys_glob != NULL &&
-		    !strglobmatch(sys_dirent->d_name, subsys_glob))
-			continue;
-
 		dir_path = get_events_file(sys_dirent->d_name);
 		if (!dir_path)
 			continue;
@@ -94,41 +89,40 @@ void print_tracepoint_events(const char *subsys_glob,
 			if (tp_event_has_id(dir_path, evt_dirent) != 0)
 				continue;
 
-			if (event_glob != NULL &&
-			    !strglobmatch(evt_dirent->d_name, event_glob))
-				continue;
-
 			snprintf(evt_path, MAXPATHLEN, "%s:%s",
 				 sys_dirent->d_name, evt_dirent->d_name);
-			if (name_only)
-				printf("%s ", evt_path);
-			else {
-				printf("  %-50s [%s]\n", evt_path,
-				       event_type_descriptors[PERF_TYPE_TRACEPOINT]);
-			}
-			printed = true;
+			print_cb->print_event(print_state,
+					/*topic=*/NULL,
+					/*pmu_name=*/NULL,
+					evt_path,
+					/*event_alias=*/NULL,
+					/*deprecated=*/false,
+					"Tracepoint event",
+					/*desc=*/NULL,
+					/*long_desc=*/NULL,
+					/*encoding_desc=*/NULL,
+					/*metric_name=*/NULL,
+					/*metric_expr=*/NULL);
 		}
 		free(dir_path);
 		free(evt_namelist);
 	}
 	free(sys_namelist);
-	if (printed && pager_in_use())
-		printf("\n");
 }
 
-void print_sdt_events(const char *subsys_glob, const char *event_glob,
-		      bool name_only)
+void print_sdt_events(const struct print_callbacks *print_cb, void *print_state)
 {
-	struct probe_cache *pcache;
-	struct probe_cache_entry *ent;
 	struct strlist *bidlist, *sdtlist;
-	struct strlist_config cfg = {.dont_dupstr = true};
-	struct str_node *nd, *nd2;
-	char *buf, *path, *ptr = NULL;
-	bool show_detail = false;
-	int ret;
-
-	sdtlist = strlist__new(NULL, &cfg);
+	struct str_node *bid_nd, *sdt_name, *next_sdt_name;
+	const char *last_sdt_name = NULL;
+
+	/*
+	 * The implicitly sorted sdtlist will hold the tracepoint name followed
+	 * by @<buildid>. If the tracepoint name is unique (determined by
+	 * looking at the adjacent nodes) the @<buildid> is dropped otherwise
+	 * the executable path and buildid are added to the name.
+	 */
+	sdtlist = strlist__new(NULL, NULL);
 	if (!sdtlist) {
 		pr_debug("Failed to allocate new strlist for SDT\n");
 		return;
@@ -138,65 +132,76 @@ void print_sdt_events(const char *subsys_glob, const char *event_glob,
 		pr_debug("Failed to get buildids: %d\n", errno);
 		return;
 	}
-	strlist__for_each_entry(nd, bidlist) {
-		pcache = probe_cache__new(nd->s, NULL);
+	strlist__for_each_entry(bid_nd, bidlist) {
+		struct probe_cache *pcache;
+		struct probe_cache_entry *ent;
+
+		pcache = probe_cache__new(bid_nd->s, NULL);
 		if (!pcache)
 			continue;
 		list_for_each_entry(ent, &pcache->entries, node) {
-			if (!ent->sdt)
-				continue;
-			if (subsys_glob &&
-			    !strglobmatch(ent->pev.group, subsys_glob))
-				continue;
-			if (event_glob &&
-			    !strglobmatch(ent->pev.event, event_glob))
-				continue;
-			ret = asprintf(&buf, "%s:%s@%s", ent->pev.group,
-					ent->pev.event, nd->s);
-			if (ret > 0)
-				strlist__add(sdtlist, buf);
+			char buf[1024];
+
+			snprintf(buf, sizeof(buf), "%s:%s@%s",
+				 ent->pev.group, ent->pev.event, bid_nd->s);
+			strlist__add(sdtlist, buf);
 		}
 		probe_cache__delete(pcache);
 	}
 	strlist__delete(bidlist);
 
-	strlist__for_each_entry(nd, sdtlist) {
-		buf = strchr(nd->s, '@');
-		if (buf)
-			*(buf++) = '\0';
-		if (name_only) {
-			printf("%s ", nd->s);
-			continue;
-		}
-		nd2 = strlist__next(nd);
-		if (nd2) {
-			ptr = strchr(nd2->s, '@');
-			if (ptr)
-				*ptr = '\0';
-			if (strcmp(nd->s, nd2->s) == 0)
-				show_detail = true;
+	strlist__for_each_entry(sdt_name, sdtlist) {
+		bool show_detail = false;
+		char *bid = strchr(sdt_name->s, '@');
+		char *evt_name = NULL;
+
+		if (bid)
+			*(bid++) = '\0';
+
+		if (last_sdt_name && !strcmp(last_sdt_name, sdt_name->s)) {
+			show_detail = true;
+		} else {
+			next_sdt_name = strlist__next(sdt_name);
+			if (next_sdt_name) {
+				char *bid2 = strchr(next_sdt_name->s, '@');
+
+				if (bid2)
+					*bid2 = '\0';
+				if (strcmp(sdt_name->s, next_sdt_name->s) == 0)
+					show_detail = true;
+				if (bid2)
+					*bid2 = '@';
+			}
 		}
+		last_sdt_name = sdt_name->s;
+
 		if (show_detail) {
-			path = build_id_cache__origname(buf);
-			ret = asprintf(&buf, "%s@%s(%.12s)", nd->s, path, buf);
-			if (ret > 0) {
-				printf("  %-50s [%s]\n", buf, "SDT event");
-				free(buf);
+			char *path = build_id_cache__origname(bid);
+
+			if (path) {
+				asprintf(&evt_name, "%s@%s(%.12s)", sdt_name->s, path, bid);
+				free(path);
 			}
-			free(path);
-		} else
-			printf("  %-50s [%s]\n", nd->s, "SDT event");
-		if (nd2) {
-			if (strcmp(nd->s, nd2->s) != 0)
-				show_detail = false;
-			if (ptr)
-				*ptr = '@';
 		}
+		print_cb->print_event(print_state,
+				/*topic=*/NULL,
+				/*pmu_name=*/NULL,
+				evt_name ?: sdt_name->s,
+				/*event_alias=*/NULL,
+				/*deprecated=*/false,
+				"SDT event",
+				/*desc=*/NULL,
+				/*long_desc=*/NULL,
+				/*encoding_desc=*/NULL,
+				/*metric_name=*/NULL,
+				/*metric_expr=*/NULL);
+
+		free(evt_name);
 	}
 	strlist__delete(sdtlist);
 }
 
-int print_hwcache_events(const char *event_glob, bool name_only)
+int print_hwcache_events(const struct print_callbacks *print_cb, void *print_state)
 {
 	struct strlist *evt_name_list = strlist__new(NULL, NULL);
 	struct str_node *nd;
@@ -216,9 +221,6 @@ int print_hwcache_events(const char *event_glob, bool name_only)
 				char name[64];
 
 				__evsel__hw_cache_type_op_res_name(type, op, i, name, sizeof(name));
-				if (event_glob != NULL && !strglobmatch(name, event_glob))
-					continue;
-
 				if (!perf_pmu__has_hybrid()) {
 					if (is_event_supported(PERF_TYPE_HW_CACHE,
 							       type | (op << 8) | (i << 16)))
@@ -240,55 +242,45 @@ int print_hwcache_events(const char *event_glob, bool name_only)
 	}
 
 	strlist__for_each_entry(nd, evt_name_list) {
-		if (name_only) {
-			printf("%s ", nd->s);
-			continue;
-		}
-		printf("  %-50s [%s]\n", nd->s, event_type_descriptors[PERF_TYPE_HW_CACHE]);
+		print_cb->print_event(print_state,
+				"cache",
+				/*pmu_name=*/NULL,
+				nd->s,
+				/*event_alias=*/NULL,
+				/*deprecated=*/false,
+				event_type_descriptors[PERF_TYPE_HW_CACHE],
+				/*desc=*/NULL,
+				/*long_desc=*/NULL,
+				/*encoding_desc=*/NULL,
+				/*metric_name=*/NULL,
+				/*metric_expr=*/NULL);
 	}
-	if (!strlist__empty(evt_name_list) && pager_in_use())
-		printf("\n");
-
 	strlist__delete(evt_name_list);
 	return 0;
 }
 
-static void print_tool_event(const struct event_symbol *syms, const char *event_glob,
-			     bool name_only)
-{
-	if (syms->symbol == NULL)
-		return;
-
-	if (event_glob && !(strglobmatch(syms->symbol, event_glob) ||
-	      (syms->alias && strglobmatch(syms->alias, event_glob))))
-		return;
-
-	if (name_only)
-		printf("%s ", syms->symbol);
-	else {
-		char name[MAX_NAME_LEN];
-
-		if (syms->alias && strlen(syms->alias))
-			snprintf(name, MAX_NAME_LEN, "%s OR %s", syms->symbol, syms->alias);
-		else
-			strlcpy(name, syms->symbol, MAX_NAME_LEN);
-		printf("  %-50s [%s]\n", name, "Tool event");
-	}
-}
-
-void print_tool_events(const char *event_glob, bool name_only)
+void print_tool_events(const struct print_callbacks *print_cb, void *print_state)
 {
 	// Start at 1 because the first enum entry means no tool event.
-	for (int i = 1; i < PERF_TOOL_MAX; ++i)
-		print_tool_event(event_symbols_tool + i, event_glob, name_only);
-
-	if (pager_in_use())
-		printf("\n");
+	for (int i = 1; i < PERF_TOOL_MAX; ++i) {
+		print_cb->print_event(print_state,
+				"tool",
+				/*pmu_name=*/NULL,
+				event_symbols_tool[i].symbol,
+				event_symbols_tool[i].alias,
+				/*deprecated=*/false,
+				"Tool event",
+				/*desc=*/NULL,
+				/*long_desc=*/NULL,
+				/*encoding_desc=*/NULL,
+				/*metric_name=*/NULL,
+				/*metric_expr=*/NULL);
+	}
 }
 
-void print_symbol_events(const char *event_glob, unsigned int type,
-			 struct event_symbol *syms, unsigned int max,
-			 bool name_only)
+void print_symbol_events(const struct print_callbacks *print_cb, void *print_state,
+			 unsigned int type, const struct event_symbol *syms,
+			 unsigned int max)
 {
 	struct strlist *evt_name_list = strlist__new(NULL, NULL);
 	struct str_node *nd;
@@ -305,10 +297,6 @@ void print_symbol_events(const char *event_glob, unsigned int type,
 		if (syms[i].symbol == NULL)
 			continue;
 
-		if (event_glob != NULL && !(strglobmatch(syms[i].symbol, event_glob) ||
-		      (syms[i].alias && strglobmatch(syms[i].alias, event_glob))))
-			continue;
-
 		if (!is_event_supported(type, i))
 			continue;
 
@@ -322,63 +310,88 @@ void print_symbol_events(const char *event_glob, unsigned int type,
 	}
 
 	strlist__for_each_entry(nd, evt_name_list) {
-		if (name_only) {
-			printf("%s ", nd->s);
-			continue;
+		char *alias = strstr(nd->s, " OR ");
+
+		if (alias) {
+			*alias = '\0';
+			alias += 4;
 		}
-		printf("  %-50s [%s]\n", nd->s, event_type_descriptors[type]);
+		print_cb->print_event(print_state,
+				/*topic=*/NULL,
+				/*pmu_name=*/NULL,
+				nd->s,
+				alias,
+				/*deprecated=*/false,
+				event_type_descriptors[type],
+				/*desc=*/NULL,
+				/*long_desc=*/NULL,
+				/*encoding_desc=*/NULL,
+				/*metric_name=*/NULL,
+				/*metric_expr=*/NULL);
 	}
-	if (!strlist__empty(evt_name_list) && pager_in_use())
-		printf("\n");
-
 	strlist__delete(evt_name_list);
 }
 
 /*
  * Print the help text for the event symbols:
  */
-void print_events(const char *event_glob, bool name_only, bool quiet_flag,
-			bool long_desc, bool details_flag, bool deprecated,
-			const char *pmu_name)
+void print_events(const struct print_callbacks *print_cb, void *print_state)
 {
-	print_symbol_events(event_glob, PERF_TYPE_HARDWARE,
-			    event_symbols_hw, PERF_COUNT_HW_MAX, name_only);
-
-	print_symbol_events(event_glob, PERF_TYPE_SOFTWARE,
-			    event_symbols_sw, PERF_COUNT_SW_MAX, name_only);
-	print_tool_events(event_glob, name_only);
-
-	print_hwcache_events(event_glob, name_only);
-
-	print_pmu_events(event_glob, name_only, quiet_flag, long_desc,
-			details_flag, deprecated, pmu_name);
-
-	if (event_glob != NULL)
-		return;
-
-	if (!name_only) {
-		printf("  %-50s [%s]\n",
-		       "rNNN",
-		       event_type_descriptors[PERF_TYPE_RAW]);
-		printf("  %-50s [%s]\n",
-		       "cpu/t1=v1[,t2=v2,t3 ...]/modifier",
-		       event_type_descriptors[PERF_TYPE_RAW]);
-		if (pager_in_use())
-			printf("   (see 'man perf-list' on how to encode it)\n\n");
-
-		printf("  %-50s [%s]\n",
-		       "mem:<addr>[/len][:access]",
-			event_type_descriptors[PERF_TYPE_BREAKPOINT]);
-		if (pager_in_use())
-			printf("\n");
-	}
-
-	print_tracepoint_events(NULL, NULL, name_only);
-
-	print_sdt_events(NULL, NULL, name_only);
-
-	metricgroup__print(true, true, NULL, name_only, details_flag,
-			   pmu_name);
-
-	print_libpfm_events(name_only, long_desc);
+	print_symbol_events(print_cb, print_state, PERF_TYPE_HARDWARE,
+			event_symbols_hw, PERF_COUNT_HW_MAX);
+	print_symbol_events(print_cb, print_state, PERF_TYPE_SOFTWARE,
+			event_symbols_sw, PERF_COUNT_SW_MAX);
+
+	print_tool_events(print_cb, print_state);
+
+	print_hwcache_events(print_cb, print_state);
+
+	print_pmu_events(print_cb, print_state);
+
+	print_cb->print_event(print_state,
+			/*topic=*/NULL,
+			/*pmu_name=*/NULL,
+			"rNNN",
+			/*event_alias=*/NULL,
+			/*deprecated=*/false,
+			event_type_descriptors[PERF_TYPE_RAW],
+			/*desc=*/NULL,
+			/*long_desc=*/NULL,
+			/*encoding_desc=*/NULL,
+			/*metric_name=*/NULL,
+			/*metric_expr=*/NULL);
+
+	print_cb->print_event(print_state,
+			/*topic=*/NULL,
+			/*pmu_name=*/NULL,
+			"cpu/t1=v1[,t2=v2,t3 ...]/modifier",
+			/*event_alias=*/NULL,
+			/*deprecated=*/false,
+			event_type_descriptors[PERF_TYPE_RAW],
+			"(see 'man perf-list' on how to encode it)",
+			/*long_desc=*/NULL,
+			/*encoding_desc=*/NULL,
+			/*metric_name=*/NULL,
+			/*metric_expr=*/NULL);
+
+	print_cb->print_event(print_state,
+			/*topic=*/NULL,
+			/*pmu_name=*/NULL,
+			"mem:<addr>[/len][:access]",
+			/*event_alias=*/NULL,
+			/*deprecated=*/false,
+			event_type_descriptors[PERF_TYPE_BREAKPOINT],
+			/*desc=*/NULL,
+			/*long_desc=*/NULL,
+			/*encoding_desc=*/NULL,
+			/*metric_name=*/NULL,
+			/*metric_expr=*/NULL);
+
+	print_tracepoint_events(print_cb, print_state);
+
+	print_sdt_events(print_cb, print_state);
+
+	metricgroup__print(print_cb, print_state);
+
+	print_libpfm_events(print_cb, print_state);
 }
diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.h
index 1da9910d83a6..235c3e123079 100644
--- a/tools/perf/util/print-events.h
+++ b/tools/perf/util/print-events.h
@@ -2,21 +2,37 @@
 #ifndef __PERF_PRINT_EVENTS_H
 #define __PERF_PRINT_EVENTS_H
 
+#include <linux/perf_event.h>
 #include <stdbool.h>
 
 struct event_symbol;
 
-void print_events(const char *event_glob, bool name_only, bool quiet_flag,
-		  bool long_desc, bool details_flag, bool deprecated,
-		  const char *pmu_name);
-int print_hwcache_events(const char *event_glob, bool name_only);
-void print_sdt_events(const char *subsys_glob, const char *event_glob,
-		      bool name_only);
-void print_symbol_events(const char *event_glob, unsigned int type,
-			 struct event_symbol *syms, unsigned int max,
-			 bool name_only);
-void print_tool_events(const char *event_glob, bool name_only);
-void print_tracepoint_events(const char *subsys_glob, const char *event_glob,
-			     bool name_only);
+struct print_callbacks {
+	void (*print_start)(void *print_state);
+	void (*print_end)(void *print_state);
+	void (*print_event)(void *print_state, const char *topic,
+			const char *pmu_name,
+			const char *event_name, const char *event_alias,
+			bool deprecated, const char *event_type_desc,
+			const char *desc, const char *long_desc,
+			const char *encoding_desc,
+			const char *metric_name, const char *metric_expr);
+	void (*print_metric)(void *print_state,
+			const char *group,
+			const char *name,
+			const char *desc,
+			const char *long_desc,
+			const char *expr);
+};
+
+/** Print all events, the default when no options are specified. */
+void print_events(const struct print_callbacks *print_cb, void *print_state);
+int print_hwcache_events(const struct print_callbacks *print_cb, void *print_state);
+void print_sdt_events(const struct print_callbacks *print_cb, void *print_state);
+void print_symbol_events(const struct print_callbacks *print_cb, void *print_state,
+			 unsigned int type, const struct event_symbol *syms,
+			 unsigned int max);
+void print_tool_events(const struct print_callbacks *print_cb, void *print_state);
+void print_tracepoint_events(const struct print_callbacks *print_cb, void *print_state);
 
 #endif /* __PERF_PRINT_EVENTS_H */
-- 
2.38.1.431.g37b22c650d-goog

[PATCH v1 9/9] perf list: Add json output option

[Ian Rogers]

Output events and metrics in a json format by overriding the print
callbacks. Currently other command line options aren't supported and
metrics are repeated once per metric group.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/Documentation/perf-list.txt |   4 +
 tools/perf/builtin-list.c              | 283 ++++++++++++++++++++-----
 2 files changed, 229 insertions(+), 58 deletions(-)

diff --git a/tools/perf/Documentation/perf-list.txt b/tools/perf/Documentation/perf-list.txt
index 44a819af573d..43263ca88ff7 100644
--- a/tools/perf/Documentation/perf-list.txt
+++ b/tools/perf/Documentation/perf-list.txt
@@ -43,6 +43,10 @@ Print deprecated events. By default the deprecated events are hidden.
 Print PMU events and metrics limited to the specific PMU name.
 (e.g. --unit cpu, --unit msr, --unit cpu_core, --unit cpu_atom)
 
+-j::
+--json::
+Output in json format.
+
 [[EVENT_MODIFIERS]]
 EVENT MODIFIERS
 ---------------
diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index 91e2b6f52548..910b5c3a7365 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -19,6 +19,7 @@
 #include "util/strlist.h"
 #include <subcmd/pager.h>
 #include <subcmd/parse-options.h>
+#include <stdarg.h>
 #include <stdio.h>
 
 struct print_state {
@@ -217,10 +218,165 @@ static void default_print_metric(void *ps,
 	}
 }
 
+struct json_print_state {
+	/** Should a separator be printed prior to the next item? */
+	bool need_sep;
+};
+
+static void json_print_start(void *print_state __maybe_unused)
+{
+	printf("[\n");
+}
+
+static void json_print_end(void *ps)
+{
+	struct json_print_state *print_state = ps;
+
+	printf("%s]\n", print_state->need_sep ? "\n" : "");
+}
+
+static void fix_escape_printf(const char *fmt, ...)
+{
+	va_list args;
+	char buf[2048];
+	size_t buf_pos = 0;
+
+	va_start(args, fmt);
+	for (size_t fmt_pos = 0; fmt_pos < strlen(fmt); fmt_pos++) {
+		switch (fmt[fmt_pos]) {
+		case '%': {
+			const char *s = va_arg(args, const char*);
+
+			fmt_pos++;
+			assert(fmt[fmt_pos] == 's');
+			for (size_t s_pos = 0; s_pos < strlen(s); s_pos++) {
+				switch (s[s_pos]) {
+				case '\\':
+					__fallthrough;
+				case '\"':
+					buf[buf_pos++] = '\\';
+					assert(buf_pos < sizeof(buf));
+					__fallthrough;
+				default:
+					buf[buf_pos++] = s[s_pos];
+					assert(buf_pos < sizeof(buf));
+					break;
+				}
+			}
+			break;
+		}
+		default:
+			buf[buf_pos++] = fmt[fmt_pos];
+			assert(buf_pos < sizeof(buf));
+			break;
+		}
+	}
+	va_end(args);
+	buf[buf_pos] = '\0';
+	fputs(buf, stdout);
+}
+
+static void json_print_event(void *ps, const char *pmu_name, const char *topic,
+			     const char *event_name, const char *event_alias,
+			     bool deprecated, const char *event_type_desc,
+			     const char *desc, const char *long_desc,
+			     const char *encoding_desc,
+			     const char *metric_name, const char *metric_expr)
+{
+	struct json_print_state *print_state = ps;
+	bool need_sep = false;
+
+	printf("%s{\n", print_state->need_sep ? ",\n" : "");
+	print_state->need_sep = true;
+	if (pmu_name) {
+		fix_escape_printf("\t\"Unit\": \"%s\"", pmu_name);
+		need_sep = true;
+	}
+	if (topic) {
+		fix_escape_printf("%s\t\"Topic\": \"%s\"", need_sep ? ",\n" : "", topic);
+		need_sep = true;
+	}
+	if (event_name) {
+		fix_escape_printf("%s\t\"EventName\": \"%s\"", need_sep ? ",\n" : "", event_name);
+		need_sep = true;
+	}
+	if (event_alias && strlen(event_alias)) {
+		fix_escape_printf("%s\t\"EventAlias\": \"%s\"", need_sep ? ",\n" : "", event_alias);
+		need_sep = true;
+	}
+	if (event_type_desc) {
+		fix_escape_printf("%s\t\"EventType\": \"%s\"", need_sep ? ",\n" : "",
+				  event_type_desc);
+		need_sep = true;
+	}
+	if (deprecated) {
+		fix_escape_printf("%s\t\"Deprecated\": \"%s\"", need_sep ? ",\n" : "",
+				  deprecated ? "1" : "0");
+		need_sep = true;
+	}
+	if (desc) {
+		fix_escape_printf("%s\t\"BriefDescription\": \"%s\"", need_sep ? ",\n" : "", desc);
+		need_sep = true;
+	}
+	if (long_desc) {
+		fix_escape_printf("%s\t\"PublicDescription\": \"%s\"", need_sep ? ",\n" : "",
+				  long_desc);
+		need_sep = true;
+	}
+	if (encoding_desc) {
+		fix_escape_printf("%s\t\"Encoding\": \"%s\"", need_sep ? ",\n" : "", encoding_desc);
+		need_sep = true;
+	}
+	if (metric_name) {
+		fix_escape_printf("%s\t\"MetricName\": \"%s\"", need_sep ? ",\n" : "", metric_name);
+		need_sep = true;
+	}
+	if (metric_expr) {
+		fix_escape_printf("%s\t\"MetricExpr\": \"%s\"", need_sep ? ",\n" : "", metric_expr);
+		need_sep = true;
+	}
+	printf("%s}", need_sep ? "\n" : "");
+}
+
+static void json_print_metric(void *ps __maybe_unused, const char *group,
+			      const char *name, const char *desc,
+			      const char *long_desc, const char *expr)
+{
+	struct json_print_state *print_state = ps;
+	bool need_sep = false;
+
+	printf("%s{\n", print_state->need_sep ? ",\n" : "");
+	print_state->need_sep = true;
+	if (group) {
+		fix_escape_printf("\t\"MetricGroup\": \"%s\"", group);
+		need_sep = true;
+	}
+	if (name) {
+		fix_escape_printf("%s\t\"MetricName\": \"%s\"", need_sep ? ",\n" : "", name);
+		need_sep = true;
+	}
+	if (expr) {
+		fix_escape_printf("%s\t\"MetricExpr\": \"%s\"", need_sep ? ",\n" : "", expr);
+		need_sep = true;
+	}
+	if (desc) {
+		fix_escape_printf("%s\t\"BriefDescription\": \"%s\"", need_sep ? ",\n" : "", desc);
+		need_sep = true;
+	}
+	if (long_desc) {
+		fix_escape_printf("%s\t\"PublicDescription\": \"%s\"", need_sep ? ",\n" : "",
+				  long_desc);
+		need_sep = true;
+	}
+	printf("%s}", need_sep ? "\n" : "");
+}
+
 int cmd_list(int argc, const char **argv)
 {
 	int i, ret = 0;
-	struct print_state ps = {};
+	struct print_state default_ps = {};
+	struct print_state json_ps = {};
+	void *ps = &default_ps;
 	struct print_callbacks print_cb = {
 		.print_start = default_print_start,
 		.print_end = default_print_end,
@@ -229,15 +385,17 @@ int cmd_list(int argc, const char **argv)
 	};
 	const char *hybrid_name = NULL;
 	const char *unit_name = NULL;
+	bool json = false;
 	struct option list_options[] = {
-		OPT_BOOLEAN(0, "raw-dump", &ps.name_only, "Dump raw events"),
-		OPT_BOOLEAN('d', "desc", &ps.desc,
+		OPT_BOOLEAN(0, "raw-dump", &default_ps.name_only, "Dump raw events"),
+		OPT_BOOLEAN('j', "json", &json, "JSON encode events and metrics"),
+		OPT_BOOLEAN('d', "desc", &default_ps.desc,
 			    "Print extra event descriptions. --no-desc to not print."),
-		OPT_BOOLEAN('v', "long-desc", &ps.long_desc,
+		OPT_BOOLEAN('v', "long-desc", &default_ps.long_desc,
 			    "Print longer event descriptions."),
-		OPT_BOOLEAN(0, "details", &ps.detailed,
+		OPT_BOOLEAN(0, "details", &default_ps.detailed,
 			    "Print information on the perf event names and expressions used internally by events."),
-		OPT_BOOLEAN(0, "deprecated", &ps.deprecated,
+		OPT_BOOLEAN(0, "deprecated", &default_ps.deprecated,
 			    "Print deprecated events."),
 		OPT_STRING(0, "cputype", &hybrid_name, "hybrid cpu type",
 			   "Limit PMU or metric printing to the given hybrid PMU (e.g. core or atom)."),
@@ -261,28 +419,37 @@ int cmd_list(int argc, const char **argv)
 
 	setup_pager();
 
-	if (!ps.name_only)
+	if (!default_ps.name_only)
 		setup_pager();
 
-	ps.desc = !ps.long_desc;
-	ps.last_topic = strdup("");
-	assert(ps.last_topic);
-	ps.visited_metrics = strlist__new(NULL, NULL);
-	assert(ps.visited_metrics);
-	if (unit_name)
-		ps.pmu_glob = strdup(unit_name);
-	else if (hybrid_name) {
-		ps.pmu_glob = perf_pmu__hybrid_type_to_pmu(hybrid_name);
-		if (!ps.pmu_glob)
-			pr_warning("WARNING: hybrid cputype is not supported!\n");
+	if (json) {
+		print_cb = (struct print_callbacks){
+			.print_start = json_print_start,
+			.print_end = json_print_end,
+			.print_event = json_print_event,
+			.print_metric = json_print_metric,
+		};
+		ps = &json_ps;
+	} else {
+		default_ps.desc = !default_ps.long_desc;
+		default_ps.last_topic = strdup("");
+		assert(default_ps.last_topic);
+		default_ps.visited_metrics = strlist__new(NULL, NULL);
+		assert(default_ps.visited_metrics);
+		if (unit_name)
+			default_ps.pmu_glob = strdup(unit_name);
+		else if (hybrid_name) {
+			default_ps.pmu_glob = perf_pmu__hybrid_type_to_pmu(hybrid_name);
+			if (!default_ps.pmu_glob)
+				pr_warning("WARNING: hybrid cputype is not supported!\n");
+		}
 	}
-
 	print_cb.print_start(&ps);
 
 	if (argc == 0) {
-		ps.metrics = true;
-		ps.metricgroups = true;
-		print_events(&print_cb, &ps);
+		default_ps.metrics = true;
+		default_ps.metricgroups = true;
+		print_events(&print_cb, ps);
 		goto out;
 	}
 
@@ -290,32 +457,32 @@ int cmd_list(int argc, const char **argv)
 		char *sep, *s;
 
 		if (strcmp(argv[i], "tracepoint") == 0)
-			print_tracepoint_events(&print_cb, &ps);
+			print_tracepoint_events(&print_cb, ps);
 		else if (strcmp(argv[i], "hw") == 0 ||
 			 strcmp(argv[i], "hardware") == 0)
-			print_symbol_events(&print_cb, &ps, PERF_TYPE_HARDWARE,
+			print_symbol_events(&print_cb, ps, PERF_TYPE_HARDWARE,
 					event_symbols_hw, PERF_COUNT_HW_MAX);
 		else if (strcmp(argv[i], "sw") == 0 ||
 			 strcmp(argv[i], "software") == 0) {
-			print_symbol_events(&print_cb, &ps, PERF_TYPE_SOFTWARE,
+			print_symbol_events(&print_cb, ps, PERF_TYPE_SOFTWARE,
 					event_symbols_sw, PERF_COUNT_SW_MAX);
-			print_tool_events(&print_cb, &ps);
+			print_tool_events(&print_cb, ps);
 		} else if (strcmp(argv[i], "cache") == 0 ||
 			 strcmp(argv[i], "hwcache") == 0)
-			print_hwcache_events(&print_cb, &ps);
+			print_hwcache_events(&print_cb, ps);
 		else if (strcmp(argv[i], "pmu") == 0)
-			print_pmu_events(&print_cb, &ps);
+			print_pmu_events(&print_cb, ps);
 		else if (strcmp(argv[i], "sdt") == 0)
-			print_sdt_events(&print_cb, &ps);
+			print_sdt_events(&print_cb, ps);
 		else if (strcmp(argv[i], "metric") == 0 || strcmp(argv[i], "metrics") == 0) {
-			ps.metricgroups = false;
-			ps.metrics = true;
-			metricgroup__print(&print_cb, &ps);
+			default_ps.metricgroups = false;
+			default_ps.metrics = true;
+			metricgroup__print(&print_cb, ps);
 		} else if (strcmp(argv[i], "metricgroup") == 0 ||
 			   strcmp(argv[i], "metricgroups") == 0) {
-			ps.metricgroups = true;
-			ps.metrics = false;
-			metricgroup__print(&print_cb, &ps);
+			default_ps.metricgroups = true;
+			default_ps.metrics = false;
+			metricgroup__print(&print_cb, ps);
 		} else if ((sep = strchr(argv[i], ':')) != NULL) {
 			int sep_idx;
 
@@ -327,41 +494,41 @@ int cmd_list(int argc, const char **argv)
 			}
 
 			s[sep_idx] = '\0';
-			ps.pmu_glob = s;
-			ps.event_glob = s + sep_idx + 1;
-			print_tracepoint_events(&print_cb, &ps);
-			print_sdt_events(&print_cb, &ps);
-			ps.metrics = true;
-			ps.metricgroups = true;
-			metricgroup__print(&print_cb, &ps);
+			default_ps.pmu_glob = s;
+			default_ps.event_glob = s + sep_idx + 1;
+			print_tracepoint_events(&print_cb, ps);
+			print_sdt_events(&print_cb, ps);
+			default_ps.metrics = true;
+			default_ps.metricgroups = true;
+			metricgroup__print(&print_cb, ps);
 			free(s);
 		} else {
 			if (asprintf(&s, "*%s*", argv[i]) < 0) {
 				printf("Critical: Not enough memory! Trying to continue...\n");
 				continue;
 			}
-			ps.event_glob = s;
-			print_symbol_events(&print_cb, &ps, PERF_TYPE_HARDWARE,
+			default_ps.event_glob = s;
+			print_symbol_events(&print_cb, ps, PERF_TYPE_HARDWARE,
 					event_symbols_hw, PERF_COUNT_HW_MAX);
-			print_symbol_events(&print_cb, &ps, PERF_TYPE_SOFTWARE,
+			print_symbol_events(&print_cb, ps, PERF_TYPE_SOFTWARE,
 					event_symbols_sw, PERF_COUNT_SW_MAX);
-			print_tool_events(&print_cb, &ps);
-			print_hwcache_events(&print_cb, &ps);
-			print_pmu_events(&print_cb, &ps);
-			print_tracepoint_events(&print_cb, &ps);
-			print_sdt_events(&print_cb, &ps);
-			ps.metrics = true;
-			ps.metricgroups = true;
-			metricgroup__print(&print_cb, &ps);
+			print_tool_events(&print_cb, ps);
+			print_hwcache_events(&print_cb, ps);
+			print_pmu_events(&print_cb, ps);
+			print_tracepoint_events(&print_cb, ps);
+			print_sdt_events(&print_cb, ps);
+			default_ps.metrics = true;
+			default_ps.metricgroups = true;
+			metricgroup__print(&print_cb, ps);
 			free(s);
 		}
 	}
 
 out:
-	print_cb.print_end(&ps);
-	free(ps.pmu_glob);
-	free(ps.last_topic);
-	free(ps.last_metricgroups);
-	strlist__delete(ps.visited_metrics);
+	print_cb.print_end(ps);
+	free(default_ps.pmu_glob);
+	free(default_ps.last_topic);
+	free(default_ps.last_metricgroups);
+	strlist__delete(default_ps.visited_metrics);
 	return ret;
 }
-- 
2.38.1.431.g37b22c650d-goog