From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 77B4BE77199 for ; Wed, 8 Jan 2025 02:34:11 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id ED92D6B008A; Tue, 7 Jan 2025 21:34:10 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E89656B008C; Tue, 7 Jan 2025 21:34:10 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D50F36B0093; Tue, 7 Jan 2025 21:34:10 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id B86266B008A for ; Tue, 7 Jan 2025 21:34:10 -0500 (EST) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 61FF1160E2D for ; Wed, 8 Jan 2025 02:34:10 +0000 (UTC) X-FDA: 82982715060.14.917F1E7 Received: from mail-qv1-f49.google.com (mail-qv1-f49.google.com [209.85.219.49]) by imf21.hostedemail.com (Postfix) with ESMTP id 86F6D1C0006 for ; Wed, 8 Jan 2025 02:34:08 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=FcfuIuBK; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf21.hostedemail.com: domain of yosryahmed@google.com designates 209.85.219.49 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1736303648; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=qEAVPjDiQWu3tIjXyUt/7wC+P0xHFWQXNCXnwqpbpvA=; b=ObD6YoPgB8pk2uzmUwGzs98YLfUCTSjxdwiX1w5w56ACpqjJ8EF2kWKMGS92iSEdiVD1XH mIJyRc+3zTet99xIAp0wV/PozJfHexmZIRDEW7SOwT9UAvtolk+lsiDi7iJmKQsWrhRHJG 4x+USWZObMoLQLQNjCE3AqmMaQoTiCo= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1736303648; a=rsa-sha256; cv=none; b=fduO7wUgQYpGsDy3apijNetECsuPnJo6pMG1nUPOPfkRItJtakXVVAVMlRvtlQe115kGhU 0x3KGVol1jU+QuYMI1U0tMBWjiiAiYojYSPtamE1ES6Kow+zk5VdqFZCaaj8zSvGOxtgam UplO+1KPU2ZoysNCf1ZixLR4YnJhxYQ= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=FcfuIuBK; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf21.hostedemail.com: domain of yosryahmed@google.com designates 209.85.219.49 as permitted sender) smtp.mailfrom=yosryahmed@google.com Received: by mail-qv1-f49.google.com with SMTP id 6a1803df08f44-6dcd4f1aaccso105073896d6.2 for ; Tue, 07 Jan 2025 18:34:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1736303647; x=1736908447; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=qEAVPjDiQWu3tIjXyUt/7wC+P0xHFWQXNCXnwqpbpvA=; b=FcfuIuBKAz2nYg/DSSJbuKSOZhtnlaNRNlXbpmkNRonJNZ7r0Up1D1FLndZpmH/FMh np/f6UrYaeD1b7lXnRTjpp/pmfKT2+20SHbEMPlHHky/WQiImbYttVC/+P2j3KGeXJCJ 9+mF42F1bz00NvH08fzpX9TfI5d31iQ7kgZZwcnMVxOwD6Md+Tdb76o1nrrhFJG8OHiU jeYFhd/0zj53dm4W0xfb+3xnNxt3lmDQ7icdIaPJjphB6kYqfD08tgW/ncc7MxwycIqD iX7CJpuynTN9LviVYVMNpTdlSOA/JRbfVbH2pZ3EBXzTr7z6jLyGhuuneokM2Fpe6V7h pXnQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736303647; x=1736908447; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=qEAVPjDiQWu3tIjXyUt/7wC+P0xHFWQXNCXnwqpbpvA=; b=ryyeaTg6jbX44bDqIhNNAFDEVcCVTnlZuy7HONmhh3E1T5mv7VgAV1v/4DDLA0O7hc IrNsT0uP59cKHjkYb4lGpTFhjG46vE7W70FVxaZyMT9zDKHtPwtLtpDXWE9NwuK/rM8f QiQpkYIgENMlxZBCGmZnBcWTu4gp/aHuQa6jMy484hguWi5No3GWf0ixaiXB0zCTpfn/ V99Kfd26bWNHoQ2JHAxigg04z6x6uLkZO5ZtlZaQZ53HQzcSzmKuW+aZldi7hL1WTwMF prZmnpzi0rpBo2NkISfvZVSMV+J/1CYAaLgJ5QYvUF5ht2cWrVFwnByg8ymLfS4pt4dl s70A== X-Forwarded-Encrypted: i=1; AJvYcCVzdZbBeN9G9w6BBrBX/D6tr2K9LonjYfHqCigHSLIyAS/kgieTtUqtdqaW52+uLnuLLwN9rKGLgw==@kvack.org X-Gm-Message-State: AOJu0YypFsMKJCWGdyW3j+9+3yAQ+DyezN6o1169boYDNiehY8JnCwk5 +olP447r3mqVyBw02TAPqmiFNZu6o8HAv8RUunpKFRAe0tNZa1DDQkaX1pILoUs2uyn9UZD0QBH +t3D5ud4Nru+v4MFCljOV5rxnjoy5e697hj7E X-Gm-Gg: ASbGncv1+inBSDcJUxkMtIf83G0kuOIte5CE0pHTJshheEKknwcKGt/WUB6S/RKHRVd i5nzF5f1q96uNhr3E6gEFq5u+LX7TsVjr01I+FtxTTO3itxh6km5RpVzbeJkO2XH1baBE X-Google-Smtp-Source: AGHT+IEp0mFaD6EWYlTmvXY23uKOhOykJzare1tAuHmU4auCcWrgdZicFHEaCzxL3Fmwe7WWBY5KdD0SCwA3RVmAebE= X-Received: by 2002:a05:6214:2266:b0:6d8:a856:133 with SMTP id 6a1803df08f44-6df9b1ea668mr22346316d6.12.1736303647461; Tue, 07 Jan 2025 18:34:07 -0800 (PST) MIME-Version: 1.0 References: <20250107222236.2715883-1-yosryahmed@google.com> <20250107222236.2715883-2-yosryahmed@google.com> In-Reply-To: From: Yosry Ahmed Date: Tue, 7 Jan 2025 18:33:30 -0800 X-Gm-Features: AbW1kvZFpesSFBOCECViZeKfLo_g51tw8jkJijUXun9isjO0Ksdckii3zW75WgQ Message-ID: Subject: Re: [PATCH v2 2/2] mm: zswap: disable migration while using per-CPU acomp_ctx To: Johannes Weiner , Nhat Pham Cc: Andrew Morton , Chengming Zhou , Vitaly Wool , Barry Song , Sam Sun , "linux-mm@kvack.org" , "linux-kernel@vger.kernel.org" , "stable@vger.kernel.org" , "Sridhar, Kanchana P" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: 7htd4um6fn6fzcmjo83x864knke8sk34 X-Rspamd-Queue-Id: 86F6D1C0006 X-Rspam-User: X-Rspamd-Server: rspam01 X-HE-Tag: 1736303648-289072 X-HE-Meta: U2FsdGVkX18XIcumT71VSaW1yOdw5s+dMxvk/FC5LzwQ3TrbL+C68Yu7f1YvUVh/1IA7l6CxEW8kDeTWPP3V3WLc550RgsI155v/bKekvlLmQW8JXFcRjyUOO/ptnY6+x8m3RCTa9WbofN5aj8VoWVX2a3Dv+BVc21u+Lef9Z2i5FGsfuWYwAuqjndMiH7RbSV6uojROhNGmzkV+2+pPGxcvolmDXbKZo0QhqarlOKWwO0kdn8cnFwlD3KqNCourfbQCEWkVH37/KrP+SkRI8HifKwLBwHhV90kb0qXKW0izabJUMm6WWP9D1giOINLVgLOgYk3MHR+dipwcrgCMcwW7ULRQ24Mnvz5+vV7IDYsqVPchHyrzEGppoafhBS5edmeKa1UzsebFWyVJyREc0b7mau3k3jtYuU230zIqT40Zgiu55J87fcLM0L8B/rK9hON7z99Q51o+h3JyMNcPu0vlo8t7fmR0qmnzV0w0mCJR27v4UxKQv42YIPm5cKJzvlyiHejTsnum3gwvy+TLNMfb0oJ2L3VxF31MvgbxUrNT8qXIpvJTDjQu2sOCsXRMlADEowF4fDTQyAw1rg5tQxVdis7xEM0vLkfJSUq27UUXLhDnnHaslBWseR1DwADdAGr3RsjoUsZFryxHHKNy+N7tIMqMlvyWeds5I/TsuTWH7nE08HaiLo67piMx+OedKyvjuJNZ9aPqFfy+GXf5VIBa1esEXLYcd4xujH0P0au+XXfMGlTSur5FQkHc8br5xPvd27E7fyONfflaqJhHGcpvL/UqryDgGUB3g5sbWSyV3a2bNHyfu6gLsROYEDUYjMFpRYSKyOo6+YdQcoMr7uCJzs9QFCUsTlg2HtYsRNcCNFuDNWqg/HO4ZyOomBYLa/OoLuiHxNAPkjTFkv3IlcCSa4hiFQxhTSWuFkx/kYvWtmc1Uc3iVK/OB+8ppe6snEYwVAA2DeZSrs6BP7u 1YPmHAz2 aEQShzt2dfom6IA2a5KrnlHbmqqVYAPeRiBenRzch6SZCaaYE/RAVaW9flKjJ13fD2rqkjQ4VoJE5b2o1ITGgJRv/WGSg80J7Y1asFPbJVbDKgDItw16LiEl4RUKB3fI9TzScZ5V2vi2K+uIKUrz8uTGxwHtGrQCLXb+snL9PF3uUizPTtd9MoVtQZPoR3+xC1Gml/A/mjvOwR/cESpB/edXfKVGnNvV4jjSM7xHuNV7Ec1XPr5N5RAbjEq6mLnhm3DPSSUvoCFdw3ZepM48rXCpqh0RE43mqUi/ljkkynqly1YM7Zk5FY68XOg== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Tue, Jan 7, 2025 at 5:18=E2=80=AFPM Yosry Ahmed = wrote: > > [..] > > > > Couple of possibly related thoughts: > > > > > > > > 1) I have been thinking some more about the purpose of this per-cpu > > > acomp_ctx > > > > mutex. It appears that the main benefit is it causes task bloc= ked errors > > > (which are > > > > useful to detect problems) if any computes in the code section= it covers > > > take a > > > > long duration. Other than that, it does not protect a resource= , nor > > > prevent > > > > cpu offlining from deleting its containing structure. > > > > > > It does protect resources. Consider this case: > > > - Process A runs on CPU #1, gets the acomp_ctx, and locks it, then is > > > migrated to CPU #2. > > > - Process B runs on CPU #1, gets the same acomp_ctx, and tries to loc= k > > > it then waits for process A. Without the lock they would be using the > > > same lock. > > > > > > It is also possible that process A simply gets preempted while runnin= g > > > on CPU #1 by another task that also tries to use the acomp_ctx. The > > > mutex also protects against this case. > > > > Got it, thanks for the explanations. It seems with this patch, the mute= x > > would be redundant in the first example. Would this also be true of the > > second example where process A gets preempted? > > I think the mutex is still required in the second example. Migration > being disabled does not prevent other processes from running on the > same CPU and attempting to use the same acomp_ctx. > > > If not, is it worth > > figuring out a solution that works for both migration and preemption? > > Not sure exactly what you mean here. I suspect you mean have a single > mechanism to protect against concurrent usage and CPU hotunplug rather > than disabling migration and having a mutex. Yeah that would be ideal, > but probably not for a hotfix. 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? 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 =3D hlist_entry(node, struct zswap_pool, no= de); struct crypto_acomp_ctx *acomp_ctx =3D 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 =3D 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 =3D raw_cpu_ptr(acomp_ctx); + mutex_lock(&ctx->mutex); + if (likely(ctx->req)) + return ctx; + /* Raced with zswap_cpu_comp_dead() on CPU hotunplug */ + mutex_unlock(&ctx->mutex); + } +} + +static void acomp_ctx_put_unlock(struct crypto_acomp_ctx *ctx) +{ + mutex_unlock(&ctx->mutex); +} + static bool zswap_compress(struct page *page, struct zswap_entry *entry, struct zswap_pool *pool) { @@ -893,10 +916,7 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry, gfp_t gfp; u8 *dst; - acomp_ctx =3D raw_cpu_ptr(pool->acomp_ctx); - - mutex_lock(&acomp_ctx->mutex); - + acomp_ctx =3D acomp_ctx_get_cpu_locked(pool->acomp_ctx); dst =3D acomp_ctx->buffer; sg_init_table(&input, 1); sg_set_page(&input, page, PAGE_SIZE, 0); @@ -949,7 +969,7 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry, else if (alloc_ret) zswap_reject_alloc_fail++; - mutex_unlock(&acomp_ctx->mutex); + acomp_ctx_put_unlock(acomp_ctx); return comp_ret =3D=3D 0 && alloc_ret =3D=3D 0; } @@ -960,9 +980,7 @@ static void zswap_decompress(struct zswap_entry *entry, struct folio *folio) struct crypto_acomp_ctx *acomp_ctx; u8 *src; - acomp_ctx =3D raw_cpu_ptr(entry->pool->acomp_ctx); - mutex_lock(&acomp_ctx->mutex); - + acomp_ctx =3D acomp_ctx_get_cpu_locked(entry->pool->acomp_ctx); src =3D zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO); /*