Re: [PATCH 6/8] perf tools: Make --raw-dump work as a proper option for perf list.

Re: [PATCH 6/8] perf tools: Make --raw-dump work as a proper option for perf list.

[Ramkumar Ramachandra]

Dongsheng Yang wrote:
>  tools/perf/builtin-list.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)

Looks good to me. Thanks for doing this.

[PATCH 0/8 V2] Enhancement for perf list.

[Dongsheng Yang]

Hi Arnaldo,
	This patchset is for enhancement of perf list version 2.

Ramkumar and David,
	The patch [6/8] is from your work 52A8D2DA.7050409@gmail.com, and
I make --raw-dump works as a option for other argument here. Do you mind
I add signed-off-by Ramkumar and signed-off-by David in this patch?

changelog:
	-v1
	 * remove the patch for event_glob
	 * add a patch [3/8] to add a missing free in print_pmu_events() fucntion.

Dongsheng Yang (8):
  perf tools: Make the all print_xxx_event functions to return unsigned
    int.
  perf tools: Make the print_pmu_events funtion to return unsigned int.
  perf tools: free aliases[j] in pmu.c if name_only is true.
  perf tools: Improve the message of perf list for unexpected input.
  perf tools: Add support of name_only for print_events_type() function.
  perf tools: Make --raw-dump work as a proper option for perf list.
  perf tools: Fix bug when --raw-dump is not the first arguement for
    perf list.
  perf tools: Enhancement for perf list with --raw-dump.

 tools/perf/builtin-list.c      | 32 +++++++++------
 tools/perf/util/parse-events.c | 88 ++++++++++++++++++++++++------------------
 tools/perf/util/parse-events.h |  8 ++--
 tools/perf/util/pmu.c          | 15 ++++---
 tools/perf/util/pmu.h          |  2 +-
 5 files changed, 85 insertions(+), 60 deletions(-)

-- 
1.8.2.1

[PATCH 1/8] perf tools: Make the all print_xxx_event functions to return unsigned int.

[Dongsheng Yang]

Currently, the most of print_XXX_event() functions are returning void. Then we can
not know whether the printing work is completed well.

