Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650986 > unrolled thread

[PATCH v2 0/5] kmod: help make deterministic

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 0/5] kmod: help make deterministic "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-26 02:20 +0200
    [PATCH v2 2/5] kmod: reduce atomic operations on kmod_concurrent "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-26 02:20 +0200
      Re: [PATCH v2 2/5] kmod: reduce atomic operations on kmod_concurrent Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-05-26 03:20 +0200
        Re: [PATCH v2 2/5] kmod: reduce atomic operations on kmod_concurrent "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-26 22:10 +0200
    [PATCH v3 1/4] module: use list_for_each_entry_rcu() on find_module_all() "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-27 03:30 +0200
    [PATCH v3 2/4] kmod: reduce atomic operations on kmod_concurrent and simplify "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-27 03:30 +0200
    [PATCH v3 4/4] kmod: throttle kmod thread limit "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-27 03:40 +0200
    [PATCH v3 0/4] kmod: help make deterministic "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-27 03:40 +0200

#1650986 — [PATCH v2 0/5] kmod: help make deterministic

On this v2 I'm following Dmitry Torokhov's recommendation from the v1 series
[0] and I stop dancing around with work arounds and try to go straight for what
I think should be a proper fix for kmod: throttle instead of failing when kmod
max concurrent threshold is reached.

This patch series depends on the unsigned int range proc sysctl changes which
Andrew Morton recently merged into his -mm tree [1].

The kmod stress test driver uses a new license (GPL on Linux, copyleft-next
outside of Linux). Linus was fine with the copyleft-next so long as it was
clear GPL applies to Linux [2] and an or clause was used if I wanted to use
copyleft-next. Later based on discussions with Alan and Ted ironed out an "or"
language clause to use [3].

All code is also available on my 20170525-kmod-throttle branch of my linux-next
tree based on tag next-20170525 [4].

If there are any questions please let me know.

[0] https://lkml.kernel.org/r/20170519032444.18416-1-mcgrof@kernel.org
[1] https://lkml.kernel.org/r/20170519033554.18592-1-mcgrof@kernel.org
[2] https://lkml.kernel.org/r/CA+55aFyhxcvD+q7tp+-yrSFDKfR0mOHgyEAe=f_94aKLsOu0Og@mail.gmail.com
[3] https://lkml.kernel.org/r/1495234558.7848.122.camel@linux.intel.com
[4] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170525-kmod-throttle

  Luis

Luis R. Rodriguez (5):
  module: use list_for_each_entry_rcu() on find_module_all()
  kmod: reduce atomic operations on kmod_concurrent
  kmod: add test driver to stress test the module loader
  kmod: add helpers for getting kmod limit
  kmod: throttle kmod thread limit

 Documentation/sysctl/kernel.txt       |   20 +
 include/linux/kmod.h                  |    7 +
 init/main.c                           |    1 +
 kernel/kmod.c                         |   92 ++-
 kernel/module.c                       |    2 +-
 kernel/sysctl.c                       |    7 +
 lib/Kconfig.debug                     |   25 +
 lib/Makefile                          |    1 +
 lib/test_kmod.c                       | 1246 +++++++++++++++++++++++++++++++++
 tools/testing/selftests/kmod/Makefile |   11 +
 tools/testing/selftests/kmod/config   |    7 +
 tools/testing/selftests/kmod/kmod.sh  |  615 ++++++++++++++++
 12 files changed, 2004 insertions(+), 30 deletions(-)
 create mode 100644 lib/test_kmod.c
 create mode 100644 tools/testing/selftests/kmod/Makefile
 create mode 100644 tools/testing/selftests/kmod/config
 create mode 100755 tools/testing/selftests/kmod/kmod.sh

-- 
2.11.0

[toc] | [next] | [standalone]

#1650987 — [PATCH v2 2/5] kmod: reduce atomic operations on kmod_concurrent

When checking if we want to allow a kmod thread to kick off we increment,
then read to see if we should enable a thread. If we were over the allowed
limit limit we decrement. Splitting the increment far apart from decrement
means there could be a time where two increments happen potentially
giving a false failure on a thread which should have been allowed.

CPU1			CPU2
atomic_inc()
			atomic_inc()
atomic_read()
			atomic_read()
atomic_dec()
			atomic_dec()

