RE: [PATCH v13 19/22] mm: zswap: Per-CPU acomp_ctx resources exist from pool creation to deletion.

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

> -----Original Message-----
> From: Yosry Ahmed <yosry.ahmed@linux.dev>
> Sent: Friday, December 12, 2025 2:25 PM
> To: Sridhar, Kanchana P <kanchana.p.sridhar@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-mm@kvack.org;
> hannes@cmpxchg.org; nphamcs@gmail.com; chengming.zhou@linux.dev;
> usamaarif642@gmail.com; ryan.roberts@arm.com; 21cnbao@gmail.com;
> ying.huang@linux.alibaba.com; akpm@linux-foundation.org;
> senozhatsky@chromium.org; sj@kernel.org; kasong@tencent.com; 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>; Gomes, Vinicius <vinicius.gomes@intel.com>;
> Feghali, Wajdi K <wajdi.k.feghali@intel.com>; Gopal, Vinodh
> <vinodh.gopal@intel.com>
> Subject: Re: [PATCH v13 19/22] mm: zswap: Per-CPU acomp_ctx resources
> exist from pool creation to deletion.
> 
> On Fri, Dec 12, 2025 at 08:53:13PM +0000, Sridhar, Kanchana P wrote:
> [..]
> > > On Fri, Dec 12, 2025 at 06:17:07PM +0000, Sridhar, Kanchana P wrote:
> > > > >
> > > > > >  	ret =
> > > > > cpuhp_state_add_instance(CPUHP_MM_ZSWP_POOL_PREPARE,
> > > > > >  				       &pool->node);
> > > > > >  	if (ret)
> > > > > > -		goto error;
> > > > > > +		goto ref_fail;
> > > > >
> > > > > IIUC we shouldn't call cpuhp_state_remove_instance() on failure, we
> > > > > probably should add a new label.
> > > >
> > > > In this case we should because it is part of the pool creation failure
> > > > handling flow, at the end of which, the pool will be deleted.
> > >
> > > What I mean is, when cpuhp_state_add_instance() fails we goto ref_fail
> > > which will call cpuhp_state_remove_instance(). But the current code does
> > > not call cpuhp_state_remove_instance() if cpuhp_state_add_instance()
> > > fails.
> >
> > I see what you mean. The current mainline code does not call
> > cpuhp_state_remove_instance() if cpuhp_state_add_instance() fails,
> because
> > the cpuhotplug code will call the teardown callback in this case.
> >
> > In this patch, I do need to call cpuhp_state_remove_instance() and
> > acomp_ctx_dealloc() in this case because there is no teardown callback
> > being registered.
> 
> Hmm looking at cpuhp_state_add_instance(), it seems like it doesn't add
> the node to the list on failure. cpuhp_state_remove_instance() only
> removes the node from the list when there's no teardown cb, so it will
> be a nop in this case.
> 
> What we need to do is manual cleanup, since there is no teardown cb,
> which is already being done by acomp_ctx_dealloc() IIUC.
> 
> So I think calling cpuhp_state_remove_instance() when
> cpuhp_state_add_instance() fails is not needed, and I don't see other
> callers doing it.

You are right. I too have verified this. I will create a label for the call
to acomp_ctx_dealloc() and fix this.

> 
> [..]
> > > > > > @@ -322,9 +346,15 @@ static struct zswap_pool
> > > > > *__zswap_pool_create_fallback(void)
> > > > > >
> > > > > >  static void zswap_pool_destroy(struct zswap_pool *pool)
> > > > > >  {
> > > > > > +	int cpu;
> > > > > > +
> > > > > >  	zswap_pool_debug("destroying", pool);
> > > > > >
> > > > > >
> 	cpuhp_state_remove_instance(CPUHP_MM_ZSWP_POOL_PREPARE,
> > > > > &pool->node);
> > > > > > +
> > > > > > +	for_each_possible_cpu(cpu)
> > > > > > +		acomp_ctx_dealloc(per_cpu_ptr(pool->acomp_ctx,
> cpu));
> > > > > > +
> > > > > >  	free_percpu(pool->acomp_ctx);
> > > > > >
> > > > > >  	zs_destroy_pool(pool->zs_pool);
> > > > > > @@ -736,39 +766,35 @@ static int
> > > zswap_cpu_comp_prepare(unsigned int
> > > > > cpu, struct hlist_node *node)
> > > > > >  {
> > > > > >  	struct zswap_pool *pool = hlist_entry(node, struct
> zswap_pool,
> > > > > node);
> > > > > >  	struct crypto_acomp_ctx *acomp_ctx = per_cpu_ptr(pool-
> > > > > >acomp_ctx, cpu);
> > > > > > -	struct crypto_acomp *acomp = NULL;
> > > > > > -	struct acomp_req *req = NULL;
> > > > > > -	u8 *buffer = NULL;
> > > > > > -	int ret;
> > > > > > +	int ret = -ENOMEM;
> > > > > >
> > > > > > -	buffer = kmalloc_node(PAGE_SIZE, GFP_KERNEL,
> cpu_to_node(cpu));
> > > > > > -	if (!buffer) {
> > > > > > -		ret = -ENOMEM;
> > > > > > -		goto fail;
> > > > > > -	}
> > > > > > +	/*
> > > > > > +	 * To handle cases where the CPU goes through online-
> offline-online
> > > > > > +	 * transitions, we return if the acomp_ctx has already been
> initialized.
> > > > > > +	 */
> > > > > > +	if (!IS_ERR_OR_NULL(acomp_ctx->acomp))
> > > > > > +		return 0;
> > > > >
> > > > > Is it possible for acomp_ctx->acomp to be an ERR value here? If it is,
> > > > > then zswap initialization should have failed. Maybe
> WARN_ON_ONCE() for
> > > > > that case?
> > > >
> > > > This is checking for a valid acomp_ctx->acomp using the same criteria
> > > > being uniformly used in acomp_ctx_dealloc(). This check is necessary to
> > > > handle the case where the CPU goes through online-offline-online state
> > > > transitions.
> > >
> > > I think I am confused. I thought now we don't free this on CPU offline,
> > > so either it's NULL because this is the first time we initialize it on
> > > this CPU, or it is allocated.
> >
> > Yes, this is correct.
> >
> > > If it is an ERR value, then the pool
> > > creation should have failed and we wouldn't be calling this again on CPU
> > > online.
> > >
> > > In other words, what scenario do we expect to legitimately see an ERR
> > > value here?
> >
> > I am using "(!IS_ERR_OR_NULL(acomp_ctx->acomp)" as a check for the
> > acomp being allocated already. I could instead have used "if (acomp_ctx-
> >acomp)",
> > but use the former to be consistent with patch 20/22.
> >
> > I cannot think of a scenario where we can expect an ERR value here.
> 
> Yeah maybe do if (acomp_ctx->acomp) and
> WARN_ON_ONCE(IS_ERR(acomp_ctx->acomp))?

Sure.

Thanks,
Kanchana