This patch change the return type to unsigned int, it means the count of events we
have printed in print_XXX_event() function.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/util/parse-events.c | 76 ++++++++++++++++++++++++------------------
 tools/perf/util/parse-events.h |  8 ++---
 2 files changed, 48 insertions(+), 36 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 0153435..e2a2066 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1000,22 +1000,23 @@ static const char * const event_type_descriptors[] = {
  * Print the events from <debugfs_mount_point>/tracing/events
  */
 
-void print_tracepoint_events(const char *subsys_glob, const char *event_glob,
-			     bool name_only)
+unsigned int print_tracepoint_events(const char *subsys_glob, const char *event_glob,
+			             bool name_only)
 {
 	DIR *sys_dir, *evt_dir;
 	struct dirent *sys_next, *evt_next, sys_dirent, evt_dirent;
 	char evt_path[MAXPATHLEN];
 	char dir_path[MAXPATHLEN];
+	unsigned int count = 0;
 
 	if (debugfs_valid_mountpoint(tracing_events_path)) {
 		printf("  [ Tracepoints not available: %s ]\n", strerror(errno));
-		return;
+		return count;
 	}
 
 	sys_dir = opendir(tracing_events_path);
 	if (!sys_dir)
-		return;
+		return count;
 
 	for_each_subsystem(sys_dir, sys_dirent, sys_next) {
 		if (subsys_glob != NULL && 
@@ -1035,6 +1036,7 @@ void print_tracepoint_events(const char *subsys_glob, const char *event_glob,
 
 			if (name_only) {
 				printf("%s:%s ", sys_dirent.d_name, evt_dirent.d_name);
+				count++;
 				continue;
 			}
 
@@ -1042,10 +1044,13 @@ void print_tracepoint_events(const char *subsys_glob, const char *event_glob,
 				 sys_dirent.d_name, evt_dirent.d_name);
 			printf("  %-50s [%s]\n", evt_path,
 				event_type_descriptors[PERF_TYPE_TRACEPOINT]);
+			count++;
 		}
 		closedir(evt_dir);
 	}
 	closedir(sys_dir);
+
+	return count;
 }
 
 /*
@@ -1116,11 +1121,12 @@ static bool is_event_supported(u8 type, unsigned config)
 	return ret;
 }
 
-static void __print_events_type(u8 type, struct event_symbol *syms,
-				unsigned max)
+static unsigned int __print_events_type(u8 type, struct event_symbol *syms,
+					unsigned max)
 {
 	char name[64];
 	unsigned i;
+	unsigned int count = 0;
 
 	for (i = 0; i < max ; i++, syms++) {
 		if (!is_event_supported(type, i))
@@ -1133,20 +1139,23 @@ static void __print_events_type(u8 type, struct event_symbol *syms,
 			snprintf(name, sizeof(name), "%s", syms->symbol);
 
 		printf("  %-50s [%s]\n", name, event_type_descriptors[type]);
+		count++;
 	}
+
+	return count;
 }
 
-void print_events_type(u8 type)
+unsigned int print_events_type(u8 type)
 {
 	if (type == PERF_TYPE_SOFTWARE)
-		__print_events_type(type, event_symbols_sw, PERF_COUNT_SW_MAX);
+		return __print_events_type(type, event_symbols_sw, PERF_COUNT_SW_MAX);
 	else
-		__print_events_type(type, event_symbols_hw, PERF_COUNT_HW_MAX);
+		return __print_events_type(type, event_symbols_hw, PERF_COUNT_HW_MAX);
 }
 
-int print_hwcache_events(const char *event_glob, bool name_only)
+unsigned int print_hwcache_events(const char *event_glob, bool name_only)
 {
-	unsigned int type, op, i, printed = 0;
+	unsigned int type, op, i, count = 0;
 	char name[64];
 
 	for (type = 0; type < PERF_COUNT_HW_CACHE_MAX; type++) {
@@ -1170,21 +1179,22 @@ int print_hwcache_events(const char *event_glob, bool name_only)
 				else
 					printf("  %-50s [%s]\n", name,
 					       event_type_descriptors[PERF_TYPE_HW_CACHE]);
-				++printed;
+				count++;
 			}
 		}
 	}
 
-	if (printed)
+	if (count)
 		printf("\n");
-	return printed;
+	return count;
 }
 
-static void print_symbol_events(const char *event_glob, unsigned type,
-				struct event_symbol *syms, unsigned max,
-				bool name_only)
+static unsigned print_symbol_events(const char *event_glob, unsigned type,
+				    struct event_symbol *syms, unsigned max,
+				    bool name_only)
 {
-	unsigned i, printed = 0;
+	unsigned i;
+	unsigned int count = 0;
 	char name[MAX_NAME_LEN];
 
 	for (i = 0; i < max; i++, syms++) {
@@ -1199,6 +1209,7 @@ static void print_symbol_events(const char *event_glob, unsigned type,
 
 		if (name_only) {
 			printf("%s ", syms->symbol);
+			count++;
 			continue;
 		}
 
@@ -1209,35 +1220,35 @@ static void print_symbol_events(const char *event_glob, unsigned type,
 
 		printf("  %-50s [%s]\n", name, event_type_descriptors[type]);
 
-		printed++;
+		count++;
 	}
 
-	if (printed)
+	if (count)
 		printf("\n");
+	return count;
 }
 
 /*
  * Print the help text for the event symbols:
  */
-void print_events(const char *event_glob, bool name_only)
+unsigned int print_events(const char *event_glob, bool name_only)
 {
-	if (!name_only) {
-		printf("\n");
-		printf("List of pre-defined events (to be used in -e):\n");
-	}
+	unsigned int count = 0;
 
-	print_symbol_events(event_glob, PERF_TYPE_HARDWARE,
-			    event_symbols_hw, PERF_COUNT_HW_MAX, name_only);
+	count += 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);
+	count += print_symbol_events(event_glob, PERF_TYPE_SOFTWARE,
+				     event_symbols_sw, PERF_COUNT_SW_MAX,
+				     name_only);
 
-	print_hwcache_events(event_glob, name_only);
+	count += print_hwcache_events(event_glob, name_only);
 
-	print_pmu_events(event_glob, name_only);
+	count += print_pmu_events(event_glob, name_only);
 
 	if (event_glob != NULL)
-		return;
+		return count;
 
 	if (!name_only) {
 		printf("  %-50s [%s]\n",
@@ -1256,6 +1267,7 @@ void print_events(const char *event_glob, bool name_only)
 	}
 
 	print_tracepoint_events(NULL, NULL, name_only);
+	return count;
 }
 
 int parse_events__is_hardcoded_term(struct parse_events_term *term)
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index f1cb4c4..bb7d674 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -101,11 +101,11 @@ void parse_events_update_lists(struct list_head *list_event,
 			       struct list_head *list_all);
 void parse_events_error(void *data, void *scanner, char const *msg);
 
-void print_events(const char *event_glob, bool name_only);
-void print_events_type(u8 type);
-void print_tracepoint_events(const char *subsys_glob, const char *event_glob,
+unsigned int print_events(const char *event_glob, bool name_only);
+unsigned int print_events_type(u8 type);
+unsigned int print_tracepoint_events(const char *subsys_glob, const char *event_glob,
 			     bool name_only);
-int print_hwcache_events(const char *event_glob, bool name_only);
+unsigned int print_hwcache_events(const char *event_glob, bool name_only);
 extern int is_valid_tracepoint(const char *event_string);
 
 extern int valid_debugfs_mount(const char *debugfs);
-- 
1.8.2.1

[PATCH 2/8] perf tools: Make the print_pmu_events funtion to return unsigned int.

[Dongsheng Yang]

Sometimes we need to know how many events have been printed in print_pmu_events.

This patch make this function to return the number of events we have printed.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/util/pmu.c | 12 +++++++-----
 tools/perf/util/pmu.h |  2 +-
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 0934d64..331dc2c 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -711,12 +711,12 @@ static int cmp_string(const void *a, const void *b)
 	return strcmp(*as, *bs);
 }
 
-void print_pmu_events(const char *event_glob, bool name_only)
+unsigned int print_pmu_events(const char *event_glob, bool name_only)
 {
 	struct perf_pmu *pmu;
 	struct perf_pmu_alias *alias;
 	char buf[1024];
-	int printed = 0;
+	unsigned int count = 0;
 	int len, j;
 	char **aliases;
 
@@ -727,7 +727,7 @@ void print_pmu_events(const char *event_glob, bool name_only)
 			len++;
 	aliases = malloc(sizeof(char *) * len);
 	if (!aliases)
-		return;
+		return count;
 	pmu = NULL;
 	j = 0;
 	while ((pmu = perf_pmu__scan(pmu)) != NULL)
@@ -752,15 +752,17 @@ void print_pmu_events(const char *event_glob, bool name_only)
 	for (j = 0; j < len; j++) {
 		if (name_only) {
 			printf("%s ", aliases[j]);
+			count++;
 			continue;
 		}
 		printf("  %-50s [Kernel PMU event]\n", aliases[j]);
 		zfree(&aliases[j]);
-		printed++;
+		count++;
 	}
-	if (printed)
+	if (count)
 		printf("\n");
 	free(aliases);
+	return count;
 }
 
 bool pmu_have_event(const char *pname, const char *name)
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 9183380..2987fe2 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -42,7 +42,7 @@ int perf_pmu__format_parse(char *dir, struct list_head *head);
 
 struct perf_pmu *perf_pmu__scan(struct perf_pmu *pmu);
 
-void print_pmu_events(const char *event_glob, bool name_only);
+unsigned int print_pmu_events(const char *event_glob, bool name_only);
 bool pmu_have_event(const char *pname, const char *name);
 
 int perf_pmu__test(void);
-- 
1.8.2.1

[PATCH 3/8] perf tools: free aliases[j] in pmu.c if name_only is true.

[Dongsheng Yang]

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/util/pmu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 331dc2c..169c480 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -752,6 +752,7 @@ unsigned int print_pmu_events(const char *event_glob, bool name_only)
 	for (j = 0; j < len; j++) {
 		if (name_only) {
 			printf("%s ", aliases[j]);
+			zfree(&aliases[j]);
 			count++;
 			continue;
 		}
-- 
1.8.2.1

[PATCH 4/8] perf tools: Improve the message of perf list for unexpected input.

[Dongsheng Yang]

Example:
        # perf list test

        List of pre-defined events (to be used in -e):
        # echo $?
        0

Verification:
        # perf list test

        No event for test.
        Usage:
                perf list [hw|sw|cache|tracepoint|pmu|event_glob]
        # echo $?
        255

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/builtin-list.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index 011195e..fed6792 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -19,6 +19,7 @@
 int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	int i;
+	unsigned int count = 0;
 	const struct option list_options[] = {
 		OPT_END()
 	};
@@ -41,26 +42,27 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 		if (i)
 			putchar('\n');
 		if (strncmp(argv[i], "tracepoint", 10) == 0)
-			print_tracepoint_events(NULL, NULL, false);
+			count += print_tracepoint_events(NULL, NULL, false);
 		else if (strcmp(argv[i], "hw") == 0 ||
 			 strcmp(argv[i], "hardware") == 0)
-			print_events_type(PERF_TYPE_HARDWARE);
+			count += print_events_type(PERF_TYPE_HARDWARE);
 		else if (strcmp(argv[i], "sw") == 0 ||
 			 strcmp(argv[i], "software") == 0)
-			print_events_type(PERF_TYPE_SOFTWARE);
+			count += print_events_type(PERF_TYPE_SOFTWARE);
 		else if (strcmp(argv[i], "cache") == 0 ||
 			 strcmp(argv[i], "hwcache") == 0)
-			print_hwcache_events(NULL, false);
+			count += print_hwcache_events(NULL, false);
 		else if (strcmp(argv[i], "pmu") == 0)
-			print_pmu_events(NULL, false);
+			count += print_pmu_events(NULL, false);
 		else if (strcmp(argv[i], "--raw-dump") == 0)
-			print_events(NULL, true);
+			count += print_events(NULL, true);
 		else {
 			char *sep = strchr(argv[i], ':'), *s;
 			int sep_idx;
 
 			if (sep == NULL) {
-				print_events(argv[i], false);
+				if(!(count += print_events(argv[i], false)))
+					goto err_out;
 				continue;
 			}
 			sep_idx = sep - argv[i];
@@ -69,9 +71,16 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 				return -1;
 
 			s[sep_idx] = '\0';
-			print_tracepoint_events(s, s + sep_idx + 1, false);
+			if (!(count += print_tracepoint_events(s, s + sep_idx + 1, false)))
+				goto err_out;
 			free(s);
 		}
 	}
+
 	return 0;
+
+err_out:
+	pr_info("\nNo event for %s.\n", argv[i]);
+	pr_info("Usage:\n\t%s\n", list_usage[0]);
+	return -1;
 }
-- 
1.8.2.1

[PATCH 5/8] perf tools: Add support of name_only for print_events_type() function.

[Dongsheng Yang]

The all print_xxx_event() functions are supporting name_only argument except
print_event_type().

This patch add an argument of name_only for print_events_type() function.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/util/parse-events.c | 14 ++++++++------
 tools/perf/util/parse-events.h |  2 +-
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index e2a2066..d70f362 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1122,7 +1122,7 @@ static bool is_event_supported(u8 type, unsigned config)
 }
 
 static unsigned int __print_events_type(u8 type, struct event_symbol *syms,
-					unsigned max)
+					unsigned max, bool name_only)
 {
 	char name[64];
 	unsigned i;
@@ -1137,20 +1137,22 @@ static unsigned int __print_events_type(u8 type, struct event_symbol *syms,
 				 syms->symbol, syms->alias);
 		else
 			snprintf(name, sizeof(name), "%s", syms->symbol);
-
-		printf("  %-50s [%s]\n", name, event_type_descriptors[type]);
+		if (name_only)
+			printf("  %-50s\n", name);
+		else
+			printf("  %-50s [%s]\n", name, event_type_descriptors[type]);
 		count++;
 	}
 
 	return count;
 }
 
