Re: [patch 3/3] NUMA slab locking fixes -- fix cpu down and up locking

Andrew Morton
2006-02-04T10:04:22Z

Ravikiran G Thirumalai <kiran@scalex86.org> wrote:
>
> This fixes locking and bugs in cpu_down and cpu_up paths of the NUMA slab
> allocator. Sonny Rao <sonny@burdell.org> reported problems sometime back
> on POWER5 boxes, when the last cpu on the nodes were being offlined. We could
> not reproduce the same on x86_64 because the cpumask (node_to_cpumask) was not
> being updated on cpu down. Since that issue is now fixed, we can reproduce
> Sonny's problems on x86_64 NUMA, and here is the fix.
>
> The problem earlier was on CPU_DOWN, if it was the last cpu on the node to go
> down, the array_caches (shared, alien) and the kmem_list3 of the node were
> being freed (kfree) with the kmem_list3 lock held. If the l3 or the
> array_caches were to come from the same cache being cleared, we hit on badness.
>
> This patch cleans up the locking in cpu_up and cpu_down path.
> We cannot really free l3 on cpu down because, there is no node offlining yet
> and even though a cpu is not yet up, node local memory can be allocated
> for it. So l3s are usually allocated at keme_cache_create and destroyed at kmem_cache_destroy. Hence, we don't need cachep->spinlock protection to get
> to the cachep->nodelist[nodeid] either.
>
> Patch survived onlining and offlining on a 4 core 2 node Tyan box with a
> 4 dbench process running all the time.
>
> ...
>
> + if (!(nc = alloc_arraycache(node,
> + cachep->limit, cachep->batchcount)))
> goto bad;
> + if (!(shared = alloc_arraycache(node,
> + cachep->shared*cachep->batchcount, 0xbaadf00d)))
> + goto bad;

Please don't do things like that - it's quite hard to read and we avoid it.
Cleanup patch below.

--- devel/mm/slab.c~numa-slab-locking-fixes-fix-cpu-down-and-up-locking-tidy 2006-02-04 02:01:33.000000000 -0800
+++ devel-akpm/mm/slab.c 2006-02-04 02:01:33.000000000 -0800
@@ -936,9 +936,11 @@ static int __devinit cpuup_callback(stru
l3->next_reap = jiffies + REAPTIMEOUT_LIST3 +
((unsigned long)cachep) % REAPTIMEOUT_LIST3;

- /* The l3s don't come and go as cpus come and
- go. cache_chain_mutex is sufficient
- protection here */
+ /*
+ * The l3s don't come and go as CPUs come and
+ * go. cache_chain_mutex is sufficient
+ * protection here.
+ */
cachep->nodelists[node] = l3;
}

@@ -952,17 +954,22 @@ static int __devinit cpuup_callback(stru
/* Now we can go ahead with allocating the shared array's
& array cache's */
list_for_each_entry(cachep, &cache_chain, next) {
- struct array_cache *nc, *shared, **alien;
-
- if (!(nc = alloc_arraycache(node,
- cachep->limit, cachep->batchcount)))
+ struct array_cache *nc;
+ struct array_cache *shared;
+ struct array_cache **alien;
+
+ nc = alloc_arraycache(node, cachep->limit,
+ cachep->batchcount);
+ if (!nc)
goto bad;
- if (!(shared = alloc_arraycache(node,
- cachep->shared*cachep->batchcount, 0xbaadf00d)))
+ shared = alloc_arraycache(node,
+ cachep->shared * cachep->batchcount,
+ 0xbaadf00d);
+ if (!shared)
goto bad;
#ifdef CONFIG_NUMA
- if (!(alien = alloc_alien_cache(node,
- cachep->limit)))
+ alien = alloc_alien_cache(node, cachep->limit);
+ if (!alien)
goto bad;
#endif
cachep->array[cpu] = nc;
@@ -972,8 +979,10 @@ static int __devinit cpuup_callback(stru

spin_lock_irq(&l3->list_lock);
if (!l3->shared) {
- /* we are serialised from CPU_DEAD or
- CPU_UP_CANCELLED by the cpucontrol lock */
+ /*
+ * We are serialised from CPU_DEAD or
+ * CPU_UP_CANCELLED by the cpucontrol lock
+ */
l3->shared = shared;
shared = NULL;
}
@@ -993,19 +1002,22 @@ static int __devinit cpuup_callback(stru
break;
#ifdef CONFIG_HOTPLUG_CPU
case CPU_DEAD:
- /* Even if all the cpus of a node are down, we don't
- * free the kmem_list3 of any cache. This to avoid a race
- * between cpu_down, and a kmalloc allocation from another
- * cpu for memory from the node of the cpu going down.
- * The list3 structure is usually allocated from
- * kmem_cache_create and gets destroyed at kmem_cache_destroy
+ /*
+ * Even if all the cpus of a node are down, we don't free the
+ * kmem_list3 of any cache. This to avoid a race between
+ * cpu_down, and a kmalloc allocation from another cpu for
+ * memory from the node of the cpu going down. The list3
+ * structure is usually allocated from kmem_cache_create() and
+ * gets destroyed at kmem_cache_destroy().
*/
/* fall thru */
case CPU_UP_CANCELED:
mutex_lock(&cache_chain_mutex);

list_for_each_entry(cachep, &cache_chain, next) {
- struct array_cache *nc, *shared, **alien;
+ struct array_cache *nc;
+ struct array_cache *shared;
+ struct array_cache **alien;
cpumask_t mask;

mask = node_to_cpumask(node);
@@ -1029,7 +1041,8 @@ static int __devinit cpuup_callback(stru
goto free_array_cache;
}

- if ((shared = l3->shared)) {
+ shared = l3->shared;
+ if (shared) {
free_block(cachep, l3->shared->entry,
l3->shared->avail, node);
l3->shared = NULL;
@@ -1045,7 +1058,7 @@ static int __devinit cpuup_callback(stru
drain_alien_cache(cachep, alien);
free_alien_cache(alien);
}
- free_array_cache:
+free_array_cache:
kfree(nc);
}
/*
@@ -1054,7 +1067,6 @@ static int __devinit cpuup_callback(stru
* shrink each nodelist to its limit.
*/
list_for_each_entry(cachep, &cache_chain, next) {
-
l3 = cachep->nodelists[node];
if (!l3)
continue;
_