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

[Zi Yan]

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@xxxxxxxxxx>
>> >> To: Minchan Kim <minchan@xxxxxxxxxx>
>> >> To: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
>> >> To: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> >> Cc: linux-mm@xxxxxxxxx
>> >> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> >> ---
>> >>  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.