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

[yunhui cui]

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:
> > On 8/6/26 16:34, Lorenzo Stoakes (ARM) wrote:
> > > On Thu, Aug 06, 2026 at 01:35:30PM +0200, David Hildenbrand (Arm) wrote:
> > >>>
> > >>> Let's at least split out the folio check into a helper to make things
> > >>> clearer:
> > >>>
> > >>> static bool poison_splits_folio(const struct folio *folio)
> > >>> {
> > >>>   /* Hugetlb is, as always, a world unto itself. */
> > >>>   if (folio_test_hugetlb(folio))
> > >>>           return false;
> > >>>   /* Soft-offline errors out, hwpoison traverse DAX intact. */
> > >>>   if (folio_is_zone_device(folio))
> > >>>           return false;
> > >>>   return true;
> > >>> }
> > >>>
> > >>> Then for your patch:
> > >>>
> > >>> -         size = PAGE_SIZE;
> > >>> -         if (folio_test_hugetlb(folio) || folio_is_zone_device(folio))
> > >>> -                 size = folio_size(folio);
> > >>> +         size = poison_splits_folio(folio) ? PAGE_SIZE : folio_size(folio);
> > >>>
> > >>> I tried writing something that was neater and nicer but AI kept pointing
> > >>> out how it was totally broken and I really really hate this code (not your
> > >>> fault :).
> > >>
> > >> No, I don't think any such special casing on folios is the right way to handle it.
> > >
> > > I mean the issue here is the stride varies depending on whether the thing is
> > > hugetlb or not (and some weird DAX thing), and the poisoning causes a split
> > > otherwise so if you want to poison a range you have to account for that.
> >
> > 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?

For stable, would you agree that I should omit the explicit stable Cc from
the next revision?

>
> >
> > --
> > Cheers,
> >
> > David
>
> --
> Cheers, Lorenzo

Thanks，
Yunhui