Re: [PATCH v2 2/8] makedumpfile: Apply relocation while loading module debuginfo.

[Mahesh Jagannath Salgaonkar <mahesh@linux.vnet.ibm.com>]

Hi Keni'chi,

On 07/28/2011 02:41 PM, Ken'ichi Ohmichi wrote:
> 
> Hi Mahesh,
> 
> This feature's patches are very large, and I created a devel branch
> on sourceforge git tree for sharing the code with you.
> 
> http://makedumpfile.git.sourceforge.net/git/gitweb.cgi?p=makedumpfile/makedumpfile;a=shortlog;h=refs/heads/filter-out-devel
> 
> I commited all of your patches and my cleanup patches.
> If you notice some problems or cleate some patches, please let me know them.
> 

Sure. Will take a look.

> On Wed, 27 Jul 2011 11:39:44 +0530
> Mahesh Jagannath Salgaonkar <mahesh@linux.vnet.ibm.com> wrote:
>>>> + *
>>>> + * This function uses dwfl API's to apply relocation before reading the
>>>> + * dwarf information from module debuginfo.
>>>> + * On success, this function sets the dwarf_info.elfd and dwarf_info.dwarfd
>>>> + * after applying relocation to module debuginfo.
>>>> + */
>>>> +static int
>>>> +init_dwarf_info(void)
>>>
>>> Is the searching method of debuginfo file only for kernel module ?
>>> Or also for vmlinux ?
>>
>> Yes, the searching method is only for module debuginfo. For vmlinux we
>> set the dwarf_info.name_debuginfo before call to init_dwarf_info() and
>> we never call search routine dwfl_linux_kernel_report_offline() for
>> vmlinux. The routine dwfl_report_offline() applies the relocation on
>> specified debuginfo module. In case vmlinux it basically does nothing.
>>
>>> I feel this function is a little complex, and I'd like to make it simple.
>>> If only for kernel module, we can do it by separating the searching method
>>> from this function and calling it in case of kernel module.
>>
>> The init_dwarf_info() function gets called everytime when
>> get_debug_info() is called. The get_debug_info() is called multiple
>> times for same debuginfo file. This function tries to avoid the repeated
>> search for same debuginfo, hence the function is little complex.
>> 	- In case of kernel module the very first invocation of
>> init_dwarf_info() would call dwfl_linux_kernel_report_offline() which
>> will iterate over the available kernel modules and process the debuginfo
>> only for the module for which we are interested in.
>> 	- We set the dwarf_info.name_debuginfo with the debuginfo file name
>> found during the first invocation.
>> 	- The second invocation of init_dwarf_info() for same kernel module,
>> will find dwarf_info.name_debuginfo is already set and will avoid the
>> debuginfo search. In this case it will just apply relocation using
>> routine dwfl_report_offline().
>>
>> This function does not have any special handling code for vmlinux. The
>> function is independent of whether it is called for vmlinux or kernel
>> module. In case of vmlinux this function has absolutely no side effects.
> 
> If adding the searching method to the blow position and removing the code
> from init_dwarf_info(), I guess it makes the code simple.
> 
> @ process_config_file()
>  9402                         if (!set_dwarf_debuginfo(config->module_name,
>  9403                                                                 NULL, -1)) {
>  9404                                 ERRMSG("Skipping to next Module section\n");
>  9405                                 skip_section = 1;
>  9406                                 free_config(config);
>  9407                                 continue;
>  9408                         }
>  9409                         << HERE >> 

This may not be the correct place to call search method. We may end up
calling search method multiple times for same kernel module. I think
moving the search method inside set_dwarf_debuginfo() routine at below
position is a better place:

@set_dwarf_debuginfo()
	......
	......
	if (!strcmp(dwarf_info.module_name, "vmlinux") ||
                !strcmp(dwarf_info.module_name, "xen-syms"))
                return TRUE;
+	 << HERE >>
-        /* check to see whether module debuginfo is available */
-        if (!init_dwarf_info())
-                return FALSE;
-        else
-                clean_dwfl_info();
	 return TRUE;
}

And then we can remove search routine from init_dwarf_info(). What do
you think?

Thanks,
-Mahesh.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec