Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1578282 > unrolled thread

[PATCH v3] fork: free vmapped stacks in cache when cpus are offline

Started byHoeun Ryu <hoeun.ryu@gmail.com>
First post2017-02-10 09:40 +0100
Last post2017-02-11 00:40 +0100
Articles 10 — 3 participants

Back to article view | Back to linux.kernel


Contents

  [PATCH v3] fork: free vmapped stacks in cache when cpus are offline Hoeun Ryu <hoeun.ryu@gmail.com> - 2017-02-10 09:40 +0100
    Re: [PATCH v3] fork: free vmapped stacks in cache when cpus are  offline Michal Hocko <mhocko@kernel.org> - 2017-02-10 13:10 +0100
      Re: [PATCH v3] fork: free vmapped stacks in cache when cpus are offline Hoeun Ryu <hoeun.ryu@gmail.com> - 2017-02-10 15:40 +0100
        Re: [PATCH v3] fork: free vmapped stacks in cache when cpus are  offline Michal Hocko <mhocko@kernel.org> - 2017-02-10 15:50 +0100
          Re: [PATCH v3] fork: free vmapped stacks in cache when cpus are offline Hoeun Ryu <hoeun.ryu@gmail.com> - 2017-02-10 16:10 +0100
            Re: [PATCH v3] fork: free vmapped stacks in cache when cpus are  offline Thomas Gleixner <tglx@linutronix.de> - 2017-02-10 16:40 +0100
          Re: [PATCH v3] fork: free vmapped stacks in cache when cpus are  offline Thomas Gleixner <tglx@linutronix.de> - 2017-02-10 16:40 +0100
            Re: [PATCH v3] fork: free vmapped stacks in cache when cpus are offline Hoeun Ryu <hoeun.ryu@gmail.com> - 2017-02-10 18:00 +0100
              Re: [PATCH v3] fork: free vmapped stacks in cache when cpus are  offline Thomas Gleixner <tglx@linutronix.de> - 2017-02-10 19:00 +0100
                Re: [PATCH v3] fork: free vmapped stacks in cache when cpus are offline Hoeun Ryu <hoeun.ryu@gmail.com> - 2017-02-11 00:40 +0100

#1578282 — [PATCH v3] fork: free vmapped stacks in cache when cpus are offline

FromHoeun Ryu <hoeun.ryu@gmail.com>
Date2017-02-10 09:40 +0100
Subject[PATCH v3] fork: free vmapped stacks in cache when cpus are offline
Message-ID<t9eUi-105-17@gated-at.bofh.it>
 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 cached stacks may remain unfreed even when cpu are offline.
 By adding a cpu hotplug callback to free the cached stacks when a cpu
goes offline, the pages of the cached stacks are not wasted.

Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
---
Changes in v3:
 fix misuse of per-cpu api
 fix location of function definition within CONFIG_VMAP_STACK

Changes in v2:
 remove cpuhp callback for `startup`, only `teardown` callback is installed.

 kernel/fork.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/kernel/fork.c b/kernel/fork.c
index 937ba59..8fad87ba 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -168,6 +168,24 @@ void __weak arch_release_thread_stack(unsigned long *stack)
  */
 #define NR_CACHED_STACKS 2
 static DEFINE_PER_CPU(struct vm_struct *, cached_stacks[NR_CACHED_STACKS]);
+
+static int free_vm_stack_cache(unsigned int cpu)
+{
+	struct vm_struct **cached_vm_stacks = per_cpu_ptr(cached_stacks, cpu);
+	int i;
+
+	for (i = 0; i < NR_CACHED_STACKS; i++) {
+		struct vm_struct **vm_stack = &cached_vm_stacks[i];
+
+		if (*vm_stack == NULL)
+			continue;
+
+		vfree((*vm_stack)->addr);
+		*vm_stack = NULL;
+	}
+
+	return 0;
+}
 #endif
 
 static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
@@ -456,6 +474,11 @@ void __init fork_init(void)
 	for (i = 0; i < UCOUNT_COUNTS; i++) {
 		init_user_ns.ucount_max[i] = max_threads/2;
 	}
