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@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/crash-utility
>
>--
>Crash-utility mailing list
>Crash-utility@xxxxxxxxxx
>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
-- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/crash-utility
