[PATCH 1/2] mm/compaction: clean up unused code lines

[PATCH 1/2] mm/compaction: clean up unused code lines

[Heesub Shin]

This commit removes code lines currently not in use or never called.

Signed-off-by: Heesub Shin <heesub.shin@samsung.com>
Cc: Dongjun Shin <d.j.shin@samsung.com>
Cc: Sunghwan Yun <sunghwan.yun@samsung.com>
---
 mm/compaction.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 9635083..1ef9144 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -208,12 +208,6 @@ static bool compact_checklock_irqsave(spinlock_t *lock, unsigned long *flags,
 	return true;
 }
 
-static inline bool compact_trylock_irqsave(spinlock_t *lock,
-			unsigned long *flags, struct compact_control *cc)
-{
-	return compact_checklock_irqsave(lock, flags, false, cc);
-}
-
 /* Returns true if the page is within a block suitable for migration to */
 static bool suitable_migration_target(struct page *page)
 {
@@ -728,7 +722,6 @@ static void isolate_freepages(struct zone *zone,
 			continue;
 
 		/* Found a block suitable for isolating free pages from */
-		isolated = 0;
 
 		/*
 		 * As pfn may not start aligned, pfn+pageblock_nr_page
@@ -1160,9 +1153,6 @@ static void __compact_pgdat(pg_data_t *pgdat, struct compact_control *cc)
 			if (zone_watermark_ok(zone, cc->order,
 						low_wmark_pages(zone), 0, 0))
 				compaction_defer_reset(zone, cc->order, false);
-			/* Currently async compaction is never deferred. */
-			else if (cc->sync)
-				defer_compaction(zone, cc->order);
 		}
 
 		VM_BUG_ON(!list_empty(&cc->freepages));
-- 
1.8.3.2

[PATCH 1/2] mm/compaction: clean up unused code lines

[Heesub Shin]

This commit removes code lines currently not in use or never called.

Signed-off-by: Heesub Shin <heesub.shin@samsung.com>
Cc: Dongjun Shin <d.j.shin@samsung.com>
Cc: Sunghwan Yun <sunghwan.yun@samsung.com>
---
 mm/compaction.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 9635083..1ef9144 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -208,12 +208,6 @@ static bool compact_checklock_irqsave(spinlock_t *lock, unsigned long *flags,
 	return true;
 }
 
-static inline bool compact_trylock_irqsave(spinlock_t *lock,
-			unsigned long *flags, struct compact_control *cc)
-{
-	return compact_checklock_irqsave(lock, flags, false, cc);
-}
-
 /* Returns true if the page is within a block suitable for migration to */
 static bool suitable_migration_target(struct page *page)
 {
@@ -728,7 +722,6 @@ static void isolate_freepages(struct zone *zone,
 			continue;
 
 		/* Found a block suitable for isolating free pages from */
-		isolated = 0;
 
 		/*
 		 * As pfn may not start aligned, pfn+pageblock_nr_page
@@ -1160,9 +1153,6 @@ static void __compact_pgdat(pg_data_t *pgdat, struct compact_control *cc)
 			if (zone_watermark_ok(zone, cc->order,
 						low_wmark_pages(zone), 0, 0))
 				compaction_defer_reset(zone, cc->order, false);
-			/* Currently async compaction is never deferred. */
-			else if (cc->sync)
-				defer_compaction(zone, cc->order);
 		}
 
 		VM_BUG_ON(!list_empty(&cc->freepages));
-- 
1.8.3.2

--
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/compaction: fix to initialize free scanner properly

[Heesub Shin]

Free scanner does not works well on systems having zones which do not
span to pageblock-aligned boundary.

zone->compact_cached_free_pfn is reset when the migration and free
scanner across or compaction restarts. After the reset, if end_pfn of
the zone was not aligned to pageblock_nr_pages, free scanner tries to
isolate free pages from the middle of pageblock to the end, which can
be very small range.

Signed-off-by: Heesub Shin <heesub.shin@samsung.com>
Cc: Dongjun Shin <d.j.shin@samsung.com>
Cc: Sunghwan Yun <sunghwan.yun@samsung.com>
---
 mm/compaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 1ef9144..fefe1da 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -983,7 +983,7 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
 	 */
 	cc->migrate_pfn = zone->compact_cached_migrate_pfn;
 	cc->free_pfn = zone->compact_cached_free_pfn;
-	if (cc->free_pfn < start_pfn || cc->free_pfn > end_pfn) {
+	if (cc->free_pfn < start_pfn || cc->free_pfn >= end_pfn) {
 		cc->free_pfn = end_pfn & ~(pageblock_nr_pages-1);
 		zone->compact_cached_free_pfn = cc->free_pfn;
 	}
-- 
1.8.3.2

[PATCH 2/2] mm/compaction: fix to initialize free scanner properly

[Heesub Shin]

Free scanner does not works well on systems having zones which do not
span to pageblock-aligned boundary.

zone->compact_cached_free_pfn is reset when the migration and free
scanner across or compaction restarts. After the reset, if end_pfn of
the zone was not aligned to pageblock_nr_pages, free scanner tries to
isolate free pages from the middle of pageblock to the end, which can
be very small range.

Signed-off-by: Heesub Shin <heesub.shin@samsung.com>
Cc: Dongjun Shin <d.j.shin@samsung.com>
Cc: Sunghwan Yun <sunghwan.yun@samsung.com>
---
 mm/compaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 1ef9144..fefe1da 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -983,7 +983,7 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
 	 */
 	cc->migrate_pfn = zone->compact_cached_migrate_pfn;
 	cc->free_pfn = zone->compact_cached_free_pfn;
-	if (cc->free_pfn < start_pfn || cc->free_pfn > end_pfn) {
+	if (cc->free_pfn < start_pfn || cc->free_pfn >= end_pfn) {
 		cc->free_pfn = end_pfn & ~(pageblock_nr_pages-1);
 		zone->compact_cached_free_pfn = cc->free_pfn;
 	}
-- 
1.8.3.2

--
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/compaction: clean up unused code lines

[Vlastimil Babka]

On 04/03/2014 10:57 AM, Heesub Shin wrote:
> This commit removes code lines currently not in use or never called.
>
> Signed-off-by: Heesub Shin <heesub.shin@samsung.com>
> Cc: Dongjun Shin <d.j.shin@samsung.com>
> Cc: Sunghwan Yun <sunghwan.yun@samsung.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>   mm/compaction.c | 10 ----------
>   1 file changed, 10 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 9635083..1ef9144 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -208,12 +208,6 @@ static bool compact_checklock_irqsave(spinlock_t *lock, unsigned long *flags,
>   	return true;
>   }
>
> -static inline bool compact_trylock_irqsave(spinlock_t *lock,
> -			unsigned long *flags, struct compact_control *cc)
> -{
> -	return compact_checklock_irqsave(lock, flags, false, cc);
> -}
> -
>   /* Returns true if the page is within a block suitable for migration to */
>   static bool suitable_migration_target(struct page *page)
>   {
> @@ -728,7 +722,6 @@ static void isolate_freepages(struct zone *zone,
>   			continue;
>
>   		/* Found a block suitable for isolating free pages from */
> -		isolated = 0;
>
>   		/*
>   		 * As pfn may not start aligned, pfn+pageblock_nr_page
> @@ -1160,9 +1153,6 @@ static void __compact_pgdat(pg_data_t *pgdat, struct compact_control *cc)
>   			if (zone_watermark_ok(zone, cc->order,
>   						low_wmark_pages(zone), 0, 0))
>   				compaction_defer_reset(zone, cc->order, false);
> -			/* Currently async compaction is never deferred. */
> -			else if (cc->sync)
> -				defer_compaction(zone, cc->order);
>   		}
>
>   		VM_BUG_ON(!list_empty(&cc->freepages));
>

Re: [PATCH 1/2] mm/compaction: clean up unused code lines

[Vlastimil Babka]

On 04/03/2014 10:57 AM, Heesub Shin wrote:
> This commit removes code lines currently not in use or never called.
>
> Signed-off-by: Heesub Shin <heesub.shin@samsung.com>
> Cc: Dongjun Shin <d.j.shin@samsung.com>
> Cc: Sunghwan Yun <sunghwan.yun@samsung.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>   mm/compaction.c | 10 ----------
>   1 file changed, 10 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 9635083..1ef9144 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -208,12 +208,6 @@ static bool compact_checklock_irqsave(spinlock_t *lock, unsigned long *flags,
>   	return true;
>   }
>
> -static inline bool compact_trylock_irqsave(spinlock_t *lock,
> -			unsigned long *flags, struct compact_control *cc)
> -{
> -	return compact_checklock_irqsave(lock, flags, false, cc);
> -}
> -
>   /* Returns true if the page is within a block suitable for migration to */
>   static bool suitable_migration_target(struct page *page)
>   {
> @@ -728,7 +722,6 @@ static void isolate_freepages(struct zone *zone,
>   			continue;
>
>   		/* Found a block suitable for isolating free pages from */
> -		isolated = 0;
>
>   		/*
>   		 * As pfn may not start aligned, pfn+pageblock_nr_page
> @@ -1160,9 +1153,6 @@ static void __compact_pgdat(pg_data_t *pgdat, struct compact_control *cc)
>   			if (zone_watermark_ok(zone, cc->order,
>   						low_wmark_pages(zone), 0, 0))
>   				compaction_defer_reset(zone, cc->order, false);
> -			/* Currently async compaction is never deferred. */
> -			else if (cc->sync)
> -				defer_compaction(zone, cc->order);
>   		}
>
>   		VM_BUG_ON(!list_empty(&cc->freepages));
>

--
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 2/2] mm/compaction: fix to initialize free scanner properly

[Vlastimil Babka]

On 04/03/2014 10:57 AM, Heesub Shin wrote:
> Free scanner does not works well on systems having zones which do not
> span to pageblock-aligned boundary.
>
> zone->compact_cached_free_pfn is reset when the migration and free
> scanner across or compaction restarts. After the reset, if end_pfn of
> the zone was not aligned to pageblock_nr_pages, free scanner tries to
> isolate free pages from the middle of pageblock to the end, which can
> be very small range.

Hm good catch. I think that the same problem can happen (at least 
theoretically) through zone->compact_cached_free_pfn with 
CONFIG_HOLES_IN_ZONE enabled. Then compact_cached_free_pfn could be set
to a non-aligned-to-pageblock pfn and spoil scans. I'll send a patch 
that solves it on isolate_freepages() level, which allows further 
simplification of the function.

Vlastimil

> Signed-off-by: Heesub Shin <heesub.shin@samsung.com>
> Cc: Dongjun Shin <d.j.shin@samsung.com>
> Cc: Sunghwan Yun <sunghwan.yun@samsung.com>
> ---
>   mm/compaction.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 1ef9144..fefe1da 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -983,7 +983,7 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
>   	 */
>   	cc->migrate_pfn = zone->compact_cached_migrate_pfn;
>   	cc->free_pfn = zone->compact_cached_free_pfn;
> -	if (cc->free_pfn < start_pfn || cc->free_pfn > end_pfn) {
> +	if (cc->free_pfn < start_pfn || cc->free_pfn >= end_pfn) {
>   		cc->free_pfn = end_pfn & ~(pageblock_nr_pages-1);
>   		zone->compact_cached_free_pfn = cc->free_pfn;
>   	}
>

Re: [PATCH 2/2] mm/compaction: fix to initialize free scanner properly

[Vlastimil Babka]

On 04/03/2014 10:57 AM, Heesub Shin wrote:
> Free scanner does not works well on systems having zones which do not
> span to pageblock-aligned boundary.
>
> zone->compact_cached_free_pfn is reset when the migration and free
> scanner across or compaction restarts. After the reset, if end_pfn of
> the zone was not aligned to pageblock_nr_pages, free scanner tries to
> isolate free pages from the middle of pageblock to the end, which can
> be very small range.

Hm good catch. I think that the same problem can happen (at least 
theoretically) through zone->compact_cached_free_pfn with 
CONFIG_HOLES_IN_ZONE enabled. Then compact_cached_free_pfn could be set
to a non-aligned-to-pageblock pfn and spoil scans. I'll send a patch 
that solves it on isolate_freepages() level, which allows further 
simplification of the function.

Vlastimil

> Signed-off-by: Heesub Shin <heesub.shin@samsung.com>
> Cc: Dongjun Shin <d.j.shin@samsung.com>
> Cc: Sunghwan Yun <sunghwan.yun@samsung.com>
> ---
>   mm/compaction.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 1ef9144..fefe1da 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -983,7 +983,7 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
>   	 */
>   	cc->migrate_pfn = zone->compact_cached_migrate_pfn;
>   	cc->free_pfn = zone->compact_cached_free_pfn;
> -	if (cc->free_pfn < start_pfn || cc->free_pfn > end_pfn) {
> +	if (cc->free_pfn < start_pfn || cc->free_pfn >= end_pfn) {
>   		cc->free_pfn = end_pfn & ~(pageblock_nr_pages-1);
>   		zone->compact_cached_free_pfn = cc->free_pfn;
>   	}
>

--
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 1/2] mm/compaction: make isolate_freepages start at pageblock boundary

[Vlastimil Babka]

The compaction freepage scanner implementation in isolate_freepages() starts
by taking the current cc->free_pfn value as the first pfn. In a for loop, it
scans from this first pfn to the end of the pageblock, and then subtracts
pageblock_nr_pages from the first pfn to obtain the first pfn for the next
for loop iteration.

This means that when cc->free_pfn starts at offset X rather than being aligned
on pageblock boundary, the scanner will start at offset X in all scanned
pageblock, ignoring potentially many free pages. Currently this can happen when
a) zone's end pfn is not pageblock aligned, or
b) through zone->compact_cached_free_pfn with CONFIG_HOLES_IN_ZONE enabled and
   a hole spanning the beginning of a pageblock

This patch fixes the problem by aligning the initial pfn in isolate_freepages()
to pageblock boundary. This also allows to replace the end-of-pageblock
alignment within the for loop with a simple pageblock_nr_pages increment.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Reported-by: Heesub Shin <heesub.shin@samsung.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Rik van Riel <riel@redhat.com>
---
 mm/compaction.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 37f9762..627dc2e 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -671,16 +671,20 @@ static void isolate_freepages(struct zone *zone,
 				struct compact_control *cc)
 {
 	struct page *page;
-	unsigned long high_pfn, low_pfn, pfn, z_end_pfn, end_pfn;
+	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
 	int nr_freepages = cc->nr_freepages;
 	struct list_head *freelist = &cc->freepages;
 
 	/*
 	 * Initialise the free scanner. The starting point is where we last
-	 * scanned from (or the end of the zone if starting). The low point
-	 * is the end of the pageblock the migration scanner is using.
+	 * successfully isolated from, zone-cached value, or the end of the
+	 * zone when isolating for the first time. We need this aligned to
+	 * the pageblock boundary, because we do pfn -= pageblock_nr_pages
+	 * in the for loop.
+	 * The low boundary is the end of the pageblock the migration scanner
+	 * is using.
 	 */
-	pfn = cc->free_pfn;
+	pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
 	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
 
 	/*
@@ -700,6 +704,7 @@ static void isolate_freepages(struct zone *zone,
 	for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
 					pfn -= pageblock_nr_pages) {
 		unsigned long isolated;
+		unsigned long end_pfn;
 
 		/*
 		 * This can iterate a massively long zone without finding any
@@ -734,13 +739,10 @@ static void isolate_freepages(struct zone *zone,
 		isolated = 0;
 
 		/*
-		 * As pfn may not start aligned, pfn+pageblock_nr_page
-		 * may cross a MAX_ORDER_NR_PAGES boundary and miss
-		 * a pfn_valid check. Ensure isolate_freepages_block()
-		 * only scans within a pageblock
+		 * Take care when isolating in last pageblock of a zone which
+		 * ends in the middle of a pageblock.
 		 */
-		end_pfn = ALIGN(pfn + 1, pageblock_nr_pages);
-		end_pfn = min(end_pfn, z_end_pfn);
+		end_pfn = min(pfn + pageblock_nr_pages, z_end_pfn);
 		isolated = isolate_freepages_block(cc, pfn, end_pfn,
 						   freelist, false);
 		nr_freepages += isolated;
-- 
1.8.4.5

[PATCH 1/2] mm/compaction: make isolate_freepages start at pageblock boundary

[Vlastimil Babka]

The compaction freepage scanner implementation in isolate_freepages() starts
by taking the current cc->free_pfn value as the first pfn. In a for loop, it
scans from this first pfn to the end of the pageblock, and then subtracts
pageblock_nr_pages from the first pfn to obtain the first pfn for the next
for loop iteration.

This means that when cc->free_pfn starts at offset X rather than being aligned
on pageblock boundary, the scanner will start at offset X in all scanned
pageblock, ignoring potentially many free pages. Currently this can happen when
a) zone's end pfn is not pageblock aligned, or
b) through zone->compact_cached_free_pfn with CONFIG_HOLES_IN_ZONE enabled and
   a hole spanning the beginning of a pageblock

This patch fixes the problem by aligning the initial pfn in isolate_freepages()
to pageblock boundary. This also allows to replace the end-of-pageblock
alignment within the for loop with a simple pageblock_nr_pages increment.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Reported-by: Heesub Shin <heesub.shin@samsung.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Rik van Riel <riel@redhat.com>
---
 mm/compaction.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 37f9762..627dc2e 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -671,16 +671,20 @@ static void isolate_freepages(struct zone *zone,
 				struct compact_control *cc)
 {
 	struct page *page;
-	unsigned long high_pfn, low_pfn, pfn, z_end_pfn, end_pfn;
+	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
 	int nr_freepages = cc->nr_freepages;
 	struct list_head *freelist = &cc->freepages;
 
 	/*
 	 * Initialise the free scanner. The starting point is where we last
-	 * scanned from (or the end of the zone if starting). The low point
-	 * is the end of the pageblock the migration scanner is using.
+	 * successfully isolated from, zone-cached value, or the end of the
+	 * zone when isolating for the first time. We need this aligned to
+	 * the pageblock boundary, because we do pfn -= pageblock_nr_pages
+	 * in the for loop.
+	 * The low boundary is the end of the pageblock the migration scanner
+	 * is using.
 	 */
-	pfn = cc->free_pfn;
+	pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
 	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
 
 	/*
@@ -700,6 +704,7 @@ static void isolate_freepages(struct zone *zone,
 	for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
 					pfn -= pageblock_nr_pages) {
 		unsigned long isolated;
+		unsigned long end_pfn;
 
 		/*
 		 * This can iterate a massively long zone without finding any
@@ -734,13 +739,10 @@ static void isolate_freepages(struct zone *zone,
 		isolated = 0;
 
 		/*
-		 * As pfn may not start aligned, pfn+pageblock_nr_page
-		 * may cross a MAX_ORDER_NR_PAGES boundary and miss
-		 * a pfn_valid check. Ensure isolate_freepages_block()
-		 * only scans within a pageblock
+		 * Take care when isolating in last pageblock of a zone which
+		 * ends in the middle of a pageblock.
 		 */
-		end_pfn = ALIGN(pfn + 1, pageblock_nr_pages);
-		end_pfn = min(end_pfn, z_end_pfn);
+		end_pfn = min(pfn + pageblock_nr_pages, z_end_pfn);
 		isolated = isolate_freepages_block(cc, pfn, end_pfn,
 						   freelist, false);
 		nr_freepages += isolated;
-- 
1.8.4.5

--
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/compaction: cleanup isolate_freepages()

[Vlastimil Babka]

isolate_freepages() is currently somewhat hard to follow thanks to many
different pfn variables. Especially misleading is the name 'high_pfn' which
looks like it is related to the 'low_pfn' variable, but in fact it is not.

This patch renames the 'high_pfn' variable to a hopefully less confusing name,
and slightly changes its handling without a functional change. A comment made
obsolete by recent changes is also updated.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Rik van Riel <riel@redhat.com>
---
 mm/compaction.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 627dc2e..169c7b2 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -671,7 +671,7 @@ static void isolate_freepages(struct zone *zone,
 				struct compact_control *cc)
 {
 	struct page *page;
-	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
+	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
 	int nr_freepages = cc->nr_freepages;
 	struct list_head *freelist = &cc->freepages;
 
@@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
 	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
 
 	/*
-	 * Take care that if the migration scanner is at the end of the zone
-	 * that the free scanner does not accidentally move to the next zone
-	 * in the next isolation cycle.
+	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
+	 * none, the pfn < low_pfn check will kick in.
 	 */
-	high_pfn = min(low_pfn, pfn);
+	next_free_pfn = 0;
 
 	z_end_pfn = zone_end_pfn(zone);
 
@@ -754,7 +753,7 @@ static void isolate_freepages(struct zone *zone,
 		 */
 		if (isolated) {
 			cc->finished_update_free = true;
-			high_pfn = max(high_pfn, pfn);
+			next_free_pfn = max(next_free_pfn, pfn);
 		}
 	}
 
@@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
 	 * so that compact_finished() may detect this
 	 */
 	if (pfn < low_pfn)
-		cc->free_pfn = max(pfn, zone->zone_start_pfn);
-	else
-		cc->free_pfn = high_pfn;
+		next_free_pfn = max(pfn, zone->zone_start_pfn);
+
+	cc->free_pfn = next_free_pfn;
 	cc->nr_freepages = nr_freepages;
 }
 
-- 
1.8.4.5

[PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Vlastimil Babka]

isolate_freepages() is currently somewhat hard to follow thanks to many
different pfn variables. Especially misleading is the name 'high_pfn' which
looks like it is related to the 'low_pfn' variable, but in fact it is not.

This patch renames the 'high_pfn' variable to a hopefully less confusing name,
and slightly changes its handling without a functional change. A comment made
obsolete by recent changes is also updated.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Rik van Riel <riel@redhat.com>
---
 mm/compaction.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 627dc2e..169c7b2 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -671,7 +671,7 @@ static void isolate_freepages(struct zone *zone,
 				struct compact_control *cc)
 {
 	struct page *page;
-	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
+	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
 	int nr_freepages = cc->nr_freepages;
 	struct list_head *freelist = &cc->freepages;
 
@@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
 	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
 
 	/*
-	 * Take care that if the migration scanner is at the end of the zone
-	 * that the free scanner does not accidentally move to the next zone
-	 * in the next isolation cycle.
+	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
+	 * none, the pfn < low_pfn check will kick in.
 	 */
-	high_pfn = min(low_pfn, pfn);
+	next_free_pfn = 0;
 
 	z_end_pfn = zone_end_pfn(zone);
 
@@ -754,7 +753,7 @@ static void isolate_freepages(struct zone *zone,
 		 */
 		if (isolated) {
 			cc->finished_update_free = true;
-			high_pfn = max(high_pfn, pfn);
+			next_free_pfn = max(next_free_pfn, pfn);
 		}
 	}
 
@@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
 	 * so that compact_finished() may detect this
 	 */
 	if (pfn < low_pfn)
-		cc->free_pfn = max(pfn, zone->zone_start_pfn);
-	else
-		cc->free_pfn = high_pfn;
+		next_free_pfn = max(pfn, zone->zone_start_pfn);
+
+	cc->free_pfn = next_free_pfn;
 	cc->nr_freepages = nr_freepages;
 }
 
-- 
1.8.4.5

--
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/compaction: make isolate_freepages start at pageblock boundary

[Joonsoo Kim]

On Tue, Apr 15, 2014 at 11:18:26AM +0200, Vlastimil Babka wrote:
> The compaction freepage scanner implementation in isolate_freepages() starts
> by taking the current cc->free_pfn value as the first pfn. In a for loop, it
> scans from this first pfn to the end of the pageblock, and then subtracts
> pageblock_nr_pages from the first pfn to obtain the first pfn for the next
> for loop iteration.
> 
> This means that when cc->free_pfn starts at offset X rather than being aligned
> on pageblock boundary, the scanner will start at offset X in all scanned
> pageblock, ignoring potentially many free pages. Currently this can happen when
> a) zone's end pfn is not pageblock aligned, or
> b) through zone->compact_cached_free_pfn with CONFIG_HOLES_IN_ZONE enabled and
>    a hole spanning the beginning of a pageblock
> 
> This patch fixes the problem by aligning the initial pfn in isolate_freepages()
> to pageblock boundary. This also allows to replace the end-of-pageblock
> alignment within the for loop with a simple pageblock_nr_pages increment.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Reported-by: Heesub Shin <heesub.shin@samsung.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Rik van Riel <riel@redhat.com>
> ---
>  mm/compaction.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)

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

Re: [PATCH 1/2] mm/compaction: make isolate_freepages start at pageblock boundary

[Joonsoo Kim]

On Tue, Apr 15, 2014 at 11:18:26AM +0200, Vlastimil Babka wrote:
> The compaction freepage scanner implementation in isolate_freepages() starts
> by taking the current cc->free_pfn value as the first pfn. In a for loop, it
> scans from this first pfn to the end of the pageblock, and then subtracts
> pageblock_nr_pages from the first pfn to obtain the first pfn for the next
> for loop iteration.
> 
> This means that when cc->free_pfn starts at offset X rather than being aligned
> on pageblock boundary, the scanner will start at offset X in all scanned
> pageblock, ignoring potentially many free pages. Currently this can happen when
> a) zone's end pfn is not pageblock aligned, or
> b) through zone->compact_cached_free_pfn with CONFIG_HOLES_IN_ZONE enabled and
>    a hole spanning the beginning of a pageblock
> 
> This patch fixes the problem by aligning the initial pfn in isolate_freepages()
> to pageblock boundary. This also allows to replace the end-of-pageblock
> alignment within the for loop with a simple pageblock_nr_pages increment.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Reported-by: Heesub Shin <heesub.shin@samsung.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Rik van Riel <riel@redhat.com>
> ---
>  mm/compaction.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Joonsoo Kim]

On Tue, Apr 15, 2014 at 11:18:27AM +0200, Vlastimil Babka wrote:
> isolate_freepages() is currently somewhat hard to follow thanks to many
> different pfn variables. Especially misleading is the name 'high_pfn' which
> looks like it is related to the 'low_pfn' variable, but in fact it is not.
> 
> This patch renames the 'high_pfn' variable to a hopefully less confusing name,
> and slightly changes its handling without a functional change. A comment made
> obsolete by recent changes is also updated.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Rik van Riel <riel@redhat.com>
> ---
>  mm/compaction.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)

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

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Joonsoo Kim]

On Tue, Apr 15, 2014 at 11:18:27AM +0200, Vlastimil Babka wrote:
> isolate_freepages() is currently somewhat hard to follow thanks to many
> different pfn variables. Especially misleading is the name 'high_pfn' which
> looks like it is related to the 'low_pfn' variable, but in fact it is not.
> 
> This patch renames the 'high_pfn' variable to a hopefully less confusing name,
> and slightly changes its handling without a functional change. A comment made
> obsolete by recent changes is also updated.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Rik van Riel <riel@redhat.com>
> ---
>  mm/compaction.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm/compaction: make isolate_freepages start at pageblock boundary

[Rik van Riel]

On 04/15/2014 05:18 AM, Vlastimil Babka wrote:
> The compaction freepage scanner implementation in isolate_freepages() starts
> by taking the current cc->free_pfn value as the first pfn. In a for loop, it
> scans from this first pfn to the end of the pageblock, and then subtracts
> pageblock_nr_pages from the first pfn to obtain the first pfn for the next
> for loop iteration.
>
> This means that when cc->free_pfn starts at offset X rather than being aligned
> on pageblock boundary, the scanner will start at offset X in all scanned
> pageblock, ignoring potentially many free pages. Currently this can happen when
> a) zone's end pfn is not pageblock aligned, or
> b) through zone->compact_cached_free_pfn with CONFIG_HOLES_IN_ZONE enabled and
>     a hole spanning the beginning of a pageblock
>
> This patch fixes the problem by aligning the initial pfn in isolate_freepages()
> to pageblock boundary. This also allows to replace the end-of-pageblock
> alignment within the for loop with a simple pageblock_nr_pages increment.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Reported-by: Heesub Shin <heesub.shin@samsung.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Rik van Riel <riel@redhat.com>

Acked-by: Rik van Riel <riel@redhat.com>

Re: [PATCH 1/2] mm/compaction: make isolate_freepages start at pageblock boundary

[Rik van Riel]

On 04/15/2014 05:18 AM, Vlastimil Babka wrote:
> The compaction freepage scanner implementation in isolate_freepages() starts
> by taking the current cc->free_pfn value as the first pfn. In a for loop, it
> scans from this first pfn to the end of the pageblock, and then subtracts
> pageblock_nr_pages from the first pfn to obtain the first pfn for the next
> for loop iteration.
>
> This means that when cc->free_pfn starts at offset X rather than being aligned
> on pageblock boundary, the scanner will start at offset X in all scanned
> pageblock, ignoring potentially many free pages. Currently this can happen when
> a) zone's end pfn is not pageblock aligned, or
> b) through zone->compact_cached_free_pfn with CONFIG_HOLES_IN_ZONE enabled and
>     a hole spanning the beginning of a pageblock
>
> This patch fixes the problem by aligning the initial pfn in isolate_freepages()
> to pageblock boundary. This also allows to replace the end-of-pageblock
> alignment within the for loop with a simple pageblock_nr_pages increment.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Reported-by: Heesub Shin <heesub.shin@samsung.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Rik van Riel <riel@redhat.com>

Acked-by: Rik van Riel <riel@redhat.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Rik van Riel]

On 04/15/2014 05:18 AM, Vlastimil Babka wrote:
> isolate_freepages() is currently somewhat hard to follow thanks to many
> different pfn variables. Especially misleading is the name 'high_pfn' which
> looks like it is related to the 'low_pfn' variable, but in fact it is not.
>
> This patch renames the 'high_pfn' variable to a hopefully less confusing name,
> and slightly changes its handling without a functional change. A comment made
> obsolete by recent changes is also updated.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Rik van Riel <riel@redhat.com>

Acked-by: Rik van Riel <riel@redhat.com>

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Rik van Riel]

On 04/15/2014 05:18 AM, Vlastimil Babka wrote:
> isolate_freepages() is currently somewhat hard to follow thanks to many
> different pfn variables. Especially misleading is the name 'high_pfn' which
> looks like it is related to the 'low_pfn' variable, but in fact it is not.
>
> This patch renames the 'high_pfn' variable to a hopefully less confusing name,
> and slightly changes its handling without a functional change. A comment made
> obsolete by recent changes is also updated.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Rik van Riel <riel@redhat.com>

Acked-by: Rik van Riel <riel@redhat.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm/compaction: make isolate_freepages start at pageblock boundary

[Minchan Kim]

On Tue, Apr 15, 2014 at 11:18:26AM +0200, Vlastimil Babka wrote:
> The compaction freepage scanner implementation in isolate_freepages() starts
> by taking the current cc->free_pfn value as the first pfn. In a for loop, it
> scans from this first pfn to the end of the pageblock, and then subtracts
> pageblock_nr_pages from the first pfn to obtain the first pfn for the next
> for loop iteration.
> 
> This means that when cc->free_pfn starts at offset X rather than being aligned
> on pageblock boundary, the scanner will start at offset X in all scanned
> pageblock, ignoring potentially many free pages. Currently this can happen when
> a) zone's end pfn is not pageblock aligned, or
> b) through zone->compact_cached_free_pfn with CONFIG_HOLES_IN_ZONE enabled and
>    a hole spanning the beginning of a pageblock
> 
> This patch fixes the problem by aligning the initial pfn in isolate_freepages()
> to pageblock boundary. This also allows to replace the end-of-pageblock
> alignment within the for loop with a simple pageblock_nr_pages increment.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Reported-by: Heesub Shin <heesub.shin@samsung.com>

Acked-by: Minchan Kim <minchan@kernel.org>

-stable stuff?

-- 
Kind regards,
Minchan Kim

Re: [PATCH 1/2] mm/compaction: make isolate_freepages start at pageblock boundary

[Minchan Kim]

On Tue, Apr 15, 2014 at 11:18:26AM +0200, Vlastimil Babka wrote:
> The compaction freepage scanner implementation in isolate_freepages() starts
> by taking the current cc->free_pfn value as the first pfn. In a for loop, it
> scans from this first pfn to the end of the pageblock, and then subtracts
> pageblock_nr_pages from the first pfn to obtain the first pfn for the next
> for loop iteration.
> 
> This means that when cc->free_pfn starts at offset X rather than being aligned
> on pageblock boundary, the scanner will start at offset X in all scanned
> pageblock, ignoring potentially many free pages. Currently this can happen when
> a) zone's end pfn is not pageblock aligned, or
> b) through zone->compact_cached_free_pfn with CONFIG_HOLES_IN_ZONE enabled and
>    a hole spanning the beginning of a pageblock
> 
> This patch fixes the problem by aligning the initial pfn in isolate_freepages()
> to pageblock boundary. This also allows to replace the end-of-pageblock
> alignment within the for loop with a simple pageblock_nr_pages increment.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Reported-by: Heesub Shin <heesub.shin@samsung.com>

Acked-by: Minchan Kim <minchan@kernel.org>

-stable stuff?

-- 
Kind regards,
Minchan Kim

--
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 2/2] mm/compaction: cleanup isolate_freepages()

[Minchan Kim]

Hi Vlastimil,

Below just nitpicks.

On Tue, Apr 15, 2014 at 11:18:27AM +0200, Vlastimil Babka wrote:
> isolate_freepages() is currently somewhat hard to follow thanks to many
> different pfn variables. Especially misleading is the name 'high_pfn' which
> looks like it is related to the 'low_pfn' variable, but in fact it is not.

Indeed.

> 
> This patch renames the 'high_pfn' variable to a hopefully less confusing name,
> and slightly changes its handling without a functional change. A comment made
> obsolete by recent changes is also updated.

It's clean up patch so if we do fixing, I'd like to do more.

> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Rik van Riel <riel@redhat.com>
> ---
>  mm/compaction.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 627dc2e..169c7b2 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -671,7 +671,7 @@ static void isolate_freepages(struct zone *zone,
>  				struct compact_control *cc)
>  {
>  	struct page *page;
> -	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
> +	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;

Could you add comment for each variable?

unsigned long pfn; /* scanning cursor */
unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
unsigned long next_free_pfn; /* start pfn for scaning at next truen */
unsigned long z_end_pfn; /* zone's end pfn */


>  	int nr_freepages = cc->nr_freepages;
>  	struct list_head *freelist = &cc->freepages;
>  
> @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>  
>  	/*
> -	 * Take care that if the migration scanner is at the end of the zone
> -	 * that the free scanner does not accidentally move to the next zone
> -	 * in the next isolation cycle.
> +	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> +	 * none, the pfn < low_pfn check will kick in.

       "none" what? I'd like to clear more.

>  	 */
> -	high_pfn = min(low_pfn, pfn);
> +	next_free_pfn = 0;
>  
>  	z_end_pfn = zone_end_pfn(zone);
>  
> @@ -754,7 +753,7 @@ static void isolate_freepages(struct zone *zone,
>  		 */
>  		if (isolated) {
>  			cc->finished_update_free = true;
> -			high_pfn = max(high_pfn, pfn);
> +			next_free_pfn = max(next_free_pfn, pfn);
>  		}
>  	}
>  
> @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
>  	 * so that compact_finished() may detect this
>  	 */
>  	if (pfn < low_pfn)
> -		cc->free_pfn = max(pfn, zone->zone_start_pfn);
> -	else
> -		cc->free_pfn = high_pfn;
> +		next_free_pfn = max(pfn, zone->zone_start_pfn);

Why we need max operation?
IOW, what's the problem if we do (next_free_pfn = pfn)?

> +
> +	cc->free_pfn = next_free_pfn;
>  	cc->nr_freepages = nr_freepages;
>  }
>  
> -- 
> 1.8.4.5
> 
> --
> 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>

-- 
Kind regards,
Minchan Kim

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Minchan Kim]

Hi Vlastimil,

Below just nitpicks.

On Tue, Apr 15, 2014 at 11:18:27AM +0200, Vlastimil Babka wrote:
> isolate_freepages() is currently somewhat hard to follow thanks to many
> different pfn variables. Especially misleading is the name 'high_pfn' which
> looks like it is related to the 'low_pfn' variable, but in fact it is not.

Indeed.

> 
> This patch renames the 'high_pfn' variable to a hopefully less confusing name,
> and slightly changes its handling without a functional change. A comment made
> obsolete by recent changes is also updated.

It's clean up patch so if we do fixing, I'd like to do more.

> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Rik van Riel <riel@redhat.com>
> ---
>  mm/compaction.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 627dc2e..169c7b2 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -671,7 +671,7 @@ static void isolate_freepages(struct zone *zone,
>  				struct compact_control *cc)
>  {
>  	struct page *page;
> -	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
> +	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;

Could you add comment for each variable?

unsigned long pfn; /* scanning cursor */
unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
unsigned long next_free_pfn; /* start pfn for scaning at next truen */
unsigned long z_end_pfn; /* zone's end pfn */


>  	int nr_freepages = cc->nr_freepages;
>  	struct list_head *freelist = &cc->freepages;
>  
> @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>  
>  	/*
> -	 * Take care that if the migration scanner is at the end of the zone
> -	 * that the free scanner does not accidentally move to the next zone
> -	 * in the next isolation cycle.
> +	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> +	 * none, the pfn < low_pfn check will kick in.

       "none" what? I'd like to clear more.

>  	 */
> -	high_pfn = min(low_pfn, pfn);
> +	next_free_pfn = 0;
>  
>  	z_end_pfn = zone_end_pfn(zone);
>  
> @@ -754,7 +753,7 @@ static void isolate_freepages(struct zone *zone,
>  		 */
>  		if (isolated) {
>  			cc->finished_update_free = true;
> -			high_pfn = max(high_pfn, pfn);
> +			next_free_pfn = max(next_free_pfn, pfn);
>  		}
>  	}
>  
> @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
>  	 * so that compact_finished() may detect this
>  	 */
>  	if (pfn < low_pfn)
> -		cc->free_pfn = max(pfn, zone->zone_start_pfn);
> -	else
> -		cc->free_pfn = high_pfn;
> +		next_free_pfn = max(pfn, zone->zone_start_pfn);

Why we need max operation?
IOW, what's the problem if we do (next_free_pfn = pfn)?

> +
> +	cc->free_pfn = next_free_pfn;
>  	cc->nr_freepages = nr_freepages;
>  }
>  
> -- 
> 1.8.4.5
> 
> --
> 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>

-- 
Kind regards,
Minchan Kim

--
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 2/2] mm/compaction: cleanup isolate_freepages()

[Andrew Morton]

On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:

> Hi Vlastimil,
> 
> Below just nitpicks.

It seems you were ignored ;)

> >  {
> >  	struct page *page;
> > -	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
> > +	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> 
> Could you add comment for each variable?
> 
> unsigned long pfn; /* scanning cursor */
> unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
> unsigned long next_free_pfn; /* start pfn for scaning at next truen */
> unsigned long z_end_pfn; /* zone's end pfn */
> 
> 
> > @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
> >  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >  
> >  	/*
> > -	 * Take care that if the migration scanner is at the end of the zone
> > -	 * that the free scanner does not accidentally move to the next zone
> > -	 * in the next isolation cycle.
> > +	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> > +	 * none, the pfn < low_pfn check will kick in.
> 
>        "none" what? I'd like to clear more.

I did this:

--- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
+++ a/mm/compaction.c
@@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
 				struct compact_control *cc)
 {
 	struct page *page;
-	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
+	unsigned long pfn;	     /* scanning cursor */
+	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
+	unsigned long next_free_pfn; /* start pfn for scaning at next round */
+	unsigned long z_end_pfn;     /* zone's end pfn */
 	int nr_freepages = cc->nr_freepages;
 	struct list_head *freelist = &cc->freepages;
 
@@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
 	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
 
 	/*
-	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
-	 * none, the pfn < low_pfn check will kick in.
+	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
+	 * isolated, the pfn < low_pfn check will kick in.
 	 */
 	next_free_pfn = 0;
 
> > @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
> >  	 * so that compact_finished() may detect this
> >  	 */
> >  	if (pfn < low_pfn)
> > -		cc->free_pfn = max(pfn, zone->zone_start_pfn);
> > -	else
> > -		cc->free_pfn = high_pfn;
> > +		next_free_pfn = max(pfn, zone->zone_start_pfn);
> 
> Why we need max operation?
> IOW, what's the problem if we do (next_free_pfn = pfn)?

An answer to this would be useful, thanks.

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Andrew Morton]

On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:

> Hi Vlastimil,
> 
> Below just nitpicks.

It seems you were ignored ;)

> >  {
> >  	struct page *page;
> > -	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
> > +	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> 
> Could you add comment for each variable?
> 
> unsigned long pfn; /* scanning cursor */
> unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
> unsigned long next_free_pfn; /* start pfn for scaning at next truen */
> unsigned long z_end_pfn; /* zone's end pfn */
> 
> 
> > @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
> >  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >  
> >  	/*
> > -	 * Take care that if the migration scanner is at the end of the zone
> > -	 * that the free scanner does not accidentally move to the next zone
> > -	 * in the next isolation cycle.
> > +	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> > +	 * none, the pfn < low_pfn check will kick in.
> 
>        "none" what? I'd like to clear more.

I did this:

--- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
+++ a/mm/compaction.c
@@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
 				struct compact_control *cc)
 {
 	struct page *page;
-	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
+	unsigned long pfn;	     /* scanning cursor */
+	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
+	unsigned long next_free_pfn; /* start pfn for scaning at next round */
+	unsigned long z_end_pfn;     /* zone's end pfn */
 	int nr_freepages = cc->nr_freepages;
 	struct list_head *freelist = &cc->freepages;
 
@@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
 	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
 
 	/*
-	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
-	 * none, the pfn < low_pfn check will kick in.
+	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
+	 * isolated, the pfn < low_pfn check will kick in.
 	 */
 	next_free_pfn = 0;
 
> > @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
> >  	 * so that compact_finished() may detect this
> >  	 */
> >  	if (pfn < low_pfn)
> > -		cc->free_pfn = max(pfn, zone->zone_start_pfn);
> > -	else
> > -		cc->free_pfn = high_pfn;
> > +		next_free_pfn = max(pfn, zone->zone_start_pfn);
> 
> Why we need max operation?
> IOW, what's the problem if we do (next_free_pfn = pfn)?

An answer to this would be useful, thanks.

--
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 2/2] mm/compaction: cleanup isolate_freepages()

[Vlastimil Babka]

On 21.4.2014 21:41, Andrew Morton wrote:
> On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
>
>> Hi Vlastimil,
>>
>> Below just nitpicks.
> It seems you were ignored ;)

Oops, I managed to miss your e-mail, sorry.

>>>   {
>>>   	struct page *page;
>>> -	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
>>> +	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>> Could you add comment for each variable?
>>
>> unsigned long pfn; /* scanning cursor */
>> unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
>> unsigned long next_free_pfn; /* start pfn for scaning at next truen */
>> unsigned long z_end_pfn; /* zone's end pfn */
>>
>>
>>> @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
>>>   	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>   
>>>   	/*
>>> -	 * Take care that if the migration scanner is at the end of the zone
>>> -	 * that the free scanner does not accidentally move to the next zone
>>> -	 * in the next isolation cycle.
>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>> +	 * none, the pfn < low_pfn check will kick in.
>>         "none" what? I'd like to clear more.

If there are no updates to next_free_pfn within the for cycle. Which 
matches Andrew's formulation below.

> I did this:

Thanks!

>
> --- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
> +++ a/mm/compaction.c
> @@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
>   				struct compact_control *cc)
>   {
>   	struct page *page;
> -	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> +	unsigned long pfn;	     /* scanning cursor */
> +	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
> +	unsigned long next_free_pfn; /* start pfn for scaning at next round */
> +	unsigned long z_end_pfn;     /* zone's end pfn */

Yes that works.

>   	int nr_freepages = cc->nr_freepages;
>   	struct list_head *freelist = &cc->freepages;
>   
> @@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
>   	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>   
>   	/*
> -	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> -	 * none, the pfn < low_pfn check will kick in.
> +	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> +	 * isolated, the pfn < low_pfn check will kick in.

OK.

>   	 */
>   	next_free_pfn = 0;
>   
>>> @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
>>>   	 * so that compact_finished() may detect this
>>>   	 */
>>>   	if (pfn < low_pfn)
>>> -		cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>> -	else
>>> -		cc->free_pfn = high_pfn;
>>> +		next_free_pfn = max(pfn, zone->zone_start_pfn);
>> Why we need max operation?
>> IOW, what's the problem if we do (next_free_pfn = pfn)?
> An answer to this would be useful, thanks.

The idea (originally, not new here) is that the free scanner wants to 
remember the highest-pfn
block where it managed to isolate some pages. If the following page 
migration fails, these isolated
pages might be put back and would be skipped in further compaction 
attempt if we used just
"next_free_pfn = pfn", until the scanners get reset.

The question of course is if such situations are frequent and makes any 
difference to compaction
outcome. And the downsides are potentially useless rescans and code 
complexity. Maybe Mel
remembers how important this is? It should probably be profiled before 
changes are made.

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Vlastimil Babka]

On 21.4.2014 21:41, Andrew Morton wrote:
> On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
>
>> Hi Vlastimil,
>>
>> Below just nitpicks.
> It seems you were ignored ;)

Oops, I managed to miss your e-mail, sorry.

>>>   {
>>>   	struct page *page;
>>> -	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
>>> +	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>> Could you add comment for each variable?
>>
>> unsigned long pfn; /* scanning cursor */
>> unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
>> unsigned long next_free_pfn; /* start pfn for scaning at next truen */
>> unsigned long z_end_pfn; /* zone's end pfn */
>>
>>
>>> @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
>>>   	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>   
>>>   	/*
>>> -	 * Take care that if the migration scanner is at the end of the zone
>>> -	 * that the free scanner does not accidentally move to the next zone
>>> -	 * in the next isolation cycle.
>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>> +	 * none, the pfn < low_pfn check will kick in.
>>         "none" what? I'd like to clear more.

If there are no updates to next_free_pfn within the for cycle. Which 
matches Andrew's formulation below.

> I did this:

Thanks!

>
> --- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
> +++ a/mm/compaction.c
> @@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
>   				struct compact_control *cc)
>   {
>   	struct page *page;
> -	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> +	unsigned long pfn;	     /* scanning cursor */
> +	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
> +	unsigned long next_free_pfn; /* start pfn for scaning at next round */
> +	unsigned long z_end_pfn;     /* zone's end pfn */

Yes that works.

>   	int nr_freepages = cc->nr_freepages;
>   	struct list_head *freelist = &cc->freepages;
>   
> @@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
>   	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>   
>   	/*
> -	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> -	 * none, the pfn < low_pfn check will kick in.
> +	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> +	 * isolated, the pfn < low_pfn check will kick in.

OK.

>   	 */
>   	next_free_pfn = 0;
>   
>>> @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
>>>   	 * so that compact_finished() may detect this
>>>   	 */
>>>   	if (pfn < low_pfn)
>>> -		cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>> -	else
>>> -		cc->free_pfn = high_pfn;
>>> +		next_free_pfn = max(pfn, zone->zone_start_pfn);
>> Why we need max operation?
>> IOW, what's the problem if we do (next_free_pfn = pfn)?
> An answer to this would be useful, thanks.

The idea (originally, not new here) is that the free scanner wants to 
remember the highest-pfn
block where it managed to isolate some pages. If the following page 
migration fails, these isolated
pages might be put back and would be skipped in further compaction 
attempt if we used just
"next_free_pfn = pfn", until the scanners get reset.

The question of course is if such situations are frequent and makes any 
difference to compaction
outcome. And the downsides are potentially useless rescans and code 
complexity. Maybe Mel
remembers how important this is? It should probably be profiled before 
changes are made.

--
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 2/2] mm/compaction: cleanup isolate_freepages()

[Minchan Kim]

On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
> On 21.4.2014 21:41, Andrew Morton wrote:
> >On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
> >
> >>Hi Vlastimil,
> >>
> >>Below just nitpicks.
> >It seems you were ignored ;)
> 
> Oops, I managed to miss your e-mail, sorry.
> 
> >>>  {
> >>>  	struct page *page;
> >>>-	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
> >>>+	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> >>Could you add comment for each variable?
> >>
> >>unsigned long pfn; /* scanning cursor */
> >>unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
> >>unsigned long next_free_pfn; /* start pfn for scaning at next truen */
> >>unsigned long z_end_pfn; /* zone's end pfn */
> >>
> >>
> >>>@@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
> >>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>  	/*
> >>>-	 * Take care that if the migration scanner is at the end of the zone
> >>>-	 * that the free scanner does not accidentally move to the next zone
> >>>-	 * in the next isolation cycle.
> >>>+	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> >>>+	 * none, the pfn < low_pfn check will kick in.
> >>        "none" what? I'd like to clear more.
> 
> If there are no updates to next_free_pfn within the for cycle. Which
> matches Andrew's formulation below.
> 
> >I did this:
> 
> Thanks!
> 
> >
> >--- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
> >+++ a/mm/compaction.c
> >@@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
> >  				struct compact_control *cc)
> >  {
> >  	struct page *page;
> >-	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> >+	unsigned long pfn;	     /* scanning cursor */
> >+	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
> >+	unsigned long next_free_pfn; /* start pfn for scaning at next round */
> >+	unsigned long z_end_pfn;     /* zone's end pfn */
> 
> Yes that works.
> 
> >  	int nr_freepages = cc->nr_freepages;
> >  	struct list_head *freelist = &cc->freepages;
> >@@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
> >  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >  	/*
> >-	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> >-	 * none, the pfn < low_pfn check will kick in.
> >+	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> >+	 * isolated, the pfn < low_pfn check will kick in.
> 
> OK.
> 
> >  	 */
> >  	next_free_pfn = 0;
> >>>@@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
> >>>  	 * so that compact_finished() may detect this
> >>>  	 */
> >>>  	if (pfn < low_pfn)
> >>>-		cc->free_pfn = max(pfn, zone->zone_start_pfn);
> >>>-	else
> >>>-		cc->free_pfn = high_pfn;
> >>>+		next_free_pfn = max(pfn, zone->zone_start_pfn);
> >>Why we need max operation?
> >>IOW, what's the problem if we do (next_free_pfn = pfn)?
> >An answer to this would be useful, thanks.
> 
> The idea (originally, not new here) is that the free scanner wants
> to remember the highest-pfn
> block where it managed to isolate some pages. If the following page
> migration fails, these isolated
> pages might be put back and would be skipped in further compaction
> attempt if we used just
> "next_free_pfn = pfn", until the scanners get reset.
> 
> The question of course is if such situations are frequent and makes
> any difference to compaction
> outcome. And the downsides are potentially useless rescans and code
> complexity. Maybe Mel
> remembers how important this is? It should probably be profiled
> before changes are made.

I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
At that time, I didn't Cced so I missed that code so let's ask this time.
In that patch, you added this.

if (pfn < low_pfn)
  cc->free_pfn = max(pfn, zone->zone_start_pfn);
else
  cc->free_pfn = high_pfn;

So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
compact_finished to stop compaction. And your [1/2] patch in this patchset
always makes free page scanner start on pageblock boundary so when the
loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
would be lower than migration scanner so compact_finished will always detect
it so I think you could just do

if (pfn < low_pfn)
  next_free_pfn = pfn;

cc->free_pfn = next_free_pfn;

Or, if you want to clear *reset*,
if (pfn < lown_pfn)
  next_free_pfn = zone->zone_start_pfn;

cc->free_pfn = next_free_pfn;

That's why I asked about max operation. What am I missing?
> 
> --
> 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>

-- 
Kind regards,
Minchan Kim

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Minchan Kim]

