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

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 <[email protected]>
> >> To: Minchan Kim <[email protected]>
> >> To: Sergey Senozhatsky <[email protected]>
> >> To: Andrew Morton <[email protected]>
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> ---
> >>  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.