Hi Andi,
On Fri, 30 May 2014 14:50:08 -0700, Andi Kleen wrote:
> @@ -756,22 +816,31 @@ void print_pmu_events(const char *event_glob, bool name_only)
> (!is_cpu && strglobmatch(alias->name,
> event_glob))))
> continue;
> - aliases[j] = name;
> - if (is_cpu && !name_only)
> - aliases[j] = format_alias_or(buf, sizeof(buf),
> - pmu, alias);
> - aliases[j] = strdup(aliases[j]);
> + aliases[j].name = name;
> + if (is_cpu && !name_only && !alias->desc)
> + aliases[j].name = format_alias_or(buf,
> + sizeof(buf),
> + pmu, alias);
> + aliases[j].name = strdup(aliases[j].name);
> + aliases[j].desc = alias->desc;
> j++;
> }
> len = j;
> - qsort(aliases, len, sizeof(char *), cmp_string);
> + qsort(aliases, len, sizeof(struct pair), cmp_pair);
> for (j = 0; j < len; j++) {
> if (name_only) {
> - printf("%s ", aliases[j]);
> + printf("%s ", aliases[j].name);
> continue;
> }
> - printf(" %-50s [Kernel PMU event]\n", aliases[j]);
> - zfree(&aliases[j]);
> + if (aliases[j].desc) {
> + if (numdesc++ == 0 && printed)
> + printf("\n");
> + printf(" %-50s [", aliases[j].name);
> + wordwrap(aliases[j].desc, 53, columns, 1);
> + printf("]\n");
> + } else
> + printf(" %-50s [Kernel PMU event]\n", aliases[j].name);
> + zfree(&aliases[j].name);
Hmm.. this will print the description at right side and I think it'd be
better if it prints in another line(s) like below:
agu_bypass_cancel.count [Kernel PMU event]
This event counts executed load operations with all the following
traits: 1. addressing of the format [base + offset], 2. the offset
is between 1 and 2047, 3. the address specified in the base register
is in one page and the address [base+offset] is in an
arith.fpu_div [Kernel PMU event]
Divide operations executed
arith.fpu_div_active [Kernel PMU event]
Cycles when divider is busy executing divide operations
...
I just tweaked it using -v option for perf list. Below is the change I
made on top of your series. What do you think?
Thanks,
Namhyung
diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index 086c96fa959b..e65a3b428a44 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -13,6 +13,7 @@
#include "util/parse-events.h"
#include "util/cache.h"
+#include "util/debug.h"
#include "util/pmu.h"
#include "util/parse-options.h"
@@ -22,6 +23,8 @@ int cmd_list(int argc, const char **argv, const char *prefix __maybe_unused)
const struct option list_options[] = {
OPT_STRING(0, "events-file", &json_file, "json file",
"Read event json file"),
+ OPT_INCR('v', "verbose", &verbose,
+ "be more verbose (show event description if exist)"),
OPT_END()
};
const char * const list_usage[] = {
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index b87f52058bb4..99a2156e6c54 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -838,14 +838,16 @@ void print_pmu_events(const char *event_glob, bool name_only)
printf("%s ", aliases[j].name);
continue;
}
- if (aliases[j].desc) {
- if (numdesc++ == 0 && printed)
- printf("\n");
- printf(" %-50s [", aliases[j].name);
- wordwrap(aliases[j].desc, 53, columns, 1);
- printf("]\n");
- } else
- printf(" %-50s [Kernel PMU event]\n", aliases[j].name);
+
+ if (aliases[j].desc && numdesc++ == 0 && printed)
+ printf("\n");
+ printf(" %-50s [Kernel PMU event]\n", aliases[j].name);
+
+ if (aliases[j].desc && verbose) {
+ printf("%8s", "");
+ wordwrap(aliases[j].desc, 8, columns, 1);
+ printf("\n");
+ }
zfree(&aliases[j].name);
printed++;
}
> Hmm.. this will print the description at right side and I think it'd be
> better if it prints in another line(s) like below:
I think it's better to show the descriptions by default without
an extra option. I suspect most people want to see them, or if they
need them they won't know about obscure -v options.
>
> agu_bypass_cancel.count [Kernel PMU event]
> This event counts executed load operations with all the following
> traits: 1. addressing of the format [base + offset], 2. the offset
> is between 1 and 2047, 3. the address specified in the base register
> is in one page and the address [base+offset] is in an
The problem with this format is that it is not compatible, so it would
break existing parsers that look at perf list output. That is why I ended up
with the right side format.
> arith.fpu_div [Kernel PMU event]
> Divide operations executed
> arith.fpu_div_active [Kernel PMU event]
> Cycles when divider is busy executing divide operations
> ...
>
>
> I just tweaked it using -v option for perf list. Below is the change I
> made on top of your series. What do you think?
I prefer not to apply that patch.
I guess what could make sense is a quiet option to not print
descriptions.
-Andi
Hi Andi,
On Mon, 9 Jun 2014 18:52:45 +0200, Andi Kleen wrote:
>> Hmm.. this will print the description at right side and I think it'd be
>> better if it prints in another line(s) like below:
>
> I think it's better to show the descriptions by default without
> an extra option. I suspect most people want to see them, or if they
> need them they won't know about obscure -v options.
I partially agree, but I guess adding -v option to the command is not
that hard for them. :) Maybe we could add a message in order to advise
users to use -v option for description like below:
$ perf list
# Use -v/--verbose to see event description (if exists)
...
>
>>
>> agu_bypass_cancel.count [Kernel PMU event]
>> This event counts executed load operations with all the following
>> traits: 1. addressing of the format [base + offset], 2. the offset
>> is between 1 and 2047, 3. the address specified in the base register
>> is in one page and the address [base+offset] is in an
>
> The problem with this format is that it is not compatible, so it would
> break existing parsers that look at perf list output. That is why I ended up
> with the right side format.
Hmm.. I think existing parsers should not use -v option then. The perf
list was for listing only event names (and their types).
I think the main problem of the right side format is that it hurts
readability especially on small screens/terminals.
Thanks,
Namhyung
>
>
>> arith.fpu_div [Kernel PMU event]
>> Divide operations executed
>> arith.fpu_div_active [Kernel PMU event]
>> Cycles when divider is busy executing divide operations
>> ...
>>
>>
>> I just tweaked it using -v option for perf list. Below is the change I
>> made on top of your series. What do you think?
>
> I prefer not to apply that patch.
>
> I guess what could make sense is a quiet option to not print
> descriptions.
>
> -Andi