module: Make the 'usage' lists be two-way

When adding a module that depends on another one, we used to create a one-way list of "modules_which_use_me", so that module unloading could see who needs a module. It's actually quite simple to make that list go both ways: so that we not only can see "who uses me", but also see a list of modules that are "used by me". In fact, we always wanted that list in "module_unload_free()": when we unload a module, we want to also release all the other modules that are used by that module. But because we didn't have that list, we used to first iterate over all modules, and then iterate over each "used by me" list of that module. By making the list two-way, we simplify module_unload_free(), and it allows for some trivial fixes later too. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (cleaned & rebased)

module: Make the 'usage' lists be two-way
When adding a module that depends on another one, we used to create a one-way list of "modules_which_use_me", so that module unloading could see who needs a module. It's actually quite simple to make that list go both ways: so that we not only can see "who uses me", but also see a list of modules that are "used by me". In fact, we always wanted that list in "module_unload_free()": when we unload a module, we want to also release all the other modules that are used by that module. But because we didn't have that list, we used to first iterate over all modules, and then iterate over each "used by me" list of that module. By making the list two-way, we simplify module_unload_free(), and it allows for some trivial fixes later too. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (cleaned & rebased)
Linus Torvalds · Rusty Russell
1 parent ad8456361f
Showing 2 changed files with 51 additions and 32 deletions Side-by-side Diff
include/linux/module.h
kernel/module.c
@@ -359,7 +359,9 @@
  
 #ifdef CONFIG_MODULE_UNLOAD
 	/* What modules depend on me? */
-	struct list_head modules_which_use_me;
+	struct list_head source_list;
+	/* What modules do I depend on? */
+	struct list_head target_list;
  
 	/* Who is waiting for us to be unloaded */
 	struct task_struct *waiter;
