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 <[email protected]> > 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 <linux/uprobes.h> >> #include <linux/aio.h> >> #include <linux/compiler.h> >> +#include <linux/math64.h> >> >> #include <asm/pgtable.h> >> #include <asm/pgalloc.h> >> @@ -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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
