tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Enabling built-in modules earlier in init



Attached is the latest set of diffs to address this issue. To summarize the changes:

1. Add an allocator for the 'struct module *' and use it instead of local allocations. The source of the module (builtin, boot, or filesys) is passed as an argument to the allocator.

2. Add a new member mod_flags in the 'struct module *'. The only flag bit defined at this time is MODFLG_MUST_FORCE. If this flag is set, and the entry is on the list of builtins, it means that the module has been explicitly unloaded and any re-loads require MODCTL_LOAD_FORCE flag.

3. Provide a module_require_force() routine to set the above flag. Once set, the flag should never be unset.

4. Rename original module_init2() to module_start_unload_thread() to be more descriptive of what it does.

5. Add a new module_builtin_require_force() to set the MODFLG_MUST_FORCE flag for any module that still has not been successfully initialized, and call this near the end of system initialization.


All of this was triggered by the discovery that, with the exception of secmodel modules, no modules were initialized early enough in the startup sequence to be used during autoconfiguration. (While modules can be explicitly loaded at this time, they cannot be autoloaded and thus would not be eligible for auto-unloading.) Therefore, the recent module-ification (by myself) of the various xxxVERBOSE options broke those options. This breakage remains in-tree at this time. (And yes, it is my fault for not completely testing the changes.)


Multiple earlier versions of this change have been reviewed on this list, and I think I've addressed all of the comments, except for one. I don't think we've come to a final resolution on whether the new routine in item #5 should be a separate routine, or if it should be included in module_class_init() (and executed only when class == MODULE_CLASS_ANY). My personal preference is to maintain a separate routine rather than overloading module_class_init(), but pooka@ suggested that the atomicity resulting from combining/overloading might be useful.


Since all of the 'options xxxVERBOSE' are currently broken, I'd like to get some consensus on these changes, especially on which path to take in resolving the above issue, so I get this committed and undo my breakage.

:)



-------------------------------------------------------------------------
| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:       |
| Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com    |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net |
| Kernel Developer |                          | pgoyette at netbsd.org  |
-------------------------------------------------------------------------
Index: sys/module.h
===================================================================
RCS file: /cvsroot/src/sys/sys/module.h,v
retrieving revision 1.23
diff -u -p -r1.23 module.h
--- sys/module.h        24 May 2010 03:50:25 -0000      1.23
+++ sys/module.h        18 Jun 2010 11:37:27 -0000
@@ -90,6 +90,9 @@ typedef struct module {
        time_t                  mod_autotime;
        void                    *mod_ctf;
        u_int                   mod_fbtentries; /* DTrace FBT entrie count */
+       int                     mod_flags;
+#define MODFLG_MUST_FORCE      0x01
+
 } module_t;
 
 /*
@@ -120,7 +123,8 @@ extern struct modlist       module_builtins;
 extern u_int           module_gen;
 
 void   module_init(void);
-void   module_init2(void);
+void   module_start_unload_thread(void);
+void   module_builtin_require_force(void);
 void   module_init_md(void);
 void   module_init_class(modclass_t);
 int    module_prime(void *, size_t);
Index: kern/kern_module.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_module.c,v
retrieving revision 1.69
diff -u -p -r1.69 kern_module.c
--- kern/kern_module.c  26 May 2010 23:53:21 -0000      1.69
+++ kern/kern_module.c  18 Jun 2010 11:37:27 -0000
@@ -87,9 +87,11 @@ static kauth_listener_t      module_listener;
 static modinfo_t module_dummy;
 __link_set_add_rodata(modules, module_dummy);
 
+static module_t        *module_newmodule(modsrc_t);
+static void    module_require_force(module_t *);
 static int     module_do_load(const char *, bool, int, prop_dictionary_t,
                    module_t **, modclass_t class, bool);
-static int     module_do_unload(const char *);
+static int     module_do_unload(const char *, bool);
 static int     module_do_builtin(const char *, module_t **);
 static int     module_fetch_info(module_t *);
 static void    module_thread(void *);
@@ -155,6 +157,32 @@ module_listener_cb(kauth_cred_t cred, ka
 }
 
 /*
+ * Allocate a new module_t
+ */
+static module_t *
+module_newmodule(modsrc_t source)
+{
+       module_t *mod;
+
+       mod = kmem_zalloc(sizeof(*mod), KM_SLEEP);
+       if (mod != NULL) {
+               mod->mod_source = source;
+               mod->mod_info = NULL;
+               mod->mod_flags = 0;
+       }
+       return mod;
+}
+
+/*
+ * Require the -f (force) flag to load a module
+ */
+static void
+module_require_force(struct module *mod)
+{
+       mod->mod_flags |= MODFLG_MUST_FORCE;
+}
+
+/*
  * Add modules to the builtin list.  This can done at boottime or
  * at runtime if the module is linked into the kernel with an
  * external linker.  All or none of the input will be handled.
@@ -192,9 +220,8 @@ module_builtin_add(modinfo_t *const *mip
                        mipskip++;
                        continue;
                }
-               modp[i] = kmem_zalloc(sizeof(*modp[i]), KM_SLEEP);
+               modp[i] = module_newmodule(MODULE_SOURCE_KERNEL);
                modp[i]->mod_info = mip[i+mipskip];
-               modp[i]->mod_source = MODULE_SOURCE_KERNEL;
        }
        mutex_enter(&module_lock);
 
@@ -265,7 +292,7 @@ module_builtin_remove(modinfo_t *mi, boo
                        return rv;
 
                mutex_enter(&module_lock);
-               rv = module_do_unload(mi->mi_name);
+               rv = module_do_unload(mi->mi_name, true);
                if (rv) {
                        goto out;
                }
@@ -335,12 +362,12 @@ module_init(void)
 }
 
 /*
- * module_init2:
+ * module_start_unload_thread:
  *
  *     Start the auto unload kthread.
  */
 void
