LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/4 v6] max_threadx handling
@ 2015-03-15 16:13 Heinrich Schuchardt
  2015-03-15 16:13 ` [PATCH 1/4 v6] kernel/fork.c: new function for max_threads Heinrich Schuchardt
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Heinrich Schuchardt @ 2015-03-15 16:13 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Aaron Tomlin, Andy Lutomirski, Davidlohr Bueso, David Rientjes,
	David S. Miller, Fabian Frederick, Guenter Roeck, H. Peter Anvin,
	Ingo Molnar, Jens Axboe, Joe Perches, Johannes Weiner,
	Jonathan Corbet, Kees Cook, Michael Marineau, Oleg Nesterov,
	Paul E. McKenney, Peter Zijlstra, Prarit Bhargava, Rik van Riel,
	Rusty Russell, Steven Rostedt, Thomas Gleixner, Vladimir Davydov,
	linux-kernel, linux-doc, Heinrich Schuchardt

In fork_init a division by zero may occur.

In the first patch the calculation of max_threads is moved from fork_init
to a new separate function.

The incorrect calculation of max threads is addressed in the 
second patch.

Furthermore max_threads is checked against FUTEX_TID_MASK.

The third patch addresses max_threads being set by writing to
/proc/sys/kernel/threads-max. The same limits are applied as
in fork_init.

The fourth patch adds a description of threads-max to
Documentation/sysctl/kernel.txt.

New in version 6:
  Check against overflow of totalram_pages * PAGE_SIZE.
  Introduce argument of set_max_threads in 3rd patch where it is needed,
  not before.
  Update Documentation/sysctl/kernel.txt concerning threads-max.

New in version 5:
  Do not update limits of the init process
  Do not update max_threads on memory hotplug events
  Corrections to commit messages

New in version 4:
  Separation of refactoring and correction into separate patches
  (as requested by Ingo Molnar)
  Remove redundant argument of fork_init

New in version 3:
  Determination of max_threads moved to separate function.
  Handling of /proc/sys/kernel/threads-max
  Handling of memory hotplugging.

New in version 2:
  Use div64_u64 for 64-bit division.

Thank you for your helpful feedback:
  Andrew Morton <akpm@linux-foundation.org>
  David Rientjes <rientjes@google.com>
  Guenter Roeck <linux@roeck-us.net>
  Ingo Molnar <mingo@kernel.org>
  Oleg Nesterov <oleg@redhat.com>
  Peter Zijlstra <peterz@infradead.org>
  Vladimir Davydov <vdavydov@parallels.com>

Heinrich Schuchardt (4):
  kernel/fork.c: new function for max_threads
  kernel/fork.c: avoid division by zero
  kernel/sysctl.c: threads-max observe limits
  Doc/sysctl/kernel.txt: document threads-max

 Documentation/sysctl/kernel.txt | 21 +++++++++++
 include/linux/sysctl.h          |  3 ++
 init/main.c                     |  4 +--
 kernel/fork.c                   | 78 ++++++++++++++++++++++++++++++++++-------
 kernel/sysctl.c                 |  6 ++--
 5 files changed, 93 insertions(+), 19 deletions(-)

-- 
2.1.4


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/4 v6] kernel/fork.c: new function for max_threads
  2015-03-15 16:13 [PATCH 0/4 v6] max_threadx handling Heinrich Schuchardt
@ 2015-03-15 16:13 ` Heinrich Schuchardt
  2015-03-15 16:13 ` [PATCH 2/4 v6] kernel/fork.c: avoid division by zero Heinrich Schuchardt
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Heinrich Schuchardt @ 2015-03-15 16:13 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Aaron Tomlin, Andy Lutomirski, Davidlohr Bueso, David Rientjes,
	David S. Miller, Fabian Frederick, Guenter Roeck, H. Peter Anvin,
	Ingo Molnar, Jens Axboe, Joe Perches, Johannes Weiner,
	Jonathan Corbet, Kees Cook, Michael Marineau, Oleg Nesterov,
	Paul E. McKenney, Peter Zijlstra, Prarit Bhargava, Rik van Riel,
	Rusty Russell, Steven Rostedt, Thomas Gleixner, Vladimir Davydov,
	linux-kernel, linux-doc, Heinrich Schuchardt

PAGE_SIZE is not guaranteed to be equal to or less than 8 times the
THREAD_SIZE.

E.g. architecture hexagon may have page size 1M and thread size 4096.
This would lead to a division by zero in the calculation of max_threads.

With this patch the buggy code is moved to a separate function
set_max_threads. The error is not fixed.

After fixing the problem in a separate patch the new function can be
reused to adjust max_threads after adding or removing memory.

Argument mempages of function fork_init() is removed as totalram_pages
is an exported symbol.

The creation of separate patches for refactoring to a new function
and for fixing the logic was suggested by Ingo Molnar.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 init/main.c   |  4 ++--
 kernel/fork.c | 34 +++++++++++++++++++++-------------
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/init/main.c b/init/main.c
index d4037ab..f470d14 100644
--- a/init/main.c
+++ b/init/main.c
@@ -93,7 +93,7 @@
 static int kernel_init(void *);
 
 extern void init_IRQ(void);
-extern void fork_init(unsigned long);
+extern void fork_init(void);
 extern void radix_tree_init(void);
 #ifndef CONFIG_DEBUG_RODATA
 static inline void mark_rodata_ro(void) { }
@@ -650,7 +650,7 @@ asmlinkage __visible void __init start_kernel(void)
 #endif
 	thread_info_cache_init();
 	cred_init();
-	fork_init(totalram_pages);
+	fork_init();
 	proc_caches_init();
 	buffer_init();
 	key_init();
diff --git a/kernel/fork.c b/kernel/fork.c
index 4dc2dda..bf1ff00 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -253,7 +253,26 @@ EXPORT_SYMBOL_GPL(__put_task_struct);
 
 void __init __weak arch_task_cache_init(void) { }
 
-void __init fork_init(unsigned long mempages)
+/*
+ * set_max_threads
+ */
+static void set_max_threads(void)
+{
+	/*
+	 * The default maximum number of threads is set to a safe
+	 * value: the thread structures can take up at most half
+	 * of memory.
+	 */
+	max_threads = totalram_pages / (8 * THREAD_SIZE / PAGE_SIZE);
+
+	/*
+	 * we need to allow at least 20 threads to boot a system
+	 */
+	if (max_threads < 20)
+		max_threads = 20;
+}
+
+void __init fork_init(void)
 {
 #ifndef CONFIG_ARCH_TASK_STRUCT_ALLOCATOR
 #ifndef ARCH_MIN_TASKALIGN
@@ -268,18 +287,7 @@ void __init fork_init(unsigned long mempages)
 	/* do the arch specific task caches init */
 	arch_task_cache_init();
 
-	/*
-	 * The default maximum number of threads is set to a safe
-	 * value: the thread structures can take up at most half
-	 * of memory.
-	 */
-	max_threads = mempages / (8 * THREAD_SIZE / PAGE_SIZE);
-
-	/*
-	 * we need to allow at least 20 threads to boot a system
-	 */
-	if (max_threads < 20)
-		max_threads = 20;
+	set_max_threads();
 
 	init_task.signal->rlim[RLIMIT_NPROC].rlim_cur = max_threads/2;
 	init_task.signal->rlim[RLIMIT_NPROC].rlim_max = max_threads/2;
-- 
2.1.4


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/4 v6] kernel/fork.c: avoid division by zero
  2015-03-15 16:13 [PATCH 0/4 v6] max_threadx handling Heinrich Schuchardt
  2015-03-15 16:13 ` [PATCH 1/4 v6] kernel/fork.c: new function for max_threads Heinrich Schuchardt
