make module refcounts use percpu_counters

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.

---

 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
@@ -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
@@ -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;
 }

_

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.

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

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

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

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

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.

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

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

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

Cheers,
Rusty.