On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
> On 21.4.2014 21:41, Andrew Morton wrote:
> >On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
> >
> >>Hi Vlastimil,
> >>
> >>Below just nitpicks.
> >It seems you were ignored ;)
> 
> Oops, I managed to miss your e-mail, sorry.
> 
> >>>  {
> >>>  	struct page *page;
> >>>-	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
> >>>+	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> >>Could you add comment for each variable?
> >>
> >>unsigned long pfn; /* scanning cursor */
> >>unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
> >>unsigned long next_free_pfn; /* start pfn for scaning at next truen */
> >>unsigned long z_end_pfn; /* zone's end pfn */
> >>
> >>
> >>>@@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
> >>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>  	/*
> >>>-	 * Take care that if the migration scanner is at the end of the zone
> >>>-	 * that the free scanner does not accidentally move to the next zone
> >>>-	 * in the next isolation cycle.
> >>>+	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> >>>+	 * none, the pfn < low_pfn check will kick in.
> >>        "none" what? I'd like to clear more.
> 
> If there are no updates to next_free_pfn within the for cycle. Which
> matches Andrew's formulation below.
> 
> >I did this:
> 
> Thanks!
> 
> >
> >--- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
> >+++ a/mm/compaction.c
> >@@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
> >  				struct compact_control *cc)
> >  {
> >  	struct page *page;
> >-	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> >+	unsigned long pfn;	     /* scanning cursor */
> >+	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
> >+	unsigned long next_free_pfn; /* start pfn for scaning at next round */
> >+	unsigned long z_end_pfn;     /* zone's end pfn */
> 
> Yes that works.
> 
> >  	int nr_freepages = cc->nr_freepages;
> >  	struct list_head *freelist = &cc->freepages;
> >@@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
> >  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >  	/*
> >-	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> >-	 * none, the pfn < low_pfn check will kick in.
> >+	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> >+	 * isolated, the pfn < low_pfn check will kick in.
> 
> OK.
> 
> >  	 */
> >  	next_free_pfn = 0;
> >>>@@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
> >>>  	 * so that compact_finished() may detect this
> >>>  	 */
> >>>  	if (pfn < low_pfn)
> >>>-		cc->free_pfn = max(pfn, zone->zone_start_pfn);
> >>>-	else
> >>>-		cc->free_pfn = high_pfn;
> >>>+		next_free_pfn = max(pfn, zone->zone_start_pfn);
> >>Why we need max operation?
> >>IOW, what's the problem if we do (next_free_pfn = pfn)?
> >An answer to this would be useful, thanks.
> 
> The idea (originally, not new here) is that the free scanner wants
> to remember the highest-pfn
> block where it managed to isolate some pages. If the following page
> migration fails, these isolated
> pages might be put back and would be skipped in further compaction
> attempt if we used just
> "next_free_pfn = pfn", until the scanners get reset.
> 
> The question of course is if such situations are frequent and makes
> any difference to compaction
> outcome. And the downsides are potentially useless rescans and code
> complexity. Maybe Mel
> remembers how important this is? It should probably be profiled
> before changes are made.

I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
At that time, I didn't Cced so I missed that code so let's ask this time.
In that patch, you added this.

if (pfn < low_pfn)
  cc->free_pfn = max(pfn, zone->zone_start_pfn);
else
  cc->free_pfn = high_pfn;

So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
compact_finished to stop compaction. And your [1/2] patch in this patchset
always makes free page scanner start on pageblock boundary so when the
loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
would be lower than migration scanner so compact_finished will always detect
it so I think you could just do

if (pfn < low_pfn)
  next_free_pfn = pfn;

cc->free_pfn = next_free_pfn;

Or, if you want to clear *reset*,
if (pfn < lown_pfn)
  next_free_pfn = zone->zone_start_pfn;

cc->free_pfn = next_free_pfn;

That's why I asked about max operation. What am I missing?
> 
> --
> 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>

-- 
Kind regards,
Minchan Kim

--
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 2/2] mm/compaction: cleanup isolate_freepages()

[Vlastimil Babka]

On 22.4.2014 1:53, Minchan Kim wrote:
> On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
>> On 21.4.2014 21:41, Andrew Morton wrote:
>>> On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
>>>
>>>> Hi Vlastimil,
>>>>
>>>> Below just nitpicks.
>>> It seems you were ignored ;)
>> Oops, I managed to miss your e-mail, sorry.
>>
>>>>>   {
>>>>>   	struct page *page;
>>>>> -	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
>>>>> +	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>>> Could you add comment for each variable?
>>>>
>>>> unsigned long pfn; /* scanning cursor */
>>>> unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
>>>> unsigned long next_free_pfn; /* start pfn for scaning at next truen */
>>>> unsigned long z_end_pfn; /* zone's end pfn */
>>>>
>>>>
>>>>> @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
>>>>>   	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>   	/*
>>>>> -	 * Take care that if the migration scanner is at the end of the zone
>>>>> -	 * that the free scanner does not accidentally move to the next zone
>>>>> -	 * in the next isolation cycle.
>>>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>>>> +	 * none, the pfn < low_pfn check will kick in.
>>>>         "none" what? I'd like to clear more.
>> If there are no updates to next_free_pfn within the for cycle. Which
>> matches Andrew's formulation below.
>>
>>> I did this:
>> Thanks!
>>
>>> --- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
>>> +++ a/mm/compaction.c
>>> @@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
>>>   				struct compact_control *cc)
>>>   {
>>>   	struct page *page;
>>> -	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>> +	unsigned long pfn;	     /* scanning cursor */
>>> +	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
>>> +	unsigned long next_free_pfn; /* start pfn for scaning at next round */
>>> +	unsigned long z_end_pfn;     /* zone's end pfn */
>> Yes that works.
>>
>>>   	int nr_freepages = cc->nr_freepages;
>>>   	struct list_head *freelist = &cc->freepages;
>>> @@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
>>>   	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>   	/*
>>> -	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>> -	 * none, the pfn < low_pfn check will kick in.
>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>>> +	 * isolated, the pfn < low_pfn check will kick in.
>> OK.
>>
>>>   	 */
>>>   	next_free_pfn = 0;
>>>>> @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
>>>>>   	 * so that compact_finished() may detect this
>>>>>   	 */
>>>>>   	if (pfn < low_pfn)
>>>>> -		cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>>>> -	else
>>>>> -		cc->free_pfn = high_pfn;
>>>>> +		next_free_pfn = max(pfn, zone->zone_start_pfn);
>>>> Why we need max operation?
>>>> IOW, what's the problem if we do (next_free_pfn = pfn)?
>>> An answer to this would be useful, thanks.
>> The idea (originally, not new here) is that the free scanner wants
>> to remember the highest-pfn
>> block where it managed to isolate some pages. If the following page
>> migration fails, these isolated
>> pages might be put back and would be skipped in further compaction
>> attempt if we used just
>> "next_free_pfn = pfn", until the scanners get reset.
>>
>> The question of course is if such situations are frequent and makes
>> any difference to compaction
>> outcome. And the downsides are potentially useless rescans and code
>> complexity. Maybe Mel
>> remembers how important this is? It should probably be profiled
>> before changes are made.
> I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
> At that time, I didn't Cced so I missed that code so let's ask this time.
> In that patch, you added this.
>
> if (pfn < low_pfn)
>    cc->free_pfn = max(pfn, zone->zone_start_pfn);
> else
>    cc->free_pfn = high_pfn;

Oh, right, this max(), not the one in the for loop. Sorry, I should have 
read more closely.
But still maybe it's a good opportunity to kill the other max() as well. 
I'll try some testing.

Anyway, this is what I answered to Mel when he asked the same thing when 
I sent
that 7ed695069c3c patch:

If a zone starts in a middle of a pageblock and migrate scanner isolates
enough pages early to stay within that pageblock, low_pfn will be at the
end of that pageblock and after the for cycle in this function ends, pfn
might be at the beginning of that pageblock. It might not be an actual
problem (this compaction will finish at this point, and if someone else
is racing, he will probably check the boundaries himself), but I played
it safe.


> So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
> compact_finished to stop compaction. And your [1/2] patch in this patchset
> always makes free page scanner start on pageblock boundary so when the
> loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
> would be lower than migration scanner so compact_finished will always detect
> it so I think you could just do
>
> if (pfn < low_pfn)
>    next_free_pfn = pfn;
>
> cc->free_pfn = next_free_pfn;

That could work. I was probably wrong about danger of racing in the 
reply to Mel,
because free_pfn is stored in cc (private), not zone (shared).

>
> Or, if you want to clear *reset*,
> if (pfn < lown_pfn)
>    next_free_pfn = zone->zone_start_pfn;
>
> cc->free_pfn = next_free_pfn;

That would work as well but is less straightforward I think. Might be 
misleading if
someone added tracepoints to track the free scanner progress with pfn's 
(which
might happen soon...)

> That's why I asked about max operation. What am I missing?
>> --
>> 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 2/2] mm/compaction: cleanup isolate_freepages()

[Vlastimil Babka]

On 22.4.2014 1:53, Minchan Kim wrote:
> On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
>> On 21.4.2014 21:41, Andrew Morton wrote:
>>> On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
>>>
>>>> Hi Vlastimil,
>>>>
>>>> Below just nitpicks.
>>> It seems you were ignored ;)
>> Oops, I managed to miss your e-mail, sorry.
>>
>>>>>   {
>>>>>   	struct page *page;
>>>>> -	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
>>>>> +	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>>> Could you add comment for each variable?
>>>>
>>>> unsigned long pfn; /* scanning cursor */
>>>> unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
>>>> unsigned long next_free_pfn; /* start pfn for scaning at next truen */
>>>> unsigned long z_end_pfn; /* zone's end pfn */
>>>>
>>>>
>>>>> @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
>>>>>   	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>   	/*
>>>>> -	 * Take care that if the migration scanner is at the end of the zone
>>>>> -	 * that the free scanner does not accidentally move to the next zone
>>>>> -	 * in the next isolation cycle.
>>>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>>>> +	 * none, the pfn < low_pfn check will kick in.
>>>>         "none" what? I'd like to clear more.
>> If there are no updates to next_free_pfn within the for cycle. Which
>> matches Andrew's formulation below.
>>
>>> I did this:
>> Thanks!
>>
>>> --- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
>>> +++ a/mm/compaction.c
>>> @@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
>>>   				struct compact_control *cc)
>>>   {
>>>   	struct page *page;
>>> -	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>> +	unsigned long pfn;	     /* scanning cursor */
>>> +	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
>>> +	unsigned long next_free_pfn; /* start pfn for scaning at next round */
>>> +	unsigned long z_end_pfn;     /* zone's end pfn */
>> Yes that works.
>>
>>>   	int nr_freepages = cc->nr_freepages;
>>>   	struct list_head *freelist = &cc->freepages;
>>> @@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
>>>   	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>   	/*
>>> -	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>> -	 * none, the pfn < low_pfn check will kick in.
>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>>> +	 * isolated, the pfn < low_pfn check will kick in.
>> OK.
>>
>>>   	 */
>>>   	next_free_pfn = 0;
>>>>> @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
>>>>>   	 * so that compact_finished() may detect this
>>>>>   	 */
>>>>>   	if (pfn < low_pfn)
>>>>> -		cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>>>> -	else
>>>>> -		cc->free_pfn = high_pfn;
>>>>> +		next_free_pfn = max(pfn, zone->zone_start_pfn);
>>>> Why we need max operation?
>>>> IOW, what's the problem if we do (next_free_pfn = pfn)?
>>> An answer to this would be useful, thanks.
>> The idea (originally, not new here) is that the free scanner wants
>> to remember the highest-pfn
>> block where it managed to isolate some pages. If the following page
>> migration fails, these isolated
>> pages might be put back and would be skipped in further compaction
>> attempt if we used just
>> "next_free_pfn = pfn", until the scanners get reset.
>>
>> The question of course is if such situations are frequent and makes
>> any difference to compaction
>> outcome. And the downsides are potentially useless rescans and code
>> complexity. Maybe Mel
>> remembers how important this is? It should probably be profiled
>> before changes are made.
> I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
> At that time, I didn't Cced so I missed that code so let's ask this time.
> In that patch, you added this.
>
> if (pfn < low_pfn)
>    cc->free_pfn = max(pfn, zone->zone_start_pfn);
> else
>    cc->free_pfn = high_pfn;

Oh, right, this max(), not the one in the for loop. Sorry, I should have 
read more closely.
But still maybe it's a good opportunity to kill the other max() as well. 
I'll try some testing.

Anyway, this is what I answered to Mel when he asked the same thing when 
I sent
that 7ed695069c3c patch:

If a zone starts in a middle of a pageblock and migrate scanner isolates
enough pages early to stay within that pageblock, low_pfn will be at the
end of that pageblock and after the for cycle in this function ends, pfn
might be at the beginning of that pageblock. It might not be an actual
problem (this compaction will finish at this point, and if someone else
is racing, he will probably check the boundaries himself), but I played
it safe.


> So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
> compact_finished to stop compaction. And your [1/2] patch in this patchset
> always makes free page scanner start on pageblock boundary so when the
> loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
> would be lower than migration scanner so compact_finished will always detect
> it so I think you could just do
>
> if (pfn < low_pfn)
>    next_free_pfn = pfn;
>
> cc->free_pfn = next_free_pfn;

That could work. I was probably wrong about danger of racing in the 
reply to Mel,
because free_pfn is stored in cc (private), not zone (shared).

>
> Or, if you want to clear *reset*,
> if (pfn < lown_pfn)
>    next_free_pfn = zone->zone_start_pfn;
>
> cc->free_pfn = next_free_pfn;

That would work as well but is less straightforward I think. Might be 
misleading if
someone added tracepoints to track the free scanner progress with pfn's 
(which
might happen soon...)

> That's why I asked about max operation. What am I missing?
>> --
>> 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 2/2] mm/compaction: cleanup isolate_freepages()

[Minchan Kim]

On Tue, Apr 22, 2014 at 08:33:35AM +0200, Vlastimil Babka wrote:
> On 22.4.2014 1:53, Minchan Kim wrote:
> >On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
> >>On 21.4.2014 21:41, Andrew Morton wrote:
> >>>On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
> >>>
> >>>>Hi Vlastimil,
> >>>>
> >>>>Below just nitpicks.
> >>>It seems you were ignored ;)
> >>Oops, I managed to miss your e-mail, sorry.
> >>
> >>>>>  {
> >>>>>  	struct page *page;
> >>>>>-	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
> >>>>>+	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> >>>>Could you add comment for each variable?
> >>>>
> >>>>unsigned long pfn; /* scanning cursor */
> >>>>unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
> >>>>unsigned long next_free_pfn; /* start pfn for scaning at next truen */
> >>>>unsigned long z_end_pfn; /* zone's end pfn */
> >>>>
> >>>>
> >>>>>@@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
> >>>>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>>>  	/*
> >>>>>-	 * Take care that if the migration scanner is at the end of the zone
> >>>>>-	 * that the free scanner does not accidentally move to the next zone
> >>>>>-	 * in the next isolation cycle.
> >>>>>+	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> >>>>>+	 * none, the pfn < low_pfn check will kick in.
> >>>>        "none" what? I'd like to clear more.
> >>If there are no updates to next_free_pfn within the for cycle. Which
> >>matches Andrew's formulation below.
> >>
> >>>I did this:
> >>Thanks!
> >>
> >>>--- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
> >>>+++ a/mm/compaction.c
> >>>@@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
> >>>  				struct compact_control *cc)
> >>>  {
> >>>  	struct page *page;
> >>>-	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> >>>+	unsigned long pfn;	     /* scanning cursor */
> >>>+	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
> >>>+	unsigned long next_free_pfn; /* start pfn for scaning at next round */
> >>>+	unsigned long z_end_pfn;     /* zone's end pfn */
> >>Yes that works.
> >>
> >>>  	int nr_freepages = cc->nr_freepages;
> >>>  	struct list_head *freelist = &cc->freepages;
> >>>@@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
> >>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>  	/*
> >>>-	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> >>>-	 * none, the pfn < low_pfn check will kick in.
> >>>+	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> >>>+	 * isolated, the pfn < low_pfn check will kick in.
> >>OK.
> >>
> >>>  	 */
> >>>  	next_free_pfn = 0;
> >>>>>@@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
> >>>>>  	 * so that compact_finished() may detect this
> >>>>>  	 */
> >>>>>  	if (pfn < low_pfn)
> >>>>>-		cc->free_pfn = max(pfn, zone->zone_start_pfn);
> >>>>>-	else
> >>>>>-		cc->free_pfn = high_pfn;
> >>>>>+		next_free_pfn = max(pfn, zone->zone_start_pfn);
> >>>>Why we need max operation?
> >>>>IOW, what's the problem if we do (next_free_pfn = pfn)?
> >>>An answer to this would be useful, thanks.
> >>The idea (originally, not new here) is that the free scanner wants
> >>to remember the highest-pfn
> >>block where it managed to isolate some pages. If the following page
> >>migration fails, these isolated
> >>pages might be put back and would be skipped in further compaction
> >>attempt if we used just
> >>"next_free_pfn = pfn", until the scanners get reset.
> >>
> >>The question of course is if such situations are frequent and makes
> >>any difference to compaction
> >>outcome. And the downsides are potentially useless rescans and code
> >>complexity. Maybe Mel
> >>remembers how important this is? It should probably be profiled
> >>before changes are made.
> >I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
> >At that time, I didn't Cced so I missed that code so let's ask this time.
> >In that patch, you added this.
> >
> >if (pfn < low_pfn)
> >   cc->free_pfn = max(pfn, zone->zone_start_pfn);
> >else
> >   cc->free_pfn = high_pfn;
> 
> Oh, right, this max(), not the one in the for loop. Sorry, I should
> have read more closely.
> But still maybe it's a good opportunity to kill the other max() as
> well. I'll try some testing.
> 
> Anyway, this is what I answered to Mel when he asked the same thing
> when I sent
> that 7ed695069c3c patch:
> 
> If a zone starts in a middle of a pageblock and migrate scanner isolates
> enough pages early to stay within that pageblock, low_pfn will be at the
> end of that pageblock and after the for cycle in this function ends, pfn
> might be at the beginning of that pageblock. It might not be an actual
> problem (this compaction will finish at this point, and if someone else
> is racing, he will probably check the boundaries himself), but I played
> it safe.
> 
> 
> >So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
> >compact_finished to stop compaction. And your [1/2] patch in this patchset
> >always makes free page scanner start on pageblock boundary so when the
> >loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
> >would be lower than migration scanner so compact_finished will always detect
> >it so I think you could just do
> >
> >if (pfn < low_pfn)
> >   next_free_pfn = pfn;
> >
> >cc->free_pfn = next_free_pfn;
> 
> That could work. I was probably wrong about danger of racing in the
> reply to Mel,
> because free_pfn is stored in cc (private), not zone (shared).
> 
> >
> >Or, if you want to clear *reset*,
> >if (pfn < lown_pfn)
> >   next_free_pfn = zone->zone_start_pfn;
> >
> >cc->free_pfn = next_free_pfn;
> 
> That would work as well but is less straightforward I think. Might
> be misleading if
> someone added tracepoints to track the free scanner progress with
> pfn's (which
> might happen soon...)

My preference is to add following with pair of compact_finished

static inline void finish_compact(struct compact_control *cc)
{
  cc->free_pfn = cc->migrate_pfn;
}

But I don't care.
If you didn't send this patch as clean up, I would never interrupt
on the way but you said it's cleanup patch and the one made me spend a
few minutes to understand the code so it's not a clean up patch. ;-).
So, IMO, it's worth to tidy it up.


-- 
Kind regards,
Minchan Kim

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Minchan Kim]

On Tue, Apr 22, 2014 at 08:33:35AM +0200, Vlastimil Babka wrote:
> On 22.4.2014 1:53, Minchan Kim wrote:
> >On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
> >>On 21.4.2014 21:41, Andrew Morton wrote:
> >>>On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
> >>>
> >>>>Hi Vlastimil,
> >>>>
> >>>>Below just nitpicks.
> >>>It seems you were ignored ;)
> >>Oops, I managed to miss your e-mail, sorry.
> >>
> >>>>>  {
> >>>>>  	struct page *page;
> >>>>>-	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
> >>>>>+	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> >>>>Could you add comment for each variable?
> >>>>
> >>>>unsigned long pfn; /* scanning cursor */
> >>>>unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
> >>>>unsigned long next_free_pfn; /* start pfn for scaning at next truen */
> >>>>unsigned long z_end_pfn; /* zone's end pfn */
> >>>>
> >>>>
> >>>>>@@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
> >>>>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>>>  	/*
> >>>>>-	 * Take care that if the migration scanner is at the end of the zone
> >>>>>-	 * that the free scanner does not accidentally move to the next zone
> >>>>>-	 * in the next isolation cycle.
> >>>>>+	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> >>>>>+	 * none, the pfn < low_pfn check will kick in.
> >>>>        "none" what? I'd like to clear more.
> >>If there are no updates to next_free_pfn within the for cycle. Which
> >>matches Andrew's formulation below.
> >>
> >>>I did this:
> >>Thanks!
> >>
> >>>--- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
> >>>+++ a/mm/compaction.c
> >>>@@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
> >>>  				struct compact_control *cc)
> >>>  {
> >>>  	struct page *page;
> >>>-	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> >>>+	unsigned long pfn;	     /* scanning cursor */
> >>>+	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
> >>>+	unsigned long next_free_pfn; /* start pfn for scaning at next round */
> >>>+	unsigned long z_end_pfn;     /* zone's end pfn */
> >>Yes that works.
> >>
> >>>  	int nr_freepages = cc->nr_freepages;
> >>>  	struct list_head *freelist = &cc->freepages;
> >>>@@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
> >>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>  	/*
> >>>-	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> >>>-	 * none, the pfn < low_pfn check will kick in.
> >>>+	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> >>>+	 * isolated, the pfn < low_pfn check will kick in.
> >>OK.
> >>
> >>>  	 */
> >>>  	next_free_pfn = 0;
> >>>>>@@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
> >>>>>  	 * so that compact_finished() may detect this
> >>>>>  	 */
> >>>>>  	if (pfn < low_pfn)
> >>>>>-		cc->free_pfn = max(pfn, zone->zone_start_pfn);
> >>>>>-	else
> >>>>>-		cc->free_pfn = high_pfn;
> >>>>>+		next_free_pfn = max(pfn, zone->zone_start_pfn);
> >>>>Why we need max operation?
> >>>>IOW, what's the problem if we do (next_free_pfn = pfn)?
> >>>An answer to this would be useful, thanks.
> >>The idea (originally, not new here) is that the free scanner wants
> >>to remember the highest-pfn
> >>block where it managed to isolate some pages. If the following page
> >>migration fails, these isolated
> >>pages might be put back and would be skipped in further compaction
> >>attempt if we used just
> >>"next_free_pfn = pfn", until the scanners get reset.
> >>
> >>The question of course is if such situations are frequent and makes
> >>any difference to compaction
> >>outcome. And the downsides are potentially useless rescans and code
> >>complexity. Maybe Mel
> >>remembers how important this is? It should probably be profiled
> >>before changes are made.
> >I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
> >At that time, I didn't Cced so I missed that code so let's ask this time.
> >In that patch, you added this.
> >
> >if (pfn < low_pfn)
> >   cc->free_pfn = max(pfn, zone->zone_start_pfn);
> >else
> >   cc->free_pfn = high_pfn;
> 
> Oh, right, this max(), not the one in the for loop. Sorry, I should
> have read more closely.
> But still maybe it's a good opportunity to kill the other max() as
> well. I'll try some testing.
> 
> Anyway, this is what I answered to Mel when he asked the same thing
> when I sent
> that 7ed695069c3c patch:
> 
> If a zone starts in a middle of a pageblock and migrate scanner isolates
> enough pages early to stay within that pageblock, low_pfn will be at the
> end of that pageblock and after the for cycle in this function ends, pfn
> might be at the beginning of that pageblock. It might not be an actual
> problem (this compaction will finish at this point, and if someone else
> is racing, he will probably check the boundaries himself), but I played
> it safe.
> 
> 
> >So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
> >compact_finished to stop compaction. And your [1/2] patch in this patchset
> >always makes free page scanner start on pageblock boundary so when the
> >loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
> >would be lower than migration scanner so compact_finished will always detect
> >it so I think you could just do
> >
> >if (pfn < low_pfn)
> >   next_free_pfn = pfn;
> >
> >cc->free_pfn = next_free_pfn;
> 
> That could work. I was probably wrong about danger of racing in the
> reply to Mel,
> because free_pfn is stored in cc (private), not zone (shared).
> 
> >
> >Or, if you want to clear *reset*,
> >if (pfn < lown_pfn)
> >   next_free_pfn = zone->zone_start_pfn;
> >
> >cc->free_pfn = next_free_pfn;
> 
> That would work as well but is less straightforward I think. Might
> be misleading if
> someone added tracepoints to track the free scanner progress with
> pfn's (which
> might happen soon...)

My preference is to add following with pair of compact_finished

static inline void finish_compact(struct compact_control *cc)
{
  cc->free_pfn = cc->migrate_pfn;
}

