Re: [PATCH v2 2/2] mm: zswap: disable migration while using per-CPU acomp_ctx

[Chengming Zhou <chengming.zhou@linux.dev>]

On 2025/1/8 13:34, Yosry Ahmed wrote:
> On Tue, Jan 7, 2025 at 9:00 PM Chengming Zhou <chengming.zhou@linux.dev> wrote:
>>
>> On 2025/1/8 12:46, Nhat Pham wrote:
>>> On Wed, Jan 8, 2025 at 9:34 AM Yosry Ahmed <yosryahmed@google.com> wrote:
>>>>
>>>>
>>>> Actually, using the mutex to protect against CPU hotunplug is not too
>>>> complicated. The following diff is one way to do it (lightly tested).
>>>> Johannes, Nhat, any preferences between this patch (disabling
>>>> migration) and the following diff?
>>>
>>> I mean if this works, this over migration diasbling any day? :)
>>>
>>>>
>>>> diff --git a/mm/zswap.c b/mm/zswap.c
>>>> index f6316b66fb236..4d6817c679a54 100644
>>>> --- a/mm/zswap.c
>>>> +++ b/mm/zswap.c
>>>> @@ -869,17 +869,40 @@ static int zswap_cpu_comp_dead(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);
>>>>
>>>> +       mutex_lock(&acomp_ctx->mutex);
>>>>           if (!IS_ERR_OR_NULL(acomp_ctx)) {
>>>>                   if (!IS_ERR_OR_NULL(acomp_ctx->req))
>>>>                           acomp_request_free(acomp_ctx->req);
>>>> +               acomp_ctx->req = NULL;
>>>>                   if (!IS_ERR_OR_NULL(acomp_ctx->acomp))
>>>>                           crypto_free_acomp(acomp_ctx->acomp);
>>>>                   kfree(acomp_ctx->buffer);
>>>>           }
>>>> +       mutex_unlock(&acomp_ctx->mutex);
>>>>
>>>>           return 0;
>>>>    }
>>>>
>>>> +static struct crypto_acomp_ctx *acomp_ctx_get_cpu_locked(
>>>> +               struct crypto_acomp_ctx __percpu *acomp_ctx)
>>>> +{
>>>> +       struct crypto_acomp_ctx *ctx;
>>>> +
>>>> +       for (;;) {
>>>> +               ctx = raw_cpu_ptr(acomp_ctx);
>>>> +               mutex_lock(&ctx->mutex);
>>>
>>> I'm a bit confused. IIUC, ctx is per-cpu right? What's protecting this
>>> cpu-local data (including the mutex) from being invalidated under us
>>> while we're sleeping and waiting for the mutex?
> 
> Please correct me if I am wrong, but my understanding is that memory
> allocated with alloc_percpu() is allocated for each *possible* CPU,
> and does not go away when CPUs are offlined. We allocate the per-CPU
> crypto_acomp_ctx structs with alloc_percpu() (including the mutex), so
> they should not go away with CPU offlining.

Ah, right! I missed that only buffer and req is dynamically allocated
by the cpu online callback.

Then your fix is safe to me, thanks for your explanation!

> 
> OTOH, we allocate the crypto_acomp_ctx.acompx, crypto_acomp_ctx.req,
> and crypto_acomp_ctx.buffer only for online CPUs through the CPU
> hotplug notifiers (i.e. zswap_cpu_comp_prepare() and
> zswap_cpu_comp_dead()). These are the resources that can go away with
> CPU offlining, and what we need to protect about.
> 
> The approach I am taking here is to hold the per-CPU mutex in the CPU
> offlining code while we free these resources, and set
> crypto_acomp_ctx.req to NULL. In acomp_ctx_get_cpu_locked(), we hold
> the mutex of the current CPU, and check if crypto_acomp_ctx.req is
> NULL.
> 
> If it is NULL, then the CPU is offlined between raw_cpu_ptr() and
> acquiring the mutex, and we retry on the new CPU that we end up on. If
> it is not NULL, then we are guaranteed that the resources will not be
> freed by CPU offlining until acomp_ctx_put_unlock() is called and the
> mutex is unlocked.
> 
>>
>> Yeah, it's not safe, we can only use this_cpu_ptr(), which will disable
>> preempt (so cpu offline can't kick in), and get refcount of ctx. Since
>> we can't mutex_lock in the preempt disabled section.
> 
> My understanding is that the purpose of this_cpu_ptr() disabling
> preemption is to prevent multiple CPUs accessing per-CPU data of a
> single CPU concurrently. In the zswap case, we don't really need that
> because we use the mutex to protect against it (and we cannot disable
> preemption anyway).

Yes, your fix is correct, preemption disable is not needed in this case.

Thanks.