Messages in this thread | | | Date | Tue, 18 Apr 2023 11:30:36 -0700 | From | Luis Chamberlain <> | Subject | Re: [PATCH v3 3/4] module: add debug stats to help identify memory pressure |
| |
On Mon, Apr 17, 2023 at 01:18:14PM +0200, Petr Pavlu wrote: > On 4/14/23 07:08, Luis Chamberlain wrote:
<-- Petr's spell checking -->
> Note that there are plenty of other typos in the added comments and > documentation. Please review them with a spell checker.
Yes I am terrible at that, I've now integrated a spell checker into my workflow. Fixed all these, thanks.
> > @@ -2500,6 +2503,18 @@ static noinline int do_init_module(struct module *mod) > > { > > int ret = 0; > > struct mod_initfree *freeinit; > > +#if defined(CONFIG_MODULE_STATS) > > + unsigned int text_size = 0, total_size = 0; > > + > > + for_each_mod_mem_type(type) { > > + const struct module_memory *mod_mem = &mod->mem[type]; > > + if (mod_mem->size) { > > + total_size += mod_mem->size; > > + if (type == MOD_TEXT || type == MOD_INIT_TEXT) > > + text_size += mod->mem[type].size; > > 'text_size += mod_mem->size;' would be simpler.
Sure.
> > +extern struct dentry *mod_debugfs_root; > > Files kernel/module/stats.c and kernel/module/tracking.c both add this extern > declaration. Can it be moved to kernel/module/internal.h?
Sure.
> > +#if defined(CONFIG_MODULE_DECOMPRESS) > > + if (flags & MODULE_INIT_COMPRESSED_FILE) > > + atomic_long_add(info->compressed_len, &invalid_mod_byte); > > Variable invalid_mod_byte is not declared, should be invalid_mod_bytes.
Arnd already sent a fix for that, thanks.
> > +int try_add_failed_module(const char *name, size_t len, enum fail_dup_mod_reason reason) > > Function try_add_failed_module() is only called from > module_patient_check_exists() which always passes in a NUL-terminated string. > The len parameter could be then dropped and the comparison in > try_add_failed_module() could simply use strcmp().
Sure, did that.
> Indentation in try_add_failed_module() uses spaces instead of tabs in a few > places.
Fixed.
> > + size = MAX_PREAMBLE + min((unsigned int)(floads + fbecoming) * MAX_BYTES_PER_MOD, > > + (unsigned int) MAX_FAILED_MOD_PRINT * MAX_BYTES_PER_MOD); > > Using > 'size = MAX_PREAMBLE + min((unsigned int)(floads + fbecoming), (unsigned int)MAX_FAILED_MOD_PRINT) * MAX_BYTES_PER_MOD;' > is a bit simpler and avoids any theoretical overflow of > '(floads + fbecoming) * MAX_BYTES_PER_MOD'.
Sure.
> > + len += scnprintf(buf + len, size - len, "%25s\t%15s\t%25s\n", > > + "module-name", "How-many-times", "Reason"); > > "module-name" -> "Module-name"
OK sure.
> Function module_stats_init() requires mod_debugfs_root being initialized which > is done in module_debugfs_init(). Both functions are recorded to be called via > module_init(). Just to make sure, is their ordering guaranteed in some way?
Link order takes care of that and main.o goes first.
> mod_debugfs_root is initialized in module_debugfs_init() only if > CONFIG_MODULE_DEBUG is set. However, my reading is that feature > CONFIG_MODULE_UNLOAD_TAINT_TRACKING is orthogonal to it and doesn't require > CONFIG_MODULE_DEBUG, so it looks this change breaks this tracking?
Ah yes We need a bool CONFIG_MODULE_DEBUGFS which is selected by those that need it. Added.
Luis
| |