Update slab memory hotplug series

When fallback_alloc() runs the node of the CPU might not be initialized yet. Handle this case by allocating in another node.

v2: Try to allocate from all nodes (David Rientjes)

Signed-off-by Andi Kleen <[email protected]>

Quoted 5 lines · Andi Kleen

When fallback_alloc() runs the node of the CPU might not be initialized yet. Handle this case by allocating in another node.

v2: Try to allocate from all nodes (David Rientjes)

You don't need to specifically address the cpuset restriction in fallback_alloc() since kmem_getpages() will return NULL whenever a zone is tried from an unallowed node, I just thought it was a faster optimization considering you (i) would operate over a nodemask and not the entire zonelist, (ii) it would avoid the zone_to_nid() for all zones since you already did a zonelist iteration in this function, and (iii) it wouldn't needlessly call kmem_getpages() for unallowed nodes.

When fallback_alloc() runs the node of the CPU might not be initialized yet. Handle this case by allocating in another node.

v2: Try to allocate from all nodes (David Rientjes)

Signed-off-by Andi Kleen <[email protected]>

---
 mm/slab.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Index: linux-2.6.32-memhotadd/mm/slab.c
===================================================================

mm/slab.c +11 -1

@@ -3188,7 +3188,24 @@ retry:
 		if (local_flags & __GFP_WAIT)
 			local_irq_enable();
 		kmem_flagcheck(cache, flags);
