(memory hotplug) Make kmem_cache_node for SLUB on memory online to avoid panic(take 3)

PATCH In review Yasunori Goto 5 participants 11 messages Oct 18, 2007 – Oct 23, 2007

Subsystems (2)

Memory management Slab allocator

lore

Share & tools

lore

Yasunori Goto

author

2007-10-18 03:25 UTC #0 lore raw

PATCH 1/1 (memory hotplug) Make kmem_cache_node for SLUB on memory online to avoid panic(take 3)

This patch fixes panic due to access NULL pointer of kmem_cache_node at discard_slab() after memory online.

When memory online is called, kmem_cache_nodes are created for all SLUBs for new node whose memory are available.

slab_mem_going_online_callback() is called to make kmem_cache_node() in callback of memory online event. If it (or other callbacks) fails, then slab_mem_offline_callback() is called for rollback.

In memory offline, slab_mem_going_offline_callback() is called to shrink all slub cache, then slab_mem_offline_callback() is called later.

This patch is tested on my ia64 box.

Please apply.

Signed-off-by Yasunori Goto <[email protected]>

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

mm/slub.c +115 -0

Index: current/mm/slub.c
===================================================================
@@ -20,6 +20,7 @@
 #include <linux/mempolicy.h>
 #include <linux/ctype.h>
 #include <linux/kallsyms.h>
+#include <linux/memory.h>
 
 /*
  * Lock order:

@@ -2688,6 +2689,118 @@ int kmem_cache_shrink(struct kmem_cache 
 }
 EXPORT_SYMBOL(kmem_cache_shrink);
 
+#if defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)
+static int slab_mem_going_offline_callback(void *arg)
+{
+	struct kmem_cache *s;
+
+	down_read(&slub_lock);
+	list_for_each_entry(s, &slab_caches, list)
+		kmem_cache_shrink(s);
+	up_read(&slub_lock);
+
+	return 0;
+}
+
+static void slab_mem_offline_callback(void *arg)
+{
+	struct kmem_cache_node *n;
+	struct kmem_cache *s;
+	struct memory_notify *marg = arg;
+	int offline_node;
+
+	offline_node = marg->status_change_nid;
+
+	/*
+	 * If the node still has available memory. we need kmem_cache_node
+	 * for it yet.
+	 */
+	if (offline_node < 0)
+		return;
+
+	down_read(&slub_lock);
+	list_for_each_entry(s, &slab_caches, list) {
+		n = get_node(s, offline_node);
+		if (n) {
+			/*
+			 * if n->nr_slabs > 0, slabs still exist on the node
+			 * that is going down. We were unable to free them,
+			 * and offline_pages() function shoudn't call this
+			 * callback. So, we must fail.
+			 */
+			BUG_ON(atomic_read(&n->nr_slabs));
+
+			s->node[offline_node] = NULL;
+			kmem_cache_free(kmalloc_caches, n);
+		}
+	}
+	up_read(&slub_lock);
+}
+
+static int slab_mem_going_online_callback(void *arg)
+{
+	struct kmem_cache_node *n;
+	struct kmem_cache *s;
+	struct memory_notify *marg = arg;
+	int nid = marg->status_change_nid;
+
+	/*
+	 * If the node's memory is already available, then kmem_cache_node is
+	 * already created. Nothing to do.
+	 */
+	if (nid < 0)
+		return 0;
+
+	/*
+	 * We are bringing a node online. No memory is availabe yet. We must
+	 * allocate a kmem_cache_node structure in order to bring the node
+	 * online.
+	 */
+	down_read(&slub_lock);
+	list_for_each_entry(s, &slab_caches, list) {
+  		/*
+		 * XXX: kmem_cache_alloc_node will fallback to other nodes
+		 *      since memory is not yet available from the node that
+		 *      is brought up.
+  		 */
+		n = kmem_cache_alloc(kmalloc_caches, GFP_KERNEL);
+		if (!n)
+			return -ENOMEM;
+		init_kmem_cache_node(n);
+		s->node[nid] = n;
+  	}
+	up_read(&slub_lock);
+
+  	return 0;
+}
+
+static int slab_memory_callback(struct notifier_block *self,
+				unsigned long action, void *arg)
+{
+	int ret = 0;
+
+	switch (action) {
+	case MEM_GOING_ONLINE:
+		ret = slab_mem_going_online_callback(arg);
+		break;
+	case MEM_GOING_OFFLINE:
+		ret = slab_mem_going_offline_callback(arg);
+		break;
+	case MEM_OFFLINE:
+	case MEM_CANCEL_ONLINE:
+		slab_mem_offline_callback(arg);
+		break;
+	case MEM_ONLINE:
+	case MEM_CANCEL_OFFLINE:
+		break;
+	}
+
+	ret = notifier_from_errno(ret);
+	return ret;
+}
+
+#endif /* CONFIG_MEMORY_HOTPLUG */
+
 /********************************************************************
  *			Basic setup of slabs
  *******************************************************************/

