[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <09eaca7b-9988-41c7-8d6e-4802055b3f1e@redhat.com>
Date: Mon, 6 Oct 2025 16:18:51 +0200
From: David Hildenbrand <david@...hat.com>
To: Lance Yang <lance.yang@...ux.dev>, Wei Yang <richard.weiyang@...il.com>
Cc: akpm@...ux-foundation.org, lorenzo.stoakes@...cle.com,
Liam.Howlett@...cle.com, baohua@...nel.org, baolin.wang@...ux.alibaba.com,
dev.jain@....com, hughd@...gle.com, ioworker0@...il.com,
kirill@...temov.name, linux-kernel@...r.kernel.org, linux-mm@...ck.org,
mpenttil@...hat.com, npache@...hat.com, ryan.roberts@....com, ziy@...dia.com
Subject: Re: [PATCH mm-new v2 1/1] mm/khugepaged: abort collapse scan on
non-swap entries
On 05.10.25 04:12, Lance Yang wrote:
>
>
> On 2025/10/5 09:05, Wei Yang wrote:
>> On Wed, Oct 01, 2025 at 06:05:57PM +0800, Lance Yang wrote:
>>>
>>>
>>> On 2025/10/1 16:54, Wei Yang wrote:
>>>> On Wed, Oct 01, 2025 at 11:22:51AM +0800, Lance Yang wrote:
>>>>> From: Lance Yang <lance.yang@...ux.dev>
>>>>>
>>>>> Currently, special non-swap entries (like migration, hwpoison, or PTE
>>>>> markers) are not caught early in hpage_collapse_scan_pmd(), leading to
>>>>> failures deep in the swap-in logic.
>>>>>
>>>>> hpage_collapse_scan_pmd()
>>>>> `- collapse_huge_page()
>>>>> `- __collapse_huge_page_swapin() -> fails!
>>>>>
>>>>> As David suggested[1], this patch skips any such non-swap entries
>>>>> early. If any one is found, the scan is aborted immediately with the
>>>>> SCAN_PTE_NON_PRESENT result, as Lorenzo suggested[2], avoiding wasted
>>>>> work.
>>>>>
>>>>> [1] https://lore.kernel.org/linux-mm/7840f68e-7580-42cb-a7c8-1ba64fd6df69@redhat.com
>>>>> [2] https://lore.kernel.org/linux-mm/7df49fe7-c6b7-426a-8680-dcd55219c8bd@lucifer.local
>>>>>
>>>>> Suggested-by: David Hildenbrand <david@...hat.com>
>>>>> Suggested-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
>>>>> Signed-off-by: Lance Yang <lance.yang@...ux.dev>
>>>>> ---
>>>>> v1 -> v2:
>>>>> - Skip all non-present entries except swap entries (per David) thanks!
>>>>> - https://lore.kernel.org/linux-mm/20250924100207.28332-1-lance.yang@linux.dev/
>>>>>
>>>>> mm/khugepaged.c | 32 ++++++++++++++++++--------------
>>>>> 1 file changed, 18 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>>>>> index 7ab2d1a42df3..d0957648db19 100644
>>>>> --- a/mm/khugepaged.c
>>>>> +++ b/mm/khugepaged.c
>>>>> @@ -1284,7 +1284,23 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
>>>>> for (addr = start_addr, _pte = pte; _pte < pte + HPAGE_PMD_NR;
>>>>> _pte++, addr += PAGE_SIZE) {
>>>>> pte_t pteval = ptep_get(_pte);
>>>>> - if (is_swap_pte(pteval)) {
>>>>
>>>> It looks is_swap_pte() is mis-leading?
>>>
>>> Hmm.. not to me, IMO. is_swap_pte() just means:
>>>
>>> !pte_none(pte) && !pte_present(pte)
>>>
>>
>> Maybe it has some reason.
>>
>> I took another look into __collapse_huge_page_swapin(), which just check
>> is_swap_pte() before do_swap_page().
Thanks for pointing that out.
A function that is called __collapse_huge_page_swapin() and documented
to "Bring missing pages in from swap" will handle other types as well.
Unbelievable horrible.
So let's think this through so we can document it in the changelog properly.
We could have currently ended up in do_swap_page() with
(1) Migration entries. We would have waited.
-> Maybe worth it to wait, maybe not. I suspect we don't stumble into
that frequently such that we don't care. We could always unlock this
separately later.
(2) Device-exclusive entries. We would have converted to non-exclusive.
-> See make_device_exclusive(), we cannot tolerate PMD entries and have
to split them through FOLL_SPLIT_PMD. As popped up during a recent
discussion, collapsing here is actually counter-productive, because
the next conversion will PTE-map it again. (until recently, it would
not have worked with large folios at all IIRC).
-> Ok to not collapse.
(3) Device-private entries. We would have migrated to RAM.
-> Device-private still does not support THPs, so collapsing right now
just means that the next device access would split the folio again.
-> Ok to not collapse.
(4) HWPoison entries
-> Cannot collapse
(5) Markers
-> Cannot collapse
I suggest we add that in some form to the patch description, stating
that we can unlock later what we really need, and not account it towards
max_swap_ptes.
>>
>> We have filtered non-swap entries in hpage_collapse_scan_pmd(), but we drop
>> mmap lock before isolation. This looks we may have a chance to get non-swap
>> entry.
>
> Thanks for pointing that out!
>
> Yep, there is a theoretical window between dropping the mmap lock
> after the initial scan and re-acquiring it for isolation.
>
>>
>> Do you think it is reasonable to add a non_swap_entry() check before
>> do_swap_page()?
>
> However, that seems unlikely in practice. IMHO, the early check in
> hpage_collapse_scan_pmd() is sufficient for now, so I'd prefer to
> keep it as-is :)
I think we really should add that check, as per reasoning above.
I was looking into some possible races with uffd-wp being set before we
enter do_swap_page(), but I think it might be okay (although very
confusing).
--
Cheers
David / dhildenb
Powered by blists - more mailing lists