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(a)redhat.com >> https://www.redhat.com/mailman/listinfo/crash-utility > >-- >Crash-utility mailing list &gt;Crash-utility(a)redhat.com >https://www.redhat.com/mailman/listinfo/crash-utility -- Crash-utility mailing list Crash-utility(a)redhat.com https://www.redhat.com/mailman/listinfo/crash-utility