On Thu, Dec 05, 2024 at 05:49:58PM +0000, Matthew Wilcox (Oracle) wrote:
> From: Alex Shi <alexs@xxxxxxxxxx>
>
> After the page to zpdesc conversion, there still left few comments or
> function named with page not zpdesc, let's update the comments and
> rename function create_page_chain() as create_zpdesc_chain().
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).
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.
(...Or I might be thinking about this the wrong way)
> Signed-off-by: Alex Shi <alexs@xxxxxxxxxx>
> ---
> mm/zsmalloc.c | 61 ++++++++++++++++++++++++++-------------------------
> 1 file changed, 31 insertions(+), 30 deletions(-)
>
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index c0e7c055847a..1f5ff0fdeb42 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -15,20 +15,19 @@
>
> /*
> * 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.
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -194,7 +193,10 @@ struct size_class {
> */
> int size;
> int objs_per_zspage;
> - /* Number of PAGE_SIZE sized pages to combine to form a 'zspage' */
> + /*
> + * Number of PAGE_SIZE sized zpdescs/pages to combine to
> + * form a 'zspage'
> + */
> int pages_per_zspage;
>
> unsigned int index;
> @@ -908,7 +910,7 @@ static void free_zspage(struct zs_pool *pool, struct size_class *class,
>
> /*
> * Since zs_free couldn't be sleepable, this function cannot call
> - * lock_page. The page locks trylock_zspage got will be released
> + * lock_page. The zpdesc locks trylock_zspage got will be released
> * by __free_zspage.
> */
> if (!trylock_zspage(zspage)) {
> @@ -965,7 +967,7 @@ static void init_zspage(struct size_class *class, struct zspage *zspage)
> set_freeobj(zspage, 0);
> }
>
> -static void create_page_chain(struct size_class *class, struct zspage *zspage,
> +static void create_zpdesc_chain(struct size_class *class, struct zspage *zspage,
> struct zpdesc *zpdescs[])
> {
> int i;
> @@ -974,9 +976,9 @@ static void create_page_chain(struct size_class *class, struct zspage *zspage,
> int nr_zpdescs = class->pages_per_zspage;
>
> /*
> - * Allocate individual pages and link them together as:
> - * 1. all pages are linked together using zpdesc->next
> - * 2. each sub-page point to zspage using zpdesc->zspage
> + * Allocate individual zpdescs and link them together as:
> + * 1. all zpdescs are linked together using zpdesc->next
> + * 2. each sub-zpdesc point to zspage using zpdesc->zspage
> *
> * we set PG_private to identify the first zpdesc (i.e. no other zpdesc
> * has this flag set).
> @@ -1034,7 +1036,7 @@ static struct zspage *alloc_zspage(struct zs_pool *pool,
> zpdescs[i] = zpdesc;
> }
>
> - create_page_chain(class, zspage, zpdescs);
> + create_zpdesc_chain(class, zspage, zpdescs);
> init_zspage(class, zspage);
> zspage->pool = pool;
> zspage->class = class->index;
> @@ -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.
> kunmap_local(vaddr);
> @@ -1441,7 +1443,6 @@ static void obj_free(int class_size, unsigned long obj)
> unsigned int f_objidx;
> void *vaddr;
>
> -
> obj_to_location(obj, &f_zpdesc, &f_objidx);
> f_offset = offset_in_page(class_size * f_objidx);
> zspage = get_zspage(f_zpdesc);
> @@ -1684,19 +1685,19 @@ static int putback_zspage(struct size_class *class, struct zspage *zspage)
> #ifdef CONFIG_COMPACTION
> /*
> * To prevent zspage destroy during migration, zspage freeing should
> - * hold locks of all pages in the zspage.
> + * hold locks of all component zpdesc in the zspage.
> */
> static void lock_zspage(struct zspage *zspage)
> {
> struct zpdesc *curr_zpdesc, *zpdesc;
>
> /*
> - * Pages we haven't locked yet can be migrated off the list while we're
> + * Zpdesc we haven't locked yet can be migrated off the list while we're
> * trying to lock them, so we need to be careful and only attempt to
> - * lock each page under migrate_read_lock(). Otherwise, the page we lock
> - * may no longer belong to the zspage. This means that we may wait for
> - * the wrong page to unlock, so we must take a reference to the page
> - * prior to waiting for it to unlock outside migrate_read_lock().
> + * lock each zpdesc under migrate_read_lock(). Otherwise, the zpdesc we
> + * lock may no longer belong to the zspage. This means that we may wait
> + * for the wrong zpdesc to unlock, so we must take a reference to the
> + * zpdesc prior to waiting for it to unlock outside migrate_read_lock().
> */
> while (1) {
> migrate_read_lock(zspage);
> @@ -1771,7 +1772,7 @@ static void replace_sub_page(struct size_class *class, struct zspage *zspage,
> idx++;
> } while ((zpdesc = get_next_zpdesc(zpdesc)) != NULL);
>
> - create_page_chain(class, zspage, zpdescs);
> + create_zpdesc_chain(class, zspage, zpdescs);
> first_obj_offset = get_first_obj_offset(oldzpdesc);
> set_first_obj_offset(newzpdesc, first_obj_offset);
> if (unlikely(ZsHugePage(zspage)))
> @@ -1782,8 +1783,8 @@ static void replace_sub_page(struct size_class *class, struct zspage *zspage,
> static bool zs_page_isolate(struct page *page, isolate_mode_t mode)
> {
> /*
> - * Page is locked so zspage couldn't be destroyed. For detail, look at
> - * lock_zspage in free_zspage.
> + * Page/zpdesc is locked so zspage couldn't be destroyed. For detail,
> + * look at lock_zspage in free_zspage.
> */
> VM_BUG_ON_PAGE(PageIsolated(page), page);
>
> @@ -1810,7 +1811,7 @@ static int zs_page_migrate(struct page *newpage, struct page *page,
> /* We're committed, tell the world that this is a Zsmalloc page. */
> __zpdesc_set_zsmalloc(newzpdesc);
>
> - /* The page is locked, so this pointer must remain valid */
> + /* The zpdesc/page is locked, so this pointer must remain valid */
> zspage = get_zspage(zpdesc);
> pool = zspage->pool;
>
> @@ -1883,7 +1884,7 @@ static const struct movable_operations zsmalloc_mops = {
> };
>
> /*
> - * Caller should hold page_lock of all pages in the zspage
> + * Caller should hold zpdesc locks of all in the zspage
> * In here, we cannot use zspage meta data.
> */
> static void async_free_zspage(struct work_struct *work)
> --
> 2.45.2
>
>
