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

[Kanchana P. Sridhar]

On Mon, Mar 16, 2026 at 11:30 AM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
>
> > > > 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.
>
> I think it's fine as long as acomp_request_alloc() never returns an
> error. Maybe do it in a separate patch first, change IS_ERR_OR_NULL()
> to a NULL check in zswap_cpu_comp_dead(), with the reasoning explained
> in the changelog, to avoid hiding that change within the bigger patch.

Sounds good.

>
> Actually looking at zswap_cpu_comp_dead(), is the IS_ERR_OR_NULL()
> check on acomp_ctx also misleading? Should that also just be a NULL
> check?

Even a NULL check would be redundant in this case, per my
understanding, because if the alloc_percpu() call in
zswap_pool_create() had failed, pool creation would have failed.

I think a NULL check on the acomp_ctx would still be a good idea, just
in case, since this is all part of CPU hotplug. I agree, we don't need
an IS_ERR() check on acomp_ctx.

Thanks,
Kanchana