@@ -2709,6 +2822,8 @@ void __init kmem_cache_init(void)
 		sizeof(struct kmem_cache_node), GFP_KERNEL);
 	kmalloc_caches[0].refcount = -1;
 	caches++;
+
+	hotplug_memory_notifier(slab_memory_callback, 1);
 #endif
 
 	/* Able to allocate the per node structures */

Signature · 8 lines

Yasunori Goto 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

maintainer

2007-10-18 03:46 UTC #1 lore raw

Quoted 11 lines · On Thu, 18 Oct 2007 12:25:37 +0900 Yasunori Goto <[email protected]> wrote:

This patch fixes panic due to access NULL pointer of kmem_cache_node at discard_slab() after memory online.
When memory online is called, kmem_cache_nodes are created for all SLUBs for new node whose memory are available.
slab_mem_going_online_callback() is called to make kmem_cache_node() in callback of memory online event. If it (or other callbacks) fails, then slab_mem_offline_callback() is called for rollback.

Quoted 8 lines

In memory offline, slab_mem_going_offline_callback() is called to shrink all slub cache, then slab_mem_offline_callback() is called later.
This patch is tested on my ia64 box.
...
+#if defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)

hm. There should be no linkage between memory hotpluggability and NUMA, surely?

Quoted 29 lines

mm/slub.c +29 -0

