Received-SPF: pass (google.com: domain of linux-kernel+bounces-210971-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: "Huang, Ying" <ying.huang@intel.com>
To: Hugh Dickins <hughd@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,  Zi Yan <ziy@nvidia.com>,
  Baolin Wang <baolin.wang@linux.alibaba.com>,  David Hildenbrand
 <david@redhat.com>,  linux-kernel@vger.kernel.org,  linux-mm@kvack.org
Subject: Re: [PATCH hotfix] mm/migrate: fix kernel BUG at mm/compaction.c:2761!
In-Reply-To: <46c948b4-4dd8-6e03-4c7b-ce4e81cfa536@google.com> (Hugh Dickins's
	message of "Tue, 11 Jun 2024 22:06:20 -0700 (PDT)")
References: <46c948b4-4dd8-6e03-4c7b-ce4e81cfa536@google.com>
Date: Wed, 12 Jun 2024 14:19:17 +0800
Message-ID: <87msnq7key.fsf@yhuang6-desk2.ccr.corp.intel.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=ascii

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").

Otherwise, this looks good to me.  Thanks!

>  					stats->nr_thp_split += is_thp;
>  					stats->nr_split++;
>  					continue;

--
Best Regards,
Huang, Ying