Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965078AbcDYVYq (ORCPT ); Mon, 25 Apr 2016 17:24:46 -0400 Received: from mail.kernel.org ([198.145.29.136]:41080 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964906AbcDYVYo (ORCPT ); Mon, 25 Apr 2016 17:24:44 -0400 Date: Tue, 26 Apr 2016 06:24:38 +0900 From: Masami Hiramatsu To: Ravi Bangoria Cc: linux-kernel@vger.kernel.org, acme@kernel.org, peterz@infradead.org, mingo@redhat.com, alexander.shishkin@linux.intel.com, masami.hiramatsu.pt@hitachi.com, wangnan0@huawei.com, namhyung@kernel.org, srikar@linux.vnet.ibm.com, naveen.n.rao@linux.vnet.ibm.com Subject: Re: [RFC] perf probe: Fix offline module name missmatch issue Message-Id: <20160426062438.aea663884cf5bd95f766f2d5@kernel.org> In-Reply-To: <1461580708-15169-1-git-send-email-ravi.bangoria@linux.vnet.ibm.com> References: <1461580708-15169-1-git-send-email-ravi.bangoria@linux.vnet.ibm.com> X-Mailer: Sylpheed 3.4.3 (GTK+ 2.24.28; x86_64-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3960 Lines: 152 Hi Ravi, On Mon, 25 Apr 2016 16:08:27 +0530 Ravi Bangoria wrote: > Perf can add a probe on kernel module which has not been loaded yet. > Current implementation finds module name from path. But if filename > is different from actual module name then perf fails to register > probe while loading module because of mismatch in names. For example, > samples/kobject/kobject-example.ko is loaded as kobject_example. Ah! right, good catch! Have some comment below; > > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c > index 8319fbb..05d0905 100644 > --- a/tools/perf/util/probe-event.c > +++ b/tools/perf/util/probe-event.c > @@ -265,6 +265,65 @@ static bool kprobe_warn_out_range(const char *symbol, unsigned long address) > return true; > } > > +/* > + * NOTE: > + * '.gnu.linkonce.this_module' section of kernel module elf directly > + * maps to 'struct module' from linux/module.h. This section contains > + * actual module name which will be used by kernel after loading it. > + * But, we cannot use 'struct module' here since linux/module.h is not > + * exposed to user-space. Offset of 'name' has remained same from long > + * time, so hardcoding it here. > + */ > +#ifdef __LP64__ > +#define MOD_NAME_OFFSET 24 > +#else > +#define MOD_NAME_OFFSET 12 > +#endif > + > +/* > + * @module can be module name of module file path. In case of path, > + * inspect elf and find out what is actual module name. > + * Caller has to free mod_name after using it. > + */ > +char *find_module_name(const char *module) Could you make this function static, since there is no caller outside this file? > +{ > + int fd; > + Elf *elf; > + GElf_Ehdr ehdr; > + GElf_Shdr shdr; > + Elf_Data *data; > + Elf_Scn *sec; > + char *mod_name = NULL; > + > + fd = open(module, O_RDONLY); > + if (fd < 0) > + return NULL; > + > + elf = elf_begin(fd, PERF_ELF_C_READ_MMAP, NULL); > + if (elf == NULL) > + goto elf_err; > + > + if (gelf_getehdr(elf, &ehdr) == NULL) > + goto ret_err; > + > + sec = elf_section_by_name(elf, &ehdr, &shdr, > + ".gnu.linkonce.this_module", NULL); > + if (!sec) > + goto ret_err; > + > + data = elf_getdata(sec, NULL); > + if (!data || !data->d_buf) > + goto ret_err; > + > + mod_name = strdup((char *)data->d_buf + MOD_NAME_OFFSET); > + > +ret_err: > + elf_end(elf); > +elf_err: > + close(fd); > + return mod_name; > +} > + > #ifdef HAVE_DWARF_SUPPORT > > static int kernel_get_module_dso(const char *module, struct dso **pdso) > @@ -583,32 +642,23 @@ static int add_module_to_probe_trace_events(struct probe_trace_event *tevs, > int ntevs, const char *module) > { > int i, ret = 0; > - char *tmp; > + char *mod_name; > > if (!module) > return 0; > > - tmp = strrchr(module, '/'); > - if (tmp) { > - /* This is a module path -- get the module name */ > - module = strdup(tmp + 1); > - if (!module) > - return -ENOMEM; > - tmp = strchr(module, '.'); > - if (tmp) > - *tmp = '\0'; > - tmp = (char *)module; /* For free() */ > - } > + mod_name = find_module_name(module); > > for (i = 0; i < ntevs; i++) { > - tevs[i].point.module = strdup(module); > + tevs[i].point.module = > + strdup(mod_name ? mod_name : module); > if (!tevs[i].point.module) { > ret = -ENOMEM; > break; > } > } > > - free(tmp); > + free(mod_name); > return ret; > } > > diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h > index e54e7b0..0468fa3 100644 > --- a/tools/perf/util/probe-event.h > +++ b/tools/perf/util/probe-event.h > @@ -166,4 +166,6 @@ int e_snprintf(char *str, size_t size, const char *format, ...) > int copy_to_probe_trace_arg(struct probe_trace_arg *tvar, > struct perf_probe_arg *pvar); > > +char *find_module_name(const char *module); And remove this. Others looks good to me! Thank you, > + > #endif /*_PROBE_EVENT_H */ > -- > 2.1.4 > -- Masami Hiramatsu