[RFC PATCH 0/5] module: Remove stop_machine from module unloading

[RFC PATCH 0/5] module: Remove stop_machine from module unloading

[Masami Hiramatsu]

Hi,

Here is a series of patches which remove stop_machine() from
module unloading.

Currently, each module unloading calls stop_machine()s 2 times.
One is for safely removing module from lists and one is to
check the reference counter. However, both are not necessary
for those purposes (required by current implementation).

First, we can use RCU for the list operation, we just need
a synchronize_rcu right before cleaning up.
Second, the reference counter can be checked atomically by
using atomic_t, instead of per-cpu module_ref. Of course,
for BIG SMP machines, atomic operation is not efficient.
However, they usually don't need to remove most of modules too.

In this series, I just fixed to use RCU for the module(and
bugs) list for the first stop_machine.
And for the second one, I replaced module_ref with atomic_t
and introduced a "module lockup" module load option, which
makes a module un-removable (lock up the module in kernel).
The lockup modules can not be removed except forced, and the
kernel skips module refcounting on those modules. Thus we
can minimize the performance impact on the BIG SMP machines.

BTW, of course this requires to update libkmod to support
new MODULE_INIT_LOCKUP_MODULE flag. I'm not sure where is
good to send the patches I have. Sould I better sending
kmod patches on LKML?

Thank you,

---

Masami Hiramatsu (5):
      module: Wait for RCU synchronizing before releasing a module
      module: Unlink module with RCU synchronizing instead of stop_machine
      lib/bug: Use RCU list ops for module_bug_list
      module: Lock up a module when loading with a LOCLUP flag
      module: Remove stop_machine from module unloading


 include/linux/module.h        |   22 ++----
 include/trace/events/module.h |    2 -
 include/uapi/linux/module.h   |    1 
 kernel/module.c               |  147 +++++++++++++++++------------------------
 lib/bug.c                     |   20 ++++--
 5 files changed, 85 insertions(+), 107 deletions(-)

--

[RFC PATCH 1/5] module: Wait for RCU synchronizing before releasing a module

[Masami Hiramatsu]

Wait for RCU synchronizing on failure path of module loading
before releasing struct module, because the memory of mod->list
can still be accessed by list walkers (e.g. kallsyms).

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 kernel/module.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/module.c b/kernel/module.c
index 03214bd2..4c8a4f1 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3324,6 +3324,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	/* Unlink carefully: kallsyms could be walking list. */
 	list_del_rcu(&mod->list);
 	wake_up_all(&module_wq);
+	/* Wait for RCU synchronizing before releasing mod->list. */
+	synchronize_rcu();
 	mutex_unlock(&module_mutex);
  free_module:
 	module_deallocate(mod, info);

[RFC PATCH 2/5] module: Unlink module with RCU synchronizing instead of stop_machine

[Masami Hiramatsu]

Unlink module from module list with RCU synchronizing instead
of using stop_machine(). Since module list is already protected
by rcu, we don't need stop_machine() anymore.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 kernel/module.c |   18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 4c8a4f1..84b068d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1697,18 +1697,6 @@ static void mod_sysfs_teardown(struct module *mod)
 	mod_sysfs_fini(mod);
 }
 
-/*
- * unlink the module with the whole machine is stopped with interrupts off
- * - this defends against kallsyms not taking locks
- */
-static int __unlink_module(void *_mod)
-{
-	struct module *mod = _mod;
-	list_del(&mod->list);
-	module_bug_cleanup(mod);
-	return 0;
-}
-
 #ifdef CONFIG_DEBUG_SET_MODULE_RONX
 /*
  * LKM RO/NX protection: protect module's text/ro-data
@@ -1858,7 +1846,11 @@ static void free_module(struct module *mod)
 
 	/* Now we can delete it from the lists */
 	mutex_lock(&module_mutex);
-	stop_machine(__unlink_module, mod, NULL);
+	/* Unlink carefully: kallsyms could be walking list. */
+	list_del_rcu(&mod->list);
+	/* Wait for RCU synchronizing before releasing mod->list. */
+	synchronize_rcu();
+	module_bug_cleanup(mod);
 	mutex_unlock(&module_mutex);
 
 	/* This may be NULL, but that's OK */

[RFC PATCH 3/5] lib/bug: Use RCU list ops for module_bug_list

[Masami Hiramatsu]

Actually since module_bug_list should be used in BUG context,
we may not need this. But for someone who want to use this
from normal context, this makes module_bug_list an RCU list.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 kernel/module.c |    5 +++--
 lib/bug.c       |   20 ++++++++++++++------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 84b068d..3bd0dc9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1848,9 +1848,10 @@ static void free_module(struct module *mod)
 	mutex_lock(&module_mutex);
 	/* Unlink carefully: kallsyms could be walking list. */
 	list_del_rcu(&mod->list);
-	/* Wait for RCU synchronizing before releasing mod->list. */
-	synchronize_rcu();
+	/* Remove this module from bug list, this uses list_del_rcu */
 	module_bug_cleanup(mod);
+	/* Wait for RCU synchronizing before releasing mod->list and buglist. */
+	synchronize_rcu();
 	mutex_unlock(&module_mutex);
 
 	/* This may be NULL, but that's OK */
