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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF8kJuMdNJTZrWsK-VkxWhCo_7=T0Cb5Cwg5nBtv-ex4M1Dhkw@mail.gmail.com>
Date: Tue, 19 Dec 2023 15:33:28 -0800
From: Chris Li <chrisl@...nel.org>
To: Yosry Ahmed <yosryahmed@...gle.com>
Cc: Nhat Pham <nphamcs@...il.com>, Chengming Zhou <zhouchengming@...edance.com>, 
	Seth Jennings <sjenning@...hat.com>, Vitaly Wool <vitaly.wool@...sulko.com>, 
	Dan Streetman <ddstreet@...e.org>, Johannes Weiner <hannes@...xchg.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org
Subject: Re: [PATCH v3 6/6] mm/zswap: directly use percpu mutex and buffer in load/store

Hi Yosry,


On Tue, Dec 19, 2023 at 3:05 PM Yosry Ahmed <yosryahmed@...gle.com> 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