Remove stop_machine during module load

Remove stop_machine during module load

module loading currently does a stop_machine on each module load to insert the module into the global module lists. Especially on larger systems this can be quite expensive.

It does that to handle concurrent lock lessmodule list readers like kallsyms.

I don't think stop_machine() is actually needed to insert something into a list though. There are no concurrent writers because the module mutex is taken. And the RCU list functions know how to insert a node into a list with the right memory ordering so that concurrent readers don't go off into the wood.

So remove the stop_machine for the module list insert and just do a list_add_rcu() instead.

Module removal will still do a stop_machine of course, it needs that for other reasons.

[cc Paul McKenney for review. It's not RCU, but quite similar.]

Index: linux-2.6.27-rc4-misc/kernel/module.c
===================================================================
@@ -42,6 +42,7 @@
 #include <linux/string.h>
 #include <linux/mutex.h>
 #include <linux/unwind.h>
+#include <linux/rculist.h>
 #include <asm/uaccess.h>
 #include <asm/cacheflush.h>
 #include <linux/license.h>

@@ -61,7 +62,7 @@
 #define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
 
 /* List of modules, protected by module_mutex or preempt_disable
- * (add/delete uses stop_machine). */
+ * (delete uses stop_machine/add uses RCU list operations). */
 static DEFINE_MUTEX(module_mutex);
 static LIST_HEAD(modules);
 

@@ -1391,17 +1392,6 @@ static void mod_kobject_remove(struct mo
 }
 
 /*
- * link the module with the whole machine is stopped with interrupts off
- * - this defends against kallsyms not taking locks
- */
-static int __link_module(void *_mod)
-{
-	struct module *mod = _mod;
-	list_add(&mod->list, &modules);
-	return 0;
-}
-
-/*
  * unlink the module with the whole machine is stopped with interrupts off
  * - this defends against kallsyms not taking locks
  */

@@ -2196,8 +2186,12 @@ static struct module *load_module(void _
 
 	/* Now sew it into the lists so we can get lockdep and oops
          * info during argument parsing.  Noone should access us, since
-         * strong_try_module_get() will fail. */
-	stop_machine(__link_module, mod, NULL);
+         * strong_try_module_get() will fail.
+	 * lockdep/oops can run asynchronous, so use the RCU list insertion
+	 * function to insert in a way safe to concurrent readers.
+	 * The mutex protects against concurrent writers.
+	 */
+	list_add_rcu(&mod->list, &modules);
 
 	/* Size of section 0 is 0, so this works well if no params */
 	err = parse_args(mod->name, mod->args,

Seems plausible, and faster module loading on big machines would be very nice. But aren't adjustments also needed on the removal side?

Ah, no they do not, because __unlink_module() is only called from the context of stop_machine

OK, what about the read side? Not so good for __unlink_module() to yank the module out from under a reader. Therefore, all readers must either disable interrupts to block stop_machine() or must hold some sort of mutex that prevents modules from being unloaded.

First, where the heck -is- the read side...

o each_symbol() needs its list_for_each_entry() to become
list_for_each_entry_rcu() and needs local_irq_disable() across the loop. (I think -- looks to me like irqs are enabled, anyway...)

o module_address_lookup() -- ditto. Also lookup_module_symbol_name().
And lookup_module_symbol_attrs(). And module_get_kallsym(). As well as module_kallsyms_lookup_name(). And it looks like search_module_extables() also. Ditto is_module_address(). Plus __module_text_address(). Perhaps module_update_markers().

o print_modules() -- needs the same treatment, but not sure
how wise it is to invoke a potentially very large number of printk()s with interrupts disabled.

An alternative would be to have free_module() do a synchronize_rcu() after the stop_machine(). Heck, if you are incurring a stop_machine(), what is an additional synchronize_rcu() among friends? ;-)

Yet another approach would be to use call_rcu() to defer the various kfree() &c calls later in free_module.

Both the synchronize_rcu() and the call_rcu() approaches of course require that the list traversals all be done under either rcu_read_lock() or local_irq_disable(). This works with preemptable RCU -- rcu_read_lock() blocks either the synchronize_rcu() or the call_rcu(), which ever is chosen, while the local_irq_disable() blocks the stop_machine().

But gack, there do appear to be lots of module_free() definitions out there! More definitions than uses, it appears. So maybe not so bad.

So maybe acquire module_mutex() or whatever to exclude module load and unload? Unless of course print_modules() is sometimes invoked with module_mutex() already held!

Too much fun!!! ;-)

o find_module() -- ditto. Unless the comment about callers
holding module_mutex really is honored, and unless module_mutex really prevents modules from being loaded and unloaded.

o My guess is that already_uses() is an innocent bystander and
thus need not change, but I cannot claim to be an expert on the modules code. Ditto for print_unload_info ().

o The caller of module_unload_free() presumably holds whatever
mutex prevents other modules from being loaded and unloaded, so should not need to change.

Anyway, the general idea looks good, and getting rid of stop_machine() for module load would be very cool on big machines, but the removal and read sides need some help as noted above.

Thanx, Paul

Hi Paul,

Thanks for the excellent review.

That's independent from my patch isn't it? I don't think I'm changing anything here. All of the issues you're pointing out are already in the code (except for the missing read_barrier_depends() perhaps)

I think the lockless users like oops or sysrq-t typically have preemption disabled, so they should be ok regarding that.

Ah that's needed for the Alpha barrier depends semantics, right?

I think that would be a the better approach.

-Andi

Ah -- perhaps I was confusing preventing CPU hotplug with preventing stop_machine(). So disabling preemption holds off stop_machine()? Yep, looks that way.

Yep! And to prevent compiler optimizations that could have the same effect.

Or maybe just disable preemption around the remaining readers, preventing any stop_machine()-based deletions from being carried out during the searches.

(And here I call myself a fan of real-time response!!! But I suppose that stop_machine() is going to be pretty hard on realtime response in any case, so just don't mess with modules while your real-time application is running...)

Thanx, Paul

Thanks Andi, but doesn't this mean that we should be doing list_for_each_rcu in all the kallsyms readers?

Rusty.

Yes I did that in v2. But only for the ones that don't hold modules_mutex anyways. Also I found that print_modules() does not always disable preemption.

-Andi

[email protected]