RE: [PATCH v4 09/10] mm: zswap: Allocate pool batching resources if the crypto_alg supports batching.

["Sridhar, Kanchana P" <kanchana.p.sridhar@intel.com>]

Hi Nhat,

> -----Original Message-----
> From: Sridhar, Kanchana P <kanchana.p.sridhar@intel.com>
> Sent: Monday, December 2, 2024 4:31 PM
> To: Nhat Pham <nphamcs@gmail.com>
> Cc: linux-kernel@vger.kernel.org; linux-mm@kvack.org;
> hannes@cmpxchg.org; yosryahmed@google.com;
> chengming.zhou@linux.dev; usamaarif642@gmail.com;
> ryan.roberts@arm.com; ying.huang@intel.com; 21cnbao@gmail.com;
> akpm@linux-foundation.org; linux-crypto@vger.kernel.org;
> herbert@gondor.apana.org.au; davem@davemloft.net;
> clabbe@baylibre.com; ardb@kernel.org; ebiggers@google.com;
> surenb@google.com; Accardi, Kristen C <kristen.c.accardi@intel.com>;
> Feghali, Wajdi K <wajdi.k.feghali@intel.com>; Gopal, Vinodh
> <vinodh.gopal@intel.com>; Sridhar, Kanchana P
> <kanchana.p.sridhar@intel.com>
> Subject: RE: [PATCH v4 09/10] mm: zswap: Allocate pool batching resources if
> the crypto_alg supports batching.
> 
> Hi Nhat,
> 
> > -----Original Message-----
> > From: Nhat Pham <nphamcs@gmail.com>
> > Sent: Monday, December 2, 2024 11:16 AM
> > To: Sridhar, Kanchana P <kanchana.p.sridhar@intel.com>
> > Cc: linux-kernel@vger.kernel.org; linux-mm@kvack.org;
> > hannes@cmpxchg.org; yosryahmed@google.com;
> > chengming.zhou@linux.dev; usamaarif642@gmail.com;
> > ryan.roberts@arm.com; ying.huang@intel.com; 21cnbao@gmail.com;
> > akpm@linux-foundation.org; linux-crypto@vger.kernel.org;
> > herbert@gondor.apana.org.au; davem@davemloft.net;
> > clabbe@baylibre.com; ardb@kernel.org; ebiggers@google.com;
> > surenb@google.com; Accardi, Kristen C <kristen.c.accardi@intel.com>;
> > Feghali, Wajdi K <wajdi.k.feghali@intel.com>; Gopal, Vinodh
> > <vinodh.gopal@intel.com>
> > Subject: Re: [PATCH v4 09/10] mm: zswap: Allocate pool batching resources
> if
> > the crypto_alg supports batching.
> >
> > On Fri, Nov 22, 2024 at 11:01 PM Kanchana P Sridhar
> > <kanchana.p.sridhar@intel.com> wrote:
> > >
> > > This patch does the following:
> > >
> > > 1) Modifies the definition of "struct crypto_acomp_ctx" to represent a
> > >    configurable number of acomp_reqs and buffers. Adds a "nr_reqs" to
> > >    "struct crypto_acomp_ctx" to contain the nr of resources that will be
> > >    allocated in the cpu onlining code.
> > >
> > > 2) The zswap_cpu_comp_prepare() cpu onlining code will detect if the
> > >    crypto_acomp created for the pool (in other words, the zswap
> > compression
> > >    algorithm) has registered an implementation for batch_compress() and
> > >    batch_decompress(). If so, it will set "nr_reqs" to
> > >    SWAP_CRYPTO_BATCH_SIZE and allocate these many reqs/buffers, and
> > set
> > >    the acomp_ctx->nr_reqs accordingly. If the crypto_acomp does not
> > support
> > >    batching, "nr_reqs" defaults to 1.
> > >
> > > 3) Adds a "bool can_batch" to "struct zswap_pool" that step (2) will set to
> > >    true if the batching API are present for the crypto_acomp.
> >
> > Why do we need this "can_batch" field? IIUC, this can be determined
> > from the compressor internal fields itself, no?
> >
> > acomp_has_async_batching(acomp);
> >
> > Is this just for convenience, or is this actually an expensive thing to
> compute?
> 
> Thanks for your comments. This is a good question. I tried not to imply that
> batching resources have been allocated for the cpu based only on what
> acomp_has_async_batching() returns. It is possible that the cpu onlining
> code ran into an -ENOMEM error on any particular cpu. In this case, I set
> the pool->can_batch to "false", mainly for convenience, so that zswap
> can be somewhat insulated from migration. I agree that this may not be
> the best solution; and whether or not batching is enabled can be directly
> determined just before the call to crypto_acomp_batch_compress()
> based on:
> 
> acomp_ctx->nr_reqs == SWAP_CRYPTO_BATCH_SIZE;
> 
> I currently have a BUG_ON() for this condition not being met, that relies
> on the pool->can_batch gating the flow to get to zswap_batch_compress().
> 
> I think a better solution would be to check for having
> SWAP_CRYPTO_BATCH_SIZE
> # of acomp_ctx resources right after we acquire the acomp_ctx->mutex and
> before
> the call to crypto_acomp_batch_compress(). If so, we proceed, and if not, we
> call
> crypto_acomp_compress(). It seems this might be the only way to know for
> sure
> whether the crypto batching API can be called, given that migration is possible
> at any point in zswap_store(). Once we have obtained the mutex_lock, it
> seems
> we can proceed with batching based on this check (although the UAF situation
> remains as a larger issue, beyond the scope of this patch). I would appreciate
> other ideas as well.
> 
> Also, I have submitted a patch-series [1] with Yosry's & Johannes' suggestions
> to this series. This is setting up a consolidated
> zswap_store()/zswap_store_pages()
> code path for batching and non-batching compressors. My goal is for [1] to
> go through code reviews and be able to transition to batching, with a simple
> check:
> 
> if (acomp_ctx->nr_reqs == SWAP_CRYPTO_BATCH_SIZE)
>          zswap_batch_compress();
> else
>          zswap_compress();
> 
> Please feel free to provide code review comments in [1]. Thanks!
> 
> [1]: https://patchwork.kernel.org/project/linux-mm/list/?series=912937
> 
> >
> > >
> > > SWAP_CRYPTO_BATCH_SIZE is set to 8, which will be the IAA compress
> > batching
> >
> > I like a sane default value as much as the next guy, but this seems a
> > bit odd to me:
> >
> > 1. The placement of this constant/default value seems strange to me.
> > This is a compressor-specific value no? Why are we enforcing this
> > batching size at the zswap level, and uniformly at that? What if we
> > introduce a new batch compression algorithm...? Or am I missing
> > something, and this is a sane default for other compressors too?
> 
> You bring up an excellent point. This is a compressor-specific value.
> Instead of setting this up as a constant, which as you correctly observe,
> may not make sense for a non-IAA compressor, one way to get
> this could be by querying the compressor, say:
> 
> int acomp_get_max_batchsize(struct crypto_acomp *tfm) {...};
> 
> to then allocate sufficient acomp_reqs/buffers/etc. in the zswap
> cpu onlining code.
> 
> >
> > 2. Why is this value set to 8? Experimentation? Could you add some
> > justification in documentation?
> 
> Can I get back to you later this week with a proposal for this? We plan
> to have a team discussion on how best to approach this for current
> and future hardware.

Sorry it took me quite a while to get back to you on this. I have been busy
with implementing request chaining, and other major improvements to this
series based on the comments received thus far.

I will be submitting a v5 of this series shortly, in which I have implemented
an IAA_CRYPTO_MAX_BATCH_SIZE in the iaa_crypto driver. For now I set this
to 8 since we have done all our testing with a batch size of 8, but we are still
running experiments to figure this out, hence this #define in the iaa_crypto
driver (in v5) can potentially change. Further, there is a zswap-specific
ZSWAP_MAX_BATCH_SIZE in v5, which is also 8. I would appreciate code
review comments for v5. If the approach I've taken in v5 is acceptable, I
will add more details/justification in the documentation in a v6.

Thanks,
Kanchana

> 
> Thanks,
> Kanchana