Re: [PATCH v8 21/21] mm/zsmalloc: update comments for page->zpdesc changes

[Hyeonggon Yoo <42.hyeyoo@gmail.com>]

On Wed, Dec 11, 2024 at 1:37 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Dec 11, 2024 at 12:46:43AM +0900, Hyeonggon Yoo wrote:
> > Talking about updating comments and code by replacing 'page' with 'zpdesc',
> > I'm not sure if it makes sense to replace all instances of 'page' with 'zpdesc'.
> > A zpdesc is a descriptor, not a page that contains data (at least that's
> > what I have been thinking while writing the initial patch series).
>
> Agreed.
>
> > In that context I'm still not sure if saying "a sub-zpdesc of a zspage", "lock zpdesc",
> > or "migrate zpdesc" makes sense because replacing 'page descriptor (aka. struct page)' with
> > 'zpool descriptor (aka. zpdesc)' doesn't mean zsmalloc is throwing away the conecept of pages.
>
> sub-zpdesc is a silly thing to say.  subpage is equally silly.  If it's
> a page, call it a page.

Agreed.

> However, locking the zpdesc does make sense -- we're locking the
> descriptor.

That makes sense.

> > >  /*
> > >   * Following is how we use various fields and flags of underlying
> > > - * struct page(s) to form a zspage.
> > > + * struct zpdesc(page) to form a zspage.
> > >   *
> > > - * Usage of struct page fields:
> > > - * page->private: points to zspage
> > > - * page->index: links together all component pages of a zspage
> > > + * Usage of struct zpdesc fields:
> > > + * zpdesc->zspage: points to zspage
> > > + * zpdesc->next: links together all component zpdescs of a zspage
> > >   *         For the huge page, this is always 0, so we use this field
> > >   *         to store handle.
> > > - * page->page_type: PGTY_zsmalloc, lower 24 bits locate the first object
> > > - *         offset in a subpage of a zspage
> > > - *
> > > - * Usage of struct page flags:
> > > - * PG_private: identifies the first component page
> > > - * PG_owner_priv_1: identifies the huge component page
> > > + * zpdesc->first_obj_offset: PGTY_zsmalloc, lower 24 bits locate the first
> > > + *         object offset in a subpage of a zspage
> > >   *
> > > + * Usage of struct zpdesc(page) flags:
> > > + * PG_private: identifies the first component zpdesc
> > > + * PG_lock: lock all component zpdescs for a zspage free, serialize with
> > >   */
> >
> > I think this comment is unnecessary, as it's already documented in mm/zpdesc.h.
> > It can be removed in patch 01.
>
> Agreed.  I did think about doing that, so if you want to do it too,
> that's two votes for doing it.

Will do in v9.

> > > @@ -1351,7 +1353,7 @@ static unsigned long obj_malloc(struct zs_pool *pool,
> > >             /* record handle in the header of allocated chunk */
> > >             link->handle = handle | OBJ_ALLOCATED_TAG;
> > >     else
> > > -           /* record handle to page->index */
> > > +           /* record handle to zpdesc->handle */
> > >             zspage->first_zpdesc->handle = handle | OBJ_ALLOCATED_TAG;
> >
> > the name of the field is already 'handle',
> > so we don't need a comment to explain it.
>
> Agreed.
>
> Do you want to take on producing v9 or do you want me to fold in your
> suggestions and send it?

I will send v9 with my feedback adjusted this weekend.
Thank you for rebasing and pushing this forward.

Best,
Hyeonggon