RE: [PATCH v12 20/23] mm: zswap: Per-CPU acomp_ctx resources exist from pool creation to deletion.

[Sridhar, Kanchana P]

> -----Original Message-----
> From: Yosry Ahmed <yosry.ahmed@xxxxxxxxx>
> Sent: Wednesday, October 1, 2025 8:33 AM
> To: Sridhar, Kanchana P <kanchana.p.sridhar@xxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx;
> hannes@xxxxxxxxxxx; nphamcs@xxxxxxxxx; chengming.zhou@xxxxxxxxx;
> usamaarif642@xxxxxxxxx; ryan.roberts@xxxxxxx; 21cnbao@xxxxxxxxx;
> ying.huang@xxxxxxxxxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx;
> senozhatsky@xxxxxxxxxxxx; sj@xxxxxxxxxx; kasong@xxxxxxxxxxx; linux-
> crypto@xxxxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx;
> davem@xxxxxxxxxxxxx; clabbe@xxxxxxxxxxxx; ardb@xxxxxxxxxx;
> ebiggers@xxxxxxxxxx; surenb@xxxxxxxxxx; Accardi, Kristen C
> <kristen.c.accardi@xxxxxxxxx>; Gomes, Vinicius <vinicius.gomes@xxxxxxxxx>;
> Feghali, Wajdi K <wajdi.k.feghali@xxxxxxxxx>; Gopal, Vinodh
> <vinodh.gopal@xxxxxxxxx>
> Subject: Re: [PATCH v12 20/23] mm: zswap: Per-CPU acomp_ctx resources
> exist from pool creation to deletion.
> 
> On Tue, Sep 30, 2025 at 09:56:33PM +0000, Sridhar, Kanchana P wrote:
> >
> > > -----Original Message-----
> > > From: Yosry Ahmed <yosry.ahmed@xxxxxxxxx>
> > > Sent: Tuesday, September 30, 2025 2:20 PM
> > > To: Sridhar, Kanchana P <kanchana.p.sridhar@xxxxxxxxx>
> > > Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx;
> > > hannes@xxxxxxxxxxx; nphamcs@xxxxxxxxx;
> chengming.zhou@xxxxxxxxx;
> > > usamaarif642@xxxxxxxxx; ryan.roberts@xxxxxxx; 21cnbao@xxxxxxxxx;
> > > ying.huang@xxxxxxxxxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx;
> > > senozhatsky@xxxxxxxxxxxx; sj@xxxxxxxxxx; kasong@xxxxxxxxxxx; linux-
> > > crypto@xxxxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx;
> > > davem@xxxxxxxxxxxxx; clabbe@xxxxxxxxxxxx; ardb@xxxxxxxxxx;
> > > ebiggers@xxxxxxxxxx; surenb@xxxxxxxxxx; Accardi, Kristen C
> > > <kristen.c.accardi@xxxxxxxxx>; Gomes, Vinicius
> <vinicius.gomes@xxxxxxxxx>;
> > > Feghali, Wajdi K <wajdi.k.feghali@xxxxxxxxx>; Gopal, Vinodh
> > > <vinodh.gopal@xxxxxxxxx>
> > > Subject: Re: [PATCH v12 20/23] mm: zswap: Per-CPU acomp_ctx
> resources
> > > exist from pool creation to deletion.
> > >
> > > > > > > >  static struct zswap_pool *zswap_pool_create(char *compressor)
> > > > > > > >  {
> > > > > > > >  	struct zswap_pool *pool;
> > > > > > > > @@ -263,19 +287,43 @@ static struct zswap_pool
> > > > > > > *zswap_pool_create(char *compressor)
> > > > > > > >
> > > > > > > >  	strscpy(pool->tfm_name, compressor, sizeof(pool-
> > > >tfm_name));
> > > > > > > >
> > > > > > > > -	pool->acomp_ctx = alloc_percpu(*pool->acomp_ctx);
> > > > > > > > +	/* Many things rely on the zero-initialization. */
> > > > > > > > +	pool->acomp_ctx = alloc_percpu_gfp(*pool->acomp_ctx,
> > > > > > > > +					   GFP_KERNEL |
> > > __GFP_ZERO);
> > > > > > > >  	if (!pool->acomp_ctx) {
> > > > > > > >  		pr_err("percpu alloc failed\n");
> > > > > > > >  		goto error;
> > > > > > > >  	}
> > > > > > > >
> > > > > > > > -	for_each_possible_cpu(cpu)
> > > > > > > > -		mutex_init(&per_cpu_ptr(pool->acomp_ctx, cpu)-
> > > >mutex);
> > > > > > > > -
> > > > > > > > +	/*
> > > > > > > > +	 * This is serialized against CPU hotplug operations. Hence,
> > > cores
> > > > > > > > +	 * cannot be offlined until this finishes.
> > > > > > > > +	 * In case of errors, we need to goto "ref_fail" instead of
> > > "error"
> > > > > > > > +	 * because there is no teardown callback registered anymore,
> > > for
> > > > > > > > +	 * cpuhp_state_add_instance() to de-allocate resources as it
> > > rolls
> > > > > > > back
> > > > > > > > +	 * state on cores before the CPU on which error was
> > > encountered.
> > > > > > > > +	 */
> > > > > > > >  	ret =
> > > > > > > cpuhp_state_add_instance(CPUHP_MM_ZSWP_POOL_PREPARE,
> > > > > > > >  				       &pool->node);
> > > > > > > > +
> > > > > > > > +	/*
> > > > > > > > +	 * We only needed the multi state instance add operation to
> > > invoke
> > > > > > > the
> > > > > > > > +	 * startup callback for all cores without cores getting offlined.
> > > Since
> > > > > > > > +	 * the acomp_ctx resources will now only be de-allocated
> > > when the
> > > > > > > pool
> > > > > > > > +	 * is destroyed, we can safely remove the multi state
> > > instance. This
> > > > > > > > +	 * minimizes (but does not eliminate) the possibility of
> > > > > > > > +	 * zswap_cpu_comp_prepare() being invoked again due to a
> > > CPU
> > > > > > > > +	 * offline-online transition. Removing the instance also
> > > prevents race
> > > > > > > > +	 * conditions between CPU onlining after initial pool creation,
> > > and
> > > > > > > > +	 * acomp_ctx_dealloc() freeing the acomp_ctx resources.
> > > > > > > > +	 * Note that we delete the instance before checking the error
> > > status
> > > > > > > of
> > > > > > > > +	 * the node list add operation because we want the instance
> > > removal
> > > > > > > even
> > > > > > > > +	 * in case of errors in the former.
> > > > > > > > +	 */
> > > > > > > > +
> > > 	cpuhp_state_remove_instance(CPUHP_MM_ZSWP_POOL_PREPARE,
> > > > > > > &pool->node);
> > > > > > > > +
> > > > > > >
> > > > > > > I don't understand what's wrong with the current flow? We call
> > > > > > > cpuhp_state_remove_instance() in pool deletion before freeing up
> the
> > > > > > > per-CPU resources. Why is this not enough?
> > > > > >
> > > > > > This is because with the changes proposed in this commit, the multi
> > > state
> > > > > > add instance is used during pool creation as a way to create
> acomp_ctx
> > > > > > resources correctly with just the offline/online state transitions
> > > guaranteed
> > > > > > by CPU hotplug, without needing additional mutex locking as in the
> > > > > mainline.
> > > > > > In other words, the consistency wrt safely creating/deleting
> acomp_ctx
> > > > > > resources with the changes being proposed is accomplished by the
> > > hotplug
> > > > > > state transitions guarantee. Stated differently, the hotplug
> framework
> > > > > > helps enforce the new design during pool creation without relying on
> the
> > > > > > mutex and subsequent simplifications during zswap_[de]compress()
> > > > > > proposed in this commit.
> > > > > >
> > > > > > Once this is done, deleting the CPU hotplug state seems cleaner, and
> > > > > reflects
> > > > > > the change in policy of the resources' lifetime. It also prevents race
> > > > > conditions
> > > > > > between zswap_cpu_comp_prepare() and acomp_ctx_dealloc()
> called
> > > from
> > > > > > zswap_pool_destroy().
> > > > >
> > > > > How is a race with zswap_cpu_comp_prepare() possible if we call
> > > > > cpuhp_state_remove_instance() before acomp_ctx_dealloc() in the
> pool
> > > > > deletion path?
> > > >
> > > > Good point. I agree, calling cpuhp_state_remove_instance() before
> > > > acomp_ctx_dealloc() will not cause a race. However, if we consider the
> > > > time from pool creation to deletion: if there is an online-offline-online
> > > > transition, can zswap_cpu_comp_prepare() race with the call to
> > > > cpuhp_state_remove_instance()? If so, wouldn't this cause
> unpredictable
> > > > behavior?
> > >
> > > How will this race happen?
> > >
> > > cpuhp_state_remove_instance() is called while a pool is being destroyed,
> > > while zswap_cpu_comp_prepare() while the pool is being created or
> during
> > > CPU onlining.
> > >
> > > The former cannot race, and the latter should be synchronized by hotplug
> > > code.
> > >
> > > >
> > > > I agree, this can occur even with the code in this commit, but there is
> > > > less risk of things going wrong because we remove the CPU hotplug
> > > > instance before the pool is added to zswap_pools.
> > > >
> > > > Further, removing the CPU hotplug instance directly codifies the
> > > > intent of this commit, i.e., to use this as a facilitator and manage
> memory
> > > > allotted to acomp_ctx, but not to manage those resources' lifetime
> > > > thereafter.
> > > >
> > > > Do you see any advantage of keeping the call to
> > > cpuhp_state_remove_instance()
> > > > occur before acomp_ctx_dealloc() in zswap_pool_destroy()? Please let
> me
> > > know
> > > > if I am missing something.
> > >
> > > What about more CPUs going online? Without the hotplug instance we
> don't
> > > get per-CPU resources for those. We are not using the hotplug mechanism
> > > just to facilitate per-CPU resource allocation, we use it to
> > > automatically allocate resources for newly onlined CPUs without having
> > > to preallocate for all possible CPUs.
> >
> > This is an excellent point! It makes sense, I will move the call to
> > cpuhp_state_remove_instance() to be before the call to
> > acomp_ctx_dealloc() in zswap_pool_destroy(). Thanks for catching this.
> >
> > >
> > > Also, this makes the code more difficult to reason about, and is an
> > > unncessary change from the current behavior.
> >
> > Ok.
> >
> > >
> > > The only change needed is to drop the teardown callback and do the
> > > freeing in the pool destruction path instead.
> >
> > Just to summarize: besides moving the call to
> cpuhp_state_remove_instance()
> > to zswap_pool_destroy() and more concise comments/commit logs, are
> there
> > other changes to be made in patch 20?
> 
> I don't believe so. Thanks!

Sounds good, thanks!

Thanks,
Kanchana