[PATCH] perf list: fix --raw-dump

[PATCH] perf list: fix --raw-dump

[Ramkumar Ramachandra]

While adding usage information, 44d742e (perf list: Add usage,
2013-10-30) broke

  $ perf list --raw-dump

by asking parse_options() to stop at
non-option (PARSE_OPT_STOP_AT_NON_OPTION), hence parsing the option
--raw-dump and barfing. Ask it to keep unknown options
unparsed (PARSE_OPT_KEEP_UNKNOWN) so that we have a chance to handle it
later. The trade-off that this patch makes is that

  $ perf list --foo

will not error out like it used to.

Fixes: 44d742e01e6d (perf list: Add usage)
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 I spotted this while trying to complete

   $ perf record -e <TAB>

 tools/perf/builtin-list.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index 011195e..8b076f3 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -28,7 +28,7 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 	};
 
 	argc = parse_options(argc, argv, list_options, list_usage,
-			     PARSE_OPT_STOP_AT_NON_OPTION);
+			PARSE_OPT_KEEP_UNKNOWN);
 
 	setup_pager();
 
-- 
1.8.5.rc0.5.g70ebc73.dirty

Re: [PATCH] perf list: fix --raw-dump

[David Ahern]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

On 12/11/13, 3:16 AM, Ramkumar Ramachandra wrote:
> While adding usage information, 44d742e (perf list: Add usage,
> 2013-10-30) broke
>
>    $ perf list --raw-dump
>
> by asking parse_options() to stop at
> non-option (PARSE_OPT_STOP_AT_NON_OPTION), hence parsing the option
> --raw-dump and barfing. Ask it to keep unknown options
> unparsed (PARSE_OPT_KEEP_UNKNOWN) so that we have a chance to handle it
> later. The trade-off that this patch makes is that
>
>    $ perf list --foo
>
> will not error out like it used to.

Why not make raw_dump a proper argument?


