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

[Yosry Ahmed]

On Tue, Dec 19, 2023 at 10:43 AM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
>
> On Tue, Dec 19, 2023 at 5:29 AM Chris Li <chrisl@xxxxxxxxxx> wrote:
> >
> > Hi Chengming and Yosry,
> >
> > On Mon, Dec 18, 2023 at 3:50 AM Chengming Zhou
> > <zhouchengming@xxxxxxxxxxxxx> 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?