-module_init2(void)
+module_start_unload_thread(void)
 {
        int error;
 
@@ -350,6 +377,24 @@ module_init2(void)
                panic("module_init: %d", error);
 }
 
+/*
+ * module_builtin_no_autoload:
+ *
+ * Prevent any built-in modules that have not yet been initialized from
+ * being autoloaded.
+ */
+void
+module_builtin_require_force(void)
+{
+       module_t *mod;
+
+       mutex_enter(&module_lock);
+       TAILQ_FOREACH(mod, &module_builtins, mod_chain) {
+               module_require_force(mod);
+       }
+       mutex_exit(&module_lock);
+}
+
 static struct sysctllog *module_sysctllog;
 
 static void
@@ -412,10 +457,14 @@ module_init_class(modclass_t class)
                        /*
                         * If initializing a builtin module fails, don't try
                         * to load it again.  But keep it around and queue it
-                        * on the disabled list after we're done with module
-                        * init.
+                        * on the builtins list after we're done with module
+                        * init.  Don't set it to MODFLG_MUST_FORCE in case a
+                        * future attempt to initialize can be successful.
+                        * (If the module has previously been set to
+                        * MODFLG_MUST_FORCE, don't try to override that!)
                         */
-                       if (module_do_builtin(mi->mi_name, NULL) != 0) {
+                       if (mod->mod_flags & MODFLG_MUST_FORCE ||
+                           module_do_builtin(mi->mi_name, NULL) != 0) {
                                TAILQ_REMOVE(&module_builtins, mod, mod_chain);
                                TAILQ_INSERT_TAIL(&bi_fail, mod, mod_chain);
                        }
@@ -438,7 +487,7 @@ module_init_class(modclass_t class)
                }
        } while (mod != NULL);
 
-       /* failed builtin modules remain disabled */
+       /* return failed builtin modules to builtin list */
        while ((mod = TAILQ_FIRST(&bi_fail)) != NULL) {
                TAILQ_REMOVE(&bi_fail, mod, mod_chain);
                TAILQ_INSERT_TAIL(&module_builtins, mod, mod_chain);
@@ -544,7 +593,7 @@ module_unload(const char *name)
        }
 
        mutex_enter(&module_lock);
