Message-ID: <CAJfuBxz9LseSsTccsVnu4BB6cbve97PmnxjP8jt33WhRqUUgQA@mail.gmail.com>
Date: Fri, 23 Jan 2026 16:24:16 -0700
From: jim.cromie@...il.com
To: "Christophe Leroy (CS GROUP)" <chleroy@...nel.org>
Cc: linux-kernel@...r.kernel.org, Luis Chamberlain <mcgrof@...nel.org>, 
	Petr Pavlu <petr.pavlu@...e.com>, Daniel Gomez <da.gomez@...nel.org>, 
	Sami Tolvanen <samitolvanen@...gle.com>, Aaron Tomlin <atomlin@...mlin.com>, 
	linux-modules@...r.kernel.org
Subject: Re: [RFC PATCH 1/1] module: speed modprobe by adding name_crc to
 struct module

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 <jim.cromie@...il.com> wrote:
>
> On Fri, Jan 23, 2026 at 4:39 AM Christophe Leroy (CS GROUP)
> <chleroy@...nel.org> wrote:
> >
> >
> >
> > Le 23/01/2026 à 00:46, Jim Cromie a écrit :
> > > [Vous ne recevez pas souvent de courriers de jim.cromie@...il.com. 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 <mcgrof@...nel.org>
> > > CC: Petr Pavlu <petr.pavlu@...e.com>
> > > CC: Daniel Gomez <da.gomez@...nel.org>
> > > CC: Sami Tolvanen <samitolvanen@...gle.com>
> > > CC: Aaron Tomlin <atomlin@...mlin.com>
> > > CC: linux-modules@...r.kernel.org
> > >
> > > Signed-off-by: Jim Cromie <jim.cromie@...il.com>
> > >
> > >   '#' 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
> > >
> > >
> >

Powered by blists - more mailing lists