[PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages

[PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages

[Vlastimil Babka]

The __test_page_isolated_in_pageblock() is used to verify whether all pages
in pageblock were either successfully isolated, or are hwpoisoned. Two of the
possible state of pages, that are tested, are however bogus and misleading.

Both tests rely on get_freepage_migratetype(page), which however has no
guarantees about pages on freelists. Specifically, it doesn't guarantee that
the migratetype returned by the function actually matches the migratetype of
the freelist that the page is on. Such guarantee is not its purpose and would
have negative impact on allocator performance.

The first test checks whether the freepage_migratetype equals MIGRATE_ISOLATE,
supposedly to catch races between page isolation and allocator activity. These
races should be fixed nowadays with 51bb1a4093 ("mm/page_alloc: add freepage
on isolate pageblock to correct buddy list") and related patches. As explained
above, the check wouldn't be able to catch them reliably anyway. For the same
reason false positives can happen, although they are harmless, as the
move_freepages() call would just move the page to the same freelist it's
already on. So removing the test is not a bug fix, just cleanup. After this
patch, we assume that all PageBuddy pages are on the correct freelist and that
the races were really fixed. A truly reliable verification in the form of e.g.
VM_BUG_ON() would be complicated and is arguably not needed.

The second test (page_count(page) == 0 && get_freepage_migratetype(page)
== MIGRATE_ISOLATE) is probably supposed (the code comes from a big memory
isolation patch from 2007) to catch pages on MIGRATE_ISOLATE pcplists.
However, pcplists don't contain MIGRATE_ISOLATE freepages nowadays, those are
freed directly to free lists, so the check is obsolete. Remove it as well.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Laura Abbott <lauraa@codeaurora.org>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/page_isolation.c | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index 0e69d25..9eaa489c 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -226,34 +226,16 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn,
 			continue;
 		}
 		page = pfn_to_page(pfn);
-		if (PageBuddy(page)) {
+		if (PageBuddy(page))
 			/*
-			 * If race between isolatation and allocation happens,
-			 * some free pages could be in MIGRATE_MOVABLE list
-			 * although pageblock's migratation type of the page
-			 * is MIGRATE_ISOLATE. Catch it and move the page into
-			 * MIGRATE_ISOLATE list.
+			 * If the page is on a free list, it has to be on
+			 * the correct MIGRATE_ISOLATE freelist. There is no
+			 * simple way to verify that as VM_BUG_ON(), though.
 			 */
-			if (get_freepage_migratetype(page) != MIGRATE_ISOLATE) {
-				struct page *end_page;
-
-				end_page = page + (1 << page_order(page)) - 1;
-				move_freepages(page_zone(page), page, end_page,
-						MIGRATE_ISOLATE);
-			}
 			pfn += 1 << page_order(page);
-		}
-		else if (page_count(page) == 0 &&
-			get_freepage_migratetype(page) == MIGRATE_ISOLATE)
-			pfn += 1;
-		else if (skip_hwpoisoned_pages && PageHWPoison(page)) {
-			/*
-			 * The HWPoisoned page may be not in buddy
-			 * system, and page_count() is not 0.
-			 */
+		else if (skip_hwpoisoned_pages && PageHWPoison(page))
+			/* A HWPoisoned page cannot be also PageBuddy */
 			pfn++;
-			continue;
-		}
 		else
 			break;
 	}
-- 
2.4.5

[PATCH 2/2] mm: rename and move get/set_freepage_migratetype

[Vlastimil Babka]

The pair of get/set_freepage_migratetype() functions are used to cache
pageblock migratetype for a page put on a pcplist, so that it does not have
to be retrieved again when the page is put on a free list (e.g. when pcplists
become full). Historically it was also assumed that the value is accurate for
pages on freelists (as the functions' names unfortunately suggest), but that
cannot be guaranteed without affecting various allocator fast paths. It is in
fact not needed and all such uses have been removed.

The last remaining (but pointless) usage related to pages of freelists is in
move_freepages(), which this patch removes.

To prevent further confusion, rename the functions to
get/set_pcppage_migratetype() and expand their description. Since all the
users are now in mm/page_alloc.c, move the functions there from the shared
header.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Laura Abbott <lauraa@codeaurora.org>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Johannes Weiner <hannes@cmpxchg.org>
---
 include/linux/mm.h | 12 ------------
 mm/page_alloc.c    | 42 +++++++++++++++++++++++++++++-------------
 2 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index c3a2b37..ce36145 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -310,18 +310,6 @@ struct inode;
 #define page_private(page)		((page)->private)
 #define set_page_private(page, v)	((page)->private = (v))
 
-/* It's valid only if the page is free path or free_list */
-static inline void set_freepage_migratetype(struct page *page, int migratetype)
-{
-	page->index = migratetype;
-}
-
-/* It's valid only if the page is free path or free_list */
-static inline int get_freepage_migratetype(struct page *page)
-{
-	return page->index;
-}
-
 /*
  * FIXME: take this include out, include page-flags.h in
  * files which need it (119 of them)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 41dc650..ab81150 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -125,6 +125,24 @@ unsigned long dirty_balance_reserve __read_mostly;
 int percpu_pagelist_fraction;
 gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK;
 
+/*
+ * A cached value of the page's pageblock's migratetype, used when the page is
+ * put on a pcplist. Used to avoid the pageblock migratetype lookup when
+ * freeing from pcplists in most cases, at the cost of possibly becoming stale.
+ * Also the migratetype set in the page does not necessarily match the pcplist
+ * index, e.g. page might have MIGRATE_CMA set but be on a pcplist with any
+ * other index - this ensures that it will be put on the correct CMA freelist.
+ */
+static inline int get_pcppage_migratetype(struct page *page)
+{
+	return page->index;
+}
+
+static inline void set_pcppage_migratetype(struct page *page, int migratetype)
+{
+	page->index = migratetype;
+}
+
 #ifdef CONFIG_PM_SLEEP
 /*
  * The following functions are used by the suspend/hibernate code to temporarily
@@ -789,7 +807,7 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 			page = list_entry(list->prev, struct page, lru);
 			/* must delete as __free_one_page list manipulates */
 			list_del(&page->lru);
-			mt = get_freepage_migratetype(page);
+			mt = get_pcppage_migratetype(page);
 			if (unlikely(has_isolate_pageblock(zone)))
 				mt = get_pageblock_migratetype(page);
 
@@ -959,7 +977,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
 	migratetype = get_pfnblock_migratetype(page, pfn);
 	local_irq_save(flags);
 	__count_vm_events(PGFREE, 1 << order);
-	set_freepage_migratetype(page, migratetype);
+	set_pcppage_migratetype(page, migratetype);
 	free_one_page(page_zone(page), page, pfn, order, migratetype);
 	local_irq_restore(flags);
 }
@@ -1380,7 +1398,7 @@ struct page *__rmqueue_smallest(struct zone *zone, unsigned int order,
 		rmv_page_order(page);
 		area->nr_free--;
 		expand(zone, page, order, current_order, area, migratetype);
-		set_freepage_migratetype(page, migratetype);
+		set_pcppage_migratetype(page, migratetype);
 		return page;
 	}
 
@@ -1457,7 +1475,6 @@ int move_freepages(struct zone *zone,
 		order = page_order(page);
 		list_move(&page->lru,
 			  &zone->free_area[order].free_list[migratetype]);
-		set_freepage_migratetype(page, migratetype);
 		page += 1 << order;
 		pages_moved += 1 << order;
 	}
@@ -1627,14 +1644,13 @@ __rmqueue_fallback(struct zone *zone, unsigned int order, int start_migratetype)
 		expand(zone, page, order, current_order, area,
 					start_migratetype);
 		/*
-		 * The freepage_migratetype may differ from pageblock's
+		 * The pcppage_migratetype may differ from pageblock's
 		 * migratetype depending on the decisions in
-		 * try_to_steal_freepages(). This is OK as long as it
-		 * does not differ for MIGRATE_CMA pageblocks. For CMA
-		 * we need to make sure unallocated pages flushed from
-		 * pcp lists are returned to the correct freelist.
+		 * find_suitable_fallback(). This is OK as long as it does not
+		 * differ for MIGRATE_CMA pageblocks. Those can be used as
+		 * fallback only via special __rmqueue_cma_fallback() function
 		 */
-		set_freepage_migratetype(page, start_migratetype);
+		set_pcppage_migratetype(page, start_migratetype);
 
 		trace_mm_page_alloc_extfrag(page, order, current_order,
 			start_migratetype, fallback_mt);
@@ -1710,7 +1726,7 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
 		else
 			list_add_tail(&page->lru, list);
 		list = &page->lru;
-		if (is_migrate_cma(get_freepage_migratetype(page)))
+		if (is_migrate_cma(get_pcppage_migratetype(page)))
 			__mod_zone_page_state(zone, NR_FREE_CMA_PAGES,
 					      -(1 << order));
 	}
