Re: [PATCH] perf probe: Fix module probing with shortname

['Arnaldo Carvalho de Melo']

Em Thu, Sep 24, 2015 at 10:10:31AM +0000, åæéå / HIRAMATUïMASAMI escreveu:
> 
> 
> -- 
> Masami HIRAMATSU
> Linux Technology Research Center, System Productivity Research Dept.
> Center for Technology Innovation - Systems Engineering
> Hitachi, Ltd., Research & Development Group
> E-mail: masami.hiramatsu.pt@xxxxxxxxxxx
> 
> 
> >-----Original Message-----
> >From: Arnaldo Carvalho de Melo [mailto:acme@xxxxxxxxxx]
> >Sent: Thursday, September 24, 2015 10:50 AM
> >To: Wangnan (F)
> >Cc: linux-kernel@xxxxxxxxxxxxxxx; lizefan@xxxxxxxxxx; pi3orama@xxxxxxx; Namhyung Kim; Jiri Olsa; åæéå / HIRAMATUïMASAMI
> >Subject: Re: [PATCH] perf probe: Fix module probing with shortname
> >
> >Em Wed, Sep 23, 2015 at 01:03:19PM -0300, Arnaldo Carvalho de Melo escreveu:
> >> Em Tue, Sep 22, 2015 at 11:49:02PM -0300, Arnaldo Carvalho de Melo escreveu:
> >> > Em Wed, Sep 23, 2015 at 09:14:44AM +0800, Wangnan (F) escreveu:
> >> > > On 2015/9/22 21:35, Arnaldo Carvalho de Melo wrote:
> >> > > >Em Tue, Sep 22, 2015 at 03:34:32AM +0000, Wang Nan escreveu:
> >> > > >>This is caused by a misunderstood of dso->kernel in kernel_get_module_dso()
> >> > > >>that, for kernel module, dso->kernel is DSO_TYPE_USER. dso->kernel is DSO_TYPE_KERNEL
> >> > > >>iff dso is vmlinux.
> >> > > >Kernel modules having DSO_TYPE_USER seems to be the bug, no? I'll try to
> >> > > >check that...
> >
> >> > > I also noticed this problem when I working on commit
> >> > > 1f121b03d058dd07199d8924373d3c52a207f63b ("perf tools: Deal with
> >> > > kernel module names in '[]' correctly") ;)
> >
> >> > Thanks for working on this, it is an area that needs cleaning up, too
> >> > many ways to say what a dso is, will study your findings and try to come
> >> > up with a patch proposal tomorrow.
> >> > > So care must be taken.
> >>
> >> So, yes, there are multiple uses for this dso->kernel thing, we need to
> >> look at each one and go on clarifying it so that this gets corrected and
> >> sane, but I think we need some helpers to clarify all this, namely:
> >>
> >> 	Adding DSO_TYPE_KMODULE and DSO_TYPE_GUEST_KMODULE, setting
> >> dso->kernel with it when loading host and guest kernel modules and
> >> adding:
> >>
> >> 	bool dso__is_host_kernel(struct dso *dso)
> >> 	bool dso__is_host_kmodule(struct dso *dso)
> >>
> >> 	bool dso__is_guest_kernel(struct dso *dso)
> >
> >So I tried this, ended up basically using __map__is_kernel(), in several
> >places, which I think is right, and I've stashed in my tmp.perf/core
> >branch so that you can take a look.
> >
> >But then I realized that we already have a better way to achieve what is
> >needed in that function, see the patch below, fixed things for me:
> 
> Hmm, this is a bit hacky, but yes, it looks good to me.

You mean the [] dso->short_name part? But that was already being taken
into account by the previous, dso list search.

But yeah, probably we should remove those brackets, anything requiring
full disambiguation should use the dso->long_name anyway,  but I'll
leave this for later :)

> However, I hope to have above solution, for future use.

Right, my work now is to reduce the use of dso->kernel, using
__map__is_kernel(map), we'll see what is left.
 
> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>

Thanks!
 
> Thanks!
> 
> >
> >diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> >index 2b78e8f19b45..7fb0533ab18c 100644
> >--- a/tools/perf/util/probe-event.c
> >+++ b/tools/perf/util/probe-event.c
> >@@ -269,12 +269,13 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
> > 	int ret = 0;
> >
> > 	if (module) {
> >-		list_for_each_entry(dso, &host_machine->dsos.head, node) {
> >-			if (!dso->kernel)
> >-				continue;
> >-			if (strncmp(dso->short_name + 1, module,
> >-				    dso->short_name_len - 2) == 0)
> >-				goto found;
> >+		char module_name[128];
> >+
> >+		snprintf(module_name, sizeof(module_name), "[%s]", module);
> >+		map = map_groups__find_by_name(&host_machine->kmaps, MAP__FUNCTION, module_name);
> >+		if (map) {
> >+			dso = map->dso;
> >+			goto found;
> > 		}
> > 		pr_debug("Failed to find module %s.\n", module);
> > 		return -ENOENT;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/