Re: [PATCH hotfix] mm/migrate: fix kernel BUG at mm/compaction.c:2761!

[Hugh Dickins <hughd@google.com>]

On Wed, 12 Jun 2024, Huang, Ying wrote:
> Hugh Dickins <hughd@google.com> 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 <hughd@google.com>
> > ---
> > 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