Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932117AbbBRTsa (ORCPT ); Wed, 18 Feb 2015 14:48:30 -0500 Received: from mout.gmx.net ([212.227.17.21]:64917 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753691AbbBRTsM (ORCPT ); Wed, 18 Feb 2015 14:48:12 -0500 Message-ID: <54E4EC64.5010609@gmx.de> Date: Wed, 18 Feb 2015 20:47:48 +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: Andrew Morton CC: "Kirill A. Shutemov" , Peter Zijlstra , Oleg Nesterov , Rik van Riel , Vladimir Davydov , Thomas Gleixner , David Rientjes , Kees Cook , linux-kernel@vger.kernel.org, Guenter Roeck Subject: Re: [PATCH 1/1 v2] kernel/fork.c: avoid division by zero References: <1424199698-7607-1-git-send-email-xypron.glpk@gmx.de> <20150217151511.95143a25ff83c165f4199d08@linux-foundation.org> In-Reply-To: <20150217151511.95143a25ff83c165f4199d08@linux-foundation.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-Provags-ID: V03:K0:52Cr/+Wv7gwCNS3zDvV0i7tV3zDA9igtOqNrgJNssWNl3FvD3Ov Wn9pgjHgqiun2pfHpvNiyYR7+JvWsrYRqYy+xx8MKufYWTr0+kBzsnxjPUKL4xQgiUMrYd7 vIZBrcNPGofUaPwGhpECq9EAQ6VqtmBKGoOqCiW5PhL2AH3DRqw/T22KOnMObb6cpuSxi9T OPWuiI7wamjB5LTylcuqg== X-UI-Out-Filterresults: notjunk:1; Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3254 Lines: 108 Hello Andrew, thank you for your comments. Unfortunately there is no solution with 32-bit calculus. Please, see my answers below. As fork_init is only called once there should be not performance issue in using 64-bit calculus. I think that my patch did not cover all problems connected to max_threads. I just had a look at the memory hotplugging code. Shouldn't max_threads and init_task.signal->rlim[RLIMIT_NPROC] be recalculated after adding or removing memory? This could be done in a hotplug callback. max_threads can be set by writing to /proc/sys/kernel/threads-max. Shouldn't the value be checked by the same routine and shouldn't init_task.signal->rlim[RLIMIT_NPROC] be updated? Best regards Heinrich On 18.02.2015 00:15, Andrew Morton wrote: > On Tue, 17 Feb 2015 20:01:38 +0100 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. >> >> The futex implementation assumes that tids fit into the FUTEX_TID_MASK. >> This limits the number of allowable threads. >> >> ... >> >> --- a/kernel/fork.c >> +++ b/kernel/fork.c >> @@ -74,6 +74,7 @@ >> #include >> #include >> #include >> +#include >> >> #include >> #include >> @@ -255,6 +256,8 @@ void __init __weak arch_task_cache_init(void) { } >> >> void __init fork_init(unsigned long mempages) >> { >> + u64 temp; > > That's a really poor name. We should always try to make names > meaningful. Here, something like "threads" would be better. ok. > >> #ifndef CONFIG_ARCH_TASK_STRUCT_ALLOCATOR >> #ifndef ARCH_MIN_TASKALIGN >> #define ARCH_MIN_TASKALIGN L1_CACHE_BYTES >> @@ -273,7 +276,16 @@ void __init fork_init(unsigned long mempages) >> * value: the thread structures can take up at most half >> * of memory. >> */ >> - max_threads = mempages / (8 * THREAD_SIZE / PAGE_SIZE); >> + temp = div64_u64((u64) mempages * (u64) PAGE_SIZE, >> + (u64) THREAD_SIZE * 8UL); >> + >> + /* >> + * The futex code assumes that tids fit into the FUTEX_TID_MASK. >> + */ >> + if (temp < FUTEX_TID_MASK) >> + max_threads = temp; >> + else >> + max_threads = FUTEX_TID_MASK; > > Seems rather complicated. How about > > max_threads = mempages / (8 * THREAD_SIZE); If 8 * THREAD_SIZE > mempages this gives 0. > max_threads *= PAGE_SIZE; If mempages / (8 * THREAD_SIZE) * PAGE_SIZE > INT_MAX an overflow occurs (e.g. total memory = 96TB, THREAD_SIZE = 4kB). > max_threads = min(max_threads, FUTEX_TID_MASK); > > And while we're there, I do think the comments need a refresh. What > does "the thread structures can take up at most half of memory" mean? > And what's the reasoning behind that "8"? I suggest we just delete all > that and make a new attempt at explaining why the code is this way. ok. > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/