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: <4b864565-faeb-4814-a803-66a1e54e8ae1@bytedance.com>
Date:   Thu, 7 Dec 2023 10:59:40 +0800
From:   Chengming Zhou <zhouchengming@...edance.com>
To:     Nhat Pham <nphamcs@...il.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 2023/12/7 01:12, Nhat Pham wrote:
> 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?

Yes, it's also weird to me. But it seems to be 2 pages when the zswap code
merged in mm 10 years ago. Hope the maintainer or author could shed some
light on it. :)

> 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.
> 

Agree, so I think one page is enough.

>>         acomp_request_set_params(acomp_ctx->req, &input, &output, PAGE_SIZE, dlen);

And here we just use PAGE_SIZE in the parameter actually.

Thanks!

>>         /*
>>          * it maybe looks a little bit silly that we send an asynchronous request,
>>
>> --
>> b4 0.10.1