[PATCH] Remove set_migrateflags()

[PATCH] Remove set_migrateflags()

[Christoph Lameter]

Migrate flags must be set on slab creation as agreed upon when the 
antifrag logic was reviewed. Otherwise some slabs of a slabcache will end 
up in the unmovable and others in the reclaimable section depending on 
which flag was active when a new slab page was allocated.

This likely slid in somehow when antifrag was merged. Remove it.

The buffer_heads are always allocated with __GFP_RECLAIMABLE because the 
SLAB_RECLAIM_ACCOUNT option is set. The set_migrateflags() never had any 
effect there.

Radix tree allocations are not directly reclaimable but they are allocated 
with __GFP_RECLAIMABLE set on each allocation. We now set 
SLAB_RECLAIM_ACCOUNT on radix tree slab creation making sure that radix 
tree slabs are consistently placed in the reclaimable section. Radix tree 
slabs will also be accounted as such.

There is then no user left of set_migratepages. So remove it.

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

---
 fs/buffer.c         |    3 +--
 include/linux/gfp.h |    6 ------
 lib/radix-tree.c    |    9 ++++-----
 3 files changed, 5 insertions(+), 13 deletions(-)

Index: linux-2.6/fs/buffer.c
===================================================================
--- linux-2.6.orig/fs/buffer.c	2008-02-08 13:22:14.000000000 -0800
+++ linux-2.6/fs/buffer.c	2008-02-11 15:53:51.000000000 -0800
@@ -3169,8 +3169,7 @@ static void recalc_bh_state(void)
 	
 struct buffer_head *alloc_buffer_head(gfp_t gfp_flags)
 {
-	struct buffer_head *ret = kmem_cache_alloc(bh_cachep,
-				set_migrateflags(gfp_flags, __GFP_RECLAIMABLE));
+	struct buffer_head *ret = kmem_cache_alloc(bh_cachep, gfp_flags);
 	if (ret) {
 		INIT_LIST_HEAD(&ret->b_assoc_buffers);
 		get_cpu_var(bh_accounting).nr++;
Index: linux-2.6/lib/radix-tree.c
===================================================================
--- linux-2.6.orig/lib/radix-tree.c	2008-02-07 19:07:05.000000000 -0800
+++ linux-2.6/lib/radix-tree.c	2008-02-11 15:55:19.000000000 -0800
@@ -114,8 +114,7 @@ radix_tree_node_alloc(struct radix_tree_
 		}
 	}
 	if (ret == NULL)
-		ret = kmem_cache_alloc(radix_tree_node_cachep,
-				set_migrateflags(gfp_mask, __GFP_RECLAIMABLE));
+		ret = kmem_cache_alloc(radix_tree_node_cachep, gfp_mask);
 
 	BUG_ON(radix_tree_is_indirect_ptr(ret));
 	return ret;
@@ -150,8 +149,7 @@ int radix_tree_preload(gfp_t gfp_mask)
 	rtp = &__get_cpu_var(radix_tree_preloads);
 	while (rtp->nr < ARRAY_SIZE(rtp->nodes)) {
 		preempt_enable();
-		node = kmem_cache_alloc(radix_tree_node_cachep,
-				set_migrateflags(gfp_mask, __GFP_RECLAIMABLE));
+		node = kmem_cache_alloc(radix_tree_node_cachep, gfp_mask);
 		if (node == NULL)
 			goto out;
 		preempt_disable();
@@ -1098,7 +1096,8 @@ void __init radix_tree_init(void)
 {
 	radix_tree_node_cachep = kmem_cache_create("radix_tree_node",
 			sizeof(struct radix_tree_node), 0,
-			SLAB_PANIC, radix_tree_node_ctor);
+			SLAB_PANIC | SLAB_RECLAIM_ACCOUNT,
+			radix_tree_node_ctor);
 	radix_tree_init_maxindex();
 	hotcpu_notifier(radix_tree_callback, 0);
 }
Index: linux-2.6/include/linux/gfp.h
===================================================================
--- linux-2.6.orig/include/linux/gfp.h	2008-02-07 19:07:05.000000000 -0800
+++ linux-2.6/include/linux/gfp.h	2008-02-11 15:53:12.000000000 -0800
@@ -144,12 +144,6 @@ static inline enum zone_type gfp_zone(gf
 	return base + ZONE_NORMAL;
 }
 
-static inline gfp_t set_migrateflags(gfp_t gfp, gfp_t migrate_flags)
-{
-	BUG_ON((gfp & GFP_MOVABLE_MASK) == GFP_MOVABLE_MASK);
-	return (gfp & ~(GFP_MOVABLE_MASK)) | migrate_flags;
-}
-
 /*
  * There is only one page-allocator function, and two main namespaces to
  * it. The alloc_page*() variants return 'struct page *' and as such

--
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] Remove set_migrateflags()

[Mel Gorman]

On (14/01/08 11:29), Christoph Lameter didst pronounce:
> On Mon, 14 Jan 2008, Mel Gorman wrote:
> 
> > Grouping the radix nodes into the same TLB entries as the inode and dcaches
> > does appear to help performance a small amount on kernbench at least. Applying
> > this patch showed a performance difference on elapsed time between -4.45%
> > and 0.23% and between -0.36% and 0.28% on total CPU time which appears to
> > support that position.
> 
> Ahh... Okay.
> 
> > > And thus setting __GFP_RECLAIMABLE
> > > is a bit strange. We could set SLAB_RECLAIM_ACCOUNT on radix tree slab
> > > creation if we want those to be placed in the reclaimable section.
> > > Then we are sure that the radix tree slabs are consistently placed in the
> > > reclaimable section and then the radix tree slabs will also be accounted as
> > > such.
> > > 
> > 
> > What is there right now places the pages appropriately but should they really
> > be accounted for as such too? I know that marking them like that will
> > cause SLUB to treat them differently and I don't fully understand the
> > implications of that.
> 
> Marking them makes the slab allocators set GFP_RECLAIMABLE on all page 
> allocator allocations for the radix tree and it will also cause the 
> statistics to be update correspondingly. No other differences.
> 

Ok, great.

> > NAK for now. I'm still of the opinion that radix nodes should be marked
> > reclaimable because they are often cleaned up at the same time as slabs that
> > are really reclaimable.
> 
> Do another version of this patch setting SLAB_RECLAIM_ACCOUNT for the 
> radix tree?
> 

If you send me a version, I'll review and put it through the same tests.
I can roll the patch as well if you'd prefer.

Thanks.

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
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] Remove set_migrateflags()

[Christoph Lameter]

On Mon, 14 Jan 2008, Mel Gorman wrote:

> Grouping the radix nodes into the same TLB entries as the inode and dcaches
> does appear to help performance a small amount on kernbench at least. Applying
> this patch showed a performance difference on elapsed time between -4.45%
> and 0.23% and between -0.36% and 0.28% on total CPU time which appears to
> support that position.

Ahh... Okay.

> > And thus setting __GFP_RECLAIMABLE
> > is a bit strange. We could set SLAB_RECLAIM_ACCOUNT on radix tree slab
> > creation if we want those to be placed in the reclaimable section.
> > Then we are sure that the radix tree slabs are consistently placed in the
> > reclaimable section and then the radix tree slabs will also be accounted as
> > such.
> > 
> 
> What is there right now places the pages appropriately but should they really
> be accounted for as such too? I know that marking them like that will
> cause SLUB to treat them differently and I don't fully understand the
> implications of that.

Marking them makes the slab allocators set GFP_RECLAIMABLE on all page 
allocator allocations for the radix tree and it will also cause the 
statistics to be update correspondingly. No other differences.

> NAK for now. I'm still of the opinion that radix nodes should be marked
> reclaimable because they are often cleaned up at the same time as slabs that
> are really reclaimable.

Do another version of this patch setting SLAB_RECLAIM_ACCOUNT for the 
radix tree?

--
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] Remove set_migrateflags()

[Mel Gorman]

On (10/01/08 18:42), Christoph Lameter didst pronounce:
> set_migrateflagsi() sole purpose is to set migrate flags on slab allocations.
> However, the migrate flags must set on slab creation as agreed upon when the
> antifrag logic was reviewed. Otherwise some slabs of a slabcache will end up
> in the unmovable and others in the reclaimable section depending on what
> flags was active when a new slab was allocated.
> 
> This likely slid in somehow when antifrag was merged. Remove it.
> 
> The buffer_heads are always allocated with __GFP_RECLAIMABLE because
> the SLAB_RECLAIM_ACCOUNT option is set.
> 
> The set_migrateflags() never had any effect.
> 

Ok, this part I agree with.

> Radix tree allocations are not reclaimable.

The thinking behind this was that radix nodes are often (but not always)
indirectly reclaimable as they are cleaned up when related data structures
(that are reclaimable) get taken back. This does not apply to them all of
course but enough that this seemed fair.

Grouping the radix nodes into the same TLB entries as the inode and dcaches
does appear to help performance a small amount on kernbench at least. Applying
this patch showed a performance difference on elapsed time between -4.45%
and 0.23% and between -0.36% and 0.28% on total CPU time which appears to
support that position.

Applying this patch also reduces high allocation success rates although I
will freely admit that this *could* be related to the type of workload.

> And thus setting __GFP_RECLAIMABLE
> is a bit strange. We could set SLAB_RECLAIM_ACCOUNT on radix tree slab
> creation if we want those to be placed in the reclaimable section.
> Then we are sure that the radix tree slabs are consistently placed in the
> reclaimable section and then the radix tree slabs will also be accounted as
> such.
> 

What is there right now places the pages appropriately but should they really
be accounted for as such too? I know that marking them like that will
cause SLUB to treat them differently and I don't fully understand the
implications of that.

> The simple removal of set_migrateflags() here will place the allocations
> in the unmovable section.
> 
> Signed-off-by: Christoph Lameter <clameter@sgi.com>
> 

NAK for now. I'm still of the opinion that radix nodes should be marked
reclaimable because they are often cleaned up at the same time as slabs that
are really reclaimable.

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
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] Remove set_migrateflags()

[Christoph Lameter]

set_migrateflagsi() sole purpose is to set migrate flags on slab allocations.
However, the migrate flags must set on slab creation as agreed upon when the
antifrag logic was reviewed. Otherwise some slabs of a slabcache will end up
in the unmovable and others in the reclaimable section depending on what
flags was active when a new slab was allocated.

This likely slid in somehow when antifrag was merged. Remove it.

The buffer_heads are always allocated with __GFP_RECLAIMABLE because
the SLAB_RECLAIM_ACCOUNT option is set.

The set_migrateflags() never had any effect.

Radix tree allocations are not reclaimable. And thus setting __GFP_RECLAIMABLE
is a bit strange. We could set SLAB_RECLAIM_ACCOUNT on radix tree slab
creation if we want those to be placed in the reclaimable section.
Then we are sure that the radix tree slabs are consistently placed in the
reclaimable section and then the radix tree slabs will also be accounted as
such.

The simple removal of set_migrateflags() here will place the allocations
in the unmovable section.

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

---
 fs/buffer.c         |    4 ++--
 include/linux/gfp.h |    6 ------
 lib/radix-tree.c    |    6 ++----
 3 files changed, 4 insertions(+), 12 deletions(-)

Index: linux-2.6/fs/buffer.c
===================================================================
--- linux-2.6.orig/fs/buffer.c	2008-01-09 23:52:30.401723074 -0800
+++ linux-2.6/fs/buffer.c	2008-01-10 18:24:08.000545183 -0800
@@ -3169,8 +3169,8 @@ static void recalc_bh_state(void)
 	
 struct buffer_head *alloc_buffer_head(gfp_t gfp_flags)
 {
-	struct buffer_head *ret = kmem_cache_zalloc(bh_cachep,
-				set_migrateflags(gfp_flags, __GFP_RECLAIMABLE));
+	struct buffer_head *ret = kmem_cache_zalloc(bh_cachep, gfp_flags);
+
 	if (ret) {
 		INIT_LIST_HEAD(&ret->b_assoc_buffers);
 		get_cpu_var(bh_accounting).nr++;
Index: linux-2.6/lib/radix-tree.c
===================================================================
--- linux-2.6.orig/lib/radix-tree.c	2008-01-09 23:52:30.421723289 -0800
+++ linux-2.6/lib/radix-tree.c	2008-01-10 00:18:22.913856382 -0800
@@ -98,8 +98,7 @@ radix_tree_node_alloc(struct radix_tree_
 	struct radix_tree_node *ret;
 	gfp_t gfp_mask = root_gfp_mask(root);
 
-	ret = kmem_cache_alloc(radix_tree_node_cachep,
-				set_migrateflags(gfp_mask, __GFP_RECLAIMABLE));
+	ret = kmem_cache_alloc(radix_tree_node_cachep, gfp_mask);
 	if (ret == NULL && !(gfp_mask & __GFP_WAIT)) {
 		struct radix_tree_preload *rtp;
 
@@ -143,8 +142,7 @@ int radix_tree_preload(gfp_t gfp_mask)
 	rtp = &__get_cpu_var(radix_tree_preloads);
 	while (rtp->nr < ARRAY_SIZE(rtp->nodes)) {
 		preempt_enable();
-		node = kmem_cache_alloc(radix_tree_node_cachep,
-				set_migrateflags(gfp_mask, __GFP_RECLAIMABLE));
+		node = kmem_cache_alloc(radix_tree_node_cachep, gfp_mask);
 		if (node == NULL)
 			goto out;
 		preempt_disable();
Index: linux-2.6/include/linux/gfp.h
===================================================================
--- linux-2.6.orig/include/linux/gfp.h	2008-01-10 18:21:19.543702140 -0800
+++ linux-2.6/include/linux/gfp.h	2008-01-10 18:21:30.747757986 -0800
@@ -144,12 +144,6 @@ static inline enum zone_type gfp_zone(gf
 	return base + ZONE_NORMAL;
 }
 
-static inline gfp_t set_migrateflags(gfp_t gfp, gfp_t migrate_flags)
-{
-	BUG_ON((gfp & GFP_MOVABLE_MASK) == GFP_MOVABLE_MASK);
-	return (gfp & ~(GFP_MOVABLE_MASK)) | migrate_flags;
-}
-
 /*
  * There is only one page-allocator function, and two main namespaces to
  * it. The alloc_page*() variants return 'struct page *' and as such

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