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

[Kirill Smelkov <kirr@landau.phys.spbu.ru>]

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...