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.