@@ -523,7 +523,8 @@
 {
 	int cpu;
  
-	INIT_LIST_HEAD(&mod->modules_which_use_me);
+	INIT_LIST_HEAD(&mod->source_list);
+	INIT_LIST_HEAD(&mod->target_list);
 	for_each_possible_cpu(cpu) {
 		per_cpu_ptr(mod->refptr, cpu)->incs = 0;
 		per_cpu_ptr(mod->refptr, cpu)->decs = 0;
@@ -538,8 +539,9 @@
 /* modules using other modules */
 struct module_use
 {
-	struct list_head list;
-	struct module *module_which_uses;
+	struct list_head source_list;
+	struct list_head target_list;
+	struct module *source, *target;
 };
  
 /* Does a already use b? */
@@ -547,8 +549,8 @@
 {
 	struct module_use *use;
  
-	list_for_each_entry(use, &b->modules_which_use_me, list) {
-		if (use->module_which_uses == a) {
+	list_for_each_entry(use, &b->source_list, source_list) {
+		if (use->source == a) {
 			DEBUGP("%s uses %s!\n", a->name, b->name);
 			return 1;
 		}
@@ -557,6 +559,33 @@
 	return 0;
 }
  
+/*
+ * Module a uses b
+ *  - we add 'a' as a "source", 'b' as a "target" of module use
+ *  - the module_use is added to the list of 'b' sources (so
+ *    'b' can walk the list to see who sourced them), and of 'a'
+ *    targets (so 'a' can see what modules it targets).
+ */
+static int add_module_usage(struct module *a, struct module *b)
+{
+	int no_warn;
+	struct module_use *use;
+
+	DEBUGP("Allocating new usage for %s.\n", a->name);
+	use = kmalloc(sizeof(*use), GFP_ATOMIC);
+	if (!use) {
+		printk(KERN_WARNING "%s: out of memory loading\n", a->name);
+		return -ENOMEM;
+	}
+
+	use->source = a;
+	use->target = b;
+	list_add(&use->source_list, &b->source_list);
+	list_add(&use->target_list, &a->target_list);
+	no_warn = sysfs_create_link(b->holders_dir, &a->mkobj.kobj, a->name);
+	return 0;
+}
+
 /* Module a uses b */
 int use_module(struct module *a, struct module *b)
 {
  
@@ -578,17 +607,11 @@
 	if (err)
 		return 0;
  
-	DEBUGP("Allocating new usage for %s.\n", a->name);
-	use = kmalloc(sizeof(*use), GFP_ATOMIC);
-	if (!use) {
-		printk("%s: out of memory loading\n", a->name);
+	err = add_module_usage(a, b);
+	if (err) {
 		module_put(b);
 		return 0;
 	}
-
-	use->module_which_uses = a;
-	list_add(&use->list, &b->modules_which_use_me);
-	no_warn = sysfs_create_link(b->holders_dir, &a->mkobj.kobj, a->name);
 	return 1;
 }
 EXPORT_SYMBOL_GPL(use_module);
  
@@ -596,22 +619,16 @@
 /* Clear the unload stuff of the module. */
 static void module_unload_free(struct module *mod)
 {
-	struct module *i;
+	struct module_use *use, *tmp;
  
-	list_for_each_entry(i, &modules, list) {
-		struct module_use *use;
-
-		list_for_each_entry(use, &i->modules_which_use_me, list) {
-			if (use->module_which_uses == mod) {
-				DEBUGP("%s unusing %s\n", mod->name, i->name);
-				module_put(i);
-				list_del(&use->list);
-				kfree(use);
-				sysfs_remove_link(i->holders_dir, mod->name);
-				/* There can be at most one match. */
-				break;
-			}
-		}
+	list_for_each_entry_safe(use, tmp, &mod->target_list, target_list) {
+		struct module *i = use->target;
+		DEBUGP("%s unusing %s\n", mod->name, i->name);
+		module_put(i);
+		list_del(&use->source_list);
+		list_del(&use->target_list);
+		kfree(use);
+		sysfs_remove_link(i->holders_dir, mod->name);
 	}
 }
  
@@ -735,7 +752,7 @@
 		goto out;
 	}
  
-	if (!list_empty(&mod->modules_which_use_me)) {
+	if (!list_empty(&mod->source_list)) {
 		/* Other modules depend on us: get rid of them first. */
 		ret = -EWOULDBLOCK;
 		goto out;
  
@@ -799,9 +816,9 @@
  
 	/* Always include a trailing , so userspace can differentiate
            between this and the old multi-field proc format. */
-	list_for_each_entry(use, &mod->modules_which_use_me, list) {
+	list_for_each_entry(use, &mod->source_list, source_list) {
 		printed_something = 1;
-		seq_printf(m, "%s,", use->module_which_uses->name);
+		seq_printf(m, "%s,", use->source->name);
 	}
  
 	if (mod->init != NULL && mod->exit == NULL) {
...	...	@@ -359,7 +359,9 @@
359	359
360	360	#ifdef CONFIG_MODULE_UNLOAD
361	361	/* What modules depend on me? */
362		- struct list_head modules_which_use_me;
	362	+ struct list_head source_list;
	363	+ /* What modules do I depend on? */
	364	+ struct list_head target_list;
363	365
364	366	/* Who is waiting for us to be unloaded */
365	367	struct task_struct *waiter;
...	...	@@ -523,7 +523,8 @@
523	523	{
524	524	int cpu;
525	525
526		- INIT_LIST_HEAD(&mod->modules_which_use_me);
	526	+ INIT_LIST_HEAD(&mod->source_list);
	527	+ INIT_LIST_HEAD(&mod->target_list);
527	528	for_each_possible_cpu(cpu) {
528	529	per_cpu_ptr(mod->refptr, cpu)->incs = 0;
529	530	per_cpu_ptr(mod->refptr, cpu)->decs = 0;
...	...	@@ -538,8 +539,9 @@
538	539	/* modules using other modules */
539	540	struct module_use
540	541	{
541		- struct list_head list;
542		- struct module *module_which_uses;
	542	+ struct list_head source_list;
	543	+ struct list_head target_list;
	544	+ struct module source, target;
543	545	};
544	546
545	547	/* Does a already use b? */
...	...	@@ -547,8 +549,8 @@
547	549	{
548	550	struct module_use *use;
549	551
550		- list_for_each_entry(use, &b->modules_which_use_me, list) {
551		- if (use->module_which_uses == a) {
	552	+ list_for_each_entry(use, &b->source_list, source_list) {
	553	+ if (use->source == a) {
552	554	DEBUGP("%s uses %s!\n", a->name, b->name);
553	555	return 1;
554	556	}
...	...	@@ -557,6 +559,33 @@
557	559	return 0;
558	560	}
559	561
	562	+/*
	563	+ * Module a uses b
	564	+ * - we add 'a' as a "source", 'b' as a "target" of module use
	565	+ * - the module_use is added to the list of 'b' sources (so
	566	+ * 'b' can walk the list to see who sourced them), and of 'a'
	567	+ * targets (so 'a' can see what modules it targets).
	568	+ */
	569	+static int add_module_usage(struct module a, struct module b)
	570	+{
	571	+ int no_warn;
	572	+ struct module_use *use;
	573	+
	574	+ DEBUGP("Allocating new usage for %s.\n", a->name);
	575	+ use = kmalloc(sizeof(*use), GFP_ATOMIC);
	576	+ if (!use) {
	577	+ printk(KERN_WARNING "%s: out of memory loading\n", a->name);
	578	+ return -ENOMEM;
	579	+ }
	580	+
	581	+ use->source = a;
	582	+ use->target = b;
	583	+ list_add(&use->source_list, &b->source_list);
	584	+ list_add(&use->target_list, &a->target_list);
	585	+ no_warn = sysfs_create_link(b->holders_dir, &a->mkobj.kobj, a->name);
	586	+ return 0;
	587	+}
	588	+
560	589	/* Module a uses b */
561	590	int use_module(struct module a, struct module b)
562	591	{
563	592
...	...	@@ -578,17 +607,11 @@
578	607	if (err)
579	608	return 0;
580	609
581		- DEBUGP("Allocating new usage for %s.\n", a->name);
582		- use = kmalloc(sizeof(*use), GFP_ATOMIC);
583		- if (!use) {
584		- printk("%s: out of memory loading\n", a->name);
	610	+ err = add_module_usage(a, b);
	611	+ if (err) {
585	612	module_put(b);
586	613	return 0;
587	614	}
588		-
589		- use->module_which_uses = a;
590		- list_add(&use->list, &b->modules_which_use_me);
591		- no_warn = sysfs_create_link(b->holders_dir, &a->mkobj.kobj, a->name);
592	615	return 1;
593	616	}
594	617	EXPORT_SYMBOL_GPL(use_module);
595	618
...	...	@@ -596,22 +619,16 @@
596	619	/* Clear the unload stuff of the module. */
597	620	static void module_unload_free(struct module *mod)
598	621	{
599		- struct module *i;
	622	+ struct module_use use, tmp;
600	623
601		- list_for_each_entry(i, &modules, list) {
602		- struct module_use *use;
603		-
604		- list_for_each_entry(use, &i->modules_which_use_me, list) {
605		- if (use->module_which_uses == mod) {
606		- DEBUGP("%s unusing %s\n", mod->name, i->name);
607		- module_put(i);
608		- list_del(&use->list);
609		- kfree(use);
610		- sysfs_remove_link(i->holders_dir, mod->name);
611		- /* There can be at most one match. */
612		- break;
613		- }
614		- }
	624	+ list_for_each_entry_safe(use, tmp, &mod->target_list, target_list) {
	625	+ struct module *i = use->target;
	626	+ DEBUGP("%s unusing %s\n", mod->name, i->name);
	627	+ module_put(i);
	628	+ list_del(&use->source_list);
	629	+ list_del(&use->target_list);
	630	+ kfree(use);
	631	+ sysfs_remove_link(i->holders_dir, mod->name);
615	632	}
616	633	}
617	634
...	...	@@ -735,7 +752,7 @@
735	752	goto out;
736	753	}
737	754
738		- if (!list_empty(&mod->modules_which_use_me)) {
	755	+ if (!list_empty(&mod->source_list)) {
739	756	/* Other modules depend on us: get rid of them first. */
740	757	ret = -EWOULDBLOCK;
741	758	goto out;
742	759
...	...	@@ -799,9 +816,9 @@
799	816
800	817	/* Always include a trailing , so userspace can differentiate
801	818	between this and the old multi-field proc format. */
802		- list_for_each_entry(use, &mod->modules_which_use_me, list) {
	819	+ list_for_each_entry(use, &mod->source_list, source_list) {
803	820	printed_something = 1;
804		- seq_printf(m, "%s,", use->module_which_uses->name);
	821	+ seq_printf(m, "%s,", use->source->name);
805	822	}
806	823
807	824	if (mod->init != NULL && mod->exit == NULL) {