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 3:05 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:

> > 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.

Ack.

>
> >
> > 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.

The mutex and buffer are used associated with the crypto acomp API
that is why I think it is fine to stay within the struct as well.
Using the anonymous struct to separate it out is marginally better. I
think we are in agreement here.

Chris