@ 2015-03-15 16:13 ` Heinrich Schuchardt
  2015-03-16  7:41   ` Ingo Molnar
  2015-03-15 16:13 ` [PATCH 3/4 v6] kernel/sysctl.c: threads-max observe limits Heinrich Schuchardt
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Heinrich Schuchardt @ 2015-03-15 16:13 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Aaron Tomlin, Andy Lutomirski, Davidlohr Bueso, David Rientjes,
	David S. Miller, Fabian Frederick, Guenter Roeck, H. Peter Anvin,
	Ingo Molnar, Jens Axboe, Joe Perches, Johannes Weiner,
	Jonathan Corbet, Kees Cook, Michael Marineau, Oleg Nesterov,
	Paul E. McKenney, Peter Zijlstra, Prarit Bhargava, Rik van Riel,
	Rusty Russell, Steven Rostedt, Thomas Gleixner, Vladimir Davydov,
	linux-kernel, linux-doc, Heinrich Schuchardt

PAGE_SIZE is not guaranteed to be equal to or less than 8 times the
THREAD_SIZE.

E.g. architecture hexagon may have page size 1M and thread size 4096.
This would lead to a division by zero in the calculation of max_threads.

With 32-bit calculation there is no solution which delivers valid results
for all possible combinations of the parameters.
The code is only called once.
Hence a 64-bit calculation can be used as solution.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 kernel/fork.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index bf1ff00..69ff08f 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -88,6 +88,16 @@
 #include <trace/events/task.h>
 
 /*
+ * Minimum number of threads to boot the kernel
+ */
+#define MIN_THREADS 20
+
+/*
+ * Maximum number of threads
+ */
+#define MAX_THREADS FUTEX_TID_MASK
+
+/*
  * Protected counters by write_lock_irq(&tasklist_lock)
  */
 unsigned long total_forks;	/* Handle normal Linux uptimes. */
@@ -258,18 +268,25 @@ void __init __weak arch_task_cache_init(void) { }
  */
 static void set_max_threads(void)
 {
-	/*
-	 * The default maximum number of threads is set to a safe
-	 * value: the thread structures can take up at most half
-	 * of memory.
-	 */
-	max_threads = totalram_pages / (8 * THREAD_SIZE / PAGE_SIZE);
+	u64 threads;
 
 	/*
-	 * we need to allow at least 20 threads to boot a system
+	 * The number of threads shall be limited such that the thread
+	 * structures may only consume a small part of the available memory.
 	 */
-	if (max_threads < 20)
-		max_threads = 20;
+	if (fls64(totalram_pages) + fls64(PAGE_SIZE) > 64)
+		threads = MAX_THREADS;
+	else
+		threads = div64_u64((u64) totalram_pages * (u64) PAGE_SIZE,
+				    (u64) THREAD_SIZE * 8UL);
+
+	if (threads > MAX_THREADS)
+		threads = MAX_THREADS;
+
+	if (threads < MIN_THREADS)
+		threads = MIN_THREADS;
+
+	max_threads = (int) threads;
 }
 
 void __init fork_init(void)
-- 
2.1.4


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 3/4 v6] kernel/sysctl.c: threads-max observe limits
  2015-03-15 16:13 [PATCH 0/4 v6] max_threadx handling Heinrich Schuchardt
  2015-03-15 16:13 ` [PATCH 1/4 v6] kernel/fork.c: new function for max_threads Heinrich Schuchardt
  2015-03-15 16:13 ` [PATCH 2/4 v6] kernel/fork.c: avoid division by zero Heinrich Schuchardt
@ 2015-03-15 16:13 ` Heinrich Schuchardt
  2015-03-15 16:13 ` [PATCH 4/4 v6] Doc/sysctl/kernel.txt: document threads-max Heinrich Schuchardt
  2015-03-25 18:59 ` [PATCH 0/4 v6] max_threadx handling Heinrich Schuchardt
  4 siblings, 0 replies; 12+ messages in thread