But I don't care.
If you didn't send this patch as clean up, I would never interrupt
on the way but you said it's cleanup patch and the one made me spend a
few minutes to understand the code so it's not a clean up patch. ;-).
So, IMO, it's worth to tidy it up.


-- 
Kind regards,
Minchan Kim

--
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 2/2] mm/compaction: cleanup isolate_freepages()

[Vlastimil Babka]

On 04/22/2014 08:52 AM, Minchan Kim wrote:
> On Tue, Apr 22, 2014 at 08:33:35AM +0200, Vlastimil Babka wrote:
>> On 22.4.2014 1:53, Minchan Kim wrote:
>>> On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
>>>> On 21.4.2014 21:41, Andrew Morton wrote:
>>>>> On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
>>>>>
>>>>>> Hi Vlastimil,
>>>>>>
>>>>>> Below just nitpicks.
>>>>> It seems you were ignored ;)
>>>> Oops, I managed to miss your e-mail, sorry.
>>>>
>>>>>>>  {
>>>>>>>  	struct page *page;
>>>>>>> -	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
>>>>>>> +	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>>>>> Could you add comment for each variable?
>>>>>>
>>>>>> unsigned long pfn; /* scanning cursor */
>>>>>> unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
>>>>>> unsigned long next_free_pfn; /* start pfn for scaning at next truen */
>>>>>> unsigned long z_end_pfn; /* zone's end pfn */
>>>>>>
>>>>>>
>>>>>>> @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
>>>>>>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>>>  	/*
>>>>>>> -	 * Take care that if the migration scanner is at the end of the zone
>>>>>>> -	 * that the free scanner does not accidentally move to the next zone
>>>>>>> -	 * in the next isolation cycle.
>>>>>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>>>>>> +	 * none, the pfn < low_pfn check will kick in.
>>>>>>        "none" what? I'd like to clear more.
>>>> If there are no updates to next_free_pfn within the for cycle. Which
>>>> matches Andrew's formulation below.
>>>>
>>>>> I did this:
>>>> Thanks!
>>>>
>>>>> --- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
>>>>> +++ a/mm/compaction.c
>>>>> @@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
>>>>>  				struct compact_control *cc)
>>>>>  {
>>>>>  	struct page *page;
>>>>> -	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>>>> +	unsigned long pfn;	     /* scanning cursor */
>>>>> +	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
>>>>> +	unsigned long next_free_pfn; /* start pfn for scaning at next round */
>>>>> +	unsigned long z_end_pfn;     /* zone's end pfn */
>>>> Yes that works.
>>>>
>>>>>  	int nr_freepages = cc->nr_freepages;
>>>>>  	struct list_head *freelist = &cc->freepages;
>>>>> @@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
>>>>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>  	/*
>>>>> -	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>>>> -	 * none, the pfn < low_pfn check will kick in.
>>>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>>>>> +	 * isolated, the pfn < low_pfn check will kick in.
>>>> OK.
>>>>
>>>>>  	 */
>>>>>  	next_free_pfn = 0;
>>>>>>> @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
>>>>>>>  	 * so that compact_finished() may detect this
>>>>>>>  	 */
>>>>>>>  	if (pfn < low_pfn)
>>>>>>> -		cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>>>>>> -	else
>>>>>>> -		cc->free_pfn = high_pfn;
>>>>>>> +		next_free_pfn = max(pfn, zone->zone_start_pfn);
>>>>>> Why we need max operation?
>>>>>> IOW, what's the problem if we do (next_free_pfn = pfn)?
>>>>> An answer to this would be useful, thanks.
>>>> The idea (originally, not new here) is that the free scanner wants
>>>> to remember the highest-pfn
>>>> block where it managed to isolate some pages. If the following page
>>>> migration fails, these isolated
>>>> pages might be put back and would be skipped in further compaction
>>>> attempt if we used just
>>>> "next_free_pfn = pfn", until the scanners get reset.
>>>>
>>>> The question of course is if such situations are frequent and makes
>>>> any difference to compaction
>>>> outcome. And the downsides are potentially useless rescans and code
>>>> complexity. Maybe Mel
>>>> remembers how important this is? It should probably be profiled
>>>> before changes are made.
>>> I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
>>> At that time, I didn't Cced so I missed that code so let's ask this time.
>>> In that patch, you added this.
>>>
>>> if (pfn < low_pfn)
>>>   cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>> else
>>>   cc->free_pfn = high_pfn;
>>
>> Oh, right, this max(), not the one in the for loop. Sorry, I should
>> have read more closely.
>> But still maybe it's a good opportunity to kill the other max() as
>> well. I'll try some testing.
>>
>> Anyway, this is what I answered to Mel when he asked the same thing
>> when I sent
>> that 7ed695069c3c patch:
>>
>> If a zone starts in a middle of a pageblock and migrate scanner isolates
>> enough pages early to stay within that pageblock, low_pfn will be at the
>> end of that pageblock and after the for cycle in this function ends, pfn
>> might be at the beginning of that pageblock. It might not be an actual
>> problem (this compaction will finish at this point, and if someone else
>> is racing, he will probably check the boundaries himself), but I played
>> it safe.
>>
>>
>>> So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
>>> compact_finished to stop compaction. And your [1/2] patch in this patchset
>>> always makes free page scanner start on pageblock boundary so when the
>>> loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
>>> would be lower than migration scanner so compact_finished will always detect
>>> it so I think you could just do
>>>
>>> if (pfn < low_pfn)
>>>   next_free_pfn = pfn;
>>>
>>> cc->free_pfn = next_free_pfn;
>>
>> That could work. I was probably wrong about danger of racing in the
>> reply to Mel,
>> because free_pfn is stored in cc (private), not zone (shared).
>>
>>>
>>> Or, if you want to clear *reset*,
>>> if (pfn < lown_pfn)
>>>   next_free_pfn = zone->zone_start_pfn;
>>>
>>> cc->free_pfn = next_free_pfn;
>>
>> That would work as well but is less straightforward I think. Might
>> be misleading if
>> someone added tracepoints to track the free scanner progress with
>> pfn's (which
>> might happen soon...)
> 
> My preference is to add following with pair of compact_finished
> 
> static inline void finish_compact(struct compact_control *cc)
> {
>   cc->free_pfn = cc->migrate_pfn;
> }

Yes setting free_pfn to migrate_pfn is probably the best way, as these
are the values compared in compact_finished. But I wouldn't introduce a
new function just for one instance of this. Also compact_finished()
doesn't test just the scanners to decide whether compaction should
continue, so the pairing would be imperfect anyway.
So Andrew, if you agree can you please fold in the patch below.

> But I don't care.
> If you didn't send this patch as clean up, I would never interrupt
> on the way but you said it's cleanup patch and the one made me spend a
> few minutes to understand the code so it's not a clean up patch. ;-).
> So, IMO, it's worth to tidy it up.

Yes, I understand and agree.

------8<------
From: Vlastimil Babka <vbabka@suse.cz>
Date: Tue, 22 Apr 2014 13:55:36 +0200
Subject: mm-compaction-cleanup-isolate_freepages-fix2

Cleanup detection of compaction scanners crossing in isolate_freepages().
To make sure compact_finished() observes scanners crossing, we can just set
free_pfn to migrate_pfn instead of confusing max() construct.

Suggested-by: Minchan Kim <minchan@kernel.org>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Dongjun Shin <d.j.shin@samsung.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Sunghwan Yun <sunghwan.yun@samsung.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

---
 mm/compaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 37c15fe..1c992dc 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -768,7 +768,7 @@ static void isolate_freepages(struct zone *zone,
 	 * so that compact_finished() may detect this
 	 */
 	if (pfn < low_pfn)
-		next_free_pfn = max(pfn, zone->zone_start_pfn);
+		next_free_pfn = cc->migrate_pfn;
 
 	cc->free_pfn = next_free_pfn;
 	cc->nr_freepages = nr_freepages;
-- 
1.8.4.5

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Vlastimil Babka]

On 04/22/2014 08:52 AM, Minchan Kim wrote:
> On Tue, Apr 22, 2014 at 08:33:35AM +0200, Vlastimil Babka wrote:
>> On 22.4.2014 1:53, Minchan Kim wrote:
>>> On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
>>>> On 21.4.2014 21:41, Andrew Morton wrote:
>>>>> On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
>>>>>
>>>>>> Hi Vlastimil,
>>>>>>
>>>>>> Below just nitpicks.
>>>>> It seems you were ignored ;)
>>>> Oops, I managed to miss your e-mail, sorry.
>>>>
>>>>>>>  {
>>>>>>>  	struct page *page;
>>>>>>> -	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
>>>>>>> +	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>>>>> Could you add comment for each variable?
>>>>>>
>>>>>> unsigned long pfn; /* scanning cursor */
>>>>>> unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
>>>>>> unsigned long next_free_pfn; /* start pfn for scaning at next truen */
>>>>>> unsigned long z_end_pfn; /* zone's end pfn */
>>>>>>
>>>>>>
>>>>>>> @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
>>>>>>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>>>  	/*
>>>>>>> -	 * Take care that if the migration scanner is at the end of the zone
>>>>>>> -	 * that the free scanner does not accidentally move to the next zone
>>>>>>> -	 * in the next isolation cycle.
>>>>>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>>>>>> +	 * none, the pfn < low_pfn check will kick in.
>>>>>>        "none" what? I'd like to clear more.
>>>> If there are no updates to next_free_pfn within the for cycle. Which
>>>> matches Andrew's formulation below.
>>>>
>>>>> I did this:
>>>> Thanks!
>>>>
>>>>> --- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
>>>>> +++ a/mm/compaction.c
>>>>> @@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
>>>>>  				struct compact_control *cc)
>>>>>  {
>>>>>  	struct page *page;
>>>>> -	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>>>> +	unsigned long pfn;	     /* scanning cursor */
>>>>> +	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
>>>>> +	unsigned long next_free_pfn; /* start pfn for scaning at next round */
>>>>> +	unsigned long z_end_pfn;     /* zone's end pfn */
>>>> Yes that works.
>>>>
>>>>>  	int nr_freepages = cc->nr_freepages;
>>>>>  	struct list_head *freelist = &cc->freepages;
>>>>> @@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
>>>>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>  	/*
>>>>> -	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>>>> -	 * none, the pfn < low_pfn check will kick in.
>>>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>>>>> +	 * isolated, the pfn < low_pfn check will kick in.
>>>> OK.
>>>>
>>>>>  	 */
>>>>>  	next_free_pfn = 0;
>>>>>>> @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
>>>>>>>  	 * so that compact_finished() may detect this
>>>>>>>  	 */
>>>>>>>  	if (pfn < low_pfn)
>>>>>>> -		cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>>>>>> -	else
>>>>>>> -		cc->free_pfn = high_pfn;
>>>>>>> +		next_free_pfn = max(pfn, zone->zone_start_pfn);
>>>>>> Why we need max operation?
>>>>>> IOW, what's the problem if we do (next_free_pfn = pfn)?
>>>>> An answer to this would be useful, thanks.
>>>> The idea (originally, not new here) is that the free scanner wants
>>>> to remember the highest-pfn
>>>> block where it managed to isolate some pages. If the following page
>>>> migration fails, these isolated
>>>> pages might be put back and would be skipped in further compaction
>>>> attempt if we used just
>>>> "next_free_pfn = pfn", until the scanners get reset.
>>>>
>>>> The question of course is if such situations are frequent and makes
>>>> any difference to compaction
>>>> outcome. And the downsides are potentially useless rescans and code
>>>> complexity. Maybe Mel
>>>> remembers how important this is? It should probably be profiled
>>>> before changes are made.
>>> I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
>>> At that time, I didn't Cced so I missed that code so let's ask this time.
>>> In that patch, you added this.
>>>
>>> if (pfn < low_pfn)
>>>   cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>> else
>>>   cc->free_pfn = high_pfn;
>>
>> Oh, right, this max(), not the one in the for loop. Sorry, I should
>> have read more closely.
>> But still maybe it's a good opportunity to kill the other max() as
>> well. I'll try some testing.
>>
>> Anyway, this is what I answered to Mel when he asked the same thing
>> when I sent
>> that 7ed695069c3c patch:
>>
>> If a zone starts in a middle of a pageblock and migrate scanner isolates
>> enough pages early to stay within that pageblock, low_pfn will be at the
>> end of that pageblock and after the for cycle in this function ends, pfn
>> might be at the beginning of that pageblock. It might not be an actual
>> problem (this compaction will finish at this point, and if someone else
>> is racing, he will probably check the boundaries himself), but I played
>> it safe.
>>
>>
>>> So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
>>> compact_finished to stop compaction. And your [1/2] patch in this patchset
>>> always makes free page scanner start on pageblock boundary so when the
>>> loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
>>> would be lower than migration scanner so compact_finished will always detect
>>> it so I think you could just do
>>>
>>> if (pfn < low_pfn)
>>>   next_free_pfn = pfn;
>>>
>>> cc->free_pfn = next_free_pfn;
>>
>> That could work. I was probably wrong about danger of racing in the
>> reply to Mel,
>> because free_pfn is stored in cc (private), not zone (shared).
>>
>>>
>>> Or, if you want to clear *reset*,
>>> if (pfn < lown_pfn)
>>>   next_free_pfn = zone->zone_start_pfn;
>>>
>>> cc->free_pfn = next_free_pfn;
>>
>> That would work as well but is less straightforward I think. Might
>> be misleading if
>> someone added tracepoints to track the free scanner progress with
>> pfn's (which
>> might happen soon...)
> 
> My preference is to add following with pair of compact_finished
> 
> static inline void finish_compact(struct compact_control *cc)
> {
>   cc->free_pfn = cc->migrate_pfn;
> }

Yes setting free_pfn to migrate_pfn is probably the best way, as these
are the values compared in compact_finished. But I wouldn't introduce a
new function just for one instance of this. Also compact_finished()
doesn't test just the scanners to decide whether compaction should
continue, so the pairing would be imperfect anyway.
So Andrew, if you agree can you please fold in the patch below.

> But I don't care.
> If you didn't send this patch as clean up, I would never interrupt
> on the way but you said it's cleanup patch and the one made me spend a
> few minutes to understand the code so it's not a clean up patch. ;-).
> So, IMO, it's worth to tidy it up.

Yes, I understand and agree.

------8<------
From: Vlastimil Babka <vbabka@suse.cz>
Date: Tue, 22 Apr 2014 13:55:36 +0200
Subject: mm-compaction-cleanup-isolate_freepages-fix2

Cleanup detection of compaction scanners crossing in isolate_freepages().
To make sure compact_finished() observes scanners crossing, we can just set
free_pfn to migrate_pfn instead of confusing max() construct.

Suggested-by: Minchan Kim <minchan@kernel.org>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Dongjun Shin <d.j.shin@samsung.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Sunghwan Yun <sunghwan.yun@samsung.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

---
 mm/compaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 37c15fe..1c992dc 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -768,7 +768,7 @@ static void isolate_freepages(struct zone *zone,
 	 * so that compact_finished() may detect this
 	 */
 	if (pfn < low_pfn)
-		next_free_pfn = max(pfn, zone->zone_start_pfn);
+		next_free_pfn = cc->migrate_pfn;
 
 	cc->free_pfn = next_free_pfn;
 	cc->nr_freepages = nr_freepages;
-- 
1.8.4.5

--
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 2/2] mm/compaction: cleanup isolate_freepages()

[Joonsoo Kim]

On Tue, Apr 22, 2014 at 03:17:30PM +0200, Vlastimil Babka wrote:
> On 04/22/2014 08:52 AM, Minchan Kim wrote:
> > On Tue, Apr 22, 2014 at 08:33:35AM +0200, Vlastimil Babka wrote:
> >> On 22.4.2014 1:53, Minchan Kim wrote:
> >>> On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
> >>>> On 21.4.2014 21:41, Andrew Morton wrote:
> >>>>> On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
> >>>>>
> >>>>>> Hi Vlastimil,
> >>>>>>
> >>>>>> Below just nitpicks.
> >>>>> It seems you were ignored ;)
> >>>> Oops, I managed to miss your e-mail, sorry.
> >>>>
> >>>>>>>  {
> >>>>>>>  	struct page *page;
> >>>>>>> -	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
> >>>>>>> +	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> >>>>>> Could you add comment for each variable?
> >>>>>>
> >>>>>> unsigned long pfn; /* scanning cursor */
> >>>>>> unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
> >>>>>> unsigned long next_free_pfn; /* start pfn for scaning at next truen */
> >>>>>> unsigned long z_end_pfn; /* zone's end pfn */
> >>>>>>
> >>>>>>
> >>>>>>> @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
> >>>>>>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>>>>>  	/*
> >>>>>>> -	 * Take care that if the migration scanner is at the end of the zone
> >>>>>>> -	 * that the free scanner does not accidentally move to the next zone
> >>>>>>> -	 * in the next isolation cycle.
> >>>>>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> >>>>>>> +	 * none, the pfn < low_pfn check will kick in.
> >>>>>>        "none" what? I'd like to clear more.
> >>>> If there are no updates to next_free_pfn within the for cycle. Which
> >>>> matches Andrew's formulation below.
> >>>>
> >>>>> I did this:
> >>>> Thanks!
> >>>>
> >>>>> --- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
> >>>>> +++ a/mm/compaction.c
> >>>>> @@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
> >>>>>  				struct compact_control *cc)
> >>>>>  {
> >>>>>  	struct page *page;
> >>>>> -	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> >>>>> +	unsigned long pfn;	     /* scanning cursor */
> >>>>> +	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
> >>>>> +	unsigned long next_free_pfn; /* start pfn for scaning at next round */
> >>>>> +	unsigned long z_end_pfn;     /* zone's end pfn */
> >>>> Yes that works.
> >>>>
> >>>>>  	int nr_freepages = cc->nr_freepages;
> >>>>>  	struct list_head *freelist = &cc->freepages;
> >>>>> @@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
> >>>>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>>>  	/*
> >>>>> -	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> >>>>> -	 * none, the pfn < low_pfn check will kick in.
> >>>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> >>>>> +	 * isolated, the pfn < low_pfn check will kick in.
> >>>> OK.
> >>>>
> >>>>>  	 */
> >>>>>  	next_free_pfn = 0;
> >>>>>>> @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
> >>>>>>>  	 * so that compact_finished() may detect this
> >>>>>>>  	 */
> >>>>>>>  	if (pfn < low_pfn)
> >>>>>>> -		cc->free_pfn = max(pfn, zone->zone_start_pfn);
> >>>>>>> -	else
> >>>>>>> -		cc->free_pfn = high_pfn;
> >>>>>>> +		next_free_pfn = max(pfn, zone->zone_start_pfn);
> >>>>>> Why we need max operation?
> >>>>>> IOW, what's the problem if we do (next_free_pfn = pfn)?
> >>>>> An answer to this would be useful, thanks.
> >>>> The idea (originally, not new here) is that the free scanner wants
> >>>> to remember the highest-pfn
> >>>> block where it managed to isolate some pages. If the following page
> >>>> migration fails, these isolated
> >>>> pages might be put back and would be skipped in further compaction
> >>>> attempt if we used just
> >>>> "next_free_pfn = pfn", until the scanners get reset.
> >>>>
> >>>> The question of course is if such situations are frequent and makes
> >>>> any difference to compaction
> >>>> outcome. And the downsides are potentially useless rescans and code
> >>>> complexity. Maybe Mel
> >>>> remembers how important this is? It should probably be profiled
> >>>> before changes are made.
> >>> I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
> >>> At that time, I didn't Cced so I missed that code so let's ask this time.
> >>> In that patch, you added this.
> >>>
> >>> if (pfn < low_pfn)
> >>>   cc->free_pfn = max(pfn, zone->zone_start_pfn);
> >>> else
> >>>   cc->free_pfn = high_pfn;
> >>
> >> Oh, right, this max(), not the one in the for loop. Sorry, I should
> >> have read more closely.
> >> But still maybe it's a good opportunity to kill the other max() as
> >> well. I'll try some testing.
> >>
> >> Anyway, this is what I answered to Mel when he asked the same thing
> >> when I sent
> >> that 7ed695069c3c patch:
> >>
> >> If a zone starts in a middle of a pageblock and migrate scanner isolates
> >> enough pages early to stay within that pageblock, low_pfn will be at the
> >> end of that pageblock and after the for cycle in this function ends, pfn
> >> might be at the beginning of that pageblock. It might not be an actual
> >> problem (this compaction will finish at this point, and if someone else
> >> is racing, he will probably check the boundaries himself), but I played
> >> it safe.
> >>
> >>
> >>> So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
> >>> compact_finished to stop compaction. And your [1/2] patch in this patchset
> >>> always makes free page scanner start on pageblock boundary so when the
> >>> loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
> >>> would be lower than migration scanner so compact_finished will always detect
> >>> it so I think you could just do
> >>>
> >>> if (pfn < low_pfn)
> >>>   next_free_pfn = pfn;
> >>>
> >>> cc->free_pfn = next_free_pfn;
> >>
> >> That could work. I was probably wrong about danger of racing in the
> >> reply to Mel,
> >> because free_pfn is stored in cc (private), not zone (shared).
> >>
> >>>
> >>> Or, if you want to clear *reset*,
> >>> if (pfn < lown_pfn)
> >>>   next_free_pfn = zone->zone_start_pfn;
> >>>
> >>> cc->free_pfn = next_free_pfn;
> >>
> >> That would work as well but is less straightforward I think. Might
> >> be misleading if
> >> someone added tracepoints to track the free scanner progress with
> >> pfn's (which
> >> might happen soon...)
> > 
> > My preference is to add following with pair of compact_finished
> > 
> > static inline void finish_compact(struct compact_control *cc)
> > {
> >   cc->free_pfn = cc->migrate_pfn;
> > }
> 
> Yes setting free_pfn to migrate_pfn is probably the best way, as these
> are the values compared in compact_finished. But I wouldn't introduce a
> new function just for one instance of this. Also compact_finished()
> doesn't test just the scanners to decide whether compaction should
> continue, so the pairing would be imperfect anyway.
> So Andrew, if you agree can you please fold in the patch below.
> 
> > But I don't care.
> > If you didn't send this patch as clean up, I would never interrupt
> > on the way but you said it's cleanup patch and the one made me spend a
> > few minutes to understand the code so it's not a clean up patch. ;-).
> > So, IMO, it's worth to tidy it up.
> 
> Yes, I understand and agree.
> 
> ------8<------
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Tue, 22 Apr 2014 13:55:36 +0200
> Subject: mm-compaction-cleanup-isolate_freepages-fix2
> 
> Cleanup detection of compaction scanners crossing in isolate_freepages().
> To make sure compact_finished() observes scanners crossing, we can just set
> free_pfn to migrate_pfn instead of confusing max() construct.
> 
> Suggested-by: Minchan Kim <minchan@kernel.org>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Dongjun Shin <d.j.shin@samsung.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Sunghwan Yun <sunghwan.yun@samsung.com>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> ---
>  mm/compaction.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 37c15fe..1c992dc 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -768,7 +768,7 @@ static void isolate_freepages(struct zone *zone,
>  	 * so that compact_finished() may detect this
>  	 */
>  	if (pfn < low_pfn)
> -		next_free_pfn = max(pfn, zone->zone_start_pfn);
> +		next_free_pfn = cc->migrate_pfn;
>  
>  	cc->free_pfn = next_free_pfn;
>  	cc->nr_freepages = nr_freepages;
> -- 
> 1.8.4.5
> 

Hello,

How about doing more clean-up at this time?

What I did is that taking end_pfn out of the loop and consider zone
boundary once. After then, we just subtract pageblock_nr_pages on
every iteration. With this change, we can remove local variable, z_end_pfn.
Another things I did are removing max() operation and un-needed
assignment to isolate variable.

Thanks.

--------->8------------
diff --git a/mm/compaction.c b/mm/compaction.c
index 1c992dc..95a506d 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
 				struct compact_control *cc)
 {
 	struct page *page;
-	unsigned long pfn;	     /* scanning cursor */
+	unsigned long pfn;	     /* start of scanning window */
+	unsigned long end_pfn;	     /* end of scanning window */
 	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
 	unsigned long next_free_pfn; /* start pfn for scaning at next round */
-	unsigned long z_end_pfn;     /* zone's end pfn */
 	int nr_freepages = cc->nr_freepages;
 	struct list_head *freelist = &cc->freepages;
 
@@ -688,15 +688,16 @@ static void isolate_freepages(struct zone *zone,
 	 * is using.
 	 */
 	pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
-	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
 
 	/*
-	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
-	 * isolated, the pfn < low_pfn check will kick in.
+	 * Take care when isolating in last pageblock of a zone which
+	 * ends in the middle of a pageblock.
 	 */
-	next_free_pfn = 0;
+	end_pfn = min(pfn + pageblock_nr_pages, zone_end_pfn(zone));
+	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
 
-	z_end_pfn = zone_end_pfn(zone);
+	/* If no pages are isolated, the pfn < low_pfn check will kick in. */
+	next_free_pfn = 0;
 
 	/*
 	 * Isolate free pages until enough are available to migrate the
@@ -704,9 +705,8 @@ static void isolate_freepages(struct zone *zone,
 	 * and free page scanners meet or enough free pages are isolated.
 	 */
 	for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
-					pfn -= pageblock_nr_pages) {
+		pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
 		unsigned long isolated;
-		unsigned long end_pfn;
 
 		/*
 		 * This can iterate a massively long zone without finding any
@@ -738,13 +738,6 @@ static void isolate_freepages(struct zone *zone,
 			continue;
 
 		/* Found a block suitable for isolating free pages from */
-		isolated = 0;
-
-		/*
-		 * Take care when isolating in last pageblock of a zone which
-		 * ends in the middle of a pageblock.
-		 */
-		end_pfn = min(pfn + pageblock_nr_pages, z_end_pfn);
 		isolated = isolate_freepages_block(cc, pfn, end_pfn,
 						   freelist, false);
 		nr_freepages += isolated;
@@ -754,9 +747,9 @@ static void isolate_freepages(struct zone *zone,
 		 * looking for free pages, the search will restart here as
 		 * page migration may have returned some pages to the allocator
 		 */
-		if (isolated) {
+		if (isolated && next_free_pfn == 0) {
 			cc->finished_update_free = true;
-			next_free_pfn = max(next_free_pfn, pfn);
+			next_free_pfn = pfn;
 		}
 	}
 

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Joonsoo Kim]

On Tue, Apr 22, 2014 at 03:17:30PM +0200, Vlastimil Babka wrote:
> On 04/22/2014 08:52 AM, Minchan Kim wrote:
> > On Tue, Apr 22, 2014 at 08:33:35AM +0200, Vlastimil Babka wrote:
> >> On 22.4.2014 1:53, Minchan Kim wrote:
> >>> On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
> >>>> On 21.4.2014 21:41, Andrew Morton wrote:
> >>>>> On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
> >>>>>
> >>>>>> Hi Vlastimil,
> >>>>>>
> >>>>>> Below just nitpicks.
> >>>>> It seems you were ignored ;)
> >>>> Oops, I managed to miss your e-mail, sorry.
> >>>>
> >>>>>>>  {
> >>>>>>>  	struct page *page;
> >>>>>>> -	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
> >>>>>>> +	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> >>>>>> Could you add comment for each variable?
> >>>>>>
> >>>>>> unsigned long pfn; /* scanning cursor */
> >>>>>> unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
> >>>>>> unsigned long next_free_pfn; /* start pfn for scaning at next truen */
> >>>>>> unsigned long z_end_pfn; /* zone's end pfn */
> >>>>>>
> >>>>>>
> >>>>>>> @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
> >>>>>>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>>>>>  	/*
> >>>>>>> -	 * Take care that if the migration scanner is at the end of the zone
> >>>>>>> -	 * that the free scanner does not accidentally move to the next zone
> >>>>>>> -	 * in the next isolation cycle.
> >>>>>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> >>>>>>> +	 * none, the pfn < low_pfn check will kick in.
> >>>>>>        "none" what? I'd like to clear more.
> >>>> If there are no updates to next_free_pfn within the for cycle. Which
> >>>> matches Andrew's formulation below.
> >>>>
> >>>>> I did this:
> >>>> Thanks!
> >>>>
> >>>>> --- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
> >>>>> +++ a/mm/compaction.c
> >>>>> @@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
> >>>>>  				struct compact_control *cc)
> >>>>>  {
> >>>>>  	struct page *page;
> >>>>> -	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> >>>>> +	unsigned long pfn;	     /* scanning cursor */
> >>>>> +	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
> >>>>> +	unsigned long next_free_pfn; /* start pfn for scaning at next round */
> >>>>> +	unsigned long z_end_pfn;     /* zone's end pfn */
> >>>> Yes that works.
> >>>>
> >>>>>  	int nr_freepages = cc->nr_freepages;
> >>>>>  	struct list_head *freelist = &cc->freepages;
> >>>>> @@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
> >>>>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>>>  	/*
> >>>>> -	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> >>>>> -	 * none, the pfn < low_pfn check will kick in.
> >>>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> >>>>> +	 * isolated, the pfn < low_pfn check will kick in.
> >>>> OK.
> >>>>
> >>>>>  	 */
> >>>>>  	next_free_pfn = 0;
> >>>>>>> @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
> >>>>>>>  	 * so that compact_finished() may detect this
> >>>>>>>  	 */
> >>>>>>>  	if (pfn < low_pfn)
> >>>>>>> -		cc->free_pfn = max(pfn, zone->zone_start_pfn);
> >>>>>>> -	else
> >>>>>>> -		cc->free_pfn = high_pfn;
> >>>>>>> +		next_free_pfn = max(pfn, zone->zone_start_pfn);
> >>>>>> Why we need max operation?
> >>>>>> IOW, what's the problem if we do (next_free_pfn = pfn)?
> >>>>> An answer to this would be useful, thanks.
> >>>> The idea (originally, not new here) is that the free scanner wants
> >>>> to remember the highest-pfn
> >>>> block where it managed to isolate some pages. If the following page
> >>>> migration fails, these isolated
> >>>> pages might be put back and would be skipped in further compaction
> >>>> attempt if we used just
> >>>> "next_free_pfn = pfn", until the scanners get reset.
> >>>>
> >>>> The question of course is if such situations are frequent and makes
> >>>> any difference to compaction
> >>>> outcome. And the downsides are potentially useless rescans and code
> >>>> complexity. Maybe Mel
> >>>> remembers how important this is? It should probably be profiled
> >>>> before changes are made.
> >>> I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
> >>> At that time, I didn't Cced so I missed that code so let's ask this time.
> >>> In that patch, you added this.
> >>>
> >>> if (pfn < low_pfn)
> >>>   cc->free_pfn = max(pfn, zone->zone_start_pfn);
> >>> else
> >>>   cc->free_pfn = high_pfn;
> >>
> >> Oh, right, this max(), not the one in the for loop. Sorry, I should
> >> have read more closely.
> >> But still maybe it's a good opportunity to kill the other max() as
> >> well. I'll try some testing.
> >>
> >> Anyway, this is what I answered to Mel when he asked the same thing
> >> when I sent
> >> that 7ed695069c3c patch:
> >>
> >> If a zone starts in a middle of a pageblock and migrate scanner isolates
> >> enough pages early to stay within that pageblock, low_pfn will be at the
> >> end of that pageblock and after the for cycle in this function ends, pfn
> >> might be at the beginning of that pageblock. It might not be an actual
> >> problem (this compaction will finish at this point, and if someone else
> >> is racing, he will probably check the boundaries himself), but I played
> >> it safe.
> >>
> >>
> >>> So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
> >>> compact_finished to stop compaction. And your [1/2] patch in this patchset
> >>> always makes free page scanner start on pageblock boundary so when the
> >>> loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
> >>> would be lower than migration scanner so compact_finished will always detect
> >>> it so I think you could just do
> >>>
> >>> if (pfn < low_pfn)
> >>>   next_free_pfn = pfn;
> >>>
> >>> cc->free_pfn = next_free_pfn;
> >>
> >> That could work. I was probably wrong about danger of racing in the
> >> reply to Mel,
> >> because free_pfn is stored in cc (private), not zone (shared).
> >>
> >>>
> >>> Or, if you want to clear *reset*,
> >>> if (pfn < lown_pfn)
> >>>   next_free_pfn = zone->zone_start_pfn;
> >>>
> >>> cc->free_pfn = next_free_pfn;
> >>
> >> That would work as well but is less straightforward I think. Might
> >> be misleading if
> >> someone added tracepoints to track the free scanner progress with
> >> pfn's (which
> >> might happen soon...)
> > 
> > My preference is to add following with pair of compact_finished
> > 
> > static inline void finish_compact(struct compact_control *cc)
> > {
> >   cc->free_pfn = cc->migrate_pfn;
> > }
> 
> Yes setting free_pfn to migrate_pfn is probably the best way, as these
> are the values compared in compact_finished. But I wouldn't introduce a
> new function just for one instance of this. Also compact_finished()
> doesn't test just the scanners to decide whether compaction should
> continue, so the pairing would be imperfect anyway.
> So Andrew, if you agree can you please fold in the patch below.
> 
> > But I don't care.
> > If you didn't send this patch as clean up, I would never interrupt
> > on the way but you said it's cleanup patch and the one made me spend a
> > few minutes to understand the code so it's not a clean up patch. ;-).
> > So, IMO, it's worth to tidy it up.
> 
> Yes, I understand and agree.
> 
> ------8<------
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Tue, 22 Apr 2014 13:55:36 +0200
> Subject: mm-compaction-cleanup-isolate_freepages-fix2
> 
> Cleanup detection of compaction scanners crossing in isolate_freepages().
> To make sure compact_finished() observes scanners crossing, we can just set
> free_pfn to migrate_pfn instead of confusing max() construct.
> 
> Suggested-by: Minchan Kim <minchan@kernel.org>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Dongjun Shin <d.j.shin@samsung.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Sunghwan Yun <sunghwan.yun@samsung.com>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> ---
>  mm/compaction.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 37c15fe..1c992dc 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -768,7 +768,7 @@ static void isolate_freepages(struct zone *zone,
>  	 * so that compact_finished() may detect this
>  	 */
>  	if (pfn < low_pfn)
> -		next_free_pfn = max(pfn, zone->zone_start_pfn);
> +		next_free_pfn = cc->migrate_pfn;
>  
>  	cc->free_pfn = next_free_pfn;
>  	cc->nr_freepages = nr_freepages;
> -- 
> 1.8.4.5
> 

Hello,

How about doing more clean-up at this time?

What I did is that taking end_pfn out of the loop and consider zone
boundary once. After then, we just subtract pageblock_nr_pages on
every iteration. With this change, we can remove local variable, z_end_pfn.
Another things I did are removing max() operation and un-needed
assignment to isolate variable.

Thanks.

--------->8------------
diff --git a/mm/compaction.c b/mm/compaction.c
index 1c992dc..95a506d 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
 				struct compact_control *cc)
 {
 	struct page *page;
-	unsigned long pfn;	     /* scanning cursor */
+	unsigned long pfn;	     /* start of scanning window */
+	unsigned long end_pfn;	     /* end of scanning window */
 	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
 	unsigned long next_free_pfn; /* start pfn for scaning at next round */
-	unsigned long z_end_pfn;     /* zone's end pfn */
 	int nr_freepages = cc->nr_freepages;
 	struct list_head *freelist = &cc->freepages;
 
@@ -688,15 +688,16 @@ static void isolate_freepages(struct zone *zone,
 	 * is using.
 	 */
 	pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
-	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
 
 	/*
-	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
-	 * isolated, the pfn < low_pfn check will kick in.
+	 * Take care when isolating in last pageblock of a zone which
+	 * ends in the middle of a pageblock.
 	 */
-	next_free_pfn = 0;
+	end_pfn = min(pfn + pageblock_nr_pages, zone_end_pfn(zone));
+	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
 
-	z_end_pfn = zone_end_pfn(zone);
+	/* If no pages are isolated, the pfn < low_pfn check will kick in. */
+	next_free_pfn = 0;
 
 	/*
 	 * Isolate free pages until enough are available to migrate the
@@ -704,9 +705,8 @@ static void isolate_freepages(struct zone *zone,
 	 * and free page scanners meet or enough free pages are isolated.
 	 */
 	for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
-					pfn -= pageblock_nr_pages) {
+		pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
 		unsigned long isolated;
-		unsigned long end_pfn;
 
 		/*
 		 * This can iterate a massively long zone without finding any
@@ -738,13 +738,6 @@ static void isolate_freepages(struct zone *zone,
 			continue;
 
 		/* Found a block suitable for isolating free pages from */
-		isolated = 0;
-
-		/*
-		 * Take care when isolating in last pageblock of a zone which
-		 * ends in the middle of a pageblock.
-		 */
-		end_pfn = min(pfn + pageblock_nr_pages, z_end_pfn);
 		isolated = isolate_freepages_block(cc, pfn, end_pfn,
 						   freelist, false);
 		nr_freepages += isolated;
@@ -754,9 +747,9 @@ static void isolate_freepages(struct zone *zone,
 		 * looking for free pages, the search will restart here as
 		 * page migration may have returned some pages to the allocator
 		 */
-		if (isolated) {
+		if (isolated && next_free_pfn == 0) {
 			cc->finished_update_free = true;
-			next_free_pfn = max(next_free_pfn, pfn);
+			next_free_pfn = pfn;
 		}
 	}
 

--
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 2/2] mm/compaction: cleanup isolate_freepages()

[Vlastimil Babka]

On 04/23/2014 04:58 AM, Joonsoo Kim wrote:
> On Tue, Apr 22, 2014 at 03:17:30PM +0200, Vlastimil Babka wrote:
>> On 04/22/2014 08:52 AM, Minchan Kim wrote:
>>> On Tue, Apr 22, 2014 at 08:33:35AM +0200, Vlastimil Babka wrote:
>>>> On 22.4.2014 1:53, Minchan Kim wrote:
>>>>> On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
>>>>>> On 21.4.2014 21:41, Andrew Morton wrote:
>>>>>>> On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
>>>>>>>
>>>>>>>> Hi Vlastimil,
>>>>>>>>
>>>>>>>> Below just nitpicks.
>>>>>>> It seems you were ignored ;)
>>>>>> Oops, I managed to miss your e-mail, sorry.
>>>>>>
>>>>>>>>>  {
>>>>>>>>>  	struct page *page;
>>>>>>>>> -	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
>>>>>>>>> +	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>>>>>>> Could you add comment for each variable?
>>>>>>>>
>>>>>>>> unsigned long pfn; /* scanning cursor */
>>>>>>>> unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
>>>>>>>> unsigned long next_free_pfn; /* start pfn for scaning at next truen */
>>>>>>>> unsigned long z_end_pfn; /* zone's end pfn */
>>>>>>>>
>>>>>>>>
>>>>>>>>> @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
>>>>>>>>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>>>>>  	/*
>>>>>>>>> -	 * Take care that if the migration scanner is at the end of the zone
>>>>>>>>> -	 * that the free scanner does not accidentally move to the next zone
>>>>>>>>> -	 * in the next isolation cycle.
>>>>>>>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>>>>>>>> +	 * none, the pfn < low_pfn check will kick in.
>>>>>>>>        "none" what? I'd like to clear more.
>>>>>> If there are no updates to next_free_pfn within the for cycle. Which
>>>>>> matches Andrew's formulation below.
>>>>>>
>>>>>>> I did this:
>>>>>> Thanks!
>>>>>>
>>>>>>> --- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
>>>>>>> +++ a/mm/compaction.c
>>>>>>> @@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
>>>>>>>  				struct compact_control *cc)
>>>>>>>  {
>>>>>>>  	struct page *page;
>>>>>>> -	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>>>>>> +	unsigned long pfn;	     /* scanning cursor */
>>>>>>> +	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
>>>>>>> +	unsigned long next_free_pfn; /* start pfn for scaning at next round */
>>>>>>> +	unsigned long z_end_pfn;     /* zone's end pfn */
>>>>>> Yes that works.
>>>>>>
>>>>>>>  	int nr_freepages = cc->nr_freepages;
>>>>>>>  	struct list_head *freelist = &cc->freepages;
>>>>>>> @@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
>>>>>>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>>>  	/*
>>>>>>> -	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>>>>>> -	 * none, the pfn < low_pfn check will kick in.
>>>>>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>>>>>>> +	 * isolated, the pfn < low_pfn check will kick in.
>>>>>> OK.
>>>>>>
>>>>>>>  	 */
>>>>>>>  	next_free_pfn = 0;
>>>>>>>>> @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
>>>>>>>>>  	 * so that compact_finished() may detect this
>>>>>>>>>  	 */
>>>>>>>>>  	if (pfn < low_pfn)
>>>>>>>>> -		cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>>>>>>>> -	else
>>>>>>>>> -		cc->free_pfn = high_pfn;
>>>>>>>>> +		next_free_pfn = max(pfn, zone->zone_start_pfn);
>>>>>>>> Why we need max operation?
>>>>>>>> IOW, what's the problem if we do (next_free_pfn = pfn)?
>>>>>>> An answer to this would be useful, thanks.
>>>>>> The idea (originally, not new here) is that the free scanner wants
>>>>>> to remember the highest-pfn
>>>>>> block where it managed to isolate some pages. If the following page
>>>>>> migration fails, these isolated
>>>>>> pages might be put back and would be skipped in further compaction
>>>>>> attempt if we used just
>>>>>> "next_free_pfn = pfn", until the scanners get reset.
>>>>>>
>>>>>> The question of course is if such situations are frequent and makes
>>>>>> any difference to compaction
>>>>>> outcome. And the downsides are potentially useless rescans and code
>>>>>> complexity. Maybe Mel
>>>>>> remembers how important this is? It should probably be profiled
>>>>>> before changes are made.
>>>>> I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
>>>>> At that time, I didn't Cced so I missed that code so let's ask this time.
>>>>> In that patch, you added this.
>>>>>
>>>>> if (pfn < low_pfn)
>>>>>   cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>>>> else
>>>>>   cc->free_pfn = high_pfn;
>>>>
>>>> Oh, right, this max(), not the one in the for loop. Sorry, I should
>>>> have read more closely.
>>>> But still maybe it's a good opportunity to kill the other max() as
>>>> well. I'll try some testing.
>>>>
>>>> Anyway, this is what I answered to Mel when he asked the same thing
>>>> when I sent
>>>> that 7ed695069c3c patch:
>>>>
>>>> If a zone starts in a middle of a pageblock and migrate scanner isolates
>>>> enough pages early to stay within that pageblock, low_pfn will be at the
>>>> end of that pageblock and after the for cycle in this function ends, pfn
>>>> might be at the beginning of that pageblock. It might not be an actual
>>>> problem (this compaction will finish at this point, and if someone else
>>>> is racing, he will probably check the boundaries himself), but I played
>>>> it safe.
>>>>
>>>>
>>>>> So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
>>>>> compact_finished to stop compaction. And your [1/2] patch in this patchset
>>>>> always makes free page scanner start on pageblock boundary so when the
>>>>> loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
>>>>> would be lower than migration scanner so compact_finished will always detect
>>>>> it so I think you could just do
>>>>>
>>>>> if (pfn < low_pfn)
>>>>>   next_free_pfn = pfn;
>>>>>
>>>>> cc->free_pfn = next_free_pfn;
>>>>
>>>> That could work. I was probably wrong about danger of racing in the
>>>> reply to Mel,
>>>> because free_pfn is stored in cc (private), not zone (shared).
>>>>
>>>>>
>>>>> Or, if you want to clear *reset*,
>>>>> if (pfn < lown_pfn)
>>>>>   next_free_pfn = zone->zone_start_pfn;
>>>>>
>>>>> cc->free_pfn = next_free_pfn;
>>>>
>>>> That would work as well but is less straightforward I think. Might
>>>> be misleading if
>>>> someone added tracepoints to track the free scanner progress with
>>>> pfn's (which
>>>> might happen soon...)
>>>
>>> My preference is to add following with pair of compact_finished
>>>
>>> static inline void finish_compact(struct compact_control *cc)
>>> {
>>>   cc->free_pfn = cc->migrate_pfn;
>>> }
>>
>> Yes setting free_pfn to migrate_pfn is probably the best way, as these
>> are the values compared in compact_finished. But I wouldn't introduce a
>> new function just for one instance of this. Also compact_finished()
>> doesn't test just the scanners to decide whether compaction should
>> continue, so the pairing would be imperfect anyway.
>> So Andrew, if you agree can you please fold in the patch below.
>>
>>> But I don't care.
>>> If you didn't send this patch as clean up, I would never interrupt
>>> on the way but you said it's cleanup patch and the one made me spend a
>>> few minutes to understand the code so it's not a clean up patch. ;-).
>>> So, IMO, it's worth to tidy it up.
>>
>> Yes, I understand and agree.
>>
>> ------8<------
>> From: Vlastimil Babka <vbabka@suse.cz>
>> Date: Tue, 22 Apr 2014 13:55:36 +0200
>> Subject: mm-compaction-cleanup-isolate_freepages-fix2
>>
>> Cleanup detection of compaction scanners crossing in isolate_freepages().
>> To make sure compact_finished() observes scanners crossing, we can just set
>> free_pfn to migrate_pfn instead of confusing max() construct.
>>
>> Suggested-by: Minchan Kim <minchan@kernel.org>
>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>> Cc: Christoph Lameter <cl@linux.com>
>> Cc: Dongjun Shin <d.j.shin@samsung.com>
>> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>> Cc: Mel Gorman <mgorman@suse.de>
>> Cc: Michal Nazarewicz <mina86@mina86.com>
>> Cc: Minchan Kim <minchan@kernel.org>
>> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>> Cc: Rik van Riel <riel@redhat.com>
>> Cc: Sunghwan Yun <sunghwan.yun@samsung.com>
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>>
>> ---
>>  mm/compaction.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index 37c15fe..1c992dc 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -768,7 +768,7 @@ static void isolate_freepages(struct zone *zone,
>>  	 * so that compact_finished() may detect this
>>  	 */
>>  	if (pfn < low_pfn)
>> -		next_free_pfn = max(pfn, zone->zone_start_pfn);
>> +		next_free_pfn = cc->migrate_pfn;
>>  
>>  	cc->free_pfn = next_free_pfn;
>>  	cc->nr_freepages = nr_freepages;
>> -- 
>> 1.8.4.5
>>
> 
> Hello,
> 
> How about doing more clean-up at this time?
> 
> What I did is that taking end_pfn out of the loop and consider zone
> boundary once. After then, we just subtract pageblock_nr_pages on
> every iteration. With this change, we can remove local variable, z_end_pfn.
> Another things I did are removing max() operation and un-needed
> assignment to isolate variable.
> 
> Thanks.
> 
> --------->8------------
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 1c992dc..95a506d 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
>  				struct compact_control *cc)
>  {
>  	struct page *page;
> -	unsigned long pfn;	     /* scanning cursor */
> +	unsigned long pfn;	     /* start of scanning window */
> +	unsigned long end_pfn;	     /* end of scanning window */
>  	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
>  	unsigned long next_free_pfn; /* start pfn for scaning at next round */
> -	unsigned long z_end_pfn;     /* zone's end pfn */
>  	int nr_freepages = cc->nr_freepages;
>  	struct list_head *freelist = &cc->freepages;
>  
> @@ -688,15 +688,16 @@ static void isolate_freepages(struct zone *zone,
>  	 * is using.
>  	 */
>  	pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
> -	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>  
>  	/*
> -	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> -	 * isolated, the pfn < low_pfn check will kick in.
> +	 * Take care when isolating in last pageblock of a zone which
> +	 * ends in the middle of a pageblock.
>  	 */
> -	next_free_pfn = 0;
> +	end_pfn = min(pfn + pageblock_nr_pages, zone_end_pfn(zone));
> +	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>  
> -	z_end_pfn = zone_end_pfn(zone);
> +	/* If no pages are isolated, the pfn < low_pfn check will kick in. */
> +	next_free_pfn = 0;
>  
>  	/*
>  	 * Isolate free pages until enough are available to migrate the
> @@ -704,9 +705,8 @@ static void isolate_freepages(struct zone *zone,
>  	 * and free page scanners meet or enough free pages are isolated.
>  	 */
>  	for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
> -					pfn -= pageblock_nr_pages) {
> +		pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {

If zone_end_pfn was in the middle of a pageblock, then your end_pfn will
always be in the middle of a pageblock and you will not scan half of all
pageblocks.

>  		unsigned long isolated;
> -		unsigned long end_pfn;
>  
>  		/*
>  		 * This can iterate a massively long zone without finding any
> @@ -738,13 +738,6 @@ static void isolate_freepages(struct zone *zone,
>  			continue;
>  
>  		/* Found a block suitable for isolating free pages from */
> -		isolated = 0;
> -
> -		/*
> -		 * Take care when isolating in last pageblock of a zone which
> -		 * ends in the middle of a pageblock.
> -		 */
> -		end_pfn = min(pfn + pageblock_nr_pages, z_end_pfn);
>  		isolated = isolate_freepages_block(cc, pfn, end_pfn,
>  						   freelist, false);
>  		nr_freepages += isolated;
> @@ -754,9 +747,9 @@ static void isolate_freepages(struct zone *zone,
>  		 * looking for free pages, the search will restart here as
>  		 * page migration may have returned some pages to the allocator
>  		 */
> -		if (isolated) {
> +		if (isolated && next_free_pfn == 0) {
>  			cc->finished_update_free = true;
> -			next_free_pfn = max(next_free_pfn, pfn);
> +			next_free_pfn = pfn;
>  		}
>  	}
>  
> 

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Vlastimil Babka]

On 04/23/2014 04:58 AM, Joonsoo Kim wrote:
> On Tue, Apr 22, 2014 at 03:17:30PM +0200, Vlastimil Babka wrote:
>> On 04/22/2014 08:52 AM, Minchan Kim wrote:
>>> On Tue, Apr 22, 2014 at 08:33:35AM +0200, Vlastimil Babka wrote:
>>>> On 22.4.2014 1:53, Minchan Kim wrote:
>>>>> On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
>>>>>> On 21.4.2014 21:41, Andrew Morton wrote:
>>>>>>> On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
>>>>>>>
>>>>>>>> Hi Vlastimil,
>>>>>>>>
>>>>>>>> Below just nitpicks.
>>>>>>> It seems you were ignored ;)
>>>>>> Oops, I managed to miss your e-mail, sorry.
>>>>>>
>>>>>>>>>  {
>>>>>>>>>  	struct page *page;
>>>>>>>>> -	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
>>>>>>>>> +	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>>>>>>> Could you add comment for each variable?
>>>>>>>>
>>>>>>>> unsigned long pfn; /* scanning cursor */
>>>>>>>> unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
>>>>>>>> unsigned long next_free_pfn; /* start pfn for scaning at next truen */
>>>>>>>> unsigned long z_end_pfn; /* zone's end pfn */
>>>>>>>>
>>>>>>>>
>>>>>>>>> @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
>>>>>>>>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>>>>>  	/*
>>>>>>>>> -	 * Take care that if the migration scanner is at the end of the zone
>>>>>>>>> -	 * that the free scanner does not accidentally move to the next zone
>>>>>>>>> -	 * in the next isolation cycle.
>>>>>>>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>>>>>>>> +	 * none, the pfn < low_pfn check will kick in.
>>>>>>>>        "none" what? I'd like to clear more.
>>>>>> If there are no updates to next_free_pfn within the for cycle. Which
>>>>>> matches Andrew's formulation below.
>>>>>>
>>>>>>> I did this:
>>>>>> Thanks!
>>>>>>
>>>>>>> --- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
>>>>>>> +++ a/mm/compaction.c
>>>>>>> @@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
>>>>>>>  				struct compact_control *cc)
>>>>>>>  {
>>>>>>>  	struct page *page;
>>>>>>> -	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>>>>>> +	unsigned long pfn;	     /* scanning cursor */
>>>>>>> +	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
>>>>>>> +	unsigned long next_free_pfn; /* start pfn for scaning at next round */
>>>>>>> +	unsigned long z_end_pfn;     /* zone's end pfn */
>>>>>> Yes that works.
>>>>>>
>>>>>>>  	int nr_freepages = cc->nr_freepages;
>>>>>>>  	struct list_head *freelist = &cc->freepages;
>>>>>>> @@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
>>>>>>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>>>  	/*
>>>>>>> -	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>>>>>> -	 * none, the pfn < low_pfn check will kick in.
>>>>>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>>>>>>> +	 * isolated, the pfn < low_pfn check will kick in.
>>>>>> OK.
>>>>>>
>>>>>>>  	 */
>>>>>>>  	next_free_pfn = 0;
>>>>>>>>> @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
>>>>>>>>>  	 * so that compact_finished() may detect this
>>>>>>>>>  	 */
>>>>>>>>>  	if (pfn < low_pfn)
>>>>>>>>> -		cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>>>>>>>> -	else
>>>>>>>>> -		cc->free_pfn = high_pfn;
>>>>>>>>> +		next_free_pfn = max(pfn, zone->zone_start_pfn);
>>>>>>>> Why we need max operation?
>>>>>>>> IOW, what's the problem if we do (next_free_pfn = pfn)?
>>>>>>> An answer to this would be useful, thanks.
>>>>>> The idea (originally, not new here) is that the free scanner wants
>>>>>> to remember the highest-pfn
>>>>>> block where it managed to isolate some pages. If the following page
>>>>>> migration fails, these isolated
>>>>>> pages might be put back and would be skipped in further compaction
>>>>>> attempt if we used just
>>>>>> "next_free_pfn = pfn", until the scanners get reset.
>>>>>>
>>>>>> The question of course is if such situations are frequent and makes
>>>>>> any difference to compaction
>>>>>> outcome. And the downsides are potentially useless rescans and code
>>>>>> complexity. Maybe Mel
>>>>>> remembers how important this is? It should probably be profiled
>>>>>> before changes are made.
>>>>> I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
>>>>> At that time, I didn't Cced so I missed that code so let's ask this time.
>>>>> In that patch, you added this.
>>>>>
>>>>> if (pfn < low_pfn)
>>>>>   cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>>>> else
>>>>>   cc->free_pfn = high_pfn;
>>>>
>>>> Oh, right, this max(), not the one in the for loop. Sorry, I should
>>>> have read more closely.
>>>> But still maybe it's a good opportunity to kill the other max() as
>>>> well. I'll try some testing.
>>>>
>>>> Anyway, this is what I answered to Mel when he asked the same thing
>>>> when I sent
>>>> that 7ed695069c3c patch:
>>>>
>>>> If a zone starts in a middle of a pageblock and migrate scanner isolates
>>>> enough pages early to stay within that pageblock, low_pfn will be at the
>>>> end of that pageblock and after the for cycle in this function ends, pfn
>>>> might be at the beginning of that pageblock. It might not be an actual
>>>> problem (this compaction will finish at this point, and if someone else
>>>> is racing, he will probably check the boundaries himself), but I played
>>>> it safe.
>>>>
>>>>
>>>>> So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
>>>>> compact_finished to stop compaction. And your [1/2] patch in this patchset
>>>>> always makes free page scanner start on pageblock boundary so when the
>>>>> loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
>>>>> would be lower than migration scanner so compact_finished will always detect
>>>>> it so I think you could just do
>>>>>
>>>>> if (pfn < low_pfn)
>>>>>   next_free_pfn = pfn;
>>>>>
>>>>> cc->free_pfn = next_free_pfn;
>>>>
>>>> That could work. I was probably wrong about danger of racing in the
>>>> reply to Mel,
>>>> because free_pfn is stored in cc (private), not zone (shared).
>>>>
>>>>>
>>>>> Or, if you want to clear *reset*,
>>>>> if (pfn < lown_pfn)
>>>>>   next_free_pfn = zone->zone_start_pfn;
>>>>>
>>>>> cc->free_pfn = next_free_pfn;
>>>>
>>>> That would work as well but is less straightforward I think. Might
>>>> be misleading if
>>>> someone added tracepoints to track the free scanner progress with
>>>> pfn's (which
>>>> might happen soon...)
>>>
>>> My preference is to add following with pair of compact_finished
>>>
>>> static inline void finish_compact(struct compact_control *cc)
>>> {
>>>   cc->free_pfn = cc->migrate_pfn;
>>> }
>>
>> Yes setting free_pfn to migrate_pfn is probably the best way, as these
>> are the values compared in compact_finished. But I wouldn't introduce a
>> new function just for one instance of this. Also compact_finished()
>> doesn't test just the scanners to decide whether compaction should
>> continue, so the pairing would be imperfect anyway.
>> So Andrew, if you agree can you please fold in the patch below.
>>
>>> But I don't care.
>>> If you didn't send this patch as clean up, I would never interrupt
>>> on the way but you said it's cleanup patch and the one made me spend a
>>> few minutes to understand the code so it's not a clean up patch. ;-).
>>> So, IMO, it's worth to tidy it up.
>>
>> Yes, I understand and agree.
>>
>> ------8<------
>> From: Vlastimil Babka <vbabka@suse.cz>
>> Date: Tue, 22 Apr 2014 13:55:36 +0200
>> Subject: mm-compaction-cleanup-isolate_freepages-fix2
>>
>> Cleanup detection of compaction scanners crossing in isolate_freepages().
>> To make sure compact_finished() observes scanners crossing, we can just set
>> free_pfn to migrate_pfn instead of confusing max() construct.
>>
>> Suggested-by: Minchan Kim <minchan@kernel.org>
>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>> Cc: Christoph Lameter <cl@linux.com>
>> Cc: Dongjun Shin <d.j.shin@samsung.com>
>> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>> Cc: Mel Gorman <mgorman@suse.de>
>> Cc: Michal Nazarewicz <mina86@mina86.com>
>> Cc: Minchan Kim <minchan@kernel.org>
>> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>> Cc: Rik van Riel <riel@redhat.com>
>> Cc: Sunghwan Yun <sunghwan.yun@samsung.com>
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>>
>> ---
>>  mm/compaction.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index 37c15fe..1c992dc 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -768,7 +768,7 @@ static void isolate_freepages(struct zone *zone,
>>  	 * so that compact_finished() may detect this
>>  	 */
>>  	if (pfn < low_pfn)
>> -		next_free_pfn = max(pfn, zone->zone_start_pfn);
>> +		next_free_pfn = cc->migrate_pfn;
>>  
>>  	cc->free_pfn = next_free_pfn;
>>  	cc->nr_freepages = nr_freepages;
>> -- 
>> 1.8.4.5
>>
> 
> Hello,
> 
> How about doing more clean-up at this time?
> 
> What I did is that taking end_pfn out of the loop and consider zone
> boundary once. After then, we just subtract pageblock_nr_pages on
> every iteration. With this change, we can remove local variable, z_end_pfn.
> Another things I did are removing max() operation and un-needed
> assignment to isolate variable.
> 
> Thanks.
> 
> --------->8------------
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 1c992dc..95a506d 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
>  				struct compact_control *cc)
>  {
>  	struct page *page;
> -	unsigned long pfn;	     /* scanning cursor */
> +	unsigned long pfn;	     /* start of scanning window */
> +	unsigned long end_pfn;	     /* end of scanning window */
>  	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
>  	unsigned long next_free_pfn; /* start pfn for scaning at next round */
> -	unsigned long z_end_pfn;     /* zone's end pfn */
>  	int nr_freepages = cc->nr_freepages;
>  	struct list_head *freelist = &cc->freepages;
>  
> @@ -688,15 +688,16 @@ static void isolate_freepages(struct zone *zone,
>  	 * is using.
>  	 */
>  	pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
> -	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>  
>  	/*
> -	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> -	 * isolated, the pfn < low_pfn check will kick in.
> +	 * Take care when isolating in last pageblock of a zone which
> +	 * ends in the middle of a pageblock.
>  	 */
> -	next_free_pfn = 0;
> +	end_pfn = min(pfn + pageblock_nr_pages, zone_end_pfn(zone));
> +	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>  
> -	z_end_pfn = zone_end_pfn(zone);
> +	/* If no pages are isolated, the pfn < low_pfn check will kick in. */
> +	next_free_pfn = 0;
>  
>  	/*
>  	 * Isolate free pages until enough are available to migrate the
> @@ -704,9 +705,8 @@ static void isolate_freepages(struct zone *zone,
>  	 * and free page scanners meet or enough free pages are isolated.
>  	 */
>  	for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
> -					pfn -= pageblock_nr_pages) {
> +		pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {

If zone_end_pfn was in the middle of a pageblock, then your end_pfn will
always be in the middle of a pageblock and you will not scan half of all
pageblocks.

>  		unsigned long isolated;
> -		unsigned long end_pfn;
>  
>  		/*
>  		 * This can iterate a massively long zone without finding any
> @@ -738,13 +738,6 @@ static void isolate_freepages(struct zone *zone,
>  			continue;
>  
>  		/* Found a block suitable for isolating free pages from */
> -		isolated = 0;
> -
> -		/*
> -		 * Take care when isolating in last pageblock of a zone which
> -		 * ends in the middle of a pageblock.
> -		 */
> -		end_pfn = min(pfn + pageblock_nr_pages, z_end_pfn);
>  		isolated = isolate_freepages_block(cc, pfn, end_pfn,
>  						   freelist, false);
>  		nr_freepages += isolated;
> @@ -754,9 +747,9 @@ static void isolate_freepages(struct zone *zone,
>  		 * looking for free pages, the search will restart here as
>  		 * page migration may have returned some pages to the allocator
>  		 */
> -		if (isolated) {
> +		if (isolated && next_free_pfn == 0) {
>  			cc->finished_update_free = true;
> -			next_free_pfn = max(next_free_pfn, pfn);
> +			next_free_pfn = pfn;
>  		}
>  	}
>  
> 

--
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 2/2] mm/compaction: cleanup isolate_freepages()

[Joonsoo Kim]

2014-04-23 16:30 GMT+09:00 Vlastimil Babka <vbabka@suse.cz>:
> On 04/23/2014 04:58 AM, Joonsoo Kim wrote:
>> On Tue, Apr 22, 2014 at 03:17:30PM +0200, Vlastimil Babka wrote:
>>> On 04/22/2014 08:52 AM, Minchan Kim wrote:
>>>> On Tue, Apr 22, 2014 at 08:33:35AM +0200, Vlastimil Babka wrote:
>>>>> On 22.4.2014 1:53, Minchan Kim wrote:
>>>>>> On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
>>>>>>> On 21.4.2014 21:41, Andrew Morton wrote:
>>>>>>>> On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
>>>>>>>>
>>>>>>>>> Hi Vlastimil,
>>>>>>>>>
>>>>>>>>> Below just nitpicks.
>>>>>>>> It seems you were ignored ;)
>>>>>>> Oops, I managed to miss your e-mail, sorry.
>>>>>>>
>>>>>>>>>>  {
>>>>>>>>>>       struct page *page;
>>>>>>>>>> -     unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
>>>>>>>>>> +     unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>>>>>>>> Could you add comment for each variable?
>>>>>>>>>
>>>>>>>>> unsigned long pfn; /* scanning cursor */
>>>>>>>>> unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
>>>>>>>>> unsigned long next_free_pfn; /* start pfn for scaning at next truen */
>>>>>>>>> unsigned long z_end_pfn; /* zone's end pfn */
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
>>>>>>>>>>       low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>>>>>>       /*
>>>>>>>>>> -      * Take care that if the migration scanner is at the end of the zone
>>>>>>>>>> -      * that the free scanner does not accidentally move to the next zone
>>>>>>>>>> -      * in the next isolation cycle.
>>>>>>>>>> +      * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>>>>>>>>> +      * none, the pfn < low_pfn check will kick in.
>>>>>>>>>        "none" what? I'd like to clear more.
>>>>>>> If there are no updates to next_free_pfn within the for cycle. Which
>>>>>>> matches Andrew's formulation below.
>>>>>>>
>>>>>>>> I did this:
>>>>>>> Thanks!
>>>>>>>
>>>>>>>> --- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
>>>>>>>> +++ a/mm/compaction.c
>>>>>>>> @@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
>>>>>>>>                                 struct compact_control *cc)
>>>>>>>>  {
>>>>>>>>         struct page *page;
>>>>>>>> -       unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>>>>>>> +       unsigned long pfn;           /* scanning cursor */
>>>>>>>> +       unsigned long low_pfn;       /* lowest pfn scanner is able to scan */
>>>>>>>> +       unsigned long next_free_pfn; /* start pfn for scaning at next round */
>>>>>>>> +       unsigned long z_end_pfn;     /* zone's end pfn */
>>>>>>> Yes that works.
>>>>>>>
>>>>>>>>         int nr_freepages = cc->nr_freepages;
>>>>>>>>         struct list_head *freelist = &cc->freepages;
>>>>>>>> @@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
>>>>>>>>         low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>>>>         /*
>>>>>>>> -        * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>>>>>>> -        * none, the pfn < low_pfn check will kick in.
>>>>>>>> +        * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>>>>>>>> +        * isolated, the pfn < low_pfn check will kick in.
>>>>>>> OK.
>>>>>>>
>>>>>>>>          */
>>>>>>>>         next_free_pfn = 0;
>>>>>>>>>> @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
>>>>>>>>>>        * so that compact_finished() may detect this
>>>>>>>>>>        */
>>>>>>>>>>       if (pfn < low_pfn)
>>>>>>>>>> -             cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>>>>>>>>> -     else
>>>>>>>>>> -             cc->free_pfn = high_pfn;
>>>>>>>>>> +             next_free_pfn = max(pfn, zone->zone_start_pfn);
>>>>>>>>> Why we need max operation?
>>>>>>>>> IOW, what's the problem if we do (next_free_pfn = pfn)?
>>>>>>>> An answer to this would be useful, thanks.
>>>>>>> The idea (originally, not new here) is that the free scanner wants
>>>>>>> to remember the highest-pfn
>>>>>>> block where it managed to isolate some pages. If the following page
>>>>>>> migration fails, these isolated
>>>>>>> pages might be put back and would be skipped in further compaction
>>>>>>> attempt if we used just
>>>>>>> "next_free_pfn = pfn", until the scanners get reset.
>>>>>>>
>>>>>>> The question of course is if such situations are frequent and makes
>>>>>>> any difference to compaction
>>>>>>> outcome. And the downsides are potentially useless rescans and code
>>>>>>> complexity. Maybe Mel
>>>>>>> remembers how important this is? It should probably be profiled
>>>>>>> before changes are made.
>>>>>> I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
>>>>>> At that time, I didn't Cced so I missed that code so let's ask this time.
>>>>>> In that patch, you added this.
>>>>>>
>>>>>> if (pfn < low_pfn)
>>>>>>   cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>>>>> else
>>>>>>   cc->free_pfn = high_pfn;
>>>>>
>>>>> Oh, right, this max(), not the one in the for loop. Sorry, I should
>>>>> have read more closely.
>>>>> But still maybe it's a good opportunity to kill the other max() as
>>>>> well. I'll try some testing.
>>>>>
>>>>> Anyway, this is what I answered to Mel when he asked the same thing
>>>>> when I sent
>>>>> that 7ed695069c3c patch:
>>>>>
>>>>> If a zone starts in a middle of a pageblock and migrate scanner isolates
>>>>> enough pages early to stay within that pageblock, low_pfn will be at the
>>>>> end of that pageblock and after the for cycle in this function ends, pfn
>>>>> might be at the beginning of that pageblock. It might not be an actual
>>>>> problem (this compaction will finish at this point, and if someone else
>>>>> is racing, he will probably check the boundaries himself), but I played
>>>>> it safe.
>>>>>
>>>>>
>>>>>> So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
>>>>>> compact_finished to stop compaction. And your [1/2] patch in this patchset
>>>>>> always makes free page scanner start on pageblock boundary so when the
>>>>>> loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
>>>>>> would be lower than migration scanner so compact_finished will always detect
>>>>>> it so I think you could just do
>>>>>>
>>>>>> if (pfn < low_pfn)
>>>>>>   next_free_pfn = pfn;
>>>>>>
>>>>>> cc->free_pfn = next_free_pfn;
>>>>>
>>>>> That could work. I was probably wrong about danger of racing in the
>>>>> reply to Mel,
>>>>> because free_pfn is stored in cc (private), not zone (shared).
>>>>>
>>>>>>
>>>>>> Or, if you want to clear *reset*,
>>>>>> if (pfn < lown_pfn)
>>>>>>   next_free_pfn = zone->zone_start_pfn;
>>>>>>
>>>>>> cc->free_pfn = next_free_pfn;
>>>>>
>>>>> That would work as well but is less straightforward I think. Might
>>>>> be misleading if
>>>>> someone added tracepoints to track the free scanner progress with
>>>>> pfn's (which
>>>>> might happen soon...)
>>>>
>>>> My preference is to add following with pair of compact_finished
>>>>
>>>> static inline void finish_compact(struct compact_control *cc)
>>>> {
>>>>   cc->free_pfn = cc->migrate_pfn;
>>>> }
>>>
>>> Yes setting free_pfn to migrate_pfn is probably the best way, as these
>>> are the values compared in compact_finished. But I wouldn't introduce a
>>> new function just for one instance of this. Also compact_finished()
>>> doesn't test just the scanners to decide whether compaction should
>>> continue, so the pairing would be imperfect anyway.
>>> So Andrew, if you agree can you please fold in the patch below.
>>>
>>>> But I don't care.
>>>> If you didn't send this patch as clean up, I would never interrupt
>>>> on the way but you said it's cleanup patch and the one made me spend a
>>>> few minutes to understand the code so it's not a clean up patch. ;-).
>>>> So, IMO, it's worth to tidy it up.
>>>
>>> Yes, I understand and agree.
>>>
>>> ------8<------
>>> From: Vlastimil Babka <vbabka@suse.cz>
>>> Date: Tue, 22 Apr 2014 13:55:36 +0200
>>> Subject: mm-compaction-cleanup-isolate_freepages-fix2
>>>
>>> Cleanup detection of compaction scanners crossing in isolate_freepages().
>>> To make sure compact_finished() observes scanners crossing, we can just set
>>> free_pfn to migrate_pfn instead of confusing max() construct.
>>>
>>> Suggested-by: Minchan Kim <minchan@kernel.org>
>>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>>> Cc: Christoph Lameter <cl@linux.com>
>>> Cc: Dongjun Shin <d.j.shin@samsung.com>
>>> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>>> Cc: Mel Gorman <mgorman@suse.de>
>>> Cc: Michal Nazarewicz <mina86@mina86.com>
>>> Cc: Minchan Kim <minchan@kernel.org>
>>> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>>> Cc: Rik van Riel <riel@redhat.com>
>>> Cc: Sunghwan Yun <sunghwan.yun@samsung.com>
>>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>>>
>>> ---
>>>  mm/compaction.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>> index 37c15fe..1c992dc 100644
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -768,7 +768,7 @@ static void isolate_freepages(struct zone *zone,
>>>       * so that compact_finished() may detect this
>>>       */
>>>      if (pfn < low_pfn)
>>> -            next_free_pfn = max(pfn, zone->zone_start_pfn);
>>> +            next_free_pfn = cc->migrate_pfn;
>>>
>>>      cc->free_pfn = next_free_pfn;
>>>      cc->nr_freepages = nr_freepages;
>>> --
>>> 1.8.4.5
>>>
>>
>> Hello,
>>
>> How about doing more clean-up at this time?
>>
>> What I did is that taking end_pfn out of the loop and consider zone
>> boundary once. After then, we just subtract pageblock_nr_pages on
>> every iteration. With this change, we can remove local variable, z_end_pfn.
>> Another things I did are removing max() operation and un-needed
>> assignment to isolate variable.
>>
>> Thanks.
>>
>> --------->8------------
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index 1c992dc..95a506d 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
>>                               struct compact_control *cc)
>>  {
>>       struct page *page;
>> -     unsigned long pfn;           /* scanning cursor */
>> +     unsigned long pfn;           /* start of scanning window */
>> +     unsigned long end_pfn;       /* end of scanning window */
>>       unsigned long low_pfn;       /* lowest pfn scanner is able to scan */
>>       unsigned long next_free_pfn; /* start pfn for scaning at next round */
>> -     unsigned long z_end_pfn;     /* zone's end pfn */
>>       int nr_freepages = cc->nr_freepages;
>>       struct list_head *freelist = &cc->freepages;
>>
>> @@ -688,15 +688,16 @@ static void isolate_freepages(struct zone *zone,
>>        * is using.
>>        */
>>       pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
>> -     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>
>>       /*
>> -      * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>> -      * isolated, the pfn < low_pfn check will kick in.
>> +      * Take care when isolating in last pageblock of a zone which
>> +      * ends in the middle of a pageblock.
>>        */
>> -     next_free_pfn = 0;
>> +     end_pfn = min(pfn + pageblock_nr_pages, zone_end_pfn(zone));
>> +     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>
>> -     z_end_pfn = zone_end_pfn(zone);
>> +     /* If no pages are isolated, the pfn < low_pfn check will kick in. */
>> +     next_free_pfn = 0;
>>
>>       /*
>>        * Isolate free pages until enough are available to migrate the
>> @@ -704,9 +705,8 @@ static void isolate_freepages(struct zone *zone,
>>        * and free page scanners meet or enough free pages are isolated.
>>        */
>>       for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
>> -                                     pfn -= pageblock_nr_pages) {
>> +             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
>
> If zone_end_pfn was in the middle of a pageblock, then your end_pfn will
> always be in the middle of a pageblock and you will not scan half of all
> pageblocks.
>

Okay. I think a way to fix it.
By assigning pfn(start of scanning window) to
end_pfn(end of scanning window) for the next loop, we can solve the problem
you mentioned. How about below?

-             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
+            end_pfn = pfn, pfn -= pageblock_nr_pages) {

Thanks.

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Joonsoo Kim]

2014-04-23 16:30 GMT+09:00 Vlastimil Babka <vbabka@suse.cz>:
> On 04/23/2014 04:58 AM, Joonsoo Kim wrote:
>> On Tue, Apr 22, 2014 at 03:17:30PM +0200, Vlastimil Babka wrote:
>>> On 04/22/2014 08:52 AM, Minchan Kim wrote:
>>>> On Tue, Apr 22, 2014 at 08:33:35AM +0200, Vlastimil Babka wrote:
>>>>> On 22.4.2014 1:53, Minchan Kim wrote:
>>>>>> On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
>>>>>>> On 21.4.2014 21:41, Andrew Morton wrote:
>>>>>>>> On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
>>>>>>>>
>>>>>>>>> Hi Vlastimil,
>>>>>>>>>
>>>>>>>>> Below just nitpicks.
>>>>>>>> It seems you were ignored ;)
>>>>>>> Oops, I managed to miss your e-mail, sorry.
>>>>>>>
>>>>>>>>>>  {
>>>>>>>>>>       struct page *page;
>>>>>>>>>> -     unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
>>>>>>>>>> +     unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>>>>>>>> Could you add comment for each variable?
>>>>>>>>>
>>>>>>>>> unsigned long pfn; /* scanning cursor */
>>>>>>>>> unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
>>>>>>>>> unsigned long next_free_pfn; /* start pfn for scaning at next truen */
>>>>>>>>> unsigned long z_end_pfn; /* zone's end pfn */
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
>>>>>>>>>>       low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>>>>>>       /*
>>>>>>>>>> -      * Take care that if the migration scanner is at the end of the zone
>>>>>>>>>> -      * that the free scanner does not accidentally move to the next zone
>>>>>>>>>> -      * in the next isolation cycle.
>>>>>>>>>> +      * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>>>>>>>>> +      * none, the pfn < low_pfn check will kick in.
>>>>>>>>>        "none" what? I'd like to clear more.
>>>>>>> If there are no updates to next_free_pfn within the for cycle. Which
>>>>>>> matches Andrew's formulation below.
>>>>>>>
>>>>>>>> I did this:
>>>>>>> Thanks!
>>>>>>>
>>>>>>>> --- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
>>>>>>>> +++ a/mm/compaction.c
>>>>>>>> @@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
>>>>>>>>                                 struct compact_control *cc)
>>>>>>>>  {
>>>>>>>>         struct page *page;
>>>>>>>> -       unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>>>>>>> +       unsigned long pfn;           /* scanning cursor */
>>>>>>>> +       unsigned long low_pfn;       /* lowest pfn scanner is able to scan */
>>>>>>>> +       unsigned long next_free_pfn; /* start pfn for scaning at next round */
>>>>>>>> +       unsigned long z_end_pfn;     /* zone's end pfn */
>>>>>>> Yes that works.
>>>>>>>
>>>>>>>>         int nr_freepages = cc->nr_freepages;
>>>>>>>>         struct list_head *freelist = &cc->freepages;
>>>>>>>> @@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
>>>>>>>>         low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>>>>         /*
>>>>>>>> -        * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>>>>>>> -        * none, the pfn < low_pfn check will kick in.
>>>>>>>> +        * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>>>>>>>> +        * isolated, the pfn < low_pfn check will kick in.
>>>>>>> OK.
>>>>>>>
>>>>>>>>          */
>>>>>>>>         next_free_pfn = 0;
>>>>>>>>>> @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
>>>>>>>>>>        * so that compact_finished() may detect this
>>>>>>>>>>        */
>>>>>>>>>>       if (pfn < low_pfn)
>>>>>>>>>> -             cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>>>>>>>>> -     else
>>>>>>>>>> -             cc->free_pfn = high_pfn;
>>>>>>>>>> +             next_free_pfn = max(pfn, zone->zone_start_pfn);
>>>>>>>>> Why we need max operation?
>>>>>>>>> IOW, what's the problem if we do (next_free_pfn = pfn)?
>>>>>>>> An answer to this would be useful, thanks.
>>>>>>> The idea (originally, not new here) is that the free scanner wants
>>>>>>> to remember the highest-pfn
>>>>>>> block where it managed to isolate some pages. If the following page
>>>>>>> migration fails, these isolated
>>>>>>> pages might be put back and would be skipped in further compaction
>>>>>>> attempt if we used just
>>>>>>> "next_free_pfn = pfn", until the scanners get reset.
>>>>>>>
>>>>>>> The question of course is if such situations are frequent and makes
>>>>>>> any difference to compaction
>>>>>>> outcome. And the downsides are potentially useless rescans and code
>>>>>>> complexity. Maybe Mel
>>>>>>> remembers how important this is? It should probably be profiled
>>>>>>> before changes are made.
>>>>>> I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
>>>>>> At that time, I didn't Cced so I missed that code so let's ask this time.
>>>>>> In that patch, you added this.
>>>>>>
>>>>>> if (pfn < low_pfn)
>>>>>>   cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>>>>> else
>>>>>>   cc->free_pfn = high_pfn;
>>>>>
>>>>> Oh, right, this max(), not the one in the for loop. Sorry, I should
>>>>> have read more closely.
>>>>> But still maybe it's a good opportunity to kill the other max() as
>>>>> well. I'll try some testing.
>>>>>
>>>>> Anyway, this is what I answered to Mel when he asked the same thing
>>>>> when I sent
>>>>> that 7ed695069c3c patch:
>>>>>
>>>>> If a zone starts in a middle of a pageblock and migrate scanner isolates
>>>>> enough pages early to stay within that pageblock, low_pfn will be at the
>>>>> end of that pageblock and after the for cycle in this function ends, pfn
>>>>> might be at the beginning of that pageblock. It might not be an actual
>>>>> problem (this compaction will finish at this point, and if someone else
>>>>> is racing, he will probably check the boundaries himself), but I played
>>>>> it safe.
>>>>>
>>>>>
>>>>>> So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
>>>>>> compact_finished to stop compaction. And your [1/2] patch in this patchset
>>>>>> always makes free page scanner start on pageblock boundary so when the
>>>>>> loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
>>>>>> would be lower than migration scanner so compact_finished will always detect
>>>>>> it so I think you could just do
>>>>>>
>>>>>> if (pfn < low_pfn)
>>>>>>   next_free_pfn = pfn;
>>>>>>
>>>>>> cc->free_pfn = next_free_pfn;
>>>>>
>>>>> That could work. I was probably wrong about danger of racing in the
>>>>> reply to Mel,
>>>>> because free_pfn is stored in cc (private), not zone (shared).
>>>>>
>>>>>>
>>>>>> Or, if you want to clear *reset*,
>>>>>> if (pfn < lown_pfn)
>>>>>>   next_free_pfn = zone->zone_start_pfn;
>>>>>>
>>>>>> cc->free_pfn = next_free_pfn;
>>>>>
>>>>> That would work as well but is less straightforward I think. Might
>>>>> be misleading if
>>>>> someone added tracepoints to track the free scanner progress with
>>>>> pfn's (which
>>>>> might happen soon...)
>>>>
>>>> My preference is to add following with pair of compact_finished
>>>>
>>>> static inline void finish_compact(struct compact_control *cc)
>>>> {
>>>>   cc->free_pfn = cc->migrate_pfn;
>>>> }
>>>
>>> Yes setting free_pfn to migrate_pfn is probably the best way, as these
>>> are the values compared in compact_finished. But I wouldn't introduce a
>>> new function just for one instance of this. Also compact_finished()
>>> doesn't test just the scanners to decide whether compaction should
>>> continue, so the pairing would be imperfect anyway.
>>> So Andrew, if you agree can you please fold in the patch below.
>>>
>>>> But I don't care.
>>>> If you didn't send this patch as clean up, I would never interrupt
>>>> on the way but you said it's cleanup patch and the one made me spend a
>>>> few minutes to understand the code so it's not a clean up patch. ;-).
>>>> So, IMO, it's worth to tidy it up.
>>>
>>> Yes, I understand and agree.
>>>
>>> ------8<------
>>> From: Vlastimil Babka <vbabka@suse.cz>
>>> Date: Tue, 22 Apr 2014 13:55:36 +0200
>>> Subject: mm-compaction-cleanup-isolate_freepages-fix2
>>>
>>> Cleanup detection of compaction scanners crossing in isolate_freepages().
>>> To make sure compact_finished() observes scanners crossing, we can just set
>>> free_pfn to migrate_pfn instead of confusing max() construct.
>>>
>>> Suggested-by: Minchan Kim <minchan@kernel.org>
>>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>>> Cc: Christoph Lameter <cl@linux.com>
>>> Cc: Dongjun Shin <d.j.shin@samsung.com>
>>> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>>> Cc: Mel Gorman <mgorman@suse.de>
>>> Cc: Michal Nazarewicz <mina86@mina86.com>
>>> Cc: Minchan Kim <minchan@kernel.org>
>>> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>>> Cc: Rik van Riel <riel@redhat.com>
>>> Cc: Sunghwan Yun <sunghwan.yun@samsung.com>
>>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>>>
>>> ---
>>>  mm/compaction.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>> index 37c15fe..1c992dc 100644
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -768,7 +768,7 @@ static void isolate_freepages(struct zone *zone,
>>>       * so that compact_finished() may detect this
>>>       */
>>>      if (pfn < low_pfn)
>>> -            next_free_pfn = max(pfn, zone->zone_start_pfn);
>>> +            next_free_pfn = cc->migrate_pfn;
>>>
>>>      cc->free_pfn = next_free_pfn;
>>>      cc->nr_freepages = nr_freepages;
>>> --
>>> 1.8.4.5
>>>
>>
>> Hello,
>>
>> How about doing more clean-up at this time?
>>
>> What I did is that taking end_pfn out of the loop and consider zone
>> boundary once. After then, we just subtract pageblock_nr_pages on
>> every iteration. With this change, we can remove local variable, z_end_pfn.
>> Another things I did are removing max() operation and un-needed
>> assignment to isolate variable.
>>
>> Thanks.
>>
>> --------->8------------
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index 1c992dc..95a506d 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
>>                               struct compact_control *cc)
>>  {
>>       struct page *page;
>> -     unsigned long pfn;           /* scanning cursor */
>> +     unsigned long pfn;           /* start of scanning window */
>> +     unsigned long end_pfn;       /* end of scanning window */
>>       unsigned long low_pfn;       /* lowest pfn scanner is able to scan */
>>       unsigned long next_free_pfn; /* start pfn for scaning at next round */
>> -     unsigned long z_end_pfn;     /* zone's end pfn */
>>       int nr_freepages = cc->nr_freepages;
>>       struct list_head *freelist = &cc->freepages;
>>
>> @@ -688,15 +688,16 @@ static void isolate_freepages(struct zone *zone,
>>        * is using.
>>        */
>>       pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
>> -     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>
>>       /*
>> -      * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>> -      * isolated, the pfn < low_pfn check will kick in.
>> +      * Take care when isolating in last pageblock of a zone which
>> +      * ends in the middle of a pageblock.
>>        */
>> -     next_free_pfn = 0;
>> +     end_pfn = min(pfn + pageblock_nr_pages, zone_end_pfn(zone));
>> +     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>
>> -     z_end_pfn = zone_end_pfn(zone);
>> +     /* If no pages are isolated, the pfn < low_pfn check will kick in. */
>> +     next_free_pfn = 0;
>>
>>       /*
>>        * Isolate free pages until enough are available to migrate the
>> @@ -704,9 +705,8 @@ static void isolate_freepages(struct zone *zone,
>>        * and free page scanners meet or enough free pages are isolated.
>>        */
>>       for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
>> -                                     pfn -= pageblock_nr_pages) {
>> +             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
>
> If zone_end_pfn was in the middle of a pageblock, then your end_pfn will
> always be in the middle of a pageblock and you will not scan half of all
> pageblocks.
>

Okay. I think a way to fix it.
By assigning pfn(start of scanning window) to
end_pfn(end of scanning window) for the next loop, we can solve the problem
you mentioned. How about below?

-             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
+            end_pfn = pfn, pfn -= pageblock_nr_pages) {

Thanks.

--
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 2/2] mm/compaction: cleanup isolate_freepages()

[Vlastimil Babka]

>>>
>>> Hello,
>>>
>>> How about doing more clean-up at this time?
>>>
>>> What I did is that taking end_pfn out of the loop and consider zone
>>> boundary once. After then, we just subtract pageblock_nr_pages on
>>> every iteration. With this change, we can remove local variable, z_end_pfn.
>>> Another things I did are removing max() operation and un-needed
>>> assignment to isolate variable.
>>>
>>> Thanks.
>>>
>>> --------->8------------
>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>> index 1c992dc..95a506d 100644
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
>>>                               struct compact_control *cc)
>>>  {
>>>       struct page *page;
>>> -     unsigned long pfn;           /* scanning cursor */
>>> +     unsigned long pfn;           /* start of scanning window */
>>> +     unsigned long end_pfn;       /* end of scanning window */
>>>       unsigned long low_pfn;       /* lowest pfn scanner is able to scan */
>>>       unsigned long next_free_pfn; /* start pfn for scaning at next round */
>>> -     unsigned long z_end_pfn;     /* zone's end pfn */
>>>       int nr_freepages = cc->nr_freepages;
>>>       struct list_head *freelist = &cc->freepages;
>>>
>>> @@ -688,15 +688,16 @@ static void isolate_freepages(struct zone *zone,
>>>        * is using.
>>>        */
>>>       pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
>>> -     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>
>>>       /*
>>> -      * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>>> -      * isolated, the pfn < low_pfn check will kick in.
>>> +      * Take care when isolating in last pageblock of a zone which
>>> +      * ends in the middle of a pageblock.
>>>        */
>>> -     next_free_pfn = 0;
>>> +     end_pfn = min(pfn + pageblock_nr_pages, zone_end_pfn(zone));
>>> +     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>
>>> -     z_end_pfn = zone_end_pfn(zone);
>>> +     /* If no pages are isolated, the pfn < low_pfn check will kick in. */
>>> +     next_free_pfn = 0;
>>>
>>>       /*
>>>        * Isolate free pages until enough are available to migrate the
>>> @@ -704,9 +705,8 @@ static void isolate_freepages(struct zone *zone,
>>>        * and free page scanners meet or enough free pages are isolated.
>>>        */
>>>       for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
>>> -                                     pfn -= pageblock_nr_pages) {
>>> +             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
>>
>> If zone_end_pfn was in the middle of a pageblock, then your end_pfn will
>> always be in the middle of a pageblock and you will not scan half of all
>> pageblocks.
>>
> 
> Okay. I think a way to fix it.
> By assigning pfn(start of scanning window) to
> end_pfn(end of scanning window) for the next loop, we can solve the problem
> you mentioned. How about below?
> 
> -             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
> +            end_pfn = pfn, pfn -= pageblock_nr_pages) {

Hm that's perhaps a bit subtle but it would work.
Maybe better names for pfn and end_pfn would be block_start_pfn and
block_end_pfn. And in those comments, s/scanning window/current pageblock/.
And please don't move the low_pfn assignment like you did. The comment
above the original location explains it, the comment above the new
location doesn't. It's use in the loop is also related to 'pfn', not
'end_pfn'.

> Thanks.
> 

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Vlastimil Babka]

>>>
>>> Hello,
>>>
>>> How about doing more clean-up at this time?
>>>
>>> What I did is that taking end_pfn out of the loop and consider zone
>>> boundary once. After then, we just subtract pageblock_nr_pages on
>>> every iteration. With this change, we can remove local variable, z_end_pfn.
>>> Another things I did are removing max() operation and un-needed
>>> assignment to isolate variable.
>>>
>>> Thanks.
>>>
>>> --------->8------------
>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>> index 1c992dc..95a506d 100644
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
>>>                               struct compact_control *cc)
>>>  {
>>>       struct page *page;
>>> -     unsigned long pfn;           /* scanning cursor */
>>> +     unsigned long pfn;           /* start of scanning window */
>>> +     unsigned long end_pfn;       /* end of scanning window */
>>>       unsigned long low_pfn;       /* lowest pfn scanner is able to scan */
>>>       unsigned long next_free_pfn; /* start pfn for scaning at next round */
>>> -     unsigned long z_end_pfn;     /* zone's end pfn */
>>>       int nr_freepages = cc->nr_freepages;
>>>       struct list_head *freelist = &cc->freepages;
>>>
>>> @@ -688,15 +688,16 @@ static void isolate_freepages(struct zone *zone,
>>>        * is using.
>>>        */
>>>       pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
>>> -     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>
>>>       /*
>>> -      * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>>> -      * isolated, the pfn < low_pfn check will kick in.
>>> +      * Take care when isolating in last pageblock of a zone which
>>> +      * ends in the middle of a pageblock.
>>>        */
>>> -     next_free_pfn = 0;
>>> +     end_pfn = min(pfn + pageblock_nr_pages, zone_end_pfn(zone));
>>> +     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>
>>> -     z_end_pfn = zone_end_pfn(zone);
>>> +     /* If no pages are isolated, the pfn < low_pfn check will kick in. */
>>> +     next_free_pfn = 0;
>>>
>>>       /*
>>>        * Isolate free pages until enough are available to migrate the
>>> @@ -704,9 +705,8 @@ static void isolate_freepages(struct zone *zone,
>>>        * and free page scanners meet or enough free pages are isolated.
>>>        */
>>>       for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
>>> -                                     pfn -= pageblock_nr_pages) {
>>> +             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
>>
>> If zone_end_pfn was in the middle of a pageblock, then your end_pfn will
>> always be in the middle of a pageblock and you will not scan half of all
>> pageblocks.
>>
> 
> Okay. I think a way to fix it.
> By assigning pfn(start of scanning window) to
> end_pfn(end of scanning window) for the next loop, we can solve the problem
> you mentioned. How about below?
> 
> -             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
> +            end_pfn = pfn, pfn -= pageblock_nr_pages) {

Hm that's perhaps a bit subtle but it would work.
Maybe better names for pfn and end_pfn would be block_start_pfn and
block_end_pfn. And in those comments, s/scanning window/current pageblock/.
And please don't move the low_pfn assignment like you did. The comment
above the original location explains it, the comment above the new
location doesn't. It's use in the loop is also related to 'pfn', not
'end_pfn'.

> Thanks.
> 

--
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 2/2] mm/compaction: cleanup isolate_freepages()

[Joonsoo Kim]

On Wed, Apr 23, 2014 at 04:31:14PM +0200, Vlastimil Babka wrote:
> >>>
> >>> Hello,
> >>>
> >>> How about doing more clean-up at this time?
> >>>
> >>> What I did is that taking end_pfn out of the loop and consider zone
> >>> boundary once. After then, we just subtract pageblock_nr_pages on
> >>> every iteration. With this change, we can remove local variable, z_end_pfn.
> >>> Another things I did are removing max() operation and un-needed
> >>> assignment to isolate variable.
> >>>
> >>> Thanks.
> >>>
> >>> --------->8------------
> >>> diff --git a/mm/compaction.c b/mm/compaction.c
> >>> index 1c992dc..95a506d 100644
> >>> --- a/mm/compaction.c
> >>> +++ b/mm/compaction.c
> >>> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
> >>>                               struct compact_control *cc)
> >>>  {
> >>>       struct page *page;
> >>> -     unsigned long pfn;           /* scanning cursor */
> >>> +     unsigned long pfn;           /* start of scanning window */
> >>> +     unsigned long end_pfn;       /* end of scanning window */
> >>>       unsigned long low_pfn;       /* lowest pfn scanner is able to scan */
> >>>       unsigned long next_free_pfn; /* start pfn for scaning at next round */
> >>> -     unsigned long z_end_pfn;     /* zone's end pfn */
> >>>       int nr_freepages = cc->nr_freepages;
> >>>       struct list_head *freelist = &cc->freepages;
> >>>
> >>> @@ -688,15 +688,16 @@ static void isolate_freepages(struct zone *zone,
> >>>        * is using.
> >>>        */
> >>>       pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
> >>> -     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>
> >>>       /*
> >>> -      * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> >>> -      * isolated, the pfn < low_pfn check will kick in.
> >>> +      * Take care when isolating in last pageblock of a zone which
> >>> +      * ends in the middle of a pageblock.
> >>>        */
> >>> -     next_free_pfn = 0;
> >>> +     end_pfn = min(pfn + pageblock_nr_pages, zone_end_pfn(zone));
> >>> +     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>
> >>> -     z_end_pfn = zone_end_pfn(zone);
> >>> +     /* If no pages are isolated, the pfn < low_pfn check will kick in. */
> >>> +     next_free_pfn = 0;
> >>>
> >>>       /*
> >>>        * Isolate free pages until enough are available to migrate the
> >>> @@ -704,9 +705,8 @@ static void isolate_freepages(struct zone *zone,
> >>>        * and free page scanners meet or enough free pages are isolated.
> >>>        */
> >>>       for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
> >>> -                                     pfn -= pageblock_nr_pages) {
> >>> +             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
> >>
> >> If zone_end_pfn was in the middle of a pageblock, then your end_pfn will
> >> always be in the middle of a pageblock and you will not scan half of all
> >> pageblocks.
> >>
> > 
> > Okay. I think a way to fix it.
> > By assigning pfn(start of scanning window) to
> > end_pfn(end of scanning window) for the next loop, we can solve the problem
> > you mentioned. How about below?
> > 
> > -             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
> > +            end_pfn = pfn, pfn -= pageblock_nr_pages) {
> 
> Hm that's perhaps a bit subtle but it would work.
> Maybe better names for pfn and end_pfn would be block_start_pfn and
> block_end_pfn. And in those comments, s/scanning window/current pageblock/.
> And please don't move the low_pfn assignment like you did. The comment
> above the original location explains it, the comment above the new
> location doesn't. It's use in the loop is also related to 'pfn', not
> 'end_pfn'.

Okay.
Following patch solves all your concerns.
End result looks so nice to me. :)

Thanks.

--------->8----------------
>From ae653cf8b9f8c7423cad73af38cde94eec564b50 Mon Sep 17 00:00:00 2001
From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Date: Fri, 25 Apr 2014 17:12:58 +0900
Subject: [PATCH] mm-compaction-cleanup-isolate_freepages-fix3

What I did here is taking end_pfn out of the loop and considering zone
boundary once. After then, we can just set previous pfn to end_pfn on
every iteration to move scanning window. With this change, we can remove
local variable, z_end_pfn.

Another things I did are removing max() operation and un-needed
assignment to isolate variable.

In addition, I change both the variable names, from pfn and
end_pfn to block_start_pfn and block_end_pfn, respectively.
They represent their meaning perfectly.

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

diff --git a/mm/compaction.c b/mm/compaction.c
index 1c992dc..ba80bea 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
 				struct compact_control *cc)
 {
 	struct page *page;
-	unsigned long pfn;	     /* scanning cursor */
+	unsigned long block_start_pfn;	/* start of current pageblock */
+	unsigned long block_end_pfn;	/* end of current pageblock */
 	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
 	unsigned long next_free_pfn; /* start pfn for scaning at next round */
-	unsigned long z_end_pfn;     /* zone's end pfn */
 	int nr_freepages = cc->nr_freepages;
 	struct list_head *freelist = &cc->freepages;
 
@@ -682,31 +682,33 @@ static void isolate_freepages(struct zone *zone,
 	 * Initialise the free scanner. The starting point is where we last
 	 * successfully isolated from, zone-cached value, or the end of the
 	 * zone when isolating for the first time. We need this aligned to
-	 * the pageblock boundary, because we do pfn -= pageblock_nr_pages
-	 * in the for loop.
+	 * the pageblock boundary, because we do
+	 * block_start_pfn -= pageblock_nr_pages in the for loop.
+	 * For ending point, take care when isolating in last pageblock of a
+	 * a zone which ends in the middle of a pageblock.
 	 * The low boundary is the end of the pageblock the migration scanner
 	 * is using.
 	 */
-	pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
+	block_start_pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
+	block_end_pfn = min(block_start_pfn + pageblock_nr_pages,
+						zone_end_pfn(zone));
 	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
 
 	/*
-	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
-	 * isolated, the pfn < low_pfn check will kick in.
+	 * If no pages are isolated, the block_start_pfn < low_pfn check
+	 * will kick in.
 	 */
 	next_free_pfn = 0;
 
-	z_end_pfn = zone_end_pfn(zone);
-
 	/*
 	 * Isolate free pages until enough are available to migrate the
 	 * pages on cc->migratepages. We stop searching if the migrate
 	 * and free page scanners meet or enough free pages are isolated.
 	 */
-	for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
-					pfn -= pageblock_nr_pages) {
+	for (;block_start_pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
+				block_end_pfn = block_start_pfn,
+				block_start_pfn -= pageblock_nr_pages) {
 		unsigned long isolated;
-		unsigned long end_pfn;
 
 		/*
 		 * This can iterate a massively long zone without finding any
@@ -715,7 +717,7 @@ static void isolate_freepages(struct zone *zone,
 		 */
 		cond_resched();
 
-		if (!pfn_valid(pfn))
+		if (!pfn_valid(block_start_pfn))
 			continue;
 
 		/*
@@ -725,7 +727,7 @@ static void isolate_freepages(struct zone *zone,
 		 * i.e. it's possible that all pages within a zones range of
 		 * pages do not belong to a single zone.
 		 */
-		page = pfn_to_page(pfn);
+		page = pfn_to_page(block_start_pfn);
 		if (page_zone(page) != zone)
 			continue;
 
@@ -738,15 +740,8 @@ static void isolate_freepages(struct zone *zone,
 			continue;
 
 		/* Found a block suitable for isolating free pages from */
-		isolated = 0;
-
-		/*
-		 * Take care when isolating in last pageblock of a zone which
-		 * ends in the middle of a pageblock.
-		 */
-		end_pfn = min(pfn + pageblock_nr_pages, z_end_pfn);
-		isolated = isolate_freepages_block(cc, pfn, end_pfn,
-						   freelist, false);
+		isolated = isolate_freepages_block(cc, block_start_pfn,
+					block_end_pfn, freelist, false);
 		nr_freepages += isolated;
 
 		/*
@@ -754,9 +749,9 @@ static void isolate_freepages(struct zone *zone,
 		 * looking for free pages, the search will restart here as
 		 * page migration may have returned some pages to the allocator
 		 */
-		if (isolated) {
+		if (isolated && next_free_pfn == 0) {
 			cc->finished_update_free = true;
-			next_free_pfn = max(next_free_pfn, pfn);
+			next_free_pfn = block_start_pfn;
 		}
 	}
 
@@ -767,7 +762,7 @@ static void isolate_freepages(struct zone *zone,
 	 * If we crossed the migrate scanner, we want to keep it that way
 	 * so that compact_finished() may detect this
 	 */
-	if (pfn < low_pfn)
+	if (block_start_pfn < low_pfn)
 		next_free_pfn = cc->migrate_pfn;
 
 	cc->free_pfn = next_free_pfn;
-- 
1.7.9.5

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Joonsoo Kim]

On Wed, Apr 23, 2014 at 04:31:14PM +0200, Vlastimil Babka wrote:
> >>>
> >>> Hello,
> >>>
> >>> How about doing more clean-up at this time?
> >>>
> >>> What I did is that taking end_pfn out of the loop and consider zone
> >>> boundary once. After then, we just subtract pageblock_nr_pages on
> >>> every iteration. With this change, we can remove local variable, z_end_pfn.
> >>> Another things I did are removing max() operation and un-needed
> >>> assignment to isolate variable.
> >>>
> >>> Thanks.
> >>>
> >>> --------->8------------
> >>> diff --git a/mm/compaction.c b/mm/compaction.c
> >>> index 1c992dc..95a506d 100644
> >>> --- a/mm/compaction.c
> >>> +++ b/mm/compaction.c
> >>> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
> >>>                               struct compact_control *cc)
> >>>  {
> >>>       struct page *page;
> >>> -     unsigned long pfn;           /* scanning cursor */
> >>> +     unsigned long pfn;           /* start of scanning window */
> >>> +     unsigned long end_pfn;       /* end of scanning window */
> >>>       unsigned long low_pfn;       /* lowest pfn scanner is able to scan */
> >>>       unsigned long next_free_pfn; /* start pfn for scaning at next round */
> >>> -     unsigned long z_end_pfn;     /* zone's end pfn */
> >>>       int nr_freepages = cc->nr_freepages;
> >>>       struct list_head *freelist = &cc->freepages;
> >>>
> >>> @@ -688,15 +688,16 @@ static void isolate_freepages(struct zone *zone,
> >>>        * is using.
> >>>        */
> >>>       pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
> >>> -     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>
> >>>       /*
> >>> -      * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> >>> -      * isolated, the pfn < low_pfn check will kick in.
> >>> +      * Take care when isolating in last pageblock of a zone which
> >>> +      * ends in the middle of a pageblock.
> >>>        */
> >>> -     next_free_pfn = 0;
> >>> +     end_pfn = min(pfn + pageblock_nr_pages, zone_end_pfn(zone));
> >>> +     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>
> >>> -     z_end_pfn = zone_end_pfn(zone);
> >>> +     /* If no pages are isolated, the pfn < low_pfn check will kick in. */
> >>> +     next_free_pfn = 0;
> >>>
> >>>       /*
> >>>        * Isolate free pages until enough are available to migrate the
> >>> @@ -704,9 +705,8 @@ static void isolate_freepages(struct zone *zone,
> >>>        * and free page scanners meet or enough free pages are isolated.
> >>>        */
> >>>       for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
> >>> -                                     pfn -= pageblock_nr_pages) {
> >>> +             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
> >>
> >> If zone_end_pfn was in the middle of a pageblock, then your end_pfn will
> >> always be in the middle of a pageblock and you will not scan half of all
> >> pageblocks.
> >>
> > 
> > Okay. I think a way to fix it.
> > By assigning pfn(start of scanning window) to
> > end_pfn(end of scanning window) for the next loop, we can solve the problem
> > you mentioned. How about below?
> > 
> > -             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
> > +            end_pfn = pfn, pfn -= pageblock_nr_pages) {
> 
> Hm that's perhaps a bit subtle but it would work.
> Maybe better names for pfn and end_pfn would be block_start_pfn and
> block_end_pfn. And in those comments, s/scanning window/current pageblock/.
> And please don't move the low_pfn assignment like you did. The comment
> above the original location explains it, the comment above the new
> location doesn't. It's use in the loop is also related to 'pfn', not
> 'end_pfn'.

Okay.
Following patch solves all your concerns.
End result looks so nice to me. :)

Thanks.

--------->8----------------

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Vlastimil Babka]

On 04/25/2014 10:29 AM, Joonsoo Kim wrote:
> On Wed, Apr 23, 2014 at 04:31:14PM +0200, Vlastimil Babka wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> How about doing more clean-up at this time?
>>>>>
>>>>> What I did is that taking end_pfn out of the loop and consider zone
>>>>> boundary once. After then, we just subtract pageblock_nr_pages on
>>>>> every iteration. With this change, we can remove local variable, z_end_pfn.
>>>>> Another things I did are removing max() operation and un-needed
>>>>> assignment to isolate variable.
>>>>>
>>>>> Thanks.
>>>>>
>>>>> --------->8------------
>>>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>>>> index 1c992dc..95a506d 100644
>>>>> --- a/mm/compaction.c
>>>>> +++ b/mm/compaction.c
>>>>> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
>>>>>                                struct compact_control *cc)
>>>>>   {
>>>>>        struct page *page;
>>>>> -     unsigned long pfn;           /* scanning cursor */
>>>>> +     unsigned long pfn;           /* start of scanning window */
>>>>> +     unsigned long end_pfn;       /* end of scanning window */
>>>>>        unsigned long low_pfn;       /* lowest pfn scanner is able to scan */
>>>>>        unsigned long next_free_pfn; /* start pfn for scaning at next round */
>>>>> -     unsigned long z_end_pfn;     /* zone's end pfn */
>>>>>        int nr_freepages = cc->nr_freepages;
>>>>>        struct list_head *freelist = &cc->freepages;
>>>>>
>>>>> @@ -688,15 +688,16 @@ static void isolate_freepages(struct zone *zone,
>>>>>         * is using.
>>>>>         */
>>>>>        pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
>>>>> -     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>
>>>>>        /*
>>>>> -      * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>>>>> -      * isolated, the pfn < low_pfn check will kick in.
>>>>> +      * Take care when isolating in last pageblock of a zone which
>>>>> +      * ends in the middle of a pageblock.
>>>>>         */
>>>>> -     next_free_pfn = 0;
>>>>> +     end_pfn = min(pfn + pageblock_nr_pages, zone_end_pfn(zone));
>>>>> +     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>
>>>>> -     z_end_pfn = zone_end_pfn(zone);
>>>>> +     /* If no pages are isolated, the pfn < low_pfn check will kick in. */
>>>>> +     next_free_pfn = 0;
>>>>>
>>>>>        /*
>>>>>         * Isolate free pages until enough are available to migrate the
>>>>> @@ -704,9 +705,8 @@ static void isolate_freepages(struct zone *zone,
>>>>>         * and free page scanners meet or enough free pages are isolated.
>>>>>         */
>>>>>        for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
>>>>> -                                     pfn -= pageblock_nr_pages) {
>>>>> +             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
>>>>
>>>> If zone_end_pfn was in the middle of a pageblock, then your end_pfn will
>>>> always be in the middle of a pageblock and you will not scan half of all
>>>> pageblocks.
>>>>
>>>
>>> Okay. I think a way to fix it.
>>> By assigning pfn(start of scanning window) to
>>> end_pfn(end of scanning window) for the next loop, we can solve the problem
>>> you mentioned. How about below?
>>>
>>> -             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
>>> +            end_pfn = pfn, pfn -= pageblock_nr_pages) {
>>
>> Hm that's perhaps a bit subtle but it would work.
>> Maybe better names for pfn and end_pfn would be block_start_pfn and
>> block_end_pfn. And in those comments, s/scanning window/current pageblock/.
>> And please don't move the low_pfn assignment like you did. The comment
>> above the original location explains it, the comment above the new
>> location doesn't. It's use in the loop is also related to 'pfn', not
>> 'end_pfn'.
>
> Okay.
> Following patch solves all your concerns.
> End result looks so nice to me. :)

Great, thanks!

> Thanks.
>
> --------->8----------------
>  From ae653cf8b9f8c7423cad73af38cde94eec564b50 Mon Sep 17 00:00:00 2001
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Date: Fri, 25 Apr 2014 17:12:58 +0900
> Subject: [PATCH] mm-compaction-cleanup-isolate_freepages-fix3
>
> What I did here is taking end_pfn out of the loop and considering zone
> boundary once. After then, we can just set previous pfn to end_pfn on
> every iteration to move scanning window. With this change, we can remove
> local variable, z_end_pfn.
>
> Another things I did are removing max() operation and un-needed
> assignment to isolate variable.
>
> In addition, I change both the variable names, from pfn and
> end_pfn to block_start_pfn and block_end_pfn, respectively.
> They represent their meaning perfectly.
>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Acked-by: Vlastimil Babka <vbabka@suse.de>

> diff --git a/mm/compaction.c b/mm/compaction.c
> index 1c992dc..ba80bea 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
>   				struct compact_control *cc)
>   {
>   	struct page *page;
> -	unsigned long pfn;	     /* scanning cursor */
> +	unsigned long block_start_pfn;	/* start of current pageblock */
> +	unsigned long block_end_pfn;	/* end of current pageblock */
>   	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
>   	unsigned long next_free_pfn; /* start pfn for scaning at next round */
> -	unsigned long z_end_pfn;     /* zone's end pfn */
>   	int nr_freepages = cc->nr_freepages;
>   	struct list_head *freelist = &cc->freepages;
>
> @@ -682,31 +682,33 @@ static void isolate_freepages(struct zone *zone,
>   	 * Initialise the free scanner. The starting point is where we last
>   	 * successfully isolated from, zone-cached value, or the end of the
>   	 * zone when isolating for the first time. We need this aligned to
> -	 * the pageblock boundary, because we do pfn -= pageblock_nr_pages
> -	 * in the for loop.
> +	 * the pageblock boundary, because we do
> +	 * block_start_pfn -= pageblock_nr_pages in the for loop.
> +	 * For ending point, take care when isolating in last pageblock of a
> +	 * a zone which ends in the middle of a pageblock.
>   	 * The low boundary is the end of the pageblock the migration scanner
>   	 * is using.
>   	 */
> -	pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
> +	block_start_pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
> +	block_end_pfn = min(block_start_pfn + pageblock_nr_pages,
> +						zone_end_pfn(zone));
>   	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>
>   	/*
> -	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> -	 * isolated, the pfn < low_pfn check will kick in.
> +	 * If no pages are isolated, the block_start_pfn < low_pfn check
> +	 * will kick in.
>   	 */
>   	next_free_pfn = 0;
>
> -	z_end_pfn = zone_end_pfn(zone);
> -
>   	/*
>   	 * Isolate free pages until enough are available to migrate the
>   	 * pages on cc->migratepages. We stop searching if the migrate
>   	 * and free page scanners meet or enough free pages are isolated.
>   	 */
> -	for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
> -					pfn -= pageblock_nr_pages) {
> +	for (;block_start_pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
> +				block_end_pfn = block_start_pfn,
> +				block_start_pfn -= pageblock_nr_pages) {
>   		unsigned long isolated;
> -		unsigned long end_pfn;
>
>   		/*
>   		 * This can iterate a massively long zone without finding any
> @@ -715,7 +717,7 @@ static void isolate_freepages(struct zone *zone,
>   		 */
>   		cond_resched();
>
> -		if (!pfn_valid(pfn))
> +		if (!pfn_valid(block_start_pfn))
>   			continue;
>
>   		/*
> @@ -725,7 +727,7 @@ static void isolate_freepages(struct zone *zone,
>   		 * i.e. it's possible that all pages within a zones range of
>   		 * pages do not belong to a single zone.
>   		 */
> -		page = pfn_to_page(pfn);
> +		page = pfn_to_page(block_start_pfn);
>   		if (page_zone(page) != zone)
>   			continue;
>
> @@ -738,15 +740,8 @@ static void isolate_freepages(struct zone *zone,
>   			continue;
>
>   		/* Found a block suitable for isolating free pages from */
> -		isolated = 0;
> -
> -		/*
> -		 * Take care when isolating in last pageblock of a zone which
> -		 * ends in the middle of a pageblock.
> -		 */
> -		end_pfn = min(pfn + pageblock_nr_pages, z_end_pfn);
> -		isolated = isolate_freepages_block(cc, pfn, end_pfn,
> -						   freelist, false);
> +		isolated = isolate_freepages_block(cc, block_start_pfn,
> +					block_end_pfn, freelist, false);
>   		nr_freepages += isolated;
>
>   		/*
> @@ -754,9 +749,9 @@ static void isolate_freepages(struct zone *zone,
>   		 * looking for free pages, the search will restart here as
>   		 * page migration may have returned some pages to the allocator
>   		 */
> -		if (isolated) {
> +		if (isolated && next_free_pfn == 0) {
>   			cc->finished_update_free = true;
> -			next_free_pfn = max(next_free_pfn, pfn);
> +			next_free_pfn = block_start_pfn;
>   		}
>   	}
>
> @@ -767,7 +762,7 @@ static void isolate_freepages(struct zone *zone,
>   	 * If we crossed the migrate scanner, we want to keep it that way
>   	 * so that compact_finished() may detect this
>   	 */
> -	if (pfn < low_pfn)
> +	if (block_start_pfn < low_pfn)
>   		next_free_pfn = cc->migrate_pfn;
>
>   	cc->free_pfn = next_free_pfn;
>

Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Vlastimil Babka]

On 04/25/2014 10:29 AM, Joonsoo Kim wrote:
> On Wed, Apr 23, 2014 at 04:31:14PM +0200, Vlastimil Babka wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> How about doing more clean-up at this time?
>>>>>
>>>>> What I did is that taking end_pfn out of the loop and consider zone
>>>>> boundary once. After then, we just subtract pageblock_nr_pages on
>>>>> every iteration. With this change, we can remove local variable, z_end_pfn.
>>>>> Another things I did are removing max() operation and un-needed
>>>>> assignment to isolate variable.
>>>>>
>>>>> Thanks.
>>>>>
>>>>> --------->8------------
>>>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>>>> index 1c992dc..95a506d 100644
>>>>> --- a/mm/compaction.c
>>>>> +++ b/mm/compaction.c
>>>>> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
>>>>>                                struct compact_control *cc)
>>>>>   {
>>>>>        struct page *page;
>>>>> -     unsigned long pfn;           /* scanning cursor */
>>>>> +     unsigned long pfn;           /* start of scanning window */
>>>>> +     unsigned long end_pfn;       /* end of scanning window */
>>>>>        unsigned long low_pfn;       /* lowest pfn scanner is able to scan */
>>>>>        unsigned long next_free_pfn; /* start pfn for scaning at next round */
>>>>> -     unsigned long z_end_pfn;     /* zone's end pfn */
>>>>>        int nr_freepages = cc->nr_freepages;
>>>>>        struct list_head *freelist = &cc->freepages;
>>>>>
>>>>> @@ -688,15 +688,16 @@ static void isolate_freepages(struct zone *zone,
>>>>>         * is using.
>>>>>         */
>>>>>        pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
>>>>> -     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>
>>>>>        /*
>>>>> -      * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>>>>> -      * isolated, the pfn < low_pfn check will kick in.
>>>>> +      * Take care when isolating in last pageblock of a zone which
>>>>> +      * ends in the middle of a pageblock.
>>>>>         */
>>>>> -     next_free_pfn = 0;
>>>>> +     end_pfn = min(pfn + pageblock_nr_pages, zone_end_pfn(zone));
>>>>> +     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>
>>>>> -     z_end_pfn = zone_end_pfn(zone);
>>>>> +     /* If no pages are isolated, the pfn < low_pfn check will kick in. */
>>>>> +     next_free_pfn = 0;
>>>>>
>>>>>        /*
>>>>>         * Isolate free pages until enough are available to migrate the
>>>>> @@ -704,9 +705,8 @@ static void isolate_freepages(struct zone *zone,
>>>>>         * and free page scanners meet or enough free pages are isolated.
>>>>>         */
>>>>>        for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
>>>>> -                                     pfn -= pageblock_nr_pages) {
>>>>> +             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
>>>>
>>>> If zone_end_pfn was in the middle of a pageblock, then your end_pfn will
>>>> always be in the middle of a pageblock and you will not scan half of all
>>>> pageblocks.
>>>>
>>>
>>> Okay. I think a way to fix it.
>>> By assigning pfn(start of scanning window) to
>>> end_pfn(end of scanning window) for the next loop, we can solve the problem
>>> you mentioned. How about below?
>>>
>>> -             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
>>> +            end_pfn = pfn, pfn -= pageblock_nr_pages) {
>>
>> Hm that's perhaps a bit subtle but it would work.
>> Maybe better names for pfn and end_pfn would be block_start_pfn and
>> block_end_pfn. And in those comments, s/scanning window/current pageblock/.
>> And please don't move the low_pfn assignment like you did. The comment
>> above the original location explains it, the comment above the new
>> location doesn't. It's use in the loop is also related to 'pfn', not
>> 'end_pfn'.
>
> Okay.
> Following patch solves all your concerns.
> End result looks so nice to me. :)

Great, thanks!

> Thanks.
>
> --------->8----------------
>  From ae653cf8b9f8c7423cad73af38cde94eec564b50 Mon Sep 17 00:00:00 2001
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Date: Fri, 25 Apr 2014 17:12:58 +0900
> Subject: [PATCH] mm-compaction-cleanup-isolate_freepages-fix3
>
> What I did here is taking end_pfn out of the loop and considering zone
> boundary once. After then, we can just set previous pfn to end_pfn on
> every iteration to move scanning window. With this change, we can remove
> local variable, z_end_pfn.
>
> Another things I did are removing max() operation and un-needed
> assignment to isolate variable.
>
> In addition, I change both the variable names, from pfn and
> end_pfn to block_start_pfn and block_end_pfn, respectively.
> They represent their meaning perfectly.
>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Acked-by: Vlastimil Babka <vbabka@suse.de>

> diff --git a/mm/compaction.c b/mm/compaction.c
> index 1c992dc..ba80bea 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
>   				struct compact_control *cc)
>   {
>   	struct page *page;
> -	unsigned long pfn;	     /* scanning cursor */
> +	unsigned long block_start_pfn;	/* start of current pageblock */
> +	unsigned long block_end_pfn;	/* end of current pageblock */
>   	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
>   	unsigned long next_free_pfn; /* start pfn for scaning at next round */
> -	unsigned long z_end_pfn;     /* zone's end pfn */
>   	int nr_freepages = cc->nr_freepages;
>   	struct list_head *freelist = &cc->freepages;
>
> @@ -682,31 +682,33 @@ static void isolate_freepages(struct zone *zone,
>   	 * Initialise the free scanner. The starting point is where we last
>   	 * successfully isolated from, zone-cached value, or the end of the
>   	 * zone when isolating for the first time. We need this aligned to
> -	 * the pageblock boundary, because we do pfn -= pageblock_nr_pages
> -	 * in the for loop.
> +	 * the pageblock boundary, because we do
> +	 * block_start_pfn -= pageblock_nr_pages in the for loop.
> +	 * For ending point, take care when isolating in last pageblock of a
> +	 * a zone which ends in the middle of a pageblock.
>   	 * The low boundary is the end of the pageblock the migration scanner
>   	 * is using.
>   	 */
> -	pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
> +	block_start_pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
> +	block_end_pfn = min(block_start_pfn + pageblock_nr_pages,
> +						zone_end_pfn(zone));
>   	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>
>   	/*
> -	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> -	 * isolated, the pfn < low_pfn check will kick in.
> +	 * If no pages are isolated, the block_start_pfn < low_pfn check
> +	 * will kick in.
>   	 */
>   	next_free_pfn = 0;
>
> -	z_end_pfn = zone_end_pfn(zone);
> -
>   	/*
>   	 * Isolate free pages until enough are available to migrate the
>   	 * pages on cc->migratepages. We stop searching if the migrate
>   	 * and free page scanners meet or enough free pages are isolated.
>   	 */
> -	for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
> -					pfn -= pageblock_nr_pages) {
> +	for (;block_start_pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
> +				block_end_pfn = block_start_pfn,
> +				block_start_pfn -= pageblock_nr_pages) {
>   		unsigned long isolated;
> -		unsigned long end_pfn;
>
>   		/*
>   		 * This can iterate a massively long zone without finding any
> @@ -715,7 +717,7 @@ static void isolate_freepages(struct zone *zone,
>   		 */
>   		cond_resched();
>
> -		if (!pfn_valid(pfn))
> +		if (!pfn_valid(block_start_pfn))
>   			continue;
>
>   		/*
> @@ -725,7 +727,7 @@ static void isolate_freepages(struct zone *zone,
>   		 * i.e. it's possible that all pages within a zones range of
>   		 * pages do not belong to a single zone.
>   		 */
> -		page = pfn_to_page(pfn);
> +		page = pfn_to_page(block_start_pfn);
>   		if (page_zone(page) != zone)
>   			continue;
>
> @@ -738,15 +740,8 @@ static void isolate_freepages(struct zone *zone,
>   			continue;
>
>   		/* Found a block suitable for isolating free pages from */
> -		isolated = 0;
> -
> -		/*
> -		 * Take care when isolating in last pageblock of a zone which
> -		 * ends in the middle of a pageblock.
> -		 */
> -		end_pfn = min(pfn + pageblock_nr_pages, z_end_pfn);
> -		isolated = isolate_freepages_block(cc, pfn, end_pfn,
> -						   freelist, false);
> +		isolated = isolate_freepages_block(cc, block_start_pfn,
> +					block_end_pfn, freelist, false);
>   		nr_freepages += isolated;
>
>   		/*
> @@ -754,9 +749,9 @@ static void isolate_freepages(struct zone *zone,
>   		 * looking for free pages, the search will restart here as
>   		 * page migration may have returned some pages to the allocator
>   		 */
> -		if (isolated) {
> +		if (isolated && next_free_pfn == 0) {
>   			cc->finished_update_free = true;
> -			next_free_pfn = max(next_free_pfn, pfn);
> +			next_free_pfn = block_start_pfn;
>   		}
>   	}
>
> @@ -767,7 +762,7 @@ static void isolate_freepages(struct zone *zone,
>   	 * If we crossed the migrate scanner, we want to keep it that way
>   	 * so that compact_finished() may detect this
>   	 */
> -	if (pfn < low_pfn)
> +	if (block_start_pfn < low_pfn)
>   		next_free_pfn = cc->migrate_pfn;
>
>   	cc->free_pfn = next_free_pfn;
>

--
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 2/2] mm/compaction: cleanup isolate_freepages()

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 5676 bytes --]

On Fri, Apr 25 2014, Joonsoo Kim wrote:
> Subject: [PATCH] mm-compaction-cleanup-isolate_freepages-fix3
>
> What I did here is taking end_pfn out of the loop and considering zone
> boundary once. After then, we can just set previous pfn to end_pfn on
> every iteration to move scanning window. With this change, we can remove
> local variable, z_end_pfn.
>
> Another things I did are removing max() operation and un-needed
> assignment to isolate variable.
>
> In addition, I change both the variable names, from pfn and
> end_pfn to block_start_pfn and block_end_pfn, respectively.
> They represent their meaning perfectly.
>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

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

> diff --git a/mm/compaction.c b/mm/compaction.c
> index 1c992dc..ba80bea 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
>  				struct compact_control *cc)
>  {
>  	struct page *page;
> -	unsigned long pfn;	     /* scanning cursor */
> +	unsigned long block_start_pfn;	/* start of current pageblock */
> +	unsigned long block_end_pfn;	/* end of current pageblock */
>  	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
>  	unsigned long next_free_pfn; /* start pfn for scaning at next round */
> -	unsigned long z_end_pfn;     /* zone's end pfn */
>  	int nr_freepages = cc->nr_freepages;
>  	struct list_head *freelist = &cc->freepages;
>  
> @@ -682,31 +682,33 @@ static void isolate_freepages(struct zone *zone,
>  	 * Initialise the free scanner. The starting point is where we last
>  	 * successfully isolated from, zone-cached value, or the end of the
>  	 * zone when isolating for the first time. We need this aligned to
> -	 * the pageblock boundary, because we do pfn -= pageblock_nr_pages
> -	 * in the for loop.
> +	 * the pageblock boundary, because we do
> +	 * block_start_pfn -= pageblock_nr_pages in the for loop.
> +	 * For ending point, take care when isolating in last pageblock of a
> +	 * a zone which ends in the middle of a pageblock.
>  	 * The low boundary is the end of the pageblock the migration scanner
>  	 * is using.
>  	 */
> -	pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
> +	block_start_pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
> +	block_end_pfn = min(block_start_pfn + pageblock_nr_pages,
> +						zone_end_pfn(zone));
>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>  
>  	/*
> -	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> -	 * isolated, the pfn < low_pfn check will kick in.
> +	 * If no pages are isolated, the block_start_pfn < low_pfn check
> +	 * will kick in.
>  	 */
>  	next_free_pfn = 0;
>  
> -	z_end_pfn = zone_end_pfn(zone);
> -
>  	/*
>  	 * Isolate free pages until enough are available to migrate the
>  	 * pages on cc->migratepages. We stop searching if the migrate
>  	 * and free page scanners meet or enough free pages are isolated.
>  	 */
> -	for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
> -					pfn -= pageblock_nr_pages) {
> +	for (;block_start_pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
> +				block_end_pfn = block_start_pfn,
> +				block_start_pfn -= pageblock_nr_pages) {
>  		unsigned long isolated;
> -		unsigned long end_pfn;
>  
>  		/*
>  		 * This can iterate a massively long zone without finding any
> @@ -715,7 +717,7 @@ static void isolate_freepages(struct zone *zone,
>  		 */
>  		cond_resched();
>  
> -		if (!pfn_valid(pfn))
> +		if (!pfn_valid(block_start_pfn))
>  			continue;
>  
>  		/*
> @@ -725,7 +727,7 @@ static void isolate_freepages(struct zone *zone,
>  		 * i.e. it's possible that all pages within a zones range of
>  		 * pages do not belong to a single zone.
>  		 */
> -		page = pfn_to_page(pfn);
> +		page = pfn_to_page(block_start_pfn);
>  		if (page_zone(page) != zone)
>  			continue;
>  
> @@ -738,15 +740,8 @@ static void isolate_freepages(struct zone *zone,
>  			continue;
>  
>  		/* Found a block suitable for isolating free pages from */
> -		isolated = 0;
> -
> -		/*
> -		 * Take care when isolating in last pageblock of a zone which
> -		 * ends in the middle of a pageblock.
> -		 */
> -		end_pfn = min(pfn + pageblock_nr_pages, z_end_pfn);
> -		isolated = isolate_freepages_block(cc, pfn, end_pfn,
> -						   freelist, false);
> +		isolated = isolate_freepages_block(cc, block_start_pfn,
> +					block_end_pfn, freelist, false);
>  		nr_freepages += isolated;
>  
>  		/*
> @@ -754,9 +749,9 @@ static void isolate_freepages(struct zone *zone,
>  		 * looking for free pages, the search will restart here as
>  		 * page migration may have returned some pages to the allocator
>  		 */
> -		if (isolated) {
> +		if (isolated && next_free_pfn == 0) {
>  			cc->finished_update_free = true;
> -			next_free_pfn = max(next_free_pfn, pfn);
> +			next_free_pfn = block_start_pfn;
>  		}
>  	}
>  
> @@ -767,7 +762,7 @@ static void isolate_freepages(struct zone *zone,
>  	 * If we crossed the migrate scanner, we want to keep it that way
>  	 * so that compact_finished() may detect this
>  	 */
> -	if (pfn < low_pfn)
> +	if (block_start_pfn < low_pfn)
>  		next_free_pfn = cc->migrate_pfn;
>  
>  	cc->free_pfn = next_free_pfn;
> -- 
> 1.7.9.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--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 835 bytes --]