-unsigned int print_events_type(u8 type)
+unsigned int print_events_type(u8 type, bool name_only)
 {
 	if (type == PERF_TYPE_SOFTWARE)
-		return __print_events_type(type, event_symbols_sw, PERF_COUNT_SW_MAX);
+		return __print_events_type(type, event_symbols_sw, PERF_COUNT_SW_MAX, name_only);
 	else
-		return __print_events_type(type, event_symbols_hw, PERF_COUNT_HW_MAX);
+		return __print_events_type(type, event_symbols_hw, PERF_COUNT_HW_MAX, name_only);
 }
 
 unsigned int print_hwcache_events(const char *event_glob, bool name_only)
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index bb7d674..148a767 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -102,7 +102,7 @@ void parse_events_update_lists(struct list_head *list_event,
 void parse_events_error(void *data, void *scanner, char const *msg);
 
 unsigned int print_events(const char *event_glob, bool name_only);
-unsigned int print_events_type(u8 type);
+unsigned int print_events_type(u8 type, bool name_only);
 unsigned int print_tracepoint_events(const char *subsys_glob, const char *event_glob,
 			     bool name_only);
 unsigned int print_hwcache_events(const char *event_glob, bool name_only);
-- 
1.8.2.1

[PATCH 6/8] perf tools: Make --raw-dump work as a proper option for perf list.

[Dongsheng Yang]

Ramkumar reported that perf list --raw-dump was broken by 44d742e.
Fix by making raw-dump a proper argument.

Signed-off-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Cc: Ramkumar Ramachandra <artagnon@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-list.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index fed6792..ec90d0a 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -19,8 +19,10 @@
 int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	int i;
+	bool raw_dump = false;
 	unsigned int count = 0;
 	const struct option list_options[] = {
+		OPT_BOOLEAN(0, "raw-dump", &raw_dump, "raw dump for completion"),
 		OPT_END()
 	};
 	const char * const list_usage[] = {
@@ -34,7 +36,7 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 	setup_pager();
 
 	if (argc == 0) {
-		print_events(NULL, false);
+		print_events(NULL, raw_dump);
 		return 0;
 	}
 
@@ -42,26 +44,24 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 		if (i)
 			putchar('\n');
 		if (strncmp(argv[i], "tracepoint", 10) == 0)
-			count += print_tracepoint_events(NULL, NULL, false);
+			count += print_tracepoint_events(NULL, NULL, raw_dump);
 		else if (strcmp(argv[i], "hw") == 0 ||
 			 strcmp(argv[i], "hardware") == 0)
-			count += print_events_type(PERF_TYPE_HARDWARE);
+			count += print_events_type(PERF_TYPE_HARDWARE, raw_dump);
 		else if (strcmp(argv[i], "sw") == 0 ||
 			 strcmp(argv[i], "software") == 0)
-			count += print_events_type(PERF_TYPE_SOFTWARE);
+			count += print_events_type(PERF_TYPE_SOFTWARE, raw_dump);
 		else if (strcmp(argv[i], "cache") == 0 ||
 			 strcmp(argv[i], "hwcache") == 0)
-			count += print_hwcache_events(NULL, false);
+			count += print_hwcache_events(NULL, raw_dump);
 		else if (strcmp(argv[i], "pmu") == 0)
-			count += print_pmu_events(NULL, false);
-		else if (strcmp(argv[i], "--raw-dump") == 0)
-			count += print_events(NULL, true);
+			count += print_pmu_events(NULL, raw_dump);
 		else {
 			char *sep = strchr(argv[i], ':'), *s;
 			int sep_idx;
 
 			if (sep == NULL) {
-				if(!(count += print_events(argv[i], false)))
+				if(!(count += print_events(argv[i], raw_dump)))
 					goto err_out;
 				continue;
 			}
@@ -71,7 +71,7 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 				return -1;
 
 			s[sep_idx] = '\0';
-			if (!(count += print_tracepoint_events(s, s + sep_idx + 1, false)))
+			if (!(count += print_tracepoint_events(s, s + sep_idx + 1, raw_dump)))
 				goto err_out;
 			free(s);
 		}
-- 
1.8.2.1

[PATCH 7/8] perf tools: Fix bug when --raw-dump is not the first arguement for perf list.

[Dongsheng Yang]

As we use PARSE_OPT_STOP_AT_NON_OPTION option in parse_option(), if --raw-dump
is not the first argument, it will be treat as an argument rather than option.

Example:
	# ./perf list kvmmmu --raw-dump
	  kvmmmu:kvm_mmu_pagetable_walk                      [Tracepoint event]
	  kvmmmu:kvm_mmu_paging_element                      [Tracepoint event]
	  kvmmmu:kvm_mmu_set_accessed_bit                    [Tracepoint event]
	  kvmmmu:kvm_mmu_set_dirty_bit                       [Tracepoint event]
	  kvmmmu:kvm_mmu_walker_error                        [Tracepoint event]
	  kvmmmu:kvm_mmu_get_page                            [Tracepoint event]
	  kvmmmu:kvm_mmu_sync_page                           [Tracepoint event]
	  kvmmmu:kvm_mmu_unsync_page                         [Tracepoint event]
	  kvmmmu:kvm_mmu_prepare_zap_page                    [Tracepoint event]
	  kvmmmu:mark_mmio_spte                              [Tracepoint event]
	  kvmmmu:handle_mmio_page_fault                      [Tracepoint event]
	  kvmmmu:fast_page_fault                             [Tracepoint event]

This patch replace PARSE_OPT_STOP_AT_NON_OPTION as 0, then we can parse it even if
it is not the first argument.

Verification:
	# ./perf list kvmmmu --raw-dump
	kvmmmu:kvm_mmu_pagetable_walk kvmmmu:kvm_mmu_paging_element
	 kvmmmu:kvm_mmu_set_accessed_bit kvmmmu:kvm_mmu_set_dirty_b
	it kvmmmu:kvm_mmu_walker_error kvmmmu:kv

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/builtin-list.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index ec90d0a..9430666 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -30,8 +30,7 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 		NULL
 	};
 
-	argc = parse_options(argc, argv, list_options, list_usage,
-			     PARSE_OPT_STOP_AT_NON_OPTION);
+	argc = parse_options(argc, argv, list_options, list_usage, 0);
 
 	setup_pager();
 
-- 
1.8.2.1

[PATCH 8/8] perf tools: Enhancement for perf list with --raw-dump.

[Dongsheng Yang]

As we make --raw-dump work as a proper option, we need to
make the output of it more readable.

Example:
	# ./perf list kvmmmu --raw-dump
        kvmmmu:kvm_mmu_pagetable_walk kvmmmu:kvm_mmu_paging_element
         kvmmmu:kvm_mmu_set_accessed_bit kvmmmu:kvm_mmu_set_dirty_b
        it kvmmmu:kvm_mmu_walker_error kvmmmu:kv

Verification:
	# ./perf list kvmmmu --raw-dump
	kvmmmu:kvm_mmu_pagetable_walk
	kvmmmu:kvm_mmu_paging_element
	kvmmmu:kvm_mmu_set_accessed_bit
	kvmmmu:kvm_mmu_set_dirty_bit
	kvmmmu:kvm_mmu_walker_error
	kvmmmu:kvm_mmu_get_page
	kvmmmu:kvm_mmu_sync_page
	kvmmmu:kvm_mmu_unsync_page
	kvmmmu:kvm_mmu_prepare_zap_page
	kvmmmu:mark_mmio_spte
	kvmmmu:handle_mmio_page_fault
	kvmmmu:fast_page_fault

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/util/parse-events.c | 6 +++---
 tools/perf/util/pmu.c          | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index d70f362..9e3ddc3 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1035,7 +1035,7 @@ unsigned int print_tracepoint_events(const char *subsys_glob, const char *event_
 				continue;
 
 			if (name_only) {
-				printf("%s:%s ", sys_dirent.d_name, evt_dirent.d_name);
+				printf("%s:%s\n", sys_dirent.d_name, evt_dirent.d_name);
 				count++;
 				continue;
 			}
@@ -1177,7 +1177,7 @@ unsigned int print_hwcache_events(const char *event_glob, bool name_only)
 					continue;
 
 				if (name_only)
-					printf("%s ", name);
+					printf("%s\n", name);
 				else
 					printf("  %-50s [%s]\n", name,
 					       event_type_descriptors[PERF_TYPE_HW_CACHE]);
@@ -1210,7 +1210,7 @@ static unsigned print_symbol_events(const char *event_glob, unsigned type,
 			continue;
 
 		if (name_only) {
-			printf("%s ", syms->symbol);
+			printf("%s\n", syms->symbol);
 			count++;
 			continue;
 		}
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 169c480..78f980d 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -751,7 +751,7 @@ unsigned int print_pmu_events(const char *event_glob, bool name_only)
 	qsort(aliases, len, sizeof(char *), cmp_string);
 	for (j = 0; j < len; j++) {
 		if (name_only) {
-			printf("%s ", aliases[j]);
+			printf("%s\n", aliases[j]);
 			zfree(&aliases[j]);
 			count++;
 			continue;
-- 
1.8.2.1

Re: [PATCH 2/8] perf tools: Make the print_pmu_events funtion to return unsigned int.

[Arnaldo Carvalho de Melo]

Em Mon, Dec 30, 2013 at 10:26:37AM -0500, Dongsheng Yang escreveu:
> Sometimes we need to know how many events have been printed in print_pmu_events.

"Sometimes" is too vague, of course it should be needed, otherwise why
bother with the patch? It would be better if you provided an example of
_where_ it will be used, like when printing that newline.

Anyway, reading the other patches,

- Arnaldo
 
> This patch make this function to return the number of events we have printed.
> 
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
>  tools/perf/util/pmu.c | 12 +++++++-----
>  tools/perf/util/pmu.h |  2 +-
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 0934d64..331dc2c 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -711,12 +711,12 @@ static int cmp_string(const void *a, const void *b)
>  	return strcmp(*as, *bs);
>  }
>  
> -void print_pmu_events(const char *event_glob, bool name_only)
> +unsigned int print_pmu_events(const char *event_glob, bool name_only)
>  {
>  	struct perf_pmu *pmu;
>  	struct perf_pmu_alias *alias;
>  	char buf[1024];
> -	int printed = 0;
> +	unsigned int count = 0;
>  	int len, j;
>  	char **aliases;
>  
> @@ -727,7 +727,7 @@ void print_pmu_events(const char *event_glob, bool name_only)
>  			len++;
>  	aliases = malloc(sizeof(char *) * len);
>  	if (!aliases)
> -		return;
> +		return count;
>  	pmu = NULL;
>  	j = 0;
>  	while ((pmu = perf_pmu__scan(pmu)) != NULL)
> @@ -752,15 +752,17 @@ void print_pmu_events(const char *event_glob, bool name_only)
>  	for (j = 0; j < len; j++) {
>  		if (name_only) {
>  			printf("%s ", aliases[j]);
> +			count++;
>  			continue;
>  		}
>  		printf("  %-50s [Kernel PMU event]\n", aliases[j]);
>  		zfree(&aliases[j]);
> -		printed++;
> +		count++;
>  	}
> -	if (printed)
> +	if (count)
>  		printf("\n");
>  	free(aliases);
> +	return count;
>  }
>  
>  bool pmu_have_event(const char *pname, const char *name)
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 9183380..2987fe2 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -42,7 +42,7 @@ int perf_pmu__format_parse(char *dir, struct list_head *head);
>  
>  struct perf_pmu *perf_pmu__scan(struct perf_pmu *pmu);
>  
> -void print_pmu_events(const char *event_glob, bool name_only);
> +unsigned int print_pmu_events(const char *event_glob, bool name_only);
>  bool pmu_have_event(const char *pname, const char *name);
>  
>  int perf_pmu__test(void);
> -- 
> 1.8.2.1

Re: [PATCH 3/8] perf tools: free aliases[j] in pmu.c if name_only is true.

[Arnaldo Carvalho de Melo]

What for? Please be clear in the changelogs, it helps with review.

Now I'll have to follow the logic to figure this out, i.e. _why_ we
should free aliases[j] if name_only is true? Is it safe?

- Arnaldo

Em Mon, Dec 30, 2013 at 10:26:38AM -0500, Dongsheng Yang escreveu:
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
>  tools/perf/util/pmu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 331dc2c..169c480 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -752,6 +752,7 @@ unsigned int print_pmu_events(const char *event_glob, bool name_only)
>  	for (j = 0; j < len; j++) {
>  		if (name_only) {
>  			printf("%s ", aliases[j]);
> +			zfree(&aliases[j]);
>  			count++;
>  			continue;
>  		}
> -- 
> 1.8.2.1

Re: [PATCH 4/8] perf tools: Improve the message of perf list for unexpected input.

[Arnaldo Carvalho de Melo]

Em Mon, Dec 30, 2013 at 10:26:39AM -0500, Dongsheng Yang escreveu:
> Example:
>         # perf list test
> 
>         List of pre-defined events (to be used in -e):
>         # echo $?
>         0
> 
> Verification:
>         # perf list test
> 
>         No event for test.

?

Are we "testing" events?

The message is not clear, I can get some info from looking at the Usage,
but not from 'No event for test'.

At a minimum 'test' (whatevet invalid type/class is specified and found
to be invalid/unknown) should be quoted.

Perhaps the message should be along the lines of:

	# perf list test

	'test' is not a valid event type, please see the usage below for
	acceptable ones.

>         Usage:
>                 perf list [hw|sw|cache|tracepoint|pmu|event_glob]
>         # echo $?
>         255
> 
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
>  tools/perf/builtin-list.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> index 011195e..fed6792 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -19,6 +19,7 @@
>  int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
>  {
>  	int i;
> +	unsigned int count = 0;
>  	const struct option list_options[] = {
>  		OPT_END()
>  	};
> @@ -41,26 +42,27 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
>  		if (i)
>  			putchar('\n');
>  		if (strncmp(argv[i], "tracepoint", 10) == 0)
> -			print_tracepoint_events(NULL, NULL, false);
> +			count += print_tracepoint_events(NULL, NULL, false);
>  		else if (strcmp(argv[i], "hw") == 0 ||
>  			 strcmp(argv[i], "hardware") == 0)
> -			print_events_type(PERF_TYPE_HARDWARE);
> +			count += print_events_type(PERF_TYPE_HARDWARE);
>  		else if (strcmp(argv[i], "sw") == 0 ||
>  			 strcmp(argv[i], "software") == 0)
> -			print_events_type(PERF_TYPE_SOFTWARE);
> +			count += print_events_type(PERF_TYPE_SOFTWARE);
>  		else if (strcmp(argv[i], "cache") == 0 ||
>  			 strcmp(argv[i], "hwcache") == 0)
> -			print_hwcache_events(NULL, false);
> +			count += print_hwcache_events(NULL, false);
>  		else if (strcmp(argv[i], "pmu") == 0)
> -			print_pmu_events(NULL, false);
> +			count += print_pmu_events(NULL, false);
>  		else if (strcmp(argv[i], "--raw-dump") == 0)
> -			print_events(NULL, true);
> +			count += print_events(NULL, true);
>  		else {
>  			char *sep = strchr(argv[i], ':'), *s;
>  			int sep_idx;
>  
>  			if (sep == NULL) {
> -				print_events(argv[i], false);
> +				if(!(count += print_events(argv[i], false)))
> +					goto err_out;
>  				continue;
>  			}
>  			sep_idx = sep - argv[i];
> @@ -69,9 +71,16 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
>  				return -1;
>  
>  			s[sep_idx] = '\0';
> -			print_tracepoint_events(s, s + sep_idx + 1, false);
> +			if (!(count += print_tracepoint_events(s, s + sep_idx + 1, false)))
> +				goto err_out;
>  			free(s);
>  		}
>  	}
> +
>  	return 0;
> +
> +err_out:
> +	pr_info("\nNo event for %s.\n", argv[i]);
> +	pr_info("Usage:\n\t%s\n", list_usage[0]);
> +	return -1;
>  }
> -- 
> 1.8.2.1

Re: [PATCH 5/8] perf tools: Add support of name_only for print_events_type() function.

[Arnaldo Carvalho de Melo]

Em Mon, Dec 30, 2013 at 10:26:40AM -0500, Dongsheng Yang escreveu:
> The all print_xxx_event() functions are supporting name_only argument except
> print_event_type().
> 
> This patch add an argument of name_only for print_events_type() function.

This one looks ok, as the 'name_only' parameter name together with your
changeset comments provides enough context to understand the patch at a
glance.

But it doesn't applies, as the previous one needs changes to address my
comments,

- Arnaldo
 
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
>  tools/perf/util/parse-events.c | 14 ++++++++------
>  tools/perf/util/parse-events.h |  2 +-
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index e2a2066..d70f362 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1122,7 +1122,7 @@ static bool is_event_supported(u8 type, unsigned config)
>  }
>  
>  static unsigned int __print_events_type(u8 type, struct event_symbol *syms,
> -					unsigned max)
> +					unsigned max, bool name_only)
>  {
>  	char name[64];
>  	unsigned i;
> @@ -1137,20 +1137,22 @@ static unsigned int __print_events_type(u8 type, struct event_symbol *syms,
>  				 syms->symbol, syms->alias);
>  		else
>  			snprintf(name, sizeof(name), "%s", syms->symbol);
> -
> -		printf("  %-50s [%s]\n", name, event_type_descriptors[type]);
> +		if (name_only)
> +			printf("  %-50s\n", name);
> +		else
> +			printf("  %-50s [%s]\n", name, event_type_descriptors[type]);
>  		count++;
>  	}
>  
>  	return count;
>  }
>  
> -unsigned int print_events_type(u8 type)
> +unsigned int print_events_type(u8 type, bool name_only)
>  {
>  	if (type == PERF_TYPE_SOFTWARE)
> -		return __print_events_type(type, event_symbols_sw, PERF_COUNT_SW_MAX);
> +		return __print_events_type(type, event_symbols_sw, PERF_COUNT_SW_MAX, name_only);
>  	else
> -		return __print_events_type(type, event_symbols_hw, PERF_COUNT_HW_MAX);
> +		return __print_events_type(type, event_symbols_hw, PERF_COUNT_HW_MAX, name_only);
>  }
>  
>  unsigned int print_hwcache_events(const char *event_glob, bool name_only)
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index bb7d674..148a767 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -102,7 +102,7 @@ void parse_events_update_lists(struct list_head *list_event,
>  void parse_events_error(void *data, void *scanner, char const *msg);
>  
>  unsigned int print_events(const char *event_glob, bool name_only);
> -unsigned int print_events_type(u8 type);
> +unsigned int print_events_type(u8 type, bool name_only);
>  unsigned int print_tracepoint_events(const char *subsys_glob, const char *event_glob,
>  			     bool name_only);
>  unsigned int print_hwcache_events(const char *event_glob, bool name_only);
> -- 
> 1.8.2.1

Re: [PATCH 6/8] perf tools: Make --raw-dump work as a proper option for perf list.

[Arnaldo Carvalho de Melo]

Em Mon, Dec 30, 2013 at 10:26:41AM -0500, Dongsheng Yang escreveu:
> Ramkumar reported that perf list --raw-dump was broken by 44d742e.
> Fix by making raw-dump a proper argument.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>

Are you sure David Signed-off-by' this, isn't this just an Acked-by or
perhaps a Reviewed-by? David?

- Arnaldo

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> Cc: Ramkumar Ramachandra <artagnon@gmail.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/builtin-list.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> index fed6792..ec90d0a 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -19,8 +19,10 @@
>  int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
>  {
>  	int i;
> +	bool raw_dump = false;
>  	unsigned int count = 0;
>  	const struct option list_options[] = {
> +		OPT_BOOLEAN(0, "raw-dump", &raw_dump, "raw dump for completion"),
>  		OPT_END()
>  	};
>  	const char * const list_usage[] = {
> @@ -34,7 +36,7 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
>  	setup_pager();
>  
>  	if (argc == 0) {
> -		print_events(NULL, false);
> +		print_events(NULL, raw_dump);
>  		return 0;
>  	}
>  
> @@ -42,26 +44,24 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
>  		if (i)
>  			putchar('\n');
>  		if (strncmp(argv[i], "tracepoint", 10) == 0)
> -			count += print_tracepoint_events(NULL, NULL, false);
> +			count += print_tracepoint_events(NULL, NULL, raw_dump);
>  		else if (strcmp(argv[i], "hw") == 0 ||
>  			 strcmp(argv[i], "hardware") == 0)
> -			count += print_events_type(PERF_TYPE_HARDWARE);
> +			count += print_events_type(PERF_TYPE_HARDWARE, raw_dump);
>  		else if (strcmp(argv[i], "sw") == 0 ||
>  			 strcmp(argv[i], "software") == 0)
> -			count += print_events_type(PERF_TYPE_SOFTWARE);
> +			count += print_events_type(PERF_TYPE_SOFTWARE, raw_dump);
>  		else if (strcmp(argv[i], "cache") == 0 ||
>  			 strcmp(argv[i], "hwcache") == 0)
> -			count += print_hwcache_events(NULL, false);
> +			count += print_hwcache_events(NULL, raw_dump);
>  		else if (strcmp(argv[i], "pmu") == 0)
> -			count += print_pmu_events(NULL, false);
> -		else if (strcmp(argv[i], "--raw-dump") == 0)
> -			count += print_events(NULL, true);
> +			count += print_pmu_events(NULL, raw_dump);
>  		else {
>  			char *sep = strchr(argv[i], ':'), *s;
>  			int sep_idx;
>  
>  			if (sep == NULL) {
> -				if(!(count += print_events(argv[i], false)))
> +				if(!(count += print_events(argv[i], raw_dump)))
>  					goto err_out;
>  				continue;
>  			}
> @@ -71,7 +71,7 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
>  				return -1;
>  
>  			s[sep_idx] = '\0';
> -			if (!(count += print_tracepoint_events(s, s + sep_idx + 1, false)))
> +			if (!(count += print_tracepoint_events(s, s + sep_idx + 1, raw_dump)))
>  				goto err_out;
>  			free(s);
>  		}
> -- 
> 1.8.2.1

Re: [PATCH 6/8] perf tools: Make --raw-dump work as a proper option for perf list.

[David Ahern]

On 1/8/14, 8:20 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Dec 30, 2013 at 10:26:41AM -0500, Dongsheng Yang escreveu:
>> >Ramkumar reported that perf list --raw-dump was broken by 44d742e.
>> >Fix by making raw-dump a proper argument.
>> >
>> >Signed-off-by: David Ahern<dsahern@gmail.com>
> Are you sure David Signed-off-by' this, isn't this just an Acked-by or
> perhaps a Reviewed-by? David?

I thought the last version of the patch came from Ramkumar.

This version seems like an add-on to the last patch.

David

Re: [PATCH 6/8] perf tools: Make --raw-dump work as a proper option for perf list.

[Dongsheng Yang]

On 01/08/2014 04:44 PM, David Ahern wrote:
> On 1/8/14, 8:20 AM, Arnaldo Carvalho de Melo wrote:
>> Em Mon, Dec 30, 2013 at 10:26:41AM -0500, Dongsheng Yang escreveu:
>>> >Ramkumar reported that perf list --raw-dump was broken by 44d742e.
>>> >Fix by making raw-dump a proper argument.
>>> >
>>> >Signed-off-by: David Ahern<dsahern@gmail.com>
>> Are you sure David Signed-off-by' this, isn't this just an Acked-by or
>> perhaps a Reviewed-by? David?
>
> I thought the last version of the patch came from Ramkumar.
>
> This version seems like an add-on to the last patch.
>

So, David, do you mind I add "Signed-off-by: David Ahern" in this patch?

> David
>

Re: [PATCH 5/8] perf tools: Add support of name_only for print_events_type() function.

[Dongsheng Yang]

On 01/08/2014 10:19 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Dec 30, 2013 at 10:26:40AM -0500, Dongsheng Yang escreveu:
>> The all print_xxx_event() functions are supporting name_only argument except
>> print_event_type().
>>
>> This patch add an argument of name_only for print_events_type() function.
> This one looks ok, as the 'name_only' parameter name together with your
> changeset comments provides enough context to understand the patch at a
> glance.
>
> But it doesn't applies, as the previous one needs changes to address my
> comments,

Okey, Thanx
>
> - Arnaldo
>   
>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>> ---
>>   tools/perf/util/parse-events.c | 14 ++++++++------
>>   tools/perf/util/parse-events.h |  2 +-
>>   2 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index e2a2066..d70f362 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -1122,7 +1122,7 @@ static bool is_event_supported(u8 type, unsigned config)
>>   }
>>   
>>   static unsigned int __print_events_type(u8 type, struct event_symbol *syms,
>> -					unsigned max)
>> +					unsigned max, bool name_only)
>>   {
>>   	char name[64];
>>   	unsigned i;
>> @@ -1137,20 +1137,22 @@ static unsigned int __print_events_type(u8 type, struct event_symbol *syms,
>>   				 syms->symbol, syms->alias);
>>   		else
>>   			snprintf(name, sizeof(name), "%s", syms->symbol);
>> -
>> -		printf("  %-50s [%s]\n", name, event_type_descriptors[type]);
>> +		if (name_only)
>> +			printf("  %-50s\n", name);
>> +		else
>> +			printf("  %-50s [%s]\n", name, event_type_descriptors[type]);
>>   		count++;
>>   	}
>>   
>>   	return count;
>>   }
>>   
>> -unsigned int print_events_type(u8 type)
>> +unsigned int print_events_type(u8 type, bool name_only)
>>   {
>>   	if (type == PERF_TYPE_SOFTWARE)
>> -		return __print_events_type(type, event_symbols_sw, PERF_COUNT_SW_MAX);
>> +		return __print_events_type(type, event_symbols_sw, PERF_COUNT_SW_MAX, name_only);
>>   	else
>> -		return __print_events_type(type, event_symbols_hw, PERF_COUNT_HW_MAX);
>> +		return __print_events_type(type, event_symbols_hw, PERF_COUNT_HW_MAX, name_only);
>>   }
>>   
>>   unsigned int print_hwcache_events(const char *event_glob, bool name_only)
>> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
>> index bb7d674..148a767 100644
>> --- a/tools/perf/util/parse-events.h
>> +++ b/tools/perf/util/parse-events.h
>> @@ -102,7 +102,7 @@ void parse_events_update_lists(struct list_head *list_event,
>>   void parse_events_error(void *data, void *scanner, char const *msg);
>>   
>>   unsigned int print_events(const char *event_glob, bool name_only);
>> -unsigned int print_events_type(u8 type);
>> +unsigned int print_events_type(u8 type, bool name_only);
>>   unsigned int print_tracepoint_events(const char *subsys_glob, const char *event_glob,
>>   			     bool name_only);
>>   unsigned int print_hwcache_events(const char *event_glob, bool name_only);
>> -- 
>> 1.8.2.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 4/8] perf tools: Improve the message of perf list for unexpected input.

[Dongsheng Yang]

On 01/08/2014 10:16 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Dec 30, 2013 at 10:26:39AM -0500, Dongsheng Yang escreveu:
>> Example:
>>          # perf list test
>>
>>          List of pre-defined events (to be used in -e):
>>          # echo $?
>>          0
>>
>> Verification:
>>          # perf list test
>>
>>          No event for test.
> ?
>
> Are we "testing" events?
>
> The message is not clear, I can get some info from looking at the Usage,
> but not from 'No event for test'.
>
> At a minimum 'test' (whatevet invalid type/class is specified and found
> to be invalid/unknown) should be quoted.
>
> Perhaps the message should be along the lines of:
>
> 	# perf list test
>
> 	'test' is not a valid event type, please see the usage below for
> 	acceptable ones.

Makes sense. I will update it.
>
>>          Usage:
>>                  perf list [hw|sw|cache|tracepoint|pmu|event_glob]
>>          # echo $?
>>          255
>>
>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>> ---
>>   tools/perf/builtin-list.c | 25 +++++++++++++++++--------
>>   1 file changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
>> index 011195e..fed6792 100644
>> --- a/tools/perf/builtin-list.c
>> +++ b/tools/perf/builtin-list.c
>> @@ -19,6 +19,7 @@
>>   int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
>>   {
>>   	int i;
>> +	unsigned int count = 0;
>>   	const struct option list_options[] = {
>>   		OPT_END()
>>   	};
>> @@ -41,26 +42,27 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
>>   		if (i)
>>   			putchar('\n');
>>   		if (strncmp(argv[i], "tracepoint", 10) == 0)
>> -			print_tracepoint_events(NULL, NULL, false);
>> +			count += print_tracepoint_events(NULL, NULL, false);
>>   		else if (strcmp(argv[i], "hw") == 0 ||
>>   			 strcmp(argv[i], "hardware") == 0)
>> -			print_events_type(PERF_TYPE_HARDWARE);
>> +			count += print_events_type(PERF_TYPE_HARDWARE);
>>   		else if (strcmp(argv[i], "sw") == 0 ||
>>   			 strcmp(argv[i], "software") == 0)
>> -			print_events_type(PERF_TYPE_SOFTWARE);
>> +			count += print_events_type(PERF_TYPE_SOFTWARE);
>>   		else if (strcmp(argv[i], "cache") == 0 ||
>>   			 strcmp(argv[i], "hwcache") == 0)
>> -			print_hwcache_events(NULL, false);
>> +			count += print_hwcache_events(NULL, false);
>>   		else if (strcmp(argv[i], "pmu") == 0)
>> -			print_pmu_events(NULL, false);
>> +			count += print_pmu_events(NULL, false);
>>   		else if (strcmp(argv[i], "--raw-dump") == 0)
>> -			print_events(NULL, true);
>> +			count += print_events(NULL, true);
>>   		else {
>>   			char *sep = strchr(argv[i], ':'), *s;
>>   			int sep_idx;
>>   
>>   			if (sep == NULL) {
>> -				print_events(argv[i], false);
>> +				if(!(count += print_events(argv[i], false)))
>> +					goto err_out;
>>   				continue;
>>   			}
>>   			sep_idx = sep - argv[i];
>> @@ -69,9 +71,16 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
>>   				return -1;
>>   
>>   			s[sep_idx] = '\0';
>> -			print_tracepoint_events(s, s + sep_idx + 1, false);
>> +			if (!(count += print_tracepoint_events(s, s + sep_idx + 1, false)))
>> +				goto err_out;
>>   			free(s);
>>   		}
>>   	}
>> +
>>   	return 0;
>> +
>> +err_out:
>> +	pr_info("\nNo event for %s.\n", argv[i]);
>> +	pr_info("Usage:\n\t%s\n", list_usage[0]);
>> +	return -1;
>>   }
>> -- 
>> 1.8.2.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 3/8] perf tools: free aliases[j] in pmu.c if name_only is true.