+
+#ifdef CONFIG_VMAP_STACK
+	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "vm_stack_cache",
+			  NULL, free_vm_stack_cache);
+#endif
 }
 
 int __weak arch_dup_task_struct(struct task_struct *dst,
-- 
2.7.4

[toc] | [next] | [standalone]


#1578418 — Re: [PATCH v3] fork: free vmapped stacks in cache when cpus are offline

FromMichal Hocko <mhocko@kernel.org>
Date2017-02-10 13:10 +0100
SubjectRe: [PATCH v3] fork: free vmapped stacks in cache when cpus are offline
Message-ID<t9ibx-3dS-27@gated-at.bofh.it>
In reply to#1578282
On Fri 10-02-17 17:32:07, Hoeun Ryu wrote:
[...]
> +static int free_vm_stack_cache(unsigned int cpu)
> +{
> +	struct vm_struct **cached_vm_stacks = per_cpu_ptr(cached_stacks, cpu);
> +	int i;
> +
> +	for (i = 0; i < NR_CACHED_STACKS; i++) {
> +		struct vm_struct **vm_stack = &cached_vm_stacks[i];
> +
> +		if (*vm_stack == NULL)
> +			continue;
> +
> +		vfree((*vm_stack)->addr);
> +		*vm_stack = NULL;

this seems more obscure than necessary. Probably a matter of taste but I
would find the following easier to read
		struct vm_struct *vm_stack = cached_vm_stacks[i];

		if (!vm_stack)
			continue;

		vfree(vm_stack);
		cached_vm_stacks[i] = NULL;

> +	}
> +
> +	return 0;
> +}
>  #endif
>  
>  static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
> @@ -456,6 +474,11 @@ void __init fork_init(void)
>  	for (i = 0; i < UCOUNT_COUNTS; i++) {
>  		init_user_ns.ucount_max[i] = max_threads/2;
>  	}
> +
> +#ifdef CONFIG_VMAP_STACK
> +	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "vm_stack_cache",
> +			  NULL, free_vm_stack_cache);
> +#endif

I am not familiar the new hotplug infrastructure so I might be missing
something. CPUHP_AP_ONLINE_DYN will allocate a state which is has only
30 slots available. The name also suggests this will be called on an
online event. Why doesn't this have its own state like other users. The
name should also reflect offline event CPUHP_STACK_CACHE_DEAD or
something like that.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]


#1578513

FromHoeun Ryu <hoeun.ryu@gmail.com>
Date2017-02-10 15:40 +0100
Message-ID<t9kwG-4AQ-23@gated-at.bofh.it>
In reply to#1578418
On Fri, Feb 10, 2017 at 9:05 PM, Michal Hocko <mhocko@kernel.org> wrote:
> On Fri 10-02-17 17:32:07, Hoeun Ryu wrote:
> [...]
>> +static int free_vm_stack_cache(unsigned int cpu)
>> +{
>> +     struct vm_struct **cached_vm_stacks = per_cpu_ptr(cached_stacks, cpu);
>> +     int i;
>> +
>> +     for (i = 0; i < NR_CACHED_STACKS; i++) {
>> +             struct vm_struct **vm_stack = &cached_vm_stacks[i];
>> +
>> +             if (*vm_stack == NULL)
>> +                     continue;
>> +
>> +             vfree((*vm_stack)->addr);
>> +             *vm_stack = NULL;
>
> this seems more obscure than necessary. Probably a matter of taste but I
> would find the following easier to read
>                 struct vm_struct *vm_stack = cached_vm_stacks[i];
>
>                 if (!vm_stack)
>                         continue;
>
>                 vfree(vm_stack);
>                 cached_vm_stacks[i] = NULL;
>

OK, it's easier to read, I'll fix it.

>> +     }
>> +
>> +     return 0;
>> +}
>>  #endif
>>
>>  static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
>> @@ -456,6 +474,11 @@ void __init fork_init(void)
>>       for (i = 0; i < UCOUNT_COUNTS; i++) {
>>               init_user_ns.ucount_max[i] = max_threads/2;
>>       }
>> +
>> +#ifdef CONFIG_VMAP_STACK
>> +     cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "vm_stack_cache",
>> +                       NULL, free_vm_stack_cache);
>> +#endif
>
> I am not familiar the new hotplug infrastructure so I might be missing
> something. CPUHP_AP_ONLINE_DYN will allocate a state which is has only
> 30 slots available. The name also suggests this will be called on an
> online event. Why doesn't this have its own state like other users. The
> name should also reflect offline event CPUHP_STACK_CACHE_DEAD or
> something like that.

