Re: [PATCH v3 3/4] module: add debug stats to help identify memory pressure

[Luis Chamberlain <mcgrof@kernel.org>]

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