[PATCH 1/2] mm: Restructure free-page stealing code and fix a bug

[PATCH 1/2] mm: Restructure free-page stealing code and fix a bug

[Srivatsa S. Bhat]

The free-page stealing code in __rmqueue_fallback() is somewhat hard to
follow, and has an incredible amount of subtlety hidden inside!

First off, there is a minor bug in the reporting of change-of-ownership of
pageblocks. Under some conditions, we try to move upto 'pageblock_nr_pages'
no. of pages to the preferred allocation list. But we change the ownership
of that pageblock to the preferred type only if we manage to successfully
move atleast half of that pageblock (or if page_group_by_mobility_disabled
is set).

However, the current code ignores the latter part and sets the 'migratetype'
variable to the preferred type, irrespective of whether we actually changed
the pageblock migratetype of that block or not. So, the page_alloc_extfrag
tracepoint can end up printing incorrect info (i.e., 'change_ownership'
might be shown as 1 when it must have been 0).

So fixing this involves moving the update of the 'migratetype' variable to
the right place. But looking closer, we observe that the 'migratetype' variable
is used subsequently for checks such as "is_migrate_cma()". Obviously the
intent there is to check if the *fallback* type is MIGRATE_CMA, but since we
already set the 'migratetype' variable to start_migratetype, we end up checking
if the *preferred* type is MIGRATE_CMA!!

To make things more interesting, this actually doesn't cause a bug in practice,
because we never change *anything* if the fallback type is CMA.

So, restructure the code in such a way that it is trivial to understand what
is going on, and also fix the above mentioned bug. And while at it, also add a
comment explaining the subtlety behind the migratetype used in the call to
expand().

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 mm/page_alloc.c |   96 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 60 insertions(+), 36 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b100255..027d417 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1007,6 +1007,53 @@ static void change_pageblock_range(struct page *pageblock_page,
 	}
 }
 
+/*
+ * If breaking a large block of pages, move all free pages to the preferred
+ * allocation list. If falling back for a reclaimable kernel allocation, be
+ * more aggressive about taking ownership of free pages.
+ *
+ * On the other hand, never change migration type of MIGRATE_CMA pageblocks
+ * nor move CMA pages to different free lists. We don't want unmovable pages
+ * to be allocated from MIGRATE_CMA areas.
+ *
+ * Returns the new migratetype of the pageblock (or the same old migratetype
+ * if it was unchanged).
+ */
+static inline int try_to_steal_freepages(struct zone *zone, struct page *page,
+					 int start_type, int fallback_type)
+{
+	int current_order = page_order(page);
+
+	if (is_migrate_cma(fallback_type))
+		return fallback_type;
+
+	/* Take ownership for orders >= pageblock_order */
+	if (current_order >= pageblock_order) {
+		change_pageblock_range(page, current_order, start_type);
+		return start_type;
+	}
+
+	if (current_order >= pageblock_order / 2 ||
+	    start_type == MIGRATE_RECLAIMABLE ||
+	    page_group_by_mobility_disabled) {
+
+		int pages;
+
+		pages = move_freepages_block(zone, page, start_type);
+
+		/* Claim the whole block if over half of it is free */
+		if (pages >= (1 << (pageblock_order-1)) ||
+				page_group_by_mobility_disabled) {
+
+			set_pageblock_migratetype(page, start_type);
+			return start_type;
+		}
+
+	}
+
+	return fallback_type;
+}
+
 /* Remove an element from the buddy allocator from the fallback list */
 static inline struct page *
 __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
@@ -1014,7 +1061,7 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
 	struct free_area * area;
 	int current_order;
 	struct page *page;
-	int migratetype, i;
+	int migratetype, new_type, i;
 
 	/* Find the largest possible block of pages in the other list */
 	for (current_order = MAX_ORDER-1; current_order >= order;
@@ -1034,51 +1081,28 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
 					struct page, lru);
 			area->nr_free--;
 
