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

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Tue, Aug 11, 2026 at 10:31:21AM +0800, 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:
> > > 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?

Sigh. This reads like you've just got an LLM to write up a summary.

And prior to the slop era I _never_ saw a summary like this, ever. People would
_reply to the reviewers_ like we were human beings and not prompt-generators.

I've seen this enough times now from sloppers that I believe the intent here is
to workslop reviewers into checking the plan so that response can be fed back
into the LLM as a prompt to write the patch for them.

And I'm not sure I'm interested in seeing a v3 from you given that.

Please read:

https://docs.kernel.org/process/coding-assistants.html
https://docs.kernel.org/process/generated-content.html

And follow correct kernel process on this.

--
Cheers, Lorenzo