Re: [PATCH 1/3 v5] kernel/fork.c: new function for max_threads

David Rientjes
2015-02-24T21:04:04Z

On Tue, 24 Feb 2015, Heinrich Schuchardt wrote:

> diff --git a/init/main.c b/init/main.c
> index 61b99376..21394ec 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -94,7 +94,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) { }
> @@ -655,7 +655,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..460b044 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -253,7 +253,27 @@ EXPORT_SYMBOL_GPL(__put_task_struct);
>
> void __init __weak arch_task_cache_init(void) { }
>
> -void __init fork_init(unsigned long mempages)
> +/*
> + * set_max_threads
> + * The argument is ignored.
> + */
> +static void set_max_threads(unsigned int max_threads_suggested)
> +{
> + /*
> + * 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 +288,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(UINT_MAX);
>
> init_task.signal->rlim[RLIMIT_NPROC].rlim_cur = max_threads/2;
> init_task.signal->rlim[RLIMIT_NPROC].rlim_max = max_threads/2;

I'm afraid I don't understand this. The intent of the patch is to
separate the max_threads logic into a new function, correct? If that's
true, then I don't understand why UINT_MAX is being introduced into this
path and passed to the new function when it is ignored.

I think it would be better to simply keep passing mempages to fork_init()
and then pass it to set_max_threads() where max_threads actually gets set
using the argument passed. At least, the code would then match the intent
of the patch.