-			/*
-			 * If breaking a large block of pages, move all free
-			 * pages to the preferred allocation list. If falling
-			 * back for a reclaimable kernel allocation, be more
-			 * aggressive about taking ownership of free pages
-			 *
-			 * On the other hand, never change migration
-			 * type of MIGRATE_CMA pageblocks nor move CMA
-			 * pages on different free lists. We don't
-			 * want unmovable pages to be allocated from
-			 * MIGRATE_CMA areas.
-			 */
-			if (!is_migrate_cma(migratetype) &&
-			    (current_order >= pageblock_order / 2 ||
-			     start_migratetype == MIGRATE_RECLAIMABLE ||
-			     page_group_by_mobility_disabled)) {
-				int pages;
-				pages = move_freepages_block(zone, page,
-								start_migratetype);
-
-				/* Claim the whole block if over half of it is free */
-				if (pages >= (1 << (pageblock_order-1)) ||
-						page_group_by_mobility_disabled)
-					set_pageblock_migratetype(page,
-								start_migratetype);
-
-				migratetype = start_migratetype;
-			}
+			new_type = try_to_steal_freepages(zone, page,
+							  start_migratetype,
+							  migratetype);
 
 			/* Remove the page from the freelists */
 			list_del(&page->lru);
 			rmv_page_order(page);
 
-			/* Take ownership for orders >= pageblock_order */
-			if (current_order >= pageblock_order &&
-			    !is_migrate_cma(migratetype))
-				change_pageblock_range(page, current_order,
-							start_migratetype);
-
+			/*
+			 * Borrow the excess buddy pages as well, irrespective
+			 * of whether we stole freepages, or took ownership of
+			 * the pageblock or not.
+			 *
+			 * Exception: When borrowing from MIGRATE_CMA, release
+			 * the excess buddy pages to CMA itself.
+			 */
 			expand(zone, page, order, current_order, area,
 			       is_migrate_cma(migratetype)
 			     ? migratetype : start_migratetype);
 
 			trace_mm_page_alloc_extfrag(page, order, current_order,
-				start_migratetype, migratetype);
+				start_migratetype, new_type);
 
 			return page;
 		}

--
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 2/2] mm: Fix the value of fallback_migratetype in alloc_extfrag tracepoint

[Srivatsa S. Bhat]

In the current code, the value of fallback_migratetype that is printed
using the mm_page_alloc_extfrag tracepoint, is the value of the migratetype
*after* it has been set to the preferred migratetype (if the ownership was
changed). Obviously that wouldn't have been the original intent. (We already
have a separate 'change_ownership' field to tell whether the ownership of the
pageblock was changed from the fallback_migratetype to the preferred type.)

The intent of the fallback_migratetype field is to show the migratetype
from which we borrowed pages in order to satisfy the allocation request.
So fix the code to print that value correctly.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 include/trace/events/kmem.h |   10 +++++++---
 mm/page_alloc.c             |    5 +++--
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h
index 6bc943e..d0c6134 100644
--- a/include/trace/events/kmem.h
+++ b/include/trace/events/kmem.h
@@ -268,11 +268,13 @@ TRACE_EVENT(mm_page_alloc_extfrag,
 
 	TP_PROTO(struct page *page,
 			int alloc_order, int fallback_order,
-			int alloc_migratetype, int fallback_migratetype),
+			int alloc_migratetype, int fallback_migratetype,
+			int change_ownership),
 
 	TP_ARGS(page,
 		alloc_order, fallback_order,
-		alloc_migratetype, fallback_migratetype),
+		alloc_migratetype, fallback_migratetype,
+		change_ownership),
 
 	TP_STRUCT__entry(
 		__field(	struct page *,	page			)
@@ -280,6 +282,7 @@ TRACE_EVENT(mm_page_alloc_extfrag,
 		__field(	int,		fallback_order		)
 		__field(	int,		alloc_migratetype	)
 		__field(	int,		fallback_migratetype	)
+		__field(	int,		change_ownership	)
 	),
 
 	TP_fast_assign(
@@ -288,6 +291,7 @@ TRACE_EVENT(mm_page_alloc_extfrag,
 		__entry->fallback_order		= fallback_order;
 		__entry->alloc_migratetype	= alloc_migratetype;
 		__entry->fallback_migratetype	= fallback_migratetype;
+		__entry->change_ownership	= change_ownership;
 	),
 
 	TP_printk("page=%p pfn=%lu alloc_order=%d fallback_order=%d pageblock_order=%d alloc_migratetype=%d fallback_migratetype=%d fragmenting=%d change_ownership=%d",
@@ -299,7 +303,7 @@ TRACE_EVENT(mm_page_alloc_extfrag,
 		__entry->alloc_migratetype,
 		__entry->fallback_migratetype,
 		__entry->fallback_order < pageblock_order,
-		__entry->alloc_migratetype == __entry->fallback_migratetype)
+		__entry->change_ownership)
 );
 
 #endif /* _TRACE_KMEM_H */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 027d417..af5571d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1101,8 +1101,9 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
 			       is_migrate_cma(migratetype)
 			     ? migratetype : start_migratetype);
 
