Re: [PATCH 1/5] perf tools: Add fallback functions for cases where cpumode is insufficient

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Mon, Nov 05, 2018 at 07:53:17PM +0000, Hunter, Adrian escreveu:
> > -----Original Message-----
> > From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
> > Sent: Monday, November 5, 2018 9:36 PM
> > To: Hunter, Adrian <adrian.hunter@intel.com>
> > Cc: Jiri Olsa <jolsa@redhat.com>; Andi Kleen <ak@linux.intel.com>; linux-
> > kernel@vger.kernel.org; leo.yan@linaro.org; David Miller
> > <davem@davemloft.net>; Mathieu Poirier <mathieu.poirier@linaro.org>
> > Subject: Re: [PATCH 1/5] perf tools: Add fallback functions for cases where
> > cpumode is insufficient
> > 
> > Em Mon, Nov 05, 2018 at 07:21:44PM +0000, Hunter, Adrian escreveu:
> > > > In Monday, November 5, 2018 7:30 PM, Arnaldo Carvalho de Melo wrote
> > <SNIP>
> > > > > +struct map *thread__find_map_fallback(struct thread *thread, u8
> > > > cpumode,
> > > > > +				      u64 addr, struct addr_location *al) {
> > 
> > > > You named one as _fallback...
> > 
> > > > > +	struct map *map = thread__find_map(thread, cpumode, addr, al);
> > > > > +	struct machine *machine = thread->mg->machine;
> > > > > +	u8 addr_cpumode = machine__addr_cpumode(machine, cpumode,
> > > > addr);
> > > > > +
> > > > > +	if (map || addr_cpumode == cpumode)
> > > > > +		return map;
> > > > > +
> > > > > +	return thread__find_map(thread, addr_cpumode, addr, al); }
> > > > > +
> > > > >  struct symbol *thread__find_symbol(struct thread *thread, u8
> > cpumode,
> > > > >  				   u64 addr, struct addr_location *al)  { @@ -
> > > > 1585,6 +1603,15 @@
> > > > > struct symbol *thread__find_symbol(struct thread *thread, u8
> > cpumode,
> > > > >  	return al->sym;
> > > > >  }
> > 
> > > > > +struct symbol *thread__find_symbol_fb(struct thread *thread, u8
> > > > cpumode,
> > > > > +				      u64 addr, struct addr_location *al)
> > 
> > > > ... and the other as _fb, make it consistent, please.
> > 
> > > ok
> > 
> > > > > +{
> > > > > +	al->sym = NULL;
> > > > > +	if (thread__find_map_fallback(thread, cpumode, addr, al))
> > > > > +		al->sym = map__find_symbol(al->map, al->addr);
> > > > > +	return al->sym;
> > > > > +}
> > 
> > > > >  /*
> > > > >   * Callers need to drop the reference to al->thread, obtained in
> > > > >   * machine__findnew_thread()
> > > > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > > > > index 111ae858cbcb..04edc0eac376 100644
> > > > > --- a/tools/perf/util/machine.c
> > > > > +++ b/tools/perf/util/machine.c
> > > > > @@ -2542,6 +2542,46 @@ int machine__get_kernel_start(struct
> > > > > machine
> > > > *machine)
> > > > >  	return err;
> > > > >  }
> > 
> > > > > +/*
> > > > > + * machine__single_ku_as - Machine has same address space for
> > > > > +kernel
> > > > and user.
> > > > > + * @machine: machine object
> > > > > + *
> > > > > + * Some architectures have a single address space for kernel and
> > > > > +user addresses,
> > > > > + * which makes it possible to determine if an address is in
> > > > > +kernel space or user
> > > > > + * space.
> > > > > + */
> > > > > +static bool machine__single_ku_as(struct machine *machine) {
> > > > > +	return strcmp(perf_env__arch(machine->env), "sparc"); }
> > 
> > > > Can we avoid having this strcmp be done repeatedly?
> > 
> > > It is only done if there are mapping errors
> > 
> > >  > Can we avoid having this strcmp be done repeatedly? I.e. just make
> > > this a
> > > > boolean initialized at session start, when machine->env is setup, so
> > > > we'd
> > > > have:
> > 
> > > >    machine->single_address_space
> > 
> > > > Instead of a function?
> > 
> > > Sure  thing.

Have you sent an update to this patch series addressing the concerns? I
think that renaming that _fb to __fallback and having this
machine->single_address_space is what is missing, I'll try to take a
stab at this now and have it in my perf/core branch soon.

But you have this already done, please send it.

- Arnaldo