> -----Original Message-----
> From: Arnaldo Carvalho de Melo [mailto:a...@kernel.org]
> Sent: Monday, November 5, 2018 7:30 PM
> To: Hunter, Adrian <adrian.hun...@intel.com>
> Cc: Jiri Olsa <jo...@redhat.com>; Andi Kleen <a...@linux.intel.com>; linux-
> ker...@vger.kernel.org; leo....@linaro.org; David Miller
> <da...@davemloft.net>; Mathieu Poirier <mathieu.poir...@linaro.org>
> Subject: Re: [PATCH 1/5] perf tools: Add fallback functions for cases where
> cpumode is insufficient
>
> Em Wed, Oct 31, 2018 at 11:10:39AM +0200, Adrian Hunter escreveu:
> > For branch stacks or branch samples, the sample cpumode might not be
> > correct because it applies only to the sample 'ip' and not necessary
> > to 'addr' or branch stack addresses. Add fallback functions that can
> > be used to deal with those cases
> >
> > Signed-off-by: Adrian Hunter <adrian.hun...@intel.com>
> > Cc: sta...@vger.kernel.org # 4.19
> > ---
> > tools/perf/util/event.c | 27 ++++++++++++++++++++++++++
> > tools/perf/util/machine.c | 40
> > +++++++++++++++++++++++++++++++++++++++
> > tools/perf/util/machine.h | 2 ++
> > tools/perf/util/thread.h | 4 ++++
> > 4 files changed, 73 insertions(+)
> >
> > diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c index
> > bc646185f8d9..52b24b8ce29e 100644
> > --- a/tools/perf/util/event.c
> > +++ b/tools/perf/util/event.c
> > @@ -1576,6 +1576,24 @@ struct map *thread__find_map(struct thread
> *thread, u8 cpumode, u64 addr,
> > return al->map;
> > }
> >
> > +/*
> > + * For branch stacks or branch samples, the sample cpumode might not
> > +be correct
> > + * because it applies only to the sample 'ip' and not necessary to
> > +'addr' or
> > + * branch stack addresses. If possible, use a fallback to deal with those
> cases.
> > + */
> > +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.
>
> 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.
>
> > +
> > +u8 machine__addr_cpumode(struct machine *machine, u8 cpumode, u64
> > +addr) {
> > + u8 addr_cpumode = cpumode;
> > + bool kernel_ip;
> > +
> > + if (!machine__single_ku_as(machine))
> > + goto out;
> > +
> > + kernel_ip = machine__kernel_ip(machine, addr);
> > + switch (cpumode) {
> > + case PERF_RECORD_MISC_KERNEL:
> > + case PERF_RECORD_MISC_USER:
> > + addr_cpumode = kernel_ip ? PERF_RECORD_MISC_KERNEL :
> > + PERF_RECORD_MISC_USER;
> > + break;
> > + case PERF_RECORD_MISC_GUEST_KERNEL:
> > + case PERF_RECORD_MISC_GUEST_USER:
> > + addr_cpumode = kernel_ip ?
> PERF_RECORD_MISC_GUEST_KERNEL :
> > + PERF_RECORD_MISC_GUEST_USER;
> > + break;
> > + default:
> > + break;
> > + }
> > +out:
> > + return addr_cpumode;
> > +}
> > +
> > struct dso *machine__findnew_dso(struct machine *machine, const char
> > *filename) {
> > return dsos__findnew(&machine->dsos, filename); diff --git
> > a/tools/perf/util/machine.h b/tools/perf/util/machine.h index
> > d856b85862e2..20d464135ed1 100644
> > --- a/tools/perf/util/machine.h
> > +++ b/tools/perf/util/machine.h
> > @@ -99,6 +99,8 @@ static inline bool machine__kernel_ip(struct machine
> *machine, u64 ip)
> > return ip >= kernel_start;
> > }
> >
> > +u8 machine__addr_cpumode(struct machine *machine, u8 cpumode, u64
> > +addr);
> > +
> > struct thread *machine__find_thread(struct machine *machine, pid_t pid,
> > pid_t tid);
> > struct comm *machine__thread_exec_comm(struct machine *machine,
> diff
> > --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h index
> > 36c09a9904e6..e28f9dc1110a 100644
> > --- a/tools/perf/util/thread.h
> > +++ b/tools/perf/util/thread.h
> > @@ -96,9 +96,13 @@ struct thread *thread__main_thread(struct machine
> > *machine, struct thread *threa
> >
> > struct map *thread__find_map(struct thread *thread, u8 cpumode, u64
> addr,
> > struct addr_location *al);
> > +struct map *thread__find_map_fallback(struct thread *thread, u8
> cpumode,
> > + u64 addr, struct addr_location *al);
> >
> > struct symbol *thread__find_symbol(struct thread *thread, u8 cpumode,
> > u64 addr, struct addr_location *al);
> > +struct symbol *thread__find_symbol_fb(struct thread *thread, u8
> cpumode,
> > + u64 addr, struct addr_location *al);
> >
> > void thread__find_cpumode_addr_location(struct thread *thread, u64
> addr,
> > struct addr_location *al);
> > --
> > 2.17.1