-			trace_mm_page_alloc_extfrag(page, order, current_order,
-				start_migratetype, new_type);
+			trace_mm_page_alloc_extfrag(page, order,
+				current_order, start_migratetype, migratetype,
+				new_type == start_migratetype);
 
 			return page;
 		}

--
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 1/2] mm: Restructure free-page stealing code and fix a bug

[Wanpeng Li]

On Tue, Jul 23, 2013 at 12:18:06AM +0530, Srivatsa S. Bhat wrote:
>The free-page stealing code in __rmqueue_fallback() is somewhat hard to
>follow, and has an incredible amount of subtlety hidden inside!
>
>First off, there is a minor bug in the reporting of change-of-ownership of
>pageblocks. Under some conditions, we try to move upto 'pageblock_nr_pages'
>no. of pages to the preferred allocation list. But we change the ownership
>of that pageblock to the preferred type only if we manage to successfully
>move atleast half of that pageblock (or if page_group_by_mobility_disabled
>is set).
>
>However, the current code ignores the latter part and sets the 'migratetype'
>variable to the preferred type, irrespective of whether we actually changed
>the pageblock migratetype of that block or not. So, the page_alloc_extfrag
>tracepoint can end up printing incorrect info (i.e., 'change_ownership'
>might be shown as 1 when it must have been 0).
>
>So fixing this involves moving the update of the 'migratetype' variable to
>the right place. But looking closer, we observe that the 'migratetype' variable
>is used subsequently for checks such as "is_migrate_cma()". Obviously the
>intent there is to check if the *fallback* type is MIGRATE_CMA, but since we
>already set the 'migratetype' variable to start_migratetype, we end up checking
>if the *preferred* type is MIGRATE_CMA!!
>
>To make things more interesting, this actually doesn't cause a bug in practice,
>because we never change *anything* if the fallback type is CMA.
>
>So, restructure the code in such a way that it is trivial to understand what
>is going on, and also fix the above mentioned bug. And while at it, also add a
>comment explaining the subtlety behind the migratetype used in the call to
>expand().
>

Greate catch!

>Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>---
>
> mm/page_alloc.c |   96 ++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 60 insertions(+), 36 deletions(-)
>
>diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>index b100255..027d417 100644
>--- a/mm/page_alloc.c
>+++ b/mm/page_alloc.c
>@@ -1007,6 +1007,53 @@ static void change_pageblock_range(struct page *pageblock_page,
> 	}
> }
>
>+/*
>+ * If breaking a large block of pages, move all free pages to the preferred
>+ * allocation list. If falling back for a reclaimable kernel allocation, be
>+ * more aggressive about taking ownership of free pages.
>+ *
>+ * On the other hand, never change migration type of MIGRATE_CMA pageblocks
>+ * nor move CMA pages to different free lists. We don't want unmovable pages
>+ * to be allocated from MIGRATE_CMA areas.
>+ *
>+ * Returns the new migratetype of the pageblock (or the same old migratetype
>+ * if it was unchanged).
>+ */
>+static inline int try_to_steal_freepages(struct zone *zone, struct page *page,
>+					 int start_type, int fallback_type)
>+{
>+	int current_order = page_order(page);
>+
>+	if (is_migrate_cma(fallback_type))
>+		return fallback_type;
>+
>+	/* Take ownership for orders >= pageblock_order */
>+	if (current_order >= pageblock_order) {
>+		change_pageblock_range(page, current_order, start_type);
>+		return start_type;
>+	}
>+
>+	if (current_order >= pageblock_order / 2 ||
>+	    start_type == MIGRATE_RECLAIMABLE ||
>+	    page_group_by_mobility_disabled) {
>+
>+		int pages;
>+
>+		pages = move_freepages_block(zone, page, start_type);
>+
>+		/* Claim the whole block if over half of it is free */
>+		if (pages >= (1 << (pageblock_order-1)) ||
>+				page_group_by_mobility_disabled) {
>+
>+			set_pageblock_migratetype(page, start_type);
>+			return start_type;
>+		}
>+
>+	}
>+
>+	return fallback_type;
>+}
>+
> /* Remove an element from the buddy allocator from the fallback list */
> static inline struct page *
> __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
>@@ -1014,7 +1061,7 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
> 	struct free_area * area;
> 	int current_order;
> 	struct page *page;
>-	int migratetype, i;
>+	int migratetype, new_type, i;
>
> 	/* Find the largest possible block of pages in the other list */
> 	for (current_order = MAX_ORDER-1; current_order >= order;
>@@ -1034,51 +1081,28 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
> 					struct page, lru);
> 			area->nr_free--;
>
>-			/*
>-			 * If breaking a large block of pages, move all free
>-			 * pages to the preferred allocation list. If falling
>-			 * back for a reclaimable kernel allocation, be more
>-			 * aggressive about taking ownership of free pages
>-			 *
>-			 * On the other hand, never change migration
>-			 * type of MIGRATE_CMA pageblocks nor move CMA
>-			 * pages on different free lists. We don't
>-			 * want unmovable pages to be allocated from
>-			 * MIGRATE_CMA areas.
>-			 */
>-			if (!is_migrate_cma(migratetype) &&
>-			    (current_order >= pageblock_order / 2 ||
>-			     start_migratetype == MIGRATE_RECLAIMABLE ||
>-			     page_group_by_mobility_disabled)) {
>-				int pages;
>-				pages = move_freepages_block(zone, page,
>-								start_migratetype);
>-
>-				/* Claim the whole block if over half of it is free */
>-				if (pages >= (1 << (pageblock_order-1)) ||
>-						page_group_by_mobility_disabled)
>-					set_pageblock_migratetype(page,
>-								start_migratetype);
>-
>-				migratetype = start_migratetype;
>-			}
>+			new_type = try_to_steal_freepages(zone, page,
>+							  start_migratetype,
>+							  migratetype);
>
> 			/* Remove the page from the freelists */
> 			list_del(&page->lru);
> 			rmv_page_order(page);
>
>-			/* Take ownership for orders >= pageblock_order */
>-			if (current_order >= pageblock_order &&
>-			    !is_migrate_cma(migratetype))
>-				change_pageblock_range(page, current_order,
>-							start_migratetype);
>-
>+			/*
>+			 * Borrow the excess buddy pages as well, irrespective
>+			 * of whether we stole freepages, or took ownership of
>+			 * the pageblock or not.
>+			 *
>+			 * Exception: When borrowing from MIGRATE_CMA, release
>+			 * the excess buddy pages to CMA itself.
>+			 */
> 			expand(zone, page, order, current_order, area,
> 			       is_migrate_cma(migratetype)
> 			     ? migratetype : start_migratetype);
>
> 			trace_mm_page_alloc_extfrag(page, order, current_order,
>-				start_migratetype, migratetype);
>+				start_migratetype, new_type);
>
> 			return page;
> 		}
>
>--
>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>

--
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 1/2] mm: Restructure free-page stealing code and fix a bug

[Wanpeng Li]

