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
On Thu, 17 Jun 2010, Antti Kantee wrote:
On Wed Jun 16 2010 at 15:36:30 -0700, Paul Goyette wrote:
The attached diffs add one more routine, module_init3() which gets
called from init_main() right after module_class_init(MODULE_CLASS_ANY).
module_init3() walks the list of builtin modules that have not already
been init'd and marks them disabled.
Tested briefly on my home systems and appears to work.
Any objections to committing this?
I'd still hook it to the end of module_class_init(MODULE_CLASS_ANY)
instead of adding more randomly numbered module_init<n>() calls.
The other benefit from doing so is that you get it done atomically,
which is always worthwhile, and doubly so when it's a low hanging fruit
like here.
I dislike adding another special-case-overload. In my mind, having
module_class_init(MODULE_CLASS_ANY) have the additional effect of
"disabling" un-init'd modules is not much different from automatically
registering modules in MODULE_CLASS_SECMODEL. This thread has already
noted that such registration belongs in each module's modcmd().
@@ -416,6 +434,7 @@ module_init_class(modclass_t class)
* init.
*/
if (module_do_builtin(mi->mi_name, NULL) != 0) {
+ mod->mod_disabled = true;
TAILQ_REMOVE(&module_builtins, mod, mod_chain);
TAILQ_INSERT_TAIL(&bi_fail, mod, mod_chain);
}
Why do you mark it as disabled? Doesn't this conflict with the "it
might succeed in a later module_init_class()" idea you presented earlier?
Yes, it does conflict. I've removed this line, and updated the comment
block above.
module_disabled = true/false in multiple places looks a little
error-prone. Now that struct module is growing more and more members,
maybe we can just have an object allocator which initializes the value and
afterwards the only acceptable mutation for module_disabled is setting
it to true (might make sense to rename the variable to something like
module_virgin and flip the polarity, though).
HeHe - module_virgin would seem to be a bit obscure to me if I hadn't
written the code. Perhaps module_autoload_ok would be acceptable (with
a state flip)? Or module_require_force (with no flip)?
The attached diffs use module_autoload_ok instead of module_disabled
(and state flip), provide an object allocator, and a single mutation
function. It also avoids disabling the module if it is unloaded by the
auto-unload thread; only modules that are explicitly unloaded by name
should be prevented from future autoloads.
I've renamed both module_init2() and module_init3() to more descriptive
routine names, but for now I've kept _init3() as a separate function and
not made it part of module_init_class(). I'll be doing some testing of
this over the next day or two before committing.
As always, review, comments, and suggestions (and yes, even criticisms!)
are welcomed and solicited. :)
-------------------------------------------------------------------------
| 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/sys/module.h
===================================================================
RCS file: /cvsroot/src/sys/sys/module.h,v
retrieving revision 1.23
diff -u -p -r1.23 module.h
--- sys/sys/module.h 24 May 2010 03:50:25 -0000 1.23
+++ sys/sys/module.h 17 Jun 2010 12:53:24 -0000
@@ -90,6 +90,7 @@ typedef struct module {
time_t mod_autotime;
void *mod_ctf;
u_int mod_fbtentries; /* DTrace FBT entrie count */
+ bool mod_autoload_ok;
} module_t;
/*
@@ -120,7 +121,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_no_autoload(void);
void module_init_md(void);
void module_init_class(modclass_t);
int module_prime(void *, size_t);
Index: sys/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
--- sys/kern/init_main.c 10 Jun 2010 20:54:53 -0000 1.420
+++ sys/kern/init_main.c 17 Jun 2010 12:53:24 -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 prevent any remaining un-init'ed
+ * built-in modules from being autoloaded to avoid later surprises.
*/
module_init_class(MODULE_CLASS_ANY);
+ module_builtin_no_autoload();
/*
* Finalize configuration now that all real devices have been
Index: sys/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
--- sys/kern/kern_module.c 26 May 2010 23:53:21 -0000 1.69
+++ sys/kern/kern_module.c 17 Jun 2010 12:53:24 -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_disable_autoload(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_autoload_ok = true;
+ }
+ return mod;
+}
+
+/*
+ * Disable future autoloading of a module
+ */
+static void
+module_disable_autoload(struct module *mod)
+{
+ mod->mod_autoload_ok = false;
+}
+
+/*
* 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_no_autoload(void)
+{
+ module_t *mod;
+
+ mutex_enter(&module_lock);
+ TAILQ_FOREACH(mod, &module_builtins, mod_chain) {
+ module_disable_autoload(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 autoload_disable in case
+ * a future attempt to initialize is successful.
+ * (If the module has previously been set to
+ * autoload_disable, don't try to override that!)
*/
- if (module_do_builtin(mi->mi_name, NULL) != 0) {
+ if (!mod->mod_autoload_ok ||
+ 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,7 @@ module_do_load(const char *name, bool is
}
}
if (mod) {
- if ((flags & MODCTL_LOAD_FORCE) == 0) {
+ if (!mod->mod_autoload_ok && (flags & MODCTL_LOAD_FORCE) == 0) {
if (!autoload) {
module_error("use -f to reinstate "
"builtin module \"%s\"", name);
@@ -839,7 +888,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 +902,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 +1096,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 disable_autoload)
{
module_t *mod;
int error;
@@ -1086,6 +1134,8 @@ module_do_unload(const char *name)
}
if (mod->mod_source == MODULE_SOURCE_KERNEL) {
mod->mod_nrequired = 0; /* will be re-parsed */
+ if (disable_autoload)
+ module_disable_autoload(mod);
TAILQ_INSERT_TAIL(&module_builtins, mod, mod_chain);
module_builtinlist++;
} else {
@@ -1108,11 +1158,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 +1267,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);
Home |
Main Index |
Thread Index |
Old Index