Re: [PATCH 2/4] mm/huge_memory: add page->private check back in __split_folio_to_order()

[Lance Yang <lance.yang@linux.dev>]

On Wed, Jul 01, 2026 at 10:56:47AM +0200, David Hildenbrand (Arm) wrote:
>On 6/29/26 16:39, Vlastimil Babka (SUSE) wrote:
>> On 6/29/26 04:56, Zi Yan wrote:
>
>s/add/readd/
>
>>> page->private should not be set in tail pages. Commit 4265d67e405a
>>> ("mm/migrate_device: add THP splitting during migration") removed it
>>> without a proper reason. Add it back.
>
>You can link to the discussion where we clarified that this was not intentional.

Probably this one?

https://lore.kernel.org/all/13f3fcda-7328-4aa5-afc6-75a294a82b2a@nvidia.com/

>>>
>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>> ---
>>>  mm/huge_memory.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>> index 2bccb0a53a0a..037d67fbec6e 100644
>>> --- a/mm/huge_memory.c
>>> +++ b/mm/huge_memory.c
>>> @@ -3594,6 +3594,16 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>>>  		new_folio->mapping = folio->mapping;
>>>  		new_folio->index = folio->index + i;
>>>  
>>> +		/*
>>> +		 * page->private should not be set in tail pages. Fix up and warn once
>>> +		 * if private is unexpectedly set. Do it before swap.val assignment
>>> +		 * since private overlaps with swap.val.
>>> +		 */
>>> +		if (unlikely(new_folio->private)) {
>>> +			VM_WARN_ON_ONCE_PAGE(true, new_head);
>>> +			new_folio->private = NULL;
>>> +		}
>> 
>> The unconditional warning means this is not expected to happen. In that case
>> it's odd to check and fixup always, but only warn with CONFIG_DEBUG_VM.
>> 
>> If we are reasonably sure the current code is OK, and only want to catch new
>> mistakes in development, we could just VM_WARN_ON_ONCE_PAGE() without fixup.
>> 
>> If we are paranoid, leave it as it is, but drop the "VM_" ?
>
>Agreed, either "if (WARN_ON_ONCE(new_folio->private))" + fixup, or no fixup.
>
>I'd prefer VM_WARN_ON_ONCE_PAGE().
>
>(I was only able to trigger this once while testing my own patches)

Looking at the history a bit, that check has been around for a while :)

b653db77350c ("mm: Clear page->private when splitting or migrating a
page") first started clearing tail page->private during THP split:

static void __split_huge_page_tail(struct page *head, int tail,
		struct lruvec *lruvec, struct list_head *list)
{
	struct page *page_tail = head + tail;

	...
	page_tail->private = 0;
	...
}

That was too broad for THP swapcache, tail page->private still carried
swap_entry_t there ...

71e2d666ef85 ("mm/huge_memory: do not clobber swp_entry_t during THP
split") kept that swapcache exception, and made that tail page check
explict:

static void __split_huge_page_tail(struct page *head, int tail,
		struct lruvec *lruvec, struct list_head *list)
{
	struct page *page_tail = head + tail;

	...

	/*
	 * page->private should not be set in tail pages with the exception
	 * of swap cache pages that store the swp_entry_t in tail pages.
	 * Fix up and warn once if private is unexpectedly set.
	 */
	if (!folio_test_swapcache(page_folio(head))) {
		VM_WARN_ON_ONCE_PAGE(page_tail->private != 0, head);
		page_tail->private = 0;
	}
	...
}


5aae9265ee1a ("mm: prep_compound_tail() clear page->private") followd
the same rule. Changelog says the warning had already caught real
non-zero tail page-private values:

"
    Change that warning to dump page_tail (which also dumps head), instead of
    just the head: so far we have seen dead000000000122, dead000000000003,
    dead000000000001 or 0000000000000002 in the raw output for tail private.
"

So prep_compound_tail() clears tail page->private for tail pages:

static void prep_compound_tail(struct page *head, int tail_idx)
{
	struct page *p = head + tail_idx;

	...
	set_page_private(p, 0);
}

cfeed8ffe55b ("mm/swap: stop using page->private on tail pages for
THP_SWAP") stopped keeping THP swap entries in tail page->privata.

So split code started warning and clearing old tail page->private before
setting the swap entry:

static void __split_huge_page_tail(struct page *head, int tail,
		struct lruvec *lruvec, struct list_head *list)
{
	struct page *page_tail = head + tail;

	...

	/*
	 * page->private should not be set in tail pages. Fix up and warn once
	 * if private is unexpectedly set.
	 */
	if (unlikely(page_tail->private)) {
		VM_WARN_ON_ONCE_PAGE(true, page_tail);
		page_tail->private = 0;
	}
	if (PageSwapCache(head))
		set_page_private(page_tail, (unsigned long)head->private + tail);
	...
}

00527733d0dc ("mm/huge_memory: add two new (not yet used) functions for
folio_split()") added the same check to __split_folio_to_order():

static void __split_folio_to_order(struct folio *folio, int old_order,
		int new_order)
{
		...

		/*
		 * page->private should not be set in tail pages. Fix up and warn once
		 * if private is unexpectedly set.
		 */
		if (unlikely(new_folio->private)) {
			VM_WARN_ON_ONCE_PAGE(true, new_head);
			new_folio->private = NULL;
		}

		if (folio_test_swapcache(folio))
			new_folio->swap.val = folio->swap.val + i;
		...
}

4265d67e405a ("mm/migrate_device: add THP splitting during migration")
removed that check ... Worth noting, David pointed out in the thread that
the check had caught bugs before. Guess nobody really noticed at the time.

https://lore.kernel.org/all/76750d20-cdfe-41bb-a228-9b3f171675ec@kernel.org/

So adding it back makes sense to me. Just in case it's useful later, feel
free to add:

Reviewed-by: Lance Yang <lance.yang@linux.dev>