@@ -1907,7 +1923,7 @@ void free_hot_cold_page(struct page *page, bool cold)
 		return;
 
 	migratetype = get_pfnblock_migratetype(page, pfn);
-	set_freepage_migratetype(page, migratetype);
+	set_pcppage_migratetype(page, migratetype);
 	local_irq_save(flags);
 	__count_vm_event(PGFREE);
 
@@ -2112,7 +2128,7 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
 		if (!page)
 			goto failed;
 		__mod_zone_freepage_state(zone, -(1 << order),
-					  get_freepage_migratetype(page));
+					  get_pcppage_migratetype(page));
 	}
 
 	__mod_zone_page_state(zone, NR_ALLOC_BATCH, -(1 << order));
-- 
2.4.5

Re: [PATCH 2/2] mm: rename and move get/set_freepage_migratetype

[David Rientjes]

On Tue, 21 Jul 2015, Vlastimil Babka wrote:

> The pair of get/set_freepage_migratetype() functions are used to cache
> pageblock migratetype for a page put on a pcplist, so that it does not have
> to be retrieved again when the page is put on a free list (e.g. when pcplists
> become full). Historically it was also assumed that the value is accurate for
> pages on freelists (as the functions' names unfortunately suggest), but that
> cannot be guaranteed without affecting various allocator fast paths. It is in
> fact not needed and all such uses have been removed.
> 
> The last remaining (but pointless) usage related to pages of freelists is in
> move_freepages(), which this patch removes.
> 
> To prevent further confusion, rename the functions to
> get/set_pcppage_migratetype() and expand their description. Since all the
> users are now in mm/page_alloc.c, move the functions there from the shared
> header.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Johannes Weiner <hannes@cmpxchg.org>

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH 2/2] mm: rename and move get/set_freepage_migratetype

[Vlastimil Babka]

