Currently, the --no-desc option in perf list isn't functioning as
intended.
This issue arises from the overwriting of struct option->desc with the
opposite value of struct option->long_desc. Consequently, whatever
parse_options() returns at struct option->desc gets overridden later,
rendering the --desc or --no-desc arguments ineffective.
To resolve this, set ->desc as true by default and allow parse_options()
to adjust it accordingly. This adjustment will fix the --no-desc
option while preserving the functionality of the other parameters.
Signed-off-by: Breno Leitao <[email protected]>
---
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 02bf608d585e..58589f67e800 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -491,6 +491,7 @@ int cmd_list(int argc, const char **argv)
int i, ret = 0;
struct print_state default_ps = {
.fp = stdout,
+ .desc = true,
};
struct print_state json_ps = {
.fp = stdout,
@@ -563,7 +564,6 @@ int cmd_list(int argc, const char **argv)
};
ps = &json_ps;
} else {
- default_ps.desc = !default_ps.long_desc;
default_ps.last_topic = strdup("");
assert(default_ps.last_topic);
default_ps.visited_metrics = strlist__new(NULL, NULL);
--
2.43.0
On Wed, May 08, 2024 at 06:35:17AM -0700, Breno Leitao wrote:
> Currently, the --no-desc option in perf list isn't functioning as
> intended.
>
> This issue arises from the overwriting of struct option->desc with the
> opposite value of struct option->long_desc. Consequently, whatever
> parse_options() returns at struct option->desc gets overridden later,
> rendering the --desc or --no-desc arguments ineffective.
>
> To resolve this, set ->desc as true by default and allow parse_options()
> to adjust it accordingly. This adjustment will fix the --no-desc
> option while preserving the functionality of the other parameters.
Thanks, applied to perf-tools-next, and added this:
Committer testing:
Before:
$ perf list --no-desc
<SNIP>
cache:
longest_lat_cache.miss
[Counts the number of cacheable memory requests that miss in the LLC. Counts on a per core basis. Unit: cpu_atom]
longest_lat_cache.reference
[Counts the number of cacheable memory requests that access the LLC. Counts on a per core basis. Unit: cpu_atom]
mem_bound_stalls.ifetch
[Counts the number of cycles the core is stalled due to an instruction cache or TLB miss which hit in the L2,LLC,DRAM or MMIO (Non-DRAM). Unit: cpu_atom]
mem_bound_stalls.ifetch_dram_hit
[Counts the number of cycles the core is stalled due to an instruction cache or TLB miss which hit in DRAM or MMIO (Non-DRAM). Unit: cpu_atom]
mem_bound_stalls.ifetch_l2_hit
[Counts the number of cycles the core is stalled due to an instruction cache or TLB miss which hit in the L2 cache. Unit: cpu_atom]
mem_bound_stalls.ifetch_llc_hit
[Counts the number of cycles the core is stalled due to an instruction cache or TLB miss which hit in the LLC or other core with HITE/F/M. Unit: cpu_atom]
<SNIP>
After:
$ perf list --no-desc
<SNIP>
cache:
longest_lat_cache.miss
longest_lat_cache.reference
mem_bound_stalls.ifetch
mem_bound_stalls.ifetch_dram_hit
mem_bound_stalls.ifetch_l2_hit
mem_bound_stalls.ifetch_llc_hit
<SNIP>
Signed-off-by: Breno Leitao <[email protected]>
Tested-by: Arnaldo Carvalho de Melo <[email protected]>
- Arnaldo
> Signed-off-by: Breno Leitao <[email protected]>
> ---
> 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 02bf608d585e..58589f67e800 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -491,6 +491,7 @@ int cmd_list(int argc, const char **argv)
> int i, ret = 0;
> struct print_state default_ps = {
> .fp = stdout,
> + .desc = true,
> };
> struct print_state json_ps = {
> .fp = stdout,
> @@ -563,7 +564,6 @@ int cmd_list(int argc, const char **argv)
> };
> ps = &json_ps;
> } else {
> - default_ps.desc = !default_ps.long_desc;
> default_ps.last_topic = strdup("");
> assert(default_ps.last_topic);
> default_ps.visited_metrics = strlist__new(NULL, NULL);
> --
> 2.43.0
On Sat, May 11, 2024 at 09:11:00AM -0700, Ian Rogers wrote:
> On Sat, May 11, 2024, 9:00 AM Arnaldo Carvalho de Melo
> <[1][email protected]> wrote:
>
> On Wed, May 08, 2024 at 06:35:17AM -0700, Breno Leitao wrote:
> > Currently, the --no-desc option in perf list isn't functioning as
> > intended.
> >
> > This issue arises from the overwriting of struct option->desc with
> the
> > opposite value of struct option->long_desc. Consequently, whatever
> > parse_options() returns at struct option->desc gets overridden
> later,
> > rendering the --desc or --no-desc arguments ineffective.
> >
> > To resolve this, set ->desc as true by default and allow
> parse_options()
> > to adjust it accordingly. This adjustment will fix the --no-desc
> > option while preserving the functionality of the other parameters.
>
> Thanks, applied to perf-tools-next, and added this:
>
> Committer testing:
>
> Before:
>
> $ perf list --no-desc
> <SNIP>
> cache:
> longest_lat_cache.miss
> [Counts the number of cacheable memory requests that
> miss in the LLC. Counts on a per core basis. Unit: cpu_atom]
> longest_lat_cache.reference
> [Counts the number of cacheable memory requests that
> access the LLC. Counts on a per core basis. Unit: cpu_atom]
> mem_bound_stalls.ifetch
> [Counts the number of cycles the core is stalled due to
> an instruction cache or TLB miss which hit in the L2,LLC,DRAM or
> MMIO (Non-DRAM). Unit: cpu_atom]
> mem_bound_stalls.ifetch_dram_hit
> [Counts the number of cycles the core is stalled due to
> an instruction cache or TLB miss which hit in DRAM or MMIO
> (Non-DRAM). Unit: cpu_atom]
> mem_bound_stalls.ifetch_l2_hit
> [Counts the number of cycles the core is stalled due to
> an instruction cache or TLB miss which hit in the L2 cache. Unit:
> cpu_atom]
> mem_bound_stalls.ifetch_llc_hit
> [Counts the number of cycles the core is stalled due to
> an instruction cache or TLB miss which hit in the LLC or other core
> with HITE/F/M. Unit: cpu_atom]
> <SNIP>
>
> After:
>
> $ perf list --no-desc
> <SNIP>
> cache:
> longest_lat_cache.miss
> longest_lat_cache.reference
> mem_bound_stalls.ifetch
> mem_bound_stalls.ifetch_dram_hit
> mem_bound_stalls.ifetch_l2_hit
> mem_bound_stalls.ifetch_llc_hit
> <SNIP>
>
> Signed-off-by: Breno Leitao <[2][email protected]>
> Tested-by: Arnaldo Carvalho de Melo <[3][email protected]>
>
> - Arnaldo
>
> > Signed-off-by: Breno Leitao <[4][email protected]>
> > ---
> > 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 02bf608d585e..58589f67e800 100644
> > --- a/tools/perf/builtin-list.c
> > +++ b/tools/perf/builtin-list.c
> > @@ -491,6 +491,7 @@ int cmd_list(int argc, const char **argv)
> > int i, ret = 0;
> > struct print_state default_ps = {
> > .fp = stdout,
> > + .desc = true,
> > };
> > struct print_state json_ps = {
> > .fp = stdout,
> > @@ -563,7 +564,6 @@ int cmd_list(int argc, const char **argv)
> > };
> > ps = &json_ps;
> > } else {
> > - default_ps.desc = !default_ps.long_desc;
>
> Given this, did you test the long description behavior?
So, without the patch:
perf list
<SNIP>
cache:
longest_lat_cache.miss
[Counts the number of cacheable memory requests that miss in the LLC. Counts on a per core basis. Unit: cpu_atom]
<SNIP>
perf list --long-desc
<SNIP>
cache:
longest_lat_cache.miss
[Counts the number of cacheable memory requests that miss in the Last Level Cache (LLC). Requests include demand loads,reads for ownership (RFO),instruction fetches and L1 HW
prefetches. If the platform has an L3 cache,the LLC is the L3 cache,otherwise it is the L2 cache. Counts on a per core basis]
<SNIP>
perf list --no-desc
<SNIP>
cache:
longest_lat_cache.miss
[Counts the number of cacheable memory requests that miss in the LLC. Counts on a per core basis. Unit: cpu_atom]
<SNIP>
With Breno's patch the default doesn't change, --no-desc gets fixed but
--long-desc is broken:
perf list --long-desc
<SNIP>
cache:
longest_lat_cache.miss
[Counts the number of cacheable memory requests that miss in the LLC. Counts on a per core basis. Unit: cpu_atom]
[Counts the number of cacheable memory requests that miss in the Last Level Cache (LLC). Requests include demand loads,reads for ownership (RFO),instruction fetches and L1 HW
prefetches. If the platform has an L3 cache,the LLC is the L3 cache,otherwise it is the L2 cache. Counts on a per core basis]
<SNIP>
Thanks for asking the question, I'm dropping the patch, Breno, can you
try again?
- Arnaldo
On Sat, May 11, 2024 at 02:25:53PM -0300, Arnaldo Carvalho de Melo wrote:
> With Breno's patch the default doesn't change, --no-desc gets fixed but
> --long-desc is broken:
>
> perf list --long-desc
> <SNIP>
> cache:
> longest_lat_cache.miss
> [Counts the number of cacheable memory requests that miss in the LLC. Counts on a per core basis. Unit: cpu_atom]
> [Counts the number of cacheable memory requests that miss in the Last Level Cache (LLC). Requests include demand loads,reads for ownership (RFO),instruction fetches and L1 HW
> prefetches. If the platform has an L3 cache,the LLC is the L3 cache,otherwise it is the L2 cache. Counts on a per core basis]
> <SNIP>
Oh, both descriptions (long and "short") are being displayed.
> Thanks for asking the question, I'm dropping the patch, Breno, can you
> try again?
Sure, let me think about it and send a v2.
Thanks!
On Mon, May 13, 2024 at 10:08 AM Breno Leitao <[email protected]> wrote:
>
> On Sat, May 11, 2024 at 02:25:53PM -0300, Arnaldo Carvalho de Melo wrote:
> > With Breno's patch the default doesn't change, --no-desc gets fixed but
> > --long-desc is broken:
> >
> > perf list --long-desc
> > <SNIP>
> > cache:
> > longest_lat_cache.miss
> > [Counts the number of cacheable memory requests that miss in the LLC. Counts on a per core basis. Unit: cpu_atom]
> > [Counts the number of cacheable memory requests that miss in the Last Level Cache (LLC). Requests include demand loads,reads for ownership (RFO),instruction fetches and L1 HW
> > prefetches. If the platform has an L3 cache,the LLC is the L3 cache,otherwise it is the L2 cache. Counts on a per core basis]
> > <SNIP>
>
> Oh, both descriptions (long and "short") are being displayed.
>
> > Thanks for asking the question, I'm dropping the patch, Breno, can you
> > try again?
>
> Sure, let me think about it and send a v2.
Thanks Breno! My bug, thanks for digging into this.
Ian
> Thanks!
On Mon, May 13, 2024 at 10:13:01AM -0700, Ian Rogers wrote:
> On Mon, May 13, 2024 at 10:08 AM Breno Leitao <[email protected]> wrote:
> > On Sat, May 11, 2024 at 02:25:53PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Thanks for asking the question, I'm dropping the patch, Breno, can you
> > > try again?
> >
> > Sure, let me think about it and send a v2.
>
> Thanks Breno! My bug, thanks for digging into this.
How about something like this:
Author: Breno Leitao <[email protected]>
Date: Wed May 8 06:35:17 2024 -0700
perf list: Fix the --no-desc option
Currently, the --no-desc option in perf list isn't functioning as
intended.
This issue arises from the overwriting of struct option->desc with the
opposite value of struct option->long_desc. Consequently, whatever
parse_options() returns at struct option->desc gets overridden later,
rendering the --desc or --no-desc arguments ineffective.
To resolve this, set ->desc as true by default and allow parse_options()
to adjust it accordingly. This adjustment will fix the --no-desc
option while preserving the functionality of the other parameters.
Signed-off-by: Breno Leitao <[email protected]>
diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index e27a1b1288c2..16186acdd301 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -149,7 +149,11 @@ static void default_print_event(void *ps, const char *pmu_name, const char *topi
} else
fputc('\n', fp);
- if (desc && print_state->desc) {
+ if (long_desc && print_state->long_desc) {
+ fprintf(fp, "%*s", 8, "[");
+ wordwrap(fp, long_desc, 8, pager_get_columns(), 0);
+ fprintf(fp, "]\n");
+ } else if (desc && print_state->desc) {
char *desc_with_unit = NULL;
int desc_len = -1;
@@ -165,12 +169,6 @@ static void default_print_event(void *ps, const char *pmu_name, const char *topi
fprintf(fp, "]\n");
free(desc_with_unit);
}
- long_desc = long_desc ?: desc;
- if (long_desc && print_state->long_desc) {
- fprintf(fp, "%*s", 8, "[");
- wordwrap(fp, long_desc, 8, pager_get_columns(), 0);
- fprintf(fp, "]\n");
- }
if (print_state->detailed && encoding_desc) {
fprintf(fp, "%*s", 8, "");
@@ -484,6 +482,7 @@ int cmd_list(int argc, const char **argv)
int i, ret = 0;
struct print_state default_ps = {
.fp = stdout,
+ .desc = true,
};
struct print_state json_ps = {
.fp = stdout,
@@ -556,7 +555,6 @@ int cmd_list(int argc, const char **argv)
};
ps = &json_ps;
} else {
- default_ps.desc = !default_ps.long_desc;
default_ps.last_topic = strdup("");
assert(default_ps.last_topic);
default_ps.visited_metrics = strlist__new(NULL, NULL);
On Mon, May 13, 2024 at 1:45 PM Breno Leitao <[email protected]> wrote:
>
> On Mon, May 13, 2024 at 10:13:01AM -0700, Ian Rogers wrote:
> > On Mon, May 13, 2024 at 10:08 AM Breno Leitao <[email protected]> wrote:
> > > On Sat, May 11, 2024 at 02:25:53PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Thanks for asking the question, I'm dropping the patch, Breno, can you
> > > > try again?
> > >
> > > Sure, let me think about it and send a v2.
> >
> > Thanks Breno! My bug, thanks for digging into this.
>
> How about something like this:
Looks good, could you send as a v2 and I can test.
Thanks,
Ian
> Author: Breno Leitao <[email protected]>
> Date: Wed May 8 06:35:17 2024 -0700
>
> perf list: Fix the --no-desc option
>
> Currently, the --no-desc option in perf list isn't functioning as
> intended.
>
> This issue arises from the overwriting of struct option->desc with the
> opposite value of struct option->long_desc. Consequently, whatever
> parse_options() returns at struct option->desc gets overridden later,
> rendering the --desc or --no-desc arguments ineffective.
>
> To resolve this, set ->desc as true by default and allow parse_options()
> to adjust it accordingly. This adjustment will fix the --no-desc
> option while preserving the functionality of the other parameters.
>
> Signed-off-by: Breno Leitao <[email protected]>
>
> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> index e27a1b1288c2..16186acdd301 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -149,7 +149,11 @@ static void default_print_event(void *ps, const char *pmu_name, const char *topi
> } else
> fputc('\n', fp);
>
> - if (desc && print_state->desc) {
> + if (long_desc && print_state->long_desc) {
> + fprintf(fp, "%*s", 8, "[");
> + wordwrap(fp, long_desc, 8, pager_get_columns(), 0);
> + fprintf(fp, "]\n");
> + } else if (desc && print_state->desc) {
> char *desc_with_unit = NULL;
> int desc_len = -1;
>
> @@ -165,12 +169,6 @@ static void default_print_event(void *ps, const char *pmu_name, const char *topi
> fprintf(fp, "]\n");
> free(desc_with_unit);
> }
> - long_desc = long_desc ?: desc;
> - if (long_desc && print_state->long_desc) {
> - fprintf(fp, "%*s", 8, "[");
> - wordwrap(fp, long_desc, 8, pager_get_columns(), 0);
> - fprintf(fp, "]\n");
> - }
>
> if (print_state->detailed && encoding_desc) {
> fprintf(fp, "%*s", 8, "");
> @@ -484,6 +482,7 @@ int cmd_list(int argc, const char **argv)
> int i, ret = 0;
> struct print_state default_ps = {
> .fp = stdout,
> + .desc = true,
> };
> struct print_state json_ps = {
> .fp = stdout,
> @@ -556,7 +555,6 @@ int cmd_list(int argc, const char **argv)
> };
> ps = &json_ps;
> } else {
> - default_ps.desc = !default_ps.long_desc;
> default_ps.last_topic = strdup("");
> assert(default_ps.last_topic);
> default_ps.visited_metrics = strlist__new(NULL, NULL);