linux-kernel - Re: [PATCH 2/8] perf annotate: Fix --show-total-period for tui/stdio2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200227131448.GC9899@kernel.org>
Date:   Thu, 27 Feb 2020 10:14:48 -0300
From:   Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
To:     Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
Cc:     jolsa@...hat.com, xieyisheng1@...wei.com,
        alexey.budankov@...ux.intel.com, treeze.taeung@...il.com,
        adrian.hunter@...el.com, tmricht@...ux.ibm.com,
        namhyung@...nel.org, irogers@...gle.com, songliubraving@...com,
        yao.jin@...ux.intel.com, changbin.du@...el.com, leo.yan@...aro.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/8] perf annotate: Fix --show-total-period for tui/stdio2

Em Thu, Feb 13, 2020 at 12:13:00PM +0530, Ravi Bangoria escreveu:
> perf annotate --show-total-period does not really show total period.
> The reason is we have two separate variables for the same purpose.
> One is in symbol_conf.show_total_period and another is
> annotation_options.show_total_period. We save command line option
> in symbol_conf.show_total_period but uses
> annotation_option.show_total_period while rendering tui/stdio2 browser.
> 
> Though, we copy symbol_conf.show_total_period to
> annotation__default_options.show_total_period but that is not really
> effective as we don't use annotation__default_options once we copy
> default options to dynamic variable annotate.opts in cmd_annotate().
> 
> Instead of all these complication, keep only one variable and use it
> all over. symbol_conf.show_total_period is used by perf report/top as
> well. So let's kill annotation_options.show_total_period.
> 
> On a side note, I've kept annotation_options.show_total_period definition
> because it's still used by perf-config code. Follow up patch to fix
> perf-config for annotate will remove annotation_options.show_total_period.

IIRC this was an attempt to reduce the number of entries in symbol_conf,
that became a hodpodge of options, all global, argh.

But then, making changes like that sticky in a session, i.e. you go
annotate some symbol, toggle some option, then go back to the main
window and go back to annotation and things are the way you left off in
the previous symbol has been over the years considered a nice feature...

So I'll apply your changes, at some point we can sanitize symbol_conf.

Thanks,

- Arnaldo
 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
> ---
>  tools/perf/ui/browsers/annotate.c | 6 +++---
>  tools/perf/util/annotate.c        | 5 ++---
>  tools/perf/util/annotate.h        | 2 +-
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 0dbbf35e6ed1..7e5b44becb5c 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -833,13 +833,13 @@ static int annotate_browser__run(struct annotate_browser *browser,
>  			map_symbol__annotation_dump(ms, evsel, browser->opts);
>  			continue;
>  		case 't':
> -			if (notes->options->show_total_period) {
> -				notes->options->show_total_period = false;
> +			if (symbol_conf.show_total_period) {
> +				symbol_conf.show_total_period = false;
>  				notes->options->show_nr_samples = true;
>  			} else if (notes->options->show_nr_samples)
>  				notes->options->show_nr_samples = false;
>  			else
> -				notes->options->show_total_period = true;
> +				symbol_conf.show_total_period = true;
>  			annotation__update_column_widths(notes);
>  			continue;
>  		case 'c':
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index b2a26adeb4cd..c0c3832e3789 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -2892,7 +2892,7 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
>  			percent = annotation_data__percent(&al->data[i], percent_type);
>  
>  			obj__set_percent_color(obj, percent, current_entry);
> -			if (notes->options->show_total_period) {
> +			if (symbol_conf.show_total_period) {
>  				obj__printf(obj, "%11" PRIu64 " ", al->data[i].he.period);
>  			} else if (notes->options->show_nr_samples) {
>  				obj__printf(obj, "%6" PRIu64 " ",
> @@ -2908,7 +2908,7 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
>  			obj__printf(obj, "%-*s", pcnt_width, " ");
>  		else {
>  			obj__printf(obj, "%-*s", pcnt_width,
> -					   notes->options->show_total_period ? "Period" :
> +					   symbol_conf.show_total_period ? "Period" :
>  					   notes->options->show_nr_samples ? "Samples" : "Percent");
>  		}
>  	}
> @@ -3132,7 +3132,6 @@ void annotation_config__init(void)
>  {
>  	perf_config(annotation__config, NULL);
>  
> -	annotation__default_options.show_total_period = symbol_conf.show_total_period;
>  	annotation__default_options.show_nr_samples   = symbol_conf.show_nr_samples;
>  }
>  
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 2f333dfb586d..632e28b67990 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -307,7 +307,7 @@ static inline int annotation__cycles_width(struct annotation *notes)
>  
>  static inline int annotation__pcnt_width(struct annotation *notes)
>  {
> -	return (notes->options->show_total_period ? 12 : 7) * notes->nr_events;
> +	return (symbol_conf.show_total_period ? 12 : 7) * notes->nr_events;
>  }
>  
>  static inline bool annotation_line__filter(struct annotation_line *al, struct annotation *notes)
> -- 
> 2.24.1
> 

-- 

- Arnaldo