From: Heinrich Schuchardt @ 2015-03-15 16:13 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Aaron Tomlin, Andy Lutomirski, Davidlohr Bueso, David Rientjes,
	David S. Miller, Fabian Frederick, Guenter Roeck, H. Peter Anvin,
	Ingo Molnar, Jens Axboe, Joe Perches, Johannes Weiner,
	Jonathan Corbet, Kees Cook, Michael Marineau, Oleg Nesterov,
	Paul E. McKenney, Peter Zijlstra, Prarit Bhargava, Rik van Riel,
	Rusty Russell, Steven Rostedt, Thomas Gleixner, Vladimir Davydov,
	linux-kernel, linux-doc, Heinrich Schuchardt

Users can change the maximum number of threads by writing to
/proc/sys/kernel/threads-max.

With the patch the value entered is checked against the same
limits that apply when fork_init is called.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/linux/sysctl.h |  3 +++
 kernel/fork.c          | 31 +++++++++++++++++++++++++++++--
 kernel/sysctl.c        |  6 ++----
 3 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index b7361f8..795d5fe 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -212,4 +212,7 @@ static inline void setup_sysctl_set(struct ctl_table_set *p,
 
 #endif /* CONFIG_SYSCTL */
 
+int sysctl_max_threads(struct ctl_table *table, int write,
+		       void __user *buffer, size_t *lenp, loff_t *ppos);
+
 #endif /* _LINUX_SYSCTL_H */
diff --git a/kernel/fork.c b/kernel/fork.c
index 69ff08f..3a359fda 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -74,6 +74,7 @@
 #include <linux/uprobes.h>
 #include <linux/aio.h>
 #include <linux/compiler.h>
+#include <linux/sysctl.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
@@ -266,7 +267,7 @@ void __init __weak arch_task_cache_init(void) { }
 /*
  * set_max_threads
  */
-static void set_max_threads(void)
+static void set_max_threads(unsigned int max_threads_suggested)
 {
 	u64 threads;
 
@@ -280,6 +281,9 @@ static void set_max_threads(void)
 		threads = div64_u64((u64) totalram_pages * (u64) PAGE_SIZE,
 				    (u64) THREAD_SIZE * 8UL);
 
+	if (threads > max_threads_suggested)
+		threads = max_threads_suggested;
+
 	if (threads > MAX_THREADS)
 		threads = MAX_THREADS;
 
@@ -304,7 +308,7 @@ void __init fork_init(void)
 	/* do the arch specific task caches init */
 	arch_task_cache_init();
 
-	set_max_threads();
+	set_max_threads(MAX_THREADS);
 
 	init_task.signal->rlim[RLIMIT_NPROC].rlim_cur = max_threads/2;
 	init_task.signal->rlim[RLIMIT_NPROC].rlim_max = max_threads/2;
@@ -2024,3 +2028,26 @@ int unshare_files(struct files_struct **displaced)
 	task_unlock(task);
 	return 0;
 }
+
+int sysctl_max_threads(struct ctl_table *table, int write,
+		       void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct ctl_table t;
+	int ret;
+	int threads = max_threads;
+	int min = MIN_THREADS;
+	int max = MAX_THREADS;
+
+	t = *table;
+	t.data = &threads;
+	t.extra1 = &min;
+	t.extra2 = &max;
+
+	ret = proc_dointvec_minmax(&t, write, buffer, lenp, ppos);
+	if (ret || !write)
+		return ret;
+
+	set_max_threads(threads);
+
+	return 0;
+}
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f57c5ae..80920a6 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -92,11 +92,9 @@
 #include <linux/nmi.h>
 #endif
 
-
 #if defined(CONFIG_SYSCTL)
 
 /* External variables not in a header file. */
-extern int max_threads;
 extern int suid_dumpable;
 #ifdef CONFIG_COREDUMP
 extern int core_uses_pid;
@@ -702,10 +700,10 @@ static struct ctl_table kern_table[] = {
 #endif
 	{
 		.procname	= "threads-max",
-		.data		= &max_threads,
+		.data		= NULL,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= sysctl_max_threads,
 	},
 	{
 		.procname	= "random",
-- 
2.1.4


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 4/4 v6] Doc/sysctl/kernel.txt: document threads-max
  2015-03-15 16:13 [PATCH 0/4 v6] max_threadx handling Heinrich Schuchardt
                   ` (2 preceding siblings ...)
  2015-03-15 16:13 ` [PATCH 3/4 v6] kernel/sysctl.c: threads-max observe limits Heinrich Schuchardt
@ 2015-03-15 16:13 ` Heinrich Schuchardt
  2015-03-15 16:30   ` Guenter Roeck
  2015-03-25 18:59 ` [PATCH 0/4 v6] max_threadx handling Heinrich Schuchardt
  4 siblings, 1 reply; 12+ messages in thread
From: Heinrich Schuchardt @ 2015-03-15 16:13 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Aaron Tomlin, Andy Lutomirski, Davidlohr Bueso, David Rientjes,
	David S. Miller, Fabian Frederick, Guenter Roeck, H. Peter Anvin,
	Ingo Molnar, Jens Axboe, Joe Perches, Johannes Weiner,
	Jonathan Corbet, Kees Cook, Michael Marineau, Oleg Nesterov,
	Paul E. McKenney, Peter Zijlstra, Prarit Bhargava, Rik van Riel,
	Rusty Russell, Steven Rostedt, Thomas Gleixner, Vladimir Davydov,
	linux-kernel, linux-doc, Heinrich Schuchardt

File /proc/sys/kernel/threads-max controls the maximum number
of threads that can be created using fork().

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 Documentation/sysctl/kernel.txt | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 75511ef..40f9c90 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -846,6 +846,27 @@ can be ORed together:
 
 ==============================================================
 
+threads-max
+
+This value controls the maximum number of threads that can be created
+using fork().
+
+During initialization the kernel sets this value such that even if the
+maximum number of threads is created, the thread structures occupy only
+a part (1/8th) of the available RAM pages.
+
+The minimum value that can be written to threads-max is 20.
+The maximum vlaue that can be written to threads-max is given by the
+constant FUTEX_TID_MASK (0x3fffffff).
+If a value outside of this range is written to threads-max an error
+EINVAL occurs.
+
+The value written is checked against the available RAM pages. If the
+thread structures would occupy too much (more than 1/8th) of the
+available RAM pages threads-max is reduced accordingly.
+
+==============================================================
+
 unknown_nmi_panic:
 
 The value in this file affects behavior of handling NMI. When the
-- 
2.1.4


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/4 v6] Doc/sysctl/kernel.txt: document threads-max
  2015-03-15 16:13 ` [PATCH 4/4 v6] Doc/sysctl/kernel.txt: document threads-max Heinrich Schuchardt
@ 2015-03-15 16:30   ` Guenter Roeck
  0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2015-03-15 16:30 UTC (permalink / raw)
  To: Heinrich Schuchardt, Andrew Morton
  Cc: Aaron Tomlin, Andy Lutomirski, Davidlohr Bueso, David Rientjes,
	David S. Miller, Fabian Frederick, H. Peter Anvin, Ingo Molnar,
	Jens Axboe, Joe Perches, Johannes Weiner, Jonathan Corbet,
	Kees Cook, Michael Marineau, Oleg Nesterov, Paul E. McKenney,
	Peter Zijlstra, Prarit Bhargava, Rik van Riel, Rusty Russell,
	Steven Rostedt, Thomas Gleixner, Vladimir Davydov, linux-kernel,
	linux-doc

On 03/15/2015 09:13 AM, Heinrich Schuchardt wrote:
> File /proc/sys/kernel/threads-max controls the maximum number
> of threads that can be created using fork().
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>   Documentation/sysctl/kernel.txt | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
>
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index 75511ef..40f9c90 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -846,6 +846,27 @@ can be ORed together:
>
>   ==============================================================
>
> +threads-max
> +
> +This value controls the maximum number of threads that can be created
> +using fork().
> +
> +During initialization the kernel sets this value such that even if the
> +maximum number of threads is created, the thread structures occupy only
> +a part (1/8th) of the available RAM pages.
> +
> +The minimum value that can be written to threads-max is 20.
> +The maximum vlaue that can be written to threads-max is given by the

value

> +constant FUTEX_TID_MASK (0x3fffffff).
> +If a value outside of this range is written to threads-max an error
> +EINVAL occurs.
> +
> +The value written is checked against the available RAM pages. If the
> +thread structures would occupy too much (more than 1/8th) of the
> +available RAM pages threads-max is reduced accordingly.
> +
> +==============================================================
> +
>   unknown_nmi_panic:
>
>   The value in this file affects behavior of handling NMI. When the
>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4 v6] kernel/fork.c: avoid division by zero
  2015-03-15 16:13 ` [PATCH 2/4 v6] kernel/fork.c: avoid division by zero Heinrich Schuchardt
@ 2015-03-16  7:41   ` Ingo Molnar
  2015-03-17 19:19     ` Heinrich Schuchardt
  0 siblings, 1 reply; 12+ messages in thread
From: Ingo Molnar @ 2015-03-16  7:41 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Andrew Morton, Aaron Tomlin, Andy Lutomirski, Davidlohr Bueso,
	David Rientjes, David S. Miller, Fabian Frederick, Guenter Roeck,
	H. Peter Anvin, Jens Axboe, Joe Perches, Johannes Weiner,
	Jonathan Corbet, Kees Cook, Michael Marineau, Oleg Nesterov,
	Paul E. McKenney, Peter Zijlstra, Prarit Bhargava, Rik van Riel,
	Rusty Russell, Steven Rostedt, Thomas Gleixner, Vladimir Davydov,
	linux-kernel, linux-doc


* Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> PAGE_SIZE is not guaranteed to be equal to or less than 8 times the
> THREAD_SIZE.
> 
> E.g. architecture hexagon may have page size 1M and thread size 4096.
> This would lead to a division by zero in the calculation of max_threads.
> 
> With 32-bit calculation there is no solution which delivers valid results
> for all possible combinations of the parameters.
> The code is only called once.
> Hence a 64-bit calculation can be used as solution.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  kernel/fork.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index bf1ff00..69ff08f 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -88,6 +88,16 @@
>  #include <trace/events/task.h>
>  
>  /*
> + * Minimum number of threads to boot the kernel
> + */
> +#define MIN_THREADS 20
> +
> +/*
> + * Maximum number of threads
> + */
> +#define MAX_THREADS FUTEX_TID_MASK
> +
> +/*
>   * Protected counters by write_lock_irq(&tasklist_lock)
>   */
>  unsigned long total_forks;	/* Handle normal Linux uptimes. */
> @@ -258,18 +268,25 @@ void __init __weak arch_task_cache_init(void) { }
>   */
>  static void set_max_threads(void)
>  {
> -	/*
> -	 * The default maximum number of threads is set to a safe
> -	 * value: the thread structures can take up at most half
> -	 * of memory.
> -	 */
> -	max_threads = totalram_pages / (8 * THREAD_SIZE / PAGE_SIZE);
> +	u64 threads;
>  
>  	/*
> -	 * we need to allow at least 20 threads to boot a system
> +	 * The number of threads shall be limited such that the thread
> +	 * structures may only consume a small part of the available memory.
>  	 */
> -	if (max_threads < 20)
> -		max_threads = 20;
> +	if (fls64(totalram_pages) + fls64(PAGE_SIZE) > 64)
> +		threads = MAX_THREADS;
> +	else
> +		threads = div64_u64((u64) totalram_pages * (u64) PAGE_SIZE,
> +				    (u64) THREAD_SIZE * 8UL);
> +
> +	if (threads > MAX_THREADS)
> +		threads = MAX_THREADS;
> +
> +	if (threads < MIN_THREADS)
> +		threads = MIN_THREADS;
> +
> +	max_threads = (int) threads;
>  }

So why does this patch do two things:

	- parametrizes set_max_threads() via defines
	- fixes a bug

?

Those two things should be done in two separate patches, first the 
introduction of parameters, then the fixing of the bug.

I suggested this in my first review: separate out and keep the fix 
portion of the series minimal.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4 v6] kernel/fork.c: avoid division by zero
  2015-03-16  7:41   ` Ingo Molnar
@ 2015-03-17 19:19     ` Heinrich Schuchardt
  2015-03-18 16:49       ` Ingo Molnar
  0 siblings, 1 reply; 12+ messages in thread
From: Heinrich Schuchardt @ 2015-03-17 19:19 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Andrew Morton, Aaron Tomlin, Andy Lutomirski, Davidlohr Bueso,
	David Rientjes, David S. Miller, Fabian Frederick, Guenter Roeck,
	H. Peter Anvin, Jens Axboe, Joe Perches, Johannes Weiner,
	Jonathan Corbet, Kees Cook, Michael Marineau, Oleg Nesterov,
	Paul E. McKenney, Peter Zijlstra, Prarit Bhargava, Rik van Riel,
	Rusty Russell, Steven Rostedt, Thomas Gleixner, Vladimir Davydov,
	linux-kernel, linux-doc

On 16.03.2015 08:41, Ingo Molnar wrote:
> 
> * Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> 
>> PAGE_SIZE is not guaranteed to be equal to or less than 8 times the
>> THREAD_SIZE.
>>
>> E.g. architecture hexagon may have page size 1M and thread size 4096.
>> This would lead to a division by zero in the calculation of max_threads.
>>
>> With 32-bit calculation there is no solution which delivers valid results
>> for all possible combinations of the parameters.
>> The code is only called once.
>> Hence a 64-bit calculation can be used as solution.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  kernel/fork.c | 35 ++++++++++++++++++++++++++---------
>>  1 file changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> index bf1ff00..69ff08f 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -88,6 +88,16 @@
>>  #include <trace/events/task.h>
>>  
>>  /*
>> + * Minimum number of threads to boot the kernel
>> + */
>> +#define MIN_THREADS 20
>> +
>> +/*
>> + * Maximum number of threads
>> + */
>> +#define MAX_THREADS FUTEX_TID_MASK
>> +
>> +/*
>>   * Protected counters by write_lock_irq(&tasklist_lock)
>>   */
>>  unsigned long total_forks;	/* Handle normal Linux uptimes. */
>> @@ -258,18 +268,25 @@ void __init __weak arch_task_cache_init(void) { }
>>   */
>>  static void set_max_threads(void)
>>  {
>> -	/*
>> -	 * The default maximum number of threads is set to a safe
>> -	 * value: the thread structures can take up at most half
>> -	 * of memory.
>> -	 */
>> -	max_threads = totalram_pages / (8 * THREAD_SIZE / PAGE_SIZE);
>> +	u64 threads;
>>  
>>  	/*
>> -	 * we need to allow at least 20 threads to boot a system
>> +	 * The number of threads shall be limited such that the thread
>> +	 * structures may only consume a small part of the available memory.
>>  	 */
>> -	if (max_threads < 20)
>> -		max_threads = 20;
>> +	if (fls64(totalram_pages) + fls64(PAGE_SIZE) > 64)
>> +		threads = MAX_THREADS;
>> +	else
>> +		threads = div64_u64((u64) totalram_pages * (u64) PAGE_SIZE,
>> +				    (u64) THREAD_SIZE * 8UL);
>> +
>> +	if (threads > MAX_THREADS)
>> +		threads = MAX_THREADS;
>> +
>> +	if (threads < MIN_THREADS)
>> +		threads = MIN_THREADS;
>> +
>> +	max_threads = (int) threads;
>>  }
> 
> So why does this patch do two things:
> 
> 	- parametrizes set_max_threads() via defines
> 	- fixes a bug
> 
> ?
> 
> Those two things should be done in two separate patches, first the 
> introduction of parameters, then the fixing of the bug.
> 
> I suggested this in my first review: separate out and keep the fix 
> portion of the series minimal.

Hello Ingo,

you requested me in you first review to separate the move to a separate
function and the code fix. That was already done in a previous version
of the patch.

With this patch version set_max_threads does not have any parameters
all. A parameter is introduced in a later patch. It is not needed before.

Maybe you wanted to refer to constants?

Introduction of constant MAX_THREADS before fixing the bug does not make
any sense because the problematic code moved to set_max_threads with the
division by zero bug would not use it. Introducing it in a later patch
does not make sense because checking for conversion overflow when
converting u64 to u32 is necessary.

Splitting of patches is advisable if we assume that on some releases
only part of the patch series is used. This is not applicable to this patch.

Best regards

Heinrich

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4 v6] kernel/fork.c: avoid division by zero
  2015-03-17 19:19     ` Heinrich Schuchardt
@ 2015-03-18 16:49       ` Ingo Molnar
  2015-03-18 17:06         ` Oleg Nesterov
  0 siblings, 1 reply; 12+ messages in thread
From: Ingo Molnar @ 2015-03-18 16:49 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Andrew Morton, Aaron Tomlin, Andy Lutomirski, Davidlohr Bueso,
	David Rientjes, David S. Miller, Fabian Frederick, Guenter Roeck,
	H. Peter Anvin, Jens Axboe, Joe Perches, Johannes Weiner,
	Jonathan Corbet, Kees Cook, Michael Marineau, Oleg Nesterov,
	Paul E. McKenney, Peter Zijlstra, Prarit Bhargava, Rik van Riel,
	Rusty Russell, Steven Rostedt, Thomas Gleixner, Vladimir Davydov,
	linux-kernel, linux-doc


* Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> On 16.03.2015 08:41, Ingo Molnar wrote:
> > 
> > * Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > 
> >> PAGE_SIZE is not guaranteed to be equal to or less than 8 times the
> >> THREAD_SIZE.
> >>
> >> E.g. architecture hexagon may have page size 1M and thread size 4096.
> >> This would lead to a division by zero in the calculation of max_threads.
> >>
> >> With 32-bit calculation there is no solution which delivers valid results
> >> for all possible combinations of the parameters.
> >> The code is only called once.
> >> Hence a 64-bit calculation can be used as solution.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >>  kernel/fork.c | 35 ++++++++++++++++++++++++++---------
> >>  1 file changed, 26 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/kernel/fork.c b/kernel/fork.c
> >> index bf1ff00..69ff08f 100644
> >> --- a/kernel/fork.c
> >> +++ b/kernel/fork.c
> >> @@ -88,6 +88,16 @@
> >>  #include <trace/events/task.h>
> >>  
> >>  /*
> >> + * Minimum number of threads to boot the kernel
> >> + */
> >> +#define MIN_THREADS 20
> >> +
> >> +/*
> >> + * Maximum number of threads
> >> + */
> >> +#define MAX_THREADS FUTEX_TID_MASK
> >> +
> >> +/*
> >>   * Protected counters by write_lock_irq(&tasklist_lock)
> >>   */
> >>  unsigned long total_forks;	/* Handle normal Linux uptimes. */
> >> @@ -258,18 +268,25 @@ void __init __weak arch_task_cache_init(void) { }
> >>   */
> >>  static void set_max_threads(void)
> >>  {
> >> -	/*
> >> -	 * The default maximum number of threads is set to a safe
> >> -	 * value: the thread structures can take up at most half
> >> -	 * of memory.
> >> -	 */
> >> -	max_threads = totalram_pages / (8 * THREAD_SIZE / PAGE_SIZE);
> >> +	u64 threads;
> >>  
> >>  	/*
> >> -	 * we need to allow at least 20 threads to boot a system
> >> +	 * The number of threads shall be limited such that the thread
> >> +	 * structures may only consume a small part of the available memory.
> >>  	 */
> >> -	if (max_threads < 20)
> >> -		max_threads = 20;
> >> +	if (fls64(totalram_pages) + fls64(PAGE_SIZE) > 64)
> >> +		threads = MAX_THREADS;
> >> +	else
> >> +		threads = div64_u64((u64) totalram_pages * (u64) PAGE_SIZE,
> >> +				    (u64) THREAD_SIZE * 8UL);
> >> +
> >> +	if (threads > MAX_THREADS)
> >> +		threads = MAX_THREADS;
> >> +
> >> +	if (threads < MIN_THREADS)
> >> +		threads = MIN_THREADS;
> >> +
> >> +	max_threads = (int) threads;
> >>  }
> > 
> > So why does this patch do two things:
> > 
> > 	- parametrizes set_max_threads() via defines
> > 	- fixes a bug
> > 
> > ?
> > 
> > Those two things should be done in two separate patches, first the 
> > introduction of parameters, then the fixing of the bug.
> > 
> > I suggested this in my first review: separate out and keep the fix 
> > portion of the series minimal.
> 
> Hello Ingo,
> 
> you requested me in you first review to separate the move to a separate
> function and the code fix. That was already done in a previous version
> of the patch.
> 
> With this patch version set_max_threads does not have any parameters
> all. A parameter is introduced in a later patch. It is not needed before.
> 
> Maybe you wanted to refer to constants?

Those are parameters to the function defined via macros at the moment. 

I wanted to point out that this patch does not look like a simple 
bugfix: is it so hard to separate out the bugfix from cleanup changes, 
while leaving the cleanups non-functional?

> Introduction of constant MAX_THREADS before fixing the bug does not 
> make any sense because the problematic code moved to set_max_threads 
> with the division by zero bug would not use it. Introducing it in a 
> later patch does not make sense because checking for conversion 
> overflow when converting u64 to u32 is necessary.
> 
> Splitting of patches is advisable if we assume that on some releases 
> only part of the patch series is used. This is not applicable to 
> this patch.

So what I noticed was the MIN_THREADS parametrization change - why 
isn't that done independently?

But ... no strong feelings from me, I'm not NAK-ing it or anything, 
maybe I'd have used this well-known pattern:

	threads = min(max(MIN_THREADS, threads), MAX_THREADS);

instead of:

> >> +	if (threads > MAX_THREADS)
> >> +		threads = MAX_THREADS;
> >> +
> >> +	if (threads < MIN_THREADS)
> >> +		threads = MIN_THREADS;
> >> +

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4 v6] kernel/fork.c: avoid division by zero
  2015-03-18 16:49       ` Ingo Molnar
@ 2015-03-18 17:06         ` Oleg Nesterov
  2015-03-25 12:25           ` Ingo Molnar
  0 siblings, 1 reply; 12+ messages in thread
From: Oleg Nesterov @ 2015-03-18 17:06 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Heinrich Schuchardt, Andrew Morton, Aaron Tomlin,
	Andy Lutomirski, Davidlohr Bueso, David Rientjes,
	David S. Miller, Fabian Frederick, Guenter Roeck, H. Peter Anvin,
	Jens Axboe, Joe Perches, Johannes Weiner, Jonathan Corbet,
	Kees Cook, Michael Marineau, Paul E. McKenney, Peter Zijlstra,
	Prarit Bhargava, Rik van Riel, Rusty Russell, Steven Rostedt,
	Thomas Gleixner, Vladimir Davydov, linux-kernel, linux-doc

On 03/18, Ingo Molnar wrote:
>
> But ... no strong feelings from me, I'm not NAK-ing it or anything,
> maybe I'd have used this well-known pattern:
>
> 	threads = min(max(MIN_THREADS, threads), MAX_THREADS);
>
> instead of:
>
> > >> +	if (threads > MAX_THREADS)
> > >> +		threads = MAX_THREADS;
> > >> +
> > >> +	if (threads < MIN_THREADS)
> > >> +		threads = MIN_THREADS;

	clamp(threads, MIN_THREADS, MAX_THREADS)

;)

Oleg.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4 v6] kernel/fork.c: avoid division by zero
  2015-03-18 17:06         ` Oleg Nesterov
