On Tue, 26 Apr 2016 14:26:48 +0530
Ravi Bangoria <ravi.bango...@linux.vnet.ibm.com> wrote:
> Thanks Masami for reviewing.
>
> Please find my replies to your comment.
>
> On Tuesday 26 April 2016 02:54 AM, Masami Hiramatsu wrote:
> > Hi Ravi,
> >
> > On Mon, 25 Apr 2016 16:08:27 +0530
> > Ravi Bangoria <ravi.bango...@linux.vnet.ibm.com> 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?
>
> Yes. no caller outside of this file. But,
>
> In this patch, function find_module_name is defined outside of
> #ifdef HAVE_DWARF_SUPPORT while it's being called from inside of
> #ifdef HAVE_DWARF_SUPPORT.
>
> If I make it static and if there is no dwarf support, there will be
> compilation
> error about function defined but not used.
>
> And in second patch("perf probe: Fix module probe issue if no dwarf
> support"),
> I'm calling it from outside of #ifdef HAVE_DWARF_SUPPORT.
>
> So I have two options:
> 1. merge both the patches and make definition as static
> 2. make function static in second patch
>
> I've chose second approach and sent v2. But please let me know if there is
> better way to do it.
Ah, I see.
In that case, you can swap the patch in the series and move find_module_name
in the other patch ;)
Thanks!
--
Masami Hiramatsu <mhira...@kernel.org>
