Messages in this thread | | | Date | Sun, 17 Jan 2010 19:59:36 +0300 | From | Kirill Smelkov <> | Subject | Re: [PATCH 1/6] perf top: teach it to autolocate vmlinux |
| |
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...
| |