Apologies, I hit send too soon, and missed some of your points. name_hash vs name_crc was failure to recheck my names for consistency.
On Fri, Jan 23, 2026 at 4:10 PM <[email protected]> wrote: > > On Fri, Jan 23, 2026 at 4:39 AM Christophe Leroy (CS GROUP) > <[email protected]> wrote: > > > > > > > > Le 23/01/2026 à 00:46, Jim Cromie a écrit : > > > [Vous ne recevez pas souvent de courriers de [email protected]. > > > Découvrez pourquoi ceci est important à > > > https://aka.ms/LearnAboutSenderIdentification ] > > > > > > "modprobe foo" currently does strcmp on the name, this can be improved. > > > > > > So this commit: > > > > > > 1. adds name_crc to struct module > > > 2. modpost.c computes the value and > > > 3. outputs it for "modinfo foo" to see/use. > > > > > > 4. adds hotpath to find_module_all() > > > this uses name_crc to do quick "name-check" > > > falls back to strcmp only to guard against collisions. > > > > > > This should significantly reduce modprobe workload, and shorten module > > > load-time. > > > > Any numbers of how significant is the reduction ? > > Not at this time. > In absolute terms, my box is running fedora, has 165 modules, > so the significance is minor in most circumstances. > In relative terms, a numerical equality test is much faster than strcmp. > > I recall seeing Luis doing quite a bit of work tuning module loading, > I was hoping he could shed light / opine / provide numbers > on the utility of this. > > > > > > > > > Since it alters struct module, its binary incompatible. This means: > > > > > > 1. RFC for its wide "blast radius". > > > 2. suitable for major version bump *only* > > > > > > 3. it opens door for further struct module reorg, to: > > > a. segregate fields by "temperature" > > > b. pack out paholes. > > > c. improve cache locality (by reordering coldest on bottom) > > > name should be cold now. > > > bikeshedding is appropriate here. > > > > > > NB: this isn't a substitute for CONFIG_MODULE_SIG. > > > It reimplements crc_le(), doesn't reuse kernel's version. > > > > Why not use the kernel's version ? > > I briefly fiddled with doing so, but sorting the includes looked like a > hassle, > so I punted and asked the AI for an implementation. > > > > > > > > > > CC: Luis Chamberlain <[email protected]> > > > CC: Petr Pavlu <[email protected]> > > > CC: Daniel Gomez <[email protected]> > > > CC: Sami Tolvanen <[email protected]> > > > CC: Aaron Tomlin <[email protected]> > > > CC: [email protected] > > > > > > Signed-off-by: Jim Cromie <[email protected]> > > > > > > '#' will be ignored, and an empty message aborts the commit. > > > --- > > > include/linux/module.h | 15 ++++++++------- > > > kernel/module/main.c | 8 ++++++-- > > > scripts/mod/modpost.c | 18 ++++++++++++++++++ > > > scripts/mod/modpost.h | 6 +++++- > > > 4 files changed, 37 insertions(+), 10 deletions(-) > > > > > > diff --git a/include/linux/module.h b/include/linux/module.h > > > index d80c3ea57472..4ea6c5ae3374 100644 > > > --- a/include/linux/module.h > > > +++ b/include/linux/module.h > > > @@ -402,10 +402,18 @@ struct klp_modinfo { > > > > > > struct module { > > > enum module_state state; > > > + u32 name_hash; > > > > In the subject you say "name_crc" > > > > > > > > /* Member of list of modules */ > > > struct list_head list; > > > > > > + /* Sysfs stuff. */ > > > + struct module_kobject mkobj; > > > + struct module_attribute *modinfo_attrs; > > > + const char *version; > > > + const char *srcversion; > > > + struct kobject *holders_dir; > > > + > > > > Shouldn't this move be another patch ? > > > > > /* Unique handle for this module */ > > > char name[MODULE_NAME_LEN]; > > > > > > @@ -414,13 +422,6 @@ struct module { > > > unsigned char build_id[BUILD_ID_SIZE_MAX]; > > > #endif > > > > > > - /* Sysfs stuff. */ > > > - struct module_kobject mkobj; > > > - struct module_attribute *modinfo_attrs; > > > - const char *version; > > > - const char *srcversion; > > > - struct kobject *holders_dir; > > > - > > > /* Exported symbols */ > > > const struct kernel_symbol *syms; > > > const u32 *crcs; > > > diff --git a/kernel/module/main.c b/kernel/module/main.c > > > index d855f43a2be3..685218b2c5ef 100644 > > > --- a/kernel/module/main.c > > > +++ b/kernel/module/main.c > > > @@ -39,6 +39,7 @@ > > > #include <linux/mutex.h> > > > #include <linux/rculist.h> > > > #include <linux/uaccess.h> > > > +#include <linux/crc32.h> > > > #include <asm/cacheflush.h> > > > #include <linux/set_memory.h> > > > #include <asm/mmu_context.h> > > > @@ -431,13 +432,16 @@ struct module *find_module_all(const char *name, > > > size_t len, > > > bool even_unformed) > > > { > > > struct module *mod; > > > + u32 incoming_name_hash = crc32_le(0, name, len); > > > > > > list_for_each_entry_rcu(mod, &modules, list, > > > lockdep_is_held(&module_mutex)) { > > > if (!even_unformed && mod->state == > > > MODULE_STATE_UNFORMED) > > > continue; > > > - if (strlen(mod->name) == len && !memcmp(mod->name, name, > > > len)) > > > - return mod; > > > + if (mod->name_hash == incoming_name_hash) { > > > + if (strlen(mod->name) == len && > > > !memcmp(mod->name, name, len)) > > > + return mod; > > > + } > > > > Why not just adding the following instead of modifing existing test: thats an alternative. I modified the existing test cuz its qualitatively the same test, except for collisions. > > > > if (mod->name_hash != incoming_name_hash) > > continue; > > > > > } > > > return NULL; > > > } > > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > > > index 755b842f1f9b..ae90e0bf9330 100644 > > > --- a/scripts/mod/modpost.c > > > +++ b/scripts/mod/modpost.c > > > @@ -21,6 +21,22 @@ > > > #include <stdbool.h> > > > #include <errno.h> > > > > > > +/* Local CRC32 implementation for modpost.c */ > > > +#define CRCPOLY_LE 0xEDB88320 > > > + > > > +typedef uint32_t u32; > > > + > > > +static u32 crc32_le(u32 crc, char *p, size_t len) > > > +{ > > > + int i; > > > + while (len--) { > > > + crc ^= *p++; > > > + for (i = 0; i < 8; i++) > > > + crc = (crc >> 1) ^ ((crc & 1) ? CRCPOLY_LE : 0); > > > + } > > > + return crc; > > > +} > > > + > > > > Why do you re-implement crc32_le() ? I didnt want to wrestle with the headers. If theres utility here, I can do so. > > > > > #include <hash.h> > > > #include <hashtable.h> > > > #include <list.h> > > > @@ -1581,6 +1597,7 @@ static void read_symbols(const char *modname) > > > > > > /* strip trailing .o */ > > > mod = new_module(modname, strlen(modname) - strlen(".o")); > > > + mod->name_hash = crc32_le(0, mod->name, strlen(mod->name)); > > > > > > /* save .no_trim_symbol section for later use */ > > > if (info.no_trim_symbol_len) { > > > @@ -1834,6 +1851,7 @@ static void add_header(struct buffer *b, struct > > > module *mod) > > > buf_printf(b, "#include <linux/compiler.h>\n"); > > > buf_printf(b, "\n"); > > > buf_printf(b, "MODULE_INFO(name, KBUILD_MODNAME);\n"); > > > + buf_printf(b, "MODULE_INFO(name_crc, \"0x%08x\");\n", > > > mod->name_hash); > > > buf_printf(b, "\n"); > > > buf_printf(b, "__visible struct module __this_module\n"); > > > buf_printf(b, "__section(\".gnu.linkonce.this_module\") = {\n"); > > > diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h > > > index 2aecb8f25c87..3fc3cfd0a039 100644 > > > --- a/scripts/mod/modpost.h > > > +++ b/scripts/mod/modpost.h > > > @@ -11,11 +11,14 @@ > > > #include <fcntl.h> > > > #include <unistd.h> > > > #include <elf.h> > > > +#include <stdint.h> > > > #include "../../include/linux/module_symbol.h" > > > > > > #include <list_types.h> > > > #include "elfconfig.h" > > > > > > +typedef uint32_t u32; > > > + > > > /* On BSD-alike OSes elf.h defines these according to host's word size > > > */ > > > #undef ELF_ST_BIND > > > #undef ELF_ST_TYPE > > > @@ -126,7 +129,8 @@ struct module { > > > bool seen; > > > bool has_init; > > > bool has_cleanup; > > > - char srcversion[25]; > > > + char srcversion[25]; > > > + u32 name_hash; > > > // Missing namespace dependencies > > > struct list_head missing_namespaces; > > > // Actual imported namespaces > > > -- > > > 2.52.0 > > > > > > > >
