[PATCH] slab: minor cleanup to kmem_cache_alloc_node

[PATCH] slab: minor cleanup to kmem_cache_alloc_node

[Pekka J Enberg]

This patch gets rid of one if-else statement by moving current node allocation
check at the beginning of kmem_cache_alloc_node().

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

 slab.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 7f80ec3..a05bbfe 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2881,7 +2881,7 @@ void *kmem_cache_alloc_node(kmem_cache_t
 	unsigned long save_flags;
 	void *ptr;
 
-	if (nodeid == -1)
+	if (nodeid == -1 || nodeid == numa_node_id())
 		return __cache_alloc(cachep, flags);
 
 	if (unlikely(!cachep->nodelists[nodeid])) {
@@ -2892,10 +2892,7 @@ void *kmem_cache_alloc_node(kmem_cache_t
 
 	cache_alloc_debugcheck_before(cachep, flags);
 	local_irq_save(save_flags);
-	if (nodeid == numa_node_id())
-		ptr = ____cache_alloc(cachep, flags);
-	else
-		ptr = __cache_alloc_node(cachep, flags, nodeid);
+	ptr = __cache_alloc_node(cachep, flags, nodeid);
 	local_irq_restore(save_flags);
 	ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, __builtin_return_address(0));
 

Re: [PATCH] slab: minor cleanup to kmem_cache_alloc_node

[Eric Dumazet]

Pekka J Enberg a écrit :
> This patch gets rid of one if-else statement by moving current node allocation
> check at the beginning of kmem_cache_alloc_node().
> 

Are you sure this is valid ?
What about preemption ?
The "if (nodeid == numa_node_id())" check was done with IRQ off, and you want 
do do it with IRQ on.

> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
> ---
> 
>  slab.c |    7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 7f80ec3..a05bbfe 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2881,7 +2881,7 @@ void *kmem_cache_alloc_node(kmem_cache_t
>  	unsigned long save_flags;
>  	void *ptr;
>  
> -	if (nodeid == -1)
> +	if (nodeid == -1 || nodeid == numa_node_id())

<Imagine preemption here, and thread migrated to another NUMA node.>

>  		return __cache_alloc(cachep, flags);
>  
>  	if (unlikely(!cachep->nodelists[nodeid])) {
> @@ -2892,10 +2892,7 @@ void *kmem_cache_alloc_node(kmem_cache_t
>  
>  	cache_alloc_debugcheck_before(cachep, flags);
>  	local_irq_save(save_flags);
> -	if (nodeid == numa_node_id())
> -		ptr = ____cache_alloc(cachep, flags);
> -	else
> -		ptr = __cache_alloc_node(cachep, flags, nodeid);
> +	ptr = __cache_alloc_node(cachep, flags, nodeid);
>  	local_irq_restore(save_flags);
>  	ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, __builtin_return_address(0));
>  

Re: [PATCH] slab: minor cleanup to kmem_cache_alloc_node

[Pekka Enberg]

Hi Eric,

On Mon, 2005-11-21 at 16:58 +0100, Eric Dumazet wrote:
> Are you sure this is valid ?
> What about preemption ?
> The "if (nodeid == numa_node_id())" check was done with IRQ off, and you want 
> do do it with IRQ on.

You're right. Please ignore the patch.

			Pekka

Re: [PATCH] slab: minor cleanup to kmem_cache_alloc_node

[Christoph Lameter]

On Mon, 21 Nov 2005, Pekka J Enberg wrote:

> This patch gets rid of one if-else statement by moving current node allocation
> check at the beginning of kmem_cache_alloc_node().

The problem with this is that the numa_node may change if irqs are still 
active and your patch moves the check for the numa node outside of the 
section where irqs are enabled.

You could move the check for -1 into the section where interrupts are 
disabled.

Re: [PATCH] slab: minor cleanup to kmem_cache_alloc_node

[Pekka Enberg]

On Mon, 2005-11-21 at 09:21 -0800, Christoph Lameter wrote:
> On Mon, 21 Nov 2005, Pekka J Enberg wrote:
> 
> > This patch gets rid of one if-else statement by moving current node allocation
> > check at the beginning of kmem_cache_alloc_node().
> 
> The problem with this is that the numa_node may change if irqs are still 
> active and your patch moves the check for the numa node outside of the 
> section where irqs are enabled.
> 
> You could move the check for -1 into the section where interrupts are 
> disabled.

So we could do something like the following. Unfortunately it does not
seem much of an improvement... Thoughts?

			Pekka

Index: 2.6/mm/slab.c
===================================================================
--- 2.6.orig/mm/slab.c
+++ 2.6/mm/slab.c
@@ -2866,21 +2866,23 @@ void *kmem_cache_alloc_node(kmem_cache_t
 	unsigned long save_flags;
 	void *ptr;
 
-	if (nodeid == -1)
-		return __cache_alloc(cachep, flags);
+	cache_alloc_debugcheck_before(cachep, flags);
+	local_irq_save(save_flags);
+
+	if (nodeid == -1 || nodeid == numa_node_id()) {
+		ptr = ____cache_alloc(cachep, flags);
+		goto out;
+	}
 
 	if (unlikely(!cachep->nodelists[nodeid])) {
 		/* Fall back to __cache_alloc if we run into trouble */
 		printk(KERN_WARNING "slab: not allocating in inactive node %d for cache %s\n", nodeid, cachep->name);
-		return __cache_alloc(cachep,flags);
+		ptr = ____cache_alloc(cachep,flags);
+		goto out;
 	}
 
-	cache_alloc_debugcheck_before(cachep, flags);
-	local_irq_save(save_flags);
-	if (nodeid == numa_node_id())
-		ptr = ____cache_alloc(cachep, flags);
-	else
-		ptr = __cache_alloc_node(cachep, flags, nodeid);
+	ptr = __cache_alloc_node(cachep, flags, nodeid);
+  out:
 	local_irq_restore(save_flags);
 	ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, __builtin_return_address(0));
 

Re: [PATCH] slab: minor cleanup to kmem_cache_alloc_node

[Christoph Lameter]

On Mon, 21 Nov 2005, Pekka Enberg wrote:

> On Mon, 2005-11-21 at 09:21 -0800, Christoph Lameter wrote:
> > You could move the check for -1 into the section where interrupts are 
> > disabled.
> So we could do something like the following. Unfortunately it does not
> seem much of an improvement... Thoughts?

Remove the gotos. Something like this. It would be nice if we could remove 
the printk. Hmm....

Index: linux-2.6.15-rc1-mm2/mm/slab.c
===================================================================
--- linux-2.6.15-rc1-mm2.orig/mm/slab.c	2005-11-21 10:51:03.000000000 -0800
+++ linux-2.6.15-rc1-mm2/mm/slab.c	2005-11-21 11:43:31.000000000 -0800
@@ -2890,21 +2890,20 @@ void *kmem_cache_alloc_node(kmem_cache_t
 	unsigned long save_flags;
 	void *ptr;
 
-	if (nodeid == -1)
-		return __cache_alloc(cachep, flags);
+	cache_alloc_debugcheck_before(cachep, flags);
+	local_irq_save(save_flags);
 
-	if (unlikely(!cachep->nodelists[nodeid])) {
+	if (nodeid == -1 || nodeid == numa_node_id())
+		ptr = ____cache_alloc(cachep, flags);
+
+	else if (unlikely(!cachep->nodelists[nodeid])) {
 		/* Fall back to __cache_alloc if we run into trouble */
 		printk(KERN_WARNING "slab: not allocating in inactive node %d for cache %s\n", nodeid, cachep->name);
-		return __cache_alloc(cachep,flags);
-	}
+		ptr =  ____cache_alloc(cachep,flags);
 
-	cache_alloc_debugcheck_before(cachep, flags);
-	local_irq_save(save_flags);
-	if (nodeid == numa_node_id())
-		ptr = ____cache_alloc(cachep, flags);
-	else
+	} else
 		ptr = __cache_alloc_node(cachep, flags, nodeid);
+
 	local_irq_restore(save_flags);
 	ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, __builtin_return_address(0));
 

Re: [PATCH] slab: minor cleanup to kmem_cache_alloc_node

[Pekka Enberg]

Hi,

On Mon, 2005-11-21 at 11:47 -0800, Christoph Lameter wrote:
> Remove the gotos. Something like this. It would be nice if we could remove 
> the printk. Hmm....

Definite improvement to my patch. I think I like Joe's suggestion 
better, though. (Any mistakes in the following are mine.)

			Pekka

Index: 2.6/mm/slab.c
===================================================================
--- 2.6.orig/mm/slab.c
+++ 2.6/mm/slab.c
@@ -2866,21 +2866,16 @@ void *kmem_cache_alloc_node(kmem_cache_t
 	unsigned long save_flags;
 	void *ptr;
 
-	if (nodeid == -1)
-		return __cache_alloc(cachep, flags);
-
-	if (unlikely(!cachep->nodelists[nodeid])) {
-		/* Fall back to __cache_alloc if we run into trouble */
-		printk(KERN_WARNING "slab: not allocating in inactive node %d for cache %s\n", nodeid, cachep->name);
-		return __cache_alloc(cachep,flags);
-	}
-
 	cache_alloc_debugcheck_before(cachep, flags);
 	local_irq_save(save_flags);
-	if (nodeid == numa_node_id())
+
+	if (nodeid == -1 || nodeid == numa_node_id())
 		ptr = ____cache_alloc(cachep, flags);
-	else
+	else if (likely(cachep->nodelists[nodeid]))
 		ptr = __cache_alloc_node(cachep, flags, nodeid);
+	else
+		ptr = ____cache_alloc(cachep, flags);
+
 	local_irq_restore(save_flags);
 	ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, __builtin_return_address(0));
 

Re: [PATCH] slab: minor cleanup to kmem_cache_alloc_node

[Christoph Lameter]

On Mon, 21 Nov 2005, Pekka Enberg wrote:

> Hi,
> 
> On Mon, 2005-11-21 at 11:47 -0800, Christoph Lameter wrote:
> > Remove the gotos. Something like this. It would be nice if we could remove 
> > the printk. Hmm....
> 
> Definite improvement to my patch. I think I like Joe's suggestion 
> better, though. (Any mistakes in the following are mine.)

If we drop the printk then this may be even simpler

Signed-off-by: Christoph Lameter <clameter@sgi.com>

Index: linux-2.6.15-rc1-mm2/mm/slab.c
===================================================================
--- linux-2.6.15-rc1-mm2.orig/mm/slab.c	2005-11-21 13:16:07.000000000 -0800
+++ linux-2.6.15-rc1-mm2/mm/slab.c	2005-11-21 13:16:59.000000000 -0800
@@ -2890,21 +2890,14 @@ void *kmem_cache_alloc_node(kmem_cache_t
 	unsigned long save_flags;
 	void *ptr;
 
-	if (nodeid == -1)
-		return __cache_alloc(cachep, flags);
-
-	if (unlikely(!cachep->nodelists[nodeid])) {
-		/* Fall back to __cache_alloc if we run into trouble */
-		printk(KERN_WARNING "slab: not allocating in inactive node %d for cache %s\n", nodeid, cachep->name);
-		return __cache_alloc(cachep,flags);
-	}
-
 	cache_alloc_debugcheck_before(cachep, flags);
 	local_irq_save(save_flags);
-	if (nodeid == numa_node_id())
+
+	if (nodeid == -1 || nodeid == numa_node_id() || !cachep->nodelists[nodeid])
 		ptr = ____cache_alloc(cachep, flags);
 	else
 		ptr = __cache_alloc_node(cachep, flags, nodeid);
+
 	local_irq_restore(save_flags);
 	ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, __builtin_return_address(0));
 

Re: [PATCH] slab: minor cleanup to kmem_cache_alloc_node

[Pekka J Enberg]

On Mon, 21 Nov 2005, Christoph Lameter wrote:
> If we drop the printk then this may be even simpler
> 
> Signed-off-by: Christoph Lameter <clameter@sgi.com>

Even better. Thanks!

Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>

> Index: linux-2.6.15-rc1-mm2/mm/slab.c
> ===================================================================
> --- linux-2.6.15-rc1-mm2.orig/mm/slab.c	2005-11-21 13:16:07.000000000 -0800
> +++ linux-2.6.15-rc1-mm2/mm/slab.c	2005-11-21 13:16:59.000000000 -0800
> @@ -2890,21 +2890,14 @@ void *kmem_cache_alloc_node(kmem_cache_t
>  	unsigned long save_flags;
>  	void *ptr;
>  
> -	if (nodeid == -1)
> -		return __cache_alloc(cachep, flags);
> -
> -	if (unlikely(!cachep->nodelists[nodeid])) {
> -		/* Fall back to __cache_alloc if we run into trouble */
> -		printk(KERN_WARNING "slab: not allocating in inactive node %d for cache %s\n", nodeid, cachep->name);
> -		return __cache_alloc(cachep,flags);
> -	}
> -
>  	cache_alloc_debugcheck_before(cachep, flags);
>  	local_irq_save(save_flags);
> -	if (nodeid == numa_node_id())
> +
> +	if (nodeid == -1 || nodeid == numa_node_id() || !cachep->nodelists[nodeid])
>  		ptr = ____cache_alloc(cachep, flags);
>  	else
>  		ptr = __cache_alloc_node(cachep, flags, nodeid);
> +
>  	local_irq_restore(save_flags);
>  	ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, __builtin_return_address(0));
>  
>