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

[yunhui cui]

Hi David,

On Tue, Aug 11, 2026 at 10:49 PM David Hildenbrand (Arm)
<david@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx> 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

Sorry for the late reply, and thanks for the feedback.

This turned out to be more involved than I expected. I am not an
expert in the memory-failure/hugetlb area, so if someone is interested in
taking it over, that would be appreciated.

Thanks,
Yunhui

>
> --
> Cheers,
>
> David