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

[Usama Arif <usama.arif@linux.dev>]

On 02/08/2026 00:49, Zi Yan wrote:
> 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));

mhmm do you mean 

VM_WARN_ON_ONCE_PAGE(!is_first_zpdesc(first_zpdesc, zspage), zpdesc_page(first_zpdesc)); 

here? ! is missing I think?

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

same here? ! is missing?


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