[PATCH] perf list: Fix the --no-desc option

[PATCH] perf list: Fix the --no-desc option

[Breno Leitao]

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 <leitao@debian.org>
---
 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

Re: [PATCH] perf list: Fix the --no-desc option

[Arnaldo Carvalho de Melo]

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 <leitao@debian.org>
    Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo
 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  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

Re: [PATCH] perf list: Fix the --no-desc option

[Arnaldo Carvalho de Melo]

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]acme@kernel.org> 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]leitao@debian.org>
>          Tested-by: Arnaldo Carvalho de Melo <[3]acme@redhat.com>
> 
>      - Arnaldo
> 
>      > Signed-off-by: Breno Leitao <[4]leitao@debian.org>
>      > ---
>      >  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

Re: [PATCH] perf list: Fix the --no-desc option

[Breno Leitao]

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!

Re: [PATCH] perf list: Fix the --no-desc option

[Ian Rogers]

On Mon, May 13, 2024 at 10:08 AM Breno Leitao <leitao@debian.org> 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!

Re: [PATCH] perf list: Fix the --no-desc option

[Breno Leitao]

On Mon, May 13, 2024 at 10:13:01AM -0700, Ian Rogers wrote:
> On Mon, May 13, 2024 at 10:08 AM Breno Leitao <leitao@debian.org> 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 <leitao@debian.org>
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 <leitao@debian.org>

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);

Re: [PATCH] perf list: Fix the --no-desc option

[Ian Rogers]

On Mon, May 13, 2024 at 1:45 PM Breno Leitao <leitao@debian.org> 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 <leitao@debian.org> 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 <leitao@debian.org>
> 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 <leitao@debian.org>
>
> 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);