linux-kernel - Re: [PATCH 4/7] mm/zswap: change dstmem size to one page

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: <CAKEwX=M3iYV--kn+TEhLytijAOPH0_077KzvuGBE3+2r7AcW7Q@mail.gmail.com>
Date:   Wed, 6 Dec 2023 09:12:38 -0800
From:   Nhat Pham <nphamcs@...il.com>
To:     Chengming Zhou <zhouchengming@...edance.com>
Cc:     Vitaly Wool <vitaly.wool@...sulko.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>,
        Seth Jennings <sjenning@...hat.com>,
        Dan Streetman <ddstreet@...e.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Yosry Ahmed <yosryahmed@...gle.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/7] mm/zswap: change dstmem size to one page

On Wed, Dec 6, 2023 at 1:46 AM Chengming Zhou
<zhouchengming@...edance.com> wrote:
>
> Maybe I missed something, but the dstmem size of 2 * PAGE_SIZE is
> very confusing, since we only need at most one page when compress,
> and the "dlen" is also PAGE_SIZE in acomp_request_set_params().
>
> So change it to one page, and fix the comments.
>
> Signed-off-by: Chengming Zhou <zhouchengming@...edance.com>
> ---
>  mm/zswap.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index d93a7b58b5af..999671dcb469 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -699,7 +699,7 @@ static int zswap_dstmem_prepare(unsigned int cpu)
>         struct mutex *mutex;
>         u8 *dst;
>
> -       dst = kmalloc_node(PAGE_SIZE * 2, GFP_KERNEL, cpu_to_node(cpu));
> +       dst = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu));
>         if (!dst)
>                 return -ENOMEM;
>
> @@ -1649,8 +1649,7 @@ bool zswap_store(struct folio *folio)
>         sg_init_table(&input, 1);
>         sg_set_page(&input, page, PAGE_SIZE, 0);
>
> -       /* zswap_dstmem is of size (PAGE_SIZE * 2). Reflect same in sg_list */
> -       sg_init_one(&output, dst, PAGE_SIZE * 2);
> +       sg_init_one(&output, dst, PAGE_SIZE);

Hmm. This is very weird. It looks very intentional though, so perhaps
we should consult the maintainer or the original author of this logic
to double check this?
My best guess is for cases where the compression algorithm fails - i.e
the output (header + payload) is somehow bigger than the original
data. But not sure if this happens at all, and if the size > PAGE_SIZE
we don't wanna store the output in zswap anyway.

>         acomp_request_set_params(acomp_ctx->req, &input, &output, PAGE_SIZE, dlen);
>         /*
>          * it maybe looks a little bit silly that we send an asynchronous request,
>
> --
> b4 0.10.1