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

[Zi Yan]

On Tue Sep 8, 2026 at 11:07 AM EDT, David Hildenbrand (Arm) wrote:
> On 9/8/26 04:56, Zi Yan wrote:
>> 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
>
> Assied-by: LLM :)
>
>> To: Minchan Kim <minchan@xxxxxxxxxx>
>> To: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
>> To: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: linux-mm@xxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Acked-by: Johannes Weiner <hannes@xxxxxxxxxxx>
>> Signed-off-by: Zi Yan <ziy@xxxxxxxxxx>
>> ---
>
> Nice cleanup, one comment below.
>
> Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

Thanks.
>
>>  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 825022a7a328f..24526185fd31f 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));
>> -}
>
> I would keep this helper, reworking it and moving all comments regarding *how*
> first detection works in there
>
> -static inline bool __maybe_unused is_first_zpdesc(struct zpdesc *zpdesc)
> +static inline bool __maybe_unused is_first_zpdesc(const struct zpdesc *zpdesc)
>  {
> -       return PagePrivate(zpdesc_page(zpdesc));
> +       /* Only the first zpdesc has its zspage->first_zpdesc set to itself. */
> +       return zpdesc == zpdesc->zspage->first_zpdesc;
>  }

That is the version I had in RFC[1]. But how it works is not only
1) "Only the first zpdesc has its zspage->first_zpdesc set to itself.", but also
2) all zpdescs point to the same zspage.

obj_allocated() checks for 1, since zspage comes from zspage and
the requirment 2 is tautology.

get_first_zpdesc() checks for 2, since first_zpdesc comes from zspage's
first_zpdesc, another tautology.

I also had a version to spell out both[2], but Johannes did not lik it,
because it[2] is a "super predicates", making the required checks unclear.


[1] https://lore.kernel.org/all/20260731-remove-pg_private-v1-1-142c97ba3562@xxxxxxxxxx/
[2] https://lore.kernel.org/all/DKFF32LRYH32.16JIXI4LUS5E9@xxxxxxxxxx/


-- 
Best Regards,
Yan, Zi