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

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