@ 2015-03-25 12:25           ` Ingo Molnar
  0 siblings, 0 replies; 12+ messages in thread
From: Ingo Molnar @ 2015-03-25 12:25 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Heinrich Schuchardt, Andrew Morton, Aaron Tomlin,
	Andy Lutomirski, Davidlohr Bueso, David Rientjes,
	David S. Miller, Fabian Frederick, Guenter Roeck, H. Peter Anvin,
	Jens Axboe, Joe Perches, Johannes Weiner, Jonathan Corbet,
	Kees Cook, Michael Marineau, Paul E. McKenney, Peter Zijlstra,
	Prarit Bhargava, Rik van Riel, Rusty Russell, Steven Rostedt,
	Thomas Gleixner, Vladimir Davydov, linux-kernel, linux-doc


* Oleg Nesterov <oleg@redhat.com> wrote:

> On 03/18, Ingo Molnar wrote:
> >
> > But ... no strong feelings from me, I'm not NAK-ing it or anything,
> > maybe I'd have used this well-known pattern:
> >
> > 	threads = min(max(MIN_THREADS, threads), MAX_THREADS);
> >
> > instead of:
> >
> > > >> +	if (threads > MAX_THREADS)
> > > >> +		threads = MAX_THREADS;
> > > >> +
> > > >> +	if (threads < MIN_THREADS)
> > > >> +		threads = MIN_THREADS;
> 
> 	clamp(threads, MIN_THREADS, MAX_THREADS)
> 
> ;)

Indeed ;-)

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/4 v6] max_threadx handling
  2015-03-15 16:13 [PATCH 0/4 v6] max_threadx handling Heinrich Schuchardt
                   ` (3 preceding siblings ...)
  2015-03-15 16:13 ` [PATCH 4/4 v6] Doc/sysctl/kernel.txt: document threads-max Heinrich Schuchardt
@ 2015-03-25 18:59 ` Heinrich Schuchardt
  4 siblings, 0 replies; 12+ messages in thread
