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 CC3E9C41535 for ; Wed, 20 Dec 2023 00:21:28 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3DF7A6B007B; Tue, 19 Dec 2023 19:21:28 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 3687A6B007D; Tue, 19 Dec 2023 19:21:28 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1E1B06B007E; Tue, 19 Dec 2023 19:21:28 -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 099936B007B for ; Tue, 19 Dec 2023 19:21:28 -0500 (EST) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id C59238047A for ; Wed, 20 Dec 2023 00:21:27 +0000 (UTC) X-FDA: 81585292614.24.ED33BE2 Received: from mail-io1-f53.google.com (mail-io1-f53.google.com [209.85.166.53]) by imf01.hostedemail.com (Postfix) with ESMTP id EF70140009 for ; Wed, 20 Dec 2023 00:21:25 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="DV8JX/LO"; spf=pass (imf01.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.166.53 as permitted sender) smtp.mailfrom=nphamcs@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1703031686; 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=WfYZl5ZrcfbZuMK/TopLSwtg7XNXi/JhwSQdO+z2kF8=; b=Ju1gs+dRwJiYPuzKVPPEnMTJC47yQQt69LGFQNYlM7C3npmZO+hEgCScfZ58O4dYvhoAUW nsljjYSvT+bclki5d5fq+jUHSLmye7ZFwkMCxRbxpPsJwhVkPwgL6ZNtsecxsUZDyu1JzE N3W+IfI47uC4qw3zt7UEQHAWFU7nfck= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1703031686; a=rsa-sha256; cv=none; b=K8dkEgG8/77OE3FqoupeSvbfw2d+CxIYiwxKatIryERG5XdIXAcIEtHkoYV4W/HyD9gP7f /ddfD9Gn6T/dMIlHepARknMsl05v6zkJsl5E0qA1nk9fEFpY0PuSSxfvibI+0w7dhhf1wH Jvb+tlQHI9UVYxRATUL5r2Dctgc4xXc= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="DV8JX/LO"; spf=pass (imf01.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.166.53 as permitted sender) smtp.mailfrom=nphamcs@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-io1-f53.google.com with SMTP id ca18e2360f4ac-7b7fd9e17d8so21116039f.3 for ; Tue, 19 Dec 2023 16:21:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1703031685; x=1703636485; 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=WfYZl5ZrcfbZuMK/TopLSwtg7XNXi/JhwSQdO+z2kF8=; b=DV8JX/LOvqYYFj6otff/7RhrbXOImBps3ZbKDTIMVq9PGgEs6A2/SoyW9pn3+/LwHB hUKu89mUQyahCjwtHskwD5rnNR2KvY5TrnoldCTKrN7ISvkL7HAChm0JekjCJpNQt8h7 /v4rtslgLhGUP8Kqt8uvSGCPdQlkezGiASLhEFggbx0UO02WWcMxq58uU9yD2K5oQBCR n9zKZedED4E/ahFGACkpvzAz0j437MlV3MdTAZKHLZIeqFBM3WgTTXy93oEggpq0JEve ADFl2i6gYNRK/Y3A+d0AqinyCyt+P6YWMH8v8cbFRijblAVeRQzP63pzxinJhSLDoLLs NpYg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1703031685; x=1703636485; 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=WfYZl5ZrcfbZuMK/TopLSwtg7XNXi/JhwSQdO+z2kF8=; b=GstL/kwYzZH/eRU3SmhskLQRmNnsb3XDf3EXUqfrEdDp7ywbpBt74p7RP9lGuVn8Mi CjmpnNn9eq9NDLRMu498QOimUTE7EjEO+0ciOBLd9wuNsQUJ6oPSCJ9v5FPtlyXfjwCa t9wosNz33Nmu8EWh6SIxh/AuaufgEACvEzyd2/MWdQGPrKvXjnS+KVZYUIlp7NbaZVge 4ZIM229iIK4rTn7Y6J54dA69knVLGsPBJh334iCAjqHYNlQ5MHVdMcVetZh/JWXjF0IF cpIhasvM9B1c5362cDb1hXd8beqJa1JHkn3fz62Q5/5CXi0Rvm2uL+pRD9dUCyOLDjTM 75Vg== X-Gm-Message-State: AOJu0YykJjiPfwXaDxMlaXnjNz/EQaZ4cee4gBcAO5XF2ysfA5/o+s1R up0P+Vz0HBW/AXw1gXsJSaTUYzTfnEw90Bpo4DU= X-Google-Smtp-Source: AGHT+IH8HoIT17fSSZbOHgb3YqJpC/ukBGjW+e32jgnnwlNoqvIMO1p3SvoENHMGYIpCveLGx+INexzbqhJKFp11Vsk= X-Received: by 2002:a05:6602:123b:b0:7ba:72f2:430c with SMTP id z27-20020a056602123b00b007ba72f2430cmr127233iot.6.1703031684948; Tue, 19 Dec 2023 16:21:24 -0800 (PST) MIME-Version: 1.0 References: <20231211052850.3513230-1-debug.penguin32@gmail.com> In-Reply-To: From: Nhat Pham Date: Tue, 19 Dec 2023 16:21:13 -0800 Message-ID: Subject: Re: [PATCH] mm/zswap: Improve with alloc_workqueue() call To: Ronald Monthero Cc: sjenning@redhat.com, akpm@linux-foundation.org, Dan Streetman , Vitaly Wool , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Chris Li Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: EF70140009 X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: 39mnk1uc44f6q1ot33r7wj9xzydxk7em X-HE-Tag: 1703031685-795648 X-HE-Meta: U2FsdGVkX1+u6NjEw4KWJEHmDeG6aWY1WP+RLCvaUDauL5dOeA4b7Iz5fd6s4i3qyRCqVU4Lh7Drjqiv7wuYTdCT0QhWjpQFFgq4Esfttt4dWdDOd9G9PKAfxokslVQ1a8ecLfUOFgJjfMFMwtSiTWnb3jiwRZFmqDMtDzfoaI2KpwJYSMHhoz3Gi9aiRfAxNnZy8/vGtsLu8ynlUNHD4lWzCG17Aj0KI81VkLCzqd/XN49VMl7wwBQlY2FlwVpJG9DpgfoAQpXg77ifkGGE6kmkWXpk0xLqRExvQBaYgm9u8O2PNDzo6KpkNexVir6PlIoF06fKdO6qUqDn+HXId4PGko6EYEHtmE3bCwRh2japLrPAx2eL2q6W4GErtlnnSkK8Y4UVQ8NyPZgqc2wmsMmccd1h2xJ6oX3qVszSIvwUOWRhOLRakK2wmJg844FkMg6QdxAjlvEWcTptXeToK8Xy+UgM/sw81qqOs5F4/v5RK1+3ctN1czAi7kdJNvYyFtETKdl+uHlMYkQgsZafuqoPyNntHVwHSVT+PRR3GYI7epv6E0SgahxpUYH1tu4LisMBD2Q8j+d6CeTvTU0ggxPDG7kn49JDObRvYxrr9f3v/Kh/PJDXAVZOPYVakdLOxnpBvsXyJU92gporeoUdARO3KIvteF3HXag8fEgmmqx4ebbslnV+IoBxMs+0x2xhTtL/nPoL+R/8sjY/X+5LW+jKa3+VwpKk2tYA651U+tSA899aWLGO4b267TTFof0ucpfPy4GNHT9kUmDHiNVmcEFqId76EcOdtqqUvN91QtJbn87DX3y8jIX7tu1R8ykU6n2Btp35+mPQNce/oQhbMMLIU2b4n27Nqo83J2GPkLT2DoiCWD3buG1CixGIiNtEZ4FYYNili370SSE3JtlLOiAwmV48etSAqiTXCn6mPmIiDF7Gq5oZNXnjra0aD0fBOJJJ31umQRkVR4XoIbA L4sW3/4Y WAiJfUA6gaP03b90KxleNuhZ14t3ltdtjabTLMDIUzIoju7zbCJEd0rvJGjWrBgiMOmMaovi+w6wG7JdT8xmM+Njj3Fptd1ijIPLXJo9Jyn3hkjHNJc2oPNvwqUTWHlJcR1qW/FqPmdFMTYZs+/qzjTqh85WvWdfKh+nvQGdQrRxpXTZQAysXP5Fs31h/cpy8dN5GKZiCBuimL7QKsQ8sLvXUhwLCIZNN7T5F9CBbp2fM79FOOkFi2nVuPlDmzjlDFweJputhurUbSamB7MtvSvP6MQq06h5nnhuM8zJKlk7PDFHLEBdGbzge55p3EA/rkK8doSWPmzEry/GCSV1JLFG69zR3lNKvIgmfBxekwGVycLG27MxHjgHiGepTJr371vAKwqe3ldCt0/bI7sr1238ApDUHXEiwGQKEpnwqKee/M6xx996+rE23aRIcayPqTyy1BxnnOR/DWfttpXaav+S5gXhuFgzM0DIYzQUb7HZf0Bg3RwPrRZX9yDJkL+Q/Z5UonepTctLL/3OoktyK7TpUVKLPe2A7Z7IuxBXaSfd0Ydyl17rK014KlwqDAo4vFtMB X-Bogosity: Ham, tests=bogofilter, spamicity=0.000038, 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 Fri, Dec 15, 2023 at 1:04=E2=80=AFAM Ronald Monthero wrote: > > On Thu, Dec 14, 2023 at 10:28=E2=80=AFAM Nhat Pham wr= ote: > > > .. > < snipped > > > > I should have been clearer. I'm not against the change per-se - I > > agree that we should replace create_workqueue() with > > alloc_workqueue(). What I meant was, IIUC, there are two behavioral > > changes with this new workqueue creation: > > > > a) We're replacing a bounded workqueue (which as you noted, is fixed > > by create_workqueue()) with an unbounded one (WQ_UNBOUND). This seems > > fine to me - I doubt locality buys us much here. > > Yes the workqueue attribute change per se but the existing > functionality remains seamless and the change is more a forward > looking change. imo under a memory pressure scenario an unbound > workqueue might workaround the scenario better as the number of > backing pools is dynamic. And with the WQ_UNBOUND attribute the > scheduler is more open to exercise some improvisations in any > demanding scenarios for offloading cpu time slicing for workers, ie if > any other worker of the same primary cpu had to be served due to > workers with WQ_HIGHPRI and WQ_CPU_INTENSIVE.Although normal and > highpri worker-pools don't interact with each other, the target cpu > atleast need not be the same if our worker for zswap is WQ_UNBOUND. I don't object to the change per-se. I just meant that these changes/discussion should be spelled out in the patch's changelog :) IMHO, we should document behavior changes if there are any. For instance, when we switched to kmap_local_page() for zswap, there is a discussion in the changelog regarding how it differs from the old (and deprecated) kmap_atomic(): https://lore.kernel.org/linux-mm/20231127160058.586446-1-fabio.maria.de.fra= ncesco@linux.intel.com/ and how that difference doesn't matter in the case of zswap. > > Also noting that the existing wq of zwap worker has the WQ_MEM_RECLAIM > attribute, so is there a rescue worker for zswap during a memory > pressure scenario ? > Quoting: "All work items which might be used on code paths that handle > memory reclaim are required to be queued on wq's that have a > rescue-worker reserved for execution under memory pressure. Else it is > possible that the worker-pool deadlocks waiting for execution contexts > to free up" Seems like it, but this behavior is not changed by your patch :) So i'm not too concerned by it one way or another. > > Also additional thought if adding WQ_FREEZABLE attribute while > creating the zswap worker make sense in scenarios to handle freeze and > unfreeze of specific cgroups or file system wide freeze and unfreeze > scenarios ? Does zswap worker participate in freeze/unfreeze code path > scenarios ? I don't think so, no? This zswap worker is scheduled upon the zswap pool limit hit (which happens on the zswap store/swapping/memory reclaim) path. > > > b) create_workqueue() limits the number of concurrent per-cpu > > execution contexts at 1 (i.e only one single global reclaimer), > > whereas after this patch this is set to the default value. This seems > > fine to me too - I don't remember us taking advantage of the previous > > concurrency limitation. Also, in practice, the task_struct is > > one-to-one with the zswap_pool's anyway, and most of the time, there > > is just a single pool being used. (But it begs the question - what's > > the point of using 0 instead of 1 here?) > > Nothing in particular but I left it at default 0, which can go upto > 256 ( @maxactive per cpu). > But if zswap worker is always intended to only have 1 active worker per c= pu, > then that's fine with 1, otherwise a default setting might be flexible > for scaling. > just a thought, does having a higher value help for larger memory systems= ? I don't think having higher value helps here tbh. We only have one work_struct per pool, so it shouldn't make a difference either way :) > > > Both seem fine (to me anyway - other reviewers feel free to take a > > look and fact-check everything). I just feel like this should be > > explicitly noted in the changelog, IMHO, in case we are mistaken and > > need to revisit this :) Either way, not a NACK from me. > > Thanks Nhat, for checking. Above are my thoughts, I could be missing > some info or incorrect > on certain fronts so I would seek clarifications. > Also thanks in advance to other experts/maintainers, please share > feedback and suggestions. > > BR, > ronald Also +Chris Li, who is also working on improving zswap :)