diff --git a/lib/bug.c b/lib/bug.c
index d1d7c78..0c3bd95 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -64,16 +64,22 @@ static LIST_HEAD(module_bug_list);
 static const struct bug_entry *module_find_bug(unsigned long bugaddr)
 {
 	struct module *mod;
+	const struct bug_entry *bug = NULL;
 
-	list_for_each_entry(mod, &module_bug_list, bug_list) {
-		const struct bug_entry *bug = mod->bug_table;
+	rcu_read_lock();
+	list_for_each_entry_rcu(mod, &module_bug_list, bug_list) {
 		unsigned i;
 
+		bug = mod->bug_table;
 		for (i = 0; i < mod->num_bugs; ++i, ++bug)
 			if (bugaddr == bug_addr(bug))
-				return bug;
+				goto out;
 	}
-	return NULL;
+	bug = NULL;
+out:
+	rcu_read_unlock();
+
+	return bug;
 }
 
 void module_bug_finalize(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs,
@@ -99,13 +105,15 @@ void module_bug_finalize(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs,
 	 * Strictly speaking this should have a spinlock to protect against
 	 * traversals, but since we only traverse on BUG()s, a spinlock
 	 * could potentially lead to deadlock and thus be counter-productive.
+	 * Thus, this uses RCU to safely manipulate the bug list, since BUG
+	 * must run in non-interruptive state.
 	 */
-	list_add(&mod->bug_list, &module_bug_list);
+	list_add_rcu(&mod->bug_list, &module_bug_list);
 }
 
 void module_bug_cleanup(struct module *mod)
 {
-	list_del(&mod->bug_list);
+	list_del_rcu(&mod->bug_list);
 }
 
 #else

[RFC PATCH 4/5] module: Lock up a module when loading with a LOCLUP flag

[Masami Hiramatsu]

Lock-up a module in kernel so that it is not removed unless
forcibly unload. This is done by loading a module with
MODULE_INIT_LOCKUP_MODULE flag (via finit_module).
This speeds up try_module_get by skipping refcount inc/dec for
the locked modules.

Note that this flag requires to update libkmod to support it.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 include/linux/module.h      |    6 ++++++
 include/uapi/linux/module.h |    1 +
 kernel/module.c             |   28 ++++++++++++++++++++--------
 3 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 71f282a..670cb2e 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -205,6 +205,7 @@ struct module_use {
 
 enum module_state {
 	MODULE_STATE_LIVE,	/* Normal state. */
+	MODULE_STATE_LOCKUP,	/* Module is never removed except forced */
 	MODULE_STATE_COMING,	/* Full formed, running module_init. */
 	MODULE_STATE_GOING,	/* Going away. */
 	MODULE_STATE_UNFORMED,	/* Still setting it up. */
@@ -390,6 +391,11 @@ static inline int module_is_live(struct module *mod)
 	return mod->state != MODULE_STATE_GOING;
 }
 
+static inline int module_is_locked(struct module *mod)
+{
+	return mod->state == MODULE_STATE_LOCKUP;
+}
+
 struct module *__module_text_address(unsigned long addr);
 struct module *__module_address(unsigned long addr);
 bool is_module_address(unsigned long addr);
diff --git a/include/uapi/linux/module.h b/include/uapi/linux/module.h
index 38da425..8195603 100644
--- a/include/uapi/linux/module.h
+++ b/include/uapi/linux/module.h
@@ -4,5 +4,6 @@
 /* Flags for sys_finit_module: */
 #define MODULE_INIT_IGNORE_MODVERSIONS	1
 #define MODULE_INIT_IGNORE_VERMAGIC	2
+#define MODULE_INIT_LOCKUP_MODULE	4
 
 #endif /* _UAPI_LINUX_MODULE_H */
diff --git a/kernel/module.c b/kernel/module.c
index 3bd0dc9..85ffc1d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -753,7 +753,7 @@ static int __try_stop_module(void *_sref)
 	struct stopref *sref = _sref;
 
 	/* If it's not unused, quit unless we're forcing. */
-	if (module_refcount(sref->mod) != 0) {
+	if (module_is_locked(sref->mod) || module_refcount(sref->mod) != 0) {
 		if (!(*sref->forced = try_force_unload(sref->flags)))
 			return -EWOULDBLOCK;
 	}
@@ -830,7 +830,8 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 	}
 
 	/* Doing init or already dying? */
-	if (mod->state != MODULE_STATE_LIVE) {
+	if (mod->state != MODULE_STATE_LIVE &&
+	    mod->state != MODULE_STATE_LOCKUP) {
 		/* FIXME: if (force), slam module count damn the torpedoes */
 		pr_debug("%s already dying\n", mod->name);
 		ret = -EBUSY;
@@ -947,6 +948,9 @@ bool try_module_get(struct module *module)
 	bool ret = true;
 
 	if (module) {
+		if (module_is_locked(module))
+			goto end;
+
 		preempt_disable();
 
 		if (likely(module_is_live(module))) {
@@ -957,13 +961,14 @@ bool try_module_get(struct module *module)
 
 		preempt_enable();
 	}
+end:
 	return ret;
 }
 EXPORT_SYMBOL(try_module_get);
 
 void module_put(struct module *module)
 {
-	if (module) {
+	if (module && !module_is_locked(module)) {
 		preempt_disable();
 		smp_wmb(); /* see comment in module_refcount */
 		__this_cpu_inc(module->refptr->decs);
@@ -1026,6 +1031,7 @@ static ssize_t show_initstate(struct module_attribute *mattr,
 
 	switch (mk->mod->state) {
 	case MODULE_STATE_LIVE:
+	case MODULE_STATE_LOCKUP:
 		state = "live";
 		break;
 	case MODULE_STATE_COMING:
@@ -2981,6 +2987,7 @@ static bool finished_loading(const char *name)
 	mutex_lock(&module_mutex);
 	mod = find_module_all(name, strlen(name), true);
 	ret = !mod || mod->state == MODULE_STATE_LIVE
+		|| mod->state == MODULE_STATE_LOCKUP
 		|| mod->state == MODULE_STATE_GOING;
 	mutex_unlock(&module_mutex);
 
@@ -2999,7 +3006,7 @@ static void do_mod_ctors(struct module *mod)
 }
 
 /* This is where the real work happens */
-static int do_init_module(struct module *mod)
+static int do_init_module(struct module *mod, bool locked)
 {
 	int ret = 0;
 
@@ -3034,7 +3041,7 @@ static int do_init_module(struct module *mod)
 	}
 
 	/* Now it's a first class citizen! */
-	mod->state = MODULE_STATE_LIVE;
+	mod->state = locked ? MODULE_STATE_LOCKUP : MODULE_STATE_LIVE;
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_LIVE, mod);
 
@@ -3290,7 +3297,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	/* Done! */
 	trace_module_load(mod);
 
-	return do_init_module(mod);
+	return do_init_module(mod, flags & MODULE_INIT_LOCKUP_MODULE);
 
  bug_cleanup:
 	/* module_bug_cleanup needs module_mutex protection */
@@ -3358,8 +3365,9 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
 
 	pr_debug("finit_module: fd=%d, uargs=%p, flags=%i\n", fd, uargs, flags);
 
-	if (flags & ~(MODULE_INIT_IGNORE_MODVERSIONS
-		      |MODULE_INIT_IGNORE_VERMAGIC))
+	if (flags & ~(MODULE_INIT_IGNORE_MODVERSIONS |
+		      MODULE_INIT_IGNORE_VERMAGIC |
+		      MODULE_INIT_LOCKUP_MODULE))
 		return -EINVAL;
 
 	err = copy_module_from_fd(fd, &info);
@@ -3602,10 +3610,14 @@ static char *module_flags(struct module *mod, char *buf)
 
 	BUG_ON(mod->state == MODULE_STATE_UNFORMED);
 	if (mod->taints ||
+	    mod->state == MODULE_STATE_LOCKUP ||
 	    mod->state == MODULE_STATE_GOING ||
 	    mod->state == MODULE_STATE_COMING) {
 		buf[bx++] = '(';
 		bx += module_flags_taint(mod, buf + bx);
+		/* Show a - for module-is-locked */
+		if (mod->state == MODULE_STATE_LOCKUP)
+			buf[bx++] = '*';
 		/* Show a - for module-is-being-unloaded */
 		if (mod->state == MODULE_STATE_GOING)
 			buf[bx++] = '-';

[RFC PATCH 5/5] module: Remove stop_machine from module unloading

[Masami Hiramatsu]

Remove stop_machine from module unloading by replacing module_ref
with atomic_t. Note that this can cause a performance regression
on big-SMP machine by direct memory access. For those machines,
you can lockdwon all modules. Since the lockdown skips reference
counting, it'll be more scalable than per-cpu module_ref counters.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/module.h        |   16 ------
 include/trace/events/module.h |    2 -
 kernel/module.c               |  108 +++++++++++++++--------------------------
 3 files changed, 41 insertions(+), 85 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 670cb2e..3ebe049 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -211,20 +211,6 @@ enum module_state {
 	MODULE_STATE_UNFORMED,	/* Still setting it up. */
 };
 
-/**
- * struct module_ref - per cpu module reference counts
- * @incs: number of module get on this cpu
- * @decs: number of module put on this cpu
- *
- * We force an alignment on 8 or 16 bytes, so that alloc_percpu()
- * put @incs/@decs in same cache line, with no extra memory cost,
- * since alloc_percpu() is fine grained.
- */
-struct module_ref {
-	unsigned long incs;
-	unsigned long decs;
-} __attribute((aligned(2 * sizeof(unsigned long))));
-
 struct module {
 	enum module_state state;
 
@@ -368,7 +354,7 @@ struct module {
 	/* Destruction function. */
 	void (*exit)(void);
 
-	struct module_ref __percpu *refptr;
+	atomic_t refcnt;
 #endif
 
 #ifdef CONFIG_CONSTRUCTORS
diff --git a/include/trace/events/module.h b/include/trace/events/module.h
index 7c5cbfe..81c4c18 100644
--- a/include/trace/events/module.h
+++ b/include/trace/events/module.h
@@ -80,7 +80,7 @@ DECLARE_EVENT_CLASS(module_refcnt,
 
 	TP_fast_assign(
 		__entry->ip	= ip;
-		__entry->refcnt	= __this_cpu_read(mod->refptr->incs) - __this_cpu_read(mod->refptr->decs);
+		__entry->refcnt	= atomic_read(&mod->refcnt);
 		__assign_str(name, mod->name);
 	),
 
diff --git a/kernel/module.c b/kernel/module.c
index 85ffc1d..7af6ff7 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -42,7 +42,6 @@
 #include <linux/vermagic.h>
 #include <linux/notifier.h>
 #include <linux/sched.h>
-#include <linux/stop_machine.h>
 #include <linux/device.h>
 #include <linux/string.h>
 #include <linux/mutex.h>
@@ -98,7 +97,7 @@
  * 1) List of modules (also safely readable with preempt_disable),
  * 2) module_use links,
  * 3) module_addr_min/module_addr_max.
- * (delete uses stop_machine/add uses RCU list operations). */
+ * (delete and add uses RCU list operations). */
 DEFINE_MUTEX(module_mutex);
 EXPORT_SYMBOL_GPL(module_mutex);
 static LIST_HEAD(modules);
@@ -628,18 +627,26 @@ static char last_unloaded_module[MODULE_NAME_LEN+1];
 
 EXPORT_TRACEPOINT_SYMBOL(module_get);
 
+/*
+ * MODULE_REF_BASE must be 1, since we use atomic_inc_not_zero() for
+ * recovering refcnt (see try_release_module_ref() ).
+ */
+#define MODULE_REF_BASE	1
+
 /* Init the unload section of the module. */
 static int module_unload_init(struct module *mod)
 {
-	mod->refptr = alloc_percpu(struct module_ref);
-	if (!mod->refptr)
-		return -ENOMEM;
+	/*
+	 * Initialize reference counter to MODULE_REF_BASE.
+	 * refcnt == 0 means module is going.
+	 */
+	atomic_set(&mod->refcnt, MODULE_REF_BASE);
 
 	INIT_LIST_HEAD(&mod->source_list);
 	INIT_LIST_HEAD(&mod->target_list);
 
 	/* Hold reference count during initialization. */
-	raw_cpu_write(mod->refptr->incs, 1);
+	atomic_inc(&mod->refcnt);
 
 	return 0;
 }
@@ -721,8 +728,6 @@ static void module_unload_free(struct module *mod)
 		kfree(use);
 	}
 	mutex_unlock(&module_mutex);
-
-	free_percpu(mod->refptr);
 }
 
 #ifdef CONFIG_MODULE_FORCE_UNLOAD
@@ -740,60 +745,38 @@ static inline int try_force_unload(unsigned int flags)
 }
 #endif /* CONFIG_MODULE_FORCE_UNLOAD */
 
-struct stopref
+/* Try to release refcount of module, 0 means success. */
+static int try_release_module_ref(struct module *mod)
 {
-	struct module *mod;
-	int flags;
-	int *forced;
-};
+	int ret;
 
-/* Whole machine is stopped with interrupts off when this runs. */
-static int __try_stop_module(void *_sref)
-{
-	struct stopref *sref = _sref;
+	/* Try to decrement refcnt which we set at loading */
+	ret = atomic_sub_return(MODULE_REF_BASE, &mod->refcnt);
+	if (ret)
+		/* Someone can put this right now, recover with checking */
+		ret = atomic_inc_not_zero(&mod->refcnt);
+
+	return ret;
+}
 
+static int try_stop_module(struct module *mod, int flags, int *forced)
+{
 	/* If it's not unused, quit unless we're forcing. */
-	if (module_is_locked(sref->mod) || module_refcount(sref->mod) != 0) {
-		if (!(*sref->forced = try_force_unload(sref->flags)))
+	if (module_is_locked(mod) || try_release_module_ref(mod) != 0) {
+		*forced = try_force_unload(flags);
+		if (!(*forced))
 			return -EWOULDBLOCK;
 	}
 
 	/* Mark it as dying. */
-	sref->mod->state = MODULE_STATE_GOING;
-	return 0;
-}
+	mod->state = MODULE_STATE_GOING;
 
-static int try_stop_module(struct module *mod, int flags, int *forced)
-{
-	struct stopref sref = { mod, flags, forced };
-
-	return stop_machine(__try_stop_module, &sref, NULL);
+	return 0;
 }
 
 unsigned long module_refcount(struct module *mod)
 {
-	unsigned long incs = 0, decs = 0;
-	int cpu;
-
-	for_each_possible_cpu(cpu)
-		decs += per_cpu_ptr(mod->refptr, cpu)->decs;
-	/*
-	 * ensure the incs are added up after the decs.
-	 * module_put ensures incs are visible before decs with smp_wmb.
-	 *
-	 * This 2-count scheme avoids the situation where the refcount
-	 * for CPU0 is read, then CPU0 increments the module refcount,
-	 * then CPU1 drops that refcount, then the refcount for CPU1 is
-	 * read. We would record a decrement but not its corresponding
-	 * increment so we would see a low count (disaster).
-	 *
-	 * Rare situation? But module_refcount can be preempted, and we
-	 * might be tallying up 4096+ CPUs. So it is not impossible.
-	 */
-	smp_rmb();
-	for_each_possible_cpu(cpu)
-		incs += per_cpu_ptr(mod->refptr, cpu)->incs;
-	return incs - decs;
+	return (unsigned long)atomic_read(&mod->refcnt) - MODULE_REF_BASE;
 }
 EXPORT_SYMBOL(module_refcount);
 
@@ -935,10 +918,8 @@ static struct module_attribute modinfo_refcnt =
 void __module_get(struct module *module)
 {
 	if (module) {
-		preempt_disable();
-		__this_cpu_inc(module->refptr->incs);
+		atomic_inc(&module->refcnt);
 		trace_module_get(module, _RET_IP_);
-		preempt_enable();
 	}
 }
 EXPORT_SYMBOL(__module_get);
@@ -947,21 +928,14 @@ bool try_module_get(struct module *module)
 {
 	bool ret = true;
 
-	if (module) {
-		if (module_is_locked(module))
-			goto end;
-
-		preempt_disable();
-
-		if (likely(module_is_live(module))) {
-			__this_cpu_inc(module->refptr->incs);
+	if (module && !module_is_locked(module)) {
+		if (module_is_live(module) &&
+		    atomic_inc_not_zero(&module->refcnt) != 0)
 			trace_module_get(module, _RET_IP_);
-		} else
+		else
 			ret = false;
-
-		preempt_enable();
 	}
-end:
+
 	return ret;
 }
 EXPORT_SYMBOL(try_module_get);
@@ -969,12 +943,8 @@ EXPORT_SYMBOL(try_module_get);
 void module_put(struct module *module)
 {
 	if (module && !module_is_locked(module)) {
-		preempt_disable();
-		smp_wmb(); /* see comment in module_refcount */
-		__this_cpu_inc(module->refptr->decs);
-
+		atomic_dec(&module->refcnt);
 		trace_module_put(module, _RET_IP_);
-		preempt_enable();
 	}
 }
 EXPORT_SYMBOL(module_put);

Re: [RFC PATCH 4/5] module: Lock up a module when loading with a LOCLUP flag

[Lucas De Marchi]

On Mon, Aug 25, 2014 at 10:55:48AM +0000, Masami Hiramatsu wrote:
> Lock-up a module in kernel so that it is not removed unless
> forcibly unload. This is done by loading a module with
> MODULE_INIT_LOCKUP_MODULE flag (via finit_module).
> This speeds up try_module_get by skipping refcount inc/dec for
> the locked modules.
> 
> Note that this flag requires to update libkmod to support it.

Patches to kmod go to linux-modules@vger.kernel.org

However I'm skeptical with the use case of this flag. Is this only so
that try_module_get() is a little bit faster? How much?

Then this would depend on a flag the user passed to modprobe which means
only a few modules will use the flag. If you change the default
behavior in kmod to pass this flag always, then any module the user
wants to remove will need to be forcibly removed.

I'm leaving the rest of the patch here since I'm CC'ing linux-modules.

-- 
Lucas De Marchi


> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
>  include/linux/module.h      |    6 ++++++
>  include/uapi/linux/module.h |    1 +
>  kernel/module.c             |   28 ++++++++++++++++++++--------
>  3 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 71f282a..670cb2e 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -205,6 +205,7 @@ struct module_use {
>  
>  enum module_state {
>  	MODULE_STATE_LIVE,	/* Normal state. */
> +	MODULE_STATE_LOCKUP,	/* Module is never removed except forced */
>  	MODULE_STATE_COMING,	/* Full formed, running module_init. */
>  	MODULE_STATE_GOING,	/* Going away. */
>  	MODULE_STATE_UNFORMED,	/* Still setting it up. */
> @@ -390,6 +391,11 @@ static inline int module_is_live(struct module *mod)
>  	return mod->state != MODULE_STATE_GOING;
>  }
>  
> +static inline int module_is_locked(struct module *mod)
> +{
> +	return mod->state == MODULE_STATE_LOCKUP;
> +}
> +
>  struct module *__module_text_address(unsigned long addr);
>  struct module *__module_address(unsigned long addr);
>  bool is_module_address(unsigned long addr);
> diff --git a/include/uapi/linux/module.h b/include/uapi/linux/module.h
> index 38da425..8195603 100644
> --- a/include/uapi/linux/module.h
> +++ b/include/uapi/linux/module.h
> @@ -4,5 +4,6 @@
>  /* Flags for sys_finit_module: */
>  #define MODULE_INIT_IGNORE_MODVERSIONS	1
>  #define MODULE_INIT_IGNORE_VERMAGIC	2
> +#define MODULE_INIT_LOCKUP_MODULE	4
>  
>  #endif /* _UAPI_LINUX_MODULE_H */
> diff --git a/kernel/module.c b/kernel/module.c
> index 3bd0dc9..85ffc1d 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -753,7 +753,7 @@ static int __try_stop_module(void *_sref)
>  	struct stopref *sref = _sref;
>  
>  	/* If it's not unused, quit unless we're forcing. */
> -	if (module_refcount(sref->mod) != 0) {
> +	if (module_is_locked(sref->mod) || module_refcount(sref->mod) != 0) {
>  		if (!(*sref->forced = try_force_unload(sref->flags)))
>  			return -EWOULDBLOCK;
>  	}
> @@ -830,7 +830,8 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>  	}
>  
>  	/* Doing init or already dying? */
> -	if (mod->state != MODULE_STATE_LIVE) {
> +	if (mod->state != MODULE_STATE_LIVE &&
> +	    mod->state != MODULE_STATE_LOCKUP) {
>  		/* FIXME: if (force), slam module count damn the torpedoes */
>  		pr_debug("%s already dying\n", mod->name);
>  		ret = -EBUSY;
> @@ -947,6 +948,9 @@ bool try_module_get(struct module *module)
>  	bool ret = true;
>  
>  	if (module) {
> +		if (module_is_locked(module))
> +			goto end;
> +
>  		preempt_disable();
>  
>  		if (likely(module_is_live(module))) {
> @@ -957,13 +961,14 @@ bool try_module_get(struct module *module)
>  
>  		preempt_enable();
>  	}
> +end:
>  	return ret;
>  }
>  EXPORT_SYMBOL(try_module_get);
>  
>  void module_put(struct module *module)
>  {
> -	if (module) {
> +	if (module && !module_is_locked(module)) {
>  		preempt_disable();
>  		smp_wmb(); /* see comment in module_refcount */
>  		__this_cpu_inc(module->refptr->decs);
> @@ -1026,6 +1031,7 @@ static ssize_t show_initstate(struct module_attribute *mattr,
>  
>  	switch (mk->mod->state) {
>  	case MODULE_STATE_LIVE:
> +	case MODULE_STATE_LOCKUP:
>  		state = "live";
>  		break;
>  	case MODULE_STATE_COMING:
> @@ -2981,6 +2987,7 @@ static bool finished_loading(const char *name)
>  	mutex_lock(&module_mutex);
>  	mod = find_module_all(name, strlen(name), true);
>  	ret = !mod || mod->state == MODULE_STATE_LIVE
> +		|| mod->state == MODULE_STATE_LOCKUP
>  		|| mod->state == MODULE_STATE_GOING;
>  	mutex_unlock(&module_mutex);
>  
> @@ -2999,7 +3006,7 @@ static void do_mod_ctors(struct module *mod)
>  }
>  
>  /* This is where the real work happens */
> -static int do_init_module(struct module *mod)
> +static int do_init_module(struct module *mod, bool locked)
>  {
>  	int ret = 0;
>  
> @@ -3034,7 +3041,7 @@ static int do_init_module(struct module *mod)
>  	}
>  
>  	/* Now it's a first class citizen! */
> -	mod->state = MODULE_STATE_LIVE;
> +	mod->state = locked ? MODULE_STATE_LOCKUP : MODULE_STATE_LIVE;
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_LIVE, mod);
>  
> @@ -3290,7 +3297,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	/* Done! */
>  	trace_module_load(mod);
>  
> -	return do_init_module(mod);
> +	return do_init_module(mod, flags & MODULE_INIT_LOCKUP_MODULE);
>  
>   bug_cleanup:
>  	/* module_bug_cleanup needs module_mutex protection */
> @@ -3358,8 +3365,9 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
>  
>  	pr_debug("finit_module: fd=%d, uargs=%p, flags=%i\n", fd, uargs, flags);
>  
> -	if (flags & ~(MODULE_INIT_IGNORE_MODVERSIONS
> -		      |MODULE_INIT_IGNORE_VERMAGIC))
> +	if (flags & ~(MODULE_INIT_IGNORE_MODVERSIONS |
> +		      MODULE_INIT_IGNORE_VERMAGIC |
> +		      MODULE_INIT_LOCKUP_MODULE))
>  		return -EINVAL;
>  
>  	err = copy_module_from_fd(fd, &info);
> @@ -3602,10 +3610,14 @@ static char *module_flags(struct module *mod, char *buf)
>  
>  	BUG_ON(mod->state == MODULE_STATE_UNFORMED);
>  	if (mod->taints ||
> +	    mod->state == MODULE_STATE_LOCKUP ||
>  	    mod->state == MODULE_STATE_GOING ||
>  	    mod->state == MODULE_STATE_COMING) {
>  		buf[bx++] = '(';
>  		bx += module_flags_taint(mod, buf + bx);
> +		/* Show a - for module-is-locked */
> +		if (mod->state == MODULE_STATE_LOCKUP)
> +			buf[bx++] = '*';
>  		/* Show a - for module-is-being-unloaded */
>  		if (mod->state == MODULE_STATE_GOING)
>  			buf[bx++] = '-';
> 
> 

Re: Re: [RFC PATCH 4/5] module: Lock up a module when loading with a LOCLUP flag

[Masami Hiramatsu]

(2014/08/26 14:30), Lucas De Marchi wrote:
> On Mon, Aug 25, 2014 at 10:55:48AM +0000, Masami Hiramatsu wrote:
>> Lock-up a module in kernel so that it is not removed unless
>> forcibly unload. This is done by loading a module with
>> MODULE_INIT_LOCKUP_MODULE flag (via finit_module).
>> This speeds up try_module_get by skipping refcount inc/dec for
>> the locked modules.
>>
>> Note that this flag requires to update libkmod to support it.
> 
> Patches to kmod go to linux-modules@vger.kernel.org

OK, thanks. I'll send another series for it.

> However I'm skeptical with the use case of this flag. Is this only so
> that try_module_get() is a little bit faster? How much?

For the performance side of refcounting, I guess it has no recognizable
difference compared with current one. It may be a little faster than
atomic_t ref-counter, and actual overhead will strongly depends on the
hardware configuration.
So, I can just drop this "lockup" patch from this series, if nobody
complains about using atomic_t ref-counter instead of module_ref.
I'd just like to get rid of the stop_machine() from unloading :)

> Then this would depend on a flag the user passed to modprobe which means
> only a few modules will use the flag. If you change the default
> behavior in kmod to pass this flag always, then any module the user
> wants to remove will need to be forcibly removed.

I saw an environmental variable to control it (MODPROBE_OPTIONS).
So if a user (e.g. systemtap :) ) want to make it removable, he/she
can change the env-var before running the command.

> 
> I'm leaving the rest of the patch here since I'm CC'ing linux-modules.
> 

Thank you,

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

[RFC PATCH 0/2] kmod: Support lockup option to make module un-removable

[Masami Hiramatsu]

Hi,

Here is a pair of patches which adds --lockup option to
modprobe and libkmod.

As I sent a series of patches which removes stop_machine()
from module removal: https://lkml.org/lkml/2014/8/25/142
it also adds lockup option which lock up the module in
the kernel and makes it un-removable.

These patches enables us to use that option when loading
modules. Module lockup may be good for BIG SMP machines
since the kernel skips module refcounting if the module
is locked up :)

Anyway, this is not needed if the lockup option is dropped
from the series. I send this for testing.

Thank you,

---

Masami Hiramatsu (2):
      libkmod: support lockup module option
      modprobe: Add --lockup option to make module unremovable


 libkmod/libkmod-module.c |    3 +++
 libkmod/libkmod.h        |    4 +++-
 libkmod/missing.h        |    4 ++++
 tools/modprobe.c         |   10 +++++++++-
 4 files changed, 19 insertions(+), 2 deletions(-)

--

[RFC PATCH 1/2] libkmod: support lockup module option

[Masami Hiramatsu]

Add lockup option support to load an unloadable module.
Since unloadable module is not removed normally, module refcounting
is skipped. That will improve multi-core scalability on big machine.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 libkmod/libkmod-module.c |    3 +++
 libkmod/libkmod.h        |    4 +++-
 libkmod/missing.h        |    4 ++++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index b81b451..557e637 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -789,6 +789,7 @@ extern long init_module(const void *mem, unsigned long len, const char *args);
  * behavior of this function, valid flags are
  * KMOD_INSERT_FORCE_VERMAGIC: ignore kernel version magic;
  * KMOD_INSERT_FORCE_MODVERSION: ignore symbol version hashes.
+ * KMOD_INSERT_LOCKUP_MODULE: make module unloadable.
  * @options: module's options to pass to Linux Kernel.
  *
  * Insert a module in Linux kernel. It opens the file pointed by @mod,
@@ -830,6 +831,8 @@ KMOD_EXPORT int kmod_module_insert_module(struct kmod_module *mod,
 			kernel_flags |= MODULE_INIT_IGNORE_VERMAGIC;
 		if (flags & KMOD_INSERT_FORCE_MODVERSION)
 			kernel_flags |= MODULE_INIT_IGNORE_MODVERSIONS;
+		if (flags & KMOD_INSERT_LOCKUP_MODULE)
+			kernel_flags |= MODULE_INIT_LOCKUP;
 
 		err = finit_module(kmod_file_get_fd(mod->file), args, kernel_flags);
 		if (err == 0 || errno != ENOSYS)
diff --git a/libkmod/libkmod.h b/libkmod/libkmod.h
index a7ea221..69a3237 100644
--- a/libkmod/libkmod.h
+++ b/libkmod/libkmod.h
@@ -143,10 +143,11 @@ enum kmod_remove {
 	KMOD_REMOVE_NOWAIT = O_NONBLOCK, /* always set */
 };
 
-/* Insertion flags */
+/* Insertion flags: must be same as KMOD_PROBE_* */
 enum kmod_insert {
 	KMOD_INSERT_FORCE_VERMAGIC = 0x1,
 	KMOD_INSERT_FORCE_MODVERSION = 0x2,
+	KMOD_INSERT_LOCKUP_MODULE = 0x40,
 };
 
 /* Flags to kmod_module_probe_insert_module() */
@@ -157,6 +158,7 @@ enum kmod_probe {
 	KMOD_PROBE_IGNORE_LOADED =		0x00008,
 	KMOD_PROBE_DRY_RUN =			0x00010,
 	KMOD_PROBE_FAIL_ON_LOADED =		0x00020,
+	KMOD_PROBE_LOCKUP_MODULE =		0x00040,
 
 	/* codes below can be used in return value, too */
 	KMOD_PROBE_APPLY_BLACKLIST_ALL =	0x10000,
diff --git a/libkmod/missing.h b/libkmod/missing.h
index 8d47af8..c38f588 100644
--- a/libkmod/missing.h
+++ b/libkmod/missing.h
@@ -15,6 +15,10 @@
 # define MODULE_INIT_IGNORE_VERMAGIC 2
 #endif
 
+#ifndef MODULE_INIT_LOCKUP
+# define MODULE_INIT_LOCKUP 4
+#endif
+
 #ifndef __NR_finit_module
 # define __NR_finit_module -1
 #endif

[RFC PATCH 2/2] modprobe: Add --lockup option to make module unremovable

[Masami Hiramatsu]

From: Masami Hiramatsu <masami.hiramatsu@gmail.com>

Add --lockup option for loading to make the module unremovable.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 tools/modprobe.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/modprobe.c b/tools/modprobe.c
index 6b34658..768a5bb 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -51,12 +51,13 @@ static int first_time = 0;
 static int ignore_commands = 0;
 static int use_blacklist = 0;
 static int force = 0;
+static int lockup = 0;
 static int strip_modversion = 0;
 static int strip_vermagic = 0;
 static int remove_dependencies = 0;
 static int quiet_inuse = 0;
 
-static const char cmdopts_s[] = "arRibfDcnC:d:S:sqvVh";
+static const char cmdopts_s[] = "arRibflDcnC:d:S:sqvVh";
 static const struct option cmdopts[] = {
 	{"all", no_argument, 0, 'a'},
 	{"remove", no_argument, 0, 'r'},
@@ -69,6 +70,7 @@ static const struct option cmdopts[] = {
 	{"force", no_argument, 0, 'f'},
 	{"force-modversion", no_argument, 0, 2},
 	{"force-vermagic", no_argument, 0, 1},
+	{"lockup", no_argument, 0, 'l'},
 
 	{"show-depends", no_argument, 0, 'D'},
 	{"showconfig", no_argument, 0, 'c'},
@@ -116,6 +118,7 @@ static void help(void)
 		"\t                            --force-vermagic\n"
 		"\t    --force-modversion      Ignore module's version\n"
 		"\t    --force-vermagic        Ignore module's version magic\n"
+		"\t-l, --lockup                Make module to unremovable.\n"
 		"\n"
 		"Query Options:\n"
 		"\t-D, --show-depends          Only print module dependencies and exit\n"
@@ -526,6 +529,8 @@ static int insmod(struct kmod_ctx *ctx, const char *alias,
 		flags |= KMOD_PROBE_FORCE_MODVERSION;
 	if (strip_vermagic || force)
 		flags |= KMOD_PROBE_FORCE_VERMAGIC;
+	if (lockup)
+		flags |= KMOD_PROBE_LOCKUP_MODULE;
 	if (ignore_commands)
 		flags |= KMOD_PROBE_IGNORE_COMMAND;
 	if (ignore_loaded)
@@ -798,6 +803,9 @@ static int do_modprobe(int argc, char **orig_argv)
 		case 1:
 			strip_vermagic = 1;
 			break;
+		case 'l':
+			lockup = 1;
+			break;
 		case 'D':
 			ignore_loaded = 1;
 			dry_run = 1;

Re: [RFC PATCH 0/2] kmod: Support lockup option to make module un-removable

[Lucas De Marchi]

On Tue, Aug 26, 2014 at 9:04 AM, Masami Hiramatsu
<masami.hiramatsu.pt@hitachi.com> wrote:
> Hi,
>
> Here is a pair of patches which adds --lockup option to
> modprobe and libkmod.
>
> As I sent a series of patches which removes stop_machine()
> from module removal: https://lkml.org/lkml/2014/8/25/142
> it also adds lockup option which lock up the module in
> the kernel and makes it un-removable.
>
> These patches enables us to use that option when loading
> modules. Module lockup may be good for BIG SMP machines
> since the kernel skips module refcounting if the module
> is locked up :)
>
> Anyway, this is not needed if the lockup option is dropped
> from the series. I send this for testing.

Ok. I'm not sure it's clear... I'm waiting for feedback on the kernel
patches in order to proceed with any review here. I'm not really
convinced we want this option when loading a module.

Rusty, what do you think?

-- 
Lucas De Marchi

Re: [RFC PATCH 0/5] module: Remove stop_machine from module unloading

[Masami Hiramatsu]

Ping? :)

(2014/08/25 19:55), Masami Hiramatsu wrote:
> Hi,
> 
> Here is a series of patches which remove stop_machine() from
> module unloading.
> 
> Currently, each module unloading calls stop_machine()s 2 times.
> One is for safely removing module from lists and one is to
> check the reference counter. However, both are not necessary
> for those purposes (required by current implementation).
> 
> First, we can use RCU for the list operation, we just need
> a synchronize_rcu right before cleaning up.
> Second, the reference counter can be checked atomically by
> using atomic_t, instead of per-cpu module_ref. Of course,
> for BIG SMP machines, atomic operation is not efficient.
> However, they usually don't need to remove most of modules too.
> 
> In this series, I just fixed to use RCU for the module(and
> bugs) list for the first stop_machine.
> And for the second one, I replaced module_ref with atomic_t
> and introduced a "module lockup" module load option, which
> makes a module un-removable (lock up the module in kernel).
> The lockup modules can not be removed except forced, and the
> kernel skips module refcounting on those modules. Thus we
> can minimize the performance impact on the BIG SMP machines.
> 
> BTW, of course this requires to update libkmod to support
> new MODULE_INIT_LOCKUP_MODULE flag. I'm not sure where is
> good to send the patches I have. Sould I better sending
> kmod patches on LKML?
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (5):
>       module: Wait for RCU synchronizing before releasing a module
>       module: Unlink module with RCU synchronizing instead of stop_machine
>       lib/bug: Use RCU list ops for module_bug_list
>       module: Lock up a module when loading with a LOCLUP flag
>       module: Remove stop_machine from module unloading
> 
> 
>  include/linux/module.h        |   22 ++----
>  include/trace/events/module.h |    2 -
>  include/uapi/linux/module.h   |    1 
>  kernel/module.c               |  147 +++++++++++++++++------------------------
>  lib/bug.c                     |   20 ++++--
>  5 files changed, 85 insertions(+), 107 deletions(-)
> 
> --
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [RFC PATCH 5/5] module: Remove stop_machine from module unloading

[Rusty Russell]

Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> writes:
> Remove stop_machine from module unloading by replacing module_ref
> with atomic_t. Note that this can cause a performance regression
> on big-SMP machine by direct memory access. For those machines,
> you can lockdwon all modules. Since the lockdown skips reference
> counting, it'll be more scalable than per-cpu module_ref counters.

Sorry for the delay; I didn't get to this before I left, and then
I was away for 3 weeks vacation.

First, I agree you should drop the MODULE_STATE_LOCKUP patch.  While I
can't audit every try_module_get() call, I did put an mdelay(100) in
there and did a quick boot for any obvious slowdown.

Second, this patch should be split into two parts.  The first would
simply replace module_ref with atomic_t (a significant simplification),
the second would get rid of stop machine.

> +/*
> + * MODULE_REF_BASE must be 1, since we use atomic_inc_not_zero() for
> + * recovering refcnt (see try_release_module_ref() ).
> + */
> +#define MODULE_REF_BASE	1

True, but we could use atomic_add_unless() instead, and make this
completely generic, right?

> +
>  /* Init the unload section of the module. */
>  static int module_unload_init(struct module *mod)
>  {
> -	mod->refptr = alloc_percpu(struct module_ref);
> -	if (!mod->refptr)
> -		return -ENOMEM;
> +	/*
> +	 * Initialize reference counter to MODULE_REF_BASE.
> +	 * refcnt == 0 means module is going.
> +	 */
> +	atomic_set(&mod->refcnt, MODULE_REF_BASE);
>  
>  	INIT_LIST_HEAD(&mod->source_list);
>  	INIT_LIST_HEAD(&mod->target_list);
>  
>  	/* Hold reference count during initialization. */
> -	raw_cpu_write(mod->refptr->incs, 1);
> +	atomic_inc(&mod->refcnt);
>  
>  	return 0;
>  }
> @@ -721,8 +728,6 @@ static void module_unload_free(struct module *mod)
>  		kfree(use);
>  	}
>  	mutex_unlock(&module_mutex);
> -
> -	free_percpu(mod->refptr);
>  }
>  
>  #ifdef CONFIG_MODULE_FORCE_UNLOAD
> @@ -740,60 +745,38 @@ static inline int try_force_unload(unsigned int flags)
>  }
>  #endif /* CONFIG_MODULE_FORCE_UNLOAD */
>  
> -struct stopref
> +/* Try to release refcount of module, 0 means success. */
> +static int try_release_module_ref(struct module *mod)
>  {
> -	struct module *mod;
> -	int flags;
> -	int *forced;
> -};
> +	int ret;
>  
> -/* Whole machine is stopped with interrupts off when this runs. */
> -static int __try_stop_module(void *_sref)
> -{
> -	struct stopref *sref = _sref;
> +	/* Try to decrement refcnt which we set at loading */
> +	ret = atomic_sub_return(MODULE_REF_BASE, &mod->refcnt);
> +	if (ret)
> +		/* Someone can put this right now, recover with checking */
> +		ret = atomic_inc_not_zero(&mod->refcnt);
> +
> +	return ret;
> +}

This is very clever!  I really like it.

Thanks,
Rusty.

Re: [RFC PATCH 0/2] kmod: Support lockup option to make module un-removable

[Rusty Russell]

Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
> On Tue, Aug 26, 2014 at 9:04 AM, Masami Hiramatsu
> <masami.hiramatsu.pt@hitachi.com> wrote:
>> Hi,
>>
>> Here is a pair of patches which adds --lockup option to
>> modprobe and libkmod.
>>
>> As I sent a series of patches which removes stop_machine()
>> from module removal: https://lkml.org/lkml/2014/8/25/142
>> it also adds lockup option which lock up the module in
>> the kernel and makes it un-removable.
>>
>> These patches enables us to use that option when loading
>> modules. Module lockup may be good for BIG SMP machines
>> since the kernel skips module refcounting if the module
>> is locked up :)
>>
>> Anyway, this is not needed if the lockup option is dropped
>> from the series. I send this for testing.
>
> Ok. I'm not sure it's clear... I'm waiting for feedback on the kernel
> patches in order to proceed with any review here. I'm not really
> convinced we want this option when loading a module.
>
> Rusty, what do you think?

I'm not convinced, I asked him to drop that patch.  If we have
significant performance issues, we'll have to do something smarter I
think anyway.

Cheers,
Rusty.

Re: Re: [RFC PATCH 5/5] module: Remove stop_machine from module unloading

[Masami Hiramatsu]

(2014/10/13 13:40), Rusty Russell wrote:
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> writes:
>> Remove stop_machine from module unloading by replacing module_ref
>> with atomic_t. Note that this can cause a performance regression
>> on big-SMP machine by direct memory access. For those machines,
>> you can lockdwon all modules. Since the lockdown skips reference
>> counting, it'll be more scalable than per-cpu module_ref counters.
> 
> Sorry for the delay; I didn't get to this before I left, and then
> I was away for 3 weeks vacation.
> 
> First, I agree you should drop the MODULE_STATE_LOCKUP patch.  While I
> can't audit every try_module_get() call, I did put an mdelay(100) in
> there and did a quick boot for any obvious slowdown.

OK, I might be thinking too much about performance. I'll drop it :)

> Second, this patch should be split into two parts.  The first would
> simply replace module_ref with atomic_t (a significant simplification),
> the second would get rid of stop machine.

OK, I'll do that.

> 
>> +/*
>> + * MODULE_REF_BASE must be 1, since we use atomic_inc_not_zero() for
>> + * recovering refcnt (see try_release_module_ref() ).
>> + */
>> +#define MODULE_REF_BASE	1
> 
> True, but we could use atomic_add_unless() instead, and make this
> completely generic, right?

Would you mean just replacing atomic_inc_not_zero() with atomic_add_unless()?

> 
>> +
>>  /* Init the unload section of the module. */
>>  static int module_unload_init(struct module *mod)
>>  {
>> -	mod->refptr = alloc_percpu(struct module_ref);
>> -	if (!mod->refptr)
>> -		return -ENOMEM;
>> +	/*
>> +	 * Initialize reference counter to MODULE_REF_BASE.
>> +	 * refcnt == 0 means module is going.
>> +	 */
>> +	atomic_set(&mod->refcnt, MODULE_REF_BASE);
>>  
>>  	INIT_LIST_HEAD(&mod->source_list);
>>  	INIT_LIST_HEAD(&mod->target_list);
>>  
>>  	/* Hold reference count during initialization. */
>> -	raw_cpu_write(mod->refptr->incs, 1);
>> +	atomic_inc(&mod->refcnt);
>>  
>>  	return 0;
>>  }
>> @@ -721,8 +728,6 @@ static void module_unload_free(struct module *mod)
>>  		kfree(use);
>>  	}
>>  	mutex_unlock(&module_mutex);
>> -
>> -	free_percpu(mod->refptr);
>>  }
>>  
>>  #ifdef CONFIG_MODULE_FORCE_UNLOAD
>> @@ -740,60 +745,38 @@ static inline int try_force_unload(unsigned int flags)
>>  }
>>  #endif /* CONFIG_MODULE_FORCE_UNLOAD */
>>  
>> -struct stopref
>> +/* Try to release refcount of module, 0 means success. */
>> +static int try_release_module_ref(struct module *mod)
>>  {
>> -	struct module *mod;
>> -	int flags;
>> -	int *forced;
>> -};
>> +	int ret;
>>  
>> -/* Whole machine is stopped with interrupts off when this runs. */
>> -static int __try_stop_module(void *_sref)
>> -{
>> -	struct stopref *sref = _sref;
>> +	/* Try to decrement refcnt which we set at loading */
>> +	ret = atomic_sub_return(MODULE_REF_BASE, &mod->refcnt);
>> +	if (ret)
>> +		/* Someone can put this right now, recover with checking */
>> +		ret = atomic_inc_not_zero(&mod->refcnt);
>> +
>> +	return ret;
>> +}
> 
> This is very clever!  I really like it.

Thanks!

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: Re: [RFC PATCH 5/5] module: Remove stop_machine from module unloading

[Masami Hiramatsu]

(2014/10/13 13:40), Rusty Russell wrote:
>> +/*
>> + * MODULE_REF_BASE must be 1, since we use atomic_inc_not_zero() for
>> + * recovering refcnt (see try_release_module_ref() ).
>> + */
>> +#define MODULE_REF_BASE	1
> 
> True, but we could use atomic_add_unless() instead, and make this
> completely generic, right?

Ah, I got it. You might mean replacing atomic_inc_not_zero() in try_release_module_ref
with atomic_add_unless(), then we don't need to make MODULE_REF_BASE=1, is that right?

Thank you,


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: Re: [RFC PATCH 5/5] module: Remove stop_machine from module unloading

[Rusty Russell]

Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> writes:
>>> +/*
>>> + * MODULE_REF_BASE must be 1, since we use atomic_inc_not_zero() for
>>> + * recovering refcnt (see try_release_module_ref() ).
>>> + */
>>> +#define MODULE_REF_BASE	1
>> 
>> True, but we could use atomic_add_unless() instead, and make this
>> completely generic, right?
>
> Would you mean just replacing atomic_inc_not_zero() with atomic_add_unless()?

Yes.

Cheers,
Rusty.