Re: [RFC PATCH v3] mm/thp: serialize huge-zero folio state transitions

[Lorenzo Stoakes (ARM)]

TL;DR - after spending far too long on this - this is very subtle and I am
not confident you understand the patch, I wonder if the fix is possibly
AI-generated also?

This is really subtle stuff and requires backports, so I think it's better
if I send a patch myself.

(I think this is less egregious than it might seem in that the lock
suggestion is ultimately David's in any case.)

I will acknowledge this submission in the commit message, however.

It seems the race is:

static unsigned long shrink_huge_zero_folio_scan(struct shrinker *shrink,
						 struct shrink_control *sc)
{
	if (atomic_cmpxchg(&huge_zero_refcount, 1, 0) == 1) {   <-------------------------- 1. sets to 0
		struct folio *zero_folio = xchg(&huge_zero_folio, NULL); < preempted for a very very long time>
		BUG_ON(zero_folio == NULL);
		WRITE_ONCE(huge_zero_pfn, ~0UL);                <------------------------- 6. Overwrites the valid PFN
		folio_put(zero_folio);
		return HPAGE_PMD_NR;
	}

	return 0;
}

static bool get_huge_zero_folio(void)
{
	struct folio *zero_folio;
retry:
	if (likely(atomic_inc_not_zero(&huge_zero_refcount))) <--------------------  2. sees the zero
		return true;

	zero_folio = folio_alloc((GFP_TRANSHUGE | __GFP_ZERO | __GFP_ZEROTAGS) & <--- 3. allocates (magically, very very quickly)
				 ~__GFP_MOVABLE,
			HPAGE_PMD_ORDER);
	if (!zero_folio) {
		count_vm_event(THP_ZERO_PAGE_ALLOC_FAILED);
		return false;
	}
	/* Ensure zero folio won't have large_rmappable flag set. */
	folio_clear_large_rmappable(zero_folio);
	preempt_disable();
	if (cmpxchg(&huge_zero_folio, NULL, zero_folio)) { <---------------- 4. no concurrent setter so branch not taken
		preempt_enable();
		folio_put(zero_folio);
		goto retry;
	}
	WRITE_ONCE(huge_zero_pfn, folio_pfn(zero_folio)); <----------------- 5. Writes the PFN first despite not being able to be reordered

	/* We take additional reference here. It will be put back by shrinker */
	atomic_set(&huge_zero_refcount, 2);
	preempt_enable();
	count_vm_event(THP_ZERO_PAGE_ALLOC);
	return true;
}

?

In general I don't know why we're not just making
CONFIG_PERSISTENT_HUGE_ZERO_FOLIO mandatory for CONFIG_TRANSPARENT_HUGEPAGE.

If you're embedded, don't enabled THP (which will up your reserves a bunch for
khugepaged anyway).

On Mon, Jul 27, 2026 at 06:08:53PM +0200, David Hildenbrand (Arm) wrote:
> On 7/27/26 17:40, Hengbin Zhang wrote:
> > The nonpersistent huge-zero shrinker clears huge_zero_folio and then

NIT: insert a hyphen: non-persistent.

> > invalidates huge_zero_pfn. A concurrent fault can publish a replacement

'Invalidates'? How do you mean?

You mean setting it to NULL? Clearer to say that.

> > folio and PFN between those updates, after which the old shrinker
> > invalidates the replacement generation's PFN identity.

I don't understand what you mean generation or PFN identity?


> >
> > A later partial mprotect() can then misidentify the live special PMD and
> > enter the ordinary anonymous THP split path.

What is a live special PMD?

Please don't say special :) that term is so overloaded in mm.

'...can then fail to identify the huge zero folio which then incorrectly enters
the anonymous THP split path'.

But I'd even not mention mprotect() specifically, if the huge zero folio can be
misidentified that's already a problem.

> >
> > A writer-side spinlock keeps huge_zero_folio and huge_zero_pfn updates
> > from different generations together. Lockless getters still use the

I'm very confused by what you mean.

> > refcount fast path. atomic_set_release() publishes an initialized new
> > generation before a successful atomic_inc_not_zero() can admit a getter
> > on weakly ordered architectures.

Overall I am not confident you understand this so as per above I think

> >
> > Suggested-by: David Hildenbrand <david@xxxxxxxxxx>

Should be a S-o-b and David should handle this.

> > Link: https://lore.kernel.org/r/20260724100509.2300200-1-uqbarz@xxxxxxxxx
>
> We want a Fixes: tag + Cc: stable.

Yup.

>
> Can you dig up the proper Fixes commit? Thanks
>
> > Signed-off-by: Hengbin Zhang <uqbarz@xxxxxxxxx>
> > ---
> >  mm/huge_memory.c | 22 +++++++++++++++-------
> >  1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index b5d1e9d4463d..79bec4495540 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -78,6 +78,7 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
> >  static bool split_underused_thp = true;
> >
> >  static atomic_t huge_zero_refcount;
> > +static DEFINE_SPINLOCK(huge_zero_lock);
> >  struct folio *huge_zero_folio __read_mostly;
> >  unsigned long huge_zero_pfn __read_mostly = ~0UL;
> >  unsigned long huge_anon_orders_always __read_mostly;
> > @@ -237,17 +238,18 @@ static bool get_huge_zero_folio(void)
> >  	}
> >  	/* Ensure zero folio won't have large_rmappable flag set. */
> >  	folio_clear_large_rmappable(zero_folio);
> > -	preempt_disable();
> > -	if (cmpxchg(&huge_zero_folio, NULL, zero_folio)) {
> > -		preempt_enable();
> > +	spin_lock(&huge_zero_lock);
> > +	if (READ_ONCE(huge_zero_folio)) {
>
> Same comment as below: who could possibly modify this value concurrently? I
> don't think we need the READ_ONCE.

Yeah I don't think so either.

>
> > +		spin_unlock(&huge_zero_lock);
> >  		folio_put(zero_folio);
> >  		goto retry;
> >  	}
> >  	WRITE_ONCE(huge_zero_pfn, folio_pfn(zero_folio));
> > +	WRITE_ONCE(huge_zero_folio, zero_folio);

This is still necessary however for concurrent lockless readers.

> > +	spin_unlock(&huge_zero_lock);
> >
> > -	/* We take additional reference here. It will be put back by shrinker */
> > -	atomic_set(&huge_zero_refcount, 2);
> > -	preempt_enable();
> > +	/* Publish the identity before admitting lockless getters. */

You're deleting the comment about an additional reference...

And this comment doesn't make any sense really.

> > +	atomic_set_release(&huge_zero_refcount, 2);
>
> Why do we need the _release semantics here when we just did a spin_unlock()?

This is really suspect, as I had AI look at this and it also suggested
exactly what you did here, and I'm not confident you understand it.

Are you generating this code using AI without disclosing it against the
kernel policy on this?

See https://kernel.org/doc/html/latest/process/coding-assistants.html

Can you give a detailed, coherent response as to why you chose this, if it
was not AI-generated? A human-sounding one? :)

