[PATCH 1/3] fork: dynamically allocate cache array for vmapped stacks using cpuhp

[PATCH 1/3] fork: dynamically allocate cache array for vmapped stacks using cpuhp

[Hoeun Ryu]

 Using virtually mapped stack, kernel stacks are allocated via vmalloc.
In the current implementation, two stacks per cpu can be cached when
tasks are freed and the cached stacks are used again in task duplications.
but the array for the cached stacks is statically allocated by per-cpu api.
 In this new implementation, the array for the cached stacks are dynamically
allocted and freed by cpu hotplug callbacks and the cached stacks are freed
when cpu is down. setup for cpu hotplug is established in fork_init().

Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
---
 kernel/fork.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 64 insertions(+), 17 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 61284d8..54421a9 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -167,26 +167,71 @@ void __weak arch_release_thread_stack(unsigned long *stack)
  * flush.  Try to minimize the number of calls by caching stacks.
  */
 #define NR_CACHED_STACKS 2
-static DEFINE_PER_CPU(struct vm_struct *, cached_stacks[NR_CACHED_STACKS]);
+
+struct vm_stack_cache {
+	struct vm_struct **vm_stacks;
+	int nr;
+	int cur;
+};
+
+static DEFINE_PER_CPU(struct vm_stack_cache, vm_stacks);
+
+static int alloc_vm_stack_cache(unsigned int cpu)
+{
+	struct vm_stack_cache *vm_stack_cache = &per_cpu(vm_stacks, cpu);
+	struct vm_struct **vm_stacks = vm_stack_cache->vm_stacks;
+	int i;
+
+	/* if free_vm_stack_cache() didn't free it */
+	if (!vm_stacks) {
+		vm_stacks =
+			vzalloc(sizeof(struct vm_struct *) * NR_CACHED_STACKS);
+		if (!vm_stacks)
+			return -ENOMEM;
+	}
+
+	vm_stack_cache->vm_stacks = vm_stacks;
+	vm_stack_cache->cur = 0;
+	vm_stack_cache->nr = 0;
+
+	return 0;
+}
+
+static int free_vm_stack_cache(unsigned int cpu)
+{
+	struct vm_stack_cache *vm_stack_cache = &per_cpu(vm_stacks, cpu);
+	struct vm_struct **vm_stacks = vm_stack_cache->vm_stacks;
+	int i;
+
+	for (i = 0; i < vm_stack_cache->nr; i++) {
+		vfree(vm_stacks[i]->addr);
+		vm_stacks[i] = NULL;
+	}
+
+	vm_stack_cache->nr = 0;
+	vm_stack_cache->cur = 0;
+	/* do not free vm_stack[cpu]->vm_stacks itself, reused in allocation */
+
+	return 0;
+}
+
 #endif
 
 static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
 {
 #ifdef CONFIG_VMAP_STACK
+	struct vm_stack_cache *vm_stack_cache =
+		&per_cpu(vm_stacks, smp_processor_id());
+	struct vm_struct **vm_stacks = vm_stack_cache->vm_stacks;
 	void *stack;
-	int i;
 
 	local_irq_disable();
-	for (i = 0; i < NR_CACHED_STACKS; i++) {
-		struct vm_struct *s = this_cpu_read(cached_stacks[i]);
-
-		if (!s)
-			continue;
-		this_cpu_write(cached_stacks[i], NULL);
-
-		tsk->stack_vm_area = s;
+	if (vm_stack_cache->cur > 0) {
+		struct vm_struct *vm_stack = vm_stacks[--vm_stack_cache->cur];
+		tsk->stack_vm_area = vm_stack;
 		local_irq_enable();
-		return s->addr;
+
+		return vm_stack->addr;
 	}
 	local_irq_enable();
 
@@ -216,15 +261,14 @@ static inline void free_thread_stack(struct task_struct *tsk)
 {
 #ifdef CONFIG_VMAP_STACK
 	if (task_stack_vm_area(tsk)) {
+		struct vm_stack_cache *vm_stack_cache =
+			&per_cpu(vm_stacks, smp_processor_id());
+		struct vm_struct **vm_stacks = vm_stack_cache->vm_stacks;
 		unsigned long flags;
-		int i;
 
 		local_irq_save(flags);
-		for (i = 0; i < NR_CACHED_STACKS; i++) {
-			if (this_cpu_read(cached_stacks[i]))
-				continue;
-
-			this_cpu_write(cached_stacks[i], tsk->stack_vm_area);
+		if (vm_stack_cache->cur < vm_stack_cache->nr) {
+			vm_stacks[vm_stack_cache->cur++] = tsk->stack_vm_area;
 			local_irq_restore(flags);
 			return;
 		}
@@ -456,6 +500,9 @@ void __init fork_init(void)
 	for (i = 0; i < UCOUNT_COUNTS; i++) {
 		init_user_ns.ucount_max[i] = max_threads/2;
 	}
+
+	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "vm_stack_cache",
+			  alloc_vm_stack_cache, free_vm_stack_cache);
 }
 
 int __weak arch_dup_task_struct(struct task_struct *dst,
-- 
2.7.4

Re: [PATCH 1/3] fork: dynamically allocate cache array for vmapped stacks using cpuhp

[Michal Hocko]

On Sat 04-02-17 00:30:05, Hoeun Ryu wrote:
>  Using virtually mapped stack, kernel stacks are allocated via vmalloc.
> In the current implementation, two stacks per cpu can be cached when
> tasks are freed and the cached stacks are used again in task duplications.
> but the array for the cached stacks is statically allocated by per-cpu api.
>  In this new implementation, the array for the cached stacks are dynamically
> allocted and freed by cpu hotplug callbacks and the cached stacks are freed
> when cpu is down. setup for cpu hotplug is established in fork_init().

Why do we want this? I can see that the follow up patch makes the number
configurable but the changelog doesn't describe the motivation for that.
Which workload would benefit from a higher value?

> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
> ---
>  kernel/fork.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 64 insertions(+), 17 deletions(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 61284d8..54421a9 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -167,26 +167,71 @@ void __weak arch_release_thread_stack(unsigned long *stack)
>   * flush.  Try to minimize the number of calls by caching stacks.
>   */
>  #define NR_CACHED_STACKS 2
> -static DEFINE_PER_CPU(struct vm_struct *, cached_stacks[NR_CACHED_STACKS]);
> +
> +struct vm_stack_cache {
> +	struct vm_struct **vm_stacks;
> +	int nr;
> +	int cur;
> +};
> +
> +static DEFINE_PER_CPU(struct vm_stack_cache, vm_stacks);
> +
> +static int alloc_vm_stack_cache(unsigned int cpu)
> +{
> +	struct vm_stack_cache *vm_stack_cache = &per_cpu(vm_stacks, cpu);
> +	struct vm_struct **vm_stacks = vm_stack_cache->vm_stacks;
> +	int i;
> +
> +	/* if free_vm_stack_cache() didn't free it */
> +	if (!vm_stacks) {
> +		vm_stacks =
> +			vzalloc(sizeof(struct vm_struct *) * NR_CACHED_STACKS);
> +		if (!vm_stacks)
> +			return -ENOMEM;
> +	}
> +
> +	vm_stack_cache->vm_stacks = vm_stacks;
> +	vm_stack_cache->cur = 0;
> +	vm_stack_cache->nr = 0;
> +
> +	return 0;
> +}
> +
> +static int free_vm_stack_cache(unsigned int cpu)
> +{
> +	struct vm_stack_cache *vm_stack_cache = &per_cpu(vm_stacks, cpu);
> +	struct vm_struct **vm_stacks = vm_stack_cache->vm_stacks;
> +	int i;
> +
> +	for (i = 0; i < vm_stack_cache->nr; i++) {
> +		vfree(vm_stacks[i]->addr);
> +		vm_stacks[i] = NULL;
> +	}
> +
> +	vm_stack_cache->nr = 0;
> +	vm_stack_cache->cur = 0;
> +	/* do not free vm_stack[cpu]->vm_stacks itself, reused in allocation */
> +
> +	return 0;
> +}
> +
>  #endif
>  
>  static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
>  {
>  #ifdef CONFIG_VMAP_STACK
> +	struct vm_stack_cache *vm_stack_cache =
> +		&per_cpu(vm_stacks, smp_processor_id());
> +	struct vm_struct **vm_stacks = vm_stack_cache->vm_stacks;
>  	void *stack;
> -	int i;
>  
>  	local_irq_disable();
> -	for (i = 0; i < NR_CACHED_STACKS; i++) {
> -		struct vm_struct *s = this_cpu_read(cached_stacks[i]);
> -
> -		if (!s)
> -			continue;
> -		this_cpu_write(cached_stacks[i], NULL);
> -
> -		tsk->stack_vm_area = s;
> +	if (vm_stack_cache->cur > 0) {
> +		struct vm_struct *vm_stack = vm_stacks[--vm_stack_cache->cur];
> +		tsk->stack_vm_area = vm_stack;
>  		local_irq_enable();
> -		return s->addr;
> +
> +		return vm_stack->addr;
>  	}
>  	local_irq_enable();
>  
> @@ -216,15 +261,14 @@ static inline void free_thread_stack(struct task_struct *tsk)
>  {
>  #ifdef CONFIG_VMAP_STACK
>  	if (task_stack_vm_area(tsk)) {
> +		struct vm_stack_cache *vm_stack_cache =
> +			&per_cpu(vm_stacks, smp_processor_id());
> +		struct vm_struct **vm_stacks = vm_stack_cache->vm_stacks;
>  		unsigned long flags;
> -		int i;
>  
>  		local_irq_save(flags);
> -		for (i = 0; i < NR_CACHED_STACKS; i++) {
> -			if (this_cpu_read(cached_stacks[i]))
> -				continue;
> -
> -			this_cpu_write(cached_stacks[i], tsk->stack_vm_area);
> +		if (vm_stack_cache->cur < vm_stack_cache->nr) {
> +			vm_stacks[vm_stack_cache->cur++] = tsk->stack_vm_area;
>  			local_irq_restore(flags);
>  			return;
>  		}
> @@ -456,6 +500,9 @@ void __init fork_init(void)
>  	for (i = 0; i < UCOUNT_COUNTS; i++) {
>  		init_user_ns.ucount_max[i] = max_threads/2;
>  	}
> +
> +	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "vm_stack_cache",
> +			  alloc_vm_stack_cache, free_vm_stack_cache);
>  }
>  
>  int __weak arch_dup_task_struct(struct task_struct *dst,
> -- 
> 2.7.4
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/3] fork: dynamically allocate cache array for vmapped stacks using cpuhp

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1706 bytes --]

Hi Hoeun,

[auto build test ERROR on next-20170203]
[also build test ERROR on v4.10-rc6]
[cannot apply to linus/master linux/master v4.9-rc8 v4.9-rc7 v4.9-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hoeun-Ryu/fork-dynamically-allocate-cache-array-for-vmapped-stacks-using-cpuhp/20170203-234431
config: i386-randconfig-x004-201705 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   kernel/fork.c: In function 'fork_init':
>> kernel/fork.c:505:6: error: 'alloc_vm_stack_cache' undeclared (first use in this function)
         alloc_vm_stack_cache, free_vm_stack_cache);
         ^~~~~~~~~~~~~~~~~~~~
   kernel/fork.c:505:6: note: each undeclared identifier is reported only once for each function it appears in
>> kernel/fork.c:505:28: error: 'free_vm_stack_cache' undeclared (first use in this function)
         alloc_vm_stack_cache, free_vm_stack_cache);
                               ^~~~~~~~~~~~~~~~~~~

vim +/alloc_vm_stack_cache +505 kernel/fork.c

   499	
   500		for (i = 0; i < UCOUNT_COUNTS; i++) {
   501			init_user_ns.ucount_max[i] = max_threads/2;
   502		}
   503	
   504		cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "vm_stack_cache",
 > 505				  alloc_vm_stack_cache, free_vm_stack_cache);
   506	}
   507	
   508	int __weak arch_dup_task_struct(struct task_struct *dst,

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31240 bytes --]

Re: [PATCH 1/3] fork: dynamically allocate cache array for vmapped stacks using cpuhp

[Hoeun Ryu]

On Sat, Feb 4, 2017 at 12:39 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Sat 04-02-17 00:30:05, Hoeun Ryu wrote:
>>  Using virtually mapped stack, kernel stacks are allocated via vmalloc.
>> In the current implementation, two stacks per cpu can be cached when
>> tasks are freed and the cached stacks are used again in task duplications.
>> but the array for the cached stacks is statically allocated by per-cpu api.
>>  In this new implementation, the array for the cached stacks are dynamically
>> allocted and freed by cpu hotplug callbacks and the cached stacks are freed
>> when cpu is down. setup for cpu hotplug is established in fork_init().
>
> Why do we want this? I can see that the follow up patch makes the number
> configurable but the changelog doesn't describe the motivation for that.
> Which workload would benefit from a higher value?
>

The key difference of this implementation, the cached stacks for a cpu
is freed when a cpu is down.
so the cached stacks are no longer wasted.
In the current implementation, the cached stacks for a cpu still
remain on the system when a cpu is down.
I think we could imagine what if a machine has many cpus and someone
wants to have bigger size of stack caches.

>> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
>> ---
>>  kernel/fork.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 64 insertions(+), 17 deletions(-)
>>
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> index 61284d8..54421a9 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -167,26 +167,71 @@ void __weak arch_release_thread_stack(unsigned long *stack)
>>   * flush.  Try to minimize the number of calls by caching stacks.
>>   */
>>  #define NR_CACHED_STACKS 2
>> -static DEFINE_PER_CPU(struct vm_struct *, cached_stacks[NR_CACHED_STACKS]);
>> +
>> +struct vm_stack_cache {
>> +     struct vm_struct **vm_stacks;
>> +     int nr;
>> +     int cur;
>> +};
>> +
>> +static DEFINE_PER_CPU(struct vm_stack_cache, vm_stacks);
>> +
>> +static int alloc_vm_stack_cache(unsigned int cpu)
>> +{
>> +     struct vm_stack_cache *vm_stack_cache = &per_cpu(vm_stacks, cpu);
>> +     struct vm_struct **vm_stacks = vm_stack_cache->vm_stacks;
>> +     int i;
>> +
>> +     /* if free_vm_stack_cache() didn't free it */
>> +     if (!vm_stacks) {
>> +             vm_stacks =
>> +                     vzalloc(sizeof(struct vm_struct *) * NR_CACHED_STACKS);
>> +             if (!vm_stacks)
>> +                     return -ENOMEM;
>> +     }
>> +
>> +     vm_stack_cache->vm_stacks = vm_stacks;
>> +     vm_stack_cache->cur = 0;
>> +     vm_stack_cache->nr = 0;
>> +
>> +     return 0;
>> +}
>> +
>> +static int free_vm_stack_cache(unsigned int cpu)
>> +{
>> +     struct vm_stack_cache *vm_stack_cache = &per_cpu(vm_stacks, cpu);
>> +     struct vm_struct **vm_stacks = vm_stack_cache->vm_stacks;
>> +     int i;
>> +
>> +     for (i = 0; i < vm_stack_cache->nr; i++) {
>> +             vfree(vm_stacks[i]->addr);
>> +             vm_stacks[i] = NULL;
>> +     }
>> +
>> +     vm_stack_cache->nr = 0;
>> +     vm_stack_cache->cur = 0;
>> +     /* do not free vm_stack[cpu]->vm_stacks itself, reused in allocation */
>> +
>> +     return 0;
>> +}
>> +
>>  #endif
>>
>>  static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
>>  {
>>  #ifdef CONFIG_VMAP_STACK
>> +     struct vm_stack_cache *vm_stack_cache =
>> +             &per_cpu(vm_stacks, smp_processor_id());
>> +     struct vm_struct **vm_stacks = vm_stack_cache->vm_stacks;
>>       void *stack;
>> -     int i;
>>
>>       local_irq_disable();
>> -     for (i = 0; i < NR_CACHED_STACKS; i++) {
>> -             struct vm_struct *s = this_cpu_read(cached_stacks[i]);
>> -
>> -             if (!s)
>> -                     continue;
>> -             this_cpu_write(cached_stacks[i], NULL);
>> -
>> -             tsk->stack_vm_area = s;
>> +     if (vm_stack_cache->cur > 0) {
>> +             struct vm_struct *vm_stack = vm_stacks[--vm_stack_cache->cur];
>> +             tsk->stack_vm_area = vm_stack;
>>               local_irq_enable();
>> -             return s->addr;
>> +
>> +             return vm_stack->addr;
>>       }
>>       local_irq_enable();
>>
>> @@ -216,15 +261,14 @@ static inline void free_thread_stack(struct task_struct *tsk)
>>  {
>>  #ifdef CONFIG_VMAP_STACK
>>       if (task_stack_vm_area(tsk)) {
>> +             struct vm_stack_cache *vm_stack_cache =
>> +                     &per_cpu(vm_stacks, smp_processor_id());
>> +             struct vm_struct **vm_stacks = vm_stack_cache->vm_stacks;
>>               unsigned long flags;
>> -             int i;
>>
>>               local_irq_save(flags);
>> -             for (i = 0; i < NR_CACHED_STACKS; i++) {
>> -                     if (this_cpu_read(cached_stacks[i]))
>> -                             continue;
>> -
>> -                     this_cpu_write(cached_stacks[i], tsk->stack_vm_area);
>> +             if (vm_stack_cache->cur < vm_stack_cache->nr) {
>> +                     vm_stacks[vm_stack_cache->cur++] = tsk->stack_vm_area;
>>                       local_irq_restore(flags);
>>                       return;
>>               }
>> @@ -456,6 +500,9 @@ void __init fork_init(void)
>>       for (i = 0; i < UCOUNT_COUNTS; i++) {
>>               init_user_ns.ucount_max[i] = max_threads/2;
>>       }
>> +
>> +     cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "vm_stack_cache",
>> +                       alloc_vm_stack_cache, free_vm_stack_cache);
>>  }
>>
>>  int __weak arch_dup_task_struct(struct task_struct *dst,
>> --
>> 2.7.4
>>
>
> --
> Michal Hocko
> SUSE Labs

Re: [PATCH 1/3] fork: dynamically allocate cache array for vmapped stacks using cpuhp

[Michal Hocko]

On Sat 04-02-17 01:42:56, Hoeun Ryu wrote:
> On Sat, Feb 4, 2017 at 12:39 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Sat 04-02-17 00:30:05, Hoeun Ryu wrote:
> >>  Using virtually mapped stack, kernel stacks are allocated via vmalloc.
> >> In the current implementation, two stacks per cpu can be cached when
> >> tasks are freed and the cached stacks are used again in task duplications.
> >> but the array for the cached stacks is statically allocated by per-cpu api.
> >>  In this new implementation, the array for the cached stacks are dynamically
> >> allocted and freed by cpu hotplug callbacks and the cached stacks are freed
> >> when cpu is down. setup for cpu hotplug is established in fork_init().
> >
> > Why do we want this? I can see that the follow up patch makes the number
> > configurable but the changelog doesn't describe the motivation for that.
> > Which workload would benefit from a higher value?
> >
> 
> The key difference of this implementation, the cached stacks for a cpu
> is freed when a cpu is down.
> so the cached stacks are no longer wasted.
> In the current implementation, the cached stacks for a cpu still
> remain on the system when a cpu is down.

Yes, that is true but cpu offline operation is just too rare for this to
matter all that much I believe. More importantly, though, the current
implementation could be easily fixed as well without reworking how
the caching works. If there are workloads where the wastage really
matters then please try to fix it with the current caching scheme before
extending it for larger caches. This would make it easier to backport to
older kernels.

> I think we could imagine what if a machine has many cpus and someone
> wants to have bigger size of stack caches.

Without being more specific who might want the bigger caches and why
this sounds like an insufficient justification to replace the current
(simpler) caching.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/3] fork: dynamically allocate cache array for vmapped stacks using cpuhp

[Andy Lutomirski]

On Fri, Feb 3, 2017 at 8:42 AM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
> On Sat, Feb 4, 2017 at 12:39 AM, Michal Hocko <mhocko@kernel.org> wrote:
>> On Sat 04-02-17 00:30:05, Hoeun Ryu wrote:
>>>  Using virtually mapped stack, kernel stacks are allocated via vmalloc.
>>> In the current implementation, two stacks per cpu can be cached when
>>> tasks are freed and the cached stacks are used again in task duplications.
>>> but the array for the cached stacks is statically allocated by per-cpu api.
>>>  In this new implementation, the array for the cached stacks are dynamically
>>> allocted and freed by cpu hotplug callbacks and the cached stacks are freed
>>> when cpu is down. setup for cpu hotplug is established in fork_init().
>>
>> Why do we want this? I can see that the follow up patch makes the number
>> configurable but the changelog doesn't describe the motivation for that.
>> Which workload would benefit from a higher value?
>>
>
> The key difference of this implementation, the cached stacks for a cpu
> is freed when a cpu is down.
> so the cached stacks are no longer wasted.
> In the current implementation, the cached stacks for a cpu still
> remain on the system when a cpu is down.
> I think we could imagine what if a machine has many cpus and someone
> wants to have bigger size of stack caches.

Then how about just registering a simple hotplug hook to free the
stacks without worrying about freeing the tiny array as well?

--Andy

Re: [PATCH 1/3] fork: dynamically allocate cache array for vmapped stacks using cpuhp

[Hoeun Ryu]

On Sat, Feb 4, 2017 at 2:52 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Fri, Feb 3, 2017 at 8:42 AM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
>> On Sat, Feb 4, 2017 at 12:39 AM, Michal Hocko <mhocko@kernel.org> wrote:
>>> On Sat 04-02-17 00:30:05, Hoeun Ryu wrote:
>>>>  Using virtually mapped stack, kernel stacks are allocated via vmalloc.
>>>> In the current implementation, two stacks per cpu can be cached when
>>>> tasks are freed and the cached stacks are used again in task duplications.
>>>> but the array for the cached stacks is statically allocated by per-cpu api.
>>>>  In this new implementation, the array for the cached stacks are dynamically
>>>> allocted and freed by cpu hotplug callbacks and the cached stacks are freed
>>>> when cpu is down. setup for cpu hotplug is established in fork_init().
>>>
>>> Why do we want this? I can see that the follow up patch makes the number
>>> configurable but the changelog doesn't describe the motivation for that.
>>> Which workload would benefit from a higher value?
>>>
>>
>> The key difference of this implementation, the cached stacks for a cpu
>> is freed when a cpu is down.
>> so the cached stacks are no longer wasted.
>> In the current implementation, the cached stacks for a cpu still
>> remain on the system when a cpu is down.
>> I think we could imagine what if a machine has many cpus and someone
>> wants to have bigger size of stack caches.
>
> Then how about just registering a simple hotplug hook to free the
> stacks without worrying about freeing the tiny array as well?
>

Michal, What do you think about it. it sounds fair enough.

> --Andy

Re: [PATCH 1/3] fork: dynamically allocate cache array for vmapped stacks using cpuhp

[Michal Hocko]

On Sat 04-02-17 11:01:32, Hoeun Ryu wrote:
> On Sat, Feb 4, 2017 at 2:52 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> > On Fri, Feb 3, 2017 at 8:42 AM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
> >> On Sat, Feb 4, 2017 at 12:39 AM, Michal Hocko <mhocko@kernel.org> wrote:
> >>> On Sat 04-02-17 00:30:05, Hoeun Ryu wrote:
> >>>>  Using virtually mapped stack, kernel stacks are allocated via vmalloc.
> >>>> In the current implementation, two stacks per cpu can be cached when
> >>>> tasks are freed and the cached stacks are used again in task duplications.
> >>>> but the array for the cached stacks is statically allocated by per-cpu api.
> >>>>  In this new implementation, the array for the cached stacks are dynamically
> >>>> allocted and freed by cpu hotplug callbacks and the cached stacks are freed
> >>>> when cpu is down. setup for cpu hotplug is established in fork_init().
> >>>
> >>> Why do we want this? I can see that the follow up patch makes the number
> >>> configurable but the changelog doesn't describe the motivation for that.
> >>> Which workload would benefit from a higher value?
> >>>
> >>
> >> The key difference of this implementation, the cached stacks for a cpu
> >> is freed when a cpu is down.
> >> so the cached stacks are no longer wasted.
> >> In the current implementation, the cached stacks for a cpu still
> >> remain on the system when a cpu is down.
> >> I think we could imagine what if a machine has many cpus and someone
> >> wants to have bigger size of stack caches.
> >
> > Then how about just registering a simple hotplug hook to free the
> > stacks without worrying about freeing the tiny array as well?
> >
> 
> Michal, What do you think about it. it sounds fair enough.

This is what I've tried to suggest in the other reply.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/3] fork: dynamically allocate cache array for vmapped stacks using cpuhp

[Hoeun Ryu]

On Sun, Feb 5, 2017 at 7:18 PM, Michal Hocko <mhocko@kernel.org> wrote:
> On Sat 04-02-17 11:01:32, Hoeun Ryu wrote:
>> On Sat, Feb 4, 2017 at 2:52 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>> > On Fri, Feb 3, 2017 at 8:42 AM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
>> >> On Sat, Feb 4, 2017 at 12:39 AM, Michal Hocko <mhocko@kernel.org> wrote:
>> >>> On Sat 04-02-17 00:30:05, Hoeun Ryu wrote:
>> >>>>  Using virtually mapped stack, kernel stacks are allocated via vmalloc.
>> >>>> In the current implementation, two stacks per cpu can be cached when
>> >>>> tasks are freed and the cached stacks are used again in task duplications.
>> >>>> but the array for the cached stacks is statically allocated by per-cpu api.
>> >>>>  In this new implementation, the array for the cached stacks are dynamically
>> >>>> allocted and freed by cpu hotplug callbacks and the cached stacks are freed
>> >>>> when cpu is down. setup for cpu hotplug is established in fork_init().
>> >>>
>> >>> Why do we want this? I can see that the follow up patch makes the number
>> >>> configurable but the changelog doesn't describe the motivation for that.
>> >>> Which workload would benefit from a higher value?
>> >>>
>> >>
>> >> The key difference of this implementation, the cached stacks for a cpu
>> >> is freed when a cpu is down.
>> >> so the cached stacks are no longer wasted.
>> >> In the current implementation, the cached stacks for a cpu still
>> >> remain on the system when a cpu is down.
>> >> I think we could imagine what if a machine has many cpus and someone
>> >> wants to have bigger size of stack caches.
>> >
>> > Then how about just registering a simple hotplug hook to free the
>> > stacks without worrying about freeing the tiny array as well?
>> >
>>
>> Michal, What do you think about it. it sounds fair enough.
>
> This is what I've tried to suggest in the other reply.

OK, I'll work on patch1/2 again and drop patch3.

> --
> Michal Hocko
> SUSE Labs