[PATCH 1/3] perf list: Remove dead code in argument check

[PATCH 1/3] perf list: Remove dead code in argument check

[Namhyung Kim]

The sep is already checked being not NULL.  The code seems to be a
leftover from some refactoring.

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

diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index 0a7fe4cb5555..10ab5e40a34f 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -92,13 +92,6 @@ int cmd_list(int argc, const char **argv)
 		else if ((sep = strchr(argv[i], ':')) != NULL) {
 			int sep_idx;
 
-			if (sep == NULL) {
-				print_events(argv[i], raw_dump, !desc_flag,
-							long_desc_flag,
-							details_flag,
-							deprecated);
-				continue;
-			}
 			sep_idx = sep - argv[i];
 			s = strdup(argv[i]);
 			if (s == NULL)
-- 
2.28.0.526.ge36021eeef-goog

[PATCH 2/3] perf list: Do not print 'Metric Groups:' unnecessarily

[Namhyung Kim]

It was printed unconditionally even if nothing is printed.
Check if the output list empty when filter is given.

Before:
  $ ./perf list duration

  List of pre-defined events (to be used in -e):

    duration_time                                      [Tool event]

  Metric Groups:

After:
  $ ./perf list duration

  List of pre-defined events (to be used in -e):

    duration_time                                      [Tool event]

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 8831b964288f..38464d7d2d63 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -535,10 +535,12 @@ void metricgroup__print(bool metrics, bool metricgroups, char *filter,
 		}
 	}
 
-	if (metricgroups && !raw)
-		printf("\nMetric Groups:\n\n");
-	else if (metrics && !raw)
-		printf("\nMetrics:\n\n");
+	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);
-- 
2.28.0.526.ge36021eeef-goog

Re: [PATCH 2/3] perf list: Do not print 'Metric Groups:' unnecessarily

[Arnaldo Carvalho de Melo]

Em Wed, Sep 09, 2020 at 02:58:48PM +0900, Namhyung Kim escreveu:
> It was printed unconditionally even if nothing is printed.
> Check if the output list empty when filter is given.
> 
> Before:
>   $ ./perf list duration
> 
>   List of pre-defined events (to be used in -e):
> 
>     duration_time                                      [Tool event]
> 
>   Metric Groups:
> 
> After:
>   $ ./perf list duration
> 
>   List of pre-defined events (to be used in -e):
> 
>     duration_time                                      [Tool event]
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

---

Thanks, applied.

- Arnaldo
> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 8831b964288f..38464d7d2d63 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -535,10 +535,12 @@ void metricgroup__print(bool metrics, bool metricgroups, char *filter,
>  		}
>  	}
>  
> -	if (metricgroups && !raw)
> -		printf("\nMetric Groups:\n\n");
> -	else if (metrics && !raw)
> -		printf("\nMetrics:\n\n");
> +	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);
> -- 
> 2.28.0.526.ge36021eeef-goog
> 

-- 

- Arnaldo

[PATCH 3/3] perf list: Add 'pfm' to list libpfm4 events

[Namhyung Kim]