[Dongsheng Yang]

On 01/08/2014 10:12 AM, Arnaldo Carvalho de Melo wrote:
> What for? Please be clear in the changelogs, it helps with review.
>
> Now I'll have to follow the logic to figure this out, i.e. _why_ we
> should free aliases[j] if name_only is true? Is it safe?

Sorry for my lazy on the changelogs here :(.

As aliases is a 2-D array malloced in function print_pmu_events(), we 
should free it after printf. But we just did it when (!name_only).

This patch fix the leak error in print_pmu_events() when name_only is true.

I will update the commit message in next version.

- Yang
>
> - Arnaldo
>
> Em Mon, Dec 30, 2013 at 10:26:38AM -0500, Dongsheng Yang escreveu:
>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>> ---
>>   tools/perf/util/pmu.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 331dc2c..169c480 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -752,6 +752,7 @@ unsigned int print_pmu_events(const char *event_glob, bool name_only)
>>   	for (j = 0; j < len; j++) {
>>   		if (name_only) {
>>   			printf("%s ", aliases[j]);
>> +			zfree(&aliases[j]);
>>   			count++;
>>   			continue;
>>   		}
>> -- 
>> 1.8.2.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 2/8] perf tools: Make the print_pmu_events funtion to return unsigned int.

[Dongsheng Yang]

On 01/08/2014 10:10 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Dec 30, 2013 at 10:26:37AM -0500, Dongsheng Yang escreveu:
>> Sometimes we need to know how many events have been printed in print_pmu_events.
> "Sometimes" is too vague, of course it should be needed, otherwise why
> bother with the patch? It would be better if you provided an example of
> _where_ it will be used, like when printing that newline.

Sorry for the terrible commit message :(.

What about the following one?

"When we call print_pmu_events(), we want to know how many events has 
been printed in this function. But currently it is impossible, as it 
return void.

This patch change this function to return the number of events we have 
printed."

- Yang
> Anyway, reading the other patches,
>
> - Arnaldo
>   
>> This patch make this function to return the number of events we have printed.
>>
>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>> ---
>>   tools/perf/util/pmu.c | 12 +++++++-----
>>   tools/perf/util/pmu.h |  2 +-
>>   2 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 0934d64..331dc2c 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -711,12 +711,12 @@ static int cmp_string(const void *a, const void *b)
>>   	return strcmp(*as, *bs);
>>   }
>>   
>> -void print_pmu_events(const char *event_glob, bool name_only)
>> +unsigned int print_pmu_events(const char *event_glob, bool name_only)
>>   {
>>   	struct perf_pmu *pmu;
>>   	struct perf_pmu_alias *alias;
>>   	char buf[1024];
>> -	int printed = 0;
>> +	unsigned int count = 0;
>>   	int len, j;
>>   	char **aliases;
>>   
>> @@ -727,7 +727,7 @@ void print_pmu_events(const char *event_glob, bool name_only)
>>   			len++;
>>   	aliases = malloc(sizeof(char *) * len);
>>   	if (!aliases)
>> -		return;
>> +		return count;
>>   	pmu = NULL;
>>   	j = 0;
>>   	while ((pmu = perf_pmu__scan(pmu)) != NULL)
>> @@ -752,15 +752,17 @@ void print_pmu_events(const char *event_glob, bool name_only)
>>   	for (j = 0; j < len; j++) {
>>   		if (name_only) {
>>   			printf("%s ", aliases[j]);
>> +			count++;
>>   			continue;
>>   		}
>>   		printf("  %-50s [Kernel PMU event]\n", aliases[j]);
>>   		zfree(&aliases[j]);
>> -		printed++;
>> +		count++;
>>   	}
>> -	if (printed)
>> +	if (count)
>>   		printf("\n");
>>   	free(aliases);
>> +	return count;
>>   }
>>   
>>   bool pmu_have_event(const char *pname, const char *name)
>> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
>> index 9183380..2987fe2 100644
>> --- a/tools/perf/util/pmu.h
>> +++ b/tools/perf/util/pmu.h
>> @@ -42,7 +42,7 @@ int perf_pmu__format_parse(char *dir, struct list_head *head);
>>   
>>   struct perf_pmu *perf_pmu__scan(struct perf_pmu *pmu);
>>   
>> -void print_pmu_events(const char *event_glob, bool name_only);
>> +unsigned int print_pmu_events(const char *event_glob, bool name_only);
>>   bool pmu_have_event(const char *pname, const char *name);
>>   
>>   int perf_pmu__test(void);
>> -- 
>> 1.8.2.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 2/8] perf tools: Make the print_pmu_events funtion to return unsigned int.