I'll define CPUHP_VMSTACK_CACHE_DEAD before CPUHP_BP_PREPARE_DYN in
cpuhotplug.h.
Do you think the change is made in a separate patch or not ?

Thank you for your review anyway.

>
> --
> Michal Hocko
> SUSE Labs

[toc] | [prev] | [next] | [standalone]


#1578517 — Re: [PATCH v3] fork: free vmapped stacks in cache when cpus are offline

FromMichal Hocko <mhocko@kernel.org>
Date2017-02-10 15:50 +0100
SubjectRe: [PATCH v3] fork: free vmapped stacks in cache when cpus are offline
Message-ID<t9kGm-4F0-3@gated-at.bofh.it>
In reply to#1578513
On Fri 10-02-17 23:31:41, Hoeun Ryu wrote:
> On Fri, Feb 10, 2017 at 9:05 PM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Fri 10-02-17 17:32:07, Hoeun Ryu wrote:
[...]
> >>  static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
> >> @@ -456,6 +474,11 @@ void __init fork_init(void)
> >>       for (i = 0; i < UCOUNT_COUNTS; i++) {
> >>               init_user_ns.ucount_max[i] = max_threads/2;
> >>       }
> >> +
> >> +#ifdef CONFIG_VMAP_STACK
> >> +     cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "vm_stack_cache",
> >> +                       NULL, free_vm_stack_cache);
> >> +#endif
> >
> > I am not familiar the new hotplug infrastructure so I might be missing
> > something. CPUHP_AP_ONLINE_DYN will allocate a state which is has only
> > 30 slots available. The name also suggests this will be called on an
> > online event. Why doesn't this have its own state like other users. The
> > name should also reflect offline event CPUHP_STACK_CACHE_DEAD or
> > something like that.
> 
> I'll define CPUHP_VMSTACK_CACHE_DEAD before CPUHP_BP_PREPARE_DYN in
> cpuhotplug.h.
> Do you think the change is made in a separate patch or not ?

I think it should be in a single patch. I am not sure what are the rules
to define a new state though. Let's CC Thomas.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]


#1578540

FromHoeun Ryu <hoeun.ryu@gmail.com>
Date2017-02-10 16:10 +0100
Message-ID<t9kZI-52n-25@gated-at.bofh.it>
In reply to#1578517
On Fri, Feb 10, 2017 at 11:41 PM, Michal Hocko <mhocko@kernel.org> wrote:
> On Fri 10-02-17 23:31:41, Hoeun Ryu wrote:
>> On Fri, Feb 10, 2017 at 9:05 PM, Michal Hocko <mhocko@kernel.org> wrote:
>> > On Fri 10-02-17 17:32:07, Hoeun Ryu wrote:
> [...]
>> >>  static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
>> >> @@ -456,6 +474,11 @@ void __init fork_init(void)
>> >>       for (i = 0; i < UCOUNT_COUNTS; i++) {
>> >>               init_user_ns.ucount_max[i] = max_threads/2;
>> >>       }
>> >> +
>> >> +#ifdef CONFIG_VMAP_STACK
>> >> +     cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "vm_stack_cache",
>> >> +                       NULL, free_vm_stack_cache);
>> >> +#endif
>> >
>> > I am not familiar the new hotplug infrastructure so I might be missing
>> > something. CPUHP_AP_ONLINE_DYN will allocate a state which is has only
>> > 30 slots available. The name also suggests this will be called on an
>> > online event. Why doesn't this have its own state like other users. The
>> > name should also reflect offline event CPUHP_STACK_CACHE_DEAD or
>> > something like that.
>>
>> I'll define CPUHP_VMSTACK_CACHE_DEAD before CPUHP_BP_PREPARE_DYN in
>> cpuhotplug.h.
>> Do you think the change is made in a separate patch or not ?
>
> I think it should be in a single patch. I am not sure what are the rules
> to define a new state though. Let's CC Thomas.

OK, I will.

> --
> Michal Hocko
> SUSE Labs

[toc] | [prev] | [next] | [standalone]


#1578556 — Re: [PATCH v3] fork: free vmapped stacks in cache when cpus are offline

FromThomas Gleixner <tglx@linutronix.de>
Date2017-02-10 16:40 +0100
SubjectRe: [PATCH v3] fork: free vmapped stacks in cache when cpus are offline
Message-ID<t9lsK-5dQ-19@gated-at.bofh.it>
In reply to#1578540

On Sat, 11 Feb 2017, Hoeun Ryu wrote:

