CMA: clear buffer-head lru before page migration

From d90cd3d13b73f7278c60d8fe625840664adcbb6a Mon Sep 17 00:00:00 2001

From: Gioh Kim <[email protected]>
Date: Fri, 4 Jul 2014 16:53:22 +0900
Subject: [PATCH] [RFC] CMA: clear buffer-head lru before page migration

When CMA try to migrate page, some buffer-heads can exist on lru. The bh on lru has non-zero count value so that it cannot be dropped even-if it is not used. We can drop only buffers related to the migrated page, but it can take long time more than dropping all because of searching list. There all buffers in lru are dropped.

Signed-off-by Laura Abbott <[email protected]>

Signed-off-by Gioh Kim <[email protected]>

---
 fs/buffer.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

fs/buffer.c +2 -0

index eba6e4f..4f11b7a 100644
@@ -3233,6 +3233,19 @@ int try_to_free_buffers(struct page *page)
        if (PageWriteback(page))
                return 0;
 
+#ifdef CONFIG_CMA
+       /*

Quoted 27 lines

From: Gioh Kim <[email protected]>
Date: Fri, 4 Jul 2014 16:53:22 +0900
Subject: [PATCH] [RFC] CMA: clear buffer-head lru before page migration

When CMA try to migrate page, some buffer-heads can exist on lru. The bh on lru has non-zero count value so that it cannot be dropped even-if it is not used. We can drop only buffers related to the migrated page, but it can take long time more than dropping all because of searching list. There all buffers in lru are dropped.

Signed-off-by Laura Abbott <[email protected]>

Signed-off-by Gioh Kim <[email protected]>

---
  fs/buffer.c |   13 +++++++++++++
  1 file changed, 13 insertions(+)

fs/buffer.c +2 -0

index eba6e4f..4f11b7a 100644
@@ -3233,6 +3233,19 @@ int try_to_free_buffers(struct page *page)
         if (PageWriteback(page))
                 return 0;
 
+#ifdef CONFIG_CMA
+       /*

fs/buffer.c int try_to_free_buffers(struct page *page) · line 3247

Quoted 10 lines

fs/buffer.c +10 -0

+        * When CMA try to migrate page, some buffer-heads can exist on lru.
+        * The bh on lru has non-zero count value so that it cannot
+        * be dropped even-if it is not used.
+        * We can drop only buffers related to the migrated page,
+        * but it can take long time more than dropping all
+        * because of searching list.
+        * There all buffers in lru are dropped first.
+        */
+       invalidate_bh_lrus();
+#endif

No, this will be tremendously expensive.

What I proposed is that CMA call invalidate_bh_lrus() right at the outset. Something along the lines of

mm/page_alloc.c renamed from mm/page_alloc.c~a +8 -3

@@ -6329,6 +6329,14 @@ int alloc_contig_range(unsigned long sta
  	};
  	INIT_LIST_HEAD(&cc.migratepages);
 
+#ifdef CONFIG_CMA
+	/*
+	 * Comment goes here
+	 */
+	if (migratetype == MIGRATE_CMA)
+		invalidate_bh_lrus();
+#endif
+
  	/*
  	 * What we do here is we mark all pageblocks in range as
  	 * MIGRATE_ISOLATE.  Because pageblock and max order pages may
 
 
- I'd have thought that it would make sense to do this for huge pages
   as well (MIGRATE_MOVABLE) but nobody really seems to know.
 
- There's a patch floating around ("Allow increasing the buffer-head
   per-CPU LRU size") which will double the size of the bh lrus, so this
   all becomes more important.
 
- alloc_contig_range() does lru_add_drain_all() and drain_all_pages()
   *after* performing the allocation.  I can't work out why this is the
   case and of course it is undocumented.  If this is indeed not a bug
   then probably the invalidate_bh_lrus() should happen in the same
   place.

2014-07-08 ______ 7:52, Andrew Morton ___ ___:

Quoted 43 lines · On Fri, 04 Jul 2014 17:25:09 +0900 Gioh Kim <[email protected]> wrote:

Quoted 27 lines

From: Gioh Kim <[email protected]>
Date: Fri, 4 Jul 2014 16:53:22 +0900
Subject: [PATCH] [RFC] CMA: clear buffer-head lru before page migration

When CMA try to migrate page, some buffer-heads can exist on lru. The bh on lru has non-zero count value so that it cannot be dropped even-if it is not used. We can drop only buffers related to the migrated page, but it can take long time more than dropping all because of searching list. There all buffers in lru are dropped.

Signed-off-by Laura Abbott <[email protected]>

Signed-off-by Gioh Kim <[email protected]>

---
  fs/buffer.c |   13 +++++++++++++
  1 file changed, 13 insertions(+)

fs/buffer.c +2 -0

index eba6e4f..4f11b7a 100644
@@ -3233,6 +3233,19 @@ int try_to_free_buffers(struct page *page)
         if (PageWriteback(page))
                 return 0;
 
+#ifdef CONFIG_CMA
+       /*

fs/buffer.c int try_to_free_buffers(struct page *page) · line 3247

Quoted 10 lines

fs/buffer.c +10 -0

+        * When CMA try to migrate page, some buffer-heads can exist on lru.
+        * The bh on lru has non-zero count value so that it cannot
+        * be dropped even-if it is not used.
+        * We can drop only buffers related to the migrated page,
+        * but it can take long time more than dropping all
+        * because of searching list.
+        * There all buffers in lru are dropped first.
+        */
+       invalidate_bh_lrus();
+#endif