On 07/21/2015 02:53 PM, Vlastimil Babka wrote:
> The pair of get/set_freepage_migratetype() functions are used to cache
> pageblock migratetype for a page put on a pcplist, so that it does not have
> to be retrieved again when the page is put on a free list (e.g. when pcplists
> become full). Historically it was also assumed that the value is accurate for
> pages on freelists (as the functions' names unfortunately suggest), but that
> cannot be guaranteed without affecting various allocator fast paths. It is in
> fact not needed and all such uses have been removed.
> 
> The last remaining (but pointless) usage related to pages of freelists is in
> move_freepages(), which this patch removes.

I realized there's one more callsite that can be removed. Here's
whole updated patch due to different changelog and to cope with
context changed by the fixlet to patch 1/2.

------8<------
From: Vlastimil Babka <vbabka@suse.cz>
Date: Thu, 2 Jul 2015 16:37:06 +0200
Subject: mm: rename and move get/set_freepage_migratetype

The pair of get/set_freepage_migratetype() functions are used to cache
pageblock migratetype for a page put on a pcplist, so that it does not have
to be retrieved again when the page is put on a free list (e.g. when pcplists
become full). Historically it was also assumed that the value is accurate for
pages on freelists (as the functions' names unfortunately suggest), but that
cannot be guaranteed without affecting various allocator fast paths. It is in
fact not needed and all such uses have been removed.

The last two remaining (but pointless) usages related to pages of freelists
are removed by this patch:
- move_freepages() which operates on pages already on freelists
- __free_pages_ok() which puts a page directly to freelist, bypassing pcplists

To prevent further confusion, rename the functions to
get/set_pcppage_migratetype() and expand their description. Since all the
users are now in mm/page_alloc.c, move the functions there from the shared
header.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Laura Abbott <lauraa@codeaurora.org>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Johannes Weiner <hannes@cmpxchg.org>
---
 include/linux/mm.h | 12 ------------
 mm/page_alloc.c    | 41 ++++++++++++++++++++++++++++-------------
 2 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index c3a2b37..ce36145 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -310,18 +310,6 @@ struct inode;
 #define page_private(page)		((page)->private)
 #define set_page_private(page, v)	((page)->private = (v))
 
-/* It's valid only if the page is free path or free_list */
-static inline void set_freepage_migratetype(struct page *page, int migratetype)
-{
-	page->index = migratetype;
-}
-
-/* It's valid only if the page is free path or free_list */
-static inline int get_freepage_migratetype(struct page *page)
-{
-	return page->index;
-}
-
 /*
  * FIXME: take this include out, include page-flags.h in
  * files which need it (119 of them)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c61fef8..4b220cb 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -125,6 +125,24 @@ unsigned long dirty_balance_reserve __read_mostly;
 int percpu_pagelist_fraction;
 gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK;
 
+/*
+ * A cached value of the page's pageblock's migratetype, used when the page is
+ * put on a pcplist. Used to avoid the pageblock migratetype lookup when
+ * freeing from pcplists in most cases, at the cost of possibly becoming stale.
+ * Also the migratetype set in the page does not necessarily match the pcplist
+ * index, e.g. page might have MIGRATE_CMA set but be on a pcplist with any
+ * other index - this ensures that it will be put on the correct CMA freelist.
+ */
+static inline int get_pcppage_migratetype(struct page *page)
+{
+	return page->index;
+}
+
+static inline void set_pcppage_migratetype(struct page *page, int migratetype)
+{
+	page->index = migratetype;
+}
+
 #ifdef CONFIG_PM_SLEEP
 /*
  * The following functions are used by the suspend/hibernate code to temporarily
@@ -790,7 +808,7 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 			/* must delete as __free_one_page list manipulates */
 			list_del(&page->lru);
 
-			mt = get_freepage_migratetype(page);
+			mt = get_pcppage_migratetype(page);
 			/* MIGRATE_ISOLATE page should not go to pcplists */
 			VM_BUG_ON_PAGE(is_migrate_isolate(mt), page);
 			/* Pageblock could have been isolated meanwhile */
@@ -963,7 +981,6 @@ static void __free_pages_ok(struct page *page, unsigned int order)
 	migratetype = get_pfnblock_migratetype(page, pfn);
 	local_irq_save(flags);
 	__count_vm_events(PGFREE, 1 << order);
-	set_freepage_migratetype(page, migratetype);
 	free_one_page(page_zone(page), page, pfn, order, migratetype);
 	local_irq_restore(flags);
 }
@@ -1384,7 +1401,7 @@ struct page *__rmqueue_smallest(struct zone *zone, unsigned int order,
 		rmv_page_order(page);
 		area->nr_free--;
 		expand(zone, page, order, current_order, area, migratetype);
-		set_freepage_migratetype(page, migratetype);
+		set_pcppage_migratetype(page, migratetype);
 		return page;
 	}
 
@@ -1461,7 +1478,6 @@ int move_freepages(struct zone *zone,
 		order = page_order(page);
 		list_move(&page->lru,
 			  &zone->free_area[order].free_list[migratetype]);
-		set_freepage_migratetype(page, migratetype);
 		page += 1 << order;
 		pages_moved += 1 << order;
 	}
@@ -1631,14 +1647,13 @@ __rmqueue_fallback(struct zone *zone, unsigned int order, int start_migratetype)
 		expand(zone, page, order, current_order, area,
 					start_migratetype);
 		/*
-		 * The freepage_migratetype may differ from pageblock's
+		 * The pcppage_migratetype may differ from pageblock's
 		 * migratetype depending on the decisions in
-		 * try_to_steal_freepages(). This is OK as long as it
-		 * does not differ for MIGRATE_CMA pageblocks. For CMA
-		 * we need to make sure unallocated pages flushed from
-		 * pcp lists are returned to the correct freelist.
+		 * find_suitable_fallback(). This is OK as long as it does not
+		 * differ for MIGRATE_CMA pageblocks. Those can be used as
+		 * fallback only via special __rmqueue_cma_fallback() function
 		 */
-		set_freepage_migratetype(page, start_migratetype);
+		set_pcppage_migratetype(page, start_migratetype);
 
 		trace_mm_page_alloc_extfrag(page, order, current_order,
 			start_migratetype, fallback_mt);
@@ -1714,7 +1729,7 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
 		else
 			list_add_tail(&page->lru, list);
 		list = &page->lru;
-		if (is_migrate_cma(get_freepage_migratetype(page)))
+		if (is_migrate_cma(get_pcppage_migratetype(page)))
 			__mod_zone_page_state(zone, NR_FREE_CMA_PAGES,
 					      -(1 << order));
 	}