> On Fri, Feb 10, 2017 at 11:41 PM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Fri 10-02-17 23:31:41, Hoeun Ryu wrote:
> >> On Fri, Feb 10, 2017 at 9:05 PM, Michal Hocko <mhocko@kernel.org> wrote:
> >> > On Fri 10-02-17 17:32:07, Hoeun Ryu wrote:
> > [...]
> >> >>  static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
> >> >> @@ -456,6 +474,11 @@ void __init fork_init(void)
> >> >>       for (i = 0; i < UCOUNT_COUNTS; i++) {
> >> >>               init_user_ns.ucount_max[i] = max_threads/2;
> >> >>       }
> >> >> +
> >> >> +#ifdef CONFIG_VMAP_STACK
> >> >> +     cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "vm_stack_cache",
> >> >> +                       NULL, free_vm_stack_cache);
> >> >> +#endif
> >> >
> >> > I am not familiar the new hotplug infrastructure so I might be missing
> >> > something. CPUHP_AP_ONLINE_DYN will allocate a state which is has only
> >> > 30 slots available.

That's enough and when we hit that limit we just up it.

> >> > The name also suggests this will be called on an online event.

The states are symmetric.

[toc] | [prev] | [next] | [standalone]


#1578552 — Re: [PATCH v3] fork: free vmapped stacks in cache when cpus are offline

FromThomas Gleixner <tglx@linutronix.de>
Date2017-02-10 16:40 +0100
SubjectRe: [PATCH v3] fork: free vmapped stacks in cache when cpus are offline
Message-ID<t9lsJ-5dQ-1@gated-at.bofh.it>
In reply to#1578517
On Fri, 10 Feb 2017, Michal Hocko wrote:
> On Fri 10-02-17 23:31:41, Hoeun Ryu wrote:
> > On Fri, Feb 10, 2017 at 9:05 PM, Michal Hocko <mhocko@kernel.org> wrote:
> > > On Fri 10-02-17 17:32:07, Hoeun Ryu wrote:
> [...]
> > >>  static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
> > >> @@ -456,6 +474,11 @@ void __init fork_init(void)
> > >>       for (i = 0; i < UCOUNT_COUNTS; i++) {
> > >>               init_user_ns.ucount_max[i] = max_threads/2;
> > >>       }
> > >> +
> > >> +#ifdef CONFIG_VMAP_STACK
> > >> +     cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "vm_stack_cache",
> > >> +                       NULL, free_vm_stack_cache);
> > >> +#endif
> > >
> > > I am not familiar the new hotplug infrastructure so I might be missing
> > > something. CPUHP_AP_ONLINE_DYN will allocate a state which is has only
> > > 30 slots available. The name also suggests this will be called on an
> > > online event. Why doesn't this have its own state like other users. The
> > > name should also reflect offline event CPUHP_STACK_CACHE_DEAD or
> > > something like that.
> > 
> > I'll define CPUHP_VMSTACK_CACHE_DEAD before CPUHP_BP_PREPARE_DYN in
> > cpuhotplug.h.
> > Do you think the change is made in a separate patch or not ?
> 
> I think it should be in a single patch. I am not sure what are the rules
> to define a new state though. Let's CC Thomas.

So the first question is where do you want that to be called? i.e. in which
section:

CPU up		CPU down

PREPARE		DEAD		<- called on some other CPU
ONLINE		DOWN		<- called on the hotplugged CPU

And then the next question is whether you have ordering constraints,
i.e. it must be called before or after some other callback. Only in that
case you want to have an explicit state. If not, just use a dynamically
allocated one.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]


#1578624

