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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAB=+i9RSCkYyqo-qXJNRKjUig7s5ZMGWCKmur1nNcD3KkFnhUg@mail.gmail.com>
Date: Sun, 30 Jun 2024 22:45:33 +0900
From: Hyeonggon Yoo <42.hyeyoo@...il.com>
To: alexs@...nel.org
Cc: Vitaly Wool <vitaly.wool@...sulko.com>, Miaohe Lin <linmiaohe@...wei.com>, 
	Andrew Morton <akpm@...ux-foundation.org>, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org, minchan@...nel.org, willy@...radead.org, 
	senozhatsky@...omium.org, david@...hat.com
Subject: Re: [PATCH 01/20] mm/zsmalloc: add zpdesc memory descriptor for zswap.zpool

On Fri, Jun 28, 2024 at 12:06 PM <alexs@...nel.org> wrote:
>
> From: Alex Shi <alexs@...nel.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@...il.com>
> Signed-off-by: Alex Shi <alexs@...nel.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@...nel.org>
> + *           Hyeonggon Yoo <42.hyeyoo@...il.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