Re: [PATCH v3 6/6] mm/zswap: directly use percpu mutex and buffer in load/store

[Chengming Zhou <zhouchengming@bytedance.com>]

On 2023/12/21 08:19, Yosry Ahmed wrote:
> On Wed, Dec 20, 2023 at 4:20 AM Chengming Zhou
> <zhouchengming@bytedance.com> wrote:
>>
>> On 2023/12/20 05:39, Yosry Ahmed wrote:
>>> On Tue, Dec 19, 2023 at 10:43 AM Nhat Pham <nphamcs@gmail.com> wrote:
>>>>
>>>> On Tue, Dec 19, 2023 at 5:29 AM Chris Li <chrisl@kernel.org> wrote:
>>>>>
>>>>> Hi Chengming and Yosry,
>>>>>
>>>>> On Mon, Dec 18, 2023 at 3:50 AM Chengming Zhou
>>>>> <zhouchengming@bytedance.com> wrote:
>>>>>>
>>>>>> Since the introduce of reusing the dstmem in the load path, it seems
>>>>>> confusing that we are now using acomp_ctx->dstmem and acomp_ctx->mutex
>>>>>> now for purposes other than what the naming suggests.
>>>>>>
>>>>>> Yosry suggested 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.
>>>>>>
>>>>>> So this patch remove dstmem and mutex from acomp_ctx, and rename the
>>>>>> zswap_dstmem to zswap_buffer, using the percpu mutex and buffer on
>>>>>> the load and store paths.
>>>>>
>>>>> Sorry joining this discussion late.
>>>>>
>>>>> I get the rename of "dstmem" to "buffer". Because the buffer is used
>>>>> for both load and store as well. What I don't get is that, why do we
>>>>> move it out of the acomp_ctx struct. Now we have 3 per cpu entry:
>>>>> buffer, mutex and acomp_ctx. I think we should do the reverse, fold
>>>>> this three per cpu entry into one struct the acomp_ctx. Each per_cpu
>>>>> load() has a sequence of dance around the cpu id and disable preempt
>>>>> etc, while each of the struct member load is just a plan memory load.
>>>>> It seems to me it would be more optimal to combine this three per cpu
>>>>> entry into acomp_ctx. Just do the per cpu for the acomp_ctx once.
>>>>
>>>> I agree with Chris. From a practicality POV, what Chris says here
>>>> makes sense. From a semantic POV, this buffer is only used in
>>>> (de)compression contexts - be it in store, load, or writeback - so it
>>>> belonging to the orignal struct still makes sense to me. Why separate
>>>> it out, without any benefits. Just rename the old field buffer or
>>>> zswap_buffer and call it a day? It will be a smaller patch too!
>>>>
>>>
>>> My main concern is that the struct name is specific for the crypto
>>> acomp stuff, but that buffer and mutex are not.
>>> How about we keep it in the struct, but refactor the struct as follows:
>>>
>>> struct zswap_ctx {
>>>     struct {
>>>         struct crypto_acomp *acomp;
>>>         struct acomp_req *req;
>>>         struct crypto_wait wait;
>>>     }  acomp_ctx;
>>>     u8 *dstmem;
>>>     struct mutex *mutex;
>>> };
>>>
>>> , and then rename zswap_pool.acomp_ctx to zswap_pool.ctx?
>>
>> I think there are two viewpoints here, both works ok to me.
>>
>> The first is from ownship or lifetime, these percpu mutex and dstmem
>> are shared between all pools, so no one pool own the mutex and dstmem,
>> nor does the percpu acomp_ctx in each pool.
>>
>> The second is from usage, these percpu mutex and dstmem are used by
>> the percpu acomp_ctx in each pool, so it's easy to use to put pointers
>> to them in the percpu acomp_ctx.
>>
>> Actually I think it's simpler to let the percpu acomp_ctx has its own
>> mutex and dstmem, which in fact are the necessary parts when it use
>> the acomp interfaces.
>>
>> This way, we could delete the percpu mutex and dstmem, and its hotplugs,
>> and not shared anymore between all pools. Maybe we would have many pools
>> at the same time in the future, like different compression algorithm or
>> different zpool for different memcg workloads. Who knows? :-)
>>
>> So how about this patch below? Just RFC.
> 
> The general approach looks fine to me, although I still prefer we
> reorganize the struct as Chris and I discussed: rename
> crypto_acomp_ctx to a generic name, add a (anonymous) struct for the
> crypto_acomp stuff, rename dstmem to buffer or so.
> 
> I think we can also make the mutex a static part of the struct, any
> advantage to dynamically allocating it?

Agree, it seems no much advantage to me, I can change to a static part.
As for the restructure, I have no strong opinion about it, maybe it's
better for me to leave it as it is.

Thanks!