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

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

On Sat Aug 1, 2026 at 10:12 AM EDT, Usama Arif wrote:
> On Fri, 31 Jul 2026 22:13:24 -0400 Zi Yan <ziy@nvidia.com> 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(-)
>> 
>
> create_page_chain() sets zpdesc->zspage = zspage before assigning zspage->first_zpdesc,
> so LGTM.
>
> I think the assertion in get_first_zpdesc()
> should be changed to
>
> VM_BUG_ON_PAGE(first_zpdesc->zspage != zspage, zpdesc_page(first_zpdesc));
>
> in the current check, we are checking
>
> zspage->first_zpdesc->zspage->first_zpdesc == zspage->first_zpdesc
>
> which is just testing the backpointer.

Got it. Thank you for pointing this out.

is_first_zpdesc() is also used in obj_allocated() for the same check and
getting rid of it in get_first_zpdesc() causes inconsistency.

How about the changes below on top of this patch? is_first_zpdesc() can
be used for both sites and looks cleaner. And VM_BUG_ON_PAGE is replaced
with VM_WARN_ON_ONCE_PAGE.


diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index e8ef227624efa..f021d2df99404 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -471,9 +471,10 @@ 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)
+static inline bool __maybe_unused is_first_zpdesc(struct zpdesc *zpdesc,
+						  struct zspage *zspage)
 {
-	return zpdesc->zspage->first_zpdesc == zpdesc;
+	return zpdesc->zspage == zspage && zspage->first_zpdesc == zpdesc;
 }
 
 /* Protected by class->lock */
@@ -491,7 +492,7 @@ 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));
+	VM_WARN_ON_ONCE_PAGE(is_first_zpdesc(first_zpdesc, zspage), zpdesc_page(first_zpdesc));
 	return first_zpdesc;
 }
 
@@ -828,7 +829,7 @@ 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));
+		VM_WARN_ON_ONCE_PAGE(is_first_zpdesc(zpdesc, zspage), zpdesc_page(zpdesc));
 		handle = zpdesc->handle;
 	} else
 		handle = *(unsigned long *)obj;

>
> With the above VM_BUG_ON check change, please feel free to add:
>
> Acked-by: Usama Arif <usama.arif@linux.dev>
>
>
>> 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;
>>  }
>>  
>>  /* Protected by class->lock */
>> @@ -848,9 +843,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 +993,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 +1002,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