In this case a read on CPU1 gets the atomic_inc()'s and we could negate
it from getting a kmod thread. We could try to prevent this with a lock
or preemption but that is overkill. We can fix by reducing the number of
atomic operations. We do this by inverting the logic of of the enabler,
instead of incrementing kmod_concurrent as we get new kmod users, define the
variable kmod_concurrent_max as the max number of currently allowed kmod
users and as we get new kmod users just decrement it if its still positive.
This combines the dec and read in one atomic operation.

In this case we no longer get the same false failure:

CPU1			CPU2
atomic_dec_if_positive()
			atomic_dec_if_positive()
atomic_inc()
			atomic_inc()

Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 include/linux/kmod.h |  2 ++
 init/main.c          |  1 +
 kernel/kmod.c        | 44 +++++++++++++++++++++++++-------------------
 3 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index c4e441e00db5..8e2f302b214a 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -38,10 +38,12 @@ int __request_module(bool wait, const char *name, ...);
 #define request_module_nowait(mod...) __request_module(false, mod)
 #define try_then_request_module(x, mod...) \
 	((x) ?: (__request_module(true, mod), (x)))
+void init_kmod_umh(void);
 #else
 static inline int request_module(const char *name, ...) { return -ENOSYS; }
 static inline int request_module_nowait(const char *name, ...) { return -ENOSYS; }
 #define try_then_request_module(x, mod...) (x)
+static inline void init_kmod_umh(void) { }
 #endif
 
 
diff --git a/init/main.c b/init/main.c
index 9ec09ff8a930..9b20be716cf7 100644
--- a/init/main.c
+++ b/init/main.c
@@ -650,6 +650,7 @@ asmlinkage __visible void __init start_kernel(void)
 	thread_stack_cache_init();
 	cred_init();
 	fork_init();
+	init_kmod_umh();
 	proc_caches_init();
 	buffer_init();
 	key_init();
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 563f97e2be36..cafd27b92d19 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -46,6 +46,7 @@
 #include <trace/events/module.h>
 
 extern int max_threads;
+unsigned int max_modprobes;
 
 #define CAP_BSET	(void *)1
 #define CAP_PI		(void *)2
@@ -56,6 +57,8 @@ static DEFINE_SPINLOCK(umh_sysctl_lock);
 static DECLARE_RWSEM(umhelper_sem);
 
 #ifdef CONFIG_MODULES
