On Tue, Jan 7, 2025 at 11:57 PM Barry Song 21cnbao@gmail.com wrote:
On Wed, Jan 8, 2025 at 6:56 PM Yosry Ahmed yosryahmed@google.com wrote:
On Tue, Jan 7, 2025 at 9:34 PM Yosry Ahmed yosryahmed@google.com 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.
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.
..and now that I explain all of this I am wondering if the complexity is warranted in the first place. It goes back all the way to the first zswap commit, so I can't tell the justification for it.
Personally, I would vote for the added complexity, as it avoids the potential negative side effects of reverting the scheduler's optimization for selecting a suitable CPU for a woken-up task and I have been looking for an approach to resolve it by cpuhotplug lock (obviously quite hacky and more complex than using this mutex)
Oh, I was not talking about my proposed diff, but the existing logic that allocates the requests and buffers in the hotplug callbacks instead of just using alloc_percpu() to allocate them once for each possible CPU. I was wondering if there are actual setups where this matters and a significant amount of memory is being saved. Otherwise we should simplify things and just rip out the hotplug callbacks.
Anyway, for now I will cleanup and send the mutex diff as a new patch.
for (;;) in acomp_ctx_get_cpu_locked() is a bit tricky but correct and really interesting, maybe it needs some comments.
I am not sure if they are setups that have significantly different numbers of online and possible CPUs. Maybe we should just bite the bullet and just allocate everything with alloc_percpu() and rip out the hotplug callbacks completely.
Thanks Barry