From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754208AbbCQTYQ (ORCPT ); Tue, 17 Mar 2015 15:24:16 -0400 Received: from mout.gmx.net ([212.227.17.22]:59016 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753500AbbCQTYO (ORCPT ); Tue, 17 Mar 2015 15:24:14 -0400 Message-ID: <55087E40.1020800@gmx.de> Date: Tue, 17 Mar 2015 20:19:28 +0100 From: Heinrich Schuchardt User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.4.0 MIME-Version: 1.0 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@vger.kernel.org, linux-doc@vger.kernel.org Subject: Re: [PATCH 2/4 v6] kernel/fork.c: avoid division by zero References: <1426436018-5159-1-git-send-email-xypron.glpk@gmx.de> <1426436018-5159-3-git-send-email-xypron.glpk@gmx.de> <20150316074146.GB15458@gmail.com> In-Reply-To: <20150316074146.GB15458@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Provags-ID: V03:K0:p3xVm6ZBQUZc12PViND6bZhmYhUQjw055+ot4XQ3jadUMLxBvat MVvuR6qI9TdaOEUXKdSHPLSrnVqAHWgzYdKqE5stm5203ZfAYyKAA+kK0Rmlfhgxu1gGnSq 5SPvu/BOWgTsTErvDjHydOFAEVDIxeMzWv2Zq0QAA0q5etJvril9oz6Bx4f6ZqPOisv0uSw fZA+/8r/v89X3QRxygzqQ== X-UI-Out-Filterresults: notjunk:1; Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 16.03.2015 08:41, Ingo Molnar wrote: > > * Heinrich Schuchardt 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 >> --- >> 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 >> >> /* >> + * 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