[RFC PATCH] mm/thp: order huge zero folio PFN invalidation before removal

[RFC PATCH] mm/thp: order huge zero folio PFN invalidation before removal

[Hengbin Zhang]

The nonpersistent huge-zero shrinker removes huge_zero_folio with
xchg() and then invalidates huge_zero_pfn. A concurrent fault can
publish a new folio and its PFN between these operations, after which
the old shrinker invalidates the new generation's PFN identity.

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

Invalidate huge_zero_pfn before making huge_zero_folio NULL. A getter
which observes a zero refcount while the old pointer is still present
cannot publish a new folio: its cmpxchg() fails and it retries. A
getter which succeeds does so after the invalidation and publishes the
new PFN afterwards.

Fixes: 3b77e8c8cde5 ("mm/thp: make is_huge_zero_pmd() safe and quicker")

Signed-off-by: Hengbin Zhang <uqbarz@gmail.com>
---
 mm/huge_memory.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index b5d1e9d4463d..fcc492368160 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -298,9 +298,11 @@ 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);
+		struct folio *zero_folio;
+
 		WRITE_ONCE(huge_zero_pfn, ~0UL);
+		zero_folio = xchg(&huge_zero_folio, NULL);
+		BUG_ON(zero_folio == NULL);
 		folio_put(zero_folio);
 		return HPAGE_PMD_NR;
 	}
-- 
2.34.1

Re: [RFC PATCH] mm/thp: order huge zero folio PFN invalidation before removal

[David Hildenbrand (Arm)]

Hi,

How was this issue identified or observed?

On 7/24/26 12:05, Hengbin Zhang wrote:
> The nonpersistent huge-zero shrinker removes huge_zero_folio with
> xchg() and then invalidates huge_zero_pfn. A concurrent fault can
> publish a new folio and its PFN between these operations, after which
> the old shrinker invalidates the new generation's PFN identity.


You mean, the freeing path does

1) xchg(&huge_zero_folio, NULL);
2) WRITE_ONCE(huge_zero_pfn, ~0UL);

Whereby the allocation path does

1) cmpxchg(&huge_zero_folio, NULL, zero_folio)
2) WRITE_ONCE(huge_zero_pfn, folio_pfn(zero_folio));

Sashiko also IMHO correctly complains about a missing barrier when we re-set the
refcount.

So I wonder whether we should just stop these cmpxch games and just use a
spinlock around updating huge_zero_folio+huge_zero_pfn?

-- 
Cheers,

David

Re: [RFC PATCH] mm/thp: order huge zero folio PFN invalidation before removal

[Hengbin Zhang]

[-- Attachment #1: Type: text/plain, Size: 1858 bytes --]

Hi David,

The issue was first flagged by an AI-assisted review while I was
analyzing the THP code. I then traced the huge-zero folio lifecycle and
confirmed the interleaving. The window is difficult to hit normally, but
it can be reproduced with test-only instrumentation.

Regarding Sashiko's weak-memory-ordering concern, I think that
atomic_set_release(&huge_zero_refcount, 2) would provide the required
release semantics when publishing a newly initialized huge-zero folio.
However, this only solves the publication ordering between the writer
and lockless readers.

I think your suggestion might be a better choice. A short writer-side
spinlock
around the huge_zero_folio/huge_zero_pfn state transition looks like a
clearer solution.
I will send a spinlock-based version for further revision.

Thanks,
Hengbin

On Sat, Jul 25, 2026 at 2:48 AM David Hildenbrand (Arm) <david@kernel.org>
wrote:

> Hi,
>
> How was this issue identified or observed?
>
> On 7/24/26 12:05, Hengbin Zhang wrote:
> > The nonpersistent huge-zero shrinker removes huge_zero_folio with
> > xchg() and then invalidates huge_zero_pfn. A concurrent fault can
> > publish a new folio and its PFN between these operations, after which
> > the old shrinker invalidates the new generation's PFN identity.
>
>
> You mean, the freeing path does
>
> 1) xchg(&huge_zero_folio, NULL);
> 2) WRITE_ONCE(huge_zero_pfn, ~0UL);
>
> Whereby the allocation path does
>
> 1) cmpxchg(&huge_zero_folio, NULL, zero_folio)
> 2) WRITE_ONCE(huge_zero_pfn, folio_pfn(zero_folio));
>
> Sashiko also IMHO correctly complains about a missing barrier when we
> re-set the
> refcount.
>
> So I wonder whether we should just stop these cmpxch games and just use a
> spinlock around updating huge_zero_folio+huge_zero_pfn?
>
> --
> Cheers,
>
> David
>

[-- Attachment #2: Type: text/html, Size: 2307 bytes --]

[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

[Hengbin Zhang]

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

Hi Lorenzo,

Thanks for the reminder. I apologize for not following the expected
submission workflow.

I'm new to the community and will ensure future revisions are sent as
separate emails after responding to reviews. I will also allow sufficient
time between revisions so reviewers can follow up and provide feedback.

Thanks again for your guidance and patience.

Best regards,
Hengbin

On Mon, Jul 27, 2026 at 11:43 PM Lorenzo Stoakes (ARM) <ljs@kernel.org>
wrote:

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

[-- Attachment #2: Type: text/html, Size: 1287 bytes --]

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