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

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

[Hengbin Zhang]

The nonpersistent huge-zero shrinker clears huge_zero_folio and then
invalidates huge_zero_pfn. A concurrent fault can publish a replacement
folio and PFN between those updates, after which the old shrinker
invalidates the replacement generation's PFN identity.

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

A writer-side spinlock keeps huge_zero_folio and huge_zero_pfn updates
from different generations together. Lockless getters still use the
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.

The race was reproduced using test-only instrumentation that widens the
shrinker/allocation window; that instrumentation is not included here.

Suggested-by: David Hildenbrand <david@kernel.org>
Link: https://lore.kernel.org/r/20260724100509.2300200-1-uqbarz@gmail.com
Signed-off-by: Hengbin Zhang <uqbarz@gmail.com>
---
 mm/huge_memory.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index b5d1e9d4463d..89771a9b282d 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,19 @@ 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)) {
+		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);
+
+	/* Publish the identity before admitting lockless getters. */
+	atomic_set_release(&huge_zero_refcount, 2);
+	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();
 	count_vm_event(THP_ZERO_PAGE_ALLOC);
 	return true;
 }
@@ -297,15 +300,22 @@ static unsigned long shrink_huge_zero_folio_count(struct shrinker *shrink,
 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);
-		BUG_ON(zero_folio == NULL);
-		WRITE_ONCE(huge_zero_pfn, ~0UL);
-		folio_put(zero_folio);
-		return HPAGE_PMD_NR;
+	struct folio *zero_folio;
+
+	spin_lock(&huge_zero_lock);
+	if (atomic_cmpxchg(&huge_zero_refcount, 1, 0) != 1) {
+		spin_unlock(&huge_zero_lock);
+		return 0;
 	}
 
-	return 0;
+	zero_folio = READ_ONCE(huge_zero_folio);
+	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;
 }
 
 static struct shrinker *huge_zero_folio_shrinker;
-- 
2.34.1

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

[David Hildenbrand (Arm)]

On 7/27/26 15:18, Hengbin Zhang wrote:
> The nonpersistent huge-zero shrinker clears huge_zero_folio and then
> invalidates huge_zero_pfn. A concurrent fault can publish a replacement
> folio and PFN between those updates, after which the old shrinker
> invalidates the replacement generation's PFN identity.
> 
> A later partial mprotect() can then misidentify the live special PMD and
> enter the ordinary anonymous THP split path.
> 
> A writer-side spinlock keeps huge_zero_folio and huge_zero_pfn updates
> from different generations together. Lockless getters still use the
> 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.
> 
> The race was reproduced using test-only instrumentation that widens the
> shrinker/allocation window; that instrumentation is not included here.
> 
> Suggested-by: David Hildenbrand <david@kernel.org>
> Link: https://lore.kernel.org/r/20260724100509.2300200-1-uqbarz@gmail.com
> Signed-off-by: Hengbin Zhang <uqbarz@gmail.com>
> ---
>  mm/huge_memory.c | 36 +++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index b5d1e9d4463d..89771a9b282d 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,19 @@ 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)) {
> +		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);
> +
> +	/* Publish the identity before admitting lockless getters. */
> +	atomic_set_release(&huge_zero_refcount, 2);
> +	spin_unlock(&huge_zero_lock);

Can't we just keep the

atomic_set(&huge_zero_refcount, 2);

after the spin_unlock() ?

>  
> -	/* 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;
>  }
> @@ -297,15 +300,22 @@ static unsigned long shrink_huge_zero_folio_count(struct shrinker *shrink,
>  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);
> -		BUG_ON(zero_folio == NULL);
> -		WRITE_ONCE(huge_zero_pfn, ~0UL);
> -		folio_put(zero_folio);
> -		return HPAGE_PMD_NR;
> +	struct folio *zero_folio;
> +
> +	spin_lock(&huge_zero_lock);
> +	if (atomic_cmpxchg(&huge_zero_refcount, 1, 0) != 1) {
> +		spin_unlock(&huge_zero_lock);
> +		return 0;
>  	}

Do we really have to move that refcount modifications under the lock?

I'd assume it's sufficient to only move setting huge_zero_pfn+huge_zero_folio
under the spinlock.

-- 
Cheers,

David

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

[Hengbin Zhang]

The nonpersistent huge-zero shrinker clears huge_zero_folio and then
invalidates huge_zero_pfn. A concurrent fault can publish a replacement
folio and PFN between those updates, after which the old shrinker
invalidates the replacement generation's PFN identity.

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

A writer-side spinlock keeps huge_zero_folio and huge_zero_pfn updates
from different generations together. Lockless getters still use the
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.

Suggested-by: David Hildenbrand <david@kernel.org>
Link: https://lore.kernel.org/r/20260724100509.2300200-1-uqbarz@gmail.com
Signed-off-by: Hengbin Zhang <uqbarz@gmail.com>
---
 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)) {
+		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);
+	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. */
+	atomic_set_release(&huge_zero_refcount, 2);
 	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);
 		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;
 	}
-- 
2.34.1

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

[Lorenzo Stoakes (ARM)]

Please stop sending new revisions in response to review!

Respond to the review, then send the new revision as an entirely separate email.

Also do not respin this quickly, leave at least a delay of a day between respins
so people can have a chance to keep track and review, the workload is high
enough already.

Thanks, Lorenzo

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

[David Hildenbrand (Arm)]

On 7/27/26 17:40, Hengbin Zhang wrote:
> The nonpersistent huge-zero shrinker clears huge_zero_folio and then
> invalidates huge_zero_pfn. A concurrent fault can publish a replacement
> folio and PFN between those updates, after which the old shrinker
> invalidates the replacement generation's PFN identity.
> 
> A later partial mprotect() can then misidentify the live special PMD and
> enter the ordinary anonymous THP split path.
> 
> A writer-side spinlock keeps huge_zero_folio and huge_zero_pfn updates
> from different generations together. Lockless getters still use the
> 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.
> 
> Suggested-by: David Hildenbrand <david@kernel.org>
> Link: https://lore.kernel.org/r/20260724100509.2300200-1-uqbarz@gmail.com

We want a Fixes: tag + Cc: stable.

Can you dig up the proper Fixes commit? Thanks

> Signed-off-by: Hengbin Zhang <uqbarz@gmail.com>
> ---
>  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.

> +		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);
> +	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. */
> +	atomic_set_release(&huge_zero_refcount, 2);

Why do we need the _release semantics here when we just did a spin_unlock()?

>  	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?

>  		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

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@kernel.org>

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

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

Yup.

>
> Can you dig up the proper Fixes commit? Thanks
>
> > Signed-off-by: Hengbin Zhang <uqbarz@gmail.com>
> > ---
> >  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

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

[Lorenzo Stoakes (ARM)]

On Mon, Jul 27, 2026 at 07:09:10PM +0100, Lorenzo Stoakes (ARM) wrote:

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

Trailing sentence... tired :) delete 'so as per...'

>
> > >
> > > Suggested-by: David Hildenbrand <david@kernel.org>
>
> Should be a S-o-b and David should handle this.

Oops left this in by mistake (tired, late in UK :) this was when I was hoping I
could get David to handle it :P

Cheers, Lorenzo