LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Heinrich Schuchardt <xypron.glpk@gmx.de>
To: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Aaron Tomlin <atomlin@redhat.com>,
	Andy Lutomirski <luto@amacapital.net>,
	Davidlohr Bueso <dave@stgolabs.net>,
	David Rientjes <rientjes@google.com>,
	"David S. Miller" <davem@davemloft.net>,
	Fabian Frederick <fabf@skynet.be>,
	Guenter Roeck <linux@roeck-us.net>,
	"H. Peter Anvin" <hpa@zytor.com>, Jens Axboe <axboe@fb.com>,
	Joe Perches <joe@perches.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Jonathan Corbet <corbet@lwn.net>,
	Kees Cook <keescook@chromium.org>,
	Michael Marineau <mike@marineau.org>,
	Oleg Nesterov <oleg@redhat.com>,
	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Prarit Bhargava <prarit@redhat.com>,
	Rik van Riel <riel@redhat.com>,
	Rusty Russell <rusty@rustcorp.com.au>,
	Steven Rostedt <rostedt@goodmis.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Vladimir Davydov <vdavydov@parallels.com>,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org
Subject: Re: [PATCH 2/4 v6] kernel/fork.c: avoid division by zero
Date: Tue, 17 Mar 2015 20:19:28 +0100	[thread overview]
Message-ID: <55087E40.1020800@gmx.de> (raw)
In-Reply-To: <20150316074146.GB15458@gmail.com>

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

  reply	other threads:[~2015-03-17 19:24 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=55087E40.1020800@gmx.de \
    --to=xypron.glpk@gmx.de \
    --cc=akpm@linux-foundation.org \
    --cc=atomlin@redhat.com \
    --cc=axboe@fb.com \
    --cc=corbet@lwn.net \
    --cc=dave@stgolabs.net \
    --cc=davem@davemloft.net \
    --cc=fabf@skynet.be \
    --cc=hannes@cmpxchg.org \
    --cc=hpa@zytor.com \
    --cc=joe@perches.com \
    --cc=keescook@chromium.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=luto@amacapital.net \
    --cc=mike@marineau.org \
    --cc=mingo@kernel.org \
    --cc=oleg@redhat.com \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=peterz@infradead.org \
    --cc=prarit@redhat.com \
    --cc=riel@redhat.com \
    --cc=rientjes@google.com \
    --cc=rostedt@goodmis.org \
    --cc=rusty@rustcorp.com.au \
    --cc=tglx@linutronix.de \
    --cc=vdavydov@parallels.com \
    --subject='Re: [PATCH 2/4 v6] kernel/fork.c: avoid division by zero' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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