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

[Chris Li]

Hi Yosry,

On Tue, Dec 19, 2023 at 1:39 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
>
> 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:

If it is the naming of the struct you are not happy about. We can
change the naming.

>
> struct zswap_ctx {
>     struct {
>         struct crypto_acomp *acomp;
>         struct acomp_req *req;
>         struct crypto_wait wait;
>     }  acomp_ctx;
>     u8 *dstmem;
>     struct mutex *mutex;
> };

The compression and decompression requires the buffer and mutex. The
mutex is not used other than compress and decompress, right?
In my mind, they are fine staying in the struct. I am not sure adding
an level acomp_ctx provides anything. It makes access structure
members deeper.

If you care about separating out the crypto acomp,  how about just
remove acomp_ctx and make it an anonymous structure.
struct zswap_comp_ctx {
    struct /* cryto acomp context */ {
        struct crypto_acomp *acomp;
        struct acomp_req *req;
        struct crypto_wait wait;
     };
     u8 *dstmem;
     struct mutex *mutex;
 };

Then we remove other per_cpu_load as well.

I also think the original struct name is fine, we don't need to change
the struct name.  The value/code_change ratio for renaming the struct
alone is low. On the other hand, if we can get rid of some per cpu
load, that value is high enough to justify the patch.

Chris