On Wed, Dec 11, 2024 at 1:37 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> 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
