[PATCH] mm/migrate: Putback split folios when numa hint migration fails

[PATCH] mm/migrate: Putback split folios when numa hint migration fails

[Peter Xu]

This issue is not from any report yet, but by code observation only.

This is yet another fix besides Hugh's patch [1] but on relevant code path,
where eager split of folio can happen if the folio is already on deferred
list during a folio migration.

Here the issue is NUMA path (migrate_misplaced_folio()) may start to
encounter such folio split now even with MR_NUMA_MISPLACED hint applied.
Then when migrate_pages() didn't migrate all the folios, it's possible the
split small folios be put onto the list instead of the original folio.
Then putting back only the head page won't be enough.

Fix it by putting back all the folios on the list.

[1] https://lore.kernel.org/all/46c948b4-4dd8-6e03-4c7b-ce4e81cfa536@google.com/

Cc: Zi Yan <ziy@nvidia.com>
Cc: Yang Shi <shy828301@gmail.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: Huang Ying <ying.huang@intel.com>
Cc: David Hildenbrand <david@redhat.com>
Fixes: 7262f208ca68 ("mm/migrate: split source folio if it is on deferred split list")
Signed-off-by: Peter Xu <peterx@redhat.com>
---

Don't need to copy stable if this can still hit 6.10..  Only smoke tested.
---
 mm/migrate.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index e10d2445fbd8..20da2595527a 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2615,14 +2615,8 @@ int migrate_misplaced_folio(struct folio *folio, struct vm_area_struct *vma,
 	nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_folio,
 				     NULL, node, MIGRATE_ASYNC,
 				     MR_NUMA_MISPLACED, &nr_succeeded);
-	if (nr_remaining) {
-		if (!list_empty(&migratepages)) {
-			list_del(&folio->lru);
-			node_stat_mod_folio(folio, NR_ISOLATED_ANON +
-					folio_is_file_lru(folio), -nr_pages);
-			folio_putback_lru(folio);
-		}
-	}
+	if (nr_remaining && !list_empty(&migratepages))
+		putback_movable_pages(&migratepages);
 	if (nr_succeeded) {
 		count_vm_numa_events(NUMA_PAGE_MIGRATE, nr_succeeded);
 		if (!node_is_toptier(folio_nid(folio)) && node_is_toptier(node))
-- 
2.45.0

Re: [PATCH] mm/migrate: Putback split folios when numa hint migration fails

[Zi Yan]

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

On Mon Jul 8, 2024 at 5:55 PM EDT, Peter Xu wrote:
> This issue is not from any report yet, but by code observation only.
>
> This is yet another fix besides Hugh's patch [1] but on relevant code path,
> where eager split of folio can happen if the folio is already on deferred
> list during a folio migration.
>
> Here the issue is NUMA path (migrate_misplaced_folio()) may start to
> encounter such folio split now even with MR_NUMA_MISPLACED hint applied.
> Then when migrate_pages() didn't migrate all the folios, it's possible the
> split small folios be put onto the list instead of the original folio.
> Then putting back only the head page won't be enough.
>
> Fix it by putting back all the folios on the list.
>
> [1] https://lore.kernel.org/all/46c948b4-4dd8-6e03-4c7b-ce4e81cfa536@google.com/
>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Yang Shi <shy828301@gmail.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
> Cc: Huang Ying <ying.huang@intel.com>
> Cc: David Hildenbrand <david@redhat.com>
> Fixes: 7262f208ca68 ("mm/migrate: split source folio if it is on deferred split list")
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>
> Don't need to copy stable if this can still hit 6.10..  Only smoke tested.
> ---
>  mm/migrate.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)

LGTM. Thanks. Reviewed-by: Zi Yan <ziy@nvidia.com>


-- 
Best Regards,
Yan, Zi


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

Re: [PATCH] mm/migrate: Putback split folios when numa hint migration fails

[Andrew Morton]

On Mon,  8 Jul 2024 17:55:37 -0400 Peter Xu <peterx@redhat.com> wrote:

> This issue is not from any report yet, but by code observation only.
> 
> This is yet another fix besides Hugh's patch [1] but on relevant code path,
> where eager split of folio can happen if the folio is already on deferred
> list during a folio migration.
> 
> Here the issue is NUMA path (migrate_misplaced_folio()) may start to
> encounter such folio split now even with MR_NUMA_MISPLACED hint applied.
> Then when migrate_pages() didn't migrate all the folios, it's possible the
> split small folios be put onto the list instead of the original folio.
> Then putting back only the head page won't be enough.
> 
> Fix it by putting back all the folios on the list.

