Re: [patch 3/3] mm: NUMA slab -- minor optimizations

Re: [patch 3/3] mm: NUMA slab -- minor optimizations

[Alok Kataria]

On Wed, 2005-12-28 at 02:05, Christoph Lameter wrote:
On Tue, 27 Dec 2005, Manfred Spraul wrote:
> 
> > Isn't that a bug? What prevents an interrupt from occuring after the
> > spin_lock() and then causing a deadlock on cachep->spinlock?
> 
> Right. cache_grow() may be called when doing slab allocations in an 
> interrupt and it takes the lock in order to modify colour_next. 
> 
Yes you are right. 
Looking at the cache_grow code again i think we can do 
away with the cachep->spinlock in this code path.

The colour_next variable can be made per node to give better cache 
colouring effect.

Then this minor optimizations patch should be alright.

Comments ?

Thanks & Regards,
Alok.


--
The colour_next which is used to calculate the offset of the object in the
slab descriptor, is incremented whenever we add a slab to any of the list3
for a particular cache. This is done now for every list3 to give better 
(per node) cache colouring effect.
This also reduces thrashing on the cache_cache structure.

Signed-off-by: Alok N Kataria <alokk@calsoftinc.com>

Index: linux-2.6.15-rc7/mm/slab.c
===================================================================
--- linux-2.6.15-rc7.orig/mm/slab.c	2005-12-24 15:47:48.000000000 -0800
+++ linux-2.6.15-rc7/mm/slab.c	2006-01-02 07:00:36.000000000 -0800
@@ -293,6 +293,7 @@ struct kmem_list3 {
 	unsigned long	next_reap;
 	int		free_touched;
 	unsigned int 	free_limit;
+	unsigned int    colour_next;            /* cache colouring */
 	spinlock_t      list_lock;
 	struct array_cache	*shared;	/* shared per node */
 	struct array_cache	**alien;	/* on other nodes */
@@ -344,6 +345,7 @@ static inline void kmem_list3_init(struc
 	INIT_LIST_HEAD(&parent->slabs_free);
 	parent->shared = NULL;
 	parent->alien = NULL;
+	parent->colour_next = 0;
 	spin_lock_init(&parent->list_lock);
 	parent->free_objects = 0;
 	parent->free_touched = 0;
@@ -390,7 +392,6 @@ struct kmem_cache {
 
 	size_t			colour;		/* cache colouring range */
 	unsigned int		colour_off;	/* colour offset */
-	unsigned int		colour_next;	/* cache colouring */
 	kmem_cache_t		*slabp_cache;
 	unsigned int		slab_size;
 	unsigned int		dflags;		/* dynamic flags */
@@ -1060,7 +1061,6 @@ void __init kmem_cache_init(void)
 		BUG();
 
 	cache_cache.colour = left_over/cache_cache.colour_off;
-	cache_cache.colour_next = 0;
 	cache_cache.slab_size = ALIGN(cache_cache.num*sizeof(kmem_bufctl_t) +
 				sizeof(struct slab), cache_line_size());
 
@@ -2187,16 +2187,17 @@ static int cache_grow(kmem_cache_t *cach
 
 	/* About to mess with non-constant members - lock. */
 	check_irq_off();
-	spin_lock(&cachep->spinlock);
+	l3 = cachep->nodelists[nodeid];
+	spin_lock(&l3->list_lock);
 
 	/* Get colour for the slab, and cal the next value. */
-	offset = cachep->colour_next;
-	cachep->colour_next++;
-	if (cachep->colour_next >= cachep->colour)
-		cachep->colour_next = 0;
-	offset *= cachep->colour_off;
+	offset = l3->colour_next;
+	l3->colour_next++;
+	if (l3->colour_next >= cachep->colour)
+		l3->colour_next = 0;
+	spin_unlock(&l3->list_lock);
 
-	spin_unlock(&cachep->spinlock);
+	offset *= cachep->colour_off;
 
 	check_irq_off();
 	if (local_flags & __GFP_WAIT)
@@ -2228,7 +2229,6 @@ static int cache_grow(kmem_cache_t *cach
 	if (local_flags & __GFP_WAIT)
 		local_irq_disable();
 	check_irq_off();
-	l3 = cachep->nodelists[nodeid];
 	spin_lock(&l3->list_lock);
 
 	/* Make slab active. */

--
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/3] mm: NUMA slab -- minor optimizations

[Christoph Lameter]

On Tue, 27 Dec 2005, Manfred Spraul wrote:

> Isn't that a bug? What prevents an interrupt from occuring after the
> spin_lock() and then causing a deadlock on cachep->spinlock?

Right. cache_grow() may be called when doing slab allocations in an 
interrupt and it takes the lock in order to modify colour_next. 

--
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/3] mm: NUMA slab -- minor optimizations

[Manfred Spraul]

Ravikiran G Thirumalai wrote:

>Patch adds some minor optimizations:
>1. Keeps on chip interrupts enabled for a bit longer while draining cpu
>caches
>2. Calls numa_node_id once in cache_reap
>
>Signed-off-by: Alok N Kataria <alokk@calsoftinc.com>
>Signed-off-by: Ravikiran Thirumalai <kiran@scalex86.org>
>Signed-off-by: Shai Fultheim <shai@scalex86.org>
>
>Index: linux-2.6.15-rc1/mm/slab.c
>===================================================================
>--- linux-2.6.15-rc1.orig/mm/slab.c	2005-11-17 21:32:43.000000000 -0800
>+++ linux-2.6.15-rc1/mm/slab.c	2005-11-17 21:32:50.000000000 -0800
>@@ -1914,18 +1914,18 @@
> 
> 	smp_call_function_all_cpus(do_drain, cachep);
> 	check_irq_on();
>-	spin_lock_irq(&cachep->spinlock);
>+	spin_lock(&cachep->spinlock);
>  
>
Isn't that a bug? What prevents an interrupt from occuring after the 
spin_lock() and then causing a deadlock on cachep->spinlock?

--
    Manfred

--
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/3] mm: NUMA slab -- minor optimizations

[Christoph Lameter]

On Tue, 29 Nov 2005, Ravikiran G Thirumalai wrote:

> Patch adds some minor optimizations:
> 1. Keeps on chip interrupts enabled for a bit longer while draining cpu
> caches
> 2. Calls numa_node_id once in cache_reap
> 
> Signed-off-by: Alok N Kataria <alokk@calsoftinc.com>
> Signed-off-by: Ravikiran Thirumalai <kiran@scalex86.org>
> Signed-off-by: Shai Fultheim <shai@scalex86.org>

Ack-by: Christoph Lameter <clameter@sgi.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>

Re: [patch 3/3] mm: NUMA slab -- minor optimizations

[Ravikiran G Thirumalai]

Patch adds some minor optimizations:
1. Keeps on chip interrupts enabled for a bit longer while draining cpu
caches
2. Calls numa_node_id once in cache_reap

Signed-off-by: Alok N Kataria <alokk@calsoftinc.com>
Signed-off-by: Ravikiran Thirumalai <kiran@scalex86.org>
Signed-off-by: Shai Fultheim <shai@scalex86.org>

Index: linux-2.6.15-rc1/mm/slab.c
===================================================================
--- linux-2.6.15-rc1.orig/mm/slab.c	2005-11-17 21:32:43.000000000 -0800
+++ linux-2.6.15-rc1/mm/slab.c	2005-11-17 21:32:50.000000000 -0800
@@ -1914,18 +1914,18 @@
 
 	smp_call_function_all_cpus(do_drain, cachep);
 	check_irq_on();
-	spin_lock_irq(&cachep->spinlock);
+	spin_lock(&cachep->spinlock);
 	for_each_online_node(node)  {
 		l3 = cachep->nodelists[node];
 		if (l3) {
-			spin_lock(&l3->list_lock);
+			spin_lock_irq(&l3->list_lock);
 			drain_array_locked(cachep, l3->shared, 1, node);
-			spin_unlock(&l3->list_lock);
+			spin_unlock_irq(&l3->list_lock);
 			if (l3->alien)
 				drain_alien_cache(cachep, l3);
 		}
 	}
-	spin_unlock_irq(&cachep->spinlock);
+	spin_unlock(&cachep->spinlock);
 }
 
 static int __node_shrink(kmem_cache_t *cachep, int node)
@@ -3304,7 +3304,7 @@
 	list_for_each(walk, &cache_chain) {
 		kmem_cache_t *searchp;
 		struct list_head* p;
-		int tofree;
+		int tofree, nodeid;
 		struct slab *slabp;
 
 		searchp = list_entry(walk, kmem_cache_t, next);
@@ -3314,13 +3314,14 @@
 
 		check_irq_on();
 
-		l3 = searchp->nodelists[numa_node_id()];
+		nodeid = numa_node_id();
+		l3 = searchp->nodelists[nodeid];
 		if (l3->alien)
 			drain_alien_cache(searchp, l3);
 		spin_lock_irq(&l3->list_lock);
 
 		drain_array_locked(searchp, ac_data(searchp), 0,
-				numa_node_id());
+				nodeid);
 
 		if (time_after(l3->next_reap, jiffies))
 			goto next_unlock;
@@ -3329,7 +3330,7 @@
 
 		if (l3->shared)
 			drain_array_locked(searchp, l3->shared, 0,
-				numa_node_id());
+				nodeid);
 
 		if (l3->free_touched) {
 			l3->free_touched = 0;

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