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 <[email protected]>
---
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));
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 <[email protected]>
> ---
>
> 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));
>
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
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.
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));
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));
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));
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 <[email protected]>
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));
On Mon, 21 Nov 2005, Christoph Lameter wrote:
> If we drop the printk then this may be even simpler
>
> Signed-off-by: Christoph Lameter <[email protected]>
Even better. Thanks!
Acked-by: Pekka Enberg <[email protected]>
> 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));
>
>