[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Crash-utility] [PATCH 0/3] ikconfig and load module helperpatches.



Hi Dave,

Thanks for your additional reviews.

I have reworked get_kernel_config() with all of your suggestions.
But I have other suggestion about ikconfig data life-time.

I attach two patches of different life-time term.
 patch#1: ikconfig data has life-time which is complied with your suggestion
 patch#2: ikconfig data is permanent (no life-time, whole crash running)

I think there is a trade-off between command response time and crash VM size.

If commands which use get_kernel_config() always have to build ikconfig data,
it spends excrescent times of zlib decompress, setup, free process.
But patch#2 spends them only once in crash-startup.
(Even though there was no stress in my environs with patch#1,
 but my spec is 2GB MEM and 2.4GHz 4-CPUs.)

Also I lightly compared VM size,
there was no prominent VM gaps between them.

# zcat /proc/config.gz | wc -l
1369
ikconfig_ents: 416 (Valid 416 kernel configs)

statm: TOTAL RSS SHARED TEXT LIB DATA

With patch#1
# cat statm
22692 19515 9703 1372 0 10496 0

With patch#2
# cat statm
22695 19512 9694 1372 0 10499 0

Do you think which life-time is better?
(I would like to push patch#2...)

Thanks a lot,
Toshi.

>Hello Toshi,
>
>Here are my comments and further suggestions for this patchset:
>
>0001-new-ikconfig-API.patch
>0002-dump_kernel_table.patch
>
>  In the interest of simplification, let me suggest the following.
>
>  With your current scheme, this is required:
>
>    crash> extend module.so
>    ...
>    the module's _init() function gets called, which does:
>      read_in_kernel_config(IKCFG_SETUP);
>      ret = get_kernel_config("XXX", str);
>      read_in_kernel_config(IKCFG_FREE);  
>    ...
>    crash>
>  
>  And then you have a bunch of stuff for handling multiple extension
>  modules, reference counting, etc.
>
>  But extension modules should not have to worry about having to make
>  the two read_in_kernel_config() calls -- it should simply be a matter
>  of doing this to get a kernel configuration item:
> 
>     ret = get_kernel_config("XXX", str);
>
>  This would typically be called one (or more) times in their _init() 
>  function, but it could also be called in a registered function as well.
>
>  Anyway, let the read_in_kernel_config(IKCFG_SETUP) and (IKCFG_FREE) 
>  mechanisms be handled by the static crash code:
>
>    - read_in_kernel_config(IKCFG_SETUP) should be called by
>      the exported get_kernel_config() function.  (see below)
>
>    - read_in_kernel_config(IKCFG_FREE) should be called by the
>      restore_sanity() function, which gets called just before 
>      the "crash>" prompt for the *next* crash command is displayed.
>      (see below)
>
>  That way, the ikconfig data is only available for the life-time of
>  a particular command, for example, while the crash "extend" command 
>  is running, or perhaps later on when a module's registered command
>  is run.
>
>  That being the case, all of the stuff you have for handling multiple 
>  extension modules can be removed, because that could never happen.
>
>  The kt->ikconfig_setup counter can be removed, and replaced by 
>  a new kt->ikconfig_flags field, that has at least two flags:
>
>    #define IKCONFIG_AVAIL  0x1   /* kernel contains ikconfig data */
>    #define IKCONFIG_LOADED 0x2   /* ikconfig data is currently loaded */
>
>  And the would be set/cleared like so:
>
>  - When the initialization-time read_in_kernel_config(IKCFG_INIT) call is 
>    made, IKCFG_AVAIL would get set if the ikconfig data is available in
>    the kernel.
>
>  - When read_in_kernel_config(IKCFG_SETUP) successfully reads the ikconfig
>    data, then IKCFG_LOADED could be be set (temporarily).  
>
>  - When read_in_kernel_config(IKCFG_FREE) is called, IKCFG_LOADED gets cleared.
>
>  That being the case, get_kernel_config() could have this at the top:
>
>	switch (kt->ikconfig_flags & (IKCFG_LOADED|IKCFG_AVAIL))
>	{
>	case IKCFG_AVAIL:
>                read_in_kernel_config(IKCFG_SETUP);
>		if (!kt->config_flags & IKCFG_LOADED)
>			return IKCONFIG_N;
>		break;
>
>	case (IKCFG_LOADED|IKCFG_AVAIL):  /* already loaded */
>		break;
>
>	default:
>		return IKCFG_N;
>	}
>
>   And restore_sanity() could just do this:
>
>	if (kt->ikconfig_flags & IKCFG_LOADED)
>		read_in_kernel_config(IKCFG_FREE);
>
>  And lastly, I see no need for kt->ikconfig_refs.  You increment
>  it in get_kernel_config() each time it is called, and then set
>  it back to 0 when read_in_kernel_config(IKCFG_FREE) frees everything.
>  But nobody else reads it -- and I don't see why an extension module
>  would ever need to read it?  
>
>0003-load_module_symbols_helper.patch
>
>  Queued for the next release.
>
>Thanks again,
>  Dave
>
>
>----- Original Message -----
>> Declare get_kernel_config(), load_module_symbols_helper()
>> for crash outside extension users.
>> CRASH_MODULE_PATH valiable can apply to search non-standard module path
>> from load_module_symbols_helper() and "mod" command.
>> 
>> - get_kernel_config("config name", &val)
>> Return one of target kernel configurations.
>> If val == NULL, return value is poinited whether config is valid or
>> not.
>> IKCONFIG_N: kernel config is not set (not valid).
>> IKCONFIG_Y: kernel config is set y (valid).
>> IKCONFIG_M: kernel config is set m (valid).
>> IKCONFIG_STR: kernel config is values or strings, etc (valid).
>> 
>> "help -k" can display ikconfig setup state,
>> number of valid ikconfig entries, function calls.
>> 
>> Notes:
>> 1. How to activate get_kernel_config().
>> read_in_kernel_config(IKCFG_SETUP)
>> -> get_kernel_config() become active.
>> read_in_kernel_config(IKCFG_FREE)
>> -> get_kernel_config() become iniactive.
>> 
>> 2. This function require CONFIG_IKCONFIG=y. Otherwise user is warned.
>> 
>> Functional change from previous one.
>> "config name" can be allowed both with or without CONFIG_ prefix strings.
>> [ Dave's recommendation ]
>> 
>> - load_module_symbols_helper("module name")
>> Load specified kernel module symbols with argument.
>> This is simplified usage from original load_module_symbols().
>> 
>> Add "CRASH_MODULE_PATH" valiable which can use to resolve
>> non-standard module path.
>> 
>> Functional change from previous one.
>> "CRASH_MODULE_PATH" could be used by the "mod -s" command as well.
>> [ Dave's recommendation ]
>> Example usage:
>> < in .crashrc >
>> mod -s ext3
>> mod -s jbd
>> :
>> :
>> 
>> export CRASH_MODULE_PATH="your module's root directory"
>> /usr/sbin/crash
>> 
>> Even if target kernel is changed,
>> crash will load appropriate ext3 or jbd modules by this valiable.
>> 
>> Toshikazu Nakayama (3):
>> new ikconfig API.
>> dump_kernel_table()
>> load_module_symbols_helper().
>> 
>> crash-5.1.1/defs.h | 14 +++++
>> crash-5.1.1/kernel.c | 148
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 162 insertions(+), 0 deletions(-)
>> 
>> --
>> 1.7.4.rc2.3.g60a2e
>> 
>> 
>> --
>> Crash-utility mailing list
>> Crash-utility redhat com
>> https://www.redhat.com/mailman/listinfo/crash-utility
>
>--
>Crash-utility mailing list
>Crash-utility redhat com
>https://www.redhat.com/mailman/listinfo/crash-utility

Attachment: 0001-get_kernel_config-life-time-of-a-particular-command.patch
Description: Binary data

Attachment: 0001-get_kernel_config-life-time-of-whole-crash.patch
Description: Binary data


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]