[PATCH v3 0/8] Fixes and cleanups to compaction

[PATCH v3 0/8] Fixes and cleanups to compaction

[Kemeng Shi]

Hi all, this series contains some random cleanups and fixes to
compation. Details can be found in respective patches.

v2->v3:
-Improve comment in patch 4
-Collect ACK and RVB from David and Baolin.
-Rebase code on current mm-unstable

v1->v2:
-Add comment to skip_offline_sections_reverse in patch 1
-Add impact in git message in patch 2
-Correct comment to fast_find_block instead of remove fast_find_block
in patch 4
-Collect RVB from David and Baolin.

Kemeng Shi (8):
  mm/compaction: avoid missing last page block in section after skip
    offline sections
  mm/compaction: correct last_migrated_pfn update in compact_zone
  mm/compaction: skip page block marked skip in
    isolate_migratepages_block
  mm/compaction: correct comment of fast_find_migrateblock in
    isolate_migratepages
  mm/compaction: correct comment of cached migrate pfn update
  mm/compaction: correct comment to complete migration failure
  mm/compaction: remove unnecessary return for void function
  mm/compaction: only set skip flag if cc->no_set_skip_hint is false

 mm/compaction.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

-- 
2.30.0

[PATCH v3 1/8] mm/compaction: avoid missing last page block in section after skip offline sections

[Kemeng Shi]

skip_offline_sections_reverse will return the last pfn in found online
section. Then we set block_start_pfn to start of page block which
contains the last pfn in section. Then we continue, move one page
block forward and ignore the last page block in the online section.
Make block_start_pfn point to first page block after online section to fix
this:
1. make skip_offline_sections_reverse return end pfn of online section,
i.e. pfn of page block after online section.
2. assign block_start_pfn with next_pfn.

Fixes: f63224525309 ("mm: compaction: skip the memory hole rapidly when isolating free pages")
Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Acked-by: David Hildenbrand <david@redhat.com>

---
 mm/compaction.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 006fc35bffa1..a21b834c5898 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -250,6 +250,11 @@ static unsigned long skip_offline_sections(unsigned long start_pfn)
 	return 0;
 }
 
+/*
+ * If the PFN falls into an offline section, return the end PFN of the
+ * next online section in reverse. If the PFN falls into an online section
+ * or if there is no next online section in reverse, return 0.
+ */
 static unsigned long skip_offline_sections_reverse(unsigned long start_pfn)
 {
 	unsigned long start_nr = pfn_to_section_nr(start_pfn);
@@ -259,7 +264,7 @@ static unsigned long skip_offline_sections_reverse(unsigned long start_pfn)
 
 	while (start_nr-- > 0) {
 		if (online_section_nr(start_nr))
-			return section_nr_to_pfn(start_nr) + PAGES_PER_SECTION - 1;
+			return section_nr_to_pfn(start_nr) + PAGES_PER_SECTION;
 	}
 
 	return 0;
@@ -1690,8 +1695,7 @@ static void isolate_freepages(struct compact_control *cc)
 
 			next_pfn = skip_offline_sections_reverse(block_start_pfn);
 			if (next_pfn)
-				block_start_pfn = max(pageblock_start_pfn(next_pfn),
-						      low_pfn);
+				block_start_pfn = max(next_pfn, low_pfn);
 
 			continue;
 		}
-- 
2.30.0

Re: [PATCH v3 1/8] mm/compaction: avoid missing last page block in section after skip offline sections

[Baolin Wang]

On 8/4/2023 7:04 PM, Kemeng Shi wrote:
> skip_offline_sections_reverse will return the last pfn in found online
> section. Then we set block_start_pfn to start of page block which
> contains the last pfn in section. Then we continue, move one page
> block forward and ignore the last page block in the online section.
> Make block_start_pfn point to first page block after online section to fix
> this:
> 1. make skip_offline_sections_reverse return end pfn of online section,
> i.e. pfn of page block after online section.
> 2. assign block_start_pfn with next_pfn.
> 
> Fixes: f63224525309 ("mm: compaction: skip the memory hole rapidly when isolating free pages")

Again, the Fixes tag is not stable. Please consider squashing it into 
the original commit or hold this patch until the original patch is 
merged into mm-stable branch.

Andrew,
What do you prefer? Thanks.

> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> 
> ---
>   mm/compaction.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 006fc35bffa1..a21b834c5898 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -250,6 +250,11 @@ static unsigned long skip_offline_sections(unsigned long start_pfn)
>   	return 0;
>   }
>   
> +/*
> + * If the PFN falls into an offline section, return the end PFN of the
> + * next online section in reverse. If the PFN falls into an online section
> + * or if there is no next online section in reverse, return 0.
> + */
>   static unsigned long skip_offline_sections_reverse(unsigned long start_pfn)
>   {
>   	unsigned long start_nr = pfn_to_section_nr(start_pfn);
> @@ -259,7 +264,7 @@ static unsigned long skip_offline_sections_reverse(unsigned long start_pfn)
>   
>   	while (start_nr-- > 0) {
>   		if (online_section_nr(start_nr))
> -			return section_nr_to_pfn(start_nr) + PAGES_PER_SECTION - 1;
> +			return section_nr_to_pfn(start_nr) + PAGES_PER_SECTION;
>   	}
>   
>   	return 0;
> @@ -1690,8 +1695,7 @@ static void isolate_freepages(struct compact_control *cc)
>   
>   			next_pfn = skip_offline_sections_reverse(block_start_pfn);
>   			if (next_pfn)
> -				block_start_pfn = max(pageblock_start_pfn(next_pfn),
> -						      low_pfn);
> +				block_start_pfn = max(next_pfn, low_pfn);
>   
>   			continue;
>   		}