[Dongsheng Yang]

On 01/08/2014 10:10 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Dec 30, 2013 at 10:26:37AM -0500, Dongsheng Yang escreveu:
>> Sometimes we need to know how many events have been printed in print_pmu_events.
> "Sometimes" is too vague, of course it should be needed, otherwise why
> bother with the patch? It would be better if you provided an example of
> _where_ it will be used, like when printing that newline.
>
> Anyway, reading the other patches,

Arnaldo, I squashed this commit into [1/8] in my next version.
They are doing one thing in fact. Please help to review it.

I hope everything looks okey to you.

Thanx

- Yang
> - Arnaldo
>   
>> This patch make this function to return the number of events we have printed.
>>
>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>> ---
>>   tools/perf/util/pmu.c | 12 +++++++-----
>>   tools/perf/util/pmu.h |  2 +-
>>   2 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 0934d64..331dc2c 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -711,12 +711,12 @@ static int cmp_string(const void *a, const void *b)
>>   	return strcmp(*as, *bs);
>>   }
>>   
>> -void print_pmu_events(const char *event_glob, bool name_only)
>> +unsigned int print_pmu_events(const char *event_glob, bool name_only)
>>   {
>>   	struct perf_pmu *pmu;
>>   	struct perf_pmu_alias *alias;
>>   	char buf[1024];
>> -	int printed = 0;
>> +	unsigned int count = 0;
>>   	int len, j;
>>   	char **aliases;
>>   
>> @@ -727,7 +727,7 @@ void print_pmu_events(const char *event_glob, bool name_only)
>>   			len++;
>>   	aliases = malloc(sizeof(char *) * len);
>>   	if (!aliases)
>> -		return;
>> +		return count;
>>   	pmu = NULL;
>>   	j = 0;
>>   	while ((pmu = perf_pmu__scan(pmu)) != NULL)
>> @@ -752,15 +752,17 @@ void print_pmu_events(const char *event_glob, bool name_only)
>>   	for (j = 0; j < len; j++) {
>>   		if (name_only) {
>>   			printf("%s ", aliases[j]);
>> +			count++;
>>   			continue;
>>   		}
>>   		printf("  %-50s [Kernel PMU event]\n", aliases[j]);
>>   		zfree(&aliases[j]);
>> -		printed++;
>> +		count++;
>>   	}
>> -	if (printed)
>> +	if (count)
>>   		printf("\n");
>>   	free(aliases);
>> +	return count;
>>   }
>>   
>>   bool pmu_have_event(const char *pname, const char *name)
>> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
>> index 9183380..2987fe2 100644
>> --- a/tools/perf/util/pmu.h
>> +++ b/tools/perf/util/pmu.h
>> @@ -42,7 +42,7 @@ int perf_pmu__format_parse(char *dir, struct list_head *head);
>>   
>>   struct perf_pmu *perf_pmu__scan(struct perf_pmu *pmu);
>>   
>> -void print_pmu_events(const char *event_glob, bool name_only);
>> +unsigned int print_pmu_events(const char *event_glob, bool name_only);
>>   bool pmu_have_event(const char *pname, const char *name);
>>   
>>   int perf_pmu__test(void);
>> -- 
>> 1.8.2.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 6/8] perf tools: Make --raw-dump work as a proper option for perf list.

