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

[Yosry Ahmed <yosry@kernel.org>]

> > > 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.

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?