+static int slab_mem_going_offline_callback(void *arg)
+{
+	struct kmem_cache *s;
+
+	down_read(&slub_lock);
+	list_for_each_entry(s, &slab_caches, list)
+		kmem_cache_shrink(s);
+	up_read(&slub_lock);
+
+	return 0;
+}
+
+static void slab_mem_offline_callback(void *arg)
+{
+	struct kmem_cache_node *n;
+	struct kmem_cache *s;
+	struct memory_notify *marg = arg;
+	int offline_node;
+
+	offline_node = marg->status_change_nid;
+
+	/*
+	 * If the node still has available memory. we need kmem_cache_node
+	 * for it yet.
+	 */
+	if (offline_node < 0)
+		return;
+
+	down_read(&slub_lock);

mm/slub.c int kmem_cache_shrink(struct kmem_cache · line 2731

Quoted 10 lines

mm/slub.c +10 -0

+	list_for_each_entry(s, &slab_caches, list) {
+		n = get_node(s, offline_node);
+		if (n) {
+			/*
+			 * if n->nr_slabs > 0, slabs still exist on the node
+			 * that is going down. We were unable to free them,
+			 * and offline_pages() function shoudn't call this
+			 * callback. So, we must fail.
+			 */
+			BUG_ON(atomic_read(&n->nr_slabs));

Expereince tells us that WARN_ON is preferred for newly added code ;)

Quoted 26 lines

mm/slub.c +26 -0

+			s->node[offline_node] = NULL;
+			kmem_cache_free(kmalloc_caches, n);
+		}
+	}
+	up_read(&slub_lock);
+}
+
+static int slab_mem_going_online_callback(void *arg)
+{
+	struct kmem_cache_node *n;
+	struct kmem_cache *s;
+	struct memory_notify *marg = arg;
+	int nid = marg->status_change_nid;
+
+	/*
+	 * If the node's memory is already available, then kmem_cache_node is
+	 * already created. Nothing to do.
+	 */
+	if (nid < 0)
+		return 0;
+
+	/*
+	 * We are bringing a node online. No memory is availabe yet. We must
+	 * allocate a kmem_cache_node structure in order to bring the node
+	 * online.
+	 */

mm/slub.c int kmem_cache_shrink(struct kmem_cache · line 2768

Quoted 10 lines

mm/slub.c +10 -0

+	down_read(&slub_lock);
+	list_for_each_entry(s, &slab_caches, list) {
+  		/*
+		 * XXX: kmem_cache_alloc_node will fallback to other nodes
+		 *      since memory is not yet available from the node that
+		 *      is brought up.
+  		 */
+		n = kmem_cache_alloc(kmalloc_caches, GFP_KERNEL);
+		if (!n)
+			return -ENOMEM;

err, we forgot slub_lock. I'll fix that.

mm/slub.c int kmem_cache_shrink(struct kmem_cache · line 2775

Quoted 7 lines

mm/slub.c +7 -0

+		init_kmem_cache_node(n);
+		s->node[nid] = n;
+  	}
+	up_read(&slub_lock);
+
+  	return 0;
+}

So that's slub. Does slab already have this functionality or are you not bothering to maintain slab in this area?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2007-10-18 06:25 UTC #2 lore raw

Quoted 4 lines · Andrew Morton

Quoted 1 line
+#if defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)
hm. There should be no linkage between memory hotpluggability and NUMA, surely?

NUMA support in the slab allocators requires allocation of per node structures. The per node structures are folded into the global structure for non NUMA.

Quoted 9 lines

mm/slub.c int kmem_cache_shrink(struct kmem_cache · line 2731
Quoted 7 lines
mm/slub.c +7 -0
+			/*
+			 * if n->nr_slabs > 0, slabs still exist on the node
+			 * that is going down. We were unable to free them,
+			 * and offline_pages() function shoudn't call this
+			 * callback. So, we must fail.
+			 */
+			BUG_ON(atomic_read(&n->nr_slabs));
Expereince tells us that WARN_ON is preferred for newly added code ;)

It would be bad to just zap a per node array while there is still data in there. This will cause later failures when an attempt is made to free the objects that now have no per node structure anymore.

Quoted 10 lines

mm/slub.c int kmem_cache_shrink(struct kmem_cache · line 2768

Quoted 8 lines

mm/slub.c +8 -0

+  		/*
+		 * XXX: kmem_cache_alloc_node will fallback to other nodes
+		 *      since memory is not yet available from the node that
+		 *      is brought up.
+  		 */
+		n = kmem_cache_alloc(kmalloc_caches, GFP_KERNEL);
+		if (!n)
+			return -ENOMEM;

err, we forgot slub_lock. I'll fix that.

Right.

Quoted 2 lines

So that's slub. Does slab already have this functionality or are you not bothering to maintain slab in this area?

Slab brings up a per node structure when the corresponding cpu is brought up. That was sufficient as long as we did not have any memoryless nodes. Now we may have to fix some things over there as well.

Andrew Morton

maintainer

2007-10-18 07:00 UTC #3 lore raw

Quoted 6 lines · On Wed, 17 Oct 2007 23:25:58 -0700 (PDT) Christoph Lameter <[email protected]>…

Quoted 2 lines
So that's slub. Does slab already have this functionality or are you not bothering to maintain slab in this area?
Slab brings up a per node structure when the corresponding cpu is brought up. That was sufficient as long as we did not have any memoryless nodes. Now we may have to fix some things over there as well.

Is there amy point? Our time would be better spent in making slab.c go away. How close are we to being able to do that anwyay?

Yasunori Goto

author

2007-10-18 08:33 UTC #4 lore raw

Quoted 7 lines · On Wed, 17 Oct 2007 23:25:58 -0700 (PDT) Christoph Lameter <[email protected]>…

Quoted 6 lines
Quoted 3 lines
So that's slub. Does slab already have this functionality or are you not bothering to maintain slab in this area?
Slab brings up a per node structure when the corresponding cpu is brought up. That was sufficient as long as we did not have any memoryless nodes.

Right. At least, I don't have any experience of panic with SLAB so far. (If panic occurred, I already made a patch.).

Quoted 1 line

Quoted 1 line
Now we may have to fix some things over there as well.

Though the fix may be better for it, my priority is very low for it now.

Signature · 8 lines

Yasunori Goto 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2007-10-18 09:13 UTC #5 lore raw

In reply to Andrew Morton

Quoted 6 lines · Andrew Morton

Quoted 3 lines
Slab brings up a per node structure when the corresponding cpu is brought up. That was sufficient as long as we did not have any memoryless nodes. Now we may have to fix some things over there as well.
Is there amy point? Our time would be better spent in making slab.c go away. How close are we to being able to do that anwyay?

Well the problem right now is the regression in slab_free() on SMP. AFAICT UP and NUMA is fine and also most loads under SMP. Concurrent allocation / frees on multiple processors are several times faster (I see up to 10 fold improvements on an 8p).

However, long sequences of free operations from a single processor under SMP require too many atomic operations compared with SLAB. If I only do frees on a single processor on SMP then I can produce a 30% regression for slabs between 128 and 1024 byte in size. I have a patchset in the works that reduces the atomic operations for those.

SLAB currently has an advantage since it uses coarser grained locking. SLAB can take a global lock and then perform queue operations on multiple objects. SLUB has fine grained locking which increases concurrency but also the overhead of atomic operations.

The regression does not surface under UP since we do not need to do locking. And it does not surface under NUMA since the alien cache stuff in SLAB is reducing slab_free performance compared to SMP.

Yasunori Goto

author

2007-10-18 09:20 UTC #6 lore raw

In reply to Andrew Morton

Quoted 16 lines · On Thu, 18 Oct 2007 12:25:37 +0900 Yasunori Goto <[email protected]> wrote:

Quoted 15 lines
This patch fixes panic due to access NULL pointer of kmem_cache_node at discard_slab() after memory online.
When memory online is called, kmem_cache_nodes are created for all SLUBs for new node whose memory are available.
slab_mem_going_online_callback() is called to make kmem_cache_node() in callback of memory online event. If it (or other callbacks) fails, then slab_mem_offline_callback() is called for rollback.
In memory offline, slab_mem_going_offline_callback() is called to shrink all slub cache, then slab_mem_offline_callback() is called later.

Quoted 8 lines

Quoted 5 lines
This patch is tested on my ia64 box.
...
+#if defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)
hm. There should be no linkage between memory hotpluggability and NUMA, surely?

Sure. IBM's powerpc boxes have to support memory hotplug even if it is non-numa machine. They have the Dynamic Logical Partitioning feature.

Quoted 5 lines

mm/slub.c +5 -0

+	down_read(&slub_lock);
+	list_for_each_entry(s, &slab_caches, list) {
+		n = get_node(s, offline_node);
+		if (n) {
+			/*

Quoted 8 lines

mm/slub.c int kmem_cache_shrink(struct kmem_cache · line 2731
Quoted 6 lines
mm/slub.c +6 -0
+			 * if n->nr_slabs > 0, slabs still exist on the node
+			 * that is going down. We were unable to free them,
+			 * and offline_pages() function shoudn't call this
+			 * callback. So, we must fail.
+			 */
+			BUG_ON(atomic_read(&n->nr_slabs));
Expereince tells us that WARN_ON is preferred for newly added code ;)

Oh... Ok!

Quoted 30 lines

mm/slub.c +30 -0

+			s->node[offline_node] = NULL;
+			kmem_cache_free(kmalloc_caches, n);
+		}
+	}
+	up_read(&slub_lock);
+}
+
+static int slab_mem_going_online_callback(void *arg)
+{
+	struct kmem_cache_node *n;
+	struct kmem_cache *s;
+	struct memory_notify *marg = arg;
+	int nid = marg->status_change_nid;
+
+	/*
+	 * If the node's memory is already available, then kmem_cache_node is
+	 * already created. Nothing to do.
+	 */
+	if (nid < 0)
+		return 0;
+
+	/*
+	 * We are bringing a node online. No memory is availabe yet. We must
+	 * allocate a kmem_cache_node structure in order to bring the node
+	 * online.
+	 */
+	down_read(&slub_lock);
+	list_for_each_entry(s, &slab_caches, list) {
+  		/*
+		 * XXX: kmem_cache_alloc_node will fallback to other nodes

Quoted 8 lines

mm/slub.c int kmem_cache_shrink(struct kmem_cache · line 2768
Quoted 6 lines
mm/slub.c +6 -0
+		 *      since memory is not yet available from the node that
+		 *      is brought up.
+  		 */
+		n = kmem_cache_alloc(kmalloc_caches, GFP_KERNEL);
+		if (!n)
+			return -ENOMEM;
err, we forgot slub_lock. I'll fix that.

Oops. Indeed. Thanks for your check.

Bye.

Signature · 8 lines

Yasunori Goto 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Olof Johansson

2007-10-23 04:15 UTC #7 lore raw

PATCH 1/1 Fix warning in mm/slub.c

In reply to Yasunori Goto

Hi,

"Make kmem_cache_node for SLUB on memory online to avoid panic" introduced the following:

mm/slub.c:2737: warning: passing argument 1 of 'atomic_read' from incompatible pointer type

Signed-off-by Olof Johansson <[email protected]>

mm/slub.c +1 -1

index aac1dd3..bcdb2c8 100644
@@ -2734,7 +2734,7 @@ static void slab_mem_offline_callback(void *arg)
 			 * and offline_pages() function shoudn't call this
 			 * callback. So, we must fail.
 			 */
-			BUG_ON(atomic_read(&n->nr_slabs));
+			BUG_ON(atomic_long_read(&n->nr_slabs));
 
 			s->node[offline_node] = NULL;
 			kmem_cache_free(kmalloc_caches, n);

Yasunori Goto

author Acked-by on v1 1/1

2007-10-23 05:35 UTC #8 lore raw

Re: [PATCH] Fix warning in mm/slub.c

Quoted 15 lines

"Make kmem_cache_node for SLUB on memory online to avoid panic" introduced the following:
mm/slub.c:2737: warning: passing argument 1 of 'atomic_read' from incompatible pointer type
Signed-off-by Olof Johansson <[email protected]>
mm/slub.c +0 -0
index aac1dd3..bcdb2c8 100644

mm/slub.c static void slab_mem_offline_callback(void *arg) · line 2740

Quoted 8 lines

mm/slub.c +1 -1

@@ -2734,7 +2734,7 @@ static void slab_mem_offline_callback(void *arg)
 			 * and offline_pages() function shoudn't call this
 			 * callback. So, we must fail.
 			 */
-			BUG_ON(atomic_read(&n->nr_slabs));
+			BUG_ON(atomic_long_read(&n->nr_slabs));
 
 			s->node[offline_node] = NULL;
 			kmem_cache_free(kmalloc_caches, n);

Oops, yes. Thanks.

Acked-by Yasunori Goto <[email protected]>

Signature · 8 lines

Yasunori Goto 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Pekka Enberg

Reviewed-by

2007-10-23 07:52 UTC #9 lore raw

Re: [PATCH] Fix warning in mm/slub.c

Hi,

Quoted 21 lines · Yasunori Goto

Quoted 21 lines

"Make kmem_cache_node for SLUB on memory online to avoid panic" introduced the following:

mm/slub.c:2737: warning: passing argument 1 of 'atomic_read' from incompatible pointer type

Signed-off-by Olof Johansson <[email protected]>

mm/slub.c +1 -1

index aac1dd3..bcdb2c8 100644
@@ -2734,7 +2734,7 @@ static void slab_mem_offline_callback(void *arg)
                       * and offline_pages() function shoudn't call this
                       * callback. So, we must fail.
                       */
-                     BUG_ON(atomic_read(&n->nr_slabs));
+                     BUG_ON(atomic_long_read(&n->nr_slabs));

Quoted 7 lines

mm/slub.c static void slab_mem_offline_callback(void *arg) · line 2740
Quoted 2 lines
mm/slub.c +0 -0
                      s->node[offline_node] = NULL;
                      kmem_cache_free(kmalloc_caches, n);
Oops, yes. Thanks.
Acked-by Yasunori Goto <[email protected]>

Reviewed-by Pekka Enberg <[email protected]>

Christoph Lameter

Acked-by on v1 1/1

2007-10-23 16:21 UTC #10 lore raw

Re: [PATCH] Fix warning in mm/slub.c

In reply to Olof Johansson