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 14:48:58 -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 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;
 };

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 value/code_change ratio for renaming the struct
alone is low. On the other hand, if we can get rid of some per cpu
load, that value is high enough to justify the patch.

Chris