From: Heinrich Schuchardt @ 2015-03-25 18:59 UTC (permalink / raw)
  To: Andrew Morton, Jonathan Corbet
  Cc: Aaron Tomlin, Andy Lutomirski, Davidlohr Bueso, David Rientjes,
	David S. Miller, Fabian Frederick, Guenter Roeck, H. Peter Anvin,
	Ingo Molnar, Jens Axboe, Joe Perches, Johannes Weiner, Kees Cook,
	Michael Marineau, Oleg Nesterov, Paul E. McKenney,
	Peter Zijlstra, Prarit Bhargava, Rik van Riel, Rusty Russell,
	Steven Rostedt, Thomas Gleixner, Vladimir Davydov, linux-kernel,
	linux-doc

Hello Andrew,

could you, please, take the following patches into the MM tree:

[PATCH 1/4 v6] kernel/fork.c: new function for max_threads
https://lkml.org/lkml/2015/3/15/94

[PATCH 2/4 v6] kernel/fork.c: avoid division by zero
https://lkml.org/lkml/2015/3/15/92

[PATCH 3/4 v6] kernel/sysctl.c: threads-max observe limits
https://lkml.org/lkml/2015/3/15/95

The following patch I think will have to go through the documentation
tree. If you agree to taken above patches to the MM tree, I will send
the following to Jonathan Corbet exchanging the one word Guenter Roeck
that remarked in https://lkml.org/lkml/2015/3/15/100.

