[PATCH -mm] slub: fix cpu hotplug offline/online path

[PATCH -mm] slub: fix cpu hotplug offline/online path

[Akinobu Mita]

This patch fixes the problem introduced by:
http://kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc8/2.6.23-rc8-mm2/broken-out/slub-place-kmem_cache_cpu-structures-in-a-numa-aware-way.patch

I got slub BUG report when I tried to do cpu hotplug/unplug
$ while true; do
	echo 0 > /sys/devices/system/cpu/cpu1/online
	echo 1 > /sys/devices/system/cpu/cpu1/online
done

This is because init_alloc_cpu_cpu() is called every time when the CPU is
going to be onlined but init_alloc_cpu_cpu() is not intented to be called
twice or more for same CPU. Then it breaks kmem_cache_cpu_free list for
the CPU.

This patch removes init_alloc_cpu_cpu() from cpu hotplug notifier. But
call it for each possible CPUs not only online CPUs at initialization time.

Cc: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

---
 mm/slub.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Index: 2.6-mm/mm/slub.c
===================================================================
--- 2.6-mm.orig/mm/slub.c
+++ 2.6-mm/mm/slub.c
@@ -2029,7 +2029,7 @@ static int alloc_kmem_cache_cpus(struct 
 /*
  * Initialize the per cpu array.
  */
-static void init_alloc_cpu_cpu(int cpu)
+static void __init init_alloc_cpu_cpu(int cpu)
 {
 	int i;
 
@@ -2041,7 +2041,7 @@ static void __init init_alloc_cpu(void)
 {
 	int cpu;
 
-	for_each_online_cpu(cpu)
+	for_each_possible_cpu(cpu)
 		init_alloc_cpu_cpu(cpu);
   }
 
@@ -2973,7 +2973,6 @@ static int __cpuinit slab_cpuup_callback
 	switch (action) {
 	case CPU_UP_PREPARE:
 	case CPU_UP_PREPARE_FROZEN:
-		init_alloc_cpu_cpu(cpu);
 		down_read(&slub_lock);
 		list_for_each_entry(s, &slab_caches, list)
 			s->cpu_slab[cpu] = alloc_kmem_cache_cpu(s, cpu,

Re: [PATCH -mm] slub: fix cpu hotplug offline/online path

[Pekka Enberg]

Hi Akinobu,

On 10/9/07, Akinobu Mita <akinobu.mita@gmail.com> wrote:
> This is because init_alloc_cpu_cpu() is called every time when the CPU is
> going to be onlined but init_alloc_cpu_cpu() is not intented to be called
> twice or more for same CPU. Then it breaks kmem_cache_cpu_free list for
> the CPU.
>
> This patch removes init_alloc_cpu_cpu() from cpu hotplug notifier. But
> call it for each possible CPUs not only online CPUs at initialization time.

Looks good to me!

Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>

Re: [PATCH -mm] slub: fix cpu hotplug offline/online path

[Christoph Lameter]

On Wed, 10 Oct 2007, Akinobu Mita wrote:

> This patch removes init_alloc_cpu_cpu() from cpu hotplug notifier. But
> call it for each possible CPUs not only online CPUs at initialization time.

Could you check if a per cpu structure has already been allocated and then 
simply skip the call to init? Otherwise we end up with lots of per cpu 
structures for cpus that will never show up. 

if (!get_cpu_slab(s, cpu))
	init_alloc_cpu_cpu( ...)

Re: [PATCH -mm] slub: fix cpu hotplug offline/online path

[Pekka Enberg]

Hi Christoph,

On 10/9/07, Christoph Lameter <clameter@sgi.com> wrote:
> Could you check if a per cpu structure has already been allocated and then
> simply skip the call to init? Otherwise we end up with lots of per cpu
> structures for cpus that will never show up.

Does it matter? How?

Re: [PATCH -mm] slub: fix cpu hotplug offline/online path

[Christoph Lameter]

On Tue, 9 Oct 2007, Pekka Enberg wrote:

> Hi Christoph,
> 
> On 10/9/07, Christoph Lameter <clameter@sgi.com> wrote:
> > Could you check if a per cpu structure has already been allocated and then
> > simply skip the call to init? Otherwise we end up with lots of per cpu
> > structures for cpus that will never show up.
> 
> Does it matter? How?

Our system come by default with the possibility of 1k cpus. However, a 
small system may only have 8 cpus.

Re: [PATCH -mm] slub: fix cpu hotplug offline/online path

[Pekka Enberg]

Hi Christoph,

On 10/9/07, Christoph Lameter <clameter@sgi.com> wrote:
> Our system come by default with the possibility of 1k cpus. However, a
> small system may only have 8 cpus.

Maybe I am reading the code wrong, but I don't why that matters as
it's all statically allocated anyway (the per-cpu kmem_cache_cpu
array). In any case, I certainly don't object to your proposal, just
can't figure out why it matters.

                                   Pekka

Re: [PATCH -mm] slub: fix cpu hotplug offline/online path

[Christoph Lameter]

On Tue, 9 Oct 2007, Pekka Enberg wrote:

> Hi Christoph,
> 
> On 10/9/07, Christoph Lameter <clameter@sgi.com> wrote:
> > Our system come by default with the possibility of 1k cpus. However, a
> > small system may only have 8 cpus.
> 
> Maybe I am reading the code wrong, but I don't why that matters as
> it's all statically allocated anyway (the per-cpu kmem_cache_cpu
> array). In any case, I certainly don't object to your proposal, just
> can't figure out why it matters.

The kmem_cache_cpu structures are allocated using DEFINE_PER_CPU. 

The per cpu areas should be dynamically allocated as necessary. There is 
work in progress by Mike Travis to do that. You are right in that (on 
x86_64 at least) we define a static array for all processors.

Re: [PATCH -mm] slub: fix cpu hotplug offline/online path

[Akinobu Mita]

On Tue, Oct 09, 2007 at 11:46:14AM -0700, Christoph Lameter wrote:
> On Wed, 10 Oct 2007, Akinobu Mita wrote:
> 
> > This patch removes init_alloc_cpu_cpu() from cpu hotplug notifier. But
> > call it for each possible CPUs not only online CPUs at initialization time.
> 
> Could you check if a per cpu structure has already been allocated and then 
> simply skip the call to init? Otherwise we end up with lots of per cpu 
> structures for cpus that will never show up. 
> 
> if (!get_cpu_slab(s, cpu))
> 	init_alloc_cpu_cpu( ...)
> 

I couldn't use get_cpu_slab() for that check. But I reviced the patch to do
what you said.

Subject: slub: fix cpu hotplug offline/online path

This patch fixes the problem introduced by:
http://kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc8/2.6.23-rc8-mm2/broken-out/slub-place-kmem_cache_cpu-structures-in-a-numa-aware-way.patch

I got slub BUG report when I tried to do cpu hotplug/unplug
$ while true; do
	echo 0 > /sys/devices/system/cpu/cpu1/online
	echo 1 > /sys/devices/system/cpu/cpu1/online
done

This is because init_alloc_cpu_cpu() is called every time when the CPU is
going to be onlined but init_alloc_cpu_cpu() is not intented to be called
twice or more for same CPU. Then it breaks kmem_cache_cpu_free list for
the CPU.

This patch checks if a per cpu structure has already been allocated and then
simply skip the call to init_alloc_cpu_cpu().

Cc: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

---
 mm/slub.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: 2.6-mm/mm/slub.c
===================================================================
--- 2.6-mm.orig/mm/slub.c
+++ 2.6-mm/mm/slub.c
@@ -2033,6 +2033,11 @@ static void init_alloc_cpu_cpu(int cpu)
 {
 	int i;
 
+	if (per_cpu(kmem_cache_cpu_free, cpu)) {
+		/* Already initialized once */
+		return;
+	}
+
 	for (i = NR_KMEM_CACHE_CPU - 1; i >= 0; i--)
 		free_kmem_cache_cpu(&per_cpu(kmem_cache_cpu, cpu)[i], cpu);
 }

Re: [PATCH -mm] slub: fix cpu hotplug offline/online path

[Christoph Lameter]

On Wed, 10 Oct 2007, Akinobu Mita wrote:

> I couldn't use get_cpu_slab() for that check. But I reviced the patch to do
> what you said.

Why would get_cpu_slab not work?

> +	if (per_cpu(kmem_cache_cpu_free, cpu)) {
> +		/* Already initialized once */
> +		return;
> +	}
> +

kmem_cache_cpu_free is not only NULL if the cpu is not up yet but it is 
also NULL if the per cpu pool of kmem_cache_cpu structures was 
exhausted.

Re: [PATCH -mm] slub: fix cpu hotplug offline/online path

[Akinobu Mita]

On Wed, Oct 10, 2007 at 10:39:32AM -0700, Christoph Lameter wrote:
> On Wed, 10 Oct 2007, Akinobu Mita wrote:
> 
> > I couldn't use get_cpu_slab() for that check. But I reviced the patch to do
> > what you said.
> 
> Why would get_cpu_slab not work?


        case CPU_DEAD:
        case CPU_DEAD_FROZEN:
                down_read(&slub_lock);
                list_for_each_entry(s, &slab_caches, list) {
                        struct kmem_cache_cpu *c = get_cpu_slab(s, cpu);

                        local_irq_save(flags);
                        __flush_cpu_slab(s, cpu);
                        local_irq_restore(flags);
                        free_kmem_cache_cpu(c, cpu);
                        s->cpu_slab[cpu] = NULL; <----------------------
                }
                up_read(&slub_lock);
                break;

When CPU is offlined, cpu-hotplug notifier sets s->cpu_slab[cpu] = NULL.
This means get_cpu_slab() always return NULL when CPU is being onlined.
So I can't use get_cpu_slab to check whether kmem_cache_cpu_free
initalization for the CPU has already been done or not.

> 
> > +	if (per_cpu(kmem_cache_cpu_free, cpu)) {
> > +		/* Already initialized once */
> > +		return;
> > +	}
> > +
> 
> kmem_cache_cpu_free is not only NULL if the cpu is not up yet but it is 
> also NULL if the per cpu pool of kmem_cache_cpu structures was 
> exhausted.

cpu-hotplug notifier by CPU_DEAD event frees kmem_cache_cpu structures
for the CPU being offlined. So we have 100 kmem_cache cpu structures
when the CPU will be onlined again.

But I agree that it is not so trivial. It is why I added the comment
/* Already initialized once */ in previous patch.

This is another approach for the fix.
Use cpumask to check whether kmem_cache_cpu_free initalization for
the CPU has already been done or not.

---
 mm/slub.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: 2.6-mm/mm/slub.c
===================================================================
--- 2.6-mm.orig/mm/slub.c
+++ 2.6-mm/mm/slub.c
@@ -1959,6 +1959,7 @@ static DEFINE_PER_CPU(struct kmem_cache_
 				kmem_cache_cpu)[NR_KMEM_CACHE_CPU];
 
 static DEFINE_PER_CPU(struct kmem_cache_cpu *, kmem_cache_cpu_free);
+static cpumask_t kmem_cach_cpu_free_init_once = CPU_MASK_NONE;
 
 static struct kmem_cache_cpu *alloc_kmem_cache_cpu(struct kmem_cache *s,
 							int cpu, gfp_t flags)
@@ -2033,8 +2034,13 @@ static void init_alloc_cpu_cpu(int cpu)
 {
 	int i;
 
+	if (cpu_isset(cpu, kmem_cach_cpu_free_init_once))
+		return;
+
 	for (i = NR_KMEM_CACHE_CPU - 1; i >= 0; i--)
 		free_kmem_cache_cpu(&per_cpu(kmem_cache_cpu, cpu)[i], cpu);
+
+	cpu_set(cpu, kmem_cach_cpu_free_init_once);
 }
 
 static void __init init_alloc_cpu(void)

Re: [PATCH -mm] slub: fix cpu hotplug offline/online path

[Christoph Lameter]

On Thu, 11 Oct 2007, Akinobu Mita wrote:

> > Why would get_cpu_slab not work?
> 
> 
>         case CPU_DEAD:
>         case CPU_DEAD_FROZEN:
>                 down_read(&slub_lock);
>                 list_for_each_entry(s, &slab_caches, list) {
>                         struct kmem_cache_cpu *c = get_cpu_slab(s, cpu);
> 
>                         local_irq_save(flags);
>                         __flush_cpu_slab(s, cpu);
>                         local_irq_restore(flags);
>                         free_kmem_cache_cpu(c, cpu);
>                         s->cpu_slab[cpu] = NULL; <----------------------
>                 }
>                 up_read(&slub_lock);
>                 break;
> 
> When CPU is offlined, cpu-hotplug notifier sets s->cpu_slab[cpu] = NULL.
> This means get_cpu_slab() always return NULL when CPU is being onlined.
> So I can't use get_cpu_slab to check whether kmem_cache_cpu_free
> initalization for the CPU has already been done or not.

If you have set it to NULL then the earlier kmem_cache_cpu structures has 
been freed. Why is it a problem to allocate another one when the cpu comes 
up again?

Re: [PATCH -mm] slub: fix cpu hotplug offline/online path

[Akinobu Mita]

2007/10/12, Christoph Lameter <clameter@sgi.com>:
> On Thu, 11 Oct 2007, Akinobu Mita wrote:
>
> > > Why would get_cpu_slab not work?
> >
> >
> >         case CPU_DEAD:
> >         case CPU_DEAD_FROZEN:
> >                 down_read(&slub_lock);
> >                 list_for_each_entry(s, &slab_caches, list) {
> >                         struct kmem_cache_cpu *c = get_cpu_slab(s, cpu);
> >
> >                         local_irq_save(flags);
> >                         __flush_cpu_slab(s, cpu);
> >                         local_irq_restore(flags);
> >                         free_kmem_cache_cpu(c, cpu);
> >                         s->cpu_slab[cpu] = NULL; <----------------------
> >                 }
> >                 up_read(&slub_lock);
> >                 break;
> >
> > When CPU is offlined, cpu-hotplug notifier sets s->cpu_slab[cpu] = NULL.
> > This means get_cpu_slab() always return NULL when CPU is being onlined.
> > So I can't use get_cpu_slab to check whether kmem_cache_cpu_free
> > initalization for the CPU has already been done or not.
>
> If you have set it to NULL then the earlier kmem_cache_cpu structures has
> been freed. Why is it a problem to allocate another one when the cpu comes
> up again?
>

kmem_cache_cpu_free per-cpu singly list will be broken by calling
init_alloc_cpu_cpu() twice. It happens when online/offlining CPU.
All three patches I send attempt to make init_alloc_cpu_cpu()
called only once for each cpu.

Re: [PATCH -mm] slub: fix cpu hotplug offline/online path

[Christoph Lameter]

On Fri, 12 Oct 2007, Akinobu Mita wrote:

> > > This means get_cpu_slab() always return NULL when CPU is being onlined.
> > > So I can't use get_cpu_slab to check whether kmem_cache_cpu_free
> > > initalization for the CPU has already been done or not.
> >
> > If you have set it to NULL then the earlier kmem_cache_cpu structures has
> > been freed. Why is it a problem to allocate another one when the cpu comes
> > up again?
> >
> 
> kmem_cache_cpu_free per-cpu singly list will be broken by calling
> init_alloc_cpu_cpu() twice. It happens when online/offlining CPU.
> All three patches I send attempt to make init_alloc_cpu_cpu()
> called only once for each cpu.

Ahhh.. Its the initialization of the kmem_cache_cpu pool that is the issue 
not the allocation of the kmem_cache_cpu() structure.

Re: [PATCH -mm] slub: fix cpu hotplug offline/online path

[Christoph Lameter]

On Thu, 11 Oct 2007, Akinobu Mita wrote:

> This is another approach for the fix.

Looks like the cleanest solution.

Acked-by: Christoph Lameter <clameter@sgi.com>


> Use cpumask to check whether kmem_cache_cpu_free initalization for
> the CPU has already been done or not.
> 
> ---
>  mm/slub.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> Index: 2.6-mm/mm/slub.c
> ===================================================================
> --- 2.6-mm.orig/mm/slub.c
> +++ 2.6-mm/mm/slub.c
> @@ -1959,6 +1959,7 @@ static DEFINE_PER_CPU(struct kmem_cache_
>  				kmem_cache_cpu)[NR_KMEM_CACHE_CPU];
>  
>  static DEFINE_PER_CPU(struct kmem_cache_cpu *, kmem_cache_cpu_free);
> +static cpumask_t kmem_cach_cpu_free_init_once = CPU_MASK_NONE;
>  
>  static struct kmem_cache_cpu *alloc_kmem_cache_cpu(struct kmem_cache *s,
>  							int cpu, gfp_t flags)
> @@ -2033,8 +2034,13 @@ static void init_alloc_cpu_cpu(int cpu)
>  {
>  	int i;
>  
> +	if (cpu_isset(cpu, kmem_cach_cpu_free_init_once))
> +		return;
> +
>  	for (i = NR_KMEM_CACHE_CPU - 1; i >= 0; i--)
>  		free_kmem_cache_cpu(&per_cpu(kmem_cache_cpu, cpu)[i], cpu);
> +
> +	cpu_set(cpu, kmem_cach_cpu_free_init_once);
>  }
>  
>  static void __init init_alloc_cpu(void)
>