----- Original Message -----
> 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...)
Definitely #1.
The access of the ikconfig data is really nothing much more
than a typical memory read, although it does have to be
uncompressed.
But, think about it, when running against a compressed
diskump or compressed kdump dumpfile, every memory access
has to be uncompressed.
Let me review/test your patch #1, and I'll get back to you
with my results.
Thanks,
Dave
> 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
>
> --
> 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