No, this will be tremendously expensive.

What I proposed is that CMA call invalidate_bh_lrus() right at the outset. Something along the lines of

It's my fault.
I'm going to send another patch ASAP.

What I proposed is that CMA call invalidate_bh_lrus() right at the outset. Something along the lines of

mm/page_alloc.c renamed from mm/page_alloc.c~a +0 -0

@@ -6329,6 +6329,14 @@ int alloc_contig_range(unsigned long sta
 	};

mm/page_alloc.c renamed from mm/page_alloc.c~a +0 -1

 	/*
 	 * What we do here is we mark all pageblocks in range as
 	 * MIGRATE_ISOLATE.  Because pageblock and max order pages may
 
 
- I'd have thought that it would make sense to do this for huge pages
  as well (MIGRATE_MOVABLE) but nobody really seems to know.

Quoted 7 lines · Andrew Morton

What I proposed is that CMA call invalidate_bh_lrus() right at the outset. Something along the lines of

mm/page_alloc.c renamed from mm/page_alloc.c~a +0 -0

@@ -6329,6 +6329,14 @@ int alloc_contig_range(unsigned long sta
 	};

mm/page_alloc.c int alloc_contig_range(unsigned long sta · line 6339

Quoted 10 lines

mm/page_alloc.c renamed from mm/page_alloc.c~a +8 -0

 	INIT_LIST_HEAD(&cc.migratepages);
 
+#ifdef CONFIG_CMA
+	/*
+	 * Comment goes here
+	 */
+	if (migratetype == MIGRATE_CMA)
+		invalidate_bh_lrus();
+#endif
+

This seems reasonable, except I think it should go after start_isolate_page_range call because otherwise there's no guarantee that someone won't grab those pages back.

Also to avoid the #ifdef perhaps we want this as well:

include/linux/mmzone.h +3 -2

index 6cbd1b6..2640a55 100644
@@ -64,10 +64,11 @@ enum {
 };
 
 #ifdef CONFIG_CMA
-#  define is_migrate_cma(migratetype) unlikely((migratetype) == MIGRATE_CMA)
+#  define __is_migrate_cma(migratetype) ((migratetype) == MIGRATE_CMA)
 #else
-#  define is_migrate_cma(migratetype) false
+#  define __is_migrate_cma(migratetype) false
 #endif
+#define is_migrate_cma(migratetype) unlikely(__is_migrate_cma(migratetype))
 
 #define for_each_migratetype_order(order, type) \
 	for (order = 0; order < MAX_ORDER; order++) \

and then use “if (__is_migrate_cma(migratetype))”.

Quoted 7 lines

