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

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

On Mon, Mar 16, 2026 at 12:24 PM Yosry Ahmed <yosry@kernel.org> wrote:
>
> On Mon, Mar 16, 2026 at 12:21 PM Kanchana P. Sridhar
> <kanchanapsridhar2026@gmail.com> wrote:
> >
> > On Mon, Mar 16, 2026 at 11:30 AM Yosry Ahmed <yosry@kernel.org> 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.
>
> So I think we do one patch to convert both IS_ERR_OR_NULL() to NULL
> checks, and then the current patch 1, right?

Yes.