Anyway.

AI says this actually _is_ necessary.

And using my human brain to think about it:

static bool get_huge_zero_folio(void)
{
	struct folio *zero_folio;
retry:
	if (likely(atomic_inc_not_zero(&huge_zero_refcount)))
		return true;

	zero_folio = folio_alloc((GFP_TRANSHUGE | __GFP_ZERO | __GFP_ZEROTAGS) &
				 ~__GFP_MOVABLE,
			HPAGE_PMD_ORDER);
	if (!zero_folio) {
		count_vm_event(THP_ZERO_PAGE_ALLOC_FAILED);
		return false;
       	}
	/* Ensure zero folio won't have large_rmappable flag set. */
	folio_clear_large_rmappable(zero_folio);
	spin_lock(&huge_zero_lock);                          <--- acquire semantics
	if (huge_zero_folio) {                                            |			   <|
		spin_unlock(&huge_zero_lock);                             v			   <|
		folio_put(zero_folio);			  All loads/stores AFTER appear AFTER	   <|
		goto retry;									   <|
	}                                                  All loads/stores BEFORE appear BEFORE   <|
	WRITE_ONCE(huge_zero_pfn, folio_pfn(zero_folio));                 ^			   <| Anywhere here.
	WRITE_ONCE(huge_zero_folio, zero_folio);                          |			   <|
	spin_unlock(&huge_zero_lock);                         <--- release semantics               <|
                                                                                                   <|
	/* We take additional reference here. It will be put back by shrinker */                   <|
	atomic_set(&huge_zero_refcount, 2); ---- relaxed ordering so could be moved ----------------|

	count_vm_event(THP_ZERO_PAGE_ALLOC);
	return true;
}

So yeah I think that should be atomic_set_release() in order to prevent
that occurring before the critical section.

The retry stuff is a bit suspect now, how could we be concurrently raced in
order to require a retry? I don't think so.

It's needed because it has to pair with atomic_inc_not_zero() to prevent a
concurrent get_huge_zero_folio() invocation from incorrectly returning
before the critical section is completely finished.

>
> >  	count_vm_event(THP_ZERO_PAGE_ALLOC);
> >  	return true;
> >  }
> > @@ -298,9 +300,15 @@ static unsigned long shrink_huge_zero_folio_scan(struct shrinker *shrink,
> >  						 struct shrink_control *sc)
> >  {
> >  	if (atomic_cmpxchg(&huge_zero_refcount, 1, 0) == 1) {
> > -		struct folio *zero_folio = xchg(&huge_zero_folio, NULL);
> > +		struct folio *zero_folio;
> > +
> > +		spin_lock(&huge_zero_lock);
> > +		zero_folio = READ_ONCE(huge_zero_folio);
>
> Why do we need a READ_ONCE? Nobody should be legally be allowed to change this
> variable concurrently, no?

Yup.

>
> >  		BUG_ON(zero_folio == NULL);
> >  		WRITE_ONCE(huge_zero_pfn, ~0UL);
> > +		WRITE_ONCE(huge_zero_folio, NULL);
> > +		spin_unlock(&huge_zero_lock);
> > +
> >  		folio_put(zero_folio);
> >  		return HPAGE_PMD_NR;
> >  	}
>
>
> --
> Cheers,
>
> David

Thanks, Lorenzo