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

[Yosry Ahmed]

On Tue, Dec 19, 2023 at 2:49 PM Chris Li <chrisl@xxxxxxxxxx> wrote:
>
> 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;
>  };

I prefer naming the internal struct, but I am fine with an anonymous
struct as well. I just think it's a semantically sound separation.

>
> 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 original struct name makes it seems like the data in the struct is
only used for the crypto acomp API, which is not the case.