mm/page_alloc.c renamed from mm/page_alloc.c~a +0 -1

 	/*
 	 * What we do here is we mark all pageblocks in range as
 	 * MIGRATE_ISOLATE.  Because pageblock and max order pages may
 
 
- I'd have thought that it would make sense to do this for huge pages
  as well (MIGRATE_MOVABLE) but nobody really seems to know.

mm/page_alloc.c int alloc_contig_range(unsigned long sta

Quoted 10 lines

mm/page_alloc.c renamed from mm/page_alloc.c~a +0 -2

 
- There's a patch floating around ("Allow increasing the buffer-head
  per-CPU LRU size") which will double the size of the bh lrus, so this
  all becomes more important.
 
- alloc_contig_range() does lru_add_drain_all() and drain_all_pages()
  *after* performing the allocation.  I can't work out why this is the
  case and of course it is undocumented.  If this is indeed not a bug
  then probably the invalidate_bh_lrus() should happen in the same
  place.

The purpose is to get free non-buddy pages (so pages on PCP lists for instance) back onto the buddy list. It's safe to move those calls above the call to __alloc_contig_migrate_range, but I don't think it will change anything (except of course the fact that if migration fails, we'll do the draining for nothing).

Quoted 5 lines · Michal Nazarewicz

This seems reasonable, except I think [invalidate_bh_lrus()] should go after start_isolate_page_range call because otherwise there's no guarantee that someone won't grab those pages back.

Also to avoid the #ifdef perhaps we want this as well:

I think that we just want to remove ifdef CONFIG_CMA on above code snippet, because invalidate_bh_lrus() would also help user of alloc_contig_range() with MIGRATE_MOVABLE.

Quoted 5 lines

The purpose is to get free non-buddy pages (so pages on PCP lists for instance) back onto the buddy list. It's safe to move those calls above the call to __alloc_contig_migrate_range, but I don't think it will change anything (except of course the fact that if migration fails, we'll do the draining for nothing).

At a glance, we don't need that drain_all_pages(), because drain_all_pages() is also called by set_migratetype_isolate() after changing migratetype.

And, it is better to move up lru_add_drain_all() to ahead of __alloc_contig_migrate_range(), because some pages could be skipped to migrate due to this lru page caching mechanism.

Quoted 23 lines · Michal Nazarewicz

Quoted 7 lines · Andrew Morton

What I proposed is that CMA call invalidate_bh_lrus() right at the outset. Something along the lines of

mm/page_alloc.c renamed from mm/page_alloc.c~a +0 -0

@@ -6329,6 +6329,14 @@ int alloc_contig_range(unsigned long sta
 	};

mm/page_alloc.c int alloc_contig_range(unsigned long sta · line 6339

Quoted 10 lines

mm/page_alloc.c renamed from mm/page_alloc.c~a +8 -0

 	INIT_LIST_HEAD(&cc.migratepages);
 
+#ifdef CONFIG_CMA
+	/*
+	 * Comment goes here
+	 */
+	if (migratetype == MIGRATE_CMA)
+		invalidate_bh_lrus();
+#endif
+

This seems reasonable, except I think it should go after start_isolate_page_range call because otherwise there's no guarantee that someone won't grab those pages back.

Quoted 1 line

Also to avoid the #ifdef perhaps we want this as well:

I think that we just want to remove ifdef CONFIG_CMA on above code snippet, because invalidate_bh_lrus() would also help user of alloc_contig_range() with MIGRATE_MOVABLE.

Quoted 25 lines · Michal Nazarewicz

Quoted 23 lines · Andrew Morton

Quoted 7 lines

What I proposed is that CMA call invalidate_bh_lrus() right at the outset. Something along the lines of

mm/page_alloc.c renamed from mm/page_alloc.c~a +0 -0

@@ -6329,6 +6329,14 @@ int alloc_contig_range(unsigned long sta
  	};

mm/page_alloc.c int alloc_contig_range(unsigned long sta · line 6339

Quoted 10 lines

mm/page_alloc.c renamed from mm/page_alloc.c~a +8 -0

  	INIT_LIST_HEAD(&cc.migratepages);
 
+#ifdef CONFIG_CMA
+	/*
+	 * Comment goes here
+	 */
+	if (migratetype == MIGRATE_CMA)
+		invalidate_bh_lrus();
+#endif
+

This seems reasonable, except I think it should go after start_isolate_page_range call because otherwise there's no guarantee that someone won't grab those pages back.

Quoted 5 lines

Quoted 1 line

Also to avoid the #ifdef perhaps we want this as well:

I think that we just want to remove ifdef CONFIG_CMA on above code snippet, because invalidate_bh_lrus() would also help user of alloc_contig_range() with MIGRATE_MOVABLE.

That's what I believe also. I pinged Mel and Johannes off-list and Mel said "I hit it, the invalidation cost wasn't worth it for a THP alloc".

So hm. I do think it's worth additional investigation but some careful testing would be needed to demonstrate that it's worthwhile. If the invalidation cost is hurting then perhaps additional logic will be needed to perform the invalidation only as a last-resort thing.