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 A3851C27C4F for ; Thu, 13 Jun 2024 16:08:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D726D6B00CB; Thu, 13 Jun 2024 12:08:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D22E06B00CC; Thu, 13 Jun 2024 12:08:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BC2D86B00CD; Thu, 13 Jun 2024 12:08:20 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 9D0BE6B00CB for ; Thu, 13 Jun 2024 12:08:20 -0400 (EDT) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 47FE91C054B for ; Thu, 13 Jun 2024 16:08:20 +0000 (UTC) X-FDA: 82226347560.30.5E98499 Received: from mail-oo1-f54.google.com (mail-oo1-f54.google.com [209.85.161.54]) by imf01.hostedemail.com (Postfix) with ESMTP id 60A9040003 for ; Thu, 13 Jun 2024 16:08:18 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=iuFBq2ry; spf=pass (imf01.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.161.54 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=1718294897; 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=yqHB7d/fQOBxu70Lq3/veRtB4jM3A8VBQDLaIwljpU0=; b=Jtdmz3u01/v7OuK2xmM669UkOts8D3poh3uHKBMH7eMYo7t/5wAStKzPczKe959F+FrxiM LUVJjX2w9arnmzVSQ4kbhclIcF4dDg7lFoScIxPK02tiAX/wSwkZlFOWJG1Jh7AmBbHGYQ 2tXWVBm2XEf/uRSi8h+JeYujGmk5BN8= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=iuFBq2ry; spf=pass (imf01.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.161.54 as permitted sender) smtp.mailfrom=nphamcs@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1718294897; a=rsa-sha256; cv=none; b=cXyUVlmht8uAcrVXdTalX6i6SwT/ZLmlS3++2Ld9W5/Rx649mlEx0/ZyNr+n5GFuHPWZq8 73FI6zaPDLKLidsDd1yIViHQy8RTRvk1SRohVH+tvltdSFIJg6aS75T6GfX2GDzF/FIzhC u5QG9KTgwmBpPrt+FkFj4bcmTKGddPg= Received: by mail-oo1-f54.google.com with SMTP id 006d021491bc7-5bad66c6e27so506536eaf.3 for ; Thu, 13 Jun 2024 09:08:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1718294897; x=1718899697; 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=yqHB7d/fQOBxu70Lq3/veRtB4jM3A8VBQDLaIwljpU0=; b=iuFBq2ryLMIgFX/raOeK+jv7Q/WCG17jLsoXSX6BAjgqmPiLueMlR5js2W0rfifgKZ pyasDFlMVDDuNJv5wH76GHmY2T7cWfyHkGca9baxchhqKIMGl24aQ7jTxmBoYH61Rgv0 0Mhusjebb4jNNLkmUp7R+6bWcL12EGcVJ6eWvVTeHp/RQvjM6ZuL1ec0zVNgF7aJd83Y Xh5lMOLKSOfKPq3zalXYJB4oIumyKDOKV6caXORXyx884wmNgj4I9NwmhoggRecSgPNh MH2SlmhXKPfo5OBxZkej6B9hCTl0cvNw6xuqOKmxJ5xfWm16FCVV3TJGceiImKs8w03W A0OQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718294897; x=1718899697; 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=yqHB7d/fQOBxu70Lq3/veRtB4jM3A8VBQDLaIwljpU0=; b=es1LEn8zH6oo2KO4LO4CQxU3NKnEOTILISDWcasRfnlRSV61L0Lj13cHPEbvR1EXPh tG4nNsQZXYYYeSRU3ac5MfdK3z91Xpkixot812ipQr2gy3wFyXX1GYHn0ZqjiB8pIrL2 ElMtJq/8n9I7wnRDxxAhuWLyGurubafLD6fAxyOhQwgx2gvbSOWasy9iBgPk1ihx9hu9 KWPXLEhJEjTW3k6gIC/v2WqtmqsOMNNrq+cpx1svjv52fGKNHCVwEIi/jZJWspfdEjgo 5smd0wZI7FB0XBKADflfGw9Lvx3HE38bfH/vLOZN8zN5w9gbiPKnyY97e9QrQkQZO0q+ YVAQ== X-Forwarded-Encrypted: i=1; AJvYcCUSDOt92qLkJksAzdD0TgNsopYClVp1OQcuczoJ1iZtSqSVDgcflTiF6FUXBbQw80k8+vRo54DXii00c5MLzRDdl7I= X-Gm-Message-State: AOJu0YwzNlIat5P/atUuYL+IjgAsd+c22EfVpqO9D+sb8AZGQkqwfBFY p3qU9f76HxVhz6DfwloBAobashP/6o7O6KMF/pUCMCf7cLmY7Al6vdbx3LyfChfbACIvBIPfzN9 40Hpf7Oziu6lbI3x6OlCj4tn+Hrw= X-Google-Smtp-Source: AGHT+IGncy3iZuOU+YBrf6fE4opBg2nOmjMMNj1/60e1lgzQa7ypMxuR2TNW/Ek0QXpCbT3mDrtdp+Ov/ybyBvgzDGY= X-Received: by 2002:a05:6358:60ca:b0:183:d2fa:ff5c with SMTP id e5c5f4694b2df-19fa9e3ed22mr29344055d.12.1718294896891; Thu, 13 Jun 2024 09:08:16 -0700 (PDT) MIME-Version: 1.0 References: <20240608155316.451600-1-flintglass@gmail.com> <20240608155316.451600-2-flintglass@gmail.com> In-Reply-To: <20240608155316.451600-2-flintglass@gmail.com> From: Nhat Pham Date: Thu, 13 Jun 2024 09:08:05 -0700 Message-ID: Subject: Re: [PATCH v1 1/3] mm: zswap: fix global shrinker memcg iteration To: Takero Funaki Cc: Johannes Weiner , Yosry Ahmed , Chengming Zhou , Jonathan Corbet , Andrew Morton , Domenico Cerasuolo , linux-mm@kvack.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 60A9040003 X-Stat-Signature: hxgkb3un9ws5rc4dqf8e4k4uhz1mhr3y X-HE-Tag: 1718294898-862656 X-HE-Meta: U2FsdGVkX1+0v8frlOZDP8HtGS7COSQ0NG5zKXKimBfifUGt9CU8vaPejB0ex3+/aAQFvjKmsli8OGUWpVJiXMdzXTC6/snCuysYioqEebpBusftB1TV5stagAcUUEwf+CN8opa6Wj7N2f7qt+VAILKGOU5/dea3alm4gq1SBktzuEHYIJ6TDUMBqRAVTZcoB8+jlmHgYHivEkfXvnEnnaut3mfszPQLvweyqWLoe7Y8XQ/UaJwA3+3OuSjElOpt/6Kj9TBOjJMlCY0qvVP1Cwnf+oiigLG/7E7uTO+2uDoDMok9bIPEGNw9saKOQqb0wJLdAYqNfLGCysto3++pCSbzqQnjDM2Wtjbw/TLen/1SZxeBF8nqKjKDMMHIulBO91uCxLzBb3XS7pZNVw50w7Q6aOBIwobpaVVr7rxNwHh7DW6MQAwFrc4EauDIXJzYefjoKhq3xfpTlAvlrdbwiT6rK/lxCSI8yCz31Um0WRF/b0Fr1O2dWzwJTesulhbUhamktbMy9LXLCs0t3/zcIRVlZLCdCUFI8bJvLLO1D67cKPlc5ESxMhbhdAmP1qupjGMcwowj4UGnlTCdScXNM4gdMmXH5jKI1YzdCj9jbaPRHHQNy1qhyMHKQhyJPw2iaBiIglgB93lvTf+7d4OaV3y+x7zY1YXxw5ZsbkpQZvacjPb5pIIIi+hACrYGRP5TWEbyd4NDcoKDA3qXCXj+WYbbUOctt4cbmnMjewjQB9F98sNjELLrI8j2rJ7nsPKXXszMuxex+X8Y6oBxk60NbhXOB5cYhjM9NMCN3Pbf0BH0xE7lhWrNiacbSw5Xlu2UMcrAhF6wNsWl2bIZ8VkWmyavCFXCCD0ODWLjH3q4AOvXXBznF9yp/TB3xOv1lGL25NOMgXHJ8Qtgijem+IiYEKLPwCQGeesRMY4heR6YZRC6asdhbTHG7dhHee/YtJwnburGXXEty7xe+S7itV8 qsKuK2I9 kDxB4h6oWne+ecH/8KzsR3qT3sBhN5EIgDUXtLlyPYa0OrJ8iismnf7a2I4a1L0sjpq2AXZyvzODyZdZxCtVvt55HKkQfXwSY8Ns82g4NbR66hUjb5qkM1wmQbfcWFVOvCLbPdUaG7SYT72f3mM3enOncoC3vam4FRGAAwrAnM2t47ouiufvqpaYD/CaS0YKk8DlJSoaIfKze9lOjWndMqG23HCxvNLNOnJ6DJRqar6qT1IWwPjndQCHwxu5ih+i7WApuvCcpKKJMnuaaxqgLNJ7G0qRvpXjisYp4xXkyaHODk9WOSDU3HqSD6BLUT/0dgKoG X-Bogosity: Ham, tests=bogofilter, spamicity=0.000001, 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 Sat, Jun 8, 2024 at 8:53=E2=80=AFAM Takero Funaki = wrote: > > This patch fixes an issue where the zswap global shrinker stopped > iterating through the memcg tree. > > The problem was that `shrink_worker()` would stop iterating when a memcg > was being offlined and restart from the tree root. Now, it properly > handles the offlining memcg and continues shrinking with the next memcg. > > This patch also modified handing of the lock for offlined memcg cleaner > to adapt the change in the iteration, and avoid negligibly rare skipping > of a memcg from shrink iteration. > Honestly, I think this version is even more complicated than v0 :) Hmmm how about this: do { spin_lock(&zswap_pools_lock); do { /* no offline caller has been called to memcg yet */ if (memcg =3D=3D zswap_next_shrink) { zswap_next_shrink =3D mem_cgroup_iter(NULL, zswap_next_shrink, = NULL); memcg =3D zswap_next_shrink; if (!memcg && ++failure > MAX_FAILURE) { spin_unlock(&zswap_pools_lock); return; } } while (!zswap_next_shrink || !mem_cgroup_tryget_online(zswap_next_shr= ink)) spin_unlock(&zswap_pools_lock); /* proceed with reclaim */ } while (...) This should achieve all the goals right? 1. No restarting from the top, unless we have traversed the entire hierarch= y. 2. No skipping over zswap_next_shrink updated by the memcg offline cleaner. 3. No leaving offlined zswap_next_shrink hanging (and blocking memcg killing indefinitely). 4. No long running loop unnecessarily. If you want to be extra safe, we can put a max number of retries on the offline memcg case too (and restart from the top). 5. At the end of the inner loop, you are guaranteed to hold at least one reference to memcg (and perhaps 2, if zswap_next_shrink is not updated by the cleaner). 5. At the end of the inner loop, the selected memcg is known to not have its cleaner started yet (since it is online with zswap_pools_lock held). Our selection will undo the cleaner and hold the memcg hostage forever. Is there anything that I'm missing? We are not dropping the lock for the entirety of the inner loop, but honestly I'm fine with this trade off, for the sake of simplicity :) If you want it to be even more readable, feel free to refactor the inner loop into a helper function (but it's probably redundant since it's only used once).