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

[Chengming Zhou]

On 2023/12/7 01:12, Nhat Pham wrote:
> On Wed, Dec 6, 2023 at 1:46 AM Chengming Zhou
> <zhouchengming@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> ---
>>  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