FromHoeun Ryu <hoeun.ryu@gmail.com>
Date2017-02-10 18:00 +0100
Message-ID<t9mIb-5ZB-39@gated-at.bofh.it>
In reply to#1578552
On Sat, Feb 11, 2017 at 12:32 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 10 Feb 2017, Michal Hocko wrote:
>> On Fri 10-02-17 23:31:41, Hoeun Ryu wrote:
>> > On Fri, Feb 10, 2017 at 9:05 PM, Michal Hocko <mhocko@kernel.org> wrote:
>> > > On Fri 10-02-17 17:32:07, Hoeun Ryu wrote:
>> [...]
>> > >>  static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
>> > >> @@ -456,6 +474,11 @@ void __init fork_init(void)
>> > >>       for (i = 0; i < UCOUNT_COUNTS; i++) {
>> > >>               init_user_ns.ucount_max[i] = max_threads/2;
>> > >>       }
>> > >> +
>> > >> +#ifdef CONFIG_VMAP_STACK
>> > >> +     cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "vm_stack_cache",
>> > >> +                       NULL, free_vm_stack_cache);
>> > >> +#endif
>> > >
>> > > I am not familiar the new hotplug infrastructure so I might be missing
>> > > something. CPUHP_AP_ONLINE_DYN will allocate a state which is has only
>> > > 30 slots available. The name also suggests this will be called on an
>> > > online event. Why doesn't this have its own state like other users. The
>> > > name should also reflect offline event CPUHP_STACK_CACHE_DEAD or
>> > > something like that.
>> >
>> > I'll define CPUHP_VMSTACK_CACHE_DEAD before CPUHP_BP_PREPARE_DYN in
>> > cpuhotplug.h.
>> > Do you think the change is made in a separate patch or not ?
>>
>> I think it should be in a single patch. I am not sure what are the rules
>> to define a new state though. Let's CC Thomas.
>
> So the first question is where do you want that to be called? i.e. in which
> section:
>
> CPU up          CPU down
>
> PREPARE         DEAD            <- called on some other CPU
> ONLINE          DOWN            <- called on the hotplugged CPU
>

It doesn't matter whether the callback is called on the hotplugged CPU
or other CPUs.

> And then the next question is whether you have ordering constraints,
> i.e. it must be called before or after some other callback. Only in that
> case you want to have an explicit state. If not, just use a dynamically
> allocated one.

The cache is for virtually mapped kernel stacks. so I think the
callback should be called after all tasks are migrated to other CPUs.
It must reside before CPUHP_AP_SCHED_STARTING or CPUHP_BRINGUP_CPU.

>
> Thanks,
>
>         tglx

[toc] | [prev] | [next] | [standalone]


#1578653 — Re: [PATCH v3] fork: free vmapped stacks in cache when cpus are offline

FromThomas Gleixner <tglx@linutronix.de>
Date2017-02-10 19:00 +0100
SubjectRe: [PATCH v3] fork: free vmapped stacks in cache when cpus are offline
Message-ID<t9nEe-6Co-31@gated-at.bofh.it>
In reply to#1578624
On Sat, 11 Feb 2017, Hoeun Ryu wrote:
> On Sat, Feb 11, 2017 at 12:32 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Fri, 10 Feb 2017, Michal Hocko wrote:
> >> On Fri 10-02-17 23:31:41, Hoeun Ryu wrote:
> >> > On Fri, Feb 10, 2017 at 9:05 PM, Michal Hocko <mhocko@kernel.org> wrote:
> >> > > On Fri 10-02-17 17:32:07, Hoeun Ryu wrote:
> >> [...]
> >> > >>  static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
> >> > >> @@ -456,6 +474,11 @@ void __init fork_init(void)
> >> > >>       for (i = 0; i < UCOUNT_COUNTS; i++) {
> >> > >>               init_user_ns.ucount_max[i] = max_threads/2;
> >> > >>       }
> >> > >> +
> >> > >> +#ifdef CONFIG_VMAP_STACK
> >> > >> +     cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "vm_stack_cache",
> >> > >> +                       NULL, free_vm_stack_cache);
> >> > >> +#endif
> >> > >
> >> > > I am not familiar the new hotplug infrastructure so I might be missing
> >> > > something. CPUHP_AP_ONLINE_DYN will allocate a state which is has only
> >> > > 30 slots available. The name also suggests this will be called on an
> >> > > online event. Why doesn't this have its own state like other users. The
> >> > > name should also reflect offline event CPUHP_STACK_CACHE_DEAD or
> >> > > something like that.
> >> >
> >> > I'll define CPUHP_VMSTACK_CACHE_DEAD before CPUHP_BP_PREPARE_DYN in
> >> > cpuhotplug.h.
> >> > Do you think the change is made in a separate patch or not ?
> >>
> >> I think it should be in a single patch. I am not sure what are the rules
> >> to define a new state though. Let's CC Thomas.
> >
> > So the first question is where do you want that to be called? i.e. in which
> > section:
> >
> > CPU up          CPU down
> >
> > PREPARE         DEAD            <- called on some other CPU
> > ONLINE          DOWN            <- called on the hotplugged CPU
> >
> 
> It doesn't matter whether the callback is called on the hotplugged CPU
> or other CPUs.
>
> > And then the next question is whether you have ordering constraints,
> > i.e. it must be called before or after some other callback. Only in that
> > case you want to have an explicit state. If not, just use a dynamically
> > allocated one.
> 
> The cache is for virtually mapped kernel stacks. so I think the
> callback should be called after all tasks are migrated to other CPUs.
> It must reside before CPUHP_AP_SCHED_STARTING or CPUHP_BRINGUP_CPU.

