two proposed patches affecting delete_module(2) in

Joe Harvell (Joe.Harvell.harvell@nt.com)
Wed, 23 Sep 1998 13:51:08 -0500


These proposed patches concern the behavior when calling the delete_module system call
with a NULL argument.

Here is what the man page says:

DESCRIPTION
delete_module attempts to remove an unused loadable module entry. If name is NULL, all
unused modules marked auto-clean will be removed. This system call is only open to the
superuser.

Based on that description, the behavior I would expect is for all autoclean, unused, and unreferenced modules to be removed. Furthermore, I would expect all modules that become unreferenced (and are also unused and autoclean) as a result of this removal to be removed.

After examinig the code, I found the behavior to be the following:

All candidate modules (candidate=unreferenced, autoclean, running, not deleted, used at least once, unused) are tagged for removal. If on the next call to delete_module(NULL), these modules are still candidates, they are removed, and any candidate module that becomes unreferenced as a result of this removal is also removed.

I believe I understand the rationale for such actual behavior. If you have rmmod -a running periodically in the background, and a module was being used intermittently but frequently by some processes, it might get unloaded and reloaded needlessly because at the time rmmod -a ran, it happened to be unused.

Now to the patches:

The first one preserves the actual behavior of the existing code, but makes it more efficient. The existing code sets a flag called 'something_changed' every time it actually unloads a module, and then reprocess all the modules until that flag was not set. It appears the rationale for this is that if free_module was called, it could have made other modules candidates for removal (because they are no longer referenced). By that rationale, all the modules would have to be processed again so that these modules could also be removed. However, since a module is not loaded until all the modules it depends on are loaded, any module that becomes unreferenced by freeing another module will be AFTER that other module in the module list. Therefore, if a module becomes a candidate in this fashion, it is guaranteed to be processed in the same iteration through the module list. The module list only has to be walked once, and there is thus no need for the 'something_changed' flag. Wit!
h the flag, the module list will be processed twice, and on the second time through, it is guaranteed that no module will change state in any way because there will be no candidates. The first patch I made removes the use of this flag, and it is included below:

BEGIN PATCH
--- linux/kernel/module.c Sun Jun 7 12:37:42 1998
+++ module.c.new Wed Sep 23 13:06:07 1998
@@ -363,7 +363,6 @@
struct module *mod, *next;
char *name;
long error = -EPERM;
- int something_changed;

lock_kernel();
if (!capable(CAP_SYS_MODULE))
@@ -393,8 +392,6 @@
}

/* Do automatic reaping */
-restart:
- something_changed = 0;
for (mod = module_list; mod != &kernel_module; mod = next) {
next = mod->next;
if (mod->refs == NULL
@@ -404,16 +401,12 @@
&& (mod->flags & MOD_USED_ONCE)
&& !__MOD_IN_USE(mod)) {
if ((mod->flags & MOD_VISITED)
- && !(mod->flags & MOD_JUST_FREED)) {
+ && !(mod->flags & MOD_JUST_FREED))
mod->flags &= ~MOD_VISITED;
- } else {
+ else
free_module(mod, 1);
- something_changed = 1;
- }
}
}
- if (something_changed)
- goto restart;
for (mod = module_list; mod != &kernel_module; mod = mod->next)
mod->flags &= ~MOD_JUST_FREED;
error = 0;
END PATCH

The next patch causes delete_module to behave as implied by the man pages. It also removes the unnecessary 'something_changed' flag. Here it is:

BEGIN PATCH
--- linux/kernel/module.c Sun Jun 7 12:37:42 1998
+++ module.c.new Wed Sep 23 10:49:23 1998
@@ -54,7 +54,7 @@
static long get_mod_name(const char *user_name, char **buf);
static void put_mod_name(char *buf);
static struct module *find_module(const char *name);
-static void free_module(struct module *, int tag_freed);
+static void free_module(struct module *);

/*
@@ -363,7 +363,6 @@
struct module *mod, *next;
char *name;
long error = -EPERM;
- int something_changed;

lock_kernel();
if (!capable(CAP_SYS_MODULE))
@@ -387,14 +386,12 @@
if (mod->refs != NULL || __MOD_IN_USE(mod))
goto out;

- free_module(mod, 0);
+ free_module(mod);
error = 0;
goto out;
}

/* Do automatic reaping */
-restart:
- something_changed = 0;
for (mod = module_list; mod != &kernel_module; mod = next) {
next = mod->next;
if (mod->refs == NULL
@@ -402,20 +399,9 @@
&& (mod->flags & MOD_RUNNING)
&& !(mod->flags & MOD_DELETED)
&& (mod->flags & MOD_USED_ONCE)
- && !__MOD_IN_USE(mod)) {
- if ((mod->flags & MOD_VISITED)
- && !(mod->flags & MOD_JUST_FREED)) {
- mod->flags &= ~MOD_VISITED;
- } else {
- free_module(mod, 1);
- something_changed = 1;
- }
- }
+ && !__MOD_IN_USE(mod))
+ free_module(mod);
}
- if (something_changed)
- goto restart;
- for (mod = module_list; mod != &kernel_module; mod = mod->next)
- mod->flags &= ~MOD_JUST_FREED;
error = 0;
out:
unlock_kernel();
@@ -775,7 +761,7 @@
*/

static void
-free_module(struct module *mod, int tag_freed)
+free_module(struct module *mod)
{
struct module_ref *dep;
unsigned i;
@@ -783,8 +769,7 @@
/* Let the module clean up. */

mod->flags |= MOD_DELETED;
- if (mod->flags & MOD_RUNNING)
- {
+ if (mod->flags & MOD_RUNNING) {
if(mod->cleanup)
mod->cleanup();
mod->flags &= ~MOD_RUNNING;
@@ -797,8 +782,6 @@
for (pp = &dep->dep->refs; *pp != dep; pp = &(*pp)->next_ref)
continue;
*pp = dep->next_ref;
- if (tag_freed && dep->dep->refs == NULL)
- dep->dep->flags |= MOD_JUST_FREED;
}

/* And from the main module list. */
END PATCH

I think what would be really nice is some sort of extra parameter to delete_module, so that you could explicitly tell it to reap all modules right away. Otherwise, it could just tag them as it does currently. Please let me know what you think. Especially about a change to the sytem call interface.

--
+----------------------------------------------------------------------------+
| Joe Harvell                                  TEL:   +1 972.685.4886        |
| Carrier Packet Networks                   ESN:   445/4886               |
| Nortel (Northern Telecom)                  email:   Joe.Harvell@nortel.com |
+----------------------------------------------------------------------------+

- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/