linux-kernel - Re: [PATCH 14/20] perf annotate: Pass struct annotation_options to map_symbol__annotation

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]

Date:   Mon, 6 Aug 2018 12:22:19 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     Namhyung Kim <namhyung@...nel.org>, Jiri Olsa <jolsa@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        David Ahern <dsahern@...il.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Stephane Eranian <eranian@...gle.com>, kernel-team@....com
Subject: Re: [PATCH 14/20] perf annotate: Pass struct annotation_options to
 map_symbol__annotation_dump

Em Mon, Aug 06, 2018 at 04:24:47PM +0200, Jiri Olsa escreveu:
> On Mon, Aug 06, 2018 at 10:45:07PM +0900, Namhyung Kim wrote:
> > Hi Jiri,
> > 
> > On Sat, Aug 04, 2018 at 03:05:15PM +0200, Jiri Olsa wrote:
> > > Passing struct annotation_options to map_symbol__annotation_dump,
> > > to carry on and pass the percent_type value.
> > > 
> > > Link: http://lkml.kernel.org/n/tip-5toohgdkgpk3vn6zebusr3bb@git.kernel.org
> > > Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> > > ---
> > 
> > [SNIP]
> > > @@ -2523,7 +2526,7 @@ static void disasm_line__write(struct disasm_line *dl, struct annotation *notes,
> > >  
> > >  static void __annotation_line__write(struct annotation_line *al, struct annotation *notes,
> > >  				     bool first_line, bool current_entry, bool change_color, int width,
> > > -				     void *obj,
> > > +				     void *obj, unsigned int percent_type,
> > >  				     int  (*obj__set_color)(void *obj, int color),
> > >  				     void (*obj__set_percent_color)(void *obj, double percent, bool current),
> > >  				     int  (*obj__set_jumps_percent_color)(void *obj, int nr, bool current),
> > > @@ -2531,7 +2534,7 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
> > >  				     void (*obj__write_graph)(void *obj, int graph))
> > >  
> > >  {
> > > -	double percent_max = annotation_line__max_percent(al, notes);
> > > +	double percent_max = annotation_line__max_percent(al, notes, percent_type);
> > >  	int pcnt_width = annotation__pcnt_width(notes),
> > >  	    cycles_width = annotation__cycles_width(notes);
> > >  	bool show_title = false;
> > > @@ -2552,8 +2555,7 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
> > >  		for (i = 0; i < notes->nr_events; i++) {
> > >  			double percent;
> > >  
> > > -			percent = annotation_data__percent(&al->data[i],
> > > -							   PERCENT_HITS_LOCAL);
> > > +			percent = annotation_data__percent(&al->data[i], percent_type);
> > >  
> > >  			obj__set_percent_color(obj, percent, current_entry);
> > >  			if (notes->options->show_total_period) {
> > > @@ -2680,13 +2682,15 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
> > >  }
> > >  
> > >  void annotation_line__write(struct annotation_line *al, struct annotation *notes,
> > > -			    struct annotation_write_ops *ops)
> > > +			    struct annotation_write_ops *wops,
> > > +			    struct annotation_options *opts)
> > >  {
> > > -	__annotation_line__write(al, notes, ops->first_line, ops->current_entry,
> > > -				 ops->change_color, ops->width, ops->obj,
> > > -				 ops->set_color, ops->set_percent_color,
> > > -				 ops->set_jumps_percent_color, ops->printf,
> > > -				 ops->write_graph);
> > > +	__annotation_line__write(al, notes, wops->first_line, wops->current_entry,
> > > +				 wops->change_color, wops->width, wops->obj,
> > > +				 opts->percent_type,
> > > +				 wops->set_color, wops->set_percent_color,
> > > +				 wops->set_jumps_percent_color, wops->printf,
> > > +				 wops->write_graph);
> > 
> > This doesn't look good.  Why not just passing a pointer to wops
> > instead of each fields separately?
> 
> yep, my thoughts exactly when I saw this ;-) we probably had some

But then, while this is a valid observation, it is not related to this
patchkit, that is just adding an extra config variable, percent_type, at
some point one can try to shorten that function signature, looking at
why it was done this way originally to see if there was any reason or if
its just something to improve by shortening the function signature.

Applying Jiri's patch,

> other caller..  however I only wanted to add one more param ;-)

Right
 
> I'll check what we can do with this in v2

I'm going thru v2 already, so far its an uncontrovertial, trivial, so
I think you better just wait a teeny bit for this to be applied and then
get on over with followup patches, ok?

- Arnaldo