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 7DA1FC3DA64 for ; Fri, 26 Jul 2024 02:48:06 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E2C0F6B0085; Thu, 25 Jul 2024 22:48:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id DE75D6B008C; Thu, 25 Jul 2024 22:48:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CA33F6B0092; Thu, 25 Jul 2024 22:48:05 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id AAC2B6B0085 for ; Thu, 25 Jul 2024 22:48:05 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 59506140E9C for ; Fri, 26 Jul 2024 02:48:05 +0000 (UTC) X-FDA: 82380369330.28.FD9CF06 Received: from out-180.mta1.migadu.com (out-180.mta1.migadu.com [95.215.58.180]) by imf28.hostedemail.com (Postfix) with ESMTP id 1540CC0005 for ; Fri, 26 Jul 2024 02:48:02 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=oF9yBUp6; spf=pass (imf28.hostedemail.com: domain of chengming.zhou@linux.dev designates 95.215.58.180 as permitted sender) smtp.mailfrom=chengming.zhou@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1721962059; 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=5LIQEBI1t67ThZTPmVwWyA85Ri+bycAvzIyXFhMw+wQ=; b=dD/1flA3SutbamRjFpISyimafSuUZER9iJVH30yYibEfknRZBJdU0SAVW9eY31msERAwvc W5v5IWNXgDqSayVMXcSNe2drahJuUAWAV8krh9/Nj1IHBba8qEx5lc4LKHV9eFRkQh5fDe 4TMXUDscmLNB4TWvVHZU8nwP23EOP+M= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=oF9yBUp6; spf=pass (imf28.hostedemail.com: domain of chengming.zhou@linux.dev designates 95.215.58.180 as permitted sender) smtp.mailfrom=chengming.zhou@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721962059; a=rsa-sha256; cv=none; b=4WzTe5Jh0K+Rl85o0u7PN6jNv7R6spC1OZW+2bTqKb7XyMpt03j4qYtA804kaZeFwprRKH S4uwOtWppTxJi+linkIsNPAXCRgG/V3cyAFoO2/d9GhzD6E7kjoKNICI9Ieocj+Yt1HOlI zZ9O1ULD+BrwncIRf6eR7aoyLzOKiNU= Message-ID: <19b6a0ab-d408-4239-ad30-34a474b34a8f@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1721962080; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5LIQEBI1t67ThZTPmVwWyA85Ri+bycAvzIyXFhMw+wQ=; b=oF9yBUp6aNQslS2JQYYo+Bucj7qrTdu87hmYH6U34SEjBw63HlCXF6HepYTg3rF/ZFtQ/0 n/m9CimfJoqLsPk/z79g5HdWjLgCDeI65VC43E2Yz8IegRCe6VPXAXXHz/7K+cCfj3k+ep F5nEPAqMXrWDgfP+wl0k67C0G1P/l2w= Date: Fri, 26 Jul 2024 10:47:48 +0800 MIME-Version: 1.0 Subject: Re: [PATCH v3 1/2] mm: zswap: fix global shrinker memcg iteration To: Takero Funaki , Johannes Weiner , Yosry Ahmed , Nhat Pham , Andrew Morton Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20240720044127.508042-1-flintglass@gmail.com> <20240720044127.508042-2-flintglass@gmail.com> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Chengming Zhou In-Reply-To: <20240720044127.508042-2-flintglass@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT X-Rspamd-Server: rspam03 X-Rspam-User: X-Rspamd-Queue-Id: 1540CC0005 X-Stat-Signature: d7xo3jbk38i5us1touhudyqtrftom8tr X-HE-Tag: 1721962082-553590 X-HE-Meta: U2FsdGVkX1/clKbetFkvE8wKthC4qq9SNMSQBuP4SVPJZObCFya0i48ybmUh2s34i+d7QB7dsF7fSDUo65T+A5B5Cd/qWTjrKCpqfVzRp2GCnEhuDHa4pQ0YptlhQvmFGgQcRtg074qoTKyE51BiDO16yylfuCz3MtofG0JOiNMiAqM5ZbqyGABJAZ1qzwPGDfuGrS8vw1cz/SffrZpnCW66XEviJTOIRJXEYPAzAm7Nx5sbAOa/PeY0APEKLWZ/r+nQvj1NFtu5/JO4JRYfAOkFj02AH28xfFJWTlgCbOTpcxytXuNKny2uGBg8/uvjlIp/JyBZ6qWekAsxpS1b91JOUU2cMBTik+cbcbxK8PgqCJbM2hHnJACTqg4yt6xAfcjZ1l3rTlHWk99a5/ygDkNb8rUkuSU3qHYpkpOndOLqJQBZphnnjvw6u3dk98DfNkrRopc/05W4d2OwabFUmsnGv+JJAVtWQ8ckGqQl4QAo3qICFXxwt6JrLABkT+En6A3Z7KDZJlzh+7mTi6MRYU8TdNtdKk2HDg6bCYbH+fdWO7WPbG9qXkG1jyAfTyHPD+S7R3vuDkNHD484efIeYVRh8uICs7f3fX5C2BrZSdBdw5waljXRjp5aCngM1zpRpVowhYyyD8IpoGkgW1vsK+QZhvWXLOosoGAFOVrBD+Jjz5SHfO9u0WDGus4XO9unGy7zzSKuBh7gk2B7ai5Hl1PPVsVA5leOMtWXO5JdrsJch1j/OhPk1fpc0Odm8gfHuo4vDuhRyHgVFEpWbOMf2uHAaOBq3rkdgF/FglYXmUDd1eE+o7B+cGmmp8C/0SNS/C7MM0CSBuskKiviQa/N7b3jvEuvzxTAtoDqW2aD8dtYLCbBub7ftMmWi2LHjvyK1keDUc3ipHod5lWAgNXzLRIXhKAK17BNtwqbnID0hbnKjN2ISYvNTKW9otMtWUYxA5801zcGJJE92lGWSl5 Azh0Om/v firLmZQvP6YaCkZTIE599s2KkAXqe1uxRTJr5VWzibq72H6OBUlFAjdzcnc7ixKM82td/E7nuim5PofGnetoxpkzIoJQSx9JmSr1fb1dHILrFTQS9/PRwDQqM1CVIMwVreTIuhBQwNYTkS5qLNZlxYZYDfA== 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 2024/7/20 12:41, 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 offline memcg and continues shrinking with the next memcg. > > To avoid holding refcount of offline memcg encountered during the memcg > tree walking, shrink_worker() must continue iterating to release the > offline memcg to ensure the next memcg stored in the cursor is online. > > The offline memcg cleaner has also been changed to avoid the same issue. > When the next memcg of the offlined memcg is also offline, the refcount > stored in the iteration cursor was held until the next shrink_worker() > run. The cleaner must release the offline memcg recursively. > > Fixes: a65b0e7607cc ("zswap: make shrinking memcg-aware") > Signed-off-by: Takero Funaki Looks good to me! With other comments addressed: Reviewed-by: Chengming Zhou Thanks. > --- > mm/zswap.c | 77 +++++++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 56 insertions(+), 21 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index a50e2986cd2f..6528668c9af3 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -775,12 +775,33 @@ void zswap_folio_swapin(struct folio *folio) > } > } > > +/* > + * This function should be called when a memcg is being offlined. > + * > + * Since the global shrinker shrink_worker() may hold a reference > + * of the memcg, we must check and release the reference in > + * zswap_next_shrink. > + * > + * shrink_worker() must handle the case where this function releases > + * the reference of memcg being shrunk. > + */ > void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg) > { > /* lock out zswap shrinker walking memcg tree */ > spin_lock(&zswap_shrink_lock); > - if (zswap_next_shrink == memcg) > - zswap_next_shrink = mem_cgroup_iter(NULL, zswap_next_shrink, NULL); > + if (zswap_next_shrink == memcg) { > + do { > + zswap_next_shrink = mem_cgroup_iter(NULL, > + zswap_next_shrink, NULL); > + } while (zswap_next_shrink && > + !mem_cgroup_online(zswap_next_shrink)); > + /* > + * We verified the next memcg is online. Even if the next > + * memcg is being offlined here, another cleaner must be > + * waiting for our lock. We can leave the online memcg > + * reference. > + */ > + } > spin_unlock(&zswap_shrink_lock); > } > > @@ -1319,18 +1340,38 @@ static void shrink_worker(struct work_struct *w) > /* Reclaim down to the accept threshold */ > thr = zswap_accept_thr_pages(); > > - /* global reclaim will select cgroup in a round-robin fashion. */ > + /* global reclaim will select cgroup in a round-robin fashion. > + * > + * We save iteration cursor memcg into zswap_next_shrink, > + * which can be modified by the offline memcg cleaner > + * zswap_memcg_offline_cleanup(). > + * > + * Since the offline cleaner is called only once, we cannot leave an > + * offline memcg reference in zswap_next_shrink. > + * We can rely on the cleaner only if we get online memcg under lock. > + * > + * If we get an offline memcg, we cannot determine if the cleaner has > + * already been called or will be called later. We must put back the > + * reference before returning from this function. Otherwise, the > + * offline memcg left in zswap_next_shrink will hold the reference > + * until the next run of shrink_worker(). > + */ > do { > spin_lock(&zswap_shrink_lock); > - zswap_next_shrink = mem_cgroup_iter(NULL, zswap_next_shrink, NULL); > - memcg = zswap_next_shrink; > > /* > - * We need to retry if we have gone through a full round trip, or if we > - * got an offline memcg (or else we risk undoing the effect of the > - * zswap memcg offlining cleanup callback). This is not catastrophic > - * per se, but it will keep the now offlined memcg hostage for a while. > - * > + * Start shrinking from the next memcg after zswap_next_shrink. > + * When the offline cleaner has already advanced the cursor, > + * advancing the cursor here overlooks one memcg, but this > + * should be negligibly rare. > + */ > + do { > + zswap_next_shrink = mem_cgroup_iter(NULL, > + zswap_next_shrink, NULL); > + memcg = zswap_next_shrink; > + } while (memcg && !mem_cgroup_tryget_online(memcg)); > + > + /* > * Note that if we got an online memcg, we will keep the extra > * reference in case the original reference obtained by mem_cgroup_iter > * is dropped by the zswap memcg offlining callback, ensuring that the > @@ -1344,17 +1385,11 @@ static void shrink_worker(struct work_struct *w) > goto resched; > } > > - if (!mem_cgroup_tryget_online(memcg)) { > - /* drop the reference from mem_cgroup_iter() */ > - mem_cgroup_iter_break(NULL, memcg); > - zswap_next_shrink = NULL; > - spin_unlock(&zswap_shrink_lock); > - > - if (++failures == MAX_RECLAIM_RETRIES) > - break; > - > - goto resched; > - } > + /* > + * We verified the memcg is online and got an extra memcg > + * reference. Our memcg might be offlined concurrently but the > + * respective offline cleaner must be waiting for our lock. > + */ > spin_unlock(&zswap_shrink_lock); > > ret = shrink_memcg(memcg);