Re: [PATCH RFC 01/14] mm/zsmalloc: replace PG_private with pointer comparison

["Zi Yan" <ziy@nvidia.com>]

On Mon Aug 3, 2026 at 12:53 PM EDT, Johannes Weiner wrote:
> On Mon, Aug 03, 2026 at 11:34:35AM -0400, Zi Yan wrote:
>> On Mon Aug 3, 2026 at 11:04 AM EDT, Johannes Weiner wrote:
>> > On Fri, Jul 31, 2026 at 10:13:24PM -0400, Zi Yan wrote:
>> >> zsmalloc uses PG_private to indicate first zpdesc in the zspage chain.
>> >> Replace it with zpdesc->zspage->first_zpdesc == zpdesc. The check,
>> >> is_first_zpdesc(), is only used in VM_BUG_ON(), so performance impact
>> >> should be negligible.
>> >> 
>> >> It prepares for a future commit that remove PG_private.
>> >> 
>> >> No functional change intended.
>> >> 
>> >> Assisted-by: Claude:claude-opus-4-8
>> >> Assisted-by: Codex:gpt-5
>> >> Signed-off-by: Zi Yan <ziy@nvidia.com>
>> >> To: Minchan Kim <minchan@kernel.org>
>> >> To: Sergey Senozhatsky <senozhatsky@chromium.org>
>> >> To: Andrew Morton <akpm@linux-foundation.org>
>> >> Cc: linux-mm@kvack.org
>> >> Cc: linux-kernel@vger.kernel.org
>> >> ---
>> >>  mm/zpdesc.h   |  2 +-
>> >>  mm/zsmalloc.c | 15 +++------------
>> >>  2 files changed, 4 insertions(+), 13 deletions(-)
>> >> 
>> >> diff --git a/mm/zpdesc.h b/mm/zpdesc.h
>> >> index b8258dc78548d..4fd81c2e80769 100644
>> >> --- a/mm/zpdesc.h
>> >> +++ b/mm/zpdesc.h
>> >> @@ -26,8 +26,8 @@
>> >>   * with memcg_data.
>> >>   *
>> >>   * Page flags used:
>> >> - * * PG_private identifies the first component page.
>> >>   * * PG_locked is used by page migration code.
>> >> + * The first component page has zpdesc->zspage->first_zpdesc == zpdesc
>> >>   */
>> >>  struct zpdesc {
>> >>  	unsigned long flags;
>> >> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
>> >> index 8204b76f78308..e8ef227624efa 100644
>> >> --- a/mm/zsmalloc.c
>> >> +++ b/mm/zsmalloc.c
>> >> @@ -290,11 +290,6 @@ struct zs_pool {
>> >>  	atomic_t compaction_in_progress;
>> >>  };
>> >>  
>> >> -static inline void zpdesc_set_first(struct zpdesc *zpdesc)
>> >> -{
>> >> -	SetPagePrivate(zpdesc_page(zpdesc));
>> >> -}
>> >> -
>> >>  static inline void zpdesc_inc_zone_page_state(struct zpdesc *zpdesc)
>> >>  {
>> >>  	inc_zone_page_state(zpdesc_page(zpdesc), NR_ZSPAGES);
>> >> @@ -478,7 +473,7 @@ static void record_obj(unsigned long handle, unsigned long obj)
>> >>  
>> >>  static inline bool __maybe_unused is_first_zpdesc(struct zpdesc *zpdesc)
>> >>  {
>> >> -	return PagePrivate(zpdesc_page(zpdesc));
>> >> +	return zpdesc->zspage->first_zpdesc == zpdesc;
>> >>  }
>> >
>> > There are two checks: get_first_zpdesc() and obj_allocated().
>> >
>> > static struct zpdesc *get_first_zpdesc(struct zspage *zspage)
>> > {
>> > 	struct zpdesc *first_zpdesc = zspage->first_zpdesc;
>> >
>> > 	VM_BUG_ON_PAGE(is_first_zpdesc(first_zpdesc), zpdesc_page(first_zpdesc));
>> > 	return first_zpdesc;
>> > }
>> >
>> > If you expand the helper, this seems kind of pointless now:
>> >
>> > 	first_zpdesc = zspage->first_zpdesc;
>> > 	VM_BUG_ON_PAGE(first_zpdesc != first_zpdesc->zspage->first_zpdesc, ...);
>> >
>> > Mayyybe it could make sense to assert first_zpdesc->zspage !=
>> > zspage. But that's a separate issue that the previous check didn't
>> 
>> Usama has the same comment about this.
>> 
>> > necessarily catch. And might not be worth checking, considering how
>> > trivial create_page_chain() is.
>> >
>> > In any case, it doesn't seem worth keeping the check as-is.
>> >
>> > And with one caller remaining, you could delete the helper and inline
>> > that expression into the check in obj_allocated(). What it does now is
>> > self-explanatory; it doesn't need another name like that PagePrivate()
>> > check before did.
>> 
>> How about the version below? Basically, I made is_first_zpdesc() more
>> straightforward for backpointer checking and first_zpdesc checking.
>> 
>> 1. get_first_zpdesc() needs the backpointer check; the first_zpdesc check is
>> meaningless, since the assignment is done above.
>> 
>> 2. obj_allocated() needs the first_zpdesc check; the backpointer check
>> is meaningless, since the zspage is from get_zspage().
>
> Personally, I'm not a fan of "super predicates" where individual
> conditions are only useful for only some of the callsites. They tend
> to become obstacles to understanding the code and lead to subtle bugs
> when developers misunderstand context requirements.
>
> IMO it's better to just precisely express what each callsite
> needs. Only factor a common helper if it's actually the same.

OK. The below is what I come up with like you suggested. I realize that
there are actually two things to check for a zspage chain:

1. all zpdescs point to the same zspage,
2. for a ZsHugePage, handle is only in the first zpdesc.

get_first_zpdesc() checks 1 and obj_allocated() checks 2. So I added
comments to them.


From 4567d7d9cc8c84d5c05de92ee8905f4c5e5cf67f Mon Sep 17 00:00:00 2001
From: Zi Yan <ziy@nvidia.com>
Date: Tue, 28 Jul 2026 22:41:23 -0400
Subject: [PATCH] mm/zsmalloc: replace PG_private with pointer comparison

zsmalloc uses PG_private to indicate first zpdesc in a zspage chain. It is
equivalent to check zpdesc == zspage->first_zpdesc. Replace
is_first_zpdesc() with zpdesc == zspage->first_zpdesc in obj_allocated().

For get_first_zpdesc(), first_zpdesc is from zspage->first_zpdesc, so
replace is_first_zpdesc() with first_zpdesc->zspage == zspage, the second
requirement of a zspage chain, where all zpdescs point to the same zspage.

is_first_zpdesc(), is only used in VM_BUG_ON_PAGE(), so performance impact
should be negligible. While at it, change VM_BUG_ON() to
VM_WARN_ON_ONCE_PAGE().

It prepares for a future commit that remove PG_private.

No functional change intended.

Assisted-by: Claude:claude-opus-4-8
Assisted-by: Codex:gpt-5
Signed-off-by: Zi Yan <ziy@nvidia.com>
To: Minchan Kim <minchan@kernel.org>
To: Sergey Senozhatsky <senozhatsky@chromium.org>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 mm/zpdesc.h   |  2 +-
 mm/zsmalloc.c | 24 ++++++------------------
 2 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/mm/zpdesc.h b/mm/zpdesc.h
index b8258dc78548d..4fd81c2e80769 100644
--- a/mm/zpdesc.h
+++ b/mm/zpdesc.h
@@ -26,8 +26,8 @@
  * with memcg_data.
  *
  * Page flags used:
- * * PG_private identifies the first component page.
  * * PG_locked is used by page migration code.
+ * The first component page has zpdesc->zspage->first_zpdesc == zpdesc
  */
 struct zpdesc {
 	unsigned long flags;
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 8204b76f78308..b87679757e7b1 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -290,11 +290,6 @@ struct zs_pool {
 	atomic_t compaction_in_progress;
 };
 
-static inline void zpdesc_set_first(struct zpdesc *zpdesc)
-{
-	SetPagePrivate(zpdesc_page(zpdesc));
-}
-
 static inline void zpdesc_inc_zone_page_state(struct zpdesc *zpdesc)
 {
 	inc_zone_page_state(zpdesc_page(zpdesc), NR_ZSPAGES);
@@ -476,11 +471,6 @@ static void record_obj(unsigned long handle, unsigned long obj)
 	WRITE_ONCE(*(unsigned long *)handle, obj);
 }
 
-static inline bool __maybe_unused is_first_zpdesc(struct zpdesc *zpdesc)
-{
-	return PagePrivate(zpdesc_page(zpdesc));
-}
-
 /* Protected by class->lock */
 static inline int get_zspage_inuse(struct zspage *zspage)
 {
@@ -496,7 +486,8 @@ static struct zpdesc *get_first_zpdesc(struct zspage *zspage)
 {
 	struct zpdesc *first_zpdesc = zspage->first_zpdesc;
 
-	VM_BUG_ON_PAGE(!is_first_zpdesc(first_zpdesc), zpdesc_page(first_zpdesc));
+	/* the first zpdesc must point back to this zspage */
+	VM_WARN_ON_ONCE_PAGE(first_zpdesc->zspage != zspage, zpdesc_page(first_zpdesc));
 	return first_zpdesc;
 }
 
@@ -833,7 +824,8 @@ static inline bool obj_allocated(struct zpdesc *zpdesc, void *obj,
 	struct zspage *zspage = get_zspage(zpdesc);
 
 	if (unlikely(ZsHugePage(zspage))) {
-		VM_BUG_ON_PAGE(!is_first_zpdesc(zpdesc), zpdesc_page(zpdesc));
+		/* only first zpdesc holds the handle */
+		VM_WARN_ON_ONCE_PAGE(zspage->first_zpdesc != zpdesc, zpdesc_page(zpdesc));
 		handle = zpdesc->handle;
 	} else
 		handle = *(unsigned long *)obj;
@@ -848,9 +840,6 @@ static inline bool obj_allocated(struct zpdesc *zpdesc, void *obj,
 
 static void reset_zpdesc(struct zpdesc *zpdesc)
 {
-	struct page *page = zpdesc_page(zpdesc);
-
-	ClearPagePrivate(page);
 	zpdesc->zspage = NULL;
 	zpdesc->next = NULL;
 	/* PageZsmalloc is sticky until the page is freed to the buddy. */
@@ -1001,8 +990,8 @@ static void create_page_chain(struct size_class *class, struct zspage *zspage,
 	 * 1. all pages are linked together using zpdesc->next
 	 * 2. each sub-page point to zspage using zpdesc->zspage
 	 *
-	 * we set PG_private to identify the first zpdesc (i.e. no other zpdesc
-	 * has this flag set).
+	 * The first zpdesc has its zspage->first_zpdesc set to itself, no
+	 * other zpdesc has this set.
 	 */
 	for (i = 0; i < nr_zpdescs; i++) {
 		zpdesc = zpdescs[i];
@@ -1010,7 +999,6 @@ static void create_page_chain(struct size_class *class, struct zspage *zspage,
 		zpdesc->next = NULL;
 		if (i == 0) {
 			zspage->first_zpdesc = zpdesc;
-			zpdesc_set_first(zpdesc);
 			if (unlikely(class->objs_per_zspage == 1 &&
 					class->pages_per_zspage == 1))
 				SetZsHugePage(zspage);
-- 
2.53.0




-- 
Best Regards,
Yan, Zi