-		obj = kmem_getpages(cache, local_flags, numa_node_id());
+
+		/*
+		 * Node not set up yet? Try one that the cache has been set up
+		 * for.
+		 */
+		nid = numa_node_id();
+		if (cache->nodelists[nid] == NULL) {
+			for_each_zone_zonelist(zone, z, zonelist, high_zoneidx) {
+				nid = zone_to_nid(zone);
+				if (cache->nodelists[nid]) {
+					obj = kmem_getpages(cache, local_flags, nid);

This is a better way to go anyway because it really is a proper "fallback" alloc. I think that possibly used to work (ie. kmem_getpages would be able to pass -1 for the node there) but got broken along the line.

Although it's not such a hot path to begin with, care to put a branch annotation there?

Quoted 4 lines

This is a better way to go anyway because it really is a proper "fallback" alloc. I think that possibly used to work (ie. kmem_getpages would be able to pass -1 for the node there) but got broken along the line.

Thanks for the review.

I should add there's still one open problem: in some cases the oom killer kicks in on hotadd. Still working on that one.

In general hotadd was mighty bitrotted :/

Quoted 2 lines

Although it's not such a hot path to begin with, care to put a branch annotation there?

pointer == NULL is already default unlikely in gcc

/* Pointers are usually not NULL. */
DEF_PREDICTOR (PRED_POINTER, "pointer", HITRATE (85), 0) DEF_PREDICTOR (PRED_TREE_POINTER, "pointer (on trees)", HITRATE (85), 0)

Index: linux-2.6.32-memhotadd/mm/slab.c
===================================================================

mm/slab.c +18 -0

@@ -115,6 +115,7 @@
 #include	<linux/reciprocal_div.h>
 #include	<linux/debugobjects.h>
 #include	<linux/kmemcheck.h>
+#include	<linux/memory.h>
 
 #include	<asm/cacheflush.h>
 #include	<asm/tlbflush.h>

@@ -1554,6 +1555,23 @@ void __init kmem_cache_init(void)
 	g_cpucache_up = EARLY;
 }
 
+static int slab_memory_callback(struct notifier_block *self,
+				unsigned long action, void *arg)
+{
+	struct memory_notify *mn = (struct memory_notify *)arg;
+
+	/*
+	 * When a node goes online allocate l3s early.	 This way
+	 * kmalloc_node() works for it.
+	 */
+	if (action == MEM_ONLINE && mn->status_change_nid >= 0) {
+		mutex_lock(&cache_chain_mutex);
+		slab_node_prepare(mn->status_change_nid);
+		mutex_unlock(&cache_chain_mutex);
+	}
+	return NOTIFY_OK;
+}
+
 void __init kmem_cache_init_late(void)
 {
 	struct kmem_cache *cachep;

Quoted 40 lines · Andi Kleen

Index: linux-2.6.32-memhotadd/mm/slab.c
===================================================================

mm/slab.c +18 -0

@@ -115,6 +115,7 @@
 #include	<linux/reciprocal_div.h>
 #include	<linux/debugobjects.h>
 #include	<linux/kmemcheck.h>
+#include	<linux/memory.h>
 
 #include	<asm/cacheflush.h>
 #include	<asm/tlbflush.h>

@@ -1554,6 +1555,23 @@ void __init kmem_cache_init(void)
 	g_cpucache_up = EARLY;
 }
 
+static int slab_memory_callback(struct notifier_block *self,
+				unsigned long action, void *arg)
+{
+	struct memory_notify *mn = (struct memory_notify *)arg;
+
+	/*
+	 * When a node goes online allocate l3s early.	 This way
+	 * kmalloc_node() works for it.
+	 */
+	if (action == MEM_ONLINE && mn->status_change_nid >= 0) {
+		mutex_lock(&cache_chain_mutex);
+		slab_node_prepare(mn->status_change_nid);
+		mutex_unlock(&cache_chain_mutex);
+	}
+	return NOTIFY_OK;
+}
+
 void __init kmem_cache_init_late(void)
 {
 	struct kmem_cache *cachep;

@@ -1577,6 +1595,8 @@ void __init kmem_cache_init_late(void)
 	 */
 	register_cpu_notifier(&cpucache_notifier);
 

mm/slab.c void __init kmem_cache_init_late(void) · line 1599

Quoted 2 lines

mm/slab.c +2 -0

+	hotplug_memory_notifier(slab_memory_callback, SLAB_CALLBACK_PRI);
+

Only needed for CONFIG_NUMA, but there's no side-effects for UMA kernels since status_change_nid will always be -1.

Acked-by David Rientjes <[email protected]>

Quoted 4 lines

Quoted 4 lines

mm/slab.c +0 -0

@@ -1577,6 +1595,8 @@ void __init kmem_cache_init_late(void)
 	 */
 	register_cpu_notifier(&cpucache_notifier);
 

Quoted 5 lines

mm/slab.c void __init kmem_cache_init_late(void) · line 1599

Quoted 2 lines

mm/slab.c +2 -0

+	hotplug_memory_notifier(slab_memory_callback, SLAB_CALLBACK_PRI);
+

Only needed for CONFIG_NUMA, but there's no side-effects for UMA kernels since status_change_nid will always be -1.

Compiler doesn't know that, though.

Quoted 4 lines · Nick Piggin

Quoted 4 lines

Quoted 4 lines

mm/slab.c +0 -0

@@ -1577,6 +1595,8 @@ void __init kmem_cache_init_late(void)
 	 */
 	register_cpu_notifier(&cpucache_notifier);
 

Quoted 8 lines

Quoted 5 lines

mm/slab.c void __init kmem_cache_init_late(void) · line 1599

Quoted 2 lines

mm/slab.c +2 -0

+	hotplug_memory_notifier(slab_memory_callback, SLAB_CALLBACK_PRI);
+

Only needed for CONFIG_NUMA, but there's no side-effects for UMA kernels since status_change_nid will always be -1.

Compiler doesn't know that, though.

Right, setting up a memory hotplug callback for UMA kernels here isn't necessary although slab_node_prepare() would have to be defined unconditionally. I made this suggestion in my review of the patchset's initial version but it was left unchanged, so I'd rather see it included than otherwise stall out. This could always be enclosed in #ifdef CONFIG_NUMA later just like the callback in slub does.

Quoted 9 lines · David Rientjes

Quoted 7 lines · Nick Piggin

Quoted 7 lines

mm/slab.c void __init kmem_cache_init_late(void) · line 1599

Quoted 6 lines

mm/slab.c +2 -0

@@ -1577,6 +1595,8 @@ void __init kmem_cache_init_late(void)
 	 */
 	register_cpu_notifier(&cpucache_notifier);
 
+	hotplug_memory_notifier(slab_memory_callback, SLAB_CALLBACK_PRI);
+

Only needed for CONFIG_NUMA, but there's no side-effects for UMA kernels since status_change_nid will always be -1.

Quoted 4 lines

Quoted 2 lines

Compiler doesn't know that, though.

Right, setting up a memory hotplug callback for UMA kernels here isn't necessary although slab_node_prepare() would have to be defined unconditionally. I made this suggestion in my review of the patchset's initial version but it was left unchanged, so I'd rather see it included than otherwise stall out. This could always be enclosed in #ifdef CONFIG_NUMA later just like the callback in slub does.

It's not such a big burden to annotate critical core code with such things. Otherwise someone else ends up eventually doing it.

cache_reap can run before the node is set up and then reference a NULL l3 list. Check for this explicitely and just continue. The node will be eventually set up.

Signed-off-by Andi Kleen <[email protected]>

cache_reap can run before the node is set up and then reference a NULL l3 list. Check for this explicitely and just continue. The node will be eventually set up.

Signed-off-by Andi Kleen <[email protected]>

---
 mm/slab.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6.32-memhotadd/mm/slab.c
===================================================================

mm/slab.c +3 -1

@@ -4093,6 +4093,9 @@ static void cache_reap(struct work_struc
 		 * we can do some work if the lock was obtained.
 		 */
 		l3 = searchp->nodelists[node];
+		/* Note node yet set up */
+		if (!l3)
+			break;
 
 		reap_alien(searchp, l3);
 
 
--

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>

Quoted 4 lines · Andi Kleen

cache_reap can run before the node is set up and then reference a NULL l3 list. Check for this explicitely and just continue. The node will be eventually set up.

How, may I ask? cpuup_prepare in the hotplug notifier should always run before start_cpu_timer.

Quoted 8 lines · Nick Piggin

Quoted 4 lines · Andi Kleen

cache_reap can run before the node is set up and then reference a NULL l3 list. Check for this explicitely and just continue. The node will be eventually set up.

How, may I ask? cpuup_prepare in the hotplug notifier should always run before start_cpu_timer.

I'm not fully sure, but I have the oops to prove it :)

Quoted 7 lines · Andi Kleen

Quoted 6 lines · Nick Piggin

Quoted 4 lines · Andi Kleen

cache_reap can run before the node is set up and then reference a NULL l3 list. Check for this explicitely and just continue. The node will be eventually set up.

Quoted 4 lines

Quoted 2 lines

How, may I ask? cpuup_prepare in the hotplug notifier should always run before start_cpu_timer.

I'm not fully sure, but I have the oops to prove it :)

Hmm, it would be nice to work out why it's happening. If it's completely reproducible then could I send you a debug patch to test?

Quoted 13 lines · Nick Piggin

Quoted 11 lines · Andi Kleen

Quoted 8 lines · Nick Piggin

Quoted 4 lines · Andi Kleen

cache_reap can run before the node is set up and then reference a NULL l3 list. Check for this explicitely and just continue. The node will be eventually set up.

How, may I ask? cpuup_prepare in the hotplug notifier should always run before start_cpu_timer.

I'm not fully sure, but I have the oops to prove it :)

Quoted 2 lines

Hmm, it would be nice to work out why it's happening. If it's completely reproducible then could I send you a debug patch to test?

Looking at it again I suspect it happened this way:

cpuup_prepare fails (e.g. kmalloc_node returns NULL). The later patches might have cured that. Nothing stops the timer from starting in this case anyways.

So given that the first patches might not be needed, but it's safer to have anyways.

Hmm, but it should, because if cpuup_prepare fails then the CPU_ONLINE notifiers should never be called I think.

I'm just worried there is still an underlying problem here.

Quoted 1 line · Nick Piggin

I'm just worried there is still an underlying problem here.

So am I. What caused the breakage that requires this patchset?

Quoted 4 lines · Nick Piggin

Quoted 2 lines

I'm just worried there is still an underlying problem here.

So am I. What caused the breakage that requires this patchset?

Memory hotadd with a new node being onlined.

Andi Kleen kirjoitti:

Quoted 7 lines · Christoph Lameter

Quoted 4 lines · Nick Piggin

Quoted 2 lines

I'm just worried there is still an underlying problem here.

So am I. What caused the breakage that requires this patchset?

Memory hotadd with a new node being onlined.

So can you post the oops, please? Right now I am looking at zapping the

series from slab.git due to NAKs from both Christoph and Nick.

Quoted 7 lines · Pekka Enberg

Andi Kleen kirjoitti:

Quoted 6 lines · Christoph Lameter

Quoted 4 lines · Nick Piggin

Quoted 2 lines

I'm just worried there is still an underlying problem here.

So am I. What caused the breakage that requires this patchset?

Memory hotadd with a new node being onlined.

Quoted 1 line

So can you post the oops, please? Right now I am looking at zapping the

I can't post the oops from a pre-release system.

Quoted 1 line

series from slab.git due to NAKs from both Christoph and Nick.

Huh? They just complained about the patch, not the whole series. I don't understand how that could prompt you to drop the whole series.

Quoted 10 lines · Andi Kleen

Quoted 8 lines · Pekka Enberg

Andi Kleen kirjoitti:

Quoted 6 lines · Christoph Lameter

Quoted 4 lines · Nick Piggin

Quoted 2 lines

I'm just worried there is still an underlying problem here.

So am I. What caused the breakage that requires this patchset?

Memory hotadd with a new node being onlined.

So can you post the oops, please? Right now I am looking at zapping the

Quoted 6 lines

I can't post the oops from a pre-release system.

Quoted 1 line

series from slab.git due to NAKs from both Christoph and Nick.

Huh? They just complained about the patch, not the whole series. I don't understand how that could prompt you to drop the whole series.

Yeah, I meant the non-ACK'd patches. Sorry for the confusion.

Quoted 5 lines · Christoph Lameter

Quoted 1 line · Nick Piggin

I'm just worried there is still an underlying problem here.

So am I. What caused the breakage that requires this patchset?

Memory hotadd with a new node being onlined.

Quoted 6 lines · Andi Kleen

Quoted 4 lines · Christoph Lameter

Quoted 1 line · Nick Piggin

I'm just worried there is still an underlying problem here.

So am I. What caused the breakage that requires this patchset?

Memory hotadd with a new node being onlined.

That used to work fine.

Quoted 5 lines

Quoted 3 lines

Quoted 2 lines

Quoted 1 line

I'm just worried there is still an underlying problem here.

So am I. What caused the breakage that requires this patchset?

Memory hotadd with a new node being onlined.

That used to work fine.

OK, can we get this issue resolved? The merge window is open and Christoph seems to be unhappy with the whole patch queue. I'd hate this bug fix to miss .34...

I don't see how memory hotadd with a new node being onlined could have worked fine before since slab lacked any memory hotplug notifier until Andi just added it.

Quoted 3 lines

I don't see how memory hotadd with a new node being onlined could have worked fine before since slab lacked any memory hotplug notifier until Andi just added it.

AFAICR The cpu notifier took on that role in the past.

Quoted 6 lines · Christoph Lameter

Quoted 3 lines

I don't see how memory hotadd with a new node being onlined could have worked fine before since slab lacked any memory hotplug notifier until Andi just added it.

AFAICR The cpu notifier took on that role in the past.

The cpu notifier isn't involved if the firmware notifies the kernel that a new ACPI memory device has been added or you write a start address to /sys/devices/system/memory/probe. Hot-added memory devices can include ACPI_SRAT_MEM_HOT_PLUGGABLE entries in the SRAT for x86 that assign them non-online node ids (although all such entries get their bits set in node_possible_map at boot), so a new pgdat may be allocated for the node's registered range.

kmalloc_node() in generic kernel code. All that is done under MEM_GOING_ONLINE and not MEM_ONLINE, which is why I suggest the first and fourth patch in this series may not be necessary if we prevent setting the bit in the nodemask or building the zonelists until the slab nodelists are ready.

Quoted 5 lines

kmalloc_node() in generic kernel code. All that is done under MEM_GOING_ONLINE and not MEM_ONLINE, which is why I suggest the first and fourth patch in this series may not be necessary if we prevent setting the bit in the nodemask or building the zonelists until the slab nodelists are ready.

That sounds good.

Christoph Lameter kirjoitti:

Quoted 7 lines

Quoted 5 lines

kmalloc_node() in generic kernel code. All that is done under MEM_GOING_ONLINE and not MEM_ONLINE, which is why I suggest the first and fourth patch in this series may not be necessary if we prevent setting the bit in the nodemask or building the zonelists until the slab nodelists are ready.

That sounds good.

Andi?

Quoted 7 lines · Pekka Enberg

Christoph Lameter kirjoitti:

Quoted 6 lines

Quoted 5 lines

kmalloc_node() in generic kernel code.  All that is done under MEM_GOING_ONLINE and not MEM_ONLINE, which is why I suggest the first and fourth patch in this series may not be necessary if we prevent setting the bit in the nodemask or building the zonelists until the slab nodelists are ready.

Quoted 3 lines

Quoted 1 line

That sounds good.

Andi?

Well if Christoph wants to submit a better patch that is tested and solves the problems he can do that.

if he doesn't then I think my patch kit which has been tested is the best alternative currently.

Quoted 8 lines · Andi Kleen

Quoted 7 lines · Pekka Enberg

Christoph Lameter kirjoitti:

Quoted 6 lines

Quoted 5 lines

kmalloc_node() in generic kernel code. All that is done under MEM_GOING_ONLINE and not MEM_ONLINE, which is why I suggest the first and fourth patch in this series may not be necessary if we prevent setting the bit in the nodemask or building the zonelists until the slab nodelists are ready.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

That sounds good.

Andi?

Well if Christoph wants to submit a better patch that is tested and solves the problems he can do that.

Sure.

Quoted 2 lines

if he doesn't then I think my patch kit which has been tested is the best alternative currently.

So do you expect me to merge your patches over his objections?

Quoted 3 lines · David Rientjes

I don't see how memory hotadd with a new node being onlined could have worked fine before since slab lacked any memory hotplug notifier until Andi just added it.

AFAICR The cpu notifier took on that role in the past.

If what you say is true then memory hotplug has never worked before. Kamesan?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 3 lines · David Rientjes

I don't see how memory hotadd with a new node being onlined could have worked fine before since slab lacked any memory hotplug notifier until Andi just added it.

AFAICR The cpu notifier took on that role in the past.

If what you say is true then memory hotplug has never worked before. Kamesan?

Memory hotplug with node add never quite worked on x86 before, for various reasons not related to slab.

I mean why the core changes if this is an x86 issue?

Quoted 2 lines · Andi Kleen

Memory hotplug with node add never quite worked on x86 before, for various reasons not related to slab.

Ok but why did things break in such a big way?

Quoted 4 lines

Quoted 2 lines

Memory hotplug with node add never quite worked on x86 before, for various reasons not related to slab.

Ok but why did things break in such a big way?

1) numa memory hotadd never worked

2) the rest just bitrotted because nobody tested it.

Quoted 6 lines · Andi Kleen

Quoted 4 lines

Quoted 2 lines

Memory hotplug with node add never quite worked on x86 before, for various reasons not related to slab.

Ok but why did things break in such a big way?

1) numa memory hotadd never worked

Well Kamesan indicated that this worked if a cpu became online.

Quoted 5 lines · Christoph Lameter

Quoted 3 lines · Andi Kleen

Quoted 3 lines

Quoted 2 lines

Memory hotplug with node add never quite worked on x86 before, for various reasons not related to slab.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Ok but why did things break in such a big way?

1) numa memory hotadd never worked

