Re: [PATCH 1/5] mm/zswap: reuse dstmem when decompress

[Chengming Zhou <zhouchengming@bytedance.com>]

On 2023/12/14 21:32, Yosry Ahmed wrote:
> On Thu, Dec 14, 2023 at 5:29 AM Chengming Zhou
> <zhouchengming@bytedance.com> wrote:
>>
>> On 2023/12/14 07:24, Yosry Ahmed wrote:
>>> On Tue, Dec 12, 2023 at 8:18 PM Chengming Zhou
>>> <zhouchengming@bytedance.com> wrote:
>>>>
>>>> In the !zpool_can_sleep_mapped() case such as zsmalloc, we need to first
>>>> copy the entry->handle memory to a temporary memory, which is allocated
>>>> using kmalloc.
>>>>
>>>> Obviously we can reuse the per-compressor dstmem to avoid allocating
>>>> every time, since it's percpu-compressor and protected in mutex.
>>>>
>>>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
>>>> Reviewed-by: Nhat Pham <nphamcs@gmail.com>
>>>> ---
>>>>  mm/zswap.c | 29 +++++++++--------------------
>>>>  1 file changed, 9 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/mm/zswap.c b/mm/zswap.c
>>>> index 7ee54a3d8281..edb8b45ed5a1 100644
>>>> --- a/mm/zswap.c
>>>> +++ b/mm/zswap.c
>>>> @@ -1772,9 +1772,9 @@ bool zswap_load(struct folio *folio)
>>>>         struct zswap_entry *entry;
>>>>         struct scatterlist input, output;
>>>>         struct crypto_acomp_ctx *acomp_ctx;
>>>> -       u8 *src, *dst, *tmp;
>>>> +       unsigned int dlen = PAGE_SIZE;
>>>> +       u8 *src, *dst;
>>>>         struct zpool *zpool;
>>>> -       unsigned int dlen;
>>>>         bool ret;
>>>>
>>>>         VM_WARN_ON_ONCE(!folio_test_locked(folio));
>>>> @@ -1796,27 +1796,18 @@ bool zswap_load(struct folio *folio)
>>>>                 goto stats;
>>>>         }
>>>>
>>>> -       zpool = zswap_find_zpool(entry);
>>>> -       if (!zpool_can_sleep_mapped(zpool)) {
>>>> -               tmp = kmalloc(entry->length, GFP_KERNEL);
>>>> -               if (!tmp) {
>>>> -                       ret = false;
>>>> -                       goto freeentry;
>>>> -               }
>>>> -       }
>>>> -
>>>>         /* decompress */
>>>> -       dlen = PAGE_SIZE;
>>>> -       src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
>>>> +       acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
>>>> +       mutex_lock(acomp_ctx->mutex);
>>>>
>>>> +       zpool = zswap_find_zpool(entry);
>>>> +       src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
>>>>         if (!zpool_can_sleep_mapped(zpool)) {
>>>> -               memcpy(tmp, src, entry->length);
>>>> -               src = tmp;
>>>> +               memcpy(acomp_ctx->dstmem, src, entry->length);
>>>> +               src = acomp_ctx->dstmem;
>>>
>>> I don't like that we are now using acomp_ctx->dstmem and
>>> acomp_ctx->mutex now for purposes other than what the naming suggests.
>>
>> The "mutex" name is coherent, "dstmem" depends on how we use IMHO.
>> Change to just "mem"? Or do you have a better name to replace?
>>
>>>
>>> How about removing these two fields from acomp_ctx, and directly using
>>> zswap_dstmem and zswap_mutex in both the load and store paths, rename
>>> them, and add proper comments above their definitions that they are
>>> for generic percpu buffering on the load and store paths?
>>
>> Yes, they are percpu memory and lock, but they are used by per acomp_ctx,
>> and the cpu maybe changing in the middle, so maybe better to keep them.
> 
> I don't mean to remove completely. Keep them as (for example)
> zswap_mem and zswap_mutex global percpu variables, and not have
> pointers in acomp_ctx to them. Instead of using acomp_ctx->dstmem
> today, we directly use the global zswap_mem (same for the mutex).
> 
> This makes it clear that the buffers are not owned or exclusively used
> by the acomp_ctx. WDYT?

Does this look good to you?

```
int cpu = raw_smp_processor_id();

mutex = per_cpu(zswap_mutex, cpu);
mutex_lock(mutex);

dstmem = per_cpu(zswap_dstmem, cpu);
acomp_ctx = per_cpu_ptr(pool->acomp_ctx, cpu);

/* compress or decompress */
```

Another way I just think of is to make acomp_ctx own its lock and buffer,
and we could delete these percpu zswap_mutex and zswap_dstmem instead.

Thanks.