Re: [PATCH 1/6] perf top: teach it to autolocate vmlinux

Kirill Smelkov
2010-01-17T16:59:42Z

Arnaldo, All,

Thanks for replying, and I'm sorry for the delay in sending my reply
back. Please find my not so thoughtful reply below:

On Wed, Jan 13, 2010 at 11:39:52AM -0200, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jan 08, 2010 at 03:23:04PM +0300, Kirill Smelkov escreveu:
> > By relying on logic in dso__load_kernel_sym(), we can automatically load
> > vmlinux.
> >
> > The only thing which needs to be adjusted, is how --sym-annotate option
> > is handled - now we can't rely on vmlinux been loaded until full
> > successful pass of dso__load_vmlinux(), but that's not the case if we'll
> > do sym_filter_entry setup in symbol_filter().
> >
> > So move this step right after event__process_sample() where we know the
> > whole dso__load_kernel_sym() pass is done.
> >
> > By the way, though conceptually similar `perf top` still can't annotate
> > userspace - see next patches with fixes.
> >
> > Signed-off-by: Kirill Smelkov <kirr@landau.phys.spbu.ru>
> > Cc: Mike Galbraith <efault@gmx.de>
> > ---
>
> <SNIP>
>
> > @@ -951,6 +953,13 @@ static void event__process_sample(const event_t *self,
> > al.sym == NULL || al.filtered)
> > return;
> >
> > + /* let's see, whether we need to install initial sym_filter_entry */
> > + if (sym_filter_entry_sched) {
> > + sym_filter_entry = sym_filter_entry_sched;
> > + sym_filter_entry_sched = NULL;
> > + parse_source(sym_filter_entry);
> > + }
> > +
>
> You're assuming that the first sample is for the kernel, right? It may

Not quite so.

> be not and then the vmlinux won't be loaded at this point.

I agree, that there is an ambiguity, that e.g. for 'strstr' symbol there
are variants of which strstr to annotate - the kernel one, or the glibc
one (or even some other debug version of strstr preloaded by user
through LD_PRELOAD).

We'll get here on the first sample which hits function with name equal
to sym_filter. Sometimes this will be from vmlinux, sometimes not (but
if a symbol is only from vmlinux and produces sample hits, we'll get
here eventually for sure).

So yes, there is an ambiguity from which DSO we want sym_filter.

>
> I think that the right way is to force it to be loaded by calling:
>
> map__load(session->vmlinux_maps[MAP__FUNCTION], session, filter);
>
> after perf_session__create_kernel_maps and before parse_source(), ok?
>
> You can even create a helper:
>
> int perf_session__load_vmlinux(struct perf_session *self,
> symbol_filter_t filter)
> {
> return map__load(session->vmlinux_maps[MAP__FUNCTION],
> session, filter);
> }
>
> As this probably will be of interest for tools such as 'perf
> probe', etc.

I see your point.

Yes, you kernel people are almost always interested in kernel profile in
the first place :), but won't this be an ad-hock solution? I mean why
kernel (and only) kernel first?

In case of ambiguity, I'd better let users specify something like
vmlinux:strstr or libc.so.6:strstr to precisely define info for which
symbols they are going to see.

Anyway, as I see it, this days perf is used for kernel development
mostly, so I'd agree with ad-hoc kernel rule for now.

The problem is my spare time is very limited this month - I have only
few hours through weekends and this weekend I've already spent them all
:(. Sorry, maybe next week ...

Kirill

P.S. how about patches 4/6 and 5/6? They fix `perf annotate`
(independently for this vmlinux loading thing) and `perf top->annotate`
fix is somewhat orthogonal to the patch we are discussing...