Print libpfm4 events with 'perf list pfm' command like others.
When libpfm4 support is not enabled, it'd print nothing.
Also it support glob pattern matching for event name.

  $ perf list pfm

  List of pre-defined events (to be used in --pfm-events):

  ix86arch:
    UNHALTED_CORE_CYCLES
      [count core clock cycles whenever the clock signal ...
    INSTRUCTION_RETIRED
      [count the number of instructions at retirement. ...
    ...

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

diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index 10ab5e40a34f..167868053fe0 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -14,6 +14,7 @@
 #include "util/pmu.h"
 #include "util/debug.h"
 #include "util/metricgroup.h"
+#include "util/pfm.h"
 #include <subcmd/pager.h>
 #include <subcmd/parse-options.h>
 #include <stdio.h>
@@ -42,7 +43,7 @@ int cmd_list(int argc, const char **argv)
 		OPT_END()
 	};
 	const char * const list_usage[] = {
-		"perf list [<options>] [hw|sw|cache|tracepoint|pmu|sdt|metric|metricgroup|event_glob]",
+		"perf list [<options>] [hw|sw|cache|tracepoint|pmu|sdt|metric|metricgroup|pfm|event_glob]",
 		NULL
 	};
 
@@ -53,7 +54,7 @@ int cmd_list(int argc, const char **argv)
 
 	setup_pager();
 
-	if (!raw_dump && pager_in_use())
+	if (!raw_dump && pager_in_use() && (argc != 1 || strcmp(argv[0], "pfm")))
 		printf("\nList of pre-defined events (to be used in -e):\n\n");
 
 	if (argc == 0) {
@@ -89,6 +90,8 @@ int cmd_list(int argc, const char **argv)
 			metricgroup__print(true, false, NULL, raw_dump, details_flag);
 		else if (strcmp(argv[i], "metricgroup") == 0 || strcmp(argv[i], "metricgroups") == 0)
 			metricgroup__print(false, true, NULL, raw_dump, details_flag);
+		else if (strcmp(argv[i], "pfm") == 0)
+			print_libpfm_events(NULL, raw_dump, long_desc_flag);
 		else if ((sep = strchr(argv[i], ':')) != NULL) {
 			int sep_idx;
 
@@ -120,6 +123,7 @@ int cmd_list(int argc, const char **argv)
 			print_tracepoint_events(NULL, s, raw_dump);
 			print_sdt_events(NULL, s, raw_dump);
 			metricgroup__print(true, true, s, raw_dump, details_flag);
+			print_libpfm_events(s, raw_dump, long_desc_flag);
 			free(s);
 		}
 	}
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 772f1057647f..ae8ab930a792 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -2593,7 +2593,7 @@ static struct option __record_options[] = {
 		     "number of threads to run for event synthesis"),
 #ifdef HAVE_LIBPFM
 	OPT_CALLBACK(0, "pfm-events", &record.evlist, "event",
-		"libpfm4 event selector. use 'perf list' to list available events",
+		"libpfm4 event selector. use 'perf list pfm' to list available events",
 		parse_libpfm_events_option),
 #endif
 	OPT_CALLBACK(0, "control", &record.opts, "fd:ctl-fd[,ack-fd]",
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 483a28ef4ec4..a672d2b68e8a 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1166,7 +1166,7 @@ static struct option stat_options[] = {
 		    "threads of same physical core"),
 #ifdef HAVE_LIBPFM
 	OPT_CALLBACK(0, "pfm-events", &evsel_list, "event",
-		"libpfm4 event selector. use 'perf list' to list available events",
+		"libpfm4 event selector. use 'perf list pfm' to list available events",
 		parse_libpfm_events_option),
 #endif
 	OPT_CALLBACK(0, "control", &stat_config, "fd:ctl-fd[,ack-fd]",
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 7c64134472c7..d6adc7d34210 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1578,7 +1578,7 @@ int cmd_top(int argc, const char **argv)
 		    "Enable LBR callgraph stitching approach"),
 #ifdef HAVE_LIBPFM
 	OPT_CALLBACK(0, "pfm-events", &top.evlist, "event",
-		"libpfm4 event selector. use 'perf list' to list available events",
+		"libpfm4 event selector. use 'perf list pfm' to list available events",
 		parse_libpfm_events_option),
 #endif
 	OPTS_EVSWITCH(&top.evswitch),
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index c4d2394e2b2d..2d426a4f3bc7 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2899,7 +2899,7 @@ void print_events(const char *event_glob, bool name_only, bool quiet_flag,
 
 	metricgroup__print(true, true, NULL, name_only, details_flag);
 
-	print_libpfm_events(name_only, long_desc);
+	print_libpfm_events(NULL, name_only, long_desc);
 }
 
 int parse_events__is_hardcoded_term(struct parse_events_term *term)
diff --git a/tools/perf/util/pfm.c b/tools/perf/util/pfm.c
index d735acb6c29c..26ae2c8c0932 100644
--- a/tools/perf/util/pfm.c
+++ b/tools/perf/util/pfm.c
@@ -12,6 +12,7 @@
 #include "util/parse-events.h"
 #include "util/pmu.h"
 #include "util/pfm.h"
+#include "util/string2.h"
 
 #include <string.h>
 #include <linux/kernel.h>
@@ -227,7 +228,7 @@ print_libpfm_events_raw(pfm_pmu_info_t *pinfo, pfm_event_info_t *info)
 		printf("%s::%s\n", pinfo->name, info->name);
 }
 
-void print_libpfm_events(bool name_only, bool long_desc)
+void print_libpfm_events(const char *event_glob, bool name_only, bool long_desc)
 {
 	pfm_event_info_t info;
 	pfm_pmu_info_t pinfo;
@@ -265,6 +266,9 @@ void print_libpfm_events(bool name_only, bool long_desc)
 			if (ret != PFM_SUCCESS)
 				continue;
 
+			if (event_glob && !strglobmatch_nocase(info.name, event_glob))
+				continue;
+
 			if (!name_only && !printed_pmu) {
 				printf("%s:\n", pinfo.name);
 				printed_pmu = true;
diff --git a/tools/perf/util/pfm.h b/tools/perf/util/pfm.h
index 7d70dda87012..036e2d97b260 100644
--- a/tools/perf/util/pfm.h
+++ b/tools/perf/util/pfm.h
@@ -13,7 +13,7 @@
 int parse_libpfm_events_option(const struct option *opt, const char *str,
 			int unset);
 
-void print_libpfm_events(bool name_only, bool long_desc);
+void print_libpfm_events(const char *event_glob, bool name_only, bool long_desc);
 
 #else
 #include <linux/compiler.h>
@@ -26,7 +26,8 @@ static inline int parse_libpfm_events_option(
 	return 0;
 }
 
-static inline void print_libpfm_events(bool name_only __maybe_unused,
+static inline void print_libpfm_events(const char *event_glob __maybe_unused,
+				       bool name_only __maybe_unused,
 				       bool long_desc __maybe_unused)
 {
 }
-- 
2.28.0.526.ge36021eeef-goog

Re: [PATCH 3/3] perf list: Add 'pfm' to list libpfm4 events

[Namhyung Kim]

Hi all,

Can you please take a look at this?

Thanks
Namhyung


On Wed, Sep 9, 2020 at 2:59 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Print libpfm4 events with 'perf list pfm' command like others.
> When libpfm4 support is not enabled, it'd print nothing.
> Also it support glob pattern matching for event name.
>
>   $ perf list pfm
>
>   List of pre-defined events (to be used in --pfm-events):
>
>   ix86arch:
>     UNHALTED_CORE_CYCLES
>       [count core clock cycles whenever the clock signal ...
>     INSTRUCTION_RETIRED
>       [count the number of instructions at retirement. ...
>     ...
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> index 10ab5e40a34f..167868053fe0 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -14,6 +14,7 @@
>  #include "util/pmu.h"
>  #include "util/debug.h"
>  #include "util/metricgroup.h"
> +#include "util/pfm.h"
>  #include <subcmd/pager.h>
>  #include <subcmd/parse-options.h>
>  #include <stdio.h>
> @@ -42,7 +43,7 @@ int cmd_list(int argc, const char **argv)
>                 OPT_END()
>         };
>         const char * const list_usage[] = {
> -               "perf list [<options>] [hw|sw|cache|tracepoint|pmu|sdt|metric|metricgroup|event_glob]",
> +               "perf list [<options>] [hw|sw|cache|tracepoint|pmu|sdt|metric|metricgroup|pfm|event_glob]",
>                 NULL
>         };
>
> @@ -53,7 +54,7 @@ int cmd_list(int argc, const char **argv)
>
>         setup_pager();
>
> -       if (!raw_dump && pager_in_use())
> +       if (!raw_dump && pager_in_use() && (argc != 1 || strcmp(argv[0], "pfm")))
>                 printf("\nList of pre-defined events (to be used in -e):\n\n");
>
>         if (argc == 0) {
> @@ -89,6 +90,8 @@ int cmd_list(int argc, const char **argv)
>                         metricgroup__print(true, false, NULL, raw_dump, details_flag);
>                 else if (strcmp(argv[i], "metricgroup") == 0 || strcmp(argv[i], "metricgroups") == 0)
>                         metricgroup__print(false, true, NULL, raw_dump, details_flag);
> +               else if (strcmp(argv[i], "pfm") == 0)
> +                       print_libpfm_events(NULL, raw_dump, long_desc_flag);
>                 else if ((sep = strchr(argv[i], ':')) != NULL) {
>                         int sep_idx;
>
> @@ -120,6 +123,7 @@ int cmd_list(int argc, const char **argv)
>                         print_tracepoint_events(NULL, s, raw_dump);
>                         print_sdt_events(NULL, s, raw_dump);
>                         metricgroup__print(true, true, s, raw_dump, details_flag);
> +                       print_libpfm_events(s, raw_dump, long_desc_flag);
>                         free(s);
>                 }
>         }
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 772f1057647f..ae8ab930a792 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -2593,7 +2593,7 @@ static struct option __record_options[] = {
>                      "number of threads to run for event synthesis"),
>  #ifdef HAVE_LIBPFM
>         OPT_CALLBACK(0, "pfm-events", &record.evlist, "event",
> -               "libpfm4 event selector. use 'perf list' to list available events",
> +               "libpfm4 event selector. use 'perf list pfm' to list available events",
>                 parse_libpfm_events_option),
>  #endif
>         OPT_CALLBACK(0, "control", &record.opts, "fd:ctl-fd[,ack-fd]",
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 483a28ef4ec4..a672d2b68e8a 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1166,7 +1166,7 @@ static struct option stat_options[] = {
>                     "threads of same physical core"),
>  #ifdef HAVE_LIBPFM
>         OPT_CALLBACK(0, "pfm-events", &evsel_list, "event",
> -               "libpfm4 event selector. use 'perf list' to list available events",
> +               "libpfm4 event selector. use 'perf list pfm' to list available events",
>                 parse_libpfm_events_option),
>  #endif
>         OPT_CALLBACK(0, "control", &stat_config, "fd:ctl-fd[,ack-fd]",
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 7c64134472c7..d6adc7d34210 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1578,7 +1578,7 @@ int cmd_top(int argc, const char **argv)
>                     "Enable LBR callgraph stitching approach"),
>  #ifdef HAVE_LIBPFM
>         OPT_CALLBACK(0, "pfm-events", &top.evlist, "event",
> -               "libpfm4 event selector. use 'perf list' to list available events",
> +               "libpfm4 event selector. use 'perf list pfm' to list available events",
>                 parse_libpfm_events_option),
>  #endif
>         OPTS_EVSWITCH(&top.evswitch),
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index c4d2394e2b2d..2d426a4f3bc7 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -2899,7 +2899,7 @@ void print_events(const char *event_glob, bool name_only, bool quiet_flag,
>
>         metricgroup__print(true, true, NULL, name_only, details_flag);
>
> -       print_libpfm_events(name_only, long_desc);
> +       print_libpfm_events(NULL, name_only, long_desc);
>  }
>
>  int parse_events__is_hardcoded_term(struct parse_events_term *term)
> diff --git a/tools/perf/util/pfm.c b/tools/perf/util/pfm.c
> index d735acb6c29c..26ae2c8c0932 100644
> --- a/tools/perf/util/pfm.c
> +++ b/tools/perf/util/pfm.c
> @@ -12,6 +12,7 @@
>  #include "util/parse-events.h"
>  #include "util/pmu.h"
>  #include "util/pfm.h"
> +#include "util/string2.h"
>
>  #include <string.h>
>  #include <linux/kernel.h>
> @@ -227,7 +228,7 @@ print_libpfm_events_raw(pfm_pmu_info_t *pinfo, pfm_event_info_t *info)
>                 printf("%s::%s\n", pinfo->name, info->name);
>  }
>
> -void print_libpfm_events(bool name_only, bool long_desc)
> +void print_libpfm_events(const char *event_glob, bool name_only, bool long_desc)
>  {
>         pfm_event_info_t info;
>         pfm_pmu_info_t pinfo;
> @@ -265,6 +266,9 @@ void print_libpfm_events(bool name_only, bool long_desc)
>                         if (ret != PFM_SUCCESS)
>                                 continue;
>
> +                       if (event_glob && !strglobmatch_nocase(info.name, event_glob))
> +                               continue;
> +
>                         if (!name_only && !printed_pmu) {
>                                 printf("%s:\n", pinfo.name);
>                                 printed_pmu = true;
> diff --git a/tools/perf/util/pfm.h b/tools/perf/util/pfm.h
> index 7d70dda87012..036e2d97b260 100644
> --- a/tools/perf/util/pfm.h
> +++ b/tools/perf/util/pfm.h
> @@ -13,7 +13,7 @@
>  int parse_libpfm_events_option(const struct option *opt, const char *str,
>                         int unset);
>
> -void print_libpfm_events(bool name_only, bool long_desc);
> +void print_libpfm_events(const char *event_glob, bool name_only, bool long_desc);
>
>  #else
>  #include <linux/compiler.h>
> @@ -26,7 +26,8 @@ static inline int parse_libpfm_events_option(
>         return 0;
>  }
>
> -static inline void print_libpfm_events(bool name_only __maybe_unused,
> +static inline void print_libpfm_events(const char *event_glob __maybe_unused,
> +                                      bool name_only __maybe_unused,
>                                        bool long_desc __maybe_unused)
>  {
>  }
> --
> 2.28.0.526.ge36021eeef-goog
>

Re: [PATCH 3/3] perf list: Add 'pfm' to list libpfm4 events

[Jiri Olsa]

On Wed, Sep 09, 2020 at 02:58:49PM +0900, Namhyung Kim wrote:

SNIP

>  int parse_events__is_hardcoded_term(struct parse_events_term *term)
> diff --git a/tools/perf/util/pfm.c b/tools/perf/util/pfm.c
> index d735acb6c29c..26ae2c8c0932 100644
> --- a/tools/perf/util/pfm.c
> +++ b/tools/perf/util/pfm.c
> @@ -12,6 +12,7 @@
>  #include "util/parse-events.h"
>  #include "util/pmu.h"
>  #include "util/pfm.h"
> +#include "util/string2.h"
>  
>  #include <string.h>
>  #include <linux/kernel.h>
> @@ -227,7 +228,7 @@ print_libpfm_events_raw(pfm_pmu_info_t *pinfo, pfm_event_info_t *info)
>  		printf("%s::%s\n", pinfo->name, info->name);
>  }
>  
> -void print_libpfm_events(bool name_only, bool long_desc)
> +void print_libpfm_events(const char *event_glob, bool name_only, bool long_desc)
>  {
>  	pfm_event_info_t info;
>  	pfm_pmu_info_t pinfo;
> @@ -265,6 +266,9 @@ void print_libpfm_events(bool name_only, bool long_desc)
>  			if (ret != PFM_SUCCESS)
>  				continue;
>  
> +			if (event_glob && !strglobmatch_nocase(info.name, event_glob))
> +				continue;

you could mentioned in changelog that it also enables glob
matching for pfm events.. but other than then looks ok

Acked/Tested-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka


> +
>  			if (!name_only && !printed_pmu) {
>  				printf("%s:\n", pinfo.name);
>  				printed_pmu = true;
> diff --git a/tools/perf/util/pfm.h b/tools/perf/util/pfm.h
> index 7d70dda87012..036e2d97b260 100644
> --- a/tools/perf/util/pfm.h
> +++ b/tools/perf/util/pfm.h
> @@ -13,7 +13,7 @@
>  int parse_libpfm_events_option(const struct option *opt, const char *str,
>  			int unset);
>  
> -void print_libpfm_events(bool name_only, bool long_desc);
> +void print_libpfm_events(const char *event_glob, bool name_only, bool long_desc);
>  
>  #else
>  #include <linux/compiler.h>
> @@ -26,7 +26,8 @@ static inline int parse_libpfm_events_option(
>  	return 0;
>  }
>  
> -static inline void print_libpfm_events(bool name_only __maybe_unused,
> +static inline void print_libpfm_events(const char *event_glob __maybe_unused,
> +				       bool name_only __maybe_unused,
>  				       bool long_desc __maybe_unused)
>  {
>  }
> -- 
> 2.28.0.526.ge36021eeef-goog
> 

Re: [PATCH 3/3] perf list: Add 'pfm' to list libpfm4 events

[Namhyung Kim]

Hi Jiri,

On Wed, Sep 23, 2020 at 5:42 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Sep 09, 2020 at 02:58:49PM +0900, Namhyung Kim wrote:
>
> SNIP
>
> >  int parse_events__is_hardcoded_term(struct parse_events_term *term)
> > diff --git a/tools/perf/util/pfm.c b/tools/perf/util/pfm.c
> > index d735acb6c29c..26ae2c8c0932 100644
> > --- a/tools/perf/util/pfm.c
> > +++ b/tools/perf/util/pfm.c
> > @@ -12,6 +12,7 @@
> >  #include "util/parse-events.h"
> >  #include "util/pmu.h"
> >  #include "util/pfm.h"
> > +#include "util/string2.h"
> >
> >  #include <string.h>
> >  #include <linux/kernel.h>
> > @@ -227,7 +228,7 @@ print_libpfm_events_raw(pfm_pmu_info_t *pinfo, pfm_event_info_t *info)
> >               printf("%s::%s\n", pinfo->name, info->name);
> >  }
> >
> > -void print_libpfm_events(bool name_only, bool long_desc)
> > +void print_libpfm_events(const char *event_glob, bool name_only, bool long_desc)
> >  {
> >       pfm_event_info_t info;
> >       pfm_pmu_info_t pinfo;
> > @@ -265,6 +266,9 @@ void print_libpfm_events(bool name_only, bool long_desc)
> >                       if (ret != PFM_SUCCESS)
> >                               continue;
> >
> > +                     if (event_glob && !strglobmatch_nocase(info.name, event_glob))
> > +                             continue;
>
> you could mentioned in changelog that it also enables glob
> matching for pfm events.. but other than then looks ok

Well, I have mentioned it in the changelog.. :)
Do you want an example?

>
> Acked/Tested-by: Jiri Olsa <jolsa@kernel.org>

Thanks
Namhyung

>
> > +
> >                       if (!name_only && !printed_pmu) {
> >                               printf("%s:\n", pinfo.name);
> >                               printed_pmu = true;
> > diff --git a/tools/perf/util/pfm.h b/tools/perf/util/pfm.h
> > index 7d70dda87012..036e2d97b260 100644
> > --- a/tools/perf/util/pfm.h
> > +++ b/tools/perf/util/pfm.h
> > @@ -13,7 +13,7 @@
> >  int parse_libpfm_events_option(const struct option *opt, const char *str,
> >                       int unset);
> >
> > -void print_libpfm_events(bool name_only, bool long_desc);
> > +void print_libpfm_events(const char *event_glob, bool name_only, bool long_desc);
> >
> >  #else
> >  #include <linux/compiler.h>
> > @@ -26,7 +26,8 @@ static inline int parse_libpfm_events_option(
> >       return 0;
> >  }
> >
> > -static inline void print_libpfm_events(bool name_only __maybe_unused,
> > +static inline void print_libpfm_events(const char *event_glob __maybe_unused,
> > +                                    bool name_only __maybe_unused,
> >                                      bool long_desc __maybe_unused)
> >  {
> >  }
> > --
> > 2.28.0.526.ge36021eeef-goog
> >
>

Re: [PATCH 3/3] perf list: Add 'pfm' to list libpfm4 events

[Jiri Olsa]

On Wed, Sep 23, 2020 at 07:42:41AM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Wed, Sep 23, 2020 at 5:42 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Wed, Sep 09, 2020 at 02:58:49PM +0900, Namhyung Kim wrote:
> >
> > SNIP
> >
> > >  int parse_events__is_hardcoded_term(struct parse_events_term *term)
> > > diff --git a/tools/perf/util/pfm.c b/tools/perf/util/pfm.c
> > > index d735acb6c29c..26ae2c8c0932 100644
> > > --- a/tools/perf/util/pfm.c
> > > +++ b/tools/perf/util/pfm.c
> > > @@ -12,6 +12,7 @@
> > >  #include "util/parse-events.h"
> > >  #include "util/pmu.h"
> > >  #include "util/pfm.h"
> > > +#include "util/string2.h"
> > >
> > >  #include <string.h>
> > >  #include <linux/kernel.h>
> > > @@ -227,7 +228,7 @@ print_libpfm_events_raw(pfm_pmu_info_t *pinfo, pfm_event_info_t *info)
> > >               printf("%s::%s\n", pinfo->name, info->name);
> > >  }
> > >
> > > -void print_libpfm_events(bool name_only, bool long_desc)
> > > +void print_libpfm_events(const char *event_glob, bool name_only, bool long_desc)
> > >  {
> > >       pfm_event_info_t info;
> > >       pfm_pmu_info_t pinfo;
> > > @@ -265,6 +266,9 @@ void print_libpfm_events(bool name_only, bool long_desc)
> > >                       if (ret != PFM_SUCCESS)
> > >                               continue;
> > >
> > > +                     if (event_glob && !strglobmatch_nocase(info.name, event_glob))
> > > +                             continue;
> >
> > you could mentioned in changelog that it also enables glob
> > matching for pfm events.. but other than then looks ok
> 
> Well, I have mentioned it in the changelog.. :)
> Do you want an example?

ugh.. sry, overlooked that

jirka

> 
> >
> > Acked/Tested-by: Jiri Olsa <jolsa@kernel.org>
> 
> Thanks
> Namhyung
> 
> >
> > > +
> > >                       if (!name_only && !printed_pmu) {
> > >                               printf("%s:\n", pinfo.name);
> > >                               printed_pmu = true;
> > > diff --git a/tools/perf/util/pfm.h b/tools/perf/util/pfm.h
> > > index 7d70dda87012..036e2d97b260 100644
> > > --- a/tools/perf/util/pfm.h
> > > +++ b/tools/perf/util/pfm.h
> > > @@ -13,7 +13,7 @@
> > >  int parse_libpfm_events_option(const struct option *opt, const char *str,
> > >                       int unset);
> > >
> > > -void print_libpfm_events(bool name_only, bool long_desc);
> > > +void print_libpfm_events(const char *event_glob, bool name_only, bool long_desc);
> > >
> > >  #else
> > >  #include <linux/compiler.h>
> > > @@ -26,7 +26,8 @@ static inline int parse_libpfm_events_option(
> > >       return 0;
> > >  }
> > >
> > > -static inline void print_libpfm_events(bool name_only __maybe_unused,
> > > +static inline void print_libpfm_events(const char *event_glob __maybe_unused,
> > > +                                    bool name_only __maybe_unused,
> > >                                      bool long_desc __maybe_unused)
> > >  {
> > >  }
> > > --
> > > 2.28.0.526.ge36021eeef-goog
> > >
> >
> 

Re: [PATCH 3/3] perf list: Add 'pfm' to list libpfm4 events

[Ian Rogers]

On Tue, Sep 8, 2020 at 10:59 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Print libpfm4 events with 'perf list pfm' command like others.
> When libpfm4 support is not enabled, it'd print nothing.
> Also it support glob pattern matching for event name.
>
>   $ perf list pfm
>
>   List of pre-defined events (to be used in --pfm-events):
>
>   ix86arch:
>     UNHALTED_CORE_CYCLES
>       [count core clock cycles whenever the clock signal ...
>     INSTRUCTION_RETIRED
>       [count the number of instructions at retirement. ...
>     ...
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> index 10ab5e40a34f..167868053fe0 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -14,6 +14,7 @@
>  #include "util/pmu.h"
>  #include "util/debug.h"
>  #include "util/metricgroup.h"
> +#include "util/pfm.h"
>  #include <subcmd/pager.h>
>  #include <subcmd/parse-options.h>
>  #include <stdio.h>
> @@ -42,7 +43,7 @@ int cmd_list(int argc, const char **argv)
>                 OPT_END()
>         };
>         const char * const list_usage[] = {
> -               "perf list [<options>] [hw|sw|cache|tracepoint|pmu|sdt|metric|metricgroup|event_glob]",
> +               "perf list [<options>] [hw|sw|cache|tracepoint|pmu|sdt|metric|metricgroup|pfm|event_glob]",

Should this be "#ifdef HAVE_LIBPFM" to avoid advertising pfm events
when support isn't compiled in?

Thanks,
Ian

>                 NULL
>         };
>
> @@ -53,7 +54,7 @@ int cmd_list(int argc, const char **argv)
>
>         setup_pager();
>
> -       if (!raw_dump && pager_in_use())
> +       if (!raw_dump && pager_in_use() && (argc != 1 || strcmp(argv[0], "pfm")))
>                 printf("\nList of pre-defined events (to be used in -e):\n\n");
>
>         if (argc == 0) {
> @@ -89,6 +90,8 @@ int cmd_list(int argc, const char **argv)
>                         metricgroup__print(true, false, NULL, raw_dump, details_flag);
>                 else if (strcmp(argv[i], "metricgroup") == 0 || strcmp(argv[i], "metricgroups") == 0)
>                         metricgroup__print(false, true, NULL, raw_dump, details_flag);
> +               else if (strcmp(argv[i], "pfm") == 0)
> +                       print_libpfm_events(NULL, raw_dump, long_desc_flag);
>                 else if ((sep = strchr(argv[i], ':')) != NULL) {
>                         int sep_idx;
>
> @@ -120,6 +123,7 @@ int cmd_list(int argc, const char **argv)
>                         print_tracepoint_events(NULL, s, raw_dump);
>                         print_sdt_events(NULL, s, raw_dump);
>                         metricgroup__print(true, true, s, raw_dump, details_flag);
> +                       print_libpfm_events(s, raw_dump, long_desc_flag);
>                         free(s);
>                 }
>         }
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 772f1057647f..ae8ab930a792 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -2593,7 +2593,7 @@ static struct option __record_options[] = {
>                      "number of threads to run for event synthesis"),
>  #ifdef HAVE_LIBPFM
>         OPT_CALLBACK(0, "pfm-events", &record.evlist, "event",
> -               "libpfm4 event selector. use 'perf list' to list available events",
> +               "libpfm4 event selector. use 'perf list pfm' to list available events",
>                 parse_libpfm_events_option),
>  #endif
>         OPT_CALLBACK(0, "control", &record.opts, "fd:ctl-fd[,ack-fd]",
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 483a28ef4ec4..a672d2b68e8a 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1166,7 +1166,7 @@ static struct option stat_options[] = {
>                     "threads of same physical core"),
>  #ifdef HAVE_LIBPFM
>         OPT_CALLBACK(0, "pfm-events", &evsel_list, "event",
> -               "libpfm4 event selector. use 'perf list' to list available events",
> +               "libpfm4 event selector. use 'perf list pfm' to list available events",
>                 parse_libpfm_events_option),
>  #endif
>         OPT_CALLBACK(0, "control", &stat_config, "fd:ctl-fd[,ack-fd]",
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 7c64134472c7..d6adc7d34210 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1578,7 +1578,7 @@ int cmd_top(int argc, const char **argv)
>                     "Enable LBR callgraph stitching approach"),
>  #ifdef HAVE_LIBPFM
>         OPT_CALLBACK(0, "pfm-events", &top.evlist, "event",
> -               "libpfm4 event selector. use 'perf list' to list available events",
> +               "libpfm4 event selector. use 'perf list pfm' to list available events",
>                 parse_libpfm_events_option),
>  #endif
>         OPTS_EVSWITCH(&top.evswitch),
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index c4d2394e2b2d..2d426a4f3bc7 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -2899,7 +2899,7 @@ void print_events(const char *event_glob, bool name_only, bool quiet_flag,
>
>         metricgroup__print(true, true, NULL, name_only, details_flag);
>
> -       print_libpfm_events(name_only, long_desc);
> +       print_libpfm_events(NULL, name_only, long_desc);
>  }
>
>  int parse_events__is_hardcoded_term(struct parse_events_term *term)
> diff --git a/tools/perf/util/pfm.c b/tools/perf/util/pfm.c
> index d735acb6c29c..26ae2c8c0932 100644
> --- a/tools/perf/util/pfm.c
> +++ b/tools/perf/util/pfm.c
> @@ -12,6 +12,7 @@
>  #include "util/parse-events.h"
>  #include "util/pmu.h"
>  #include "util/pfm.h"
> +#include "util/string2.h"
>
>  #include <string.h>
>  #include <linux/kernel.h>
> @@ -227,7 +228,7 @@ print_libpfm_events_raw(pfm_pmu_info_t *pinfo, pfm_event_info_t *info)
>                 printf("%s::%s\n", pinfo->name, info->name);
>  }
>
> -void print_libpfm_events(bool name_only, bool long_desc)
> +void print_libpfm_events(const char *event_glob, bool name_only, bool long_desc)
>  {
>         pfm_event_info_t info;
>         pfm_pmu_info_t pinfo;
> @@ -265,6 +266,9 @@ void print_libpfm_events(bool name_only, bool long_desc)
>                         if (ret != PFM_SUCCESS)
>                                 continue;
>
> +                       if (event_glob && !strglobmatch_nocase(info.name, event_glob))
> +                               continue;
> +
>                         if (!name_only && !printed_pmu) {
>                                 printf("%s:\n", pinfo.name);
>                                 printed_pmu = true;
> diff --git a/tools/perf/util/pfm.h b/tools/perf/util/pfm.h
> index 7d70dda87012..036e2d97b260 100644
> --- a/tools/perf/util/pfm.h
> +++ b/tools/perf/util/pfm.h
> @@ -13,7 +13,7 @@
>  int parse_libpfm_events_option(const struct option *opt, const char *str,
>                         int unset);
>
> -void print_libpfm_events(bool name_only, bool long_desc);
> +void print_libpfm_events(const char *event_glob, bool name_only, bool long_desc);
>
>  #else
>  #include <linux/compiler.h>
> @@ -26,7 +26,8 @@ static inline int parse_libpfm_events_option(
>         return 0;
>  }
>
> -static inline void print_libpfm_events(bool name_only __maybe_unused,
> +static inline void print_libpfm_events(const char *event_glob __maybe_unused,
> +                                      bool name_only __maybe_unused,
>                                        bool long_desc __maybe_unused)
>  {
>  }
> --
> 2.28.0.526.ge36021eeef-goog
>

Re: [PATCH 3/3] perf list: Add 'pfm' to list libpfm4 events

[Namhyung Kim]

HI Ian,

On Wed, Sep 23, 2020 at 5:50 AM Ian Rogers <irogers@google.com> wrote:
>
> On Tue, Sep 8, 2020 at 10:59 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Print libpfm4 events with 'perf list pfm' command like others.
> > When libpfm4 support is not enabled, it'd print nothing.
> > Also it support glob pattern matching for event name.
> >
> >   $ perf list pfm
> >
> >   List of pre-defined events (to be used in --pfm-events):
> >
> >   ix86arch:
> >     UNHALTED_CORE_CYCLES
> >       [count core clock cycles whenever the clock signal ...
> >     INSTRUCTION_RETIRED
> >       [count the number of instructions at retirement. ...
> >     ...
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> >
> > diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> > index 10ab5e40a34f..167868053fe0 100644
> > --- a/tools/perf/builtin-list.c
> > +++ b/tools/perf/builtin-list.c
> > @@ -14,6 +14,7 @@
> >  #include "util/pmu.h"
> >  #include "util/debug.h"
> >  #include "util/metricgroup.h"
> > +#include "util/pfm.h"
> >  #include <subcmd/pager.h>
> >  #include <subcmd/parse-options.h>
> >  #include <stdio.h>
> > @@ -42,7 +43,7 @@ int cmd_list(int argc, const char **argv)
> >                 OPT_END()
> >         };
> >         const char * const list_usage[] = {
> > -               "perf list [<options>] [hw|sw|cache|tracepoint|pmu|sdt|metric|metricgroup|event_glob]",
> > +               "perf list [<options>] [hw|sw|cache|tracepoint|pmu|sdt|metric|metricgroup|pfm|event_glob]",
>
> Should this be "#ifdef HAVE_LIBPFM" to avoid advertising pfm events
> when support isn't compiled in?

I thought about that too, but it's a keyword and current code
already handles it to print nothing, so I think it's ok to have.

Thanks
Namhyung

Re: [PATCH 1/3] perf list: Remove dead code in argument check

[Arnaldo Carvalho de Melo]

Em Wed, Sep 09, 2020 at 02:58:47PM +0900, Namhyung Kim escreveu:
> The sep is already checked being not NULL.  The code seems to be a
> leftover from some refactoring.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

There is a missing --- separator from the description to the patch, I
had to add it so that 'git am' works on it, please check.

- Arnaldo

I.e. it should be right here:

---

> 
> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> index 0a7fe4cb5555..10ab5e40a34f 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -92,13 +92,6 @@ int cmd_list(int argc, const char **argv)
>  		else if ((sep = strchr(argv[i], ':')) != NULL) {
>  			int sep_idx;
>  
> -			if (sep == NULL) {
> -				print_events(argv[i], raw_dump, !desc_flag,
> -							long_desc_flag,
> -							details_flag,
> -							deprecated);
> -				continue;
> -			}
>  			sep_idx = sep - argv[i];
>  			s = strdup(argv[i]);
>  			if (s == NULL)
> -- 
> 2.28.0.526.ge36021eeef-goog
> 

-- 

- Arnaldo

Re: [PATCH 1/3] perf list: Remove dead code in argument check

[Namhyung Kim]

Hi Arnaldo,

On Wed, Sep 9, 2020 at 9:27 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> Em Wed, Sep 09, 2020 at 02:58:47PM +0900, Namhyung Kim escreveu:
> > The sep is already checked being not NULL.  The code seems to be a
> > leftover from some refactoring.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> There is a missing --- separator from the description to the patch, I
> had to add it so that 'git am' works on it, please check.

Hmm.. strange.  I don't know why it's missed..  will double-check later.

Thanks
Namhyung


>
> - Arnaldo
>
> I.e. it should be right here:
>
> ---
>
> >
> > diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> > index 0a7fe4cb5555..10ab5e40a34f 100644
> > --- a/tools/perf/builtin-list.c
> > +++ b/tools/perf/builtin-list.c
> > @@ -92,13 +92,6 @@ int cmd_list(int argc, const char **argv)
> >               else if ((sep = strchr(argv[i], ':')) != NULL) {
> >                       int sep_idx;
> >
> > -                     if (sep == NULL) {
> > -                             print_events(argv[i], raw_dump, !desc_flag,
> > -                                                     long_desc_flag,
> > -                                                     details_flag,
> > -                                                     deprecated);
> > -                             continue;
> > -                     }
> >                       sep_idx = sep - argv[i];
> >                       s = strdup(argv[i]);
> >                       if (s == NULL)
> > --
> > 2.28.0.526.ge36021eeef-goog
> >
>
> --
>
> - Arnaldo