[-- Attachment #2: 0001-perf-list-Fix-raw-dump-arg.patch --]
[-- Type: text/plain, Size: 1678 bytes --]

>From b4444edaeffba7ebc459ee1b2cbe4f9cbd057476 Mon Sep 17 00:00:00 2001
From: David Ahern <dsahern@gmail.com>
Date: Wed, 11 Dec 2013 14:00:20 -0700
Subject: [PATCH] perf list: Fix raw-dump arg

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>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Ramkumar Ramachandra <artagnon@gmail.com>
---
 tools/perf/builtin-list.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index 011195e38f21..b553d0c4ca82 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -19,7 +19,9 @@
 int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	int i;
+	bool raw_dump = false;
 	const struct option list_options[] = {
+		OPT_BOOLEAN(0, "raw-dump", &raw_dump, "raw dump for completion"),
 		OPT_END()
 	};
 	const char * const list_usage[] = {
@@ -36,6 +38,10 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 		print_events(NULL, false);
 		return 0;
 	}
+	if (raw_dump) {
+		print_events(NULL, true);
+		return 0;
+	}
 
 	for (i = 0; i < argc; ++i) {
 		if (i)
@@ -53,8 +59,6 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 			print_hwcache_events(NULL, false);
 		else if (strcmp(argv[i], "pmu") == 0)
 			print_pmu_events(NULL, false);
-		else if (strcmp(argv[i], "--raw-dump") == 0)
-			print_events(NULL, true);
 		else {
 			char *sep = strchr(argv[i], ':'), *s;
 			int sep_idx;
-- 
1.8.3.4 (Apple Git-47)

Re: [PATCH] perf list: fix --raw-dump

[Ramkumar Ramachandra]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

David Ahern wrote:
> Why not make raw_dump a proper argument?

Sure, that'd work too. I was thinking of a minimal way to fix the
problem myself.

> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> index 011195e38f21..b553d0c4ca82 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -36,6 +38,10 @@ int cmd_list(int argc, const char
>              print_events(NULL, false);
>              return 0;
>        }
> +      if (raw_dump) {
> +            print_events(NULL, true);
> +            return 0;
> +      }

This won't work because you've put it right below the `if (argc ==
0)`, which executes print_events(). You could move it up and get it to
work.

[-- Attachment #2: 0001-perf-list-Fix-raw-dump-arg.patch --]
[-- Type: text/x-patch, Size: 1721 bytes --]

From 7198a494cfef43395e8683ac3a0576277b8d1d80 Mon Sep 17 00:00:00 2001
From: David Ahern <dsahern@gmail.com>
Date: Wed, 11 Dec 2013 14:00:20 -0700
Subject: [PATCH] perf list: Fix raw-dump arg

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>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 tools/perf/builtin-list.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index 011195e..2629c24 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -19,7 +19,9 @@
 int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	int i;
+	bool raw_dump = false;
 	const struct option list_options[] = {
+		OPT_BOOLEAN(0, "raw-dump", &raw_dump, "raw dump for completion"),
 		OPT_END()
 	};
 	const char * const list_usage[] = {
@@ -32,6 +34,10 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 
 	setup_pager();
 
+	if (raw_dump) {
+		print_events(NULL, true);
+		return 0;
+	}
 	if (argc == 0) {
 		print_events(NULL, false);
 		return 0;
@@ -53,8 +59,6 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 			print_hwcache_events(NULL, false);
 		else if (strcmp(argv[i], "pmu") == 0)
 			print_pmu_events(NULL, false);
-		else if (strcmp(argv[i], "--raw-dump") == 0)
-			print_events(NULL, true);
 		else {
 			char *sep = strchr(argv[i], ':'), *s;
 			int sep_idx;
-- 
1.8.5.1.113.g8cb5bef.dirty

Re: [PATCH] perf list: fix --raw-dump

[David Ahern]

On 12/12/13, 12:34 AM, Ramkumar Ramachandra wrote:
> David Ahern wrote:
>> Why not make raw_dump a proper argument?
>
> Sure, that'd work too. I was thinking of a minimal way to fix the
> problem myself.
>
>> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
>> index 011195e38f21..b553d0c4ca82 100644
>> --- a/tools/perf/builtin-list.c
>> +++ b/tools/perf/builtin-list.c
>> @@ -36,6 +38,10 @@ int cmd_list(int argc, const char
>>               print_events(NULL, false);
>>               return 0;
>>         }
>> +      if (raw_dump) {
>> +            print_events(NULL, true);
>> +            return 0;
>> +      }
>
> This won't work because you've put it right below the `if (argc ==
> 0)`, which executes print_events(). You could move it up and get it to
> work.
>

your updated patch looks good to me.

David

Re: [PATCH] perf list: fix --raw-dump

[Arnaldo Carvalho de Melo]

Em Thu, Dec 12, 2013 at 01:04:04PM +0530, Ramkumar Ramachandra escreveu:
> David Ahern wrote:
> > Why not make raw_dump a proper argument?
> 
> Sure, that'd work too. I was thinking of a minimal way to fix the
> problem myself.

So is the patch below the one to apply? When these things happen, it
helps if you resubmit with a v2 in the subject line and then an explicit
Ack is provided, be it from, in this case, David, or Ramkumar could just
add it since David provided it in free form, ok?

- Arnaldo
 
> > diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> > index 011195e38f21..b553d0c4ca82 100644
> > --- a/tools/perf/builtin-list.c
> > +++ b/tools/perf/builtin-list.c
> > @@ -36,6 +38,10 @@ int cmd_list(int argc, const char
> >              print_events(NULL, false);
> >              return 0;
> >        }
> > +      if (raw_dump) {
> > +            print_events(NULL, true);
> > +            return 0;
> > +      }
> 
> This won't work because you've put it right below the `if (argc ==
> 0)`, which executes print_events(). You could move it up and get it to
> work.

> From 7198a494cfef43395e8683ac3a0576277b8d1d80 Mon Sep 17 00:00:00 2001
> From: David Ahern <dsahern@gmail.com>
> Date: Wed, 11 Dec 2013 14:00:20 -0700
> Subject: [PATCH] perf list: Fix raw-dump arg
> 
> 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>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Ramkumar Ramachandra <artagnon@gmail.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  tools/perf/builtin-list.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> index 011195e..2629c24 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -19,7 +19,9 @@
>  int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
>  {
>  	int i;
> +	bool raw_dump = false;
>  	const struct option list_options[] = {
> +		OPT_BOOLEAN(0, "raw-dump", &raw_dump, "raw dump for completion"),
>  		OPT_END()
>  	};
>  	const char * const list_usage[] = {
> @@ -32,6 +34,10 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
>  
>  	setup_pager();
>  
> +	if (raw_dump) {
> +		print_events(NULL, true);
> +		return 0;
> +	}
>  	if (argc == 0) {
>  		print_events(NULL, false);
>  		return 0;
> @@ -53,8 +59,6 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
>  			print_hwcache_events(NULL, false);
>  		else if (strcmp(argv[i], "pmu") == 0)
>  			print_pmu_events(NULL, false);
> -		else if (strcmp(argv[i], "--raw-dump") == 0)
> -			print_events(NULL, true);
>  		else {
>  			char *sep = strchr(argv[i], ':'), *s;
>  			int sep_idx;
> -- 
> 1.8.5.1.113.g8cb5bef.dirty
> 

Re: [PATCH] perf list: fix --raw-dump

[Ramkumar Ramachandra]

Arnaldo Carvalho de Melo wrote:
> So is the patch below the one to apply? When these things happen, it
> helps if you resubmit with a v2 in the subject line and then an explicit
> Ack is provided, be it from, in this case, David, or Ramkumar could just
> add it since David provided it in free form, ok?

Is any action required on my part: should I resubmit this, or has it
already been picked up?

p.s- Sorry about the terribly late response; was on vacation.

Re: [PATCH] perf list: fix --raw-dump

[Dongsheng Yang]

On 12/29/2013 05:11 AM, Ramkumar Ramachandra wrote:
> Arnaldo Carvalho de Melo wrote:
>> So is the patch below the one to apply? When these things happen, it
>> helps if you resubmit with a v2 in the subject line and then an explicit
>> Ack is provided, be it from, in this case, David, or Ramkumar could just
>> add it since David provided it in free form, ok?
> Is any action required on my part: should I resubmit this, or has it
> already been picked up?

Hi Ramkumar,
     I sent a patch set for enhancement of perf list recently,
[PATCH 0/8 V2] Enhancement for perf list.
<http://lkml.kernel.org/r/cover.1388416837.git.yangds.fnst@cn.fujitsu.com>.

  It contains a patch [6/8] is very similar with yours here. But I make it
works as a option for other argument,  such as hw, too. And I add
a signed-off-by Ramkumar and signed-off-by David in that patch.

Do you mind we turn to that thread to fix --raw-dump ??

Could you help to take a look at it??

Yang
>
> p.s- Sorry about the terribly late response; was on vacation.
> --
> 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] perf list: fix --raw-dump

[Dongsheng Yang]

[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]

On 12/12/2013 02:34 AM, Ramkumar Ramachandra wrote:
> David Ahern wrote:
>> Why not make raw_dump a proper argument?
> Sure, that'd work too. I was thinking of a minimal way to fix the
> problem myself.

Hi Ramkumar and David,
     If this argument is only used for perf complement, how about make 
it hidden to user? Please refer https://lkml.org/lkml/2013/12/12/52 and 
https://lkml.org/lkml/2013/12/12/53 The two patches make --raw-dump used 
internally but hidden in output of '-h'.

     If you think we should expose it to user, I think we should make it 
work as an option rather than an argument, and make the output of it 
more readable,
how about the patch attached in this mail?

Example:
     # ./perf list cache --raw-dump
     L1-dcache-loads
     L1-dcache-load-misses
     L1-dcache-stores
     L1-dcache-store-misses
     L1-dcache-prefetch-misses
     L1-icache-load-misses
     LLC-loads
     LLC-stores
     LLC-prefetches
     dTLB-loads
     dTLB-load-misses
     dTLB-stores
     dTLB-store-misses
     iTLB-loads
     iTLB-load-misses
     branch-loads
     branch-load-misses




[-- Attachment #2: 0001-perf-list-Fix-raw-dump-arg.patch --]
[-- Type: text/x-patch, Size: 6381 bytes --]

>From 7bd20eb147783e0c1e659b3bceeebb75aad8436c Mon Sep 17 00:00:00 2001
From: David Ahern <dsahern@gmail.com>
Date: Wed, 11 Dec 2013 14:00:20 -0700
Subject: [PATCH] perf list: Fix raw-dump arg

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

Signed-off-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 tools/perf/builtin-list.c      | 23 +++++++++++------------
 tools/perf/util/parse-events.c | 19 +++++++++++--------
 tools/perf/util/parse-events.h |  2 +-
 tools/perf/util/pmu.c          |  2 +-
 4 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index 011195e..9cf12f2 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -19,7 +19,9 @@
 int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	int i;
+	bool raw_dump = false;
 	const struct option list_options[] = {
+		OPT_BOOLEAN(0, "raw-dump", &raw_dump, "raw dump for completion"),
 		OPT_END()
 	};
 	const char * const list_usage[] = {
@@ -27,13 +29,12 @@ 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();
 
 	if (argc == 0) {
-		print_events(NULL, false);
+		print_events(NULL, raw_dump);
 		return 0;
 	}
 
@@ -41,26 +42,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)
-			print_tracepoint_events(NULL, NULL, false);
+			print_tracepoint_events(NULL, NULL, raw_dump);
 		else if (strcmp(argv[i], "hw") == 0 ||
 			 strcmp(argv[i], "hardware") == 0)
-			print_events_type(PERF_TYPE_HARDWARE);
+			print_events_type(PERF_TYPE_HARDWARE, raw_dump);
 		else if (strcmp(argv[i], "sw") == 0 ||
 			 strcmp(argv[i], "software") == 0)
-			print_events_type(PERF_TYPE_SOFTWARE);
+			print_events_type(PERF_TYPE_SOFTWARE, raw_dump);
 		else if (strcmp(argv[i], "cache") == 0 ||
 			 strcmp(argv[i], "hwcache") == 0)
-			print_hwcache_events(NULL, false);
+			print_hwcache_events(NULL, raw_dump);
 		else if (strcmp(argv[i], "pmu") == 0)
-			print_pmu_events(NULL, false);
-		else if (strcmp(argv[i], "--raw-dump") == 0)
-			print_events(NULL, true);
+			print_pmu_events(NULL, raw_dump);
 		else {
 			char *sep = strchr(argv[i], ':'), *s;
 			int sep_idx;
 
 			if (sep == NULL) {
-				print_events(argv[i], false);
+				print_events(argv[i], raw_dump);
 				continue;
 			}
 			sep_idx = sep - argv[i];
@@ -69,7 +68,7 @@ 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);
+			print_tracepoint_events(s, s + sep_idx + 1, raw_dump);
 			free(s);
 		}
 	}
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 969cb8f..ded4a4e 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1034,7 +1034,7 @@ void print_tracepoint_events(const char *subsys_glob, const char *event_glob,
 				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);
 				continue;
 			}
 
@@ -1117,7 +1117,7 @@ static bool is_event_supported(u8 type, unsigned config)
 }
 
 static void __print_events_type(u8 type, struct event_symbol *syms,
-				unsigned max)
+				unsigned max, bool name_only)
 {
 	char name[64];
 	unsigned i;
@@ -1132,16 +1132,19 @@ static void __print_events_type(u8 type, struct event_symbol *syms,
 		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]);
 	}
 }
 
-void print_events_type(u8 type)
+void print_events_type(u8 type, bool name_only)
 {
 	if (type == PERF_TYPE_SOFTWARE)
-		__print_events_type(type, event_symbols_sw, PERF_COUNT_SW_MAX);
+		__print_events_type(type, event_symbols_sw, PERF_COUNT_SW_MAX, name_only);
 	else
-		__print_events_type(type, event_symbols_hw, PERF_COUNT_HW_MAX);
+		__print_events_type(type, event_symbols_hw, PERF_COUNT_HW_MAX, name_only);
 }
 
 int print_hwcache_events(const char *event_glob, bool name_only)
@@ -1166,7 +1169,7 @@ 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]);
@@ -1198,7 +1201,7 @@ static void print_symbol_events(const char *event_glob, unsigned type,
 			continue;
 
 		if (name_only) {
-			printf("%s ", syms->symbol);
+			printf("%s\n", syms->symbol);
 			continue;
 		}
 
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index f1cb4c4..8f991e2 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);
 
 void print_events(const char *event_glob, bool name_only);
-void print_events_type(u8 type);
+void print_events_type(u8 type, bool name_only);
 void 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);
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 56fc10a..1c57356 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -751,7 +751,7 @@ void 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]);
 			continue;
 		}
 		printf("  %-50s [Kernel PMU event]\n", aliases[j]);
-- 
1.8.2.1