Re: [PATCH 0/2] zswap pool per-CPU acomp_ctx simplifications

["Kanchana P. Sridhar" <kanchanapsridhar2026@gmail.com>]

On Mon, Mar 16, 2026 at 8:06 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> > > > @@ -786,7 +786,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
> > > > return ret;
> > > >
> > > > acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
> > > > - if (IS_ERR(acomp_ctx->acomp)) {
> > > > + if (IS_ERR_OR_NULL(acomp_ctx->acomp)) {
> > > Does crypto_alloc_acomp_node() ever return NULL?
> > > Looking at the error handling just below this check, if this were to
> > > actually return NULL, PTR_ERR(NULL) evaluates to 0. This would cause
> > > the function to incorrectly return 0 (success) instead of an error code,
> > > hiding the allocation failure.
> >
> > This is a good catch. Just to provide context, this patch was
> > introduced based on Yosry's earlier comments in [1].
> >
> > [1]: https://patchwork.kernel.org/comment/26282128/
> >
> > crypto_alloc_acomp_node() currently does not return NULL. However, it
> > could, in future.
> > Since the rest of zswap_cpu_comp_prepare() dereferences
> > acomp_ctx->acomp, it depends on whether we want to future-proof the
> > code to handle a possible eventuality of crypto_alloc_acomp_node()
> > returning NULL.
>
> Hmm upon revisiting this, I think keeping this as IS_ERR() here is a
> better documentation for the API, and the incossitency between this code
> and acomp_ctx_dealloc() is arguably documenting that the function can
> only return an ERR, but it can also be NULL-initialized by zswap.

Yes, makes sense.

>
> >
> > If the maintainers think future-proofing is beneficial, I would need
> > to handle the PTR_ERR(NULL) which would send a false success status.
> > If we don't think we need to handle a future NULL return from
> > crypto_alloc_acomp_node(), then I don't think this change is needed.
> > We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
> > maintainers' inputs on how to proceed.
> >
> > > > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > > > - if (!acomp_ctx->req) {
> > > > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> > > Is this change necessary for acomp_request_alloc()?
> > > This function strictly returns NULL on allocation failure, not an error
> > > pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> > > without providing a functional benefit.
> >
> > As of now, acomp_request_alloc() returns a valid "req" or NULL in case
> > of an error. Same question as above. The only benefit would be making
> > the code more robust to handle changes in the acomp API in future.
>
> For this one, do we need to do IS_ERR_OR_NULL() in acomp_ctx_dealloc()
> to begin with? If acomp_request_alloc() only returns NULL, maybe that
> should also be a NULL check?

This one is debatable, since acomp_ctx_dealloc() is intended to
replace zswap_cpu_comp_dead(), which has the IS_ERR_OR_NULL(). I think
replacing this with IS_NULL(req) makes sense, but would like to
confirm with you if changing existing behavior is Ok.

>
> In this case, we don't really need to make any changes here, and I think
> this patch can just be dropped.

I agree.

Thanks,
Kanchana