@@ -1911,7 +1926,7 @@ void free_hot_cold_page(struct page *page, bool cold)
 		return;
 
 	migratetype = get_pfnblock_migratetype(page, pfn);
-	set_freepage_migratetype(page, migratetype);
+	set_pcppage_migratetype(page, migratetype);
 	local_irq_save(flags);
 	__count_vm_event(PGFREE);
 
@@ -2116,7 +2131,7 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
 		if (!page)
 			goto failed;
 		__mod_zone_freepage_state(zone, -(1 << order),
-					  get_freepage_migratetype(page));
+					  get_pcppage_migratetype(page));
 	}
 
 	__mod_zone_page_state(zone, NR_ALLOC_BATCH, -(1 << order));
-- 
2.4.5

Re: [PATCH 2/2] mm: rename and move get/set_freepage_migratetype

[Joonsoo Kim]

On Wed, Jul 22, 2015 at 02:29:08PM +0200, Vlastimil Babka wrote:
> On 07/21/2015 02:53 PM, Vlastimil Babka wrote:
> > The pair of get/set_freepage_migratetype() functions are used to cache
> > pageblock migratetype for a page put on a pcplist, so that it does not have
> > to be retrieved again when the page is put on a free list (e.g. when pcplists
> > become full). Historically it was also assumed that the value is accurate for
> > pages on freelists (as the functions' names unfortunately suggest), but that
> > cannot be guaranteed without affecting various allocator fast paths. It is in
> > fact not needed and all such uses have been removed.
> > 
> > The last remaining (but pointless) usage related to pages of freelists is in
> > move_freepages(), which this patch removes.
> 
> I realized there's one more callsite that can be removed. Here's
> whole updated patch due to different changelog and to cope with
> context changed by the fixlet to patch 1/2.
> 
> ------8<------
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 2 Jul 2015 16:37:06 +0200
> Subject: mm: rename and move get/set_freepage_migratetype
> 
> The pair of get/set_freepage_migratetype() functions are used to cache
> pageblock migratetype for a page put on a pcplist, so that it does not have
> to be retrieved again when the page is put on a free list (e.g. when pcplists
> become full). Historically it was also assumed that the value is accurate for
> pages on freelists (as the functions' names unfortunately suggest), but that
> cannot be guaranteed without affecting various allocator fast paths. It is in
> fact not needed and all such uses have been removed.
> 
> The last two remaining (but pointless) usages related to pages of freelists
> are removed by this patch:
> - move_freepages() which operates on pages already on freelists
> - __free_pages_ok() which puts a page directly to freelist, bypassing pcplists
> 
> To prevent further confusion, rename the functions to
> get/set_pcppage_migratetype() and expand their description. Since all the
> users are now in mm/page_alloc.c, move the functions there from the shared
> header.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: David Rientjes <rientjes@google.com>

Acked-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Thanks.

Re: [PATCH 2/2] mm: rename and move get/set_freepage_migratetype

[Naoya Horiguchi]

On Wed, Jul 22, 2015 at 02:29:08PM +0200, Vlastimil Babka wrote:
> On 07/21/2015 02:53 PM, Vlastimil Babka wrote:
> > The pair of get/set_freepage_migratetype() functions are used to cache
> > pageblock migratetype for a page put on a pcplist, so that it does not have
> > to be retrieved again when the page is put on a free list (e.g. when pcplists
> > become full). Historically it was also assumed that the value is accurate for
> > pages on freelists (as the functions' names unfortunately suggest), but that
> > cannot be guaranteed without affecting various allocator fast paths. It is in
> > fact not needed and all such uses have been removed.
> > 
> > The last remaining (but pointless) usage related to pages of freelists is in
> > move_freepages(), which this patch removes.
> 
> I realized there's one more callsite that can be removed. Here's
> whole updated patch due to different changelog and to cope with
> context changed by the fixlet to patch 1/2.
> 
> ------8<------
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 2 Jul 2015 16:37:06 +0200
> Subject: mm: rename and move get/set_freepage_migratetype
> 
> The pair of get/set_freepage_migratetype() functions are used to cache
> pageblock migratetype for a page put on a pcplist, so that it does not have
> to be retrieved again when the page is put on a free list (e.g. when pcplists
> become full). Historically it was also assumed that the value is accurate for
> pages on freelists (as the functions' names unfortunately suggest), but that
> cannot be guaranteed without affecting various allocator fast paths. It is in
> fact not needed and all such uses have been removed.
> 
> The last two remaining (but pointless) usages related to pages of freelists
> are removed by this patch:
> - move_freepages() which operates on pages already on freelists
> - __free_pages_ok() which puts a page directly to freelist, bypassing pcplists
> 
> To prevent further confusion, rename the functions to
> get/set_pcppage_migratetype() and expand their description. Since all the
> users are now in mm/page_alloc.c, move the functions there from the shared
> header.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Johannes Weiner <hannes@cmpxchg.org>

Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

Re: [PATCH 2/2] mm: rename and move get/set_freepage_migratetype

[Mel Gorman]

On Wed, Jul 22, 2015 at 02:29:08PM +0200, Vlastimil Babka wrote:
> Subject: mm: rename and move get/set_freepage_migratetype
> 
> The pair of get/set_freepage_migratetype() functions are used to cache
> pageblock migratetype for a page put on a pcplist, so that it does not have
> to be retrieved again when the page is put on a free list (e.g. when pcplists
> become full). Historically it was also assumed that the value is accurate for
> pages on freelists (as the functions' names unfortunately suggest), but that
> cannot be guaranteed without affecting various allocator fast paths. It is in
> fact not needed and all such uses have been removed.
> 
> The last two remaining (but pointless) usages related to pages of freelists
> are removed by this patch:
> - move_freepages() which operates on pages already on freelists
> - __free_pages_ok() which puts a page directly to freelist, bypassing pcplists
> 
> To prevent further confusion, rename the functions to
> get/set_pcppage_migratetype() and expand their description. Since all the
> users are now in mm/page_alloc.c, move the functions there from the shared
> header.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: David Rientjes <rientjes@google.com>

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 2/2] mm: rename and move get/set_freepage_migratetype

[Michal Nazarewicz]

On Wed, Jul 22 2015, Vlastimil Babka wrote:
> Subject: mm: rename and move get/set_freepage_migratetype
>
> The pair of get/set_freepage_migratetype() functions are used to cache
> pageblock migratetype for a page put on a pcplist, so that it does not have
> to be retrieved again when the page is put on a free list (e.g. when pcplists
> become full). Historically it was also assumed that the value is accurate for
> pages on freelists (as the functions' names unfortunately suggest), but that
> cannot be guaranteed without affecting various allocator fast paths. It is in
> fact not needed and all such uses have been removed.
>
> The last two remaining (but pointless) usages related to pages of freelists
> are removed by this patch:
> - move_freepages() which operates on pages already on freelists
> - __free_pages_ok() which puts a page directly to freelist, bypassing pcplists
>
> To prevent further confusion, rename the functions to
> get/set_pcppage_migratetype() and expand their description. Since all the
> users are now in mm/page_alloc.c, move the functions there from the shared
> header.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Nazarewicz <mina86@mina86.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  include/linux/mm.h | 12 ------------
>  mm/page_alloc.c    | 41 ++++++++++++++++++++++++++++-------------
>  2 files changed, 28 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index c3a2b37..ce36145 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -310,18 +310,6 @@ struct inode;
>  #define page_private(page)		((page)->private)
>  #define set_page_private(page, v)	((page)->private = (v))
>  
> -/* It's valid only if the page is free path or free_list */
> -static inline void set_freepage_migratetype(struct page *page, int migratetype)
> -{
> -	page->index = migratetype;
> -}
> -
> -/* It's valid only if the page is free path or free_list */
> -static inline int get_freepage_migratetype(struct page *page)
> -{
> -	return page->index;
> -}
> -
>  /*
>   * FIXME: take this include out, include page-flags.h in
>   * files which need it (119 of them)
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c61fef8..4b220cb 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -125,6 +125,24 @@ unsigned long dirty_balance_reserve __read_mostly;
>  int percpu_pagelist_fraction;
>  gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK;
>  
> +/*
> + * A cached value of the page's pageblock's migratetype, used when the page is
> + * put on a pcplist. Used to avoid the pageblock migratetype lookup when
> + * freeing from pcplists in most cases, at the cost of possibly becoming stale.
> + * Also the migratetype set in the page does not necessarily match the pcplist
> + * index, e.g. page might have MIGRATE_CMA set but be on a pcplist with any
> + * other index - this ensures that it will be put on the correct CMA freelist.
> + */
> +static inline int get_pcppage_migratetype(struct page *page)
> +{
> +	return page->index;
> +}
> +
> +static inline void set_pcppage_migratetype(struct page *page, int migratetype)
> +{
> +	page->index = migratetype;
> +}
> +
>  #ifdef CONFIG_PM_SLEEP
>  /*
>   * The following functions are used by the suspend/hibernate code to temporarily
> @@ -790,7 +808,7 @@ static void free_pcppages_bulk(struct zone *zone, int count,
>  			/* must delete as __free_one_page list manipulates */
>  			list_del(&page->lru);
>  
> -			mt = get_freepage_migratetype(page);
> +			mt = get_pcppage_migratetype(page);
>  			/* MIGRATE_ISOLATE page should not go to pcplists */
>  			VM_BUG_ON_PAGE(is_migrate_isolate(mt), page);
>  			/* Pageblock could have been isolated meanwhile */
> @@ -963,7 +981,6 @@ static void __free_pages_ok(struct page *page, unsigned int order)
>  	migratetype = get_pfnblock_migratetype(page, pfn);
>  	local_irq_save(flags);
>  	__count_vm_events(PGFREE, 1 << order);
> -	set_freepage_migratetype(page, migratetype);
>  	free_one_page(page_zone(page), page, pfn, order, migratetype);
>  	local_irq_restore(flags);
>  }
> @@ -1384,7 +1401,7 @@ struct page *__rmqueue_smallest(struct zone *zone, unsigned int order,
>  		rmv_page_order(page);
>  		area->nr_free--;
>  		expand(zone, page, order, current_order, area, migratetype);
> -		set_freepage_migratetype(page, migratetype);
> +		set_pcppage_migratetype(page, migratetype);
>  		return page;
>  	}
>  
> @@ -1461,7 +1478,6 @@ int move_freepages(struct zone *zone,
>  		order = page_order(page);
>  		list_move(&page->lru,
>  			  &zone->free_area[order].free_list[migratetype]);
> -		set_freepage_migratetype(page, migratetype);
>  		page += 1 << order;
>  		pages_moved += 1 << order;
>  	}
> @@ -1631,14 +1647,13 @@ __rmqueue_fallback(struct zone *zone, unsigned int order, int start_migratetype)
>  		expand(zone, page, order, current_order, area,
>  					start_migratetype);
>  		/*
> -		 * The freepage_migratetype may differ from pageblock's
> +		 * The pcppage_migratetype may differ from pageblock's
>  		 * migratetype depending on the decisions in
> -		 * try_to_steal_freepages(). This is OK as long as it
> -		 * does not differ for MIGRATE_CMA pageblocks. For CMA
> -		 * we need to make sure unallocated pages flushed from
> -		 * pcp lists are returned to the correct freelist.
> +		 * find_suitable_fallback(). This is OK as long as it does not
> +		 * differ for MIGRATE_CMA pageblocks. Those can be used as
> +		 * fallback only via special __rmqueue_cma_fallback() function
>  		 */
> -		set_freepage_migratetype(page, start_migratetype);
> +		set_pcppage_migratetype(page, start_migratetype);
>  
>  		trace_mm_page_alloc_extfrag(page, order, current_order,
>  			start_migratetype, fallback_mt);
> @@ -1714,7 +1729,7 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
>  		else
>  			list_add_tail(&page->lru, list);
>  		list = &page->lru;
> -		if (is_migrate_cma(get_freepage_migratetype(page)))
> +		if (is_migrate_cma(get_pcppage_migratetype(page)))
>  			__mod_zone_page_state(zone, NR_FREE_CMA_PAGES,
>  					      -(1 << order));
>  	}
> @@ -1911,7 +1926,7 @@ void free_hot_cold_page(struct page *page, bool cold)
>  		return;
>  
>  	migratetype = get_pfnblock_migratetype(page, pfn);
> -	set_freepage_migratetype(page, migratetype);
> +	set_pcppage_migratetype(page, migratetype);
>  	local_irq_save(flags);
>  	__count_vm_event(PGFREE);
>  
> @@ -2116,7 +2131,7 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
>  		if (!page)
>  			goto failed;
>  		__mod_zone_freepage_state(zone, -(1 << order),
> -					  get_freepage_migratetype(page));
> +					  get_pcppage_migratetype(page));
>  	}
>  
>  	__mod_zone_page_state(zone, NR_ALLOC_BATCH, -(1 << order));
> -- 
> 2.4.5
>
>
>

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

Re: [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages

[David Rientjes]

On Tue, 21 Jul 2015, Vlastimil Babka wrote:

> The __test_page_isolated_in_pageblock() is used to verify whether all pages
> in pageblock were either successfully isolated, or are hwpoisoned. Two of the
> possible state of pages, that are tested, are however bogus and misleading.
> 
> Both tests rely on get_freepage_migratetype(page), which however has no
> guarantees about pages on freelists. Specifically, it doesn't guarantee that
> the migratetype returned by the function actually matches the migratetype of
> the freelist that the page is on. Such guarantee is not its purpose and would
> have negative impact on allocator performance.
> 
> The first test checks whether the freepage_migratetype equals MIGRATE_ISOLATE,
> supposedly to catch races between page isolation and allocator activity. These
> races should be fixed nowadays with 51bb1a4093 ("mm/page_alloc: add freepage
> on isolate pageblock to correct buddy list") and related patches. As explained
> above, the check wouldn't be able to catch them reliably anyway. For the same
> reason false positives can happen, although they are harmless, as the
> move_freepages() call would just move the page to the same freelist it's
> already on. So removing the test is not a bug fix, just cleanup. After this
> patch, we assume that all PageBuddy pages are on the correct freelist and that
> the races were really fixed. A truly reliable verification in the form of e.g.
> VM_BUG_ON() would be complicated and is arguably not needed.
> 
> The second test (page_count(page) == 0 && get_freepage_migratetype(page)
> == MIGRATE_ISOLATE) is probably supposed (the code comes from a big memory
> isolation patch from 2007) to catch pages on MIGRATE_ISOLATE pcplists.
> However, pcplists don't contain MIGRATE_ISOLATE freepages nowadays, those are
> freed directly to free lists, so the check is obsolete. Remove it as well.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/page_isolation.c | 30 ++++++------------------------
>  1 file changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index 0e69d25..9eaa489c 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -226,34 +226,16 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn,
>  			continue;
>  		}
>  		page = pfn_to_page(pfn);
> -		if (PageBuddy(page)) {
> +		if (PageBuddy(page))
>  			/*
> -			 * If race between isolatation and allocation happens,
> -			 * some free pages could be in MIGRATE_MOVABLE list
> -			 * although pageblock's migratation type of the page
> -			 * is MIGRATE_ISOLATE. Catch it and move the page into
> -			 * MIGRATE_ISOLATE list.
> +			 * If the page is on a free list, it has to be on
> +			 * the correct MIGRATE_ISOLATE freelist. There is no
> +			 * simple way to verify that as VM_BUG_ON(), though.
>  			 */
> -			if (get_freepage_migratetype(page) != MIGRATE_ISOLATE) {
> -				struct page *end_page;
> -
> -				end_page = page + (1 << page_order(page)) - 1;
> -				move_freepages(page_zone(page), page, end_page,
> -						MIGRATE_ISOLATE);
> -			}
>  			pfn += 1 << page_order(page);
> -		}
> -		else if (page_count(page) == 0 &&
> -			get_freepage_migratetype(page) == MIGRATE_ISOLATE)
> -			pfn += 1;
> -		else if (skip_hwpoisoned_pages && PageHWPoison(page)) {
> -			/*
> -			 * The HWPoisoned page may be not in buddy
> -			 * system, and page_count() is not 0.
> -			 */
> +		else if (skip_hwpoisoned_pages && PageHWPoison(page))
> +			/* A HWPoisoned page cannot be also PageBuddy */
>  			pfn++;
> -			continue;
> -		}
>  		else
>  			break;
>  	}

You may want to consider stating your assumptions explicitly in the code, 
perhaps with VM_BUG_ON(), such as in free_pcppages_bulk() to ensure things 
like get_freepage_migratetype(page) != MIGRATE_ISOLATE.

Re: [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages

[Vlastimil Babka]

On 07/22/2015 12:43 AM, David Rientjes wrote:
> On Tue, 21 Jul 2015, Vlastimil Babka wrote:
> 
> 
> You may want to consider stating your assumptions explicitly in the code,
> perhaps with VM_BUG_ON(), such as in free_pcppages_bulk() to ensure things
> like get_freepage_migratetype(page) != MIGRATE_ISOLATE.

Hm, OK here's a fixup. I've pondered others but nothing made sense
unless I would have to devise really twisted ways in which somebody
broke the code in the future, and that's not worth BUG_ON().

But the checking made me realize that one more
set_freepage_migratetype() can be removed in the other patch, so I
will resend it.

------8<------
From: Vlastimil Babka <vbabka@suse.cz>
Date: Wed, 22 Jul 2015 14:16:52 +0200
Subject: [PATCH 2/3] fixup! mm, page_isolation: remove bogus tests for
 isolated pages

---
 mm/page_alloc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 41dc650..c61fef8 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -789,7 +789,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 			page = list_entry(list->prev, struct page, lru);
 			/* must delete as __free_one_page list manipulates */
 			list_del(&page->lru);
+
 			mt = get_freepage_migratetype(page);
+			/* MIGRATE_ISOLATE page should not go to pcplists */
+			VM_BUG_ON_PAGE(is_migrate_isolate(mt), page);
+			/* Pageblock could have been isolated meanwhile */
 			if (unlikely(has_isolate_pageblock(zone)))
 				mt = get_pageblock_migratetype(page);
 
-- 
2.4.5

Re: [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages

[David Rientjes]

On Wed, 22 Jul 2015, Vlastimil Babka wrote:

> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Wed, 22 Jul 2015 14:16:52 +0200
> Subject: [PATCH 2/3] fixup! mm, page_isolation: remove bogus tests for
>  isolated pages
> 
> ---
>  mm/page_alloc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 41dc650..c61fef8 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -789,7 +789,11 @@ static void free_pcppages_bulk(struct zone *zone, int count,
>  			page = list_entry(list->prev, struct page, lru);
>  			/* must delete as __free_one_page list manipulates */
>  			list_del(&page->lru);
> +
>  			mt = get_freepage_migratetype(page);
> +			/* MIGRATE_ISOLATE page should not go to pcplists */
> +			VM_BUG_ON_PAGE(is_migrate_isolate(mt), page);
> +			/* Pageblock could have been isolated meanwhile */
>  			if (unlikely(has_isolate_pageblock(zone)))
>  				mt = get_pageblock_migratetype(page);
>  

Looks good, thanks!

Re: [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages

[Joonsoo Kim]

On Tue, Jul 21, 2015 at 02:53:37PM +0200, Vlastimil Babka wrote:
> The __test_page_isolated_in_pageblock() is used to verify whether all pages
> in pageblock were either successfully isolated, or are hwpoisoned. Two of the
> possible state of pages, that are tested, are however bogus and misleading.
> 
> Both tests rely on get_freepage_migratetype(page), which however has no
> guarantees about pages on freelists. Specifically, it doesn't guarantee that
> the migratetype returned by the function actually matches the migratetype of
> the freelist that the page is on. Such guarantee is not its purpose and would
> have negative impact on allocator performance.
> 
> The first test checks whether the freepage_migratetype equals MIGRATE_ISOLATE,
> supposedly to catch races between page isolation and allocator activity. These
> races should be fixed nowadays with 51bb1a4093 ("mm/page_alloc: add freepage
> on isolate pageblock to correct buddy list") and related patches. As explained
> above, the check wouldn't be able to catch them reliably anyway. For the same
> reason false positives can happen, although they are harmless, as the
> move_freepages() call would just move the page to the same freelist it's
> already on. So removing the test is not a bug fix, just cleanup. After this
> patch, we assume that all PageBuddy pages are on the correct freelist and that
> the races were really fixed. A truly reliable verification in the form of e.g.
> VM_BUG_ON() would be complicated and is arguably not needed.
> 
> The second test (page_count(page) == 0 && get_freepage_migratetype(page)
> == MIGRATE_ISOLATE) is probably supposed (the code comes from a big memory
> isolation patch from 2007) to catch pages on MIGRATE_ISOLATE pcplists.
> However, pcplists don't contain MIGRATE_ISOLATE freepages nowadays, those are
> freed directly to free lists, so the check is obsolete. Remove it as well.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Thanks for taking care of this.

Thanks.

Re: [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages

[Naoya Horiguchi]

On Tue, Jul 21, 2015 at 02:53:37PM +0200, Vlastimil Babka wrote:
> The __test_page_isolated_in_pageblock() is used to verify whether all pages
> in pageblock were either successfully isolated, or are hwpoisoned. Two of the
> possible state of pages, that are tested, are however bogus and misleading.
> 
> Both tests rely on get_freepage_migratetype(page), which however has no
> guarantees about pages on freelists. Specifically, it doesn't guarantee that
> the migratetype returned by the function actually matches the migratetype of
> the freelist that the page is on. Such guarantee is not its purpose and would
> have negative impact on allocator performance.
> 
> The first test checks whether the freepage_migratetype equals MIGRATE_ISOLATE,
> supposedly to catch races between page isolation and allocator activity. These
> races should be fixed nowadays with 51bb1a4093 ("mm/page_alloc: add freepage
> on isolate pageblock to correct buddy list") and related patches. As explained
> above, the check wouldn't be able to catch them reliably anyway. For the same
> reason false positives can happen, although they are harmless, as the
> move_freepages() call would just move the page to the same freelist it's
> already on. So removing the test is not a bug fix, just cleanup. After this
> patch, we assume that all PageBuddy pages are on the correct freelist and that
> the races were really fixed. A truly reliable verification in the form of e.g.
> VM_BUG_ON() would be complicated and is arguably not needed.
> 
> The second test (page_count(page) == 0 && get_freepage_migratetype(page)
> == MIGRATE_ISOLATE) is probably supposed (the code comes from a big memory
> isolation patch from 2007) to catch pages on MIGRATE_ISOLATE pcplists.
> However, pcplists don't contain MIGRATE_ISOLATE freepages nowadays, those are
> freed directly to free lists, so the check is obsolete. Remove it as well.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

Looks good to me.

Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

Re: [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages

[Mel Gorman]

On Tue, Jul 21, 2015 at 02:53:37PM +0200, Vlastimil Babka wrote:
> The __test_page_isolated_in_pageblock() is used to verify whether all pages
> in pageblock were either successfully isolated, or are hwpoisoned. Two of the
> possible state of pages, that are tested, are however bogus and misleading.
> 
> Both tests rely on get_freepage_migratetype(page), which however has no
> guarantees about pages on freelists. Specifically, it doesn't guarantee that
> the migratetype returned by the function actually matches the migratetype of
> the freelist that the page is on. Such guarantee is not its purpose and would
> have negative impact on allocator performance.
> 
> The first test checks whether the freepage_migratetype equals MIGRATE_ISOLATE,
> supposedly to catch races between page isolation and allocator activity. These
> races should be fixed nowadays with 51bb1a4093 ("mm/page_alloc: add freepage
> on isolate pageblock to correct buddy list") and related patches. As explained
> above, the check wouldn't be able to catch them reliably anyway. For the same
> reason false positives can happen, although they are harmless, as the
> move_freepages() call would just move the page to the same freelist it's
> already on. So removing the test is not a bug fix, just cleanup. After this
> patch, we assume that all PageBuddy pages are on the correct freelist and that
> the races were really fixed. A truly reliable verification in the form of e.g.
> VM_BUG_ON() would be complicated and is arguably not needed.
> 
> The second test (page_count(page) == 0 && get_freepage_migratetype(page)
> == MIGRATE_ISOLATE) is probably supposed (the code comes from a big memory
> isolation patch from 2007) to catch pages on MIGRATE_ISOLATE pcplists.
> However, pcplists don't contain MIGRATE_ISOLATE freepages nowadays, those are
> freed directly to free lists, so the check is obsolete. Remove it as well.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 1/2] mm, page_isolation: remove bogus tests for isolated pages

[Michal Nazarewicz]

On Tue, Jul 21 2015, Vlastimil Babka wrote:
> The __test_page_isolated_in_pageblock() is used to verify whether all pages
> in pageblock were either successfully isolated, or are hwpoisoned. Two of the
> possible state of pages, that are tested, are however bogus and misleading.
>
> Both tests rely on get_freepage_migratetype(page), which however has no
> guarantees about pages on freelists. Specifically, it doesn't guarantee that
> the migratetype returned by the function actually matches the migratetype of
> the freelist that the page is on. Such guarantee is not its purpose and would
> have negative impact on allocator performance.
>
> The first test checks whether the freepage_migratetype equals MIGRATE_ISOLATE,
> supposedly to catch races between page isolation and allocator activity. These
> races should be fixed nowadays with 51bb1a4093 ("mm/page_alloc: add freepage
> on isolate pageblock to correct buddy list") and related patches. As explained
> above, the check wouldn't be able to catch them reliably anyway. For the same
> reason false positives can happen, although they are harmless, as the
> move_freepages() call would just move the page to the same freelist it's
> already on. So removing the test is not a bug fix, just cleanup. After this
> patch, we assume that all PageBuddy pages are on the correct freelist and that
> the races were really fixed. A truly reliable verification in the form of e.g.
> VM_BUG_ON() would be complicated and is arguably not needed.
>
> The second test (page_count(page) == 0 && get_freepage_migratetype(page)
> == MIGRATE_ISOLATE) is probably supposed (the code comes from a big memory
> isolation patch from 2007) to catch pages on MIGRATE_ISOLATE pcplists.
> However, pcplists don't contain MIGRATE_ISOLATE freepages nowadays, those are
> freed directly to free lists, so the check is obsolete. Remove it as well.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Nazarewicz <mina86@mina86.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> Cc: Laura Abbott <lauraa@codeaurora.org>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/page_isolation.c | 30 ++++++------------------------
>  1 file changed, 6 insertions(+), 24 deletions(-)
>
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index 0e69d25..9eaa489c 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -226,34 +226,16 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn,
>  			continue;
>  		}
>  		page = pfn_to_page(pfn);
> -		if (PageBuddy(page)) {
> +		if (PageBuddy(page))
>  			/*
> -			 * If race between isolatation and allocation happens,
> -			 * some free pages could be in MIGRATE_MOVABLE list
> -			 * although pageblock's migratation type of the page
> -			 * is MIGRATE_ISOLATE. Catch it and move the page into
> -			 * MIGRATE_ISOLATE list.
> +			 * If the page is on a free list, it has to be on
> +			 * the correct MIGRATE_ISOLATE freelist. There is no
> +			 * simple way to verify that as VM_BUG_ON(), though.
>  			 */
> -			if (get_freepage_migratetype(page) != MIGRATE_ISOLATE) {
> -				struct page *end_page;
> -
> -				end_page = page + (1 << page_order(page)) - 1;
> -				move_freepages(page_zone(page), page, end_page,
> -						MIGRATE_ISOLATE);
> -			}
>  			pfn += 1 << page_order(page);
> -		}
> -		else if (page_count(page) == 0 &&
> -			get_freepage_migratetype(page) == MIGRATE_ISOLATE)
> -			pfn += 1;
> -		else if (skip_hwpoisoned_pages && PageHWPoison(page)) {
> -			/*
> -			 * The HWPoisoned page may be not in buddy
> -			 * system, and page_count() is not 0.
> -			 */
> +		else if (skip_hwpoisoned_pages && PageHWPoison(page))
> +			/* A HWPoisoned page cannot be also PageBuddy */
>  			pfn++;
> -			continue;
> -		}
>  		else
>  			break;
>  	}
> -- 
> 2.4.5
>

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--