On Tue, Jul 23, 2013 at 12:18:06AM +0530, Srivatsa S. Bhat wrote:
>The free-page stealing code in __rmqueue_fallback() is somewhat hard to
>follow, and has an incredible amount of subtlety hidden inside!
>
>First off, there is a minor bug in the reporting of change-of-ownership of
>pageblocks. Under some conditions, we try to move upto 'pageblock_nr_pages'
>no. of pages to the preferred allocation list. But we change the ownership
>of that pageblock to the preferred type only if we manage to successfully
>move atleast half of that pageblock (or if page_group_by_mobility_disabled
>is set).
>
>However, the current code ignores the latter part and sets the 'migratetype'
>variable to the preferred type, irrespective of whether we actually changed
>the pageblock migratetype of that block or not. So, the page_alloc_extfrag
>tracepoint can end up printing incorrect info (i.e., 'change_ownership'
>might be shown as 1 when it must have been 0).
>
>So fixing this involves moving the update of the 'migratetype' variable to
>the right place. But looking closer, we observe that the 'migratetype' variable
>is used subsequently for checks such as "is_migrate_cma()". Obviously the
>intent there is to check if the *fallback* type is MIGRATE_CMA, but since we
>already set the 'migratetype' variable to start_migratetype, we end up checking
>if the *preferred* type is MIGRATE_CMA!!
>
>To make things more interesting, this actually doesn't cause a bug in practice,
>because we never change *anything* if the fallback type is CMA.
>
>So, restructure the code in such a way that it is trivial to understand what
>is going on, and also fix the above mentioned bug. And while at it, also add a
>comment explaining the subtlety behind the migratetype used in the call to
>expand().
>

Greate catch!

>Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>---
>
> mm/page_alloc.c |   96 ++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 60 insertions(+), 36 deletions(-)
>
>diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>index b100255..027d417 100644
>--- a/mm/page_alloc.c
>+++ b/mm/page_alloc.c
>@@ -1007,6 +1007,53 @@ static void change_pageblock_range(struct page *pageblock_page,
> 	}
> }
>
>+/*
>+ * If breaking a large block of pages, move all free pages to the preferred
>+ * allocation list. If falling back for a reclaimable kernel allocation, be
>+ * more aggressive about taking ownership of free pages.
>+ *
>+ * On the other hand, never change migration type of MIGRATE_CMA pageblocks
>+ * nor move CMA pages to different free lists. We don't want unmovable pages
>+ * to be allocated from MIGRATE_CMA areas.
>+ *
>+ * Returns the new migratetype of the pageblock (or the same old migratetype
>+ * if it was unchanged).
>+ */
>+static inline int try_to_steal_freepages(struct zone *zone, struct page *page,
>+					 int start_type, int fallback_type)
>+{
>+	int current_order = page_order(page);
>+
>+	if (is_migrate_cma(fallback_type))
>+		return fallback_type;
>+
>+	/* Take ownership for orders >= pageblock_order */
>+	if (current_order >= pageblock_order) {
>+		change_pageblock_range(page, current_order, start_type);
>+		return start_type;
>+	}
>+
>+	if (current_order >= pageblock_order / 2 ||
>+	    start_type == MIGRATE_RECLAIMABLE ||
>+	    page_group_by_mobility_disabled) {
>+
>+		int pages;
>+
>+		pages = move_freepages_block(zone, page, start_type);
>+
>+		/* Claim the whole block if over half of it is free */
>+		if (pages >= (1 << (pageblock_order-1)) ||
>+				page_group_by_mobility_disabled) {
>+
>+			set_pageblock_migratetype(page, start_type);
>+			return start_type;
>+		}
>+
>+	}
>+
>+	return fallback_type;
>+}
>+
> /* Remove an element from the buddy allocator from the fallback list */
> static inline struct page *
> __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
>@@ -1014,7 +1061,7 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
> 	struct free_area * area;
> 	int current_order;
> 	struct page *page;
>-	int migratetype, i;
>+	int migratetype, new_type, i;
>
> 	/* Find the largest possible block of pages in the other list */
> 	for (current_order = MAX_ORDER-1; current_order >= order;
>@@ -1034,51 +1081,28 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype)
> 					struct page, lru);
> 			area->nr_free--;
>
>-			/*
>-			 * If breaking a large block of pages, move all free
>-			 * pages to the preferred allocation list. If falling
>-			 * back for a reclaimable kernel allocation, be more
>-			 * aggressive about taking ownership of free pages
>-			 *
>-			 * On the other hand, never change migration
>-			 * type of MIGRATE_CMA pageblocks nor move CMA
>-			 * pages on different free lists. We don't
>-			 * want unmovable pages to be allocated from
>-			 * MIGRATE_CMA areas.
>-			 */
>-			if (!is_migrate_cma(migratetype) &&
>-			    (current_order >= pageblock_order / 2 ||
>-			     start_migratetype == MIGRATE_RECLAIMABLE ||
>-			     page_group_by_mobility_disabled)) {
>-				int pages;
>-				pages = move_freepages_block(zone, page,
>-								start_migratetype);
>-
>-				/* Claim the whole block if over half of it is free */
>-				if (pages >= (1 << (pageblock_order-1)) ||
>-						page_group_by_mobility_disabled)
>-					set_pageblock_migratetype(page,
>-								start_migratetype);
>-
>-				migratetype = start_migratetype;
>-			}
>+			new_type = try_to_steal_freepages(zone, page,
>+							  start_migratetype,
>+							  migratetype);
>
> 			/* Remove the page from the freelists */
> 			list_del(&page->lru);
> 			rmv_page_order(page);
>
>-			/* Take ownership for orders >= pageblock_order */
>-			if (current_order >= pageblock_order &&
>-			    !is_migrate_cma(migratetype))
>-				change_pageblock_range(page, current_order,
>-							start_migratetype);
>-
>+			/*
>+			 * Borrow the excess buddy pages as well, irrespective
>+			 * of whether we stole freepages, or took ownership of
>+			 * the pageblock or not.
>+			 *
>+			 * Exception: When borrowing from MIGRATE_CMA, release
>+			 * the excess buddy pages to CMA itself.
>+			 */
> 			expand(zone, page, order, current_order, area,
> 			       is_migrate_cma(migratetype)
> 			     ? migratetype : start_migratetype);
>
> 			trace_mm_page_alloc_extfrag(page, order, current_order,
>-				start_migratetype, migratetype);
>+				start_migratetype, new_type);
>
> 			return page;
> 		}
>
>--
>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>

