I hit the VM_BUG_ON(!list_empty(&cc->migratepages)) in compact_zone();
and if DEBUG_VM were off, then pages would be lost on a local list.
Our convention is that if migrate_pages() reports complete success (0),
then the migratepages list will be empty; but if it reports an error or
some pages remaining, then its caller must putback_movable_pages().
There's a new case in which migrate_pages() has been reporting complete
success, but returning with pages left on the migratepages list: when
migrate_pages_batch() successfully split a folio on the deferred list,
but then the "Failure isn't counted" call does not dispose of them all.
Since that block is expecting the large folio to have been counted as 1
failure already, and since the return code is later adjusted to success
whenever the returned list is found empty, the simple way to fix this
safely is to count splitting the deferred folio as "a failure".
Fixes: 7262f208ca68 ("mm/migrate: split source folio if it is on deferred split list")
Signed-off-by: Hugh Dickins <[email protected]>
---
A hotfix to 6.10-rc, not needed for stable.
mm/migrate.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1654,7 +1654,12 @@ static int migrate_pages_batch(struct list_head *from,
/*
* The rare folio on the deferred split list should
- * be split now. It should not count as a failure.
+ * be split now. It should not count as a failure:
+ * but increment nr_failed because, without doing so,
+ * migrate_pages() may report success with (split but
+ * unmigrated) pages still on its fromlist; whereas it
+ * always reports success when its fromlist is empty.
+ *
* Only check it without removing it from the list.
* Since the folio can be on deferred_split_scan()
* local list and removing it can cause the local list
@@ -1669,6 +1674,7 @@ static int migrate_pages_batch(struct list_head *from,
if (nr_pages > 2 &&
!list_empty(&folio->_deferred_list)) {
if (try_split_folio(folio, split_folios) == 0) {
+ nr_failed++;
stats->nr_thp_split += is_thp;
stats->nr_split++;
continue;
--
2.35.3
Hugh Dickins <[email protected]> writes:
> I hit the VM_BUG_ON(!list_empty(&cc->migratepages)) in compact_zone();
> and if DEBUG_VM were off, then pages would be lost on a local list.
>
> Our convention is that if migrate_pages() reports complete success (0),
> then the migratepages list will be empty; but if it reports an error or
> some pages remaining, then its caller must putback_movable_pages().
>
> There's a new case in which migrate_pages() has been reporting complete
> success, but returning with pages left on the migratepages list: when
> migrate_pages_batch() successfully split a folio on the deferred list,
> but then the "Failure isn't counted" call does not dispose of them all.
>
> Since that block is expecting the large folio to have been counted as 1
> failure already, and since the return code is later adjusted to success
> whenever the returned list is found empty, the simple way to fix this
> safely is to count splitting the deferred folio as "a failure".
>
> Fixes: 7262f208ca68 ("mm/migrate: split source folio if it is on deferred split list")
> Signed-off-by: Hugh Dickins <[email protected]>
> ---
> A hotfix to 6.10-rc, not needed for stable.
>
> mm/migrate.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1654,7 +1654,12 @@ static int migrate_pages_batch(struct list_head *from,
>
> /*
> * The rare folio on the deferred split list should
> - * be split now. It should not count as a failure.
> + * be split now. It should not count as a failure:
> + * but increment nr_failed because, without doing so,
> + * migrate_pages() may report success with (split but
> + * unmigrated) pages still on its fromlist; whereas it
> + * always reports success when its fromlist is empty.
> + *
> * Only check it without removing it from the list.
> * Since the folio can be on deferred_split_scan()
> * local list and removing it can cause the local list
> @@ -1669,6 +1674,7 @@ static int migrate_pages_batch(struct list_head *from,
> if (nr_pages > 2 &&
> !list_empty(&folio->_deferred_list)) {
> if (try_split_folio(folio, split_folios) == 0) {
> + nr_failed++;
It appears better to add
stats->nr_thp_failed++;
too. Otherwise, if migrate_pages_batch() is called via migrate_pages(,
MIGRATE_ASYNC, ), nr_thp_failed will not increase. But if
migrate_pages_batch() is called via migrate_pages(, MIGRATE_SYNC*, ),
nr_thp_failed will increase in migrate_pages_sync() via
stats->nr_thp_failed += astats.nr_thp_split;
That is, they are not consistent. The issue exists since commit
7262f208ca68 ("mm/migrate: split source folio if it is on deferred split
list").
Otherwise, this looks good to me. Thanks!
> stats->nr_thp_split += is_thp;
> stats->nr_split++;
> continue;
--
Best Regards,
Huang, Ying
On Wed, 12 Jun 2024, Huang, Ying wrote:
> Hugh Dickins <[email protected]> writes:
>
> > I hit the VM_BUG_ON(!list_empty(&cc->migratepages)) in compact_zone();
> > and if DEBUG_VM were off, then pages would be lost on a local list.
> >
> > Our convention is that if migrate_pages() reports complete success (0),
> > then the migratepages list will be empty; but if it reports an error or
> > some pages remaining, then its caller must putback_movable_pages().
> >
> > There's a new case in which migrate_pages() has been reporting complete
> > success, but returning with pages left on the migratepages list: when
> > migrate_pages_batch() successfully split a folio on the deferred list,
> > but then the "Failure isn't counted" call does not dispose of them all.
> >
> > Since that block is expecting the large folio to have been counted as 1
> > failure already, and since the return code is later adjusted to success
> > whenever the returned list is found empty, the simple way to fix this
> > safely is to count splitting the deferred folio as "a failure".
> >
> > Fixes: 7262f208ca68 ("mm/migrate: split source folio if it is on deferred split list")
> > Signed-off-by: Hugh Dickins <[email protected]>
> > ---
> > A hotfix to 6.10-rc, not needed for stable.
> >
> > mm/migrate.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -1654,7 +1654,12 @@ static int migrate_pages_batch(struct list_head *from,
> >
> > /*
> > * The rare folio on the deferred split list should
> > - * be split now. It should not count as a failure.
> > + * be split now. It should not count as a failure:
> > + * but increment nr_failed because, without doing so,
> > + * migrate_pages() may report success with (split but
> > + * unmigrated) pages still on its fromlist; whereas it
> > + * always reports success when its fromlist is empty.
> > + *
> > * Only check it without removing it from the list.
> > * Since the folio can be on deferred_split_scan()
> > * local list and removing it can cause the local list
> > @@ -1669,6 +1674,7 @@ static int migrate_pages_batch(struct list_head *from,
> > if (nr_pages > 2 &&
> > !list_empty(&folio->_deferred_list)) {
> > if (try_split_folio(folio, split_folios) == 0) {
> > + nr_failed++;
>
> It appears better to add
>
> stats->nr_thp_failed++;
>
> too. Otherwise, if migrate_pages_batch() is called via migrate_pages(,
> MIGRATE_ASYNC, ), nr_thp_failed will not increase. But if
> migrate_pages_batch() is called via migrate_pages(, MIGRATE_SYNC*, ),
> nr_thp_failed will increase in migrate_pages_sync() via
>
> stats->nr_thp_failed += astats.nr_thp_split;
>
> That is, they are not consistent. The issue exists since commit
> 7262f208ca68 ("mm/migrate: split source folio if it is on deferred split
> list").
Sorry, I'll have to let you take over and send your own patch instead
of mine - thanks. Those stats, and any attempt at consistency there,
is way beyond me! I thought consistency was impossible, unless all
the numbers were changed to order-0 counts.
Hugh
>
> Otherwise, this looks good to me. Thanks!
>
> > stats->nr_thp_split += is_thp;
> > stats->nr_split++;
> > continue;
>
> --
> Best Regards,
> Huang, Ying
On Wed, 12 Jun 2024 00:11:44 -0700 (PDT) Hugh Dickins <[email protected]> wrote:
> > It appears better to add
> >
> > stats->nr_thp_failed++;
> >
> > too. Otherwise, if migrate_pages_batch() is called via migrate_pages(,
> > MIGRATE_ASYNC, ), nr_thp_failed will not increase. But if
> > migrate_pages_batch() is called via migrate_pages(, MIGRATE_SYNC*, ),
> > nr_thp_failed will increase in migrate_pages_sync() via
> >
> > stats->nr_thp_failed += astats.nr_thp_split;
> >
> > That is, they are not consistent. The issue exists since commit
> > 7262f208ca68 ("mm/migrate: split source folio if it is on deferred split
> > list").
>
> Sorry, I'll have to let you take over and send your own patch instead
> of mine - thanks. Those stats, and any attempt at consistency there,
> is way beyond me! I thought consistency was impossible, unless all
> the numbers were changed to order-0 counts.
I'll add this patch as-is for now.