Re: [External] Re: [PATCH v2] mm/madvise: avoid skipping pages after splitting large folios

["David Hildenbrand (Arm)" <david@kernel.org>]

On 8/11/26 04:31, yunhui cui wrote:
> Hi Andrew, David, Lorenzo,
> 
> On Thu, Aug 6, 2026 at 11:40 PM Lorenzo Stoakes (ARM) <ljs@kernel.org> wrote:
>>
>> On Thu, Aug 06, 2026 at 04:46:10PM +0200, David Hildenbrand (Arm) wrote:
>>>
>>> We GUP'ed a single page and now try to be smart about which other pages we'd GUP
>>> next.
>>>
>>> That's just wrong, and hugetlb special-casing is just ugly.
>>>
>>> The problem here is that, if we GUP'ed a page and poisoned it, the GUP'ing the
>>> next page might fail and we'd return an error.
>>>
>>> But maybe that error can simply be handled? We have FOLL_HWPOISON.
>>>
>>> So maybe we can just use FOLL_HWPOISON and skip over the entries that already
>>> return -EHWPOISON?
>>
>> Yup this is ugly debug code so that works for me.
> 
> Thank you for the review. Based on your feedback, I went back through the
> madvise, GUP, soft-offline, and memory-failure paths and outlined the
> changes I plan to make for the next revision.
> 
> The issue is that using a page obtained for one address to infer how far
> the range walker can advance is the wrong abstraction.
> 
> For an anonymous large folio, soft_offline_page() splits the folio to
> order-0 and handles only the supplied base-page PFN. Advancing by the
> pre-split folio size can therefore skip the remaining base pages while
> madvise() still returns success.
> 
> Lorenzo also raised the semantics of a range that covers only part of a
> hugetlb page. Looking at a range that crosses a hugetlb boundary exposes
> another problem. For example, with two 2 MiB hugepages:
> 
> hugepage A: [0, 2 MiB)
> hugepage B: [2 MiB, 4 MiB)
> requested range: [2 MiB - 4 KiB, 2 MiB + 4 KiB)
> 
> The first GUP resolves the last base page in hugepage A. Adding the full
> 2 MiB hugepage size to that unaligned address produces the next address
> at 4 MiB - 4 KiB. That is already beyond the requested end at
> 2 MiB + 4 KiB, so the loop terminates without ever visiting hugepage B.
> 
> For MADV_SOFT_OFFLINE:
> 
> - ordinary pages and large folios advance by PAGE_SIZE because
>   soft_offline_page() handles the supplied base-page PFN after any split;
> 
> - hugetlb advances to the end of the current hugepage because successful
>   soft-offline migrates the complete hugepage and leaves a healthy
>   replacement mapped. If the walker advanced by PAGE_SIZE, its next GUP
>   would resolve that healthy replacement and soft-offline the same virtual
>   hugepage again;
> 
> - ZONE_DEVICE does not need a stride case because soft_offline_page()
>   rejects it.
> 
> Advancing to the current hugepage boundary, rather than adding the hugepage
> size to the original unaligned address, lets the next iteration start
> exactly at hugepage B.
> 
> For MADV_HWPOISON, I plan to follow David's suggestion and walk at
> PAGE_SIZE using:
> 
> get_user_pages_unlocked(start, 1, &page,
> FOLL_GET | FOLL_HWPOISON)
> 
> get_user_pages_unlocked() is the appropriate interface here because the
> current gup_fast_fallback() flag mask rejects FOLL_HWPOISON, while the
> memory-failure madvise path enters madvise_inject_error() without
> mmap_lock held. get_user_pages_unlocked() acquires and releases mmap_lock
> internally, handles fault retries, and propagates -EHWPOISON from the
> fault path. FOLL_GET makes the page-reference ownership consumed by
> MF_COUNT_INCREASED explicit.
> 
> A successful GUP is followed by memory_failure(). If GUP returns
> -EHWPOISON, the address was already covered by an earlier larger-granularity
> injection, so the walker continues with the next base-page address. Other
> errors are returned. This avoids hugetlb, DAX, and folio-size inference in
> the MADV_HWPOISON caller.
> 
> Device DAX is relevant only to MADV_HWPOISON because
> MADV_SOFT_OFFLINE rejects ZONE_DEVICE pages. Since the proposed
> MADV_HWPOISON walker advances by PAGE_SIZE and uses each GUP result as
> feedback rather than inferring the handled range from folio_size(), it
> should also avoid the same granularity problem for Device DAX. A
> successful GUP is passed to memory_failure(), while -EHWPOISON indicates
> that the address was already covered by an earlier injection. Advancing
> by PAGE_SIZE should therefore also work for Device DAX in principle. I do
> not currently have a suitable Device DAX setup, so this remains untested
> at runtime.
> 
> Because MADV_SOFT_OFFLINE must advance past a hugetlb replacement while
> MADV_HWPOISON can use FOLL_HWPOISON feedback during a PAGE_SIZE walk, I
> plan to use separate walking models for the two operations.
> 
> Before posting another revision, I plan to split the work into:
> 
> 1. the MADV_SOFT_OFFLINE range-walk fix;
> 2. MADV_SOFT_OFFLINE large-folio and hugetlb selftests;
> 3. the PAGE_SIZE + FOLL_HWPOISON MADV_HWPOISON walker;
> 4. MADV_HWPOISON large-folio and hugetlb selftests.
> 
> Does this separation of the SOFT_OFFLINE and HWPOISON walking models look
> reasonable?
As Lorenzo says, this reads AI generated.

I assume what you mean is:

MADV_HWPOISON will actually hwpoison the pages. We can just use FOLL_HWPOISON +
-EHWPOISON and should be good. So far the theory.

MADV_SOFT_OFFLINE will migrate pages instead. So if we don't skip multiple
pages, we could end up migrating multiple times (and setting hwpoison multiple
times).

Well, for large folios (except hugetlb) that's not a problem, because we try
splitting to order-0 either way, and if that fails, we bail out.

So what remains is hugetlb, which is nasty.

I don't really enjoy having any special-casing here, because the moment we e.g.,
change how soft-offlining deals with splitting, we would be in trouble.

Assume we start support splitting to min-order at some point, we'd also want o
skip over min-order. Gah.


Can we just make our life easier and disallow specifying ranges for
MADV_HWPOISON/MADV_SOFT_OFFLINE?

It's a pure testing interface IIRC. tools/testing/selftests/mm/memory-failure.c
seems to always call it with PAGE_SIZE. Similarly
tools/testing/selftests/mm/hugetlb-read-hwpoison.c

There is one catch I think: an existing LTP test case issues MADV_SOFT_OFFLINE
on a larger range. But it respects -EINVAL at least :)

So we could return -EINVAL and fixup the test case to issue multiple madvise().

[1]
https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/move_pages/move_pages12.c

-- 
Cheers,

David