slub: fix cpu hotplug offline/online path

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.

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.

Hi Christoph,

Quoted 3 lines · Christoph Lameter

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.

Hi Christoph,

Quoted 2 lines · Christoph Lameter

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.

Quoted 2 lines · Akinobu Mita

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.

Quoted 2 lines · Akinobu Mita

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?

Review

Quoted 6 lines

(quoted diff) +5 -0

+	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.

Quoted 1 line

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.

Quoted 19 lines

Quoted 2 lines

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;

Quoted 4 lines

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?

Quoted 8 lines

Quoted 3 lines

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.

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
===================================================================

mm/slub.c +6 -0

@@ -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)