[patch] slab: revert "slab: fix alien cache handling"

[patch] slab: revert "slab: fix alien cache handling"

[Siddha, Suresh B]

Christoph,

Can we revert git commit 3cdc0ed0cea50ea08dd146c1bbc82b1bcc2e1b80 ?
This is introducing a performance regression and nullifies the previous
commit.

commit 62918a036148230ba1ad175dc8a0952e3752ac57
Author: Siddha, Suresh B <suresh.b.siddha@intel.com>
Date:   Wed May 2 19:27:18 2007 +0200

    [PATCH] x86-64: skip cache_free_alien() on non NUMA
    
    Set use_alien_caches to 0 on non NUMA platforms.  And avoid calling the
    cache_free_alien() when use_alien_caches is not set.  This will avoid the
    cache miss that happens while dereferencing slabp to get nodeid.

Looking at the 3cdc0ed0cea50ea08dd146c1bbc82b1bcc2e1b80 changelog, I can't find
enough info of why cache_free_alien() must be called regardless if we use alien
caches or not.

Appended patch fixes the performance regression. Please  comment the need
for 3cdc0ed0cea50ea08dd146c1bbc82b1bcc2e1b80, if I miss something.

thanks,
suresh
---

Skip calling cache_free_alien() when alien caches are not used.
This will avoid cache misses that happen while accessing slabp (which
is per page memory  reference) to get nodeid. Instead use a global
variable to skip the call, which is mostly likely to be present in the
cache.

This gives a 0.8% performance boost with the database oltp workload
on a quad-core SMP platform and by any means the number is not small :)

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
---

diff --git a/mm/slab.c b/mm/slab.c
index a684778..d6e2251 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3558,7 +3558,14 @@ static inline void __cache_free(struct kmem_cache *cachep, void *objp)
 	check_irq_off();
 	objp = cache_free_debugcheck(cachep, objp, __builtin_return_address(0));
 
-	if (cache_free_alien(cachep, objp))
+	/*
+	 * Skip calling cache_free_alien() when alien caches are not used.
+	 * This will avoid cache misses that happen while accessing slabp (which
+	 * is per page memory  reference) to get nodeid. Instead use a global
+	 * variable to skip the call, which is mostly likely to be present in the
+	 * cache.
+	 */
+	if (use_alien_caches && cache_free_alien(cachep, objp))
 		return;
 
 	if (likely(ac->avail < ac->limit)) {

Re: [patch] slab: revert "slab: fix alien cache handling"

[Christoph Lameter]

On Mon, 13 Aug 2007, Siddha, Suresh B wrote:

> Can we revert git commit 3cdc0ed0cea50ea08dd146c1bbc82b1bcc2e1b80 ?

Only if you find another way to fix the bug that is addressed there.

> This is introducing a performance regression and nullifies the previous
> commit.

The implementation of noalien was breaking slab NUMA locality so 
that BUG() statements were triggered. This fixed it.

> Looking at the 3cdc0ed0cea50ea08dd146c1bbc82b1bcc2e1b80 changelog, I can't find
> enough info of why cache_free_alien() must be called regardless if we use alien
> caches or not.

cache_free_alien has to be called for any remote object because the per 
cpu queue does only contain node local objects. The noalian option was 
putting remote objects on the per cpu queue!

If you contaminate the per cpu queue with foreign objects then NUMA 
locality no longer works. Objects from other nodes are considered to be on 
the local node etc. So first of all requests for memory from one node will 
be served with memory from another,.

Plus the locks for slab lists are per node. So when we final drain the 
objects from the per cpu queue to the slab the local listlock may be taken 
to free an object to a remote node. If another free is in progress on the 
remote node then this will lead to corruption of slab meta data.

Re: [patch] slab: revert "slab: fix alien cache handling"

[Siddha, Suresh B]

On Mon, Aug 13, 2007 at 01:58:48PM -0700, Christoph Lameter wrote:
> On Mon, 13 Aug 2007, Siddha, Suresh B wrote:
> 
> > Can we revert git commit 3cdc0ed0cea50ea08dd146c1bbc82b1bcc2e1b80 ?
> 
> Only if you find another way to fix the bug that is addressed there.

Does the appended version fix both the issues? Name alien in cache_free_alien
is confusing, as the function does two things. free through alien caches
or direct remote free..

thanks,
suresh
---

Skip calling cache_free_alien() when the platform is not numa capable.
This will avoid cache misses that happen while accessing slabp (which
is per page memory  reference) to get nodeid. Instead use a global
variable to skip the call, which is mostly likely to be present in the
cache.

This gives a 0.8% performance boost with the database oltp workload
on a quad-core SMP platform and by any means the number is not small :)

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
---

diff --git a/mm/slab.c b/mm/slab.c
index a684778..6f6abef 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -883,6 +883,7 @@ static void __slab_error(const char *function, struct kmem_cache *cachep,
   */
 
 static int use_alien_caches __read_mostly = 1;
+static int numa_platform __read_mostly = 1;
 static int __init noaliencache_setup(char *s)
 {
 	use_alien_caches = 0;
@@ -1399,8 +1400,10 @@ void __init kmem_cache_init(void)
 	int order;
 	int node;
 
-	if (num_possible_nodes() == 1)
+	if (num_possible_nodes() == 1) {
 		use_alien_caches = 0;
+		numa_platform = 0;
+	}
 
 	for (i = 0; i < NUM_INIT_LISTS; i++) {
 		kmem_list3_init(&initkmem_list3[i]);
@@ -3558,7 +3561,14 @@ static inline void __cache_free(struct kmem_cache *cachep, void *objp)
 	check_irq_off();
 	objp = cache_free_debugcheck(cachep, objp, __builtin_return_address(0));
 
-	if (cache_free_alien(cachep, objp))
+	/*
+	 * Skip calling cache_free_alien() when the platform is not numa.
+	 * This will avoid cache misses that happen while accessing slabp (which
+	 * is per page memory  reference) to get nodeid. Instead use a global
+	 * variable to skip the call, which is mostly likely to be present in
+	 * the cache.
+	 */
+	if (numa_platform && cache_free_alien(cachep, objp))
 		return;
 
 	if (likely(ac->avail < ac->limit)) {

Re: [patch] slab: revert "slab: fix alien cache handling"

[Christoph Lameter]

On Mon, 13 Aug 2007, Siddha, Suresh B wrote:

> > Only if you find another way to fix the bug that is addressed there.
> 
> Does the appended version fix both the issues? Name alien in cache_free_alien
> is confusing, as the function does two things. free through alien caches
> or direct remote free..

Yes calling cache_free_alien is not necessary if you just have a single 
node. All memory is node local in that case.

I do not think that cache_free_alien is confusing since the name refers to 
the function freeing an alien object. That alien object can be freed to an 
alien cache or directly to a remote slab.

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