linux-kernel - RE: [PATCH v3 09/13] mm: zswap: Modify struct crypto_acomp_ctx to be configurable in nr of acomp

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: <SJ0PR11MB5678B69DBDCBDED4589B4785C95E2@SJ0PR11MB5678.namprd11.prod.outlook.com>
Date: Sat, 9 Nov 2024 01:03:08 +0000
From: "Sridhar, Kanchana P" <kanchana.p.sridhar@...el.com>
To: Yosry Ahmed <yosryahmed@...gle.com>
CC: Johannes Weiner <hannes@...xchg.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-mm@...ck.org" <linux-mm@...ck.org>,
	"nphamcs@...il.com" <nphamcs@...il.com>, "chengming.zhou@...ux.dev"
	<chengming.zhou@...ux.dev>, "usamaarif642@...il.com"
	<usamaarif642@...il.com>, "ryan.roberts@....com" <ryan.roberts@....com>,
	"Huang, Ying" <ying.huang@...el.com>, "21cnbao@...il.com"
	<21cnbao@...il.com>, "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
	"herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
	"davem@...emloft.net" <davem@...emloft.net>, "clabbe@...libre.com"
	<clabbe@...libre.com>, "ardb@...nel.org" <ardb@...nel.org>,
	"ebiggers@...gle.com" <ebiggers@...gle.com>, "surenb@...gle.com"
	<surenb@...gle.com>, "Accardi, Kristen C" <kristen.c.accardi@...el.com>,
	"zanussi@...nel.org" <zanussi@...nel.org>, "Feghali, Wajdi K"
	<wajdi.k.feghali@...el.com>, "Gopal, Vinodh" <vinodh.gopal@...el.com>,
	"Sridhar, Kanchana P" <kanchana.p.sridhar@...el.com>
Subject: RE: [PATCH v3 09/13] mm: zswap: Modify struct crypto_acomp_ctx to be
 configurable in nr of acomp_reqs.


> -----Original Message-----
> From: Yosry Ahmed <yosryahmed@...gle.com>
> Sent: Friday, November 8, 2024 2:54 PM
> To: Sridhar, Kanchana P <kanchana.p.sridhar@...el.com>
> Cc: Johannes Weiner <hannes@...xchg.org>; linux-kernel@...r.kernel.org;
> linux-mm@...ck.org; nphamcs@...il.com; chengming.zhou@...ux.dev;
> usamaarif642@...il.com; ryan.roberts@....com; Huang, Ying
> <ying.huang@...el.com>; 21cnbao@...il.com; akpm@...ux-foundation.org;
> linux-crypto@...r.kernel.org; herbert@...dor.apana.org.au;
> davem@...emloft.net; clabbe@...libre.com; ardb@...nel.org;
> ebiggers@...gle.com; surenb@...gle.com; Accardi, Kristen C
> <kristen.c.accardi@...el.com>; zanussi@...nel.org; Feghali, Wajdi K
> <wajdi.k.feghali@...el.com>; Gopal, Vinodh <vinodh.gopal@...el.com>
> Subject: Re: [PATCH v3 09/13] mm: zswap: Modify struct crypto_acomp_ctx
> to be configurable in nr of acomp_reqs.
> 
> [..]
> > > > >
> > > > > There are no other callers to these functions. Just do the work
> > > > > directly in the cpu callbacks here like it used to be.
> > > >
> > > > There will be other callers to zswap_create_acomp_ctx() and
> > > > zswap_delete_acomp_ctx() in patches 10 and 11 of this series, when the
> > > > per-cpu "acomp_batch_ctx" is introduced in struct zswap_pool. I was
> trying
> > > > to modularize the code first, so as to split the changes into smaller
> commits.
> > > >
> > > > The per-cpu "acomp_batch_ctx" resources are allocated in patch 11 in
> the
> > > > "zswap_pool_can_batch()" function, that allocates batching resources
> > > > for this cpu. This was to address Yosry's earlier comment about
> minimizing
> > > > the memory footprint cost of batching.
> > > >
> > > > The way I decided to do this is by reusing the code that allocates the de-
> > > facto
> > > > pool->acomp_ctx for the selected compressor for all cpu's in
> > > zswap_pool_create().
> > > > However, I did not want to add the acomp_batch_ctx multiple
> reqs/buffers
> > > > allocation to the cpuhp_state_add_instance() code path which would
> incur
> > > the
> > > > memory cost on all cpu's.
> > > >
> > > > Instead, the approach I chose to follow is to allocate the batching
> resources
> > > > in patch 11 only as needed, on "a given cpu" that has to store a large
> folio.
> > > Hope
> > > > this explains the purpose of the modularization better.
> > > >
> > > > Other ideas towards accomplishing this are very welcome.
> > >
> > > If we remove the sysctl as suggested by Johannes, then we can just
> > > allocate the number of buffers based on the compressor and whether it
> > > supports batching during the pool initialization in the cpu callbacks
> > > only.
> > >
> > > Right?
> >
> > Yes, we could do that if the sysctl is removed, as suggested by Johannes.
> > The only "drawback" of allocating the batching resources (assuming the
> > compressor allows batching) would be that the memory footprint penalty
> > would be incurred on every cpu. I was trying to further economize this
> > cost based on whether a given cpu actually needs to zswap_store() a
> > large folio, and only then allocate the batching resources. Although, I am
> > not sure if this would benefit any usage model.
> >
> > If we agree the pool initialization is the best place to allocate the batching
> > resources, then I will make this change in v4.
> 
> IIUC the additional cost would apply if someone wants to use
> deflate-iaa on hardware that supports batching but does not want to
> use batching. I don't think catering to such a use case warrants the
> complexity in advance, not until we have a user that genuinely cares.

Sure, this makes sense. I will address this in v4.

Thanks,
Kanchana