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 987CFC282D1 for ; Thu, 6 Mar 2025 19:46:23 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B5CCA280018; Thu, 6 Mar 2025 14:46:22 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B0CB4280017; Thu, 6 Mar 2025 14:46:22 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 98750280018; Thu, 6 Mar 2025 14:46:22 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 7C5E6280017 for ; Thu, 6 Mar 2025 14:46:22 -0500 (EST) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 956ED1CE412 for ; Thu, 6 Mar 2025 19:46:22 +0000 (UTC) X-FDA: 83192157804.09.A8490BA Received: from out-184.mta1.migadu.com (out-184.mta1.migadu.com [95.215.58.184]) by imf18.hostedemail.com (Postfix) with ESMTP id CBD781C001E for ; Thu, 6 Mar 2025 19:46:19 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=jN4WEpW6; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf18.hostedemail.com: domain of yosry.ahmed@linux.dev designates 95.215.58.184 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1741290380; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=cPfH0at9b2Of8Fho2dIZEk48ulRtn7SnhprVF28eu+c=; b=Y4TsPv7MqGdueo5SNUdJhPH9EKmYk/zrorVECgOZEE19XegqI8bUosVuWHo7YZct9IRgzJ pG/WrrLup4bTK6v5lhiGTI2yZlNiGrvfO/p04GGVy+eGyn2Tt7uYLg+qsO95QB5+28xU0G w6hlrDvYxmr1wLAxe7FhygmnL2zZPN0= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1741290380; a=rsa-sha256; cv=none; b=aiSCKEvL7VLae4LLIL4LywVUkpaBk89QugRheTXSglrEB6OIW1EOwcN8H6iT+OS0hkf3RU gpjeoNk6iiU3C38jN5Cw5TbUHbGFP/eFgSF+i/MoxG7MxW/pi0XsMzhFj6SxGCmJGFdaJ4 9pcvZiPOP8zdXAa781++uYy2MDTyvO0= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=jN4WEpW6; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf18.hostedemail.com: domain of yosry.ahmed@linux.dev designates 95.215.58.184 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev Date: Thu, 6 Mar 2025 19:46:10 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1741290377; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=cPfH0at9b2Of8Fho2dIZEk48ulRtn7SnhprVF28eu+c=; b=jN4WEpW6DVv9F9al+waDzh6R4ZZn8+7pjJcValcBi2Ck0+8MA2GEIChy71yw2+NBngIfJh ni4fizmxwThEXcF8Z7XU/Xu2A1PEWF1RHjfMmnAGMQU9lJey5ZEOyiYy1LQWx1hC4Nzp8G lnRdE142z5pmwE+S9UDoqPjI4nx1g+Q= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Yosry Ahmed To: Kanchana P Sridhar 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, 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, kristen.c.accardi@intel.com, wajdi.k.feghali@intel.com, vinodh.gopal@intel.com Subject: Re: [PATCH v8 12/14] mm: zswap: Simplify acomp_ctx resource allocation/deletion and mutex lock usage. Message-ID: References: <20250303084724.6490-1-kanchana.p.sridhar@intel.com> <20250303084724.6490-13-kanchana.p.sridhar@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Migadu-Flow: FLOW_OUT X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: CBD781C001E X-Stat-Signature: 47dt7rik7jfgdu8mrj6w6ci3ux11juzs X-HE-Tag: 1741290379-927847 X-HE-Meta: U2FsdGVkX181OMJ9iHyIasndppgxciEybR9eB4X0KbMAxvKjA3GvWK+/bVHlfvGg77wvG+Lc6a5xRfJbBph8p4VD1P80gNH8WPhRYvdh62IC2jz7+z82/VmtHrIAiNR4YegZJMEtQtA+J4FeGk1rYoz/aE1zltnoEJxwTdKBcvDFUh1XV6zqlKl+QKOk72gM42xOuATR3W5FF/KsxBycBFGorfV0cge8IDMxT/2ft7r7a7he3KsyeUYL4nioRlu4O50LTxNPQ5PDT1qzXS7TKXh9OkDsFTMsWFBDkFKqJegycnQNVdj8sPB2ouYKoruuHRgR7mIZCrPpvdO0GKKuRmckTRwCxr5C2BirPkDJbPUg0y0R5/bcwIy7oqvXo257KjGcYIY7/B5avCnbgVfzEAsD5/PFWu4UMUCC1UTqCJ8CsLv4FDsiQDkUwdBLDVCcqLDPm388JOYiHsDOF/5GyS9nIBll4NJxbBx5bg/S8vjt4M1kwnPySZQTWE+oBpQW4Ax2d+H/ptxXzJtXfr6F8tS3k+vN9P/mWQ48e1tS034lmvpCaNXi+JhZFf2SidcCSWV/5jYaYLXldySNfADMfxkf/KBgLLSCYWJg8tuGNGCIoPtCFl3P9dW4znwV6bjXzxuMYWkL4xIfUWKpCa6+KENiCS/TO8x3OxhSMb5z6sMf1iyKUQ96u+gDkbMsOAzKMct6Ex54dl41G/vRWRVOlAOzaEV25x4wJdZ01vtglqN5vIENjkDi0WQM08hD24GuMKpmGiUsTwsxDmpx2FjPgIUZHNfJlCLWFQMrZtCKaxAjbv2QN8QRq2eYEpxGvJshSfi2x9faSt3yaQSEoyND8rpcz0sowC5IUiOpwPyFauAagxY0mHBCA4JSz62DW1K026/jdL8j9skeUGTv7XHLvsUyb8MbG3i2eycE9RF5ivpONu0l75CdOA8Xkr4tNTafmmqXOWdYS4Fwa/5WYBE ki7fe0em UItqfhJkf/PYosZl82fzEZjF7zF+fP4MFwBoo6yatZMktQg4NMkT1WLRLQnryoe+6wa/cUZ/AxFBd/FC55PcjZtJt7M10VyI3INjFGjGg1SEtwOwnEbnPWfi3bi8Zpp7l+JGTPVRVDogBXn/1hgliUH9Ffz0ccXSblWK0RgQnu9KIYRt3G426D+PEi4E1DJ0cQoMpXR/Mvp0EUKn1/XInXTZS+5y7jFsv9wiEEBvCnN92ipefSsltc3UoiGRQFtXUHibVOdLxK1hiBNYkPTczBMIBuzerePHVUxBa 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 Thu, Mar 06, 2025 at 07:35:36PM +0000, Yosry Ahmed wrote: > On Mon, Mar 03, 2025 at 12:47:22AM -0800, Kanchana P Sridhar wrote: > > This patch modifies the acomp_ctx resources' lifetime to be from pool > > creation to deletion. A "bool __online" and "u8 nr_reqs" are added to > > "struct crypto_acomp_ctx" which simplify a few things: > > > > 1) zswap_pool_create() will initialize all members of each percpu acomp_ctx > > to 0 or NULL and only then initialize the mutex. > > 2) CPU hotplug will set nr_reqs to 1, allocate resources and set __online > > to true, without locking the mutex. > > 3) CPU hotunplug will lock the mutex before setting __online to false. It > > will not delete any resources. > > 4) acomp_ctx_get_cpu_lock() will lock the mutex, then check if __online > > is true, and if so, return the mutex for use in zswap compress and > > decompress ops. > > 5) CPU onlining after offlining will simply check if either __online or > > nr_reqs are non-0, and return 0 if so, without re-allocating the > > resources. > > 6) zswap_pool_destroy() will call a newly added zswap_cpu_comp_dealloc() to > > delete the acomp_ctx resources. > > 7) Common resource deletion code in case of zswap_cpu_comp_prepare() > > errors, and for use in zswap_cpu_comp_dealloc(), is factored into a new > > acomp_ctx_dealloc(). > > > > The CPU hot[un]plug callback functions are moved to "pool functions" > > accordingly. > > > > The per-cpu memory cost of not deleting the acomp_ctx resources upon CPU > > offlining, and only deleting them when the pool is destroyed, is as follows: > > > > IAA with batching: 64.8 KB > > Software compressors: 8.2 KB > > > > I would appreciate code review comments on whether this memory cost is > > acceptable, for the latency improvement that it provides due to a faster > > reclaim restart after a CPU hotunplug-hotplug sequence - all that the > > hotplug code needs to do is to check if acomp_ctx->nr_reqs is non-0, and > > if so, set __online to true and return, and reclaim can proceed. > > I like the idea of allocating the resources on memory hotplug but > leaving them allocated until the pool is torn down. It avoids allocating > unnecessary memory if some CPUs are never onlined, but it simplifies > things because we don't have to synchronize against the resources being > freed in CPU offline. > > The only case that would suffer from this AFAICT is if someone onlines > many CPUs, uses them once, and then offline them and not use them again. > I am not familiar with CPU hotplug use cases so I can't tell if that's > something people do, but I am inclined to agree with this > simplification. > > > > > Signed-off-by: Kanchana P Sridhar > > --- > > mm/zswap.c | 273 +++++++++++++++++++++++++++++++++++------------------ > > 1 file changed, 182 insertions(+), 91 deletions(-) > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > index 10f2a16e7586..cff96df1df8b 100644 > > --- a/mm/zswap.c > > +++ b/mm/zswap.c > > @@ -144,10 +144,12 @@ bool zswap_never_enabled(void) > > struct crypto_acomp_ctx { > > struct crypto_acomp *acomp; > > struct acomp_req *req; > > - struct crypto_wait wait; > > Is there a reason for moving this? If not please avoid unrelated changes. > > > u8 *buffer; > > + u8 nr_reqs; > > + struct crypto_wait wait; > > struct mutex mutex; > > bool is_sleepable; > > + bool __online; > > I don't believe we need this. > > If we are not freeing resources during CPU offlining, then we do not > need a CPU offline callback and acomp_ctx->__online serves no purpose. > > The whole point of synchronizing between offlining and > compress/decompress operations is to avoid UAF. If offlining does not > free resources, then we can hold the mutex directly in the > compress/decompress path and drop the hotunplug callback completely. > > I also believe nr_reqs can be dropped from this patch, as it seems like > it's only used know when to set __online. > > > }; > > > > /* > > @@ -246,6 +248,122 @@ static inline struct xarray *swap_zswap_tree(swp_entry_t swp) > > **********************************/ > > static void __zswap_pool_empty(struct percpu_ref *ref); > > > > +static void acomp_ctx_dealloc(struct crypto_acomp_ctx *acomp_ctx) > > +{ > > + if (!IS_ERR_OR_NULL(acomp_ctx) && acomp_ctx->nr_reqs) { Also, we can just return early here to save an indentation level: if (IS_ERR_OR_NULL(acomp_ctx) || !acomp_ctx->nr_reqs) return; > > + > > + if (!IS_ERR_OR_NULL(acomp_ctx->req)) > > + acomp_request_free(acomp_ctx->req); > > + acomp_ctx->req = NULL; > > + > > + kfree(acomp_ctx->buffer); > > + acomp_ctx->buffer = NULL; > > + > > + if (!IS_ERR_OR_NULL(acomp_ctx->acomp)) > > + crypto_free_acomp(acomp_ctx->acomp); > > + > > + acomp_ctx->nr_reqs = 0; > > + } > > +} > > Please split the pure refactoring into a separate patch to make it > easier to review.