Received-SPF: pass (google.com: domain of linux-kernel+bounces-3134-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <d45d1879-bbc0-4d74-b34f-92f9753d1a30@bytedance.com>
Date: Mon, 18 Dec 2023 16:06:36 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/5] mm/zswap: reuse dstmem when decompress
To: Yosry Ahmed <yosryahmed@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>, Nhat Pham <nphamcs@gmail.com>,
 Chris Li <chriscli@google.com>, Johannes Weiner <hannes@cmpxchg.org>,
 Seth Jennings <sjenning@redhat.com>, Dan Streetman <ddstreet@ieee.org>,
 Vitaly Wool <vitaly.wool@konsulko.com>, linux-kernel@vger.kernel.org,
 linux-mm@kvack.org
References: <20231213-zswap-dstmem-v1-0-896763369d04@bytedance.com>
 <20231213-zswap-dstmem-v1-1-896763369d04@bytedance.com>
 <CAJD7tkbicqevYuJbTUZBQUo2n_k-HKTixG5TbXGtdbtFJgFmfg@mail.gmail.com>
 <8f80fe3e-a8c7-463d-896b-99575c362839@bytedance.com>
 <CAJD7tkbS_3eEDbfo-w1Z+tuY44YKMYnAqaL0684Y6nh2EmHq+Q@mail.gmail.com>
 <dc84f965-9174-4ce7-aedf-f68d8e8003b4@bytedance.com>
 <CAJD7tkYdisUg+wphANLFcBSLB13jH+NxqFdc-EjKBq0H-=S00Q@mail.gmail.com>
Content-Language: en-US
From: Chengming Zhou <zhouchengming@bytedance.com>
In-Reply-To: <CAJD7tkYdisUg+wphANLFcBSLB13jH+NxqFdc-EjKBq0H-=S00Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 2023/12/15 02:24, Yosry Ahmed wrote:
> On Thu, Dec 14, 2023 at 6:42 AM Chengming Zhou
> <zhouchengming@bytedance.com> wrote:
> [..]
>>>>>> -
>>>>>>         /* 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);
> 
> Renaming to zswap_buffer or zswap_mem would be better I think, but
> yeah what I had in mind is having zswap_mutex and
> zswap_[dstmem/mem/buffer] be generic percpu buffers that are used by
> store and load paths for different purposes, not directly linked to
> acomp_ctx.
> 

Ok, I'll append a patch to do the refactor & cleanup: remove mutex
and dstmem from acomp_ctx, and rename to zswap_buffer, then directly
use them on the load/store paths.

>> 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.
> 
> You mean have two separate set of percpu buffers for zswap load &
> stores paths? This is probably unnecessary.

Alright. Thanks.