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. However, locking the zpdesc does make sense -- we're locking the descriptor. > > /* > > * 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. > > @@ -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?
