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 DBB36C3DA70 for ; Tue, 30 Jul 2024 19:18:19 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 39DF96B007B; Tue, 30 Jul 2024 15:18:19 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 34DC86B0082; Tue, 30 Jul 2024 15:18:19 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1EF5B6B0083; Tue, 30 Jul 2024 15:18:19 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id EED866B007B for ; Tue, 30 Jul 2024 15:18:18 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id A5435C049A for ; Tue, 30 Jul 2024 19:18:18 +0000 (UTC) X-FDA: 82397379876.27.37A8D19 Received: from mail-qt1-f178.google.com (mail-qt1-f178.google.com [209.85.160.178]) by imf13.hostedemail.com (Postfix) with ESMTP id E4D1920005 for ; Tue, 30 Jul 2024 19:18:16 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Dp7xUMJh; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf13.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.160.178 as permitted sender) smtp.mailfrom=nphamcs@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722367056; a=rsa-sha256; cv=none; b=OY2IY1uQCInFRdWdxJoIX7FOdvXDncMd7vsvcsC7hZpN3eeFGsq1wCMfez9fJUHNlW5p5Z C2JgzUldatX9brLwUWqv9K6vaJ5OCcVFvqHIRVXWHXNmApJSKYLFkFPh2hD+ExBFEbdJmj sDAtWpdosrPV9ygmXqtIFKQ2v3qtXLg= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Dp7xUMJh; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf13.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.160.178 as permitted sender) smtp.mailfrom=nphamcs@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1722367056; 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=H2sF0eceqoXNG5LGtMfw8S8kHnapYiMxImOpIZ5orbg=; b=gAXjAQo5n0Dnb5dUtCVdLnE1Vem/5eKeI9U87I871NLtfAe57xHHbG0zK8P7Cs8+5oQaIW TZzDmUYar4kkJQpXEQnWX8Q3jah3zB8MwCMNb+Sb2HJPH4Vm90FWbdaOp3pChWJc7vdxv6 oRz9I65ChEYm04X9Jm5/q53h51RkQ3I= Received: by mail-qt1-f178.google.com with SMTP id d75a77b69052e-44fea2d40adso30871121cf.0 for ; Tue, 30 Jul 2024 12:18:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1722367096; x=1722971896; 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=H2sF0eceqoXNG5LGtMfw8S8kHnapYiMxImOpIZ5orbg=; b=Dp7xUMJhB5TnNY7vSaJdpJpICGr2bOIxi/cTIGhslF3kze9/l8g4oz0Y5jFsof31Ho OHWs+fyyDxqw6X6FLcVrVQUr5B2MLxakDcamdfB4ijG8D+YfMdu3CYSq6ANfd2LZl24t iOYjGLQZD3nOl6SVqa/rT15jrpncIle1ZyTR0fyQQz4ryRYDddspMrqcsPuOVxuAWS2q 3wC7QuvxPE1zBrqF9Qwn6ynDQgcvSrIe1JtgBMIhuph0QPMjbNplyhF6VNfwDSIHKgc9 sBBigLE4StQDYN8B1GLc0ZDBSPPcbSuh88EtmW0pVLCG6h0S+0q1p3Wvh8WtumI3O7re KCKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722367096; x=1722971896; 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=H2sF0eceqoXNG5LGtMfw8S8kHnapYiMxImOpIZ5orbg=; b=FvbjyfqhYL4/DBX8Ynto2xjiydFC+y9c/Z6YdhFbcXEMilUAVUEPdB9uz77hBqQ6LA 1h07e4yy0F8ktksSDYvRuH+WOx8MJZspB0UexD9p/ejy893Tyli2Bk3SLT5d93qulbCC WLcsPlwHyJFRcEmxr44dnzZ8TVlVedQ3SWhNX5CnYd3SXWRXaa73YiX1kvFsyfmK6J9O 7+1tLLv1nRihtHTdaDHjt8gdYlntHpVsZoji3lIqz7rAvMjNkG1C+Fzu0c7k4fqaSAqc UzxIIQZXwzTZrRBrXZKRuC1UknOtAY7r+c8TFuFvdd4HmxNGmHUWoblMCwPvX/UUt4EH 0J7Q== X-Forwarded-Encrypted: i=1; AJvYcCWt2Ej1gLQaAoa4/kI3tiHp4l/XwleslmOfvXGRMiadyezC3nYJG1BdTN3nVJijC4PtkUEE8J184h6EiWTNmr8H3qI= X-Gm-Message-State: AOJu0Yy14IQixKh4huRAAPwi5uLnlEhCV7jsbj1bFChTZEBSdzw5UTeB gznIDg3lGBc5bxhTFfeHp3qIe5cQxIi54KsqUX/xtT0FhumD8YkTaxCd/vEBXZpQcAks5c+SEL+ VBl9AWQjyllbDQrfDagYyJ4mDOjk= X-Google-Smtp-Source: AGHT+IEznjHMLlgyMAbP6wvlIkg8tzvcopc13YOI3gNan43f7O7Hd/f6lP9/dVui+WirrqkOk8TaYGDyHaNjHQj+wKI= X-Received: by 2002:a05:6214:5019:b0:6b7:9fe3:b2be with SMTP id 6a1803df08f44-6bb55af364amr153630796d6.53.1722367095856; Tue, 30 Jul 2024 12:18:15 -0700 (PDT) MIME-Version: 1.0 References: <20240727230635.3170-1-flintglass@gmail.com> <20240727230635.3170-2-flintglass@gmail.com> In-Reply-To: <20240727230635.3170-2-flintglass@gmail.com> From: Nhat Pham Date: Tue, 30 Jul 2024 12:18:04 -0700 Message-ID: Subject: Re: [PATCH v4 1/2] mm: zswap: fix global shrinker memcg iteration To: Takero Funaki Cc: Johannes Weiner , Yosry Ahmed , Chengming Zhou , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: E4D1920005 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: ie8k58pxfk61ogsmq33xgzfckcffn3q7 X-HE-Tag: 1722367096-850131 X-HE-Meta: U2FsdGVkX1+yV9ES/F6A9q3ycBCbm3RAbEAwPZhjvcUOSqO/6pFlFM8x6EOmNPHPQqKU+WbYGaS/PUvwZqNLEAXyaH9Kml8iS2kUMjSLSZdSJb+SD8YB1+U/QXYunIWc9asu6X8jWmrR/I9LNCiKWJqCyTG8J3b99Vs3QH2oK/a1SwLjyJn0rfJ4qL7XSkAJ9xlvzZ3JsnLP4jVWrqq7iimEgdbAdPxw9JflfIuD6fG8aBxBctTGt/DDtvPwH/BXVHrJQN4r0aam0R3jWC7xZirugRymP0zeaNveCaYYFIhUEE2qgr30uaxWQN+YTSd1uAgqGZor9yVn4QRaaYhh+VZTAQX+t7ppCG+BRiehWzGmYN5rpq2WWoNwp+kuvAaB0otcAQWQuMPR8QogSiRCyfzJpb8Uuj7Y50/7y18fDc4nvos9apg5FSoMgD9J/752O8AkXTfadFJmNiPQoD4/GPe89J32WrG0ED/ydrbt8GPC6ngy8H9au7oZJ9Mhks7SxP6Ozyq9jwvR/E6TmlngqkG4EJu1Yob0t/z9z+PjnxvUe4BLbysMDBdF3T5Y01v7epsHTnv+v7XzUIXd8hjrxAwPbKOefhUYU6CDARTWXYKgcPICYmri6Vb4WivPReoEXxZO1MS574ZfnUk8d239uKbm6r5zknmGlVWAj2K956DVTdcuLt+qmOpF1+JDtWF3CCpITMfCIlzSR0d2dVl8o3i83uXolocUZB+PiAs1Jd0hQvMFYBYDPC6UTWmaz296wETwuTWISeP0aGuEPdC/7rPUDGupA5jOufLgtKrwwLIor1EzAfEWNU5tSdvBPJ1veEqs8dskSPWKUzP46ld+w6JNyh5y0aPm0p6CT5wTNbp1VirDuOmjhkbbrReMAw8m1UD766jbQ+YJyvrn6Im1aBLKh5N60FL/nRTN8ZyJIATFMITUiJnoFDWeuIhHZk2UtxS62kwyjT2eBortO4i Ieru6Mjl nAl6ZpGb56BrGmDhAgrkBeT32MTTcT3D09SIZeiwqtgDTlEL5iikaedyMOyKvX07+P3L7kjbCvd/veuPhFPf6V+QwoIc8xV53mFw1eJhMpmR1QSJBr2rM15gtZvskdOeBR9YETIhuB/0p1FbvldF4SGHBqNtzSeUNPtCP76/QjWtgHbu9MHduxrOY86unO3ZQuMxJNBuBgjOBNHSyJ04fJtdRSzDfaeuzMh+RXN2OLAWf3LxTxkIfq9nWyKx3B/yFj+jHo7JNLZwhy1KsvUVfb91f1aE6ZlfoORs2RMcJ6nx+eHva8btnrKGd4J50JCCD7i39 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000049, 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, Jul 27, 2024 at 4:06=E2=80=AFPM 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 restart iterating memcg tree > from the tree root, considering an offline memcg as a failure, and abort > shrinking after encountering the same offline memcg 16 times even if > there is only one offline memcg. After this change, an offline memcg in > the tree is no longer considered a failure. This allows the shrinker to > continue shrinking the other online memcgs regardless of whether an > offline memcg exists, gives higher zswap writeback activity. > > 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 > --- > mm/zswap.c | 73 ++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 49 insertions(+), 24 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index adeaf9c97fde..e9b5343256cd 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -765,12 +765,31 @@ 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 =3D=3D memcg) > - zswap_next_shrink =3D mem_cgroup_iter(NULL, zswap_next_sh= rink, NULL); > + if (zswap_next_shrink =3D=3D memcg) { > + do { > + zswap_next_shrink =3D mem_cgroup_iter(NULL, zswap= _next_shrink, NULL); > + } while (zswap_next_shrink && !mem_cgroup_online(zswap_ne= xt_shrink)); > + /* > + * We verified the next memcg is online. Even if the nex= t > + * 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); > } > > @@ -1304,43 +1323,49 @@ static void shrink_worker(struct work_struct *w) > /* Reclaim down to the accept threshold */ > thr =3D 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 l= ock. > + * > + * 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 referenc= e > + * until the next run of shrink_worker(). > + */ > do { > spin_lock(&zswap_shrink_lock); > - zswap_next_shrink =3D mem_cgroup_iter(NULL, zswap_next_sh= rink, NULL); > - memcg =3D 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 effe= ct of the > - * zswap memcg offlining cleanup callback). This is not c= atastrophic > - * per se, but it will keep the now offlined memcg hostag= e for a while. > - * > + * Start shrinking from the next memcg after zswap_next_s= hrink. > + * When the offline cleaner has already advanced the curs= or, > + * advancing the cursor here overlooks one memcg, but thi= s > + * should be negligibly rare. > + */ > + do { > + memcg =3D mem_cgroup_iter(NULL, zswap_next_shrink= , NULL); > + zswap_next_shrink =3D memcg; > + } while (memcg && !mem_cgroup_tryget_online(memcg)); > + /* Yeah I agree with Yosry's comment - the do while loop looks like it can become a helper in some form? But that asides, the rest LGTM: Reviewed-by: Nhat Pham