lkml.org 
[lkml]   [2018]   [Nov]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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.

Thanks

> >
> > Also have you considered making this fallback to be performed only from
> > code that is that arch specific?
> >
> > I.e. the code that supports branch samples/stacks is x86_86 specific at this
> > point and thus only in that case we would call the routines that perform the
> > fallback, which, in turn, wouldn't need to check for "sparc"?

> I will look at it, but theoretically someone could be processing x86 data but
> doing it on a machine of a different architecture.

Right, that should be supported, yes. What I meant was that when
processing perf.data file with samples where the cpumode can't be
inferred, we should use the fallback routines.

It is super unfortunate that we have addresses without a accompanying
cpumode :-\ Don't you think those coulde be fixed somehow? If this comes
from things synthesized from Intel PT traces, then we can use the
address ranges for kernel/userspace to derive that before hitting the
core code, that would be fed with addr/cpumode pairs, just like we have
hdr.misc & USER/KERNEL and the PERF_CONTEXT_ markers in callchains.

- Arnaldo

\
 
 \ /
  Last update: 2018-11-05 20:37    [W:2.557 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site