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

[David Hildenbrand (Arm)]

On 9/8/26 17:21, Zi Yan wrote:
> 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

Ah, great minds think alike.

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

yeah, agreed.

> because it[2] is a "super predicates", making the required checks unclear.

Oh well :)

Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

-- 
Cheers,

David