[PATCH 4/4 v6] Doc/sysctl/kernel.txt: document threads-max
https://lkml.org/lkml/2015/3/15/93

Best regards

Heinrich Schuchardt


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2015-03-25 19:04 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-15 16:13 [PATCH 0/4 v6] max_threadx handling Heinrich Schuchardt
2015-03-15 16:13 ` [PATCH 1/4 v6] kernel/fork.c: new function for max_threads Heinrich Schuchardt
2015-03-15 16:13 ` [PATCH 2/4 v6] kernel/fork.c: avoid division by zero Heinrich Schuchardt
2015-03-16  7:41   ` Ingo Molnar
2015-03-17 19:19     ` Heinrich Schuchardt
2015-03-18 16:49       ` Ingo Molnar
2015-03-18 17:06         ` Oleg Nesterov
2015-03-25 12:25           ` Ingo Molnar
2015-03-15 16:13 ` [PATCH 3/4 v6] kernel/sysctl.c: threads-max observe limits Heinrich Schuchardt
2015-03-15 16:13 ` [PATCH 4/4 v6] Doc/sysctl/kernel.txt: document threads-max Heinrich Schuchardt
2015-03-15 16:30   ` Guenter Roeck
2015-03-25 18:59 ` [PATCH 0/4 v6] max_threadx handling Heinrich Schuchardt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).