+static atomic_t kmod_concurrent_max = ATOMIC_INIT(0);
+#define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 
 /*
 	modprobe_path is set via /proc/sys.
@@ -127,10 +130,7 @@ int __request_module(bool wait, const char *fmt, ...)
 {
 	va_list args;
 	char module_name[MODULE_NAME_LEN];
-	unsigned int max_modprobes;
 	int ret;
-	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
-#define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 	static int kmod_loop_msg;
 
 	/*
@@ -154,21 +154,7 @@ int __request_module(bool wait, const char *fmt, ...)
 	if (ret)
 		return ret;
 
-	/* If modprobe needs a service that is in a module, we get a recursive
-	 * loop.  Limit the number of running kmod threads to max_threads/2 or
-	 * MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
-	 * would be to run the parents of this process, counting how many times
-	 * kmod was invoked.  That would mean accessing the internals of the
-	 * process tables to get the command line, proc_pid_cmdline is static
-	 * and it is not worth changing the proc code just to handle this case. 
-	 * KAO.
-	 *
-	 * "trace the ppid" is simple, but will fail if someone's
-	 * parent exits.  I think this is as good as it gets. --RR
-	 */
-	max_modprobes = min(max_threads/2, MAX_KMOD_CONCURRENT);
-	atomic_inc(&kmod_concurrent);
-	if (atomic_read(&kmod_concurrent) > max_modprobes) {
+	if (atomic_dec_if_positive(&kmod_concurrent_max) < 0) {
 		/* We may be blaming an innocent here, but unlikely */
 		if (kmod_loop_msg < 5) {
 			printk(KERN_ERR
@@ -184,10 +170,30 @@ int __request_module(bool wait, const char *fmt, ...)
 
 	ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
 
-	atomic_dec(&kmod_concurrent);
+	atomic_inc(&kmod_concurrent_max);
+
 	return ret;
 }
 EXPORT_SYMBOL(__request_module);
+
+/*
+ * If modprobe needs a service that is in a module, we get a recursive
+ * loop.  Limit the number of running kmod threads to max_threads/2 or
+ * MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
+ * would be to run the parents of this process, counting how many times
+ * kmod was invoked.  That would mean accessing the internals of the
+ * process tables to get the command line, proc_pid_cmdline is static
+ * and it is not worth changing the proc code just to handle this case.
+ *
+ * "trace the ppid" is simple, but will fail if someone's
+ * parent exits.  I think this is as good as it gets.
+ */
+void __init init_kmod_umh(void)
+{
+	max_modprobes = min(max_threads/2, MAX_KMOD_CONCURRENT);
+	atomic_set(&kmod_concurrent_max, max_modprobes);
+}
+
 #endif /* CONFIG_MODULES */
 
 static void call_usermodehelper_freeinfo(struct subprocess_info *info)
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1651017 — Re: [PATCH v2 2/5] kmod: reduce atomic operations on kmod_concurrent

On Thu, May 25, 2017 at 05:16:27PM -0700, Luis R. Rodriguez wrote:
> When checking if we want to allow a kmod thread to kick off we increment,
> then read to see if we should enable a thread. If we were over the allowed
> limit limit we decrement. Splitting the increment far apart from decrement
> means there could be a time where two increments happen potentially
> giving a false failure on a thread which should have been allowed.
> 
> CPU1			CPU2
> atomic_inc()
> 			atomic_inc()
> atomic_read()
> 			atomic_read()
> atomic_dec()
> 			atomic_dec()
> 
> In this case a read on CPU1 gets the atomic_inc()'s and we could negate
> it from getting a kmod thread. We could try to prevent this with a lock
> or preemption but that is overkill. We can fix by reducing the number of
> atomic operations. We do this by inverting the logic of of the enabler,
> instead of incrementing kmod_concurrent as we get new kmod users, define the
> variable kmod_concurrent_max as the max number of currently allowed kmod
> users and as we get new kmod users just decrement it if its still positive.
> This combines the dec and read in one atomic operation.
> 
> In this case we no longer get the same false failure:
> 
> CPU1			CPU2
> atomic_dec_if_positive()
> 			atomic_dec_if_positive()
> atomic_inc()
> 			atomic_inc()
> 
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  include/linux/kmod.h |  2 ++
>  init/main.c          |  1 +
>  kernel/kmod.c        | 44 +++++++++++++++++++++++++-------------------
>  3 files changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index c4e441e00db5..8e2f302b214a 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -38,10 +38,12 @@ int __request_module(bool wait, const char *name, ...);
>  #define request_module_nowait(mod...) __request_module(false, mod)
>  #define try_then_request_module(x, mod...) \
>  	((x) ?: (__request_module(true, mod), (x)))
> +void init_kmod_umh(void);
>  #else
>  static inline int request_module(const char *name, ...) { return -ENOSYS; }
>  static inline int request_module_nowait(const char *name, ...) { return -ENOSYS; }
>  #define try_then_request_module(x, mod...) (x)
> +static inline void init_kmod_umh(void) { }
>  #endif
>  
>  
> diff --git a/init/main.c b/init/main.c
> index 9ec09ff8a930..9b20be716cf7 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -650,6 +650,7 @@ asmlinkage __visible void __init start_kernel(void)
>  	thread_stack_cache_init();
>  	cred_init();
>  	fork_init();
> +	init_kmod_umh();
>  	proc_caches_init();
>  	buffer_init();
>  	key_init();
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 563f97e2be36..cafd27b92d19 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -46,6 +46,7 @@
>  #include <trace/events/module.h>
>  
>  extern int max_threads;
> +unsigned int max_modprobes;
>  
>  #define CAP_BSET	(void *)1
>  #define CAP_PI		(void *)2
> @@ -56,6 +57,8 @@ static DEFINE_SPINLOCK(umh_sysctl_lock);
>  static DECLARE_RWSEM(umhelper_sem);
>  
>  #ifdef CONFIG_MODULES
> +static atomic_t kmod_concurrent_max = ATOMIC_INIT(0);
> +#define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
>  
>  /*
>  	modprobe_path is set via /proc/sys.
> @@ -127,10 +130,7 @@ int __request_module(bool wait, const char *fmt, ...)
>  {
>  	va_list args;
>  	char module_name[MODULE_NAME_LEN];
> -	unsigned int max_modprobes;
>  	int ret;
> -	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
> -#define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
>  	static int kmod_loop_msg;
>  
>  	/*
> @@ -154,21 +154,7 @@ int __request_module(bool wait, const char *fmt, ...)
>  	if (ret)
>  		return ret;
>  
> -	/* If modprobe needs a service that is in a module, we get a recursive
> -	 * loop.  Limit the number of running kmod threads to max_threads/2 or
> -	 * MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
> -	 * would be to run the parents of this process, counting how many times
> -	 * kmod was invoked.  That would mean accessing the internals of the
> -	 * process tables to get the command line, proc_pid_cmdline is static
> -	 * and it is not worth changing the proc code just to handle this case. 
> -	 * KAO.
> -	 *
> -	 * "trace the ppid" is simple, but will fail if someone's
> -	 * parent exits.  I think this is as good as it gets. --RR
> -	 */
> -	max_modprobes = min(max_threads/2, MAX_KMOD_CONCURRENT);
> -	atomic_inc(&kmod_concurrent);
> -	if (atomic_read(&kmod_concurrent) > max_modprobes) {
> +	if (atomic_dec_if_positive(&kmod_concurrent_max) < 0) {
>  		/* We may be blaming an innocent here, but unlikely */
>  		if (kmod_loop_msg < 5) {
>  			printk(KERN_ERR
> @@ -184,10 +170,30 @@ int __request_module(bool wait, const char *fmt, ...)
>  
>  	ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
>  
> -	atomic_dec(&kmod_concurrent);
> +	atomic_inc(&kmod_concurrent_max);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(__request_module);
> +
> +/*
> + * If modprobe needs a service that is in a module, we get a recursive
> + * loop.  Limit the number of running kmod threads to max_threads/2 or
> + * MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
> + * would be to run the parents of this process, counting how many times
> + * kmod was invoked.  That would mean accessing the internals of the
> + * process tables to get the command line, proc_pid_cmdline is static
> + * and it is not worth changing the proc code just to handle this case.
> + *
> + * "trace the ppid" is simple, but will fail if someone's
> + * parent exits.  I think this is as good as it gets.
> + */
> +void __init init_kmod_umh(void)
> +{
> +	max_modprobes = min(max_threads/2, MAX_KMOD_CONCURRENT);
> +	atomic_set(&kmod_concurrent_max, max_modprobes);

I would love if we could initialize atomic statically. So the trouble we
are trying to solve here is we create more threads than kernel supports,
with thread count being calculated as:

	threads = div64_u64((u64) totalram_pages * (u64) PAGE_SIZE,
			    (u64) THREAD_SIZE * 8UL);

So to not being serve 50 threads we need to deal with system smaller
than 3200 pages, or ~13M memory (assume thread size is 8 pages - 64 bit
with kasan, smaller page sizes reduce memory even more). Can you run
4.12 with modules support on machine with such memory?

So maybe we shoudl simply say:

static atomic_t kmod_concurrent_max = ATOMIC_INIT(MAX_KMOD_CONCURRENT_MAX);

and call it a day? So we do not need init_kmod_umh() and don't need to
call it from init/main.c.

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1651601 — Re: [PATCH v2 2/5] kmod: reduce atomic operations on kmod_concurrent

On Thu, May 25, 2017 at 06:11:00PM -0700, Dmitry Torokhov wrote:
> On Thu, May 25, 2017 at 05:16:27PM -0700, Luis R. Rodriguez wrote:
> > When checking if we want to allow a kmod thread to kick off we increment,
> > then read to see if we should enable a thread. If we were over the allowed
> > limit limit we decrement. Splitting the increment far apart from decrement
> > means there could be a time where two increments happen potentially
> > giving a false failure on a thread which should have been allowed.
> > 
> > CPU1			CPU2
> > atomic_inc()
> > 			atomic_inc()
> > atomic_read()
> > 			atomic_read()
> > atomic_dec()
> > 			atomic_dec()
> > 
> > In this case a read on CPU1 gets the atomic_inc()'s and we could negate
> > it from getting a kmod thread. We could try to prevent this with a lock
> > or preemption but that is overkill. We can fix by reducing the number of
> > atomic operations. We do this by inverting the logic of of the enabler,
> > instead of incrementing kmod_concurrent as we get new kmod users, define the
> > variable kmod_concurrent_max as the max number of currently allowed kmod
> > users and as we get new kmod users just decrement it if its still positive.
> > This combines the dec and read in one atomic operation.
> > 
> > In this case we no longer get the same false failure:
> > 
> > CPU1			CPU2
> > atomic_dec_if_positive()
> > 			atomic_dec_if_positive()
> > atomic_inc()
> > 			atomic_inc()
> > 
> > Suggested-by: Petr Mladek <pmladek@suse.com>
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > ---
> >  include/linux/kmod.h |  2 ++
> >  init/main.c          |  1 +
> >  kernel/kmod.c        | 44 +++++++++++++++++++++++++-------------------
> >  3 files changed, 28 insertions(+), 19 deletions(-)
> > 
> > diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> > index c4e441e00db5..8e2f302b214a 100644
> > --- a/include/linux/kmod.h
> > +++ b/include/linux/kmod.h
> > @@ -38,10 +38,12 @@ int __request_module(bool wait, const char *name, ...);
> >  #define request_module_nowait(mod...) __request_module(false, mod)
> >  #define try_then_request_module(x, mod...) \
> >  	((x) ?: (__request_module(true, mod), (x)))
> > +void init_kmod_umh(void);
> >  #else
> >  static inline int request_module(const char *name, ...) { return -ENOSYS; }
> >  static inline int request_module_nowait(const char *name, ...) { return -ENOSYS; }
> >  #define try_then_request_module(x, mod...) (x)
> > +static inline void init_kmod_umh(void) { }
> >  #endif
> >  
> >  
> > diff --git a/init/main.c b/init/main.c
> > index 9ec09ff8a930..9b20be716cf7 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -650,6 +650,7 @@ asmlinkage __visible void __init start_kernel(void)
> >  	thread_stack_cache_init();
> >  	cred_init();
> >  	fork_init();
> > +	init_kmod_umh();
> >  	proc_caches_init();
> >  	buffer_init();
> >  	key_init();
> > diff --git a/kernel/kmod.c b/kernel/kmod.c
> > index 563f97e2be36..cafd27b92d19 100644
> > --- a/kernel/kmod.c
> > +++ b/kernel/kmod.c
> > @@ -46,6 +46,7 @@
> >  #include <trace/events/module.h>
> >  
> >  extern int max_threads;
> > +unsigned int max_modprobes;
> >  
> >  #define CAP_BSET	(void *)1
> >  #define CAP_PI		(void *)2
> > @@ -56,6 +57,8 @@ static DEFINE_SPINLOCK(umh_sysctl_lock);
> >  static DECLARE_RWSEM(umhelper_sem);
> >  
> >  #ifdef CONFIG_MODULES
> > +static atomic_t kmod_concurrent_max = ATOMIC_INIT(0);
> > +#define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
> >  
> >  /*
> >  	modprobe_path is set via /proc/sys.
> > @@ -127,10 +130,7 @@ int __request_module(bool wait, const char *fmt, ...)
> >  {
> >  	va_list args;
> >  	char module_name[MODULE_NAME_LEN];
> > -	unsigned int max_modprobes;
> >  	int ret;
> > -	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
> > -#define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
> >  	static int kmod_loop_msg;
> >  
> >  	/*
> > @@ -154,21 +154,7 @@ int __request_module(bool wait, const char *fmt, ...)
> >  	if (ret)
> >  		return ret;
> >  
> > -	/* If modprobe needs a service that is in a module, we get a recursive
> > -	 * loop.  Limit the number of running kmod threads to max_threads/2 or
> > -	 * MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
> > -	 * would be to run the parents of this process, counting how many times
> > -	 * kmod was invoked.  That would mean accessing the internals of the
> > -	 * process tables to get the command line, proc_pid_cmdline is static
> > -	 * and it is not worth changing the proc code just to handle this case. 
> > -	 * KAO.
> > -	 *
> > -	 * "trace the ppid" is simple, but will fail if someone's
> > -	 * parent exits.  I think this is as good as it gets. --RR
> > -	 */
> > -	max_modprobes = min(max_threads/2, MAX_KMOD_CONCURRENT);
> > -	atomic_inc(&kmod_concurrent);
> > -	if (atomic_read(&kmod_concurrent) > max_modprobes) {
> > +	if (atomic_dec_if_positive(&kmod_concurrent_max) < 0) {
> >  		/* We may be blaming an innocent here, but unlikely */
> >  		if (kmod_loop_msg < 5) {
> >  			printk(KERN_ERR
> > @@ -184,10 +170,30 @@ int __request_module(bool wait, const char *fmt, ...)
> >  
> >  	ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
> >  
> > -	atomic_dec(&kmod_concurrent);
> > +	atomic_inc(&kmod_concurrent_max);
> > +
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL(__request_module);
> > +
> > +/*
> > + * If modprobe needs a service that is in a module, we get a recursive
> > + * loop.  Limit the number of running kmod threads to max_threads/2 or
> > + * MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
> > + * would be to run the parents of this process, counting how many times
> > + * kmod was invoked.  That would mean accessing the internals of the
> > + * process tables to get the command line, proc_pid_cmdline is static
> > + * and it is not worth changing the proc code just to handle this case.
> > + *
> > + * "trace the ppid" is simple, but will fail if someone's
> > + * parent exits.  I think this is as good as it gets.
> > + */
> > +void __init init_kmod_umh(void)
> > +{
> > +	max_modprobes = min(max_threads/2, MAX_KMOD_CONCURRENT);
> > +	atomic_set(&kmod_concurrent_max, max_modprobes);
> 
> I would love if we could initialize atomic statically. So the trouble we
> are trying to solve here is we create more threads than kernel supports,
> with thread count being calculated as:
> 
> 	threads = div64_u64((u64) totalram_pages * (u64) PAGE_SIZE,
> 			    (u64) THREAD_SIZE * 8UL);
> 
> So to not being serve 50 threads we need to deal with system smaller
> than 3200 pages, or ~13M memory (assume thread size is 8 pages - 64 bit
> with kasan, smaller page sizes reduce memory even more). Can you run
> 4.12 with modules support on machine with such memory?
> 
> So maybe we shoudl simply say:
> 
> static atomic_t kmod_concurrent_max = ATOMIC_INIT(MAX_KMOD_CONCURRENT_MAX);
> 
> and call it a day? So we do not need init_kmod_umh() and don't need to
> call it from init/main.c.

I like this very much. If shit blows up we can simply use the kconfig thing
I had proposed earlier, however it would have to accept less than 50 threads
given we'd be computing this at build time, not at run time.

  Luis

[toc] | [prev] | [next] | [standalone]

#1651639 — [PATCH v3 1/4] module: use list_for_each_entry_rcu() on find_module_all()

The module list has been using RCU in a lot of other calls
for a while now, we just overlooked changing this one over to
use RCU.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index 3803449ca219..2df38d45ca37 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -603,7 +603,7 @@ static struct module *find_module_all(const char *name, size_t len,
 
 	module_assert_mutex_or_preempt();
 
-	list_for_each_entry(mod, &modules, list) {
+	list_for_each_entry_rcu(mod, &modules, list) {
 		if (!even_unformed && mod->state == MODULE_STATE_UNFORMED)
 			continue;
 		if (strlen(mod->name) == len && !memcmp(mod->name, name, len))
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1651678 — [PATCH v3 2/4] kmod: reduce atomic operations on kmod_concurrent and simplify

When checking if we want to allow a kmod thread to kick off we increment,
then read to see if we should enable a thread. If we were over the allowed
limit limit we decrement. Splitting the increment far apart from decrement
means there could be a time where two increments happen potentially
giving a false failure on a thread which should have been allowed.

CPU1			CPU2
atomic_inc()
			atomic_inc()
atomic_read()
			atomic_read()
atomic_dec()
			atomic_dec()

In this case a read on CPU1 gets the atomic_inc()'s and we could negate
it from getting a kmod thread. We could try to prevent this with a lock
or preemption but that is overkill. We can fix by reducing the number of
atomic operations. We do this by inverting the logic of of the enabler,
instead of incrementing kmod_concurrent as we get new kmod users, define the
variable kmod_concurrent_max as the max number of currently allowed kmod
users and as we get new kmod users just decrement it if its still positive.
This combines the dec and read in one atomic operation.

In this case we no longer get the same false failure:

CPU1			CPU2
atomic_dec_if_positive()
			atomic_dec_if_positive()
atomic_inc()
			atomic_inc()

The number of threads is computed at init, and since the current computation
of kmod_concurrent includes the thread count we can avoid setting
kmod_concurrent_max later in boot through an init call by simply sticking to
50 as the kmod_concurrent_max. The assumption here is a system with modules
must at least have ~16 MiB of RAM.

Suggested-by: Petr Mladek <pmladek@suse.com>
Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 kernel/kmod.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 563f97e2be36..3e346c700e80 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -45,8 +45,6 @@
 
 #include <trace/events/module.h>
 
-extern int max_threads;
-
 #define CAP_BSET	(void *)1
 #define CAP_PI		(void *)2
 
@@ -56,6 +54,19 @@ static DEFINE_SPINLOCK(umh_sysctl_lock);
 static DECLARE_RWSEM(umhelper_sem);
 
 #ifdef CONFIG_MODULES
+/*
+ * Assuming:
+ *
+ * threads = div64_u64((u64) totalram_pages * (u64) PAGE_SIZE,
+ *		       (u64) THREAD_SIZE * 8UL);
+ *
+ * If you need less than 50 threads would mean we're dealing with systems
+ * smaller than 3200 pages. This assuems you are capable of having ~13M memory,
+ * and this would only be an be an upper limit, after which the OOM killer
+ * would take effect. Systems like these are very unlikely if modules are
+ * enabled.
+ * */
+static atomic_t kmod_concurrent_max = ATOMIC_INIT(50);
 
 /*
 	modprobe_path is set via /proc/sys.
@@ -127,10 +138,7 @@ int __request_module(bool wait, const char *fmt, ...)
 {
 	va_list args;
 	char module_name[MODULE_NAME_LEN];
-	unsigned int max_modprobes;
 	int ret;
-	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
-#define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 	static int kmod_loop_msg;
 
 	/*
@@ -154,21 +162,7 @@ int __request_module(bool wait, const char *fmt, ...)
 	if (ret)
 		return ret;
 
-	/* If modprobe needs a service that is in a module, we get a recursive
-	 * loop.  Limit the number of running kmod threads to max_threads/2 or
-	 * MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
-	 * would be to run the parents of this process, counting how many times
-	 * kmod was invoked.  That would mean accessing the internals of the
-	 * process tables to get the command line, proc_pid_cmdline is static
-	 * and it is not worth changing the proc code just to handle this case. 
-	 * KAO.
-	 *
-	 * "trace the ppid" is simple, but will fail if someone's
-	 * parent exits.  I think this is as good as it gets. --RR
-	 */
-	max_modprobes = min(max_threads/2, MAX_KMOD_CONCURRENT);
-	atomic_inc(&kmod_concurrent);
-	if (atomic_read(&kmod_concurrent) > max_modprobes) {
+	if (atomic_dec_if_positive(&kmod_concurrent_max) < 0) {
 		/* We may be blaming an innocent here, but unlikely */
 		if (kmod_loop_msg < 5) {
 			printk(KERN_ERR
@@ -176,7 +170,6 @@ int __request_module(bool wait, const char *fmt, ...)
 			       module_name);
 			kmod_loop_msg++;
 		}
-		atomic_dec(&kmod_concurrent);
 		return -ENOMEM;
 	}
 
@@ -184,10 +177,12 @@ int __request_module(bool wait, const char *fmt, ...)
 
 	ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
 
-	atomic_dec(&kmod_concurrent);
+	atomic_inc(&kmod_concurrent_max);
+
 	return ret;
 }
 EXPORT_SYMBOL(__request_module);
+
 #endif /* CONFIG_MODULES */
 
 static void call_usermodehelper_freeinfo(struct subprocess_info *info)
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1651692 — [PATCH v3 4/4] kmod: throttle kmod thread limit

If we reach the limit of modprobe_limit threads running the next
request_module() call will fail. The original reason for adding
a kill was to do away with possible issues with in old circumstances
which would create a recursive series of request_module() calls.
We can do better than just be super aggressive and reject calls
once we've reached the limit by simply making pending callers wait
until the threshold has been reduced.

The only difference is the clutch helps with avoiding making
request_module() requests fatal more often. With x86_64 qemu,
with 4 cores, 4 GiB of RAM it takes the following run time to
run both tests:

time ./kmod.sh -t 0008
real    0m12.364s
user    0m0.704s
sys     0m5.373s

time ./kmod.sh -t 0009
real    0m47.638s
user    0m1.033s
sys     0m5.425s

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 kernel/kmod.c                        | 16 +++++++---------
 tools/testing/selftests/kmod/kmod.sh | 24 ++----------------------
 2 files changed, 9 insertions(+), 31 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 3e346c700e80..46b12fed6fd0 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -67,6 +67,7 @@ static DECLARE_RWSEM(umhelper_sem);
  * enabled.
  * */
 static atomic_t kmod_concurrent_max = ATOMIC_INIT(50);
+static DECLARE_WAIT_QUEUE_HEAD(kmod_wq);
 
 /*
 	modprobe_path is set via /proc/sys.
@@ -139,7 +140,6 @@ int __request_module(bool wait, const char *fmt, ...)
 	va_list args;
 	char module_name[MODULE_NAME_LEN];
 	int ret;
-	static int kmod_loop_msg;
 
 	/*
 	 * We don't allow synchronous module loading from async.  Module
@@ -163,14 +163,11 @@ int __request_module(bool wait, const char *fmt, ...)
 		return ret;
 
 	if (atomic_dec_if_positive(&kmod_concurrent_max) < 0) {
-		/* We may be blaming an innocent here, but unlikely */
-		if (kmod_loop_msg < 5) {
-			printk(KERN_ERR
-			       "request_module: runaway loop modprobe %s\n",
-			       module_name);
-			kmod_loop_msg++;
-		}
-		return -ENOMEM;
+		pr_warn_ratelimited("request_module: kmod_concurrent_max (%u) close to 0 (max_modprobes: %u), for module %s\n, throttling...",
+				    atomic_read(&kmod_concurrent_max),
+				    50, module_name);
+		wait_event_interruptible(kmod_wq,
+					 atomic_dec_if_positive(&kmod_concurrent_max) >= 0);
 	}
 
 	trace_module_request(module_name, wait, _RET_IP_);
@@ -178,6 +175,7 @@ int __request_module(bool wait, const char *fmt, ...)
 	ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
 
 	atomic_inc(&kmod_concurrent_max);
+	wake_up_all(&kmod_wq);
 
 	return ret;
 }
diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
index 10196a62ed09..8cecae9a8bca 100755
--- a/tools/testing/selftests/kmod/kmod.sh
+++ b/tools/testing/selftests/kmod/kmod.sh
@@ -59,28 +59,8 @@ ALL_TESTS="$ALL_TESTS 0004:1:1"
 ALL_TESTS="$ALL_TESTS 0005:10:1"
 ALL_TESTS="$ALL_TESTS 0006:10:1"
 ALL_TESTS="$ALL_TESTS 0007:5:1"
-
-# Disabled tests:
-#
-# 0008 x 150 -  multithreaded - push kmod_concurrent over max_modprobes for request_module()"
-# Current best-effort failure interpretation:
-# Enough module requests get loaded in place fast enough to reach over the
-# max_modprobes limit and trigger a failure -- before we're even able to
-# start processing pending requests.
-ALL_TESTS="$ALL_TESTS 0008:150:0"
-
-# 0009 x 150 - multithreaded - push kmod_concurrent over max_modprobes for get_fs_type()"
-# Current best-effort failure interpretation:
-#
-# get_fs_type() requests modules using aliases as such the optimization in
-# place today to look for already loaded modules will not take effect and
-# we end up requesting a new module to load, this bumps the kmod_concurrent,
-# and in certain circumstances can lead to pushing the kmod_concurrent over
-# the max_modprobe limit.
-#
-# This test fails much easier than test 0008 since the alias optimizations
-# are not in place.
-ALL_TESTS="$ALL_TESTS 0009:150:0"
+ALL_TESTS="$ALL_TESTS 0008:150:1"
+ALL_TESTS="$ALL_TESTS 0009:150:1"
 
 test_modprobe()
 {
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1651707 — [PATCH v3 0/4] kmod: help make deterministic

This v3 nukes the proc sysctl interface in favor for just letting userspace
just check kernel revision. Prior to whenever this is merged userspace should
try to avoid hammering more than 50 kmod threads as they can fail and it'd
get -ENOMEM.

We do away with the old heuristics on assuming you could end up with
less than max_threads/2 < 50 threads as Dmitry notes this would mean having
a system with 16 MiB of RAM with modules enabled. It simplifies our patch
"kmod: reduce atomic operations on kmod_concurrent" considerbly.

Since the sysctl interface is gone, this no longer depends on any
other patches, the series is independent. As usual the series is
available on my linux-next 20170526-kmod-only branch which is based
on next-20170526.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170526-kmod-only

  Luis

Luis R. Rodriguez (4):
  module: use list_for_each_entry_rcu() on find_module_all()
  kmod: reduce atomic operations on kmod_concurrent and simplify
  kmod: add test driver to stress test the module loader
  kmod: throttle kmod thread limit

 kernel/kmod.c                         |   55 +-
 kernel/module.c                       |    2 +-
 lib/Kconfig.debug                     |   25 +
 lib/Makefile                          |    1 +
 lib/test_kmod.c                       | 1246 +++++++++++++++++++++++++++++++++
 tools/testing/selftests/kmod/Makefile |   11 +
 tools/testing/selftests/kmod/config   |    7 +
 tools/testing/selftests/kmod/kmod.sh  |  615 ++++++++++++++++
 8 files changed, 1930 insertions(+), 32 deletions(-)
 create mode 100644 lib/test_kmod.c
 create mode 100644 tools/testing/selftests/kmod/Makefile
 create mode 100644 tools/testing/selftests/kmod/config
 create mode 100755 tools/testing/selftests/kmod/kmod.sh

-- 
2.11.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web