[RFC][PATCH] make module refcounts use percpu_counters

[RFC][PATCH] make module refcounts use percpu_counters

[Dave Hansen]

Module refcounts currently use a percpu counter stored
in the 'struct module'.  However, we also have a more
generic implementation that does stuff like handle
hotplug cpus.

I'm not actually all that convinced that this refcount
actually does a lot of good, with cpus racing bumping
the counters at the same time that they're being
summed up.  But, it certainly isn't any worse than
what was there before.  We also get any hotplug-cpu
compatible changes that happen to percpu_counters for
free.


Signed-off-by: Dave Hansen <haveblue@us.ibm.com>
---

 lxc-dave/include/linux/module.h |   10 ++++------
 lxc-dave/kernel/module.c        |   17 ++++-------------
 2 files changed, 8 insertions(+), 19 deletions(-)

diff -puN lib/percpu_counter.c~make-module-refcounts-use-percpu_counters lib/percpu_counter.c
diff -puN include/linux/percpu_counter.h~make-module-refcounts-use-percpu_counters include/linux/percpu_counter.h
diff -puN kernel/module.c~make-module-refcounts-use-percpu_counters kernel/module.c
--- lxc/kernel/module.c~make-module-refcounts-use-percpu_counters	2007-09-28 15:48:57.000000000 -0700
+++ lxc-dave/kernel/module.c	2007-09-28 15:48:57.000000000 -0700
@@ -502,13 +502,9 @@ MODINFO_ATTR(srcversion);
 /* Init the unload section of the module. */
 static void module_unload_init(struct module *mod)
 {
-	unsigned int i;
-
 	INIT_LIST_HEAD(&mod->modules_which_use_me);
-	for (i = 0; i < NR_CPUS; i++)
-		local_set(&mod->ref[i].count, 0);
 	/* Hold reference count during initialization. */
-	local_set(&mod->ref[raw_smp_processor_id()].count, 1);
+	percpu_counter_init(&mod->ref, 1);
 	/* Backwards compatibility macros put refcount during init. */
 	mod->waiter = current;
 }
