Re: [RFC] perf probe: Fix offline module name missmatch issue

[Ravi Bangoria]

On Tuesday 26 April 2016 04:16 PM, Masami Hiramatsu wrote:
> On Tue, 26 Apr 2016 14:26:48 +0530
> Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxxxxxxx> 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.bangoria@xxxxxxxxxxxxxxxxxx> 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 :)  Sent patch with changes. Please review it.

Regards,
Ravi