--
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 1/2] mm: Restructure free-page stealing code and fix a bug

[Srivatsa S. Bhat]

On 07/25/2013 08:40 AM, Wanpeng Li wrote:
> On Tue, Jul 23, 2013 at 12:18:06AM +0530, Srivatsa S. Bhat wrote:
>> The free-page stealing code in __rmqueue_fallback() is somewhat hard to
>> follow, and has an incredible amount of subtlety hidden inside!
>>
>> First off, there is a minor bug in the reporting of change-of-ownership of
>> pageblocks. Under some conditions, we try to move upto 'pageblock_nr_pages'
>> no. of pages to the preferred allocation list. But we change the ownership
>> of that pageblock to the preferred type only if we manage to successfully
>> move atleast half of that pageblock (or if page_group_by_mobility_disabled
>> is set).
>>
>> However, the current code ignores the latter part and sets the 'migratetype'
>> variable to the preferred type, irrespective of whether we actually changed
>> the pageblock migratetype of that block or not. So, the page_alloc_extfrag
>> tracepoint can end up printing incorrect info (i.e., 'change_ownership'
>> might be shown as 1 when it must have been 0).
>>
>> So fixing this involves moving the update of the 'migratetype' variable to
>> the right place. But looking closer, we observe that the 'migratetype' variable
>> is used subsequently for checks such as "is_migrate_cma()". Obviously the
>> intent there is to check if the *fallback* type is MIGRATE_CMA, but since we
>> already set the 'migratetype' variable to start_migratetype, we end up checking
>> if the *preferred* type is MIGRATE_CMA!!
>>
>> To make things more interesting, this actually doesn't cause a bug in practice,
>> because we never change *anything* if the fallback type is CMA.
>>
>> So, restructure the code in such a way that it is trivial to understand what
>> is going on, and also fix the above mentioned bug. And while at it, also add a
>> comment explaining the subtlety behind the migratetype used in the call to
>> expand().
>>
> 
> Greate catch!
> 

Thank you :-)

Regards,
Srivatsa S. Bhat

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