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 32DC5C3DA49 for ; Wed, 17 Jul 2024 01:54:47 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7F2386B0088; Tue, 16 Jul 2024 21:54:46 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7A2296B0096; Tue, 16 Jul 2024 21:54:46 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6432C6B0098; Tue, 16 Jul 2024 21:54:46 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 45F746B0096 for ; Tue, 16 Jul 2024 21:54:46 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id AEC5F4081A for ; Wed, 17 Jul 2024 01:54:45 +0000 (UTC) X-FDA: 82347575730.11.AF111AD Received: from mail-lf1-f49.google.com (mail-lf1-f49.google.com [209.85.167.49]) by imf13.hostedemail.com (Postfix) with ESMTP id CF51A20020 for ; Wed, 17 Jul 2024 01:54:43 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=yTUwX4Ud; spf=pass (imf13.hostedemail.com: domain of yosryahmed@google.com designates 209.85.167.49 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1721181264; 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=KaDTjYso0r0FTjuPE33B/LDoFqVnRXhQgdUXwAuM1v0=; b=hYD1R76V2VqoNpBm/WAB9bteRXNbNnT43r/f9NzHvbGSbPKkQyNcBmdlA46BffJ9ll5lnr zljiHQb7TdadEnQX0t9HbYn52nOvRlKupTInqbsvtf1cvrsQjgrcu7CmhcFgUcDwrcxAH0 Et4xeqTuWSgquX6Yx8cX9B0JgBMF9EM= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=yTUwX4Ud; spf=pass (imf13.hostedemail.com: domain of yosryahmed@google.com designates 209.85.167.49 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721181264; a=rsa-sha256; cv=none; b=DNtyysWrejiCDVCxxgMoQMoB+zrm4Srxt6eVXOchqTn4ng/j7GQRmY9YqfJ8TR/IfmxzwZ erGMFHxc7mYA5xOnTLYG1s0sNOLOR0A16UmulwcWYnirN7/uL18XXiBIIr1++vJpCx+BAx r14OxkM0QVARJ+NlXJ7fywPvOFnY5bo= Received: by mail-lf1-f49.google.com with SMTP id 2adb3069b0e04-52ea952ce70so6604517e87.3 for ; Tue, 16 Jul 2024 18:54:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1721181282; x=1721786082; 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=KaDTjYso0r0FTjuPE33B/LDoFqVnRXhQgdUXwAuM1v0=; b=yTUwX4UdKx+zPMXuksb5FggwQ2VnqvpmN8TU4WbMrT5pSz6G6Nx3g5r5DUmUc2LwVV lRx1P9he4yuXOBbt0FefH5p4vUtiF2TiDRMg1UkGUJ4eAXHdit9kXyx9HHw3BoynxrxO bjwmIsTlzCYLFKeUNfHaDRC05uLOy527biCrk8Na1pKeFiOItYXBGD2b/MuX5DfMtCRa q5joYwRLCo/g9x78wrNT5c9rTT3unSGYG7Yz6N8NFpmEucbn65C/8fWZHDbZAYs8aE6w CenmTr4J/BHdYgSuvKsxWR68mVQ/RF+hFOnmAwz+MPt8wf18IbTtdsLqZu73VI477YPb kYlQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721181282; x=1721786082; 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=KaDTjYso0r0FTjuPE33B/LDoFqVnRXhQgdUXwAuM1v0=; b=UWFhEYv2IoXsNtK1IUMcBSw8dFF85UvjOic2yr12jQIMCw08QKz9ZVOGjuUQSPnYGW 9c8U7/HxKUXMT587rp5ASkUDO+s1G94fwd4Duap940xjT0ECE+AMcIt5zz7RZgVf1V5w dRBgL8ZCnNsJCnJgZZAkbaBh2tP5ucxd1ZEc1dPofUD+1BQ2JskCtSeryhNtNzsAH5Wk ebX0BYtGu/5xWrogCi843YrgJwg6l8kPh3E4eIWq6Nj1YFKEzbhbwdzf1CiWGDhyxIJk SmhDPrBkvx3dYBdGWX8SlUXwHiocR1h+5QIEDX65vJOUEVSUmTQ2yVHoyFv3LjCo+Xrv +nAg== X-Forwarded-Encrypted: i=1; AJvYcCUUWHYn13quBgwLi8PFyoR4V1EOU2gKLseNJSs/C+sW6R0HfpF2rfdi91RBnbkQKuK1DPGAjSkeBhpN+dYzJzDW1t4= X-Gm-Message-State: AOJu0YybUCvF0ERzY5BXTV0ePNayrISufJ3ocZjgfb1hweYj4rzZc1ur PnnW6Ta15EHUSFIuZT0ABg0z5GIsfslWwMjpbDoL+OEE7MHfMDNWZANRrECEzdAuVT+oNR/7foZ 4LeEqmzHcZrgN5SgxrrCANZAjo94vJOvB6PGc X-Google-Smtp-Source: AGHT+IGDigwv1plpX5owBug/I1nQwxp5ZVs5gvFB3KlseUnQAYTDpst/OZ3qmz+IfZxxyfpPX0NdzIL7KR7XE+E49LI= X-Received: by 2002:a05:6512:3089:b0:52e:be0d:56dd with SMTP id 2adb3069b0e04-52ee542d8e6mr130655e87.40.1721181281405; Tue, 16 Jul 2024 18:54:41 -0700 (PDT) MIME-Version: 1.0 References: <20240706022523.1104080-1-flintglass@gmail.com> <20240706022523.1104080-2-flintglass@gmail.com> In-Reply-To: <20240706022523.1104080-2-flintglass@gmail.com> From: Yosry Ahmed Date: Tue, 16 Jul 2024 18:54:03 -0700 Message-ID: Subject: Re: [PATCH v2 1/6] mm: zswap: fix global shrinker memcg iteration To: Takero Funaki Cc: Johannes Weiner , Nhat Pham , 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-Rspamd-Server: rspam11 X-Rspamd-Queue-Id: CF51A20020 X-Stat-Signature: 44c6ogtroghrdf7uxgqwyiyriq33gb17 X-Rspam-User: X-HE-Tag: 1721181283-983151 X-HE-Meta: U2FsdGVkX19m7AdCFMt/kWUumCkxArd0kFniBEWySONz/y8BMty/AwEpyZExlpem3yPd1hcPscLPfErJ/m373XcuhzrO5trdweJoLEgp46XdYB5foc1JjBmia1nT15dg/rjeXpU2uamOjLup+iXCzTMatcmlGacJkV8zZM+bwdlHkyRrPkBv6g1Oe1gUy3oA660FUVAn5KZ2gUDuzbCjGbPU/AJEiooVTET7HQmv4kfe7euRi3eOIwwRNRvJtS70dSCDaZ1okWmMVvh34Np7WANZy3JMk6kckqVdETxQtBXyERDy/C7M86tw5/fO8kWx9w/RLkXPRPOz59IP5zLCc9UQN+TRbAw0nvPYepG3LzG/qCyztgeaY3LOXQ/2l2buglNaRE981qOLdaHT9tnnvXXB8MplCOeJqr7S4k81WcsfMY6nHYmsUEOcmpCLFjSSHnBJKzhO7EQtqXSAyZOR6DbnOGJ9WEFiggYplzNQZ3pTFpKFvwRsUIAyd3saJfliOCtzkvkDNZ7JWxpFYxEtQ+IuTrTVmonxjkTBZBxkCIZmx5H7EVd2cYk9MhsFe9wNa3n8/ynjYcPeuiiSavOMe7a6RySxyYvtwwrt8zLUgAgX0kO9DZP2HBA3VhD5865j3AGlPOhStRxHY92FNfPow8pgehrEGLazZBTVuWvAIH7TyuK8HMM6chJyrxder9H++nce1YTSZOBQDSyZWXNAC4NbUtwgW5MAikuYrwC3xFPCuTuR69RdSXR2AfoSriL74xhsRDlBn4Om91C0dn9slKUQfSXdiL7/PCtnE+o7KiySDa0wavqYEA6e/hxlgfX0U/3R/qSgAc0/+Crje04FXT5tzCYg8JCXXvS9CW1VY96GzpywY9Gtpg8Wqr21qj+WniC2Dr1u8QfB9FKfo1ZRy8XW+brcxIo5e0fDIa+6ihJYyWVdgJlpgWwHRI2wGiPRzjzEvoeJi9NcqE1Wfxe aoO+ksth JoGtNV8mIHqkQtca4DH6okEBMKRu2ipgVkkg3tRVDfw54VoS+51CnQlBoDZfqeb5fdRsR2rbt6MyaDnN1CrrSwaUFZCw30m4V5c46KeUTvICaN7NuTLrdja2ThX2psAyv1xa4w/sJuA0SyuuiPTuvzlOr84bLabcTgDsMpBpPSoAYnFRyE+yXIaqAKT2SJGCVHpLf 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 Fri, Jul 5, 2024 at 7:25=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 stop iterating when a memcg > was being offlined and restart from the tree root. Now, it properly > handles the offlie memcg and continues shrinking with the next memcg. > > Note that, to avoid a refcount leak of offline memcg encountered during > the memcg tree walking, shrink_worker() must continue iterating to find > the next online memcg. Please do not use the word "leak" here. It is confusing. The refcount is not leaked, we just have a long-standing ref that should eventually be dropped (although in theory, shrink_worker() may never me called again). Leak makes it sound like we increment the refcount but completely forget dropping it, which is not the case here. > > The following minor issues in the existing code are also resolved by the > change in the iteration logic: > > - A rare temporary refcount leak in the offline memcg cleaner, where the > next memcg of the offlined memcg is also offline. The leaked memcg > cannot be freed until the next shrink_worker() releases the reference. > > - One memcg was skipped from shrinking when the offline memcg cleaner > advanced the cursor of memcg tree. It is addressed by a flag to > indicate that the cursor has already been advanced. > > Fixes: a65b0e7607cc ("zswap: make shrinking memcg-aware") > Signed-off-by: Takero Funaki > --- > mm/zswap.c | 94 ++++++++++++++++++++++++++++++++++++++++++------------ > 1 file changed, 73 insertions(+), 21 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index a50e2986cd2f..29944d8145af 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -171,6 +171,7 @@ static struct list_lru zswap_list_lru; > /* The lock protects zswap_next_shrink updates. */ > static DEFINE_SPINLOCK(zswap_shrink_lock); > static struct mem_cgroup *zswap_next_shrink; > +static bool zswap_next_shrink_changed; > static struct work_struct zswap_shrink_work; > static struct shrinker *zswap_shrinker; > > @@ -775,12 +776,39 @@ 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) { > + /* > + * We advances the cursor to put back the offlined memcg. > + * shrink_worker() should not advance the cursor again. > + */ > + zswap_next_shrink_changed =3D true; I think this should be rare enough that it's not worth the extra complexity= imo. > + > + do { > + zswap_next_shrink =3D 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 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); > } > > @@ -1319,18 +1347,42 @@ 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 the cleaner ha= s we cannot determine if* the cleaner > + * 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. > + * To not skip a memcg, do not advance the cursor when it= has > + * already been advanced by the offline cleaner. > + */ > + do { > + if (zswap_next_shrink_changed) { > + /* cleaner advanced the cursor */ > + zswap_next_shrink_changed =3D false; > + } else { > + zswap_next_shrink =3D mem_cgroup_iter(NUL= L, > + zswap_next_shrink, NULL); > + } > + memcg =3D 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 m= em_cgroup_iter > * is dropped by the zswap memcg offlining callback, ensu= ring that the > @@ -1344,17 +1396,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 =3D NULL; > - spin_unlock(&zswap_shrink_lock); > - > - if (++failures =3D=3D MAX_RECLAIM_RETRIES) > - break; > - > - goto resched; > - } > + /* > + * We verified the memcg is online and got an extra memcg > + * reference. Our memcg might be offlined concurrently b= ut the > + * respective offline cleaner must be waiting for our loc= k. > + */ > spin_unlock(&zswap_shrink_lock); > > ret =3D shrink_memcg(memcg); > @@ -1368,6 +1414,12 @@ static void shrink_worker(struct work_struct *w) > resched: > cond_resched(); > } while (zswap_total_pages() > thr); > + > + /* > + * We can still hold the original memcg reference. > + * The reference is stored in zswap_next_shrink, and then reused > + * by the next shrink_worker(). > + */ This is unnecessary imo. > } > > /********************************* > -- > 2.43.0 >