[PATCH v3 2/8] mm/compaction: correct last_migrated_pfn update in compact_zone

[Kemeng Shi]

We record start pfn of last isolated page block with last_migrated_pfn. And
then:
1. We check if we mark the page block skip for exclusive access in
isolate_migratepages_block by test if next migrate pfn is still in last
isolated page block. If so, we will set finish_pageblock to do the rescan.
2. We check if a full cc->order block is scanned by test if last scan range
passes the cc->order block boundary. If so, we flush the pages were freed.

We treat cc->migrate_pfn before isolate_migratepages as the start pfn of
last isolated page range. However, we always align migrate_pfn to page block
or move to another page block in fast_find_migrateblock or in linearly scan
forward in isolate_migratepages before do page isolation in
isolate_migratepages_block.

Update last_migrated_pfn with pageblock_start_pfn(cc->migrate_pfn - 1)
after scan to correctly set start pfn of last isolated page range. To
avoid that:
1. Miss a rescan with finish_pageblock set as last_migrate_pfn does not
point to right pageblock and the migrate will not be in pageblock of
last_migrate_pfn as it should be.
2. Wrongly issue flush by test cc->order block boundary with wrong
last_migrate_pfn.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/compaction.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index a21b834c5898..bdb03f38b7f7 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2509,7 +2509,8 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
 			goto check_drain;
 		case ISOLATE_SUCCESS:
 			update_cached = false;
-			last_migrated_pfn = iteration_start_pfn;
+			last_migrated_pfn = max(cc->zone->zone_start_pfn,
+				pageblock_start_pfn(cc->migrate_pfn - 1));
 		}
 
 		err = migrate_pages(&cc->migratepages, compaction_alloc,
-- 
2.30.0

[PATCH v3 3/8] mm/compaction: skip page block marked skip in isolate_migratepages_block

[Kemeng Shi]

Move migrate_pfn to page block end when block is marked skip to avoid
unnecessary scan retry of that block from upper caller.
For example, compact_zone may wrongly rescan skip page block with
finish_pageblock set as following:
1. cc->migrate point to the start of page block
2. compact_zone record last_migrated_pfn to cc->migrate
3. compact_zone->isolate_migratepages->isolate_migratepages_block tries to
scan the block. The low_pfn maybe moved forward to middle of block because
of free pages at beginning of block.
4. we find first lru page could be isolated but block was exclusive marked
skip.
5. abort isolate_migratepages_block and make cc->migrate_pfn point to
found lru page at middle of block.
6. compact_zone find cc->migrate_pfn and last_migrated_pfn are in the same
block and wrongly rescan the block with finish_pageblock set.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/compaction.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/compaction.c b/mm/compaction.c
index bdb03f38b7f7..9a2f0bd5394a 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1141,6 +1141,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 				skip_updated = true;
 				if (test_and_set_skip(cc, valid_page) &&
 				    !cc->finish_pageblock) {
+					low_pfn = end_pfn;
 					goto isolate_abort;
 				}
 			}
-- 
2.30.0

[PATCH v3 4/8] mm/compaction: correct comment of fast_find_migrateblock in isolate_migratepages

[Kemeng Shi]

After 90ed667c03fe5 ("Revert "Revert "mm/compaction: fix set skip in
fast_find_migrateblock"""), we remove skip set in fast_find_migrateblock.
Correct comment that fast_find_block is used to avoid isolation_suitable
check for pageblock returned from fast_find_migrateblock because
fast_find_migrateblock will mark found pageblock skipped.
Instead, comment that fast_find_block is used to avoid a redundant check
of fast found pageblock which is already checked skip flag inside
fast_find_migrateblock.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 mm/compaction.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 9a2f0bd5394a..4bf2dc936add 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1988,9 +1988,9 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
 		block_start_pfn = cc->zone->zone_start_pfn;
 
 	/*
-	 * fast_find_migrateblock marks a pageblock skipped so to avoid
-	 * the isolation_suitable check below, check whether the fast
-	 * search was successful.
+	 * fast_find_migrateblock() has already ensured the pageblock is not
+	 * set with a skipped flag, so to avoid the isolation_suitable check
+	 * below again, check whether the fast search was successful.
 	 */
 	fast_find_block = low_pfn != cc->migrate_pfn && !cc->fast_search_fail;
 
-- 
2.30.0

Re: [PATCH v3 4/8] mm/compaction: correct comment of fast_find_migrateblock in isolate_migratepages

[Baolin Wang]

On 8/4/2023 7:04 PM, Kemeng Shi wrote:
> After 90ed667c03fe5 ("Revert "Revert "mm/compaction: fix set skip in
> fast_find_migrateblock"""), we remove skip set in fast_find_migrateblock.
> Correct comment that fast_find_block is used to avoid isolation_suitable
> check for pageblock returned from fast_find_migrateblock because
> fast_find_migrateblock will mark found pageblock skipped.
> Instead, comment that fast_find_block is used to avoid a redundant check
> of fast found pageblock which is already checked skip flag inside
> fast_find_migrateblock.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

