From: Zi Yan <[email protected]>
I am adding support for >0 folio compaction and find the
VM_BUG_ON(!list_empty(&cc->migratepages)) in compact_zone() can be triggered
when a large folio is tried to be migrated, split, and its split base
pages are not migrated. The fix is in patch 1. In the upstream tree,
large folios can be migrated via move_pages() syscall and there is no
VM_BUG_ON in do_move_pages_to_node() but pages left in the source page
list will not be returned to LRU lists. This is from my code inspection
and I have not reproduced it on upstream tree yet.
In addition, I wonder if we need to add any large folio migration stats in
addition to existing THP migration stats, at least for large folio split
stats. Otherwise, large folio migrations seem to happen without any
notice.
Thanks.
Zi Yan (1):
mm/migrate: correct nr_failed in migrate_pages_sync()
mm/migrate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
2.42.0
From: Zi Yan <[email protected]>
nr_failed was missing the rc value from migrate_pages_batch() and can
cause a mismatch between migrate_pages() return value and the number of
not migrated pages, i.e., when the return value of migrate_pages() is 0,
there are still pages left in the from page list. It will happen when a
non-PMD THP large folio fails to migrate due to -ENOMEM and is split
successfully but not all the split pages are not migrated,
migrate_pages_batch() would return non-zero, but astats.nr_thp_split = 0.
nr_failed would be 0 and returned to the caller of migrate_pages(), but
the not migrated pages are left in the from page list without being added
back to LRU lists.
Fixes: 2ef7dbb26990 ("migrate_pages: try migrate in batch asynchronously firstly")
Signed-off-by: Zi Yan <[email protected]>
---
mm/migrate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index c602bf6dec97..5348827bd958 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1834,7 +1834,7 @@ static int migrate_pages_sync(struct list_head *from, new_folio_t get_new_folio,
return rc;
}
stats->nr_thp_failed += astats.nr_thp_split;
- nr_failed += astats.nr_thp_split;
+ nr_failed += rc + astats.nr_thp_split;
/*
* Fall back to migrate all failed folios one by one synchronously. All
* failed folios except split THPs will be retried, so their failure
--
2.42.0
Zi Yan <[email protected]> writes:
> From: Zi Yan <[email protected]>
>
> nr_failed was missing the rc value from migrate_pages_batch() and can
> cause a mismatch between migrate_pages() return value and the number of
> not migrated pages, i.e., when the return value of migrate_pages() is 0,
> there are still pages left in the from page list. It will happen when a
> non-PMD THP large folio fails to migrate due to -ENOMEM and is split
> successfully but not all the split pages are not migrated,
> migrate_pages_batch() would return non-zero, but astats.nr_thp_split = 0.
> nr_failed would be 0 and returned to the caller of migrate_pages(), but
> the not migrated pages are left in the from page list without being added
> back to LRU lists.
>
> Fixes: 2ef7dbb26990 ("migrate_pages: try migrate in batch asynchronously firstly")
> Signed-off-by: Zi Yan <[email protected]>
> ---
> mm/migrate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index c602bf6dec97..5348827bd958 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1834,7 +1834,7 @@ static int migrate_pages_sync(struct list_head *from, new_folio_t get_new_folio,
> return rc;
> }
> stats->nr_thp_failed += astats.nr_thp_split;
> - nr_failed += astats.nr_thp_split;
> + nr_failed += rc + astats.nr_thp_split;
> /*
> * Fall back to migrate all failed folios one by one synchronously. All
> * failed folios except split THPs will be retried, so their failure
I don't think this is a correct fix. The failed folios will be retried
in the following synchronous migration below.
To fix the issue, we should track nr_split for all large folios (not
only THP), then use
nr_failed += astats.nr_split;
--
Best Regards,
Huang, Ying
On 9 Oct 2023, at 23:36, Huang, Ying wrote:
> Zi Yan <[email protected]> writes:
>
>> From: Zi Yan <[email protected]>
>>
>> nr_failed was missing the rc value from migrate_pages_batch() and can
>> cause a mismatch between migrate_pages() return value and the number of
>> not migrated pages, i.e., when the return value of migrate_pages() is 0,
>> there are still pages left in the from page list. It will happen when a
>> non-PMD THP large folio fails to migrate due to -ENOMEM and is split
>> successfully but not all the split pages are not migrated,
>> migrate_pages_batch() would return non-zero, but astats.nr_thp_split = 0.
>> nr_failed would be 0 and returned to the caller of migrate_pages(), but
>> the not migrated pages are left in the from page list without being added
>> back to LRU lists.
>>
>> Fixes: 2ef7dbb26990 ("migrate_pages: try migrate in batch asynchronously firstly")
>> Signed-off-by: Zi Yan <[email protected]>
>> ---
>> mm/migrate.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index c602bf6dec97..5348827bd958 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1834,7 +1834,7 @@ static int migrate_pages_sync(struct list_head *from, new_folio_t get_new_folio,
>> return rc;
>> }
>> stats->nr_thp_failed += astats.nr_thp_split;
>> - nr_failed += astats.nr_thp_split;
>> + nr_failed += rc + astats.nr_thp_split;
>> /*
>> * Fall back to migrate all failed folios one by one synchronously. All
>> * failed folios except split THPs will be retried, so their failure
>
> I don't think this is a correct fix. The failed folios will be retried
> in the following synchronous migration below.
>
> To fix the issue, we should track nr_split for all large folios (not
> only THP), then use
>
> nr_failed += astats.nr_split;
You are suggesting a new stats "nr_split" in addition to nr_thp_split? And
nr_split includes nr_thp_split?
--
Best Regards,
Yan, Zi