mm/migrate.c: In function 'migrate_misplaced_folio':
mm/migrate.c:2624:13: error: unused variable 'nr_pages' [-Werror=unused-variable]
 2624 |         int nr_pages = folio_nr_pages(folio);
      |             ^~~~~~~~

Worrisome.  Which kernel version was this tested against?

> Don't need to copy stable if this can still hit 6.10..  Only smoke tested.

Also worrisome.  Are we to take an only-smoke-tested patch which
doesn't apply to mainline and which doesn't compile on mm-unstable into
mainline based on "only smoke tested"?

Re: [PATCH] mm/migrate: Putback split folios when numa hint migration fails

[Peter Xu]

On Mon, Jul 08, 2024 at 04:04:07PM -0700, Andrew Morton wrote:
> On Mon,  8 Jul 2024 17:55:37 -0400 Peter Xu <peterx@redhat.com> wrote:
> 
> > This issue is not from any report yet, but by code observation only.
> > 
> > This is yet another fix besides Hugh's patch [1] but on relevant code path,
> > where eager split of folio can happen if the folio is already on deferred
> > list during a folio migration.
> > 
> > Here the issue is NUMA path (migrate_misplaced_folio()) may start to
> > encounter such folio split now even with MR_NUMA_MISPLACED hint applied.
> > Then when migrate_pages() didn't migrate all the folios, it's possible the
> > split small folios be put onto the list instead of the original folio.
> > Then putting back only the head page won't be enough.
> > 
> > Fix it by putting back all the folios on the list.
> 
> mm/migrate.c: In function 'migrate_misplaced_folio':
> mm/migrate.c:2624:13: error: unused variable 'nr_pages' [-Werror=unused-variable]
>  2624 |         int nr_pages = folio_nr_pages(folio);
>       |             ^~~~~~~~
> 
> Worrisome.  Which kernel version was this tested against?

mm-unstable (and on top of a few of my other totally irrelevant patches),
and I thought it also applied to mm-stable.

Totally missed this warning when still with WERROR=off locally when
building against this patch, my apologies.

> 
> > Don't need to copy stable if this can still hit 6.10..  Only smoke tested.
> 
> Also worrisome.  Are we to take an only-smoke-tested patch which
> doesn't apply to mainline and which doesn't compile on mm-unstable into
> mainline based on "only smoke tested"?

Hmm so it doesn't apply to mainline..  For the smoke test part, I was not
confident to reproduce it, and I just stumbled over it when looking at the
real BUG_ON we hit.  I thought it might be a good idea to send a patch
before everyone forgets about it.

I think it is easily overlooked probably because the issue wasn't
obvious. IIUC the sympton of hitting it should be that we leak a few of
those tail pages even if they're freed in the future from the mappings.  I
am not sure how much an issue with keep being !lru for them besides the
leaked refcounts, perhaps only vmscan won't see them.  After all, all these
is based on the chance of hitting this case and it should be rare.  I don't
think I know well enough to say.

Considering that nobody yelled after rc5 until now, and this is only
something I observed when looking at the more severe issue Hugh fixed..
maybe we should target this for next release, then stablize it and wait for
a backport to 6.10?

Thanks,

-- 
Peter Xu

Re: [PATCH] mm/migrate: Putback split folios when numa hint migration fails

[Andrew Morton]

On Tue, 9 Jul 2024 10:47:19 -0400 Peter Xu <peterx@redhat.com> wrote:

> Considering that nobody yelled after rc5 until now, and this is only
> something I observed when looking at the more severe issue Hugh fixed..
> maybe we should target this for next release, then stablize it and wait for
> a backport to 6.10?

Sure, let's do that.

Re: [PATCH] mm/migrate: Putback split folios when numa hint migration fails

[Baolin Wang]

On 2024/7/9 05:55, Peter Xu wrote:
> This issue is not from any report yet, but by code observation only.
> 
> This is yet another fix besides Hugh's patch [1] but on relevant code path,
> where eager split of folio can happen if the folio is already on deferred
> list during a folio migration.
> 
> Here the issue is NUMA path (migrate_misplaced_folio()) may start to
> encounter such folio split now even with MR_NUMA_MISPLACED hint applied.
> Then when migrate_pages() didn't migrate all the folios, it's possible the
> split small folios be put onto the list instead of the original folio.
> Then putting back only the head page won't be enough.
> 
> Fix it by putting back all the folios on the list.
> 
> [1] https://lore.kernel.org/all/46c948b4-4dd8-6e03-4c7b-ce4e81cfa536@google.com/
> 
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Yang Shi <shy828301@gmail.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
> Cc: Huang Ying <ying.huang@intel.com>
> Cc: David Hildenbrand <david@redhat.com>
> Fixes: 7262f208ca68 ("mm/migrate: split source folio if it is on deferred split list")
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---

