[PATCH] Remove stop_machine during module load

[PATCH] Remove stop_machine during module load

[Andi Kleen]

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.]

Cc: rusty@rustcorp.com.au
Cc: paulmck@us.ibm.com

Signed-off-by: Andi Kleen <ak@linux.intel.com>

Index: linux-2.6.27-rc4-misc/kernel/module.c
===================================================================
--- linux-2.6.27-rc4-misc.orig/kernel/module.c
+++ 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,

Re: [PATCH] Remove stop_machine during module load

[Paul E. McKenney]

On Fri, Aug 29, 2008 at 09:17:34PM +0200, Andi Kleen wrote:
> 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.]

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

> Cc: rusty@rustcorp.com.au
> Cc: paulmck@us.ibm.com
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> 
> Index: linux-2.6.27-rc4-misc/kernel/module.c
> ===================================================================
> --- linux-2.6.27-rc4-misc.orig/kernel/module.c
> +++ 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,

Re: [PATCH] Remove stop_machine during module load

[Andi Kleen]

Hi Paul,

Thanks for the excellent review.

On Fri, Aug 29, 2008 at 01:44:57PM -0700, Paul E. McKenney wrote:
> 
> OK, what about the read side?  Not so good for __unlink_module() to yank

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. 

> 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()

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

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

I think that would be a the better approach.

-Andi

Re: [PATCH] Remove stop_machine during module load

[Paul E. McKenney]

On Fri, Aug 29, 2008 at 11:23:30PM +0200, Andi Kleen wrote:
> Hi Paul,
> 
> Thanks for the excellent review.
> 
> On Fri, Aug 29, 2008 at 01:44:57PM -0700, Paul E. McKenney wrote:
> > 
> > OK, what about the read side?  Not so good for __unlink_module() to yank
> 
> 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 -- perhaps I was confusing preventing CPU hotplug with preventing
stop_machine().  So disabling preemption holds off stop_machine()?
Yep, looks that way.

> > 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()
> 
> Ah that's needed for the Alpha barrier depends semantics,
> right? 

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

> > 	Yet another approach would be to use call_rcu() to defer the
> > 	various kfree() &c calls later in free_module.
> 
> I think that would be a the better approach.

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

Re: [PATCH] Remove stop_machine during module load

[Rusty Russell]

On Saturday 30 August 2008 05:17:34 Andi Kleen wrote:
> 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.

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

Rusty.

Re: [PATCH] Remove stop_machine during module load

[Andi Kleen]

On Mon, Sep 01, 2008 at 05:02:00PM +1000, Rusty Russell wrote:
> On Saturday 30 August 2008 05:17:34 Andi Kleen wrote:
> > 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.
> 
> Thanks Andi, but doesn't this mean that we should be doing list_for_each_rcu 
> in all the kallsyms readers?

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

-- 
ak@linux.intel.com