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 for Android: free password hash cracker in your pocket

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

Date: Tue, 19 Dec 2023 15:04:21 -0800
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Chris Li <chrisl@...nel.org>
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

On Tue, Dec 19, 2023 at 2:49 PM Chris Li <chrisl@...nel.org> wrote:
>
> Hi Yosry,
>
> On Tue, Dec 19, 2023 at 1:39 PM Yosry Ahmed <yosryahmed@...gle.com> 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.