Re: [PATCH 01/20] mm/zsmalloc: add zpdesc memory descriptor for zswap.zpool

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

On Fri, Jun 28, 2024 at 12:06 PM <alexs@kernel.org> wrote:
>
> From: Alex Shi <alexs@kernel.org>
>
> The 1st patch introduces new memory decriptor zpdesc and rename
> zspage.first_page to zspage.first_zpdesc, no functional change.
>
> Originally-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> Signed-off-by: Alex Shi <alexs@kernel.org>
> ---
>  mm/zpdesc.h   | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  mm/zsmalloc.c | 19 ++++++++---------
>  2 files changed, 66 insertions(+), 9 deletions(-)
>  create mode 100644 mm/zpdesc.h

Hi Alex, thanks for your effort in pushing this forward!

> diff --git a/mm/zpdesc.h b/mm/zpdesc.h
> new file mode 100644
> index 000000000000..a1ab5ebaa936
> --- /dev/null
> +++ b/mm/zpdesc.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* zpdesc.h: zswap.zpool memory descriptor
> + *
> + * Written by Alex Shi <alexs@kernel.org>
> + *           Hyeonggon Yoo <42.hyeyoo@gmail.com>
> + */
> +#ifndef __MM_ZPDESC_H__
> +#define __MM_ZPDESC_H__
> +
> +/*
> + * struct zpdesc -     Memory descriptor for zpool memory, now is for zsmalloc
> + * @flags:             Page flags, PG_private: identifies the first component page
> + * @lru:               Indirected used by page migration

maybe Indirected -> Indirectly?

> + * @next:              Next zpdesc in a zspage in zsmalloc zpool
> + * @handle:            For huge zspage in zsmalloc zpool
> + * @zspage:            Pointer to zspage in zsmalloc
> + *
> + * This struct overlays struct page for now. Do not modify without a good
> + * understanding of the issues.
> + */
> +struct zpdesc {
> +       unsigned long flags;
> +       struct list_head lru;
> +       unsigned long _zp_pad_1;

for understanding, I think it'd be better to replace _zp_pad_1 with movable ops,
because mops reuses this 'mapping' field.

> +       union {
> +               /* Next zpdescs in a zspage in zsmalloc zpool */
> +               struct zpdesc *next;
> +               /* For huge zspage in zsmalloc zpool */
> +               unsigned long handle;
> +       };
> +       struct zspage *zspage;

There was a discussion with Yosry on including memcg_data on zpdesc
even if it's not used at the moment.

Maybe you can look at:
https://lore.kernel.org/linux-mm/CAB=+i9Quz9iP2-Lq=oQfKVVnzPDtOaKMm=hUPbnRg5hRxH+qaA@mail.gmail.com/

> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index fec1a39e5bbe..67bb80b7413a 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -13,17 +13,17 @@
>
>  /*
>   * 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 pages of a zspage
>   *             For the huge page, this is always 0, so we use this field
>   *             to store handle.
>   *     page->page_type: PG_zsmalloc, lower 16 bit locate the first object
>   *             offset in a subpage of a zspage
>   *
> - * Usage of struct page flags:
> + * Usage of struct zpdesc(page) flags:
>   *     PG_private: identifies the first component page
>   *     PG_owner_priv_1: identifies the huge component page

the comment for PG_owner_priv_1 can safely be removed as it's not used
after commit a41ec880aa7b ("zsmalloc: move huge compressed obj from
page to zspage")

> @@ -948,7 +949,7 @@ static void create_page_chain(struct size_class *class, struct zspage *zspage,
>                 set_page_private(page, (unsigned long)zspage);
>                 page->index = 0;
>                 if (i == 0) {
> -                       zspage->first_page = page;
> +                       zspage->first_zpdesc = page_zpdesc(page);
>                         SetPagePrivate(page);
>                         if (unlikely(class->objs_per_zspage == 1 &&
>                                         class->pages_per_zspage == 1))
> @@ -1325,7 +1326,7 @@ static unsigned long obj_malloc(struct zs_pool *pool,
>                 link->handle = handle;
>         else
>                 /* record handle to page->index */
> -               zspage->first_page->index = handle;
> +               zspage->first_zpdesc->handle = handle;

FYI this line seems to conflict with
bcc6116e39f512 ("mm/zsmalloc: move record_obj() into obj_malloc()")
on mm-unstable.

Best,
Hyeonggon