Between AP_OFFLINE and AP_SCHED_STARTING does not work because those states
path are called on the hotplugged CPU with interrupts disabled and after
the CPU has been taken out from the scheduler.

So the proper place is the dynamic states CPUHP_BP_PREPARE_DYN. You do not
have a prepare callback, but then it's called on an still online CPU
_AFTER_ the hotplugged CPU vanished from the system.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]


#1578859

FromHoeun Ryu <hoeun.ryu@gmail.com>
Date2017-02-11 00:40 +0100
Message-ID<t9sXg-1A0-3@gated-at.bofh.it>
In reply to#1578653
On Sat, Feb 11, 2017 at 2:51 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Sat, 11 Feb 2017, Hoeun Ryu wrote:
>> On Sat, Feb 11, 2017 at 12:32 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > On Fri, 10 Feb 2017, Michal Hocko wrote:
>> >> On Fri 10-02-17 23:31:41, Hoeun Ryu wrote:
>> >> > On Fri, Feb 10, 2017 at 9:05 PM, Michal Hocko <mhocko@kernel.org> wrote:
>> >> > > On Fri 10-02-17 17:32:07, Hoeun Ryu wrote:
>> >> [...]
>> >> > >>  static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
>> >> > >> @@ -456,6 +474,11 @@ void __init fork_init(void)
>> >> > >>       for (i = 0; i < UCOUNT_COUNTS; i++) {
>> >> > >>               init_user_ns.ucount_max[i] = max_threads/2;
>> >> > >>       }
>> >> > >> +
>> >> > >> +#ifdef CONFIG_VMAP_STACK
>> >> > >> +     cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "vm_stack_cache",
>> >> > >> +                       NULL, free_vm_stack_cache);
>> >> > >> +#endif
>> >> > >
>> >> > > I am not familiar the new hotplug infrastructure so I might be missing
>> >> > > something. CPUHP_AP_ONLINE_DYN will allocate a state which is has only
>> >> > > 30 slots available. The name also suggests this will be called on an
>> >> > > online event. Why doesn't this have its own state like other users. The
>> >> > > name should also reflect offline event CPUHP_STACK_CACHE_DEAD or
>> >> > > something like that.
>> >> >
>> >> > I'll define CPUHP_VMSTACK_CACHE_DEAD before CPUHP_BP_PREPARE_DYN in
>> >> > cpuhotplug.h.
>> >> > Do you think the change is made in a separate patch or not ?
>> >>
>> >> I think it should be in a single patch. I am not sure what are the rules
>> >> to define a new state though. Let's CC Thomas.
>> >
>> > So the first question is where do you want that to be called? i.e. in which
>> > section:
>> >
>> > CPU up          CPU down
>> >
>> > PREPARE         DEAD            <- called on some other CPU
>> > ONLINE          DOWN            <- called on the hotplugged CPU
>> >
>>
>> It doesn't matter whether the callback is called on the hotplugged CPU
>> or other CPUs.
>>
>> > And then the next question is whether you have ordering constraints,
>> > i.e. it must be called before or after some other callback. Only in that
>> > case you want to have an explicit state. If not, just use a dynamically
>> > allocated one.
>>
>> The cache is for virtually mapped kernel stacks. so I think the
>> callback should be called after all tasks are migrated to other CPUs.
>> It must reside before CPUHP_AP_SCHED_STARTING or CPUHP_BRINGUP_CPU.
>
> Between AP_OFFLINE and AP_SCHED_STARTING does not work because those states
> path are called on the hotplugged CPU with interrupts disabled and after
> the CPU has been taken out from the scheduler.
>
> So the proper place is the dynamic states CPUHP_BP_PREPARE_DYN. You do not
> have a prepare callback, but then it's called on an still online CPU
> _AFTER_ the hotplugged CPU vanished from the system.
>

OK, I agree.

> Thanks,
>
>         tglx

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web