[David Ahern]

On 1/9/14, 7:36 AM, Dongsheng Yang wrote:
> On 01/08/2014 04:44 PM, David Ahern wrote:
>> On 1/8/14, 8:20 AM, Arnaldo Carvalho de Melo wrote:
>>> Em Mon, Dec 30, 2013 at 10:26:41AM -0500, Dongsheng Yang escreveu:
>>>> >Ramkumar reported that perf list --raw-dump was broken by 44d742e.
>>>> >Fix by making raw-dump a proper argument.
>>>> >
>>>> >Signed-off-by: David Ahern<dsahern@gmail.com>
>>> Are you sure David Signed-off-by' this, isn't this just an Acked-by or
>>> perhaps a Reviewed-by? David?
>>
>> I thought the last version of the patch came from Ramkumar.
>>
>> This version seems like an add-on to the last patch.
>>
>
> So, David, do you mind I add "Signed-off-by: David Ahern" in this patch?

I didn't write it and content wise very little comes from me.

David

Re: [PATCH 6/8] perf tools: Make --raw-dump work as a proper option for perf list.

[Dongsheng Yang]

On 01/10/2014 10:30 AM, David Ahern wrote:
> On 1/9/14, 7:36 AM, Dongsheng Yang wrote:
>> On 01/08/2014 04:44 PM, David Ahern wrote:
>>> On 1/8/14, 8:20 AM, Arnaldo Carvalho de Melo wrote:
>>>> Em Mon, Dec 30, 2013 at 10:26:41AM -0500, Dongsheng Yang escreveu:
>>>>> >Ramkumar reported that perf list --raw-dump was broken by 44d742e.
>>>>> >Fix by making raw-dump a proper argument.
>>>>> >
>>>>> >Signed-off-by: David Ahern<dsahern@gmail.com>
>>>> Are you sure David Signed-off-by' this, isn't this just an Acked-by or
>>>> perhaps a Reviewed-by? David?
>>>
>>> I thought the last version of the patch came from Ramkumar.
>>>
>>> This version seems like an add-on to the last patch.
>>>
>>
>> So, David, do you mind I add "Signed-off-by: David Ahern" in this patch?
>
> I didn't write it and content wise very little comes from me.
>

Okey, David, thanx for your reply. I will remove this line in my patch 
next version.

> David
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe 
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>