[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.




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


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