Well Kamesan indicated that this worked if a cpu became online.

I mean in the general case. There were tons of problems all over.

Quoted 4 lines

Quoted 1 line

Well Kamesan indicated that this worked if a cpu became online.

I mean in the general case. There were tons of problems all over.

Then, it's cpu hotplug matter, not memory hotplug. cpu hotplug callback should prepaare

	l3 = searchp->nodelists[node];
	BUG_ON(!l3);

before onlined. Rather than taking care of races.

Quoted 1 line

1) numa memory hotadd never worked

Well Kamesan indicated that this worked if a cpu became online.

Quoted 1 line

2) the rest just bitrotted because nobody tested it.

Yep. David: Can you revise the relevant portions of the patchset and repost it?

Slab lacks any memory hotplug support for nodes that are hotplugged without cpus being hotplugged. This is possible at least on x86 CONFIG_MEMORY_HOTPLUG_SPARSE kernels where SRAT entries are marked ACPI_SRAT_MEM_HOT_PLUGGABLE and the regions of RAM represent a seperate node. It can also be done manually by writing the start address to /sys/devices/system/memory/probe for kernels that have CONFIG_ARCH_MEMORY_PROBE set, which is how this patch was tested, and then onlining the new memory region.

When a node is hotadded, a nodelist for that node is allocated and initialized for each slab cache. If this isn't completed due to a lack of memory, the hotadd is aborted: we have a reasonable expectation that
kmalloc_node(nid) will work for all caches if nid is online and memory is
available.

Since nodelists must be allocated and initialized prior to the new node's memory actually being online, the struct kmem_list3 is allocated off-node due to kmalloc_node()'s fallback.

When an entire node is offlined (or an online is aborted), these nodelists are subsequently drained and freed. If objects still exist either on the partial or full lists for those nodes, the offline is aborted. This scenario will not occur for an aborted online, however, since objects can never be allocated from those nodelists until the online has completed.

Signed-off-by David Rientjes <[email protected]>

Not sure how this would sync with slab use during node bootstrap and shutdown. Kame-san?

Not sure how this would sync with slab use during node bootstrap and shutdown. Kame-san?

Otherwise

Acked-by Christoph Lameter <[email protected]>

At node hot-add

* pgdat is allocated from other node (because we have no memory for "nid")
* memmap for the first section (and possiby others) will be allocated from other nodes.
* Once a section for the node is onlined, any memory can be allocated localy.