@@ -629,11 +625,7 @@ static int try_stop_module(struct module
 
 unsigned int module_refcount(struct module *mod)
 {
-	unsigned int i, total = 0;
-
-	for (i = 0; i < NR_CPUS; i++)
-		total += local_read(&mod->ref[i].count);
-	return total;
+	return percpu_counter_sum(&mod->ref);
 }
 EXPORT_SYMBOL(module_refcount);
 
@@ -720,6 +712,7 @@ sys_delete_module(const char __user *nam
 	/* Never wait if forced. */
 	if (!forced && module_refcount(mod) != 0)
 		wait_for_zero_refcount(mod);
+	percpu_counter_destroy(&mod->ref);
 
 	/* Final destruction now noone is using it. */
 	if (mod->exit != NULL) {
@@ -802,12 +795,10 @@ static struct module_attribute refcnt = 
 void module_put(struct module *module)
 {
 	if (module) {
-		unsigned int cpu = get_cpu();
-		local_dec(&module->ref[cpu].count);
+		percpu_counter_dec(&module->ref);
 		/* Maybe they're waiting for us to drop reference? */
 		if (unlikely(!module_is_live(module)))
 			wake_up_process(module->waiter);
-		put_cpu();
 	}
 }
 EXPORT_SYMBOL(module_put);
diff -puN include/asm-alpha/local.h~make-module-refcounts-use-percpu_counters include/asm-alpha/local.h
diff -puN include/asm-generic/local.h~make-module-refcounts-use-percpu_counters include/asm-generic/local.h
diff -puN include/asm-i386/local.h~make-module-refcounts-use-percpu_counters include/asm-i386/local.h
diff -puN include/asm-mips/local.h~make-module-refcounts-use-percpu_counters include/asm-mips/local.h
diff -puN include/asm-powerpc/local.h~make-module-refcounts-use-percpu_counters include/asm-powerpc/local.h
diff -puN fs/file_table.c~make-module-refcounts-use-percpu_counters fs/file_table.c
diff -puN include/linux/module.h~make-module-refcounts-use-percpu_counters include/linux/module.h
--- lxc/include/linux/module.h~make-module-refcounts-use-percpu_counters	2007-09-28 15:48:57.000000000 -0700
+++ lxc-dave/include/linux/module.h	2007-09-28 15:48:57.000000000 -0700
@@ -15,6 +15,7 @@
 #include <linux/stringify.h>
 #include <linux/kobject.h>
 #include <linux/moduleparam.h>
+#include <linux/percpu_counter.h>
 #include <asm/local.h>
 
 #include <asm/module.h>
@@ -339,7 +340,7 @@ struct module
 
 #ifdef CONFIG_MODULE_UNLOAD
 	/* Reference counts */
-	struct module_ref ref[NR_CPUS];
+	struct percpu_counter ref;
 
 	/* What modules depend on me? */
 	struct list_head modules_which_use_me;
@@ -412,8 +413,7 @@ static inline void __module_get(struct m
 {
 	if (module) {
 		BUG_ON(module_refcount(module) == 0);
-		local_inc(&module->ref[get_cpu()].count);
-		put_cpu();
+		percpu_counter_inc(&module->ref);
 	}
 }
 
@@ -422,12 +422,10 @@ static inline int try_module_get(struct 
 	int ret = 1;
 
 	if (module) {
-		unsigned int cpu = get_cpu();
 		if (likely(module_is_live(module)))
-			local_inc(&module->ref[cpu].count);
+			percpu_counter_inc(&module->ref);
 		else
 			ret = 0;
-		put_cpu();
 	}
 	return ret;
 }
_

Re: [RFC][PATCH] make module refcounts use percpu_counters

[Rusty Russell]

On Fri, 2007-09-28 at 16:00 -0700, Dave Hansen wrote:
> Module refcounts currently use a percpu counter stored
> in the 'struct module'.  However, we also have a more
> generic implementation that does stuff like handle
> hotplug cpus.
> 
> I'm not actually all that convinced that this refcount
> actually does a lot of good, with cpus racing bumping
> the counters at the same time that they're being
> summed up.  But, it certainly isn't any worse than
> what was there before.

That's why we look at the counters inside stop_machine_run().

Note that (1) the module implementation handles hotplug CPUs, and (2)
percpu_counter_sum() doesn't have to either (that's just for
percpu_counter_read()).

But it might be a useful cleanup (although a slight de-optimization).
If you want I'll queue for 2.6.24 (there are several other module
patches pending too).

In an ideal world, (1) we would have percpu pointers using the same
percpu mechanism as percpu variables, (2) we would have a modal variant
of percpu counters which would collapse to a single counter when we
cared about the precise value (probably using stop_machine for the
transition).  This would be useful for many other cases.

Cheers,
Rusty.

Re: [RFC][PATCH] make module refcounts use percpu_counters

[Dave Hansen]

On Mon, 2007-10-01 at 19:43 +1000, Rusty Russell wrote:
> On Fri, 2007-09-28 at 16:00 -0700, Dave Hansen wrote:
> > Module refcounts currently use a percpu counter stored
> > in the 'struct module'.  However, we also have a more
> > generic implementation that does stuff like handle
> > hotplug cpus.
> > 
> > I'm not actually all that convinced that this refcount
> > actually does a lot of good, with cpus racing bumping
> > the counters at the same time that they're being
> > summed up.  But, it certainly isn't any worse than
> > what was there before.
> 
> That's why we look at the counters inside stop_machine_run().

Ahhh.  That makes sense.  Although it wasn't apparent during my 3-second
perusal of the code.

> Note that (1) the module implementation handles hotplug CPUs


You're saying it handles hotplug because of stop_machine_run()?


> But it might be a useful cleanup (although a slight de-optimization).
> If you want I'll queue for 2.6.24 (there are several other module
> patches pending too).

Might as well.  It removed a very small amount of code, and opens the
door a bit for future optimizations in a single place.

> In an ideal world, (1) we would have percpu pointers using the same
> percpu mechanism as percpu variables, (2) we would have a modal variant
> of percpu counters which would collapse to a single counter when we
> cared about the precise value (probably using stop_machine for the
> transition).  This would be useful for many other cases.

Yeah, but before we do that, we need some kind of flag to get the
percpu_counter_mod() fast path shoved into the slow path that takes the
lock.

I'm not sure the stop_machine() mechanism will work very well if we try
to expand this much further for other users.  What would the SGI guys
think if these happened more than once in a blue moon?


-- Dave

Re: [RFC][PATCH] make module refcounts use percpu_counters

[Rusty Russell]

On Mon, 2007-10-01 at 10:03 -0700, Dave Hansen wrote:
> On Mon, 2007-10-01 at 19:43 +1000, Rusty Russell wrote:
> > On Fri, 2007-09-28 at 16:00 -0700, Dave Hansen wrote:
> > > Module refcounts currently use a percpu counter stored
> > > in the 'struct module'.  However, we also have a more
> > > generic implementation that does stuff like handle
> > > hotplug cpus.
> > > 
> > > I'm not actually all that convinced that this refcount
> > > actually does a lot of good, with cpus racing bumping
> > > the counters at the same time that they're being
> > > summed up.  But, it certainly isn't any worse than
> > > what was there before.
> > 
> > That's why we look at the counters inside stop_machine_run().
> 
> Ahhh.  That makes sense.  Although it wasn't apparent during my 3-second
> perusal of the code.
> 
> > Note that (1) the module implementation handles hotplug CPUs
> 
> 
> You're saying it handles hotplug because of stop_machine_run()?

No, it handles hotplug because it does every possible CPU, not every
online CPU.  percpu_counters empties cpu's counter presumably to avoid
systematic error.

Renaming percpu_counters to approximate_counters here would be nice.

> > But it might be a useful cleanup (although a slight de-optimization).
> > If you want I'll queue for 2.6.24 (there are several other module
> > patches pending too).
> 
> Might as well.  It removed a very small amount of code, and opens the
> door a bit for future optimizations in a single place.

You missed removing struct module_ref, too.  That's a little more code.

> > In an ideal world, (1) we would have percpu pointers using the same
> > percpu mechanism as percpu variables, (2) we would have a modal variant
> > of percpu counters which would collapse to a single counter when we
> > cared about the precise value (probably using stop_machine for the
> > transition).  This would be useful for many other cases.
> 
> Yeah, but before we do that, we need some kind of flag to get the
> percpu_counter_mod() fast path shoved into the slow path that takes the
> lock.

Well, there is already a branch in the fast path.  BTW, comparing before
and after applying your patch for a try_module_get/module_put pair, I
get 5.9 ns vs 20.6 ns.  We perhaps win something back on NUMA-like
machines, but it's not clear.

My initial implementation of such a counter used atomic ops via a
pointer.  The pointer was aimed at a shared counter for slow-mode.  The
problem is that you need to disable preemption around the counter update
(so you can use rcu to ensure everyone has seen the changeover).

> I'm not sure the stop_machine() mechanism will work very well if we try
> to expand this much further for other users.  What would the SGI guys
> think if these happened more than once in a blue moon?

Indeed, that's why I called it "stop_machine".  The real-time coders
hate it too.

Cheers,
Rusty.