[PATCH 0/4 v6] max_threadx handling

[PATCH 0/4 v6] max_threadx handling

[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

[PATCH 1/4 v6] kernel/fork.c: new function for max_threads

[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

[PATCH 2/4 v6] kernel/fork.c: avoid division by zero

[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

[PATCH 3/4 v6] kernel/sysctl.c: threads-max observe limits

[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

[PATCH 4/4 v6] Doc/sysctl/kernel.txt: document threads-max

[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

Re: [PATCH 4/4 v6] Doc/sysctl/kernel.txt: document threads-max

[Guenter Roeck]

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
>

Re: [PATCH 2/4 v6] kernel/fork.c: avoid division by zero

[Ingo Molnar]

* 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

Re: [PATCH 2/4 v6] kernel/fork.c: avoid division by zero

[Heinrich Schuchardt]

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

Re: [PATCH 2/4 v6] kernel/fork.c: avoid division by zero

[Ingo Molnar]

* 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

Re: [PATCH 2/4 v6] kernel/fork.c: avoid division by zero

[Oleg Nesterov]

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.

Re: [PATCH 2/4 v6] kernel/fork.c: avoid division by zero

[Ingo Molnar]

* 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

Re: [PATCH 0/4 v6] max_threadx handling

[Heinrich Schuchardt]

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