(Allocating memory from local node requires some new implementation as
bootmem allocater, we didn't that.)

Before this patch, slab's control layer is allocated by cpuhotplug. So, at least keeping this order,

    memory online -> cpu online

slab's control layer is allocated from local node.

When node-hotadd is done in this order

    cpu online -> memory online

kmalloc_node() will allocate memory from other node via fallback.

After this patch, slab's control layer is allocated by memory hotplug. Then, in any order, slab's control will be allocated via fallback routine.

Quoted 7 lines · KAMEZAWA Hiroyuki

At node hot-add

* pgdat is allocated from other node (because we have no memory for "nid")
* memmap for the first section (and possiby others) will be allocated from other nodes.
* Once a section for the node is onlined, any memory can be allocated localy.

Correct, and the struct kmem_list3 is also alloacted from other nodes with my patch.

Quoted 6 lines

(Allocating memory from local node requires some new implementation as
bootmem allocater, we didn't that.)

Before this patch, slab's control layer is allocated by cpuhotplug. So, at least keeping this order,

    memory online -> cpu online

Quoted 9 lines

slab's control layer is allocated from local node.

When node-hotadd is done in this order

    cpu online -> memory online

kmalloc_node() will allocate memory from other node via fallback.

After this patch, slab's control layer is allocated by memory hotplug. Then, in any order, slab's control will be allocated via fallback routine.

Again, this addresses memory hotplug that requires a new node to be onlined that do not have corresponding cpus that are being onlined. On x86, these represent ACPI_SRAT_MEM_HOT_PLUGGABLE regions that are onlined either by the acpi hotplug or done manually with CONFIG_ARCH_MEMORY_PROBE. On other architectures such as powerpc, this is done in different ways.

All of this is spelled out in the changelog for the patch.

Slab lacks any memory hotplug support for nodes that are hotplugged without cpus being hotplugged. This is possible at least on x86 CONFIG_MEMORY_HOTPLUG_SPARSE kernels where SRAT entries are marked ACPI_SRAT_MEM_HOT_PLUGGABLE and the regions of RAM represent a seperate node. It can also be done manually by writing the start address to /sys/devices/system/memory/probe for kernels that have CONFIG_ARCH_MEMORY_PROBE set, which is how this patch was tested, and then onlining the new memory region.

Quoted 8 lines · David Rientjes

Slab lacks any memory hotplug support for nodes that are hotplugged without cpus being hotplugged.  This is possible at least on x86 CONFIG_MEMORY_HOTPLUG_SPARSE kernels where SRAT entries are marked ACPI_SRAT_MEM_HOT_PLUGGABLE and the regions of RAM represent a seperate node.  It can also be done manually by writing the start address to /sys/devices/system/memory/probe for kernels that have CONFIG_ARCH_MEMORY_PROBE set, which is how this patch was tested, and then onlining the new memory region.

The patch looks far more complicated than my simple fix.

Is more complicated now better?

Quoted 1 line

The patch looks far more complicated than my simple fix.

I wouldn't exactly call the fallback_alloc() games "simple".

Quoted 1 line

Is more complicated now better?

Heh, heh. You can't post the oops, you don't want to rework your

patches as per review comments, and now you complain about David's patch without one bit of technical content. I'm sorry but I must

But anyway, if you have real technical concerns over the patch, please make them known; otherwise I'd much appreciate a Tested-by tag from you for David's patch.

Quoted 3 lines

But anyway, if you have real technical concerns over the patch, please make them known; otherwise I'd much appreciate a Tested-by tag from you for David's patch.

If it works it would be ok for me. The main concern would be to actually get it fixed.

The patch looks far more complicated than my simple fix.

Is more complicated now better?

Slab lacks any memory hotplug support for nodes that are hotplugged without cpus being hotplugged. This is possible at least on x86 CONFIG_MEMORY_HOTPLUG_SPARSE kernels where SRAT entries are marked ACPI_SRAT_MEM_HOT_PLUGGABLE and the regions of RAM represent a seperate node. It can also be done manually by writing the start address to /sys/devices/system/memory/probe for kernels that have CONFIG_ARCH_MEMORY_PROBE set, which is how this patch was tested, and then onlining the new memory region.

When a node is hotadded, a nodelist for that node is allocated and initialized for each slab cache. If this isn't completed due to a lack of memory, the hotadd is aborted: we have a reasonable expectation that kmalloc_node(nid) will work for all caches if nid is online and memory is available.

Since nodelists must be allocated and initialized prior to the new node's memory actually being online, the struct kmem_list3 is allocated off-node due to kmalloc_node()'s fallback.

When an entire node is offlined (or an online is aborted), these nodelists are subsequently drained and freed. If objects still exist either on the partial or full lists for those nodes, the offline is aborted. This scenario will not occur for an aborted online, however, since objects can never be allocated from those nodelists until the online has completed.

Signed-off-by David Rientjes <[email protected]>

mm/slab.c +42 -0

+#if defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)
+/*
+ * Drains and frees nodelists for a node on each slab cache, used for memory
+ * hotplug.  Returns -EBUSY if all objects cannot be drained on memory
+ * hot-remove so that the node is not removed.  When used because memory
+ * hot-add is canceled, the only result is the freed kmem_list3.
+ *
+ * Must hold cache_chain_mutex.
+ */
+static int __meminit free_cache_nodelists_node(int node)
+{
+	struct kmem_cache *cachep;
+	int ret = 0;
+
+	list_for_each_entry(cachep, &cache_chain, next) {
+		struct array_cache *shared;
+		struct array_cache **alien;
+		struct kmem_list3 *l3;
+
+		l3 = cachep->nodelists[node];
+		if (!l3)
+			continue;
+
+		spin_lock_irq(&l3->list_lock);
+		shared = l3->shared;
+		if (shared) {
+			free_block(cachep, shared->entry, shared->avail, node);
+			l3->shared = NULL;
+		}
+		alien = l3->alien;
+		l3->alien = NULL;
+		spin_unlock_irq(&l3->list_lock);
+
+		if (alien) {
+			drain_alien_cache(cachep, alien);
+			free_alien_cache(alien);
+		}
+		kfree(shared);
+
+		drain_freelist(cachep, l3, l3->free_objects);
+		if (!list_empty(&l3->slabs_full) ||
+					!list_empty(&l3->slabs_partial)) {

mm/slab.c +34 -0

+	}
+	return ret;
+}
+
+/*
+ * Onlines nid either as the result of memory hot-add or canceled hot-remove.
+ */
+static int __meminit slab_node_online(int nid)
+{
+	int ret;
+	mutex_lock(&cache_chain_mutex);
+	ret = init_cache_nodelists_node(nid);
+	mutex_unlock(&cache_chain_mutex);
+	return ret;
+}
+
+/*
+ * Offlines nid either as the result of memory hot-remove or canceled hot-add.
+ */
+static int __meminit slab_node_offline(int nid)
+{
+	int ret;
+	mutex_lock(&cache_chain_mutex);
+	ret = free_cache_nodelists_node(nid);
+	mutex_unlock(&cache_chain_mutex);
+	return ret;
+}
+
+static int __meminit slab_memory_callback(struct notifier_block *self,
+					unsigned long action, void *arg)
+{
+	struct memory_notify *mnb = arg;
+	int ret = 0;
+	int nid;

mm/slab.c +16 -2

+	case MEM_GOING_OFFLINE:
+	case MEM_CANCEL_ONLINE:
+		ret = slab_node_offline(nid);
+		break;
+	case MEM_ONLINE:
+	case MEM_OFFLINE:
+		break;
+	}
+out:
+	return ret ? notifier_from_errno(ret) : NOTIFY_OK;
+}
+#endif /* CONFIG_NUMA && CONFIG_MEMORY_HOTPLUG */
+
 /*
  * swap the static kmem_list3 with kmalloced memory
  */
-static void init_list(struct kmem_cache *cachep, struct kmem_list3 *list,
-			int nodeid)
+static void __init init_list(struct kmem_cache *cachep, struct kmem_list3 *list,
+				int nodeid)
 {
 	struct kmem_list3 *ptr;

@@ -1583,6 +1713,14 @@ void __init kmem_cache_init_late(void)
 	 */
 	register_cpu_notifier(&cpucache_notifier);
 
+#ifdef CONFIG_NUMA

Dumb question: it is possible to hot remove the (bad) memory ? And add an good one ?

Under certain conditions this is possible. If the bad memory was modified then you have a condition that requires termination of all processes that are using the memory. If its the kernel then you need to reboot.

If the memory contains a page from disk then the memory can be moved elsewhere.

If you can clean up a whole range like that then its possible to replace the memory.

Dumb question: it is possible to hot remove the (bad) memory ? And add an good one ?

Where is the detection code for the bad module ?

Quoted 47 lines

mm/slab.c +47 -0

+#if defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)
+/*
+ * Drains and frees nodelists for a node on each slab cache, used for memory
+ * hotplug.  Returns -EBUSY if all objects cannot be drained on memory
+ * hot-remove so that the node is not removed.  When used because memory
+ * hot-add is canceled, the only result is the freed kmem_list3.
+ *
+ * Must hold cache_chain_mutex.
+ */
+static int __meminit free_cache_nodelists_node(int node)
+{
+	struct kmem_cache *cachep;
+	int ret = 0;
+
+	list_for_each_entry(cachep, &cache_chain, next) {
+		struct array_cache *shared;
+		struct array_cache **alien;
+		struct kmem_list3 *l3;
+
+		l3 = cachep->nodelists[node];
+		if (!l3)
+			continue;
+
+		spin_lock_irq(&l3->list_lock);
+		shared = l3->shared;
+		if (shared) {
+			free_block(cachep, shared->entry, shared->avail, node);
+			l3->shared = NULL;
+		}
+		alien = l3->alien;
+		l3->alien = NULL;
+		spin_unlock_irq(&l3->list_lock);
+
+		if (alien) {
+			drain_alien_cache(cachep, alien);
+			free_alien_cache(alien);
+		}
+		kfree(shared);
+
+		drain_freelist(cachep, l3, l3->free_objects);
+		if (!list_empty(&l3->slabs_full) ||
+					!list_empty(&l3->slabs_partial)) {
+			/*
+			 * Continue to iterate through each slab cache to free
+			 * as many nodelists as possible even though the
+			 * offline will be canceled.
+			 */

mm/slab.c static struct notifier_block __cpuinitdata cpucache_notifier = { · line 1409

Quoted 5 lines

mm/slab.c +5 -0

+			ret = -EBUSY;
+			continue;
+		}
+		kfree(l3);
+		cachep->nodelists[node] = NULL;

What's stopping races of other CPUs trying to access l3 and array caches while they're being freed?

Quoted 40 lines

mm/slab.c +40 -0

+	}
+	return ret;
+}
+
+/*
+ * Onlines nid either as the result of memory hot-add or canceled hot-remove.
+ */
+static int __meminit slab_node_online(int nid)
+{
+	int ret;
+	mutex_lock(&cache_chain_mutex);
+	ret = init_cache_nodelists_node(nid);
+	mutex_unlock(&cache_chain_mutex);
+	return ret;
+}
+
+/*
+ * Offlines nid either as the result of memory hot-remove or canceled hot-add.
+ */
+static int __meminit slab_node_offline(int nid)
+{
+	int ret;
+	mutex_lock(&cache_chain_mutex);
+	ret = free_cache_nodelists_node(nid);
+	mutex_unlock(&cache_chain_mutex);
+	return ret;
+}
+
+static int __meminit slab_memory_callback(struct notifier_block *self,
+					unsigned long action, void *arg)
+{
+	struct memory_notify *mnb = arg;
+	int ret = 0;
+	int nid;
+
+	nid = mnb->status_change_nid;
+	if (nid < 0)
+		goto out;
+
+	switch (action) {

mm/slab.c static struct notifier_block __cpuinitdata cpucache_notifier = { · line 1453

Quoted 4 lines

mm/slab.c +4 -0

+	case MEM_GOING_ONLINE:
+	case MEM_CANCEL_OFFLINE:
+		ret = slab_node_online(nid);
+		break;

This would explode if CANCEL_OFFLINE fails. Call it theoretical and put a panic() in here and I don't mind. Otherwise you get corruption somewhere in the slab code.

Quoted 47 lines · Nick Piggin

Quoted 47 lines

mm/slab.c +47 -0

+#if defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)
+/*
+ * Drains and frees nodelists for a node on each slab cache, used for memory
+ * hotplug.  Returns -EBUSY if all objects cannot be drained on memory
+ * hot-remove so that the node is not removed.  When used because memory
+ * hot-add is canceled, the only result is the freed kmem_list3.
+ *
+ * Must hold cache_chain_mutex.
+ */
+static int __meminit free_cache_nodelists_node(int node)
+{
+	struct kmem_cache *cachep;
+	int ret = 0;
+
+	list_for_each_entry(cachep, &cache_chain, next) {
+		struct array_cache *shared;
+		struct array_cache **alien;
+		struct kmem_list3 *l3;
+
+		l3 = cachep->nodelists[node];
+		if (!l3)
+			continue;
+
+		spin_lock_irq(&l3->list_lock);
+		shared = l3->shared;
+		if (shared) {
+			free_block(cachep, shared->entry, shared->avail, node);
+			l3->shared = NULL;
+		}
+		alien = l3->alien;
+		l3->alien = NULL;
+		spin_unlock_irq(&l3->list_lock);
+
+		if (alien) {
+			drain_alien_cache(cachep, alien);
+			free_alien_cache(alien);
+		}
+		kfree(shared);
+
+		drain_freelist(cachep, l3, l3->free_objects);
+		if (!list_empty(&l3->slabs_full) ||
+					!list_empty(&l3->slabs_partial)) {
+			/*
+			 * Continue to iterate through each slab cache to free
+			 * as many nodelists as possible even though the
+			 * offline will be canceled.
+			 */

Quoted 9 lines

mm/slab.c static struct notifier_block __cpuinitdata cpucache_notifier = { · line 1409

Quoted 5 lines

mm/slab.c +5 -0

+			ret = -EBUSY;
+			continue;
+		}
+		kfree(l3);
+		cachep->nodelists[node] = NULL;

What's stopping races of other CPUs trying to access l3 and array caches while they're being freed?

numa_node_id() will not return an offlined nodeid and cache_alloc_node() already does a fallback to other onlined nodes in case a nodeid is passed to kmalloc_node() that does not have a nodelist. l3->shared and l3->alien cannot be accessed without l3->list_lock (drain, cache_alloc_refill, cache_flusharray) or cache_chain_mutex (kmem_cache_destroy, cache_reap).

Quoted 40 lines

Quoted 40 lines

mm/slab.c +40 -0

+	}
+	return ret;
+}
+
+/*
+ * Onlines nid either as the result of memory hot-add or canceled hot-remove.
+ */
+static int __meminit slab_node_online(int nid)
+{
+	int ret;
+	mutex_lock(&cache_chain_mutex);
+	ret = init_cache_nodelists_node(nid);
+	mutex_unlock(&cache_chain_mutex);
+	return ret;
+}
+
+/*
+ * Offlines nid either as the result of memory hot-remove or canceled hot-add.
+ */
+static int __meminit slab_node_offline(int nid)
+{
+	int ret;
+	mutex_lock(&cache_chain_mutex);
+	ret = free_cache_nodelists_node(nid);
+	mutex_unlock(&cache_chain_mutex);
+	return ret;
+}
+
+static int __meminit slab_memory_callback(struct notifier_block *self,
+					unsigned long action, void *arg)
+{
+	struct memory_notify *mnb = arg;
+	int ret = 0;
+	int nid;
+
+	nid = mnb->status_change_nid;
+	if (nid < 0)
+		goto out;
+
+	switch (action) {

Quoted 9 lines

mm/slab.c static struct notifier_block __cpuinitdata cpucache_notifier = { · line 1453

Quoted 4 lines

mm/slab.c +4 -0

+	case MEM_GOING_ONLINE:
+	case MEM_CANCEL_OFFLINE:
+		ret = slab_node_online(nid);
+		break;

This would explode if CANCEL_OFFLINE fails. Call it theoretical and put a panic() in here and I don't mind. Otherwise you get corruption somewhere in the slab code.

MEM_CANCEL_ONLINE would only fail here if a struct kmem_list3 couldn't be allocated anywhere on the system and if that happens then the node simply couldn't be allocated from (numa_node_id() would never return it as the cpu's node, so it's possible to fallback in this scenario).

Instead of doing this all at MEM_GOING_OFFLINE, we could delay freeing of the array caches and the nodelist until MEM_OFFLINE. We're guaranteed that all pages are freed at that point so there are no existing objects that we need to track and then if the offline fails from a different callback it would be possible to reset the l3->nodelists[node] pointers since they haven't been freed yet.

Quoted 57 lines · David Rientjes

Quoted 46 lines · Nick Piggin

Quoted 46 lines

mm/slab.c +46 -0

+#if defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)
+/*
+ * Drains and frees nodelists for a node on each slab cache, used for memory
+ * hotplug.  Returns -EBUSY if all objects cannot be drained on memory
+ * hot-remove so that the node is not removed.  When used because memory
+ * hot-add is canceled, the only result is the freed kmem_list3.
+ *
+ * Must hold cache_chain_mutex.
+ */
+static int __meminit free_cache_nodelists_node(int node)
+{
+	struct kmem_cache *cachep;
+	int ret = 0;
+
+	list_for_each_entry(cachep, &cache_chain, next) {
+		struct array_cache *shared;
+		struct array_cache **alien;
+		struct kmem_list3 *l3;
+
+		l3 = cachep->nodelists[node];
+		if (!l3)
+			continue;
+
+		spin_lock_irq(&l3->list_lock);
+		shared = l3->shared;
+		if (shared) {
+			free_block(cachep, shared->entry, shared->avail, node);
+			l3->shared = NULL;
+		}
+		alien = l3->alien;
+		l3->alien = NULL;
+		spin_unlock_irq(&l3->list_lock);
+
+		if (alien) {
+			drain_alien_cache(cachep, alien);
+			free_alien_cache(alien);
+		}
+		kfree(shared);
+
+		drain_freelist(cachep, l3, l3->free_objects);
+		if (!list_empty(&l3->slabs_full) ||
+					!list_empty(&l3->slabs_partial)) {
+			/*
+			 * Continue to iterate through each slab cache to free
+			 * as many nodelists as possible even though the
+			 * offline will be canceled.

Quoted 9 lines

mm/slab.c static struct notifier_block __cpuinitdata cpucache_notifier = { · line 1409

Quoted 6 lines

mm/slab.c +6 -0

+			 */
+			ret = -EBUSY;
+			continue;
+		}
+		kfree(l3);
+		cachep->nodelists[node] = NULL;

What's stopping races of other CPUs trying to access l3 and array caches while they're being freed?

Quoted 1 line

numa_node_id() will not return an offlined nodeid and cache_alloc_node() already does a fallback to other onlined nodes in case a nodeid is passed to kmalloc_node() that does not have a nodelist. l3->shared and l3->alien cannot be accessed without l3->list_lock (drain, cache_alloc_refill, cache_flusharray) or cache_chain_mutex (kmem_cache_destroy, cache_reap).

Yeah, but can't it _have_ a nodelist (ie. before it is set to NULL here) while it is being accessed by another CPU and concurrently being freed on this one?

Quoted 48 lines

Quoted 48 lines

Quoted 34 lines

mm/slab.c +34 -0

+	}
+	return ret;
+}
+
+/*
+ * Onlines nid either as the result of memory hot-add or canceled hot-remove.
+ */
+static int __meminit slab_node_online(int nid)
+{
+	int ret;
+	mutex_lock(&cache_chain_mutex);
+	ret = init_cache_nodelists_node(nid);
+	mutex_unlock(&cache_chain_mutex);
+	return ret;
+}
+
+/*
+ * Offlines nid either as the result of memory hot-remove or canceled hot-add.
+ */
+static int __meminit slab_node_offline(int nid)
+{
+	int ret;
+	mutex_lock(&cache_chain_mutex);
+	ret = free_cache_nodelists_node(nid);
+	mutex_unlock(&cache_chain_mutex);
+	return ret;
+}
+
+static int __meminit slab_memory_callback(struct notifier_block *self,
+					unsigned long action, void *arg)
+{
+	struct memory_notify *mnb = arg;
+	int ret = 0;
+	int nid;

mm/slab.c static struct notifier_block __cpuinitdata cpucache_notifier = { · line 1453

Quoted 10 lines

mm/slab.c +10 -0

+
+	nid = mnb->status_change_nid;
+	if (nid < 0)
+		goto out;
+
+	switch (action) {
+	case MEM_GOING_ONLINE:
+	case MEM_CANCEL_OFFLINE:
+		ret = slab_node_online(nid);
+		break;

This would explode if CANCEL_OFFLINE fails. Call it theoretical and put a panic() in here and I don't mind. Otherwise you get corruption somewhere in the slab code.

Quoted 1 line

MEM_CANCEL_ONLINE would only fail here if a struct kmem_list3 couldn't be allocated anywhere on the system and if that happens then the node simply couldn't be allocated from (numa_node_id() would never return it as the cpu's node, so it's possible to fallback in this scenario).

Why would it never return the CPU's node? It's CANCEL_OFFLINE that is the problem.

Quoted 125 lines · Nick Piggin

Quoted 57 lines · David Rientjes

Quoted 46 lines · Nick Piggin

Quoted 46 lines

mm/slab.c +46 -0

+#if defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)
+/*
+ * Drains and frees nodelists for a node on each slab cache, used for memory
+ * hotplug.  Returns -EBUSY if all objects cannot be drained on memory
+ * hot-remove so that the node is not removed.  When used because memory
+ * hot-add is canceled, the only result is the freed kmem_list3.
+ *
+ * Must hold cache_chain_mutex.
+ */
+static int __meminit free_cache_nodelists_node(int node)
+{
+	struct kmem_cache *cachep;
+	int ret = 0;
+
+	list_for_each_entry(cachep, &cache_chain, next) {
+		struct array_cache *shared;
+		struct array_cache **alien;
+		struct kmem_list3 *l3;
+
+		l3 = cachep->nodelists[node];
+		if (!l3)
+			continue;
+
+		spin_lock_irq(&l3->list_lock);
+		shared = l3->shared;
+		if (shared) {
+			free_block(cachep, shared->entry, shared->avail, node);
+			l3->shared = NULL;
+		}
+		alien = l3->alien;
+		l3->alien = NULL;
+		spin_unlock_irq(&l3->list_lock);
+
+		if (alien) {
+			drain_alien_cache(cachep, alien);
+			free_alien_cache(alien);
+		}
+		kfree(shared);
+
+		drain_freelist(cachep, l3, l3->free_objects);
+		if (!list_empty(&l3->slabs_full) ||
+					!list_empty(&l3->slabs_partial)) {
+			/*
+			 * Continue to iterate through each slab cache to free
+			 * as many nodelists as possible even though the
+			 * offline will be canceled.

Quoted 9 lines

mm/slab.c static struct notifier_block __cpuinitdata cpucache_notifier = { · line 1409

Quoted 6 lines

mm/slab.c +6 -0

+			 */
+			ret = -EBUSY;
+			continue;
+		}
+		kfree(l3);
+		cachep->nodelists[node] = NULL;

What's stopping races of other CPUs trying to access l3 and array caches while they're being freed?

Quoted 7 lines

numa_node_id() will not return an offlined nodeid and cache_alloc_node() already does a fallback to other onlined nodes in case a nodeid is passed to kmalloc_node() that does not have a nodelist. l3->shared and l3->alien cannot be accessed without l3->list_lock (drain, cache_alloc_refill, cache_flusharray) or cache_chain_mutex (kmem_cache_destroy, cache_reap).

Yeah, but can't it _have_ a nodelist (ie. before it is set to NULL here) while it is being accessed by another CPU and concurrently being freed on this one?

Quoted 53 lines

Quoted 39 lines

Quoted 39 lines

mm/slab.c +39 -0

+	}
+	return ret;
+}
+
+/*
+ * Onlines nid either as the result of memory hot-add or canceled hot-remove.
+ */
+static int __meminit slab_node_online(int nid)
+{
+	int ret;
+	mutex_lock(&cache_chain_mutex);
+	ret = init_cache_nodelists_node(nid);
+	mutex_unlock(&cache_chain_mutex);
+	return ret;
+}
+
+/*
+ * Offlines nid either as the result of memory hot-remove or canceled hot-add.
+ */
+static int __meminit slab_node_offline(int nid)
+{
+	int ret;
+	mutex_lock(&cache_chain_mutex);
+	ret = free_cache_nodelists_node(nid);
+	mutex_unlock(&cache_chain_mutex);
+	return ret;
+}
+
+static int __meminit slab_memory_callback(struct notifier_block *self,
+					unsigned long action, void *arg)
+{
+	struct memory_notify *mnb = arg;
+	int ret = 0;
+	int nid;
+
+	nid = mnb->status_change_nid;
+	if (nid < 0)
+		goto out;
+

Quoted 9 lines

mm/slab.c static struct notifier_block __cpuinitdata cpucache_notifier = { · line 1453

Quoted 5 lines

mm/slab.c +5 -0

+	switch (action) {
+	case MEM_GOING_ONLINE:
+	case MEM_CANCEL_OFFLINE:
+		ret = slab_node_online(nid);
+		break;

This would explode if CANCEL_OFFLINE fails. Call it theoretical and put a panic() in here and I don't mind. Otherwise you get corruption somewhere in the slab code.

MEM_CANCEL_ONLINE would only fail here if a struct kmem_list3 couldn't be allocated anywhere on the system and if that happens then the node simply couldn't be allocated from (numa_node_id() would never return it as the cpu's node, so it's possible to fallback in this scenario).

Quoted 2 lines

Why would it never return the CPU's node? It's CANCEL_OFFLINE that is the problem.

So I was thinking of pushing this towards Linus but I didn't see anyone respond to Nick's concerns. I'm not that familiar with all this hotplug stuff so can someone make also Nick happy so we can move forward?

Quoted 59 lines · David Rientjes

Quoted 47 lines · Nick Piggin

Quoted 47 lines

mm/slab.c +47 -0

+#if defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)
+/*
+ * Drains and frees nodelists for a node on each slab cache, used for memory
+ * hotplug.  Returns -EBUSY if all objects cannot be drained on memory
+ * hot-remove so that the node is not removed.  When used because memory
+ * hot-add is canceled, the only result is the freed kmem_list3.
+ *
+ * Must hold cache_chain_mutex.
+ */
+static int __meminit free_cache_nodelists_node(int node)
+{
+	struct kmem_cache *cachep;
+	int ret = 0;
+
+	list_for_each_entry(cachep, &cache_chain, next) {
+		struct array_cache *shared;
+		struct array_cache **alien;
+		struct kmem_list3 *l3;
+
+		l3 = cachep->nodelists[node];
+		if (!l3)
+			continue;
+
+		spin_lock_irq(&l3->list_lock);
+		shared = l3->shared;
+		if (shared) {
+			free_block(cachep, shared->entry, shared->avail, node);
+			l3->shared = NULL;
+		}
+		alien = l3->alien;
+		l3->alien = NULL;
+		spin_unlock_irq(&l3->list_lock);
+
+		if (alien) {
+			drain_alien_cache(cachep, alien);
+			free_alien_cache(alien);
+		}
+		kfree(shared);
+
+		drain_freelist(cachep, l3, l3->free_objects);
+		if (!list_empty(&l3->slabs_full) ||
+					!list_empty(&l3->slabs_partial)) {
+			/*
+			 * Continue to iterate through each slab cache to free
+			 * as many nodelists as possible even though the
+			 * offline will be canceled.
+			 */

Quoted 9 lines

mm/slab.c static struct notifier_block __cpuinitdata cpucache_notifier = { · line 1409

Quoted 5 lines

mm/slab.c +5 -0

+			ret = -EBUSY;
+			continue;
+		}
+		kfree(l3);
+		cachep->nodelists[node] = NULL;

What's stopping races of other CPUs trying to access l3 and array caches while they're being freed?

Quoted 5 lines

numa_node_id() will not return an offlined nodeid and cache_alloc_node() already does a fallback to other onlined nodes in case a nodeid is passed to kmalloc_node() that does not have a nodelist. l3->shared and l3->alien cannot be accessed without l3->list_lock (drain, cache_alloc_refill, cache_flusharray) or cache_chain_mutex (kmem_cache_destroy, cache_reap).

Yeah, but can't it _have_ a nodelist (ie. before it is set to NULL here) while it is being accessed by another CPU and concurrently being freed on this one?

Slab lacks any memory hotplug support for nodes that are hotplugged without cpus being hotplugged.  This is possible at least on x86 CONFIG_MEMORY_HOTPLUG_SPARSE kernels where SRAT entries are marked ACPI_SRAT_MEM_HOT_PLUGGABLE and the regions of RAM represent a seperate node.  It can also be done manually by writing the start address to /sys/devices/system/memory/probe for kernels that have CONFIG_ARCH_MEMORY_PROBE set, which is how this patch was tested, and then onlining the new memory region.

When a node is hotadded, a nodelist for that node is allocated and initialized for each slab cache.  If this isn't completed due to a lack of memory, the hotadd is aborted: we have a reasonable expectation that kmalloc_node(nid) will work for all caches if nid is online and memory is available.

Since nodelists must be allocated and initialized prior to the new node's memory actually being online, the struct kmem_list3 is allocated off-node due to kmalloc_node()'s fallback.

When an entire node would be offlined, its nodelists are subsequently drained.  If slab objects still exist and cannot be freed, the offline is aborted.  It is possible that objects will be allocated between this drain and page isolation, so it's still possible that the offline will still fail, however.

Signed-off-by David Rientjes <[email protected]>

---
 mm/slab.c |  157 ++++++++++++++++++++++++++++++++++++++++++++++++------------

 1 files changed, 125 insertions(+), 32 deletions(-)

mm/slab.c +51 -30

@@ -115,6 +115,7 @@
  #include       <linux/reciprocal_div.h>
  #include       <linux/debugobjects.h>
  #include       <linux/kmemcheck.h>
+#include       <linux/memory.h>
 
  #include       <asm/cacheflush.h>
  #include       <asm/tlbflush.h>

@@ -1102,6 +1103,52 @@ static inline int cache_free_alien(struct kmem_cache *cachep, void *objp)
  }
  #endif
 
+/*
+ * Allocates and initializes nodelists for a node on each slab cache, used for
+ * either memory or cpu hotplug.  If memory is being hot-added, the kmem_list3
+ * will be allocated off-node since memory is not yet online for the new node.
+ * When hotplugging memory or a cpu, existing nodelists are not replaced if
+ * already in use.
+ *
+ * Must hold cache_chain_mutex.
+ */
+static int init_cache_nodelists_node(int node)
+{
+       struct kmem_cache *cachep;
+       struct kmem_list3 *l3;
+       const int memsize = sizeof(struct kmem_list3);
+
+       list_for_each_entry(cachep, &cache_chain, next) {
+               /*
+                * Set up the size64 kmemlist for cpu before we can
+                * begin anything. Make sure some other cpu on this
+                * node has not already allocated this
+                */
+               if (!cachep->nodelists[node]) {
+                       l3 = kmalloc_node(memsize, GFP_KERNEL, node);
+                       if (!l3)
+                               return -ENOMEM;
+                       kmem_list3_init(l3);
+                       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.
+                        */
+                       cachep->nodelists[node] = l3;
+               }
+
+               spin_lock_irq(&cachep->nodelists[node]->list_lock);
+               cachep->nodelists[node]->free_limit =
+                       (1 + nr_cpus_node(node)) *
+                       cachep->batchcount + cachep->num;
+               spin_unlock_irq(&cachep->nodelists[node]->list_lock);
+       }
+       return 0;
+}
+
  static void __cpuinit cpuup_canceled(long cpu)
  {
        struct kmem_cache *cachep;

@@ -1172,7 +1219,7 @@ static int __cpuinit cpuup_prepare(long cpu)
        struct kmem_cache *cachep;
        struct kmem_list3 *l3 = NULL;
        int node = cpu_to_node(cpu);
-       const int memsize = sizeof(struct kmem_list3);
+       int err;
 
        /*
         * We need to do this right in the beginning since

@@ -1180,35 +1227,9 @@ static int __cpuinit cpuup_prepare(long cpu)
         * kmalloc_node allows us to add the slab to the right
         * kmem_list3 and not this cpu's kmem_list3
         */
-
-       list_for_each_entry(cachep, &cache_chain, next) {
-               /*
-                * Set up the size64 kmemlist for cpu before we can
-                * begin anything. Make sure some other cpu on this
-                * node has not already allocated this
-                */
-               if (!cachep->nodelists[node]) {
-                       l3 = kmalloc_node(memsize, GFP_KERNEL, node);
-                       if (!l3)
-                               goto bad;
-                       kmem_list3_init(l3);
-                       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.
-                        */
-                       cachep->nodelists[node] = l3;
-               }
-
-               spin_lock_irq(&cachep->nodelists[node]->list_lock);
-               cachep->nodelists[node]->free_limit =
-                       (1 + nr_cpus_node(node)) *
-                       cachep->batchcount + cachep->num;
-               spin_unlock_irq(&cachep->nodelists[node]->list_lock);
-       }
+       err = init_cache_nodelists_node(node);
+       if (err < 0)
+               goto bad;
 
        /*
         * Now we can go ahead with allocating the shared arrays and

@@ -1331,11 +1352,75 @@ static struct notifier_block __cpuinitdata cpucache_notifier = {

       &cpuup_callback, NULL, 0
 };

mm/slab.c +64 -0

+#if defined(CONFIG_NUMA) && defined(CONFIG_MEMORY_HOTPLUG)
+/*
+ * Drains freelist for a node on each slab cache, used for memory hot-remove.
+ * Returns -EBUSY if all objects cannot be drained so that the node is not
+ * removed.
+ *
+ * Must hold cache_chain_mutex.
+ */
+static int __meminit drain_cache_nodelists_node(int node)
+{
+       struct kmem_cache *cachep;
+       int ret = 0;
+
+       list_for_each_entry(cachep, &cache_chain, next) {
+               struct kmem_list3 *l3;
+
+               l3 = cachep->nodelists[node];
+               if (!l3)
+                       continue;
+
+               drain_freelist(cachep, l3, l3->free_objects);
+
+               if (!list_empty(&l3->slabs_full) ||
+                   !list_empty(&l3->slabs_partial)) {
+                       ret = -EBUSY;
+                       break;
+               }
+       }
+       return ret;
+}
+
+static int __meminit slab_memory_callback(struct notifier_block *self,
+                                       unsigned long action, void *arg)
+{
+       struct memory_notify *mnb = arg;
+       int ret = 0;
+       int nid;
+
+       nid = mnb->status_change_nid;
+       if (nid < 0)
+               goto out;
+
+       switch (action) {
+       case MEM_GOING_ONLINE:
+               mutex_lock(&cache_chain_mutex);
+               ret = init_cache_nodelists_node(nid);
+               mutex_unlock(&cache_chain_mutex);
+               break;
+       case MEM_GOING_OFFLINE:
+               mutex_lock(&cache_chain_mutex);
+               ret = drain_cache_nodelists_node(nid);
+               mutex_unlock(&cache_chain_mutex);
+               break;
+       case MEM_ONLINE:
+       case MEM_OFFLINE:
+       case MEM_CANCEL_ONLINE:
+       case MEM_CANCEL_OFFLINE:
+               break;
+       }
+out:
+       return ret ? notifier_from_errno(ret) : NOTIFY_OK;
+}
+#endif /* CONFIG_NUMA && CONFIG_MEMORY_HOTPLUG */
+

 /*
 * swap the static kmem_list3 with kmalloced memory */

mm/slab.c +2 -2

-static void init_list(struct kmem_cache *cachep, struct kmem_list3 *list,
-                       int nodeid)
+static void __init init_list(struct kmem_cache *cachep, struct kmem_list3 *list,
+                               int nodeid)

 {
       struct kmem_list3 *ptr;

mm/slab.c +8 -1

@@ -1580,6 +1665,14 @@ void __init kmem_cache_init_late(void)
         */
        register_cpu_notifier(&cpucache_notifier);
 
+#ifdef CONFIG_NUMA
+       /*
+        * Register a memory hotplug callback that initializes and frees
+        * nodelists.
+        */
+       hotplug_memory_notifier(slab_memory_callback, SLAB_CALLBACK_PRI);
+#endif
+
        /*
         * The reap timers are started later, with a module init call: That part
         * of the kernel is not yet operational.
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Nick, Christoph, lets make a a deal: you ACK, I merge. How does that sound to you?

Quoted 2 lines

Nick, Christoph, lets make a a deal: you ACK, I merge. How does that sound to you?

I looked through the patch before and slabwise this seems to beok but I am still not very sure how this interacts with the node and cpu bootstrap. You can have the ack with this caveat.

Acked-by Christoph Lameter <[email protected]>

Slab lacks any memory hotplug support for nodes that are hotplugged without cpus being hotplugged. This is possible at least on x86 CONFIG_MEMORY_HOTPLUG_SPARSE kernels where SRAT entries are marked ACPI_SRAT_MEM_HOT_PLUGGABLE and the regions of RAM represent a seperate node. It can also be done manually by writing the start address to /sys/devices/system/memory/probe for kernels that have CONFIG_ARCH_MEMORY_PROBE set, which is how this patch was tested, and then onlining the new memory region.

When a node is hotadded, a nodelist for that node is allocated and initialized for each slab cache. If this isn't completed due to a lack of memory, the hotadd is aborted: we have a reasonable expectation that
kmalloc_node(nid) will work for all caches if nid is online and memory is
available.

Since nodelists must be allocated and initialized prior to the new node's memory actually being online, the struct kmem_list3 is allocated off-node due to kmalloc_node()'s fallback.

When an entire node would be offlined, its nodelists are subsequently drained. If slab objects still exist and cannot be freed, the offline is aborted. It is possible that objects will be allocated between this drain and page isolation, so it's still possible that the offline will still fail, however.

Signed-off-by David Rientjes <[email protected]>

OK, can we get this issue resolved? The merge window is open and Christoph seems to be unhappy with the whole patch queue. I'd hate this bug fix to miss .34...

Quoted 3 lines

OK, can we get this issue resolved? The merge window is open and Christoph seems to be unhappy with the whole patch queue. I'd hate this bug fix to miss .34...

Merge window? These are bugs that have to be fixed independently from a merge window. The question is if this is the right approach or if there is other stuff still lurking because we are not yet seeing the full picture.

The first set of patches from Andi are almost one month old. If this issue progresses as swiftly as it has to this day, I foresee a rocky road for any of them getting merged to .34 through slab.git, that's all.

The first set of patches from Andi are almost one month old. If this issue progresses as swiftly as it has to this day, I foresee a rocky road for any

of them getting merged to .34 through slab.git, that's all.

Quoted 2 lines

How, may I ask? cpuup_prepare in the hotplug notifier should always run before start_cpu_timer.

I'm not fully sure, but I have the oops to prove it :)

Quoted 4 lines

Quoted 3 lines

How, may I ask? cpuup_prepare in the hotplug notifier should always run before start_cpu_timer.

I'm not fully sure, but I have the oops to prove it :)

I still suspect that this has something to do with Pekka's changing the boot order for allocator bootstrap. Can we clarify why these problems exist before we try band aid?

Should address all earlier comments (except for the funny cpuset case which I chose to declare a don't do that)

Also this time hopefully without missing patches.

There are still some other issues with memory hotadd, but that's the current slab set.

The patches are against 2.6.32, but apply to mainline I believe.