Good catch. With fixing the building issue pointed by Andrew, please 
feel free to add:
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

> 
> Don't need to copy stable if this can still hit 6.10..  Only smoke tested.
> ---
>   mm/migrate.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index e10d2445fbd8..20da2595527a 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2615,14 +2615,8 @@ int migrate_misplaced_folio(struct folio *folio, struct vm_area_struct *vma,
>   	nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_folio,
>   				     NULL, node, MIGRATE_ASYNC,
>   				     MR_NUMA_MISPLACED, &nr_succeeded);
> -	if (nr_remaining) {
> -		if (!list_empty(&migratepages)) {
> -			list_del(&folio->lru);
> -			node_stat_mod_folio(folio, NR_ISOLATED_ANON +
> -					folio_is_file_lru(folio), -nr_pages);
> -			folio_putback_lru(folio);
> -		}
> -	}
> +	if (nr_remaining && !list_empty(&migratepages))

Nit: you can drop the '!list_empty(&migratepages)' validation, since 
putback_movable_pages() can handle this unusual case.

> +		putback_movable_pages(&migratepages);
>   	if (nr_succeeded) {
>   		count_vm_numa_events(NUMA_PAGE_MIGRATE, nr_succeeded);
>   		if (!node_is_toptier(folio_nid(folio)) && node_is_toptier(node))

Re: [PATCH] mm/migrate: Putback split folios when numa hint migration fails

[Peter Xu]

On Tue, Jul 09, 2024 at 09:48:54AM +0800, Baolin Wang wrote:
> 
> 
> On 2024/7/9 05:55, Peter Xu wrote:
> > This issue is not from any report yet, but by code observation only.
> > 
> > This is yet another fix besides Hugh's patch [1] but on relevant code path,
> > where eager split of folio can happen if the folio is already on deferred
> > list during a folio migration.
> > 
> > Here the issue is NUMA path (migrate_misplaced_folio()) may start to
> > encounter such folio split now even with MR_NUMA_MISPLACED hint applied.
> > Then when migrate_pages() didn't migrate all the folios, it's possible the
> > split small folios be put onto the list instead of the original folio.
> > Then putting back only the head page won't be enough.
> > 
> > Fix it by putting back all the folios on the list.
> > 
> > [1] https://lore.kernel.org/all/46c948b4-4dd8-6e03-4c7b-ce4e81cfa536@google.com/
> > 
> > Cc: Zi Yan <ziy@nvidia.com>
> > Cc: Yang Shi <shy828301@gmail.com>
> > Cc: Hugh Dickins <hughd@google.com>
> > Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
> > Cc: Huang Ying <ying.huang@intel.com>
> > Cc: David Hildenbrand <david@redhat.com>
> > Fixes: 7262f208ca68 ("mm/migrate: split source folio if it is on deferred split list")
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> 
> Good catch. With fixing the building issue pointed by Andrew, please feel
> free to add:
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> 
> > 
> > Don't need to copy stable if this can still hit 6.10..  Only smoke tested.
> > ---
> >   mm/migrate.c | 10 ++--------
> >   1 file changed, 2 insertions(+), 8 deletions(-)
> > 
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index e10d2445fbd8..20da2595527a 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -2615,14 +2615,8 @@ int migrate_misplaced_folio(struct folio *folio, struct vm_area_struct *vma,
> >   	nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_folio,
> >   				     NULL, node, MIGRATE_ASYNC,
> >   				     MR_NUMA_MISPLACED, &nr_succeeded);
> > -	if (nr_remaining) {
> > -		if (!list_empty(&migratepages)) {
> > -			list_del(&folio->lru);
> > -			node_stat_mod_folio(folio, NR_ISOLATED_ANON +
> > -					folio_is_file_lru(folio), -nr_pages);
> > -			folio_putback_lru(folio);
> > -		}
> > -	}
> > +	if (nr_remaining && !list_empty(&migratepages))
> 
> Nit: you can drop the '!list_empty(&migratepages)' validation, since
> putback_movable_pages() can handle this unusual case.

Sure, considering that it should normally be !empty when the first check
passed.

Though to make this simple for now, I assume we can keep what has been
queued in Andrew's tree.  It isn't so bad either to double check the list
to avoid a function call if possible, I think.

Thanks for the comment,

-- 
Peter Xu