On 2023/12/14 21:32, Yosry Ahmed wrote:
> On Thu, Dec 14, 2023 at 5:29 AM Chengming Zhou
> <zhouchengming@xxxxxxxxxxxxx> wrote:
>>
>> On 2023/12/14 07:24, Yosry Ahmed wrote:
>>> On Tue, Dec 12, 2023 at 8:18 PM Chengming Zhou
>>> <zhouchengming@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>>>> Reviewed-by: Nhat Pham <nphamcs@xxxxxxxxx>
>>>> ---
>>>> 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.
