[PATCH] [0/4] SLAB: Fix a couple of slab memory hotadd issues

[PATCH] [0/4] SLAB: Fix a couple of slab memory hotadd issues

[Andi Kleen]

This fixes various problems in slab found during memory hotadd testing.

All straight forward bug fixes, so could be still .33 candidates.

-Andi

[PATCH] [0/4] SLAB: Fix a couple of slab memory hotadd issues

[Andi Kleen]

This fixes various problems in slab found during memory hotadd testing.

All straight forward bug fixes, so could be still .33 candidates.

-Andi

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

[PATCH] [1/4] SLAB: Handle node-not-up case in fallback_alloc()

[Andi Kleen]

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

Signed-off-by: Andi Kleen <ak@linux.intel.com>

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

Index: linux-2.6.33-rc3-ak/mm/slab.c
===================================================================
--- linux-2.6.33-rc3-ak.orig/mm/slab.c
+++ linux-2.6.33-rc3-ak/mm/slab.c
@@ -3210,7 +3210,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])
+					break;
+			}
+			if (!cache->nodelists[nid])
+				return NULL;
+		}
+
+
+		obj = kmem_getpages(cache, local_flags, nid);
 		if (local_flags & __GFP_WAIT)
 			local_irq_disable();
 		if (obj) {

[PATCH] [1/4] SLAB: Handle node-not-up case in fallback_alloc()

[Andi Kleen]

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

Signed-off-by: Andi Kleen <ak@linux.intel.com>

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

Index: linux-2.6.33-rc3-ak/mm/slab.c
===================================================================
--- linux-2.6.33-rc3-ak.orig/mm/slab.c
+++ linux-2.6.33-rc3-ak/mm/slab.c
@@ -3210,7 +3210,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])
+					break;
+			}
+			if (!cache->nodelists[nid])
+				return NULL;
+		}
+
+
+		obj = kmem_getpages(cache, local_flags, nid);
 		if (local_flags & __GFP_WAIT)
 			local_irq_disable();
 		if (obj) {

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

[PATCH] [2/4] SLAB: Set up the l3 lists for the memory of freshly added memory

[Andi Kleen]

So kmalloc_node() works even if no CPU is up yet on the new node.

Requires previous refactor patch.

Signed-off-by: Andi Kleen <ak@linux.intel.com>

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

Index: linux-2.6.33-rc3-ak/mm/slab.c
===================================================================
--- linux-2.6.33-rc3-ak.orig/mm/slab.c
+++ linux-2.6.33-rc3-ak/mm/slab.c
@@ -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>
@@ -1560,6 +1561,20 @@ 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)
+		slab_node_prepare(mn->status_change_nid);
+	return NOTIFY_OK;
+}
+
 void __init kmem_cache_init_late(void)
 {
 	struct kmem_cache *cachep;
@@ -1583,6 +1598,8 @@ void __init kmem_cache_init_late(void)
 	 */
 	register_cpu_notifier(&cpucache_notifier);
 
+	hotplug_memory_notifier(slab_memory_callback, SLAB_CALLBACK_PRI);
+
 	/*
 	 * The reap timers are started later, with a module init call: That part
 	 * of the kernel is not yet operational.

[PATCH] [2/4] SLAB: Set up the l3 lists for the memory of freshly added memory

[Andi Kleen]

So kmalloc_node() works even if no CPU is up yet on the new node.

Requires previous refactor patch.

Signed-off-by: Andi Kleen <ak@linux.intel.com>

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

Index: linux-2.6.33-rc3-ak/mm/slab.c
===================================================================
--- linux-2.6.33-rc3-ak.orig/mm/slab.c
+++ linux-2.6.33-rc3-ak/mm/slab.c
@@ -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>
@@ -1560,6 +1561,20 @@ 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)
+		slab_node_prepare(mn->status_change_nid);
+	return NOTIFY_OK;
+}
+
 void __init kmem_cache_init_late(void)
 {
 	struct kmem_cache *cachep;
@@ -1583,6 +1598,8 @@ void __init kmem_cache_init_late(void)
 	 */
 	register_cpu_notifier(&cpucache_notifier);
 
+	hotplug_memory_notifier(slab_memory_callback, SLAB_CALLBACK_PRI);
+
 	/*
 	 * The reap timers are started later, with a module init call: That part
 	 * of the kernel is not yet operational.

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

[PATCH] [3/4] SLAB: Separate node initialization into separate function

[Andi Kleen]

No functional changes.

Needed for next patch.

Signed-off-by: Andi Kleen <ak@linux.intel.com>

---
 mm/slab.c |   34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

Index: linux-2.6.33-rc3-ak/mm/slab.c
===================================================================
--- linux-2.6.33-rc3-ak.orig/mm/slab.c
+++ linux-2.6.33-rc3-ak/mm/slab.c
@@ -1171,19 +1171,9 @@ free_array_cache:
 	}
 }
 
-static int __cpuinit cpuup_prepare(long cpu)
+static int slab_node_prepare(int node)
 {
 	struct kmem_cache *cachep;
-	struct kmem_list3 *l3 = NULL;
-	int node = cpu_to_node(cpu);
-	const int memsize = sizeof(struct kmem_list3);
-
-	/*
-	 * We need to do this right in the beginning since
-	 * alloc_arraycache's are going to use this list.
-	 * 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) {
 		/*
@@ -1192,9 +1182,10 @@ static int __cpuinit cpuup_prepare(long
 		 * node has not already allocated this
 		 */
 		if (!cachep->nodelists[node]) {
-			l3 = kmalloc_node(memsize, GFP_KERNEL, node);
+			struct kmem_list3 *l3;
+			l3 = kmalloc_node(sizeof(struct kmem_list3), GFP_KERNEL, node);
 			if (!l3)
-				goto bad;
+				return -1;
 			kmem_list3_init(l3);
 			l3->next_reap = jiffies + REAPTIMEOUT_LIST3 +
 			    ((unsigned long)cachep) % REAPTIMEOUT_LIST3;
@@ -1213,6 +1204,23 @@ static int __cpuinit cpuup_prepare(long
 			cachep->batchcount + cachep->num;
 		spin_unlock_irq(&cachep->nodelists[node]->list_lock);
 	}
+	return 0;
+}
+
+static int __cpuinit cpuup_prepare(long cpu)
+{
+	struct kmem_cache *cachep;
+	struct kmem_list3 *l3 = NULL;
+	int node = cpu_to_node(cpu);
+
+	/*
+	 * We need to do this right in the beginning since
+	 * alloc_arraycache's are going to use this list.
+	 * kmalloc_node allows us to add the slab to the right
+	 * kmem_list3 and not this cpu's kmem_list3
+	 */
+	if (slab_node_prepare(node) < 0)
+		goto bad;
 
 	/*
 	 * Now we can go ahead with allocating the shared arrays and

[PATCH] [3/4] SLAB: Separate node initialization into separate function

[Andi Kleen]

No functional changes.

Needed for next patch.

Signed-off-by: Andi Kleen <ak@linux.intel.com>

---
 mm/slab.c |   34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

Index: linux-2.6.33-rc3-ak/mm/slab.c
===================================================================
--- linux-2.6.33-rc3-ak.orig/mm/slab.c
+++ linux-2.6.33-rc3-ak/mm/slab.c
@@ -1171,19 +1171,9 @@ free_array_cache:
 	}
 }
 
-static int __cpuinit cpuup_prepare(long cpu)
+static int slab_node_prepare(int node)
 {
 	struct kmem_cache *cachep;
-	struct kmem_list3 *l3 = NULL;
-	int node = cpu_to_node(cpu);
-	const int memsize = sizeof(struct kmem_list3);
-
-	/*
-	 * We need to do this right in the beginning since
-	 * alloc_arraycache's are going to use this list.
-	 * 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) {
 		/*
@@ -1192,9 +1182,10 @@ static int __cpuinit cpuup_prepare(long
 		 * node has not already allocated this
 		 */
 		if (!cachep->nodelists[node]) {
-			l3 = kmalloc_node(memsize, GFP_KERNEL, node);
+			struct kmem_list3 *l3;
+			l3 = kmalloc_node(sizeof(struct kmem_list3), GFP_KERNEL, node);
 			if (!l3)
-				goto bad;
+				return -1;
 			kmem_list3_init(l3);
 			l3->next_reap = jiffies + REAPTIMEOUT_LIST3 +
 			    ((unsigned long)cachep) % REAPTIMEOUT_LIST3;
@@ -1213,6 +1204,23 @@ static int __cpuinit cpuup_prepare(long
 			cachep->batchcount + cachep->num;
 		spin_unlock_irq(&cachep->nodelists[node]->list_lock);
 	}
+	return 0;
+}
+
+static int __cpuinit cpuup_prepare(long cpu)
+{
+	struct kmem_cache *cachep;
+	struct kmem_list3 *l3 = NULL;
+	int node = cpu_to_node(cpu);
+
+	/*
+	 * We need to do this right in the beginning since
+	 * alloc_arraycache's are going to use this list.
+	 * kmalloc_node allows us to add the slab to the right
+	 * kmem_list3 and not this cpu's kmem_list3
+	 */
+	if (slab_node_prepare(node) < 0)
+		goto bad;
 
 	/*
 	 * Now we can go ahead with allocating the shared arrays and

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

[PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

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

Signed-off-by: Andi Kleen <ak@linux.intel.com>

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

Index: linux-2.6.33-rc3-ak/mm/slab.c
===================================================================
--- linux-2.6.33-rc3-ak.orig/mm/slab.c
+++ linux-2.6.33-rc3-ak/mm/slab.c
@@ -4112,6 +4112,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);
 

[PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

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

Signed-off-by: Andi Kleen <ak@linux.intel.com>

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

Index: linux-2.6.33-rc3-ak/mm/slab.c
===================================================================
--- linux-2.6.33-rc3-ak.orig/mm/slab.c
+++ linux-2.6.33-rc3-ak/mm/slab.c
@@ -4112,6 +4112,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 majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] [0/4] SLAB: Fix a couple of slab memory hotadd issues

[Pekka Enberg]

Hi Andi,

On Wed, Feb 3, 2010 at 11:39 PM, Andi Kleen <andi@firstfloor.org> wrote:
> This fixes various problems in slab found during memory hotadd testing.
>
> All straight forward bug fixes, so could be still .33 candidates.

AFAICT, they aren't regression fixes so they should wait for .34, no?
The patches look good to me. Nick, Christoph, you might want to take a
peek at them before I shove them in linux-next.

                        Pekka

Re: [PATCH] [0/4] SLAB: Fix a couple of slab memory hotadd issues

[Pekka Enberg]

Hi Andi,

On Wed, Feb 3, 2010 at 11:39 PM, Andi Kleen <andi@firstfloor.org> wrote:
> This fixes various problems in slab found during memory hotadd testing.
>
> All straight forward bug fixes, so could be still .33 candidates.

AFAICT, they aren't regression fixes so they should wait for .34, no?
The patches look good to me. Nick, Christoph, you might want to take a
peek at them before I shove them in linux-next.

                        Pekka

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

Re: [PATCH] [2/4] SLAB: Set up the l3 lists for the memory of freshly added memory

[Christoph Lameter]

On Wed, 3 Feb 2010, Andi Kleen wrote:

> Requires previous refactor patch.

Not in this series?

> +	if (action == MEM_ONLINE && mn->status_change_nid >= 0)
> +		slab_node_prepare(mn->status_change_nid);
> +	return NOTIFY_OK;

Never seen a slab_node_prepare function before.

Re: [PATCH] [2/4] SLAB: Set up the l3 lists for the memory of freshly added memory

[Christoph Lameter]

On Wed, 3 Feb 2010, Andi Kleen wrote:

> Requires previous refactor patch.

Not in this series?

> +	if (action == MEM_ONLINE && mn->status_change_nid >= 0)
> +		slab_node_prepare(mn->status_change_nid);
> +	return NOTIFY_OK;

Never seen a slab_node_prepare function before.

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

Re: [PATCH] [3/4] SLAB: Separate node initialization into separate function

[Christoph Lameter]

Out of sequence. Last patch already uses a function introduced by this
one.

Acked-by: Christoph Lameter <cl@linux-foundation.org>

Re: [PATCH] [3/4] SLAB: Separate node initialization into separate function

[Christoph Lameter]

Out of sequence. Last patch already uses a function introduced by this
one.

Acked-by: Christoph Lameter <cl@linux-foundation.org>


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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

Acked-by: Christoph Lameter <cl@linux-foundation.org>

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

Acked-by: Christoph Lameter <cl@linux-foundation.org>

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

Re: [PATCH] [0/4] SLAB: Fix a couple of slab memory hotadd issues

[Christoph Lameter]

On Wed, 3 Feb 2010, Andi Kleen wrote:

> This fixes various problems in slab found during memory hotadd testing.

It changes the bootstrap semantics. The requirement was so far that slab
initialization must be complete before slab operations can be used.

This patchset allows such use before bootstrap on a node is complete and
also allows the running of cache reaper before bootstrap is done.

I have a bad feeling that this could be the result of Pekka's changes to
the bootstrap.

Re: [PATCH] [0/4] SLAB: Fix a couple of slab memory hotadd issues

[Christoph Lameter]

On Wed, 3 Feb 2010, Andi Kleen wrote:

> This fixes various problems in slab found during memory hotadd testing.

It changes the bootstrap semantics. The requirement was so far that slab
initialization must be complete before slab operations can be used.

This patchset allows such use before bootstrap on a node is complete and
also allows the running of cache reaper before bootstrap is done.

I have a bad feeling that this could be the result of Pekka's changes to
the bootstrap.


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

Re: [PATCH] [0/4] SLAB: Fix a couple of slab memory hotadd issues

[Andi Kleen]

Christoph Lameter <cl@linux-foundation.org> writes:

> On Wed, 3 Feb 2010, Andi Kleen wrote:
>
>> This fixes various problems in slab found during memory hotadd testing.
>
> It changes the bootstrap semantics. The requirement was so far that slab
> initialization must be complete before slab operations can be used.

The problem is that slab itself uses slab it initialize itself.

> This patchset allows such use before bootstrap on a node is complete and
> also allows the running of cache reaper before bootstrap is done.
>
> I have a bad feeling that this could be the result of Pekka's changes to
> the bootstrap.

Not sure I fully follow what you're saying.

Are you saying this is a regression fix after all?

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [0/4] SLAB: Fix a couple of slab memory hotadd issues

[Andi Kleen]

Christoph Lameter <cl@linux-foundation.org> writes:

> On Wed, 3 Feb 2010, Andi Kleen wrote:
>
>> This fixes various problems in slab found during memory hotadd testing.
>
> It changes the bootstrap semantics. The requirement was so far that slab
> initialization must be complete before slab operations can be used.

The problem is that slab itself uses slab it initialize itself.

> This patchset allows such use before bootstrap on a node is complete and
> also allows the running of cache reaper before bootstrap is done.
>
> I have a bad feeling that this could be the result of Pekka's changes to
> the bootstrap.

Not sure I fully follow what you're saying.

Are you saying this is a regression fix after all?

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [0/4] SLAB: Fix a couple of slab memory hotadd issues

[Christoph Lameter]

On Fri, 5 Feb 2010, Andi Kleen wrote:

> > It changes the bootstrap semantics. The requirement was so far that slab
> > initialization must be complete before slab operations can be used.
>
> The problem is that slab itself uses slab it initialize itself.

slab uses itself and also the page allocator to bootstrap itself. The
sequence was always a bit fragile. The page allocator also needs to use
the slab allocator in turn to bootstrap itself.

> > This patchset allows such use before bootstrap on a node is complete and
> > also allows the running of cache reaper before bootstrap is done.
> >
> > I have a bad feeling that this could be the result of Pekka's changes to
> > the bootstrap.
>
> Not sure I fully follow what you're saying.
>
> Are you saying this is a regression fix after all?

I am saying that we may have more trouble lurking here.

If we fix it this way then the bootstrap of a node is different from
system bootstrap that so far does not need these special measures.

Re: [PATCH] [0/4] SLAB: Fix a couple of slab memory hotadd issues

[Christoph Lameter]

On Fri, 5 Feb 2010, Andi Kleen wrote:

> > It changes the bootstrap semantics. The requirement was so far that slab
> > initialization must be complete before slab operations can be used.
>
> The problem is that slab itself uses slab it initialize itself.

slab uses itself and also the page allocator to bootstrap itself. The
sequence was always a bit fragile. The page allocator also needs to use
the slab allocator in turn to bootstrap itself.

> > This patchset allows such use before bootstrap on a node is complete and
> > also allows the running of cache reaper before bootstrap is done.
> >
> > I have a bad feeling that this could be the result of Pekka's changes to
> > the bootstrap.
>
> Not sure I fully follow what you're saying.
>
> Are you saying this is a regression fix after all?

I am saying that we may have more trouble lurking here.

If we fix it this way then the bootstrap of a node is different from
system bootstrap that so far does not need these special measures.

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

Re: [PATCH] [1/4] SLAB: Handle node-not-up case in fallback_alloc()

[David Rientjes]

On Wed, 3 Feb 2010, Andi Kleen wrote:

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

That other node must be allowed by current's cpuset, otherwise 
kmem_getpages() will fail when get_page_from_freelist() iterates only over 
unallowed nodes.

> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> 
> ---
>  mm/slab.c |   19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.33-rc3-ak/mm/slab.c
> ===================================================================
> --- linux-2.6.33-rc3-ak.orig/mm/slab.c
> +++ linux-2.6.33-rc3-ak/mm/slab.c
> @@ -3210,7 +3210,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])
> +					break;

If you set a bit in a nodemask_t everytime ____cache_alloc_node() fails in 
the previous for_each_zone_zonelist() iteration, you could just iterate 
that nodemask here without duplicating the zone_to_nid() and 
cache->nodelists[nid] != NULL check.

	nid = numa_node_id();
	if (!cache->nodelists[nid])
		for_each_node_mask(nid, allowed_nodes) {
			obj = kmem_getpages(cache, local_flags, nid);
			if (obj)
				break;
		}
	else
		obj = kmem_getpages(cache, local_flags, nid);

This way you can try all allowed nodes for memory instead of just one when 
cache->nodelists[numa_node_id()] == NULL.

> +			}
> +			if (!cache->nodelists[nid])
> +				return NULL;
> +		}
> +
> +
> +		obj = kmem_getpages(cache, local_flags, nid);
>  		if (local_flags & __GFP_WAIT)
>  			local_irq_disable();
>  		if (obj) {

Re: [PATCH] [1/4] SLAB: Handle node-not-up case in fallback_alloc()

[David Rientjes]

On Wed, 3 Feb 2010, Andi Kleen wrote:

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

That other node must be allowed by current's cpuset, otherwise 
kmem_getpages() will fail when get_page_from_freelist() iterates only over 
unallowed nodes.

> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> 
> ---
>  mm/slab.c |   19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.33-rc3-ak/mm/slab.c
> ===================================================================
> --- linux-2.6.33-rc3-ak.orig/mm/slab.c
> +++ linux-2.6.33-rc3-ak/mm/slab.c
> @@ -3210,7 +3210,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])
> +					break;

If you set a bit in a nodemask_t everytime ____cache_alloc_node() fails in 
the previous for_each_zone_zonelist() iteration, you could just iterate 
that nodemask here without duplicating the zone_to_nid() and 
cache->nodelists[nid] != NULL check.

	nid = numa_node_id();
	if (!cache->nodelists[nid])
		for_each_node_mask(nid, allowed_nodes) {
			obj = kmem_getpages(cache, local_flags, nid);
			if (obj)
				break;
		}
	else
		obj = kmem_getpages(cache, local_flags, nid);

This way you can try all allowed nodes for memory instead of just one when 
cache->nodelists[numa_node_id()] == NULL.

> +			}
> +			if (!cache->nodelists[nid])
> +				return NULL;
> +		}
> +
> +
> +		obj = kmem_getpages(cache, local_flags, nid);
>  		if (local_flags & __GFP_WAIT)
>  			local_irq_disable();
>  		if (obj) {

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

Re: [PATCH] [2/4] SLAB: Set up the l3 lists for the memory of freshly added memory

[David Rientjes]

On Wed, 3 Feb 2010, Andi Kleen wrote:

> Index: linux-2.6.33-rc3-ak/mm/slab.c
> ===================================================================
> --- linux-2.6.33-rc3-ak.orig/mm/slab.c
> +++ linux-2.6.33-rc3-ak/mm/slab.c
> @@ -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>
> @@ -1560,6 +1561,20 @@ 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;

No cast necessary.

> +
> +	/*
> +	 * When a node goes online allocate l3s early.	 This way
> +	 * kmalloc_node() works for it.
> +	 */
> +	if (action == MEM_ONLINE && mn->status_change_nid >= 0)
> +		slab_node_prepare(mn->status_change_nid);
> +	return NOTIFY_OK;
> +}
> +
>  void __init kmem_cache_init_late(void)
>  {
>  	struct kmem_cache *cachep;
> @@ -1583,6 +1598,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.

> +
>  	/*
>  	 * The reap timers are started later, with a module init call: That part
>  	 * of the kernel is not yet operational.

Re: [PATCH] [2/4] SLAB: Set up the l3 lists for the memory of freshly added memory

[David Rientjes]

On Wed, 3 Feb 2010, Andi Kleen wrote:

> Index: linux-2.6.33-rc3-ak/mm/slab.c
> ===================================================================
> --- linux-2.6.33-rc3-ak.orig/mm/slab.c
> +++ linux-2.6.33-rc3-ak/mm/slab.c
> @@ -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>
> @@ -1560,6 +1561,20 @@ 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;

No cast necessary.

> +
> +	/*
> +	 * When a node goes online allocate l3s early.	 This way
> +	 * kmalloc_node() works for it.
> +	 */
> +	if (action == MEM_ONLINE && mn->status_change_nid >= 0)
> +		slab_node_prepare(mn->status_change_nid);
> +	return NOTIFY_OK;
> +}
> +
>  void __init kmem_cache_init_late(void)
>  {
>  	struct kmem_cache *cachep;
> @@ -1583,6 +1598,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.

> +
>  	/*
>  	 * The reap timers are started later, with a module init call: That part
>  	 * of the kernel is not yet operational.

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

Re: [PATCH] [3/4] SLAB: Separate node initialization into separate function

[David Rientjes]

On Wed, 3 Feb 2010, Andi Kleen wrote:

> 
> No functional changes.
> 
> Needed for next patch.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> 
> ---
>  mm/slab.c |   34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> Index: linux-2.6.33-rc3-ak/mm/slab.c
> ===================================================================
> --- linux-2.6.33-rc3-ak.orig/mm/slab.c
> +++ linux-2.6.33-rc3-ak/mm/slab.c
> @@ -1171,19 +1171,9 @@ free_array_cache:
>  	}
>  }
>  
> -static int __cpuinit cpuup_prepare(long cpu)
> +static int slab_node_prepare(int node)
>  {
>  	struct kmem_cache *cachep;
> -	struct kmem_list3 *l3 = NULL;
> -	int node = cpu_to_node(cpu);
> -	const int memsize = sizeof(struct kmem_list3);
> -
> -	/*
> -	 * We need to do this right in the beginning since
> -	 * alloc_arraycache's are going to use this list.
> -	 * 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) {
>  		/*

As Christoph mentioned, this patch is out of order with the previous one 
in the series; slab_node_prepare() is called in that previous patch by a 
memory hotplug callback without holding cache_chain_mutex (it's taken by 
the cpu hotplug callback prior to calling cpuup_prepare() currently).  So 
slab_node_prepare() should note that we require the mutex and the memory 
hotplug callback should take it in the previous patch.

> @@ -1192,9 +1182,10 @@ static int __cpuinit cpuup_prepare(long
>  		 * node has not already allocated this
>  		 */
>  		if (!cachep->nodelists[node]) {
> -			l3 = kmalloc_node(memsize, GFP_KERNEL, node);
> +			struct kmem_list3 *l3;
> +			l3 = kmalloc_node(sizeof(struct kmem_list3), GFP_KERNEL, node);
>  			if (!l3)
> -				goto bad;
> +				return -1;
>  			kmem_list3_init(l3);
>  			l3->next_reap = jiffies + REAPTIMEOUT_LIST3 +
>  			    ((unsigned long)cachep) % REAPTIMEOUT_LIST3;
> @@ -1213,6 +1204,23 @@ static int __cpuinit cpuup_prepare(long
>  			cachep->batchcount + cachep->num;
>  		spin_unlock_irq(&cachep->nodelists[node]->list_lock);
>  	}
> +	return 0;
> +}
> +
> +static int __cpuinit cpuup_prepare(long cpu)
> +{
> +	struct kmem_cache *cachep;
> +	struct kmem_list3 *l3 = NULL;
> +	int node = cpu_to_node(cpu);
> +
> +	/*
> +	 * We need to do this right in the beginning since
> +	 * alloc_arraycache's are going to use this list.
> +	 * kmalloc_node allows us to add the slab to the right
> +	 * kmem_list3 and not this cpu's kmem_list3
> +	 */
> +	if (slab_node_prepare(node) < 0)
> +		goto bad;
>  
>  	/*
>  	 * Now we can go ahead with allocating the shared arrays and

Re: [PATCH] [3/4] SLAB: Separate node initialization into separate function

[David Rientjes]

On Wed, 3 Feb 2010, Andi Kleen wrote:

> 
> No functional changes.
> 
> Needed for next patch.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> 
> ---
>  mm/slab.c |   34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> Index: linux-2.6.33-rc3-ak/mm/slab.c
> ===================================================================
> --- linux-2.6.33-rc3-ak.orig/mm/slab.c
> +++ linux-2.6.33-rc3-ak/mm/slab.c
> @@ -1171,19 +1171,9 @@ free_array_cache:
>  	}
>  }
>  
> -static int __cpuinit cpuup_prepare(long cpu)
> +static int slab_node_prepare(int node)
>  {
>  	struct kmem_cache *cachep;
> -	struct kmem_list3 *l3 = NULL;
> -	int node = cpu_to_node(cpu);
> -	const int memsize = sizeof(struct kmem_list3);
> -
> -	/*
> -	 * We need to do this right in the beginning since
> -	 * alloc_arraycache's are going to use this list.
> -	 * 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) {
>  		/*

As Christoph mentioned, this patch is out of order with the previous one 
in the series; slab_node_prepare() is called in that previous patch by a 
memory hotplug callback without holding cache_chain_mutex (it's taken by 
the cpu hotplug callback prior to calling cpuup_prepare() currently).  So 
slab_node_prepare() should note that we require the mutex and the memory 
hotplug callback should take it in the previous patch.

> @@ -1192,9 +1182,10 @@ static int __cpuinit cpuup_prepare(long
>  		 * node has not already allocated this
>  		 */
>  		if (!cachep->nodelists[node]) {
> -			l3 = kmalloc_node(memsize, GFP_KERNEL, node);
> +			struct kmem_list3 *l3;
> +			l3 = kmalloc_node(sizeof(struct kmem_list3), GFP_KERNEL, node);
>  			if (!l3)
> -				goto bad;
> +				return -1;
>  			kmem_list3_init(l3);
>  			l3->next_reap = jiffies + REAPTIMEOUT_LIST3 +
>  			    ((unsigned long)cachep) % REAPTIMEOUT_LIST3;
> @@ -1213,6 +1204,23 @@ static int __cpuinit cpuup_prepare(long
>  			cachep->batchcount + cachep->num;
>  		spin_unlock_irq(&cachep->nodelists[node]->list_lock);
>  	}
> +	return 0;
> +}
> +
> +static int __cpuinit cpuup_prepare(long cpu)
> +{
> +	struct kmem_cache *cachep;
> +	struct kmem_list3 *l3 = NULL;
> +	int node = cpu_to_node(cpu);
> +
> +	/*
> +	 * We need to do this right in the beginning since
> +	 * alloc_arraycache's are going to use this list.
> +	 * kmalloc_node allows us to add the slab to the right
> +	 * kmem_list3 and not this cpu's kmem_list3
> +	 */
> +	if (slab_node_prepare(node) < 0)
> +		goto bad;
>  
>  	/*
>  	 * Now we can go ahead with allocating the shared arrays and

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

Re: [PATCH] [1/4] SLAB: Handle node-not-up case in fallback_alloc()

[Andi Kleen]

On Fri, Feb 05, 2010 at 01:06:56PM -0800, David Rientjes wrote:
> On Wed, 3 Feb 2010, Andi Kleen wrote:
> 
> > When fallback_alloc() runs the node of the CPU might not be initialized yet.
> > Handle this case by allocating in another node.
> > 
> 
> That other node must be allowed by current's cpuset, otherwise 
> kmem_getpages() will fail when get_page_from_freelist() iterates only over 
> unallowed nodes.

All theses cases are really only interesting in the memory hotplug path
itself (afterwards the slab is working anyways and memory is there)
and if someone sets funny cpusets for those he gets what he deserves ...

-Andi

Re: [PATCH] [1/4] SLAB: Handle node-not-up case in fallback_alloc()

[Andi Kleen]

On Fri, Feb 05, 2010 at 01:06:56PM -0800, David Rientjes wrote:
> On Wed, 3 Feb 2010, Andi Kleen wrote:
> 
> > When fallback_alloc() runs the node of the CPU might not be initialized yet.
> > Handle this case by allocating in another node.
> > 
> 
> That other node must be allowed by current's cpuset, otherwise 
> kmem_getpages() will fail when get_page_from_freelist() iterates only over 
> unallowed nodes.

All theses cases are really only interesting in the memory hotplug path
itself (afterwards the slab is working anyways and memory is there)
and if someone sets funny cpusets for those he gets what he deserves ...

-Andi

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

Re: [PATCH] [2/4] SLAB: Set up the l3 lists for the memory of freshly added memory

[Andi Kleen]

On Fri, Feb 05, 2010 at 01:17:56PM -0800, David Rientjes wrote:
> > +static int slab_memory_callback(struct notifier_block *self,
> > +				unsigned long action, void *arg)
> > +{
> > +	struct memory_notify *mn = (struct memory_notify *)arg;
> 
> No cast necessary.

It's standard practice to cast void *.

> >  void __init kmem_cache_init_late(void)
> >  {
> >  	struct kmem_cache *cachep;
> > @@ -1583,6 +1598,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.

Ok.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [2/4] SLAB: Set up the l3 lists for the memory of freshly added memory

[Andi Kleen]

On Fri, Feb 05, 2010 at 01:17:56PM -0800, David Rientjes wrote:
> > +static int slab_memory_callback(struct notifier_block *self,
> > +				unsigned long action, void *arg)
> > +{
> > +	struct memory_notify *mn = (struct memory_notify *)arg;
> 
> No cast necessary.

It's standard practice to cast void *.

> >  void __init kmem_cache_init_late(void)
> >  {
> >  	struct kmem_cache *cachep;
> > @@ -1583,6 +1598,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.

Ok.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [3/4] SLAB: Separate node initialization into separate function

[Andi Kleen]

> As Christoph mentioned, this patch is out of order with the previous one 

Ok.

> in the series; slab_node_prepare() is called in that previous patch by a 
> memory hotplug callback without holding cache_chain_mutex (it's taken by 
> the cpu hotplug callback prior to calling cpuup_prepare() currently).  So 
> slab_node_prepare() should note that we require the mutex and the memory 
> hotplug callback should take it in the previous patch.

AFAIK the code is correct. If you feel the need for additional
documentation feel free to send patches yourself.

-Andi

Re: [PATCH] [3/4] SLAB: Separate node initialization into separate function

[Andi Kleen]

> As Christoph mentioned, this patch is out of order with the previous one 

Ok.

> in the series; slab_node_prepare() is called in that previous patch by a 
> memory hotplug callback without holding cache_chain_mutex (it's taken by 
> the cpu hotplug callback prior to calling cpuup_prepare() currently).  So 
> slab_node_prepare() should note that we require the mutex and the memory 
> hotplug callback should take it in the previous patch.

AFAIK the code is correct. If you feel the need for additional
documentation feel free to send patches yourself.

-Andi

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

Re: [PATCH] [2/4] SLAB: Set up the l3 lists for the memory of freshly added memory

[David Rientjes]

On Sat, 6 Feb 2010, Andi Kleen wrote:

> > > +static int slab_memory_callback(struct notifier_block *self,
> > > +				unsigned long action, void *arg)
> > > +{
> > > +	struct memory_notify *mn = (struct memory_notify *)arg;
> > 
> > No cast necessary.
> 
> It's standard practice to cast void *.
> 

$ grep -r "struct memory_notify.*=" *
arch/powerpc/platforms/pseries/cmm.c:	struct memory_notify *marg = arg;
drivers/base/node.c:	struct memory_notify *mnb = arg;
drivers/net/ehea/ehea_main.c:	struct memory_notify *arg = data;
mm/ksm.c:	struct memory_notify *mn = arg;
mm/slub.c:	struct memory_notify *marg = arg;
mm/slub.c:	struct memory_notify *marg = arg;
mm/page_cgroup.c:	struct memory_notify *mn = arg;

Re: [PATCH] [2/4] SLAB: Set up the l3 lists for the memory of freshly added memory

[David Rientjes]

On Sat, 6 Feb 2010, Andi Kleen wrote:

> > > +static int slab_memory_callback(struct notifier_block *self,
> > > +				unsigned long action, void *arg)
> > > +{
> > > +	struct memory_notify *mn = (struct memory_notify *)arg;
> > 
> > No cast necessary.
> 
> It's standard practice to cast void *.
> 

$ grep -r "struct memory_notify.*=" *
arch/powerpc/platforms/pseries/cmm.c:	struct memory_notify *marg = arg;
drivers/base/node.c:	struct memory_notify *mnb = arg;
drivers/net/ehea/ehea_main.c:	struct memory_notify *arg = data;
mm/ksm.c:	struct memory_notify *mn = arg;
mm/slub.c:	struct memory_notify *marg = arg;
mm/slub.c:	struct memory_notify *marg = arg;
mm/page_cgroup.c:	struct memory_notify *mn = arg;

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

Re: [PATCH] [1/4] SLAB: Handle node-not-up case in fallback_alloc()

[David Rientjes]

On Sat, 6 Feb 2010, Andi Kleen wrote:

> > That other node must be allowed by current's cpuset, otherwise 
> > kmem_getpages() will fail when get_page_from_freelist() iterates only over 
> > unallowed nodes.
> 
> All theses cases are really only interesting in the memory hotplug path
> itself (afterwards the slab is working anyways and memory is there)
> and if someone sets funny cpusets for those he gets what he deserves ...
> 

If a hot-added node has not been initialized for the cache, your code is 
picking an existing one in zonelist order which may be excluded by 
current's cpuset.  Thus, your code has a very real chance of having 
kmem_getpages() return NULL because get_page_from_freelist() will reject 
non-atomic ALLOC_CPUSET allocations for prohibited nodes.  That isn't a 
scenario that requires a "funny cpuset," it just has to not allow whatever 
initialized node comes first in the zonelist.

My suggested alternative does not pick a single initialized node, rather 
it tries all nodes that actually have a chance of having kmem_getpages() 
succeed which increases the probability that your patch actually has an 
effect for cpuset users.

Re: [PATCH] [1/4] SLAB: Handle node-not-up case in fallback_alloc()

[David Rientjes]

On Sat, 6 Feb 2010, Andi Kleen wrote:

> > That other node must be allowed by current's cpuset, otherwise 
> > kmem_getpages() will fail when get_page_from_freelist() iterates only over 
> > unallowed nodes.
> 
> All theses cases are really only interesting in the memory hotplug path
> itself (afterwards the slab is working anyways and memory is there)
> and if someone sets funny cpusets for those he gets what he deserves ...
> 

If a hot-added node has not been initialized for the cache, your code is 
picking an existing one in zonelist order which may be excluded by 
current's cpuset.  Thus, your code has a very real chance of having 
kmem_getpages() return NULL because get_page_from_freelist() will reject 
non-atomic ALLOC_CPUSET allocations for prohibited nodes.  That isn't a 
scenario that requires a "funny cpuset," it just has to not allow whatever 
initialized node comes first in the zonelist.

My suggested alternative does not pick a single initialized node, rather 
it tries all nodes that actually have a chance of having kmem_getpages() 
succeed which increases the probability that your patch actually has an 
effect for cpuset users.

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

Re: [PATCH] [3/4] SLAB: Separate node initialization into separate function

[David Rientjes]

On Sat, 6 Feb 2010, Andi Kleen wrote:

> > in the series; slab_node_prepare() is called in that previous patch by a 
> > memory hotplug callback without holding cache_chain_mutex (it's taken by 
> > the cpu hotplug callback prior to calling cpuup_prepare() currently).  So 
> > slab_node_prepare() should note that we require the mutex and the memory 
> > hotplug callback should take it in the previous patch.
> 
> AFAIK the code is correct. If you feel the need for additional
> documentation feel free to send patches yourself.
> 

Documentation?  You're required to take cache_chain_mutex before calling 
slab_node_prepare() in your memory hotplug notifier, it iterates 
cache_chain.  Please look again.

Re: [PATCH] [3/4] SLAB: Separate node initialization into separate function

[David Rientjes]

On Sat, 6 Feb 2010, Andi Kleen wrote:

> > in the series; slab_node_prepare() is called in that previous patch by a 
> > memory hotplug callback without holding cache_chain_mutex (it's taken by 
> > the cpu hotplug callback prior to calling cpuup_prepare() currently).  So 
> > slab_node_prepare() should note that we require the mutex and the memory 
> > hotplug callback should take it in the previous patch.
> 
> AFAIK the code is correct. If you feel the need for additional
> documentation feel free to send patches yourself.
> 

Documentation?  You're required to take cache_chain_mutex before calling 
slab_node_prepare() in your memory hotplug notifier, it iterates 
cache_chain.  Please look again.

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

Re: [PATCH] [1/4] SLAB: Handle node-not-up case in fallback_alloc()

[Andi Kleen]

> If a hot-added node has not been initialized for the cache, your code is 
> picking an existing one in zonelist order which may be excluded by 
> current's cpuset.  Thus, your code has a very real chance of having 
> kmem_getpages() return NULL because get_page_from_freelist() will reject 
> non-atomic ALLOC_CPUSET allocations for prohibited nodes.  That isn't a 
> scenario that requires a "funny cpuset," it just has to not allow whatever 
> initialized node comes first in the zonelist.

The point was that you would need to run whoever triggers the memory
hotadd in a cpuset with limitations. That would be a clear
don't do that if hurts(tm)
 
> My suggested alternative does not pick a single initialized node, rather 
> it tries all nodes that actually have a chance of having kmem_getpages() 
> succeed which increases the probability that your patch actually has an 
> effect for cpuset users.

cpuset users are unlikely to trigger memory hotadds from inside limiting
cpusets. Typically that's done from udev etc.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [1/4] SLAB: Handle node-not-up case in fallback_alloc()

[Andi Kleen]

> If a hot-added node has not been initialized for the cache, your code is 
> picking an existing one in zonelist order which may be excluded by 
> current's cpuset.  Thus, your code has a very real chance of having 
> kmem_getpages() return NULL because get_page_from_freelist() will reject 
> non-atomic ALLOC_CPUSET allocations for prohibited nodes.  That isn't a 
> scenario that requires a "funny cpuset," it just has to not allow whatever 
> initialized node comes first in the zonelist.

The point was that you would need to run whoever triggers the memory
hotadd in a cpuset with limitations. That would be a clear
don't do that if hurts(tm)
 
> My suggested alternative does not pick a single initialized node, rather 
> it tries all nodes that actually have a chance of having kmem_getpages() 
> succeed which increases the probability that your patch actually has an 
> effect for cpuset users.

cpuset users are unlikely to trigger memory hotadds from inside limiting
cpusets. Typically that's done from udev etc.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [1/4] SLAB: Handle node-not-up case in fallback_alloc()

[David Rientjes]

On Sat, 6 Feb 2010, Andi Kleen wrote:

> > If a hot-added node has not been initialized for the cache, your code is 
> > picking an existing one in zonelist order which may be excluded by 
> > current's cpuset.  Thus, your code has a very real chance of having 
> > kmem_getpages() return NULL because get_page_from_freelist() will reject 
> > non-atomic ALLOC_CPUSET allocations for prohibited nodes.  That isn't a 
> > scenario that requires a "funny cpuset," it just has to not allow whatever 
> > initialized node comes first in the zonelist.
> 
> The point was that you would need to run whoever triggers the memory
> hotadd in a cpuset with limitations. That would be a clear
> don't do that if hurts(tm)
>  

With a subset of memory nodes, yes.  What else prohibits that except for 
your new code?  

There's a second issue with this approach that I eluded to above: you're 
picking the first initialized node for the cache based solely on whether 
it is allocated or not.  kmem_getpages() may still return NULL when it 
would return new slab for any other initialized node, so you're better off 
trying them all.

In other words, my earlier (untested) suggestion:

diff --git a/mm/slab.c b/mm/slab.c
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3172,6 +3172,7 @@ static void *fallback_alloc(struct kmem_cache *cache, gfp_t flags)
 	gfp_t local_flags;
 	struct zoneref *z;
 	struct zone *zone;
+	nodemask_t allowed_nodes = NODE_MASK_NONE;
 	enum zone_type high_zoneidx = gfp_zone(flags);
 	void *obj = NULL;
 	int nid;
@@ -3197,6 +3198,7 @@ retry:
 					flags | GFP_THISNODE, nid);
 				if (obj)
 					break;
+				node_set(nid, allowed_nodes);
 		}
 	}
 
@@ -3210,7 +3212,15 @@ retry:
 		if (local_flags & __GFP_WAIT)
 			local_irq_enable();
 		kmem_flagcheck(cache, flags);
-		obj = kmem_getpages(cache, local_flags, numa_node_id());
+		nid = numa_node_id();
+		if (cache->nodelists[nid])
+			obj = kmem_getpages(cache, local_flags, nid);
+		else
+			for_each_node_mask(nid, allowed_nodes) {
+				obj = kmem_getpages(cache, local_flags, nid);
+				if (obj)
+					break;
+			}
 		if (local_flags & __GFP_WAIT)
 			local_irq_disable();
 		if (obj) {

Anyway, I'll leave these otherwise unnecessary limitations to Pekka.  
Thanks.

Re: [PATCH] [1/4] SLAB: Handle node-not-up case in fallback_alloc()

[David Rientjes]

On Sat, 6 Feb 2010, Andi Kleen wrote:

> > If a hot-added node has not been initialized for the cache, your code is 
> > picking an existing one in zonelist order which may be excluded by 
> > current's cpuset.  Thus, your code has a very real chance of having 
> > kmem_getpages() return NULL because get_page_from_freelist() will reject 
> > non-atomic ALLOC_CPUSET allocations for prohibited nodes.  That isn't a 
> > scenario that requires a "funny cpuset," it just has to not allow whatever 
> > initialized node comes first in the zonelist.
> 
> The point was that you would need to run whoever triggers the memory
> hotadd in a cpuset with limitations. That would be a clear
> don't do that if hurts(tm)
>  

With a subset of memory nodes, yes.  What else prohibits that except for 
your new code?  

There's a second issue with this approach that I eluded to above: you're 
picking the first initialized node for the cache based solely on whether 
it is allocated or not.  kmem_getpages() may still return NULL when it 
would return new slab for any other initialized node, so you're better off 
trying them all.

In other words, my earlier (untested) suggestion:

diff --git a/mm/slab.c b/mm/slab.c
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3172,6 +3172,7 @@ static void *fallback_alloc(struct kmem_cache *cache, gfp_t flags)
 	gfp_t local_flags;
 	struct zoneref *z;
 	struct zone *zone;
+	nodemask_t allowed_nodes = NODE_MASK_NONE;
 	enum zone_type high_zoneidx = gfp_zone(flags);
 	void *obj = NULL;
 	int nid;
@@ -3197,6 +3198,7 @@ retry:
 					flags | GFP_THISNODE, nid);
 				if (obj)
 					break;
+				node_set(nid, allowed_nodes);
 		}
 	}
 
@@ -3210,7 +3212,15 @@ retry:
 		if (local_flags & __GFP_WAIT)
 			local_irq_enable();
 		kmem_flagcheck(cache, flags);
-		obj = kmem_getpages(cache, local_flags, numa_node_id());
+		nid = numa_node_id();
+		if (cache->nodelists[nid])
+			obj = kmem_getpages(cache, local_flags, nid);
+		else
+			for_each_node_mask(nid, allowed_nodes) {
+				obj = kmem_getpages(cache, local_flags, nid);
+				if (obj)
+					break;
+			}
 		if (local_flags & __GFP_WAIT)
 			local_irq_disable();
 		if (obj) {

Anyway, I'll leave these otherwise unnecessary limitations to Pekka.  
Thanks.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

> 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 

Yes it seems to be a bike shedding area for some reason (which color
should we paint it today?)

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

IMHO they are all bug fixes and there is no excuse for not merging them ASAP,
independent of any merge windows.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

> 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 

Yes it seems to be a bike shedding area for some reason (which color
should we paint it today?)

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

IMHO they are all bug fixes and there is no excuse for not merging them ASAP,
independent of any merge windows.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[David Rientjes]

On Mon, 1 Mar 2010, KAMEZAWA Hiroyuki wrote:

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

I can only speak for x86 and not the abundance of memory hotplug support 
that exists for powerpc, but cpu hotplug doesn't do _anything_ when a 
memory region that has a corresponding ACPI_SRAT_MEM_HOT_PLUGGABLE entry 
in the SRAT is hotadded and requires a new nodeid.  That can be triggered 
via the acpi layer with plug and play or explicitly from the command line 
via CONFIG_ARCH_MEMORY_PROBE.

Relying on cpu hotplug to set up nodelists in such a circumstance simply 
won't work.  You need memory hotplug support such as in my patch.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[David Rientjes]

On Mon, 1 Mar 2010, KAMEZAWA Hiroyuki wrote:

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

I can only speak for x86 and not the abundance of memory hotplug support 
that exists for powerpc, but cpu hotplug doesn't do _anything_ when a 
memory region that has a corresponding ACPI_SRAT_MEM_HOT_PLUGGABLE entry 
in the SRAT is hotadded and requires a new nodeid.  That can be triggered 
via the acpi layer with plug and play or explicitly from the command line 
via CONFIG_ARCH_MEMORY_PROBE.

Relying on cpu hotplug to set up nodelists in such a circumstance simply 
won't work.  You need memory hotplug support such as in my patch.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[KAMEZAWA Hiroyuki]

On Fri, 26 Feb 2010 18:31:15 +0100
Andi Kleen <andi@firstfloor.org> wrote:

> On Fri, Feb 26, 2010 at 11:24:50AM -0600, Christoph Lameter wrote:
> > On Fri, 26 Feb 2010, Andi Kleen wrote:
> > 
> > > > > 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.
> 
> 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.


Thanks,
-Kame

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[KAMEZAWA Hiroyuki]

On Fri, 26 Feb 2010 18:31:15 +0100
Andi Kleen <andi@firstfloor.org> wrote:

> On Fri, Feb 26, 2010 at 11:24:50AM -0600, Christoph Lameter wrote:
> > On Fri, 26 Feb 2010, Andi Kleen wrote:
> > 
> > > > > 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.
> 
> 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.


Thanks,
-Kame

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[David Rientjes]

On Fri, 26 Feb 2010, Christoph Lameter wrote:

> > 1) numa memory hotadd never worked
> 
> Well Kamesan indicated that this worked if a cpu became online.
> 

That may be true, but it doesn't address hotpluggable 
ACPI_SRAT_MEM_HOT_PLUGGABLE entries for CONFIG_MEMORY_HOTPLUG_SPARSE where 
no cpus are being onlined or writing to /sys/devices/system/memory/probe 
for CONFIG_ARCH_MEMORY_PROBE.

> > 2) the rest just bitrotted because nobody tested it.
> 
> Yep. David: Can you revise the relevant portions of the patchset and
> repost it?
> 

Ok.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[David Rientjes]

On Fri, 26 Feb 2010, Christoph Lameter wrote:

> > 1) numa memory hotadd never worked
> 
> Well Kamesan indicated that this worked if a cpu became online.
> 

That may be true, but it doesn't address hotpluggable 
ACPI_SRAT_MEM_HOT_PLUGGABLE entries for CONFIG_MEMORY_HOTPLUG_SPARSE where 
no cpus are being onlined or writing to /sys/devices/system/memory/probe 
for CONFIG_ARCH_MEMORY_PROBE.

> > 2) the rest just bitrotted because nobody tested it.
> 
> Yep. David: Can you revise the relevant portions of the patchset and
> repost it?
> 

Ok.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Fri, Feb 26, 2010 at 11:24:50AM -0600, Christoph Lameter wrote:
> On Fri, 26 Feb 2010, Andi Kleen wrote:
> 
> > > > 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.

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

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Fri, Feb 26, 2010 at 11:24:50AM -0600, Christoph Lameter wrote:
> On Fri, 26 Feb 2010, Andi Kleen wrote:
> 
> > > > 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.

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

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Fri, 26 Feb 2010, Andi Kleen wrote:

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

> 2) the rest just bitrotted because nobody tested it.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Fri, 26 Feb 2010, Andi Kleen wrote:

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

> 2) the rest just bitrotted because nobody tested it.

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

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Fri, Feb 26, 2010 at 09:05:48AM -0600, Christoph Lameter wrote:
> 
> I mean why the core changes if this is an x86 issue?

The slab bugs are in no way related to x86, other than x86 supporting
memory hotadd & numa.

I only wrote "on x86" because I wasn't sure about the status on the other
platforms.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Fri, Feb 26, 2010 at 09:05:48AM -0600, Christoph Lameter wrote:
> 
> I mean why the core changes if this is an x86 issue?

The slab bugs are in no way related to x86, other than x86 supporting
memory hotadd & numa.

I only wrote "on x86" because I wasn't sure about the status on the other
platforms.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Fri, Feb 26, 2010 at 09:04:56AM -0600, Christoph Lameter wrote:
> On Fri, 26 Feb 2010, Andi Kleen wrote:
> 
> > 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.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Fri, Feb 26, 2010 at 09:04:56AM -0600, Christoph Lameter wrote:
> On Fri, 26 Feb 2010, Andi Kleen wrote:
> 
> > 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.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

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

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Fri, 26 Feb 2010, Andi Kleen wrote:

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Fri, 26 Feb 2010, Andi Kleen wrote:

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

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Fri, Feb 26, 2010 at 02:35:24PM +0200, Pekka Enberg wrote:
> On Fri, Feb 26, 2010 at 1:43 PM, Andi Kleen <andi@firstfloor.org> wrote:
> > On Fri, Feb 26, 2010 at 12:45:02PM +0200, Pekka Enberg wrote:
> >> Christoph Lameter kirjoitti:
> >>>> 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?
> >
> > Well if Christoph wants to submit a better patch that is tested and solves
> > the problems he can do that.
> 
> Sure.
> 
> > 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?

Let's put it like this: i'm sure there a myriad different 
way in all the possible design spaces to change slab to 
make memory hotadd work.

Unless someone gives me a strong reason (e.g. code as submitted
doesn't work or is really unclean) I'm not very motivated to try them
all (also given that slab.c is really legacy code that will
hopefully go away at some point).  

Also there are still other bugs to fix in memory hotadd and I'm focussing
 my efforts on that.

I don't think the patches I submitted are particularly intrusive or 
unclean or broken.

As far as I can see Christoph's proposal was just another way
to do this, but it wasn't clear to me it was better enough
in any way to spend significant time on it.

So yes I would prefer if you merged them as submitted just
to fix the bugs. If someone else comes up with a better way
to do this and submits patches they could still change
to that later.

As for the timer race patch: I cannot make a strong
argument right now that it's needed, on the other hand
a bit of defensive programming also doesn't hurt. But 
if that one is not in I won't cry.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Fri, Feb 26, 2010 at 02:35:24PM +0200, Pekka Enberg wrote:
> On Fri, Feb 26, 2010 at 1:43 PM, Andi Kleen <andi@firstfloor.org> wrote:
> > On Fri, Feb 26, 2010 at 12:45:02PM +0200, Pekka Enberg wrote:
> >> Christoph Lameter kirjoitti:
> >>>> 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?
> >
> > Well if Christoph wants to submit a better patch that is tested and solves
> > the problems he can do that.
> 
> Sure.
> 
> > 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?

Let's put it like this: i'm sure there a myriad different 
way in all the possible design spaces to change slab to 
make memory hotadd work.

Unless someone gives me a strong reason (e.g. code as submitted
doesn't work or is really unclean) I'm not very motivated to try them
all (also given that slab.c is really legacy code that will
hopefully go away at some point).  

Also there are still other bugs to fix in memory hotadd and I'm focussing
 my efforts on that.

I don't think the patches I submitted are particularly intrusive or 
unclean or broken.

As far as I can see Christoph's proposal was just another way
to do this, but it wasn't clear to me it was better enough
in any way to spend significant time on it.

So yes I would prefer if you merged them as submitted just
to fix the bugs. If someone else comes up with a better way
to do this and submits patches they could still change
to that later.

As for the timer race patch: I cannot make a strong
argument right now that it's needed, on the other hand
a bit of defensive programming also doesn't hurt. But 
if that one is not in I won't cry.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Pekka Enberg]

On Fri, Feb 26, 2010 at 1:43 PM, Andi Kleen <andi@firstfloor.org> wrote:
> On Fri, Feb 26, 2010 at 12:45:02PM +0200, Pekka Enberg wrote:
>> Christoph Lameter kirjoitti:
>>>> 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?
>
> Well if Christoph wants to submit a better patch that is tested and solves
> the problems he can do that.

Sure.

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

                         Pekka

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Pekka Enberg]

On Fri, Feb 26, 2010 at 1:43 PM, Andi Kleen <andi@firstfloor.org> wrote:
> On Fri, Feb 26, 2010 at 12:45:02PM +0200, Pekka Enberg wrote:
>> Christoph Lameter kirjoitti:
>>>> 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?
>
> Well if Christoph wants to submit a better patch that is tested and solves
> the problems he can do that.

Sure.

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

                         Pekka

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Fri, Feb 26, 2010 at 12:45:02PM +0200, Pekka Enberg wrote:
> Christoph Lameter kirjoitti:
>>> 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?

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.

-Andi


-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Fri, Feb 26, 2010 at 12:45:02PM +0200, Pekka Enberg wrote:
> Christoph Lameter kirjoitti:
>>> 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?

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.

-Andi


-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Thu, Feb 25, 2010 at 12:30:26PM -0600, Christoph Lameter wrote:
> On Thu, 25 Feb 2010, David Rientjes wrote:
> 
> > 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 problem is that slab already allocates inside the notifier
and then some state wasn't set up.

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

-Andi


-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Thu, Feb 25, 2010 at 12:30:26PM -0600, Christoph Lameter wrote:
> On Thu, 25 Feb 2010, David Rientjes wrote:
> 
> > 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 problem is that slab already allocates inside the notifier
and then some state wasn't set up.

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

-Andi


-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Pekka Enberg]

Christoph Lameter kirjoitti:
>> 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?

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Pekka Enberg]

Christoph Lameter kirjoitti:
>> 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?

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[KAMEZAWA Hiroyuki]

On Thu, 25 Feb 2010 12:30:26 -0600 (CST)
Christoph Lameter <cl@linux-foundation.org> wrote:

> On Thu, 25 Feb 2010, David Rientjes wrote:
> 
> > 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?
> 
In this code,

 int node = numa_node_id();

node is got by its CPU.

At node hotplug, following order should be kept.
	Add:   memory -> cpu
	Remove: cpu -> memory

cpus must be onlined after memory. At least, we online cpus only after
memory. Then, we(our heavy test on RHEL5) never see this kind of race.


I'm sorry if my answer misses your point.

Thanks,
-Kame
 

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[KAMEZAWA Hiroyuki]

On Thu, 25 Feb 2010 12:30:26 -0600 (CST)
Christoph Lameter <cl@linux-foundation.org> wrote:

> On Thu, 25 Feb 2010, David Rientjes wrote:
> 
> > 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?
> 
In this code,

 int node = numa_node_id();

node is got by its CPU.

At node hotplug, following order should be kept.
	Add:   memory -> cpu
	Remove: cpu -> memory

cpus must be onlined after memory. At least, we online cpus only after
memory. Then, we(our heavy test on RHEL5) never see this kind of race.


I'm sorry if my answer misses your point.

Thanks,
-Kame
 

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

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Thu, 25 Feb 2010, David Rientjes wrote:

> On Thu, 25 Feb 2010, Christoph Lameter wrote:
>
> > > 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.

Yes Andi's work makes it explicit but there is already code in the cpu
notifier (see cpuup_prepare) that seems to have been intended to
initialize the node structures. Wonder why the hotplug people never
addressed that issue? Kame?


      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);
        }


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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Thu, 25 Feb 2010, David Rientjes wrote:

> On Thu, 25 Feb 2010, Christoph Lameter wrote:
>
> > > 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.

Yes Andi's work makes it explicit but there is already code in the cpu
notifier (see cpuup_prepare) that seems to have been intended to
initialize the node structures. Wonder why the hotplug people never
addressed that issue? Kame?


      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);
        }


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


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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[David Rientjes]

On Thu, 25 Feb 2010, Christoph Lameter wrote:

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

Slab isn't concerned about that until the memory is onlined by doing 
echo online > /sys/devices/system/memory/memoryX/state for the new memory 
section.  This is where all the new pages are onlined, kswapd is started 
on the new node, and the zonelists are built.  It's also where the new 
node gets set in N_HIGH_MEMORY and, thus, it's possible to call 
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.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[David Rientjes]

On Thu, 25 Feb 2010, Christoph Lameter wrote:

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

Slab isn't concerned about that until the memory is onlined by doing 
echo online > /sys/devices/system/memory/memoryX/state for the new memory 
section.  This is where all the new pages are onlined, kswapd is started 
on the new node, and the zonelists are built.  It's also where the new 
node gets set in N_HIGH_MEMORY and, thus, it's possible to call 
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.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Thu, 25 Feb 2010, Pekka Enberg wrote:

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

Onlining and offlining memory is not that frequently used.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Thu, 25 Feb 2010, Pekka Enberg wrote:

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

Onlining and offlining memory is not that frequently used.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Pekka Enberg]

Hi Christoph,

Christoph Lameter wrote:
>> 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.

			Pekka

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Pekka Enberg]

Hi Christoph,

Christoph Lameter wrote:
>> 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.

			Pekka

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Thu, 25 Feb 2010, Pekka Enberg wrote:

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

Can we get some of the hotplug authors involved in the discussion?

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Thu, 25 Feb 2010, Pekka Enberg wrote:

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

Can we get some of the hotplug authors involved in the discussion?



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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Thu, 25 Feb 2010, David Rientjes wrote:

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Thu, 25 Feb 2010, David Rientjes wrote:

> 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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[David Rientjes]

On Thu, 25 Feb 2010, Pekka Enberg wrote:

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

That said, I think the first and fourth patch in this series may be 
unnecessary if slab's notifier were to call slab_node_prepare() on 
MEM_GOING_ONLINE instead of MEM_ONLINE.  Otherwise, kswapd is already 
running, the zonelists for the new pgdat have been initialized, and the 
bit has been set in node_states[N_HIGH_MEMORY] without allocated 
cachep->nodelists[node] memory.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[David Rientjes]

On Thu, 25 Feb 2010, Pekka Enberg wrote:

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

That said, I think the first and fourth patch in this series may be 
unnecessary if slab's notifier were to call slab_node_prepare() on 
MEM_GOING_ONLINE instead of MEM_ONLINE.  Otherwise, kswapd is already 
running, the zonelists for the new pgdat have been initialized, and the 
bit has been set in node_states[N_HIGH_MEMORY] without allocated 
cachep->nodelists[node] memory.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Pekka Enberg]

Christoph Lameter wrote:
> On Sat, 20 Feb 2010, Andi Kleen wrote:
> 
>> On Fri, Feb 19, 2010 at 12:22:58PM -0600, Christoph Lameter wrote:
>>> On Mon, 15 Feb 2010, Nick Piggin wrote:
>>>
>>>> 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...

			Pekka

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Pekka Enberg]

Christoph Lameter wrote:
> On Sat, 20 Feb 2010, Andi Kleen wrote:
> 
>> On Fri, Feb 19, 2010 at 12:22:58PM -0600, Christoph Lameter wrote:
>>> On Mon, 15 Feb 2010, Nick Piggin wrote:
>>>
>>>> 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...

			Pekka

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Sat, 20 Feb 2010, Andi Kleen wrote:

> On Fri, Feb 19, 2010 at 12:22:58PM -0600, Christoph Lameter wrote:
> > On Mon, 15 Feb 2010, Nick Piggin wrote:
> >
> > > 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.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Sat, 20 Feb 2010, Andi Kleen wrote:

> On Fri, Feb 19, 2010 at 12:22:58PM -0600, Christoph Lameter wrote:
> > On Mon, 15 Feb 2010, Nick Piggin wrote:
> >
> > > 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.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Mon, Feb 22, 2010 at 06:11:03PM +0200, Pekka Enberg wrote:
> Andi Kleen wrote:
>> On Mon, Feb 22, 2010 at 12:53:27PM +0200, Pekka Enberg wrote:
>>> Andi Kleen kirjoitti:
>>>> On Fri, Feb 19, 2010 at 12:22:58PM -0600, Christoph Lameter wrote:
>>>>> On Mon, 15 Feb 2010, Nick Piggin wrote:
>>>>>
>>>>>> 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 
>>
>> I can't post the oops from a pre-release system.
>>
>>> 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.

Ok it's fine for me to drop that patch for now. I'll try to reproduce
that oops and if I can't then it might be just not needed.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Mon, Feb 22, 2010 at 06:11:03PM +0200, Pekka Enberg wrote:
> Andi Kleen wrote:
>> On Mon, Feb 22, 2010 at 12:53:27PM +0200, Pekka Enberg wrote:
>>> Andi Kleen kirjoitti:
>>>> On Fri, Feb 19, 2010 at 12:22:58PM -0600, Christoph Lameter wrote:
>>>>> On Mon, 15 Feb 2010, Nick Piggin wrote:
>>>>>
>>>>>> 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 
>>
>> I can't post the oops from a pre-release system.
>>
>>> 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.

Ok it's fine for me to drop that patch for now. I'll try to reproduce
that oops and if I can't then it might be just not needed.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Pekka Enberg]

Andi Kleen wrote:
> On Mon, Feb 22, 2010 at 12:53:27PM +0200, Pekka Enberg wrote:
>> Andi Kleen kirjoitti:
>>> On Fri, Feb 19, 2010 at 12:22:58PM -0600, Christoph Lameter wrote:
>>>> On Mon, 15 Feb 2010, Nick Piggin wrote:
>>>>
>>>>> 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 
> 
> I can't post the oops from a pre-release system.
> 
>> 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.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Pekka Enberg]

Andi Kleen wrote:
> On Mon, Feb 22, 2010 at 12:53:27PM +0200, Pekka Enberg wrote:
>> Andi Kleen kirjoitti:
>>> On Fri, Feb 19, 2010 at 12:22:58PM -0600, Christoph Lameter wrote:
>>>> On Mon, 15 Feb 2010, Nick Piggin wrote:
>>>>
>>>>> 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 
> 
> I can't post the oops from a pre-release system.
> 
>> 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.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Mon, Feb 22, 2010 at 12:53:27PM +0200, Pekka Enberg wrote:
> Andi Kleen kirjoitti:
>> On Fri, Feb 19, 2010 at 12:22:58PM -0600, Christoph Lameter wrote:
>>> On Mon, 15 Feb 2010, Nick Piggin wrote:
>>>
>>>> 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 

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

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

As far as I know nobody said the patch is wrong so far, just
that they wanted to have more analysis.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Mon, Feb 22, 2010 at 12:53:27PM +0200, Pekka Enberg wrote:
> Andi Kleen kirjoitti:
>> On Fri, Feb 19, 2010 at 12:22:58PM -0600, Christoph Lameter wrote:
>>> On Mon, 15 Feb 2010, Nick Piggin wrote:
>>>
>>>> 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 

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

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

As far as I know nobody said the patch is wrong so far, just
that they wanted to have more analysis.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Pekka Enberg]

Christoph Lameter kirjoitti:
> On Mon, 15 Feb 2010, Andi Kleen wrote:
> 
>>> 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?

I don't see how my changes broke things but maybe I'm not looking hard 
enough. Cache reaping is still setup from cpucache_init() which is an 
initcall which is not affected by my changes AFAICT and from 
cpuup_callback() which shoulda also not be affected.

				Pekka

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Pekka Enberg]

Christoph Lameter kirjoitti:
> On Mon, 15 Feb 2010, Andi Kleen wrote:
> 
>>> 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?

I don't see how my changes broke things but maybe I'm not looking hard 
enough. Cache reaping is still setup from cpucache_init() which is an 
initcall which is not affected by my changes AFAICT and from 
cpuup_callback() which shoulda also not be affected.

				Pekka

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Pekka Enberg]

Andi Kleen kirjoitti:
> On Fri, Feb 19, 2010 at 12:22:58PM -0600, Christoph Lameter wrote:
>> On Mon, 15 Feb 2010, Nick Piggin wrote:
>>
>>> 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.

			Pekka

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Pekka Enberg]

Andi Kleen kirjoitti:
> On Fri, Feb 19, 2010 at 12:22:58PM -0600, Christoph Lameter wrote:
>> On Mon, 15 Feb 2010, Nick Piggin wrote:
>>
>>> 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.

			Pekka

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Fri, Feb 19, 2010 at 12:22:58PM -0600, Christoph Lameter wrote:
> On Mon, 15 Feb 2010, Nick Piggin wrote:
> 
> > 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

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Fri, Feb 19, 2010 at 12:22:58PM -0600, Christoph Lameter wrote:
> On Mon, 15 Feb 2010, Nick Piggin wrote:
> 
> > 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

-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Mon, 15 Feb 2010, Nick Piggin wrote:

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

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Mon, 15 Feb 2010, Nick Piggin wrote:

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

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

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Mon, 15 Feb 2010, Andi Kleen wrote:

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Christoph Lameter]

On Mon, 15 Feb 2010, Andi Kleen wrote:

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

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

Nick Piggin <npiggin@suse.de> writes:
>
> Hmm, but it should, because if cpuup_prepare fails  then the
> CPU_ONLINE notifiers should never be called I think.

That's true.
  
-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

Nick Piggin <npiggin@suse.de> writes:
>
> Hmm, but it should, because if cpuup_prepare fails  then the
> CPU_ONLINE notifiers should never be called I think.

That's true.
  
-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Nick Piggin]

On Mon, Feb 15, 2010 at 11:52:53AM +0100, Andi Kleen wrote:
> On Mon, Feb 15, 2010 at 09:41:35PM +1100, Nick Piggin wrote:
> > On Mon, Feb 15, 2010 at 11:32:50AM +0100, Andi Kleen wrote:
> > > On Mon, Feb 15, 2010 at 05:15:35PM +1100, Nick Piggin wrote:
> > > > On Thu, Feb 11, 2010 at 09:54:04PM +0100, Andi Kleen wrote:
> > > > > 
> > > > > 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 :)
> > 
> > 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.

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

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

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Nick Piggin]

On Mon, Feb 15, 2010 at 11:52:53AM +0100, Andi Kleen wrote:
> On Mon, Feb 15, 2010 at 09:41:35PM +1100, Nick Piggin wrote:
> > On Mon, Feb 15, 2010 at 11:32:50AM +0100, Andi Kleen wrote:
> > > On Mon, Feb 15, 2010 at 05:15:35PM +1100, Nick Piggin wrote:
> > > > On Thu, Feb 11, 2010 at 09:54:04PM +0100, Andi Kleen wrote:
> > > > > 
> > > > > 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 :)
> > 
> > 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.

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

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

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

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Mon, Feb 15, 2010 at 09:41:35PM +1100, Nick Piggin wrote:
> On Mon, Feb 15, 2010 at 11:32:50AM +0100, Andi Kleen wrote:
> > On Mon, Feb 15, 2010 at 05:15:35PM +1100, Nick Piggin wrote:
> > > On Thu, Feb 11, 2010 at 09:54:04PM +0100, Andi Kleen wrote:
> > > > 
> > > > 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 :)
> 
> 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.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Mon, Feb 15, 2010 at 09:41:35PM +1100, Nick Piggin wrote:
> On Mon, Feb 15, 2010 at 11:32:50AM +0100, Andi Kleen wrote:
> > On Mon, Feb 15, 2010 at 05:15:35PM +1100, Nick Piggin wrote:
> > > On Thu, Feb 11, 2010 at 09:54:04PM +0100, Andi Kleen wrote:
> > > > 
> > > > 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 :)
> 
> 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.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Nick Piggin]

On Mon, Feb 15, 2010 at 11:32:50AM +0100, Andi Kleen wrote:
> On Mon, Feb 15, 2010 at 05:15:35PM +1100, Nick Piggin wrote:
> > On Thu, Feb 11, 2010 at 09:54:04PM +0100, Andi Kleen wrote:
> > > 
> > > 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 :)

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?

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Nick Piggin]

On Mon, Feb 15, 2010 at 11:32:50AM +0100, Andi Kleen wrote:
> On Mon, Feb 15, 2010 at 05:15:35PM +1100, Nick Piggin wrote:
> > On Thu, Feb 11, 2010 at 09:54:04PM +0100, Andi Kleen wrote:
> > > 
> > > 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 :)

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?

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Mon, Feb 15, 2010 at 05:15:35PM +1100, Nick Piggin wrote:
> On Thu, Feb 11, 2010 at 09:54:04PM +0100, Andi Kleen wrote:
> > 
> > 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 :)

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Andi Kleen]

On Mon, Feb 15, 2010 at 05:15:35PM +1100, Nick Piggin wrote:
> On Thu, Feb 11, 2010 at 09:54:04PM +0100, Andi Kleen wrote:
> > 
> > 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 :)

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Nick Piggin]

On Thu, Feb 11, 2010 at 09:54:04PM +0100, Andi Kleen wrote:
> 
> 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.

> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> 
> ---
>  mm/slab.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> Index: linux-2.6.32-memhotadd/mm/slab.c
> ===================================================================
> --- linux-2.6.32-memhotadd.orig/mm/slab.c
> +++ linux-2.6.32-memhotadd/mm/slab.c
> @@ -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 majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[Nick Piggin]

On Thu, Feb 11, 2010 at 09:54:04PM +0100, Andi Kleen wrote:
> 
> 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.

> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> 
> ---
>  mm/slab.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> Index: linux-2.6.32-memhotadd/mm/slab.c
> ===================================================================
> --- linux-2.6.32-memhotadd.orig/mm/slab.c
> +++ linux-2.6.32-memhotadd/mm/slab.c
> @@ -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 majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[David Rientjes]

On Thu, 11 Feb 2010, Andi Kleen wrote:

> 
> 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 <ak@linux.intel.com>

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

[David Rientjes]

On Thu, 11 Feb 2010, Andi Kleen wrote:

> 
> 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 <ak@linux.intel.com>

Acked-by: David Rientjes <rientjes@google.com>

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

[PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

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

Signed-off-by: Andi Kleen <ak@linux.intel.com>

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

Index: linux-2.6.32-memhotadd/mm/slab.c
===================================================================
--- linux-2.6.32-memhotadd.orig/mm/slab.c
+++ linux-2.6.32-memhotadd/mm/slab.c
@@ -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);
 

[PATCH] [4/4] SLAB: Fix node add timer race in cache_reap

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

Signed-off-by: Andi Kleen <ak@linux.intel.com>

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

Index: linux-2.6.32-memhotadd/mm/slab.c
===================================================================
--- linux-2.6.32-memhotadd.orig/mm/slab.c
+++ linux-2.6.32-memhotadd/mm/slab.c
@@ -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 majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>