Date: Tue, 26 Apr 2016 19:46:02 +0900
From: Masami Hiramatsu <mhiramat@kernel.org>
To: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
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: <20160426194602.c0aba9f906674a963ee9f35d@kernel.org>
In-Reply-To: <571F2D50.4010305@linux.vnet.ibm.com>
References: <1461580708-15169-1-git-send-email-ravi.bangoria@linux.vnet.ibm.com>
	<20160426062438.aea663884cf5bd95f766f2d5@kernel.org>
	<571F2D50.4010305@linux.vnet.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2916
Lines: 82

On Tue, 26 Apr 2016 14:26:48 +0530
Ravi Bangoria <ravi.bangoria@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.bangoria@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 <mhiramat@kernel.org>