-       error = module_do_unload(name);
+       error = module_do_unload(name, true);
        mutex_exit(&module_lock);
 
        return error;
@@ -794,7 +843,8 @@ module_do_load(const char *name, bool is
                }
        }
        if (mod) {
-               if ((flags & MODCTL_LOAD_FORCE) == 0) {
+               if ((mod->mod_flags & MODFLG_MUST_FORCE) &&
+                   (flags & MODCTL_LOAD_FORCE) == 0) {
                        if (!autoload) {
                                module_error("use -f to reinstate "
                                    "builtin module \"%s\"", name);
@@ -839,7 +889,7 @@ module_do_load(const char *name, bool is
                                return 0;
                        }
                }                               
-               mod = kmem_zalloc(sizeof(*mod), KM_SLEEP);
+               mod = module_newmodule(MODULE_SOURCE_FILESYS);
                if (mod == NULL) {
                        module_error("out of memory for `%s'", name);
                        depth--;
@@ -853,7 +903,6 @@ module_do_load(const char *name, bool is
                        depth--;
                        return error;
                }
-               mod->mod_source = MODULE_SOURCE_FILESYS;
                TAILQ_INSERT_TAIL(&pending, mod, mod_chain);
 
                error = module_fetch_info(mod);
@@ -1048,7 +1097,7 @@ module_do_load(const char *name, bool is
  *     Helper routine: do the dirty work of unloading a module.
  */
 static int
-module_do_unload(const char *name)
+module_do_unload(const char *name, bool load_requires_force)
 {
        module_t *mod;
        int error;
@@ -1086,6 +1135,8 @@ module_do_unload(const char *name)
        }
        if (mod->mod_source == MODULE_SOURCE_KERNEL) {
                mod->mod_nrequired = 0; /* will be re-parsed */
+               if (load_requires_force)
+                       module_require_force(mod);
                TAILQ_INSERT_TAIL(&module_builtins, mod, mod_chain);
                module_builtinlist++;
        } else {
@@ -1108,11 +1159,10 @@ module_prime(void *base, size_t size)
        module_t *mod;
        int error;
 
-       mod = kmem_zalloc(sizeof(*mod), KM_SLEEP);
+       mod = module_newmodule(MODULE_SOURCE_BOOT);
        if (mod == NULL) {
                return ENOMEM;
        }
-       mod->mod_source = MODULE_SOURCE_BOOT;
 
        error = kobj_load_mem(&mod->mod_kobj, base, size);
        if (error != 0) {
@@ -1218,7 +1268,7 @@ module_thread(void *cookie)
                        mi = mod->mod_info;
                        error = (*mi->mi_modcmd)(MODULE_CMD_AUTOUNLOAD, NULL);
                        if (error == 0 || error == ENOTTY) {
-                               (void)module_do_unload(mi->mi_name);
+                               (void)module_do_unload(mi->mi_name, false);
                        }
                }
                mutex_exit(&module_lock);
Index: kern/init_main.c
===================================================================
RCS file: /cvsroot/src/sys/kern/init_main.c,v
retrieving revision 1.420
diff -u -p -r1.420 init_main.c
--- kern/init_main.c    10 Jun 2010 20:54:53 -0000      1.420
+++ kern/init_main.c    18 Jun 2010 11:37:27 -0000
@@ -430,7 +430,7 @@ main(void)
        loginit();
 
        /* Second part of module system initialization. */
-       module_init2();
+       module_start_unload_thread();
 
        /* Initialize the file systems. */
 #ifdef NVNODE_IMPLICIT
@@ -594,9 +594,11 @@ main(void)
 
        /*
         * Load any remaining builtin modules, and hand back temporary
-        * storage to the VM system.
+        * storage to the VM system.  Then require force when loading any
+        * remaining un-init'ed built-in modules to avoid later surprises.
         */
        module_init_class(MODULE_CLASS_ANY);
+       module_builtin_require_force();
 
        /*
         * Finalize configuration now that all real devices have been


Home | Main Index | Thread Index | Old Index