mm/madvise.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
MADV_COLD or MADV_PAGEOUT over part of a PMD splits the THP in
madvise_cold_or_pageout_pte_range(). Two threads doing that to
the same THP create spurious failures.
CPU0 CPU1
---- ----
folio_get()
spin_unlock(ptl)
folio_lock()
folio_get()
spin_unlock(ptl)
folio_lock() <- blocks, keeps its ref
split_folio()
folio_expected_ref_count(folio) != folio_ref_count(folio) - 1
-EAGAIN
CPU1 cannot drop its reference until it gets the lock CPU0 holds, so CPU0's
split always fails. folio_trylock() makes CPU1 leave without ever taking a
reference. The PTE branch of this same function already does this, as do
madvise_free_pte_range() and madvise_free_huge_pmd().
Reproducer: 400 rounds of eight threads calling MADV_COLD on half of each
of eight THPs, re-formed with MADV_COLLAPSE between rounds. From
/proc/vmstat:
thp_split_page thp_split_page_failed
before 3186 860
after 3200 0
The short before count is rounds where every thread failed and the
advice was dropped for that THP entirely.
On failure the walker returns 0 and nothing retries. The PMD path becomes
best effort when the folio lock is held elsewhere - same as the PTE path.
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260817220810.1175596-1-gourry%40gourry.net
Assisted-by: LLM
Cc: <stable@vger.kernel.org>
Signed-off-by: Gregory Price (Meta) <gourry@gourry.net>
Acked-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
---
mm/madvise.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/madvise.c b/mm/madvise.c
index f75a9d139980..ae3d7127d87c 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -419,9 +419,10 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
if (next - addr != HPAGE_PMD_SIZE) {
int err;
+ if (!folio_trylock(folio))
+ goto huge_unlock;
folio_get(folio);
spin_unlock(ptl);
- folio_lock(folio);
err = split_folio(folio);
folio_unlock(folio);
folio_put(folio);
--
2.55.0On 9/12/26 13:05, Gregory Price wrote: > MADV_COLD or MADV_PAGEOUT over part of a PMD splits the THP in > madvise_cold_or_pageout_pte_range(). Two threads doing that to > the same THP create spurious failures. > > CPU0 CPU1 > ---- ---- > folio_get() > spin_unlock(ptl) > folio_lock() > folio_get() > spin_unlock(ptl) > folio_lock() <- blocks, keeps its ref > split_folio() > folio_expected_ref_count(folio) != folio_ref_count(folio) - 1 > -EAGAIN > > CPU1 cannot drop its reference until it gets the lock CPU0 holds, so CPU0's > split always fails. folio_trylock() makes CPU1 leave without ever taking a > reference. The PTE branch of this same function already does this, as do > madvise_free_pte_range() and madvise_free_huge_pmd(). > > Reproducer: 400 rounds of eight threads calling MADV_COLD on half of each > of eight THPs, re-formed with MADV_COLLAPSE between rounds. From > /proc/vmstat: > > thp_split_page thp_split_page_failed > before 3186 860 > after 3200 0 > > The short before count is rounds where every thread failed and the > advice was dropped for that THP entirely. While the split now succeeds, one of both calls will just effectively skip processing the page table. SO while CPU0 will succeed with the split, CPU1 would just skip the page table. And what happened before? Split on CPU0 failed and it would skip the page table. Split on CPU1, however, would likely have succeeded? So this is all far from perfect. ... but this matches what we do further down in the PTE scenario. Acked-by: David Hildenbrand (Arm) <david@kernel.org> -- Cheers, David
On Wed, Sep 16, 2026 at 08:25:01AM +0200, David Hildenbrand (Arm) wrote: > On 9/12/26 13:05, Gregory Price wrote: > > MADV_COLD or MADV_PAGEOUT over part of a PMD splits the THP in > > madvise_cold_or_pageout_pte_range(). Two threads doing that to > > the same THP create spurious failures. > > > > CPU0 CPU1 > > ---- ---- > > folio_get() > > spin_unlock(ptl) > > folio_lock() > > folio_get() > > spin_unlock(ptl) > > folio_lock() <- blocks, keeps its ref > > split_folio() > > folio_expected_ref_count(folio) != folio_ref_count(folio) - 1 > > -EAGAIN > > > > CPU1 cannot drop its reference until it gets the lock CPU0 holds, so CPU0's > > split always fails. folio_trylock() makes CPU1 leave without ever taking a > > reference. The PTE branch of this same function already does this, as do > > madvise_free_pte_range() and madvise_free_huge_pmd(). > > > > Reproducer: 400 rounds of eight threads calling MADV_COLD on half of each > > of eight THPs, re-formed with MADV_COLLAPSE between rounds. From > > /proc/vmstat: > > > > thp_split_page thp_split_page_failed > > before 3186 860 > > after 3200 0 > > > > The short before count is rounds where every thread failed and the > > advice was dropped for that THP entirely. bleh i thought i rewrote this sentence. reading madvise is driving me crazy. > > While the split now succeeds, one of both calls will just effectively skip > processing the page table. SO while CPU0 will succeed with the split, CPU1 would > just skip the page table. > > And what happened before? > > Split on CPU0 failed and it would skip the page table. Split on CPU1, however, > would likely have succeeded? > Yes., all correct. > So this is all far from perfect. > > ... but this matches what we do further down in the PTE scenario. > > Acked-by: David Hildenbrand (Arm) <david@kernel.org> > Thank you! ~gregory
On Sat, 12 Sep 2026 07:05:40 -0400 Gregory Price <gourry@gourry.net> wrote:
> MADV_COLD or MADV_PAGEOUT over part of a PMD splits the THP in
> madvise_cold_or_pageout_pte_range(). Two threads doing that to
> the same THP create spurious failures.
>
> CPU0 CPU1
> ---- ----
> folio_get()
> spin_unlock(ptl)
> folio_lock()
> folio_get()
> spin_unlock(ptl)
> folio_lock() <- blocks, keeps its ref
> split_folio()
> folio_expected_ref_count(folio) != folio_ref_count(folio) - 1
> -EAGAIN
>
> CPU1 cannot drop its reference until it gets the lock CPU0 holds, so CPU0's
> split always fails. folio_trylock() makes CPU1 leave without ever taking a
> reference. The PTE branch of this same function already does this, as do
> madvise_free_pte_range() and madvise_free_huge_pmd().
>
> Reproducer: 400 rounds of eight threads calling MADV_COLD on half of each
> of eight THPs, re-formed with MADV_COLLAPSE between rounds. From
> /proc/vmstat:
>
> thp_split_page thp_split_page_failed
> before 3186 860
> after 3200 0
>
> The short before count is rounds where every thread failed and the
> advice was dropped for that THP entirely.
>
> On failure the walker returns 0 and nothing retries. The PMD path becomes
> best effort when the folio lock is held elsewhere - same as the PTE path.
Can this result in more EAGAINs being returned to userspace?
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -419,9 +419,10 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> if (next - addr != HPAGE_PMD_SIZE) {
> int err;
>
> + if (!folio_trylock(folio))
> + goto huge_unlock;
> folio_get(folio);
> spin_unlock(ptl);
> - folio_lock(folio);
> err = split_folio(folio);
> folio_unlock(folio);
> folio_put(folio);
Sashiko shares my reaction, "dear god, why does that function exist".
https://sashiko.dev/#/patchset/20260912110540.3203010-1-gourry@gourry.net
Like the entire function, that random mix of "goto foo" with "return
whatever" needs to die.
Anyway, please check it out while you're on a roll.
On Tue, Sep 15, 2026 at 06:46:33PM -0700, Andrew Morton wrote:
> >
> > On failure the walker returns 0 and nothing retries. The PMD path becomes
> > best effort when the folio lock is held elsewhere - same as the PTE path.
>
> Can this result in more EAGAINs being returned to userspace?
>
I don't see where this can result in more EAGAIN, at least from the
madvise side. The race is the same, we're just giving up and saying
"I tried, doesn't matter if i succeeded".
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -419,9 +419,10 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> > if (next - addr != HPAGE_PMD_SIZE) {
> > int err;
> >
> > + if (!folio_trylock(folio))
> > + goto huge_unlock;
> > folio_get(folio);
> > spin_unlock(ptl);
> > - folio_lock(folio);
> > err = split_folio(folio);
> > folio_unlock(folio);
> > folio_put(folio);
>
> Sashiko shares my reaction, "dear god, why does that function exist".
>
> https://sashiko.dev/#/patchset/20260912110540.3203010-1-gourry@gourry.net
>
> Like the entire function, that random mix of "goto foo" with "return
> whatever" needs to die.
>
> Anyway, please check it out while you're on a roll.
I've been trying to shore up the sashiko garbage first in a backportable
manner before doing a full restructure.
I would hate to rewrite this just to carry bugs forward, and clearly the
code isn't even remotely correct as-is.
The remaining issue on that link is the memory leak patch you already
looked at.
It's taken a bit to generate positive tests for these bugs and go
through validation, but I think we are finally at the end of the
nasty part and can actually get to cleaning this up.
~Gregory
© 2016 - 2026 Red Hat, Inc.