LGTM.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

> ---
>   mm/compaction.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 9a2f0bd5394a..4bf2dc936add 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1988,9 +1988,9 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
>   		block_start_pfn = cc->zone->zone_start_pfn;
>   
>   	/*
> -	 * fast_find_migrateblock marks a pageblock skipped so to avoid
> -	 * the isolation_suitable check below, check whether the fast
> -	 * search was successful.
> +	 * fast_find_migrateblock() has already ensured the pageblock is not
> +	 * set with a skipped flag, so to avoid the isolation_suitable check
> +	 * below again, check whether the fast search was successful.
>   	 */
>   	fast_find_block = low_pfn != cc->migrate_pfn && !cc->fast_search_fail;
>   

[PATCH v3 5/8] mm/compaction: correct comment of cached migrate pfn update

[Kemeng Shi]

Commit e380bebe47715 ("mm, compaction: keep migration source private to
a single compaction instance") moved update of async and sync
compact_cached_migrate_pfn from update_pageblock_skip to
update_cached_migrate but left the comment behind.
Move the relevant comment to correct this.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/compaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 4bf2dc936add..fda61dc40f93 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -469,6 +469,7 @@ static void update_cached_migrate(struct compact_control *cc, unsigned long pfn)
 
 	pfn = pageblock_end_pfn(pfn);
 
+	/* Update where async and sync compaction should restart */
 	if (pfn > zone->compact_cached_migrate_pfn[0])
 		zone->compact_cached_migrate_pfn[0] = pfn;
 	if (cc->mode != MIGRATE_ASYNC &&
@@ -490,7 +491,6 @@ static void update_pageblock_skip(struct compact_control *cc,
 
 	set_pageblock_skip(page);
 
-	/* Update where async and sync compaction should restart */
 	if (pfn < zone->compact_cached_free_pfn)
 		zone->compact_cached_free_pfn = pfn;
 }
-- 
2.30.0

[PATCH v3 6/8] mm/compaction: correct comment to complete migration failure

[Kemeng Shi]

Commit cfccd2e63e7e0 ("mm, compaction: finish pageblocks on complete
migration failure") convert cc->order aligned check to page block
order aligned check. Correct comment relevant with it.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 mm/compaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index fda61dc40f93..9022209b64c3 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2534,7 +2534,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
 			}
 			/*
 			 * If an ASYNC or SYNC_LIGHT fails to migrate a page
-			 * within the current order-aligned block and
+			 * within the pageblock_order-aligned block and
 			 * fast_find_migrateblock may be used then scan the
 			 * remainder of the pageblock. This will mark the
 			 * pageblock "skip" to avoid rescanning in the near
-- 
2.30.0

[PATCH v3 7/8] mm/compaction: remove unnecessary return for void function

[Kemeng Shi]

Remove unnecessary return for void function

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/compaction.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 9022209b64c3..81be1d0b9622 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1445,8 +1445,6 @@ fast_isolate_around(struct compact_control *cc, unsigned long pfn)
 	/* Skip this pageblock in the future as it's full or nearly full */
 	if (start_pfn == end_pfn)
 		set_pageblock_skip(page);
-
-	return;
 }
 
 /* Search orders in round-robin fashion */
@@ -2900,7 +2898,7 @@ int compaction_register_node(struct node *node)
 
 void compaction_unregister_node(struct node *node)
 {
-	return device_remove_file(&node->dev, &dev_attr_compact);
+	device_remove_file(&node->dev, &dev_attr_compact);
 }
 #endif /* CONFIG_SYSFS && CONFIG_NUMA */
 
-- 
2.30.0

[PATCH v3 8/8] mm/compaction: only set skip flag if cc->no_set_skip_hint is false

[Kemeng Shi]

Keep the same logic as update_pageblock_skip, only set skip if
no_set_skip_hint is false which is more reasonable.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 mm/compaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 81be1d0b9622..ea61922a1619 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1443,7 +1443,7 @@ fast_isolate_around(struct compact_control *cc, unsigned long pfn)
 	isolate_freepages_block(cc, &start_pfn, end_pfn, &cc->freepages, 1, false);
 
 	/* Skip this pageblock in the future as it's full or nearly full */
-	if (start_pfn == end_pfn)
+	if (start_pfn == end_pfn && !cc->no_set_skip_hint)
 		set_pageblock_skip(page);
 }
 
-- 
2.30.0