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 30B51C4167D for ; Mon, 6 Nov 2023 20:54:58 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id BB20E8D0029; Mon, 6 Nov 2023 15:54:57 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B3ADB8D0001; Mon, 6 Nov 2023 15:54:57 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9B46C8D0029; Mon, 6 Nov 2023 15:54:57 -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 863DF8D0001 for ; Mon, 6 Nov 2023 15:54:57 -0500 (EST) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 4C206C08F6 for ; Mon, 6 Nov 2023 20:54:57 +0000 (UTC) X-FDA: 81428733834.05.2439ECB Received: from mail-il1-f176.google.com (mail-il1-f176.google.com [209.85.166.176]) by imf18.hostedemail.com (Postfix) with ESMTP id 774051C001F for ; Mon, 6 Nov 2023 20:54:55 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=eF9ATh4C; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf18.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.166.176 as permitted sender) smtp.mailfrom=nphamcs@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1699304095; a=rsa-sha256; cv=none; b=MIc+wwdx+zudxUao/F6kDf6sfptgPaZQP9NEJNNe2pxzjQYD+ATViScA3moG19/MnLD3px JRjFsVou6LDrnok5//Pc6IN6onCmZ+yyTFEMBLdr4Rzcx3nNCwLHBfTiSThoNOonrFGne/ jQFQsxiD8KbCIpj/VY17Py8TZRfYwXE= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=eF9ATh4C; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf18.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.166.176 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=1699304095; 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=Jx+XsNfbDvildu3tlQOtshcOutyN9/FTZV/cIEDMrB0=; b=q4cGnmDrQfx77NFu3sqjKykTjlycZIaB1nvOLHFsFnzyjAdOlC9AOx4GCcadzh6x0jGVG1 KYtddBJ0ZOnoAnMFe1PcCk1/sMiQHy5oDpfY4u6R3QymwacyeUuJ6JuP3P4Y+RCr/P9HAX 7Cv4bLG3QkLoTlckXxDqg6GduluvLOs= Received: by mail-il1-f176.google.com with SMTP id e9e14a558f8ab-35809893291so19554755ab.1 for ; Mon, 06 Nov 2023 12:54:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1699304094; x=1699908894; 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=Jx+XsNfbDvildu3tlQOtshcOutyN9/FTZV/cIEDMrB0=; b=eF9ATh4C10uXAyd2tsAdjbeey0d+m5KtJak9YnI/LaXfNQOofs2yqllLPnOxRgk2tE G6kR+S6uoEBKhfgoKpv5JijrSduHc0n1bfkDXxJxyZQlVNuW8S+8+8Cb0afO8b5MFkRH vBWJRkzy9Q6s0xg2nm+xd0b17wqQaRGyjQ5X43bxqzyfEytsM/RAR1LYSPfIAi46jm0c jCceYXz34x7HSXMmmXUPlhGaCJArnr24jsEjOKpEtm99Csb+qflmbW7kMBKtjst9Gkxj oDIqYEfvwZPR+JXYIMkxC8lHUwH9nwVO9Q8ZX1Oaojs+Y7ekU8XaTFLCFzoaozOIUsWq 6/zA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699304094; x=1699908894; 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=Jx+XsNfbDvildu3tlQOtshcOutyN9/FTZV/cIEDMrB0=; b=ZqESPmGIFPQkYsUxFccevTBJ8zWtlBCPr55zGTPGHTR/LmI1hME+Y6YF+w8Tda6LwV kOYDScnJL8qXqTuAQlLenDGE27l72M+6TWaKw4t50PQFHn/t2Mp3t9RmzWoAuNzRaujo P4DirdQOs8MsEcyzmDLoqeT2tSuMRptdVX6m1CdMIm6S8vQzSARhKLXpkFD/SY/5VyVT 2XtADFHe0JjbUe11Oyf0HRkVkOV7lMPwNiLMjyoAbTtVncBaorND6HFjB7+y1MXGAQ63 jMM5TBZRURq0uqmjtbhPuRpKZm3wuXo76rbPYrBPbo70UYDsVbOQ0ys6af6vHVZpqxmU gAEQ== X-Gm-Message-State: AOJu0YxMXFitS8A7Sn2PCKJ+TcOh3EXw9NFRIZkwCYAQ/kD+y+rn7FMf u4nkxrUfqfwwh2ZOnFssIe1MLfqiBrqxsMyzkUk= X-Google-Smtp-Source: AGHT+IEt1hDAAg6F23R7Tytq9GfojxV53jHCLY6KgmORMAUO1yr0EbCFtXJNTBdqSeSC90HrzCBWncwJc9awHvdnIbI= X-Received: by 2002:a05:6e02:1a84:b0:357:a049:91c4 with SMTP id k4-20020a056e021a8400b00357a04991c4mr944340ilv.22.1699304094438; Mon, 06 Nov 2023 12:54:54 -0800 (PST) MIME-Version: 1.0 References: <20231106183159.3562879-1-nphamcs@gmail.com> <20231106183159.3562879-4-nphamcs@gmail.com> In-Reply-To: From: Nhat Pham Date: Mon, 6 Nov 2023 12:54:43 -0800 Message-ID: Subject: Re: [PATCH v5 3/6] zswap: make shrinking memcg-aware To: Yosry Ahmed Cc: akpm@linux-foundation.org, hannes@cmpxchg.org, cerasuolodomenico@gmail.com, sjenning@redhat.com, ddstreet@ieee.org, vitaly.wool@konsulko.com, mhocko@kernel.org, roman.gushchin@linux.dev, shakeelb@google.com, muchun.song@linux.dev, chrisl@kernel.org, linux-mm@kvack.org, kernel-team@meta.com, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org, shuah@kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 774051C001F X-Stat-Signature: wgso7535p8c9iutqgiunecd5hfs79q4k X-HE-Tag: 1699304095-308080 X-HE-Meta: U2FsdGVkX196KV7TcS8AzAiRobYPjyS5Hn2RPrvU8L8S2fMFyWyX4+z+966Fl1cneHZSLtlSBploSSocn9B3bbbIsRtrHysosB2sm4IZu++dYtT7w3R6VNIVXec1qo45GjYOffo9AvZmZxMROpZrijdTCgi6406cNC0irXFCk1HynudhiAEbsVH7H2YIvg5MZF39nwIlbNKIAiuCXtrCktojhLeczcG2r8gSYuQtE73Lw6mkEDs5rSn90aDwVf5Ay9TZjvFBtQGOif3raAyWLyTgo/N3GbpGu2yti53lyndTAK8xHLTIn+IKbk+FN1XtxLljaElMY4Mbp8ZThV6E9Y1MZ2f+yGffVtFMqtBAWxAhGjsmlpRQZb1NEWFrZKqeEVH2U9gXzgBGFNkAs5USQgd810CL26q+2SYGBsf40DG6kKRdb9v24Gwbagp99I6AdKsXJY+qNVyF+fu1KnMM4YdKzHnTKtXnjfeeQdBSaaf0Vrw+S0FgIRxSw6mkOhiscXQLJmZg80xaqKymVdBHfem1DWTAzdM7fDYJgPx5YlTMdaQPKBh3YlIYLH8qJitnMVxxxltHiy12CiO8XrRh6dzTvQ4PXPC6fpDPOZ8K3X4qgxXBL9V2vRR9V036qHxYbPGcgloS/IkGj3nGKGq2S94mC5a7FwOEMr1t0OZKBOAXn6cB6MlQ0umKLbp2qd4AZhp2Cq6BQjzvND29morTrBISKGyYPB/QUVxERPw2XK1cMahQqaiRJ9kgEjh4+o5HaLynjkNRaEpxGpQg0zh2nS4JNnJJvarwJozOcnkUMf8kRxz7ws4RxWPFRfn0RvFUPFzEt+tOIPzapXDa+BGvlKN8xkZ5u5MM/VJrshrjybCcVgt9EIqMWJP/2fX2hrELboDPXKxjigVk/KfrmamRbazJIf1nJ7412M+5z80SlS3sZUto360rwdss4g9aUBQOPEgten+Ir/jtB7R8Za/ QJNIkMZy 6Lh0peXVIYJoH3E0P7bEYPWwcuCttwjaq41HrjH4v6kJhHKKlJYVWFzqvYUkOZ+Js4m/WhBL370+I9cXOv5DXt/g9kjU2/hoU9JNuHMzhl/uSLP1F/YXIdbj8R4wcDBsIfQUZbPe2bnPsPKQIBEe5xDGyCNRfWhDTY72yWZx1ipCYvJowSCDDxpWTG0n7DltYYiDI7Nheo6Gx5kfzJIH/I83izy1ibcOATYJ2bHzGN8/BHbRlJGlgSnTSD8qKNPJCcnrogzn+UFn7jkTWVp7zsmp9pNOpb7IrTf2n6zu8Ot9+cnfS1mn/BWnm6TR6SN+pKmUDQqRppeeiqin7AHY3Ur4z/kOF3bwz404ngRrw87AJ+9GsvvL2ClyfKqAp8RkGpxK579nJorp37/ry6QzLoPOgqqan03ETpuIKMfZvnwoEShAKLjZX8xPl0CFUdxirGi3bQqWoJwFpsPA= 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 Mon, Nov 6, 2023 at 12:26=E2=80=AFPM Yosry Ahmed = wrote: > > On Mon, Nov 6, 2023 at 10:32=E2=80=AFAM Nhat Pham wro= te: > > > > From: Domenico Cerasuolo > > > > Currently, we only have a single global LRU for zswap. This makes it > > impossible to perform worload-specific shrinking - an memcg cannot > > determine which pages in the pool it owns, and often ends up writing > > pages from other memcgs. This issue has been previously observed in > > practice and mitigated by simply disabling memcg-initiated shrinking: > > > > https://lore.kernel.org/all/20230530232435.3097106-1-nphamcs@gmail.com/= T/#u > > > > This patch fully resolves the issue by replacing the global zswap LRU > > with memcg- and NUMA-specific LRUs, and modify the reclaim logic: > > > > a) When a store attempt hits an memcg limit, it now triggers a > > synchronous reclaim attempt that, if successful, allows the new > > hotter page to be accepted by zswap. > > b) If the store attempt instead hits the global zswap limit, it will > > trigger an asynchronous reclaim attempt, in which an memcg is > > selected for reclaim in a round-robin-like fashion. > > > > Signed-off-by: Domenico Cerasuolo > > Co-developed-by: Nhat Pham > > Signed-off-by: Nhat Pham > > --- > > include/linux/memcontrol.h | 5 + > > include/linux/zswap.h | 2 + > > mm/memcontrol.c | 2 + > > mm/swap.h | 3 +- > > mm/swap_state.c | 24 +++- > > mm/zswap.c | 252 +++++++++++++++++++++++++++++-------- > > 6 files changed, 227 insertions(+), 61 deletions(-) > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > index 55c85f952afd..95f6c9e60ed1 100644 > > --- a/include/linux/memcontrol.h > > +++ b/include/linux/memcontrol.h > > @@ -1187,6 +1187,11 @@ static inline struct mem_cgroup *page_memcg_chec= k(struct page *page) > > return NULL; > > } > > > > +static inline struct mem_cgroup *get_mem_cgroup_from_objcg(struct obj_= cgroup *objcg) > > +{ > > + return NULL; > > +} > > + > > static inline bool folio_memcg_kmem(struct folio *folio) > > { > > return false; > > diff --git a/include/linux/zswap.h b/include/linux/zswap.h > > index 2a60ce39cfde..e571e393669b 100644 > > --- a/include/linux/zswap.h > > +++ b/include/linux/zswap.h > > @@ -15,6 +15,7 @@ bool zswap_load(struct folio *folio); > > void zswap_invalidate(int type, pgoff_t offset); > > void zswap_swapon(int type); > > void zswap_swapoff(int type); > > +void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg); > > > > #else > > > > @@ -31,6 +32,7 @@ static inline bool zswap_load(struct folio *folio) > > static inline void zswap_invalidate(int type, pgoff_t offset) {} > > static inline void zswap_swapon(int type) {} > > static inline void zswap_swapoff(int type) {} > > +static inline void zswap_memcg_offline_cleanup(struct mem_cgroup *memc= g) {} > > > > #endif > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 6f7fc0101252..2ef49b471a16 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -5640,6 +5640,8 @@ static void mem_cgroup_css_offline(struct cgroup_= subsys_state *css) > > page_counter_set_min(&memcg->memory, 0); > > page_counter_set_low(&memcg->memory, 0); > > > > + zswap_memcg_offline_cleanup(memcg); > > I think the "_cleanup" suffix is unnecessary. I guess most calls made > here are cleanup calls anyway. I don't have any strong preference here. > > > + > > memcg_offline_kmem(memcg); > > reparent_shrinker_deferred(memcg); > > wb_memcg_offline(memcg); > > diff --git a/mm/swap.h b/mm/swap.h > > index 73c332ee4d91..c0dc73e10e91 100644 > > --- a/mm/swap.h > > +++ b/mm/swap.h > > > @@ -289,15 +291,42 @@ static void zswap_update_total_size(void) > > zswap_pool_total_size =3D total; > > } > > > > +/* should be called under RCU */ > > +static inline struct mem_cgroup *get_mem_cgroup_from_entry(struct zswa= p_entry *entry) > > Do not use "get" in the name if we are not actually taking a ref here. > mem_cgroup_from_entry()? That works for me. > > > +{ > > + return entry->objcg ? obj_cgroup_memcg(entry->objcg) : NULL; > > +} > > + > > +static inline int entry_to_nid(struct zswap_entry *entry) > > +{ > > + return page_to_nid(virt_to_page(entry)); > > +} > > + > > +void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg) > > +{ > > + struct zswap_pool *pool; > > + > > + /* lock out zswap pools list modification */ > > + spin_lock(&zswap_pools_lock); > > + list_for_each_entry(pool, &zswap_pools, list) { > > + spin_lock(&pool->next_shrink_lock); > > This lock is only needed to synchronize updating pool->next_shrink, > right? Can we just use atomic operations instead? (e.g. cmpxchg()). I'm not entirely sure. I think in the pool destroy path, we have to also put the next_shrink memcg, so there's that. > > > + if (pool->next_shrink =3D=3D memcg) > > + pool->next_shrink =3D > > + mem_cgroup_iter(NULL, pool->next_shrink= , NULL, true); > > + spin_unlock(&pool->next_shrink_lock); > > + } > > + spin_unlock(&zswap_pools_lock); > > +} > > + > > /********************************* > > * zswap entry functions > > **********************************/ > > static struct kmem_cache *zswap_entry_cache; > > > > -static struct zswap_entry *zswap_entry_cache_alloc(gfp_t gfp) > > +static struct zswap_entry *zswap_entry_cache_alloc(gfp_t gfp, int nid) > > { > > struct zswap_entry *entry; > > - entry =3D kmem_cache_alloc(zswap_entry_cache, gfp); > > + entry =3D kmem_cache_alloc_node(zswap_entry_cache, gfp, nid); > > if (!entry) > > return NULL; > > entry->refcount =3D 1; > [..] > > @@ -1233,15 +1369,15 @@ bool zswap_store(struct folio *folio) > > zswap_invalidate_entry(tree, dupentry); > > } > > spin_unlock(&tree->lock); > > - > > - /* > > - * XXX: zswap reclaim does not work with cgroups yet. Without a > > - * cgroup-aware entry LRU, we will push out entries system-wide= based on > > - * local cgroup limits. > > - */ > > objcg =3D get_obj_cgroup_from_folio(folio); > > - if (objcg && !obj_cgroup_may_zswap(objcg)) > > - goto reject; > > + if (objcg && !obj_cgroup_may_zswap(objcg)) { > > + memcg =3D get_mem_cgroup_from_objcg(objcg); > > + if (shrink_memcg(memcg)) { > > + mem_cgroup_put(memcg); > > + goto reject; > > + } > > + mem_cgroup_put(memcg); > > Can we just use RCU here as well? (same around memcg_list_lru_alloc() > call below). For memcg_list_lru_alloc(): there's potentially sleeping in that piece of code I believe? I believe at the very least we'll have to use this gfp_t flag for it to be rcu-safe: GFP_KERNEL | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN not sure the Same go for this particular place IIRC - there's some sleeping done in zswap_writeback_entry(), correct? > > > + } > > > > /* reclaim space if needed */ > > if (zswap_is_full()) { > > @@ -1258,7 +1394,7 @@ bool zswap_store(struct folio *folio) > > } > > > > /* allocate entry */ > > - entry =3D zswap_entry_cache_alloc(GFP_KERNEL); > > + entry =3D zswap_entry_cache_alloc(GFP_KERNEL, page_to_nid(page)= ); > > if (!entry) { > > zswap_reject_kmemcache_fail++; > > goto reject; > > @@ -1285,6 +1421,15 @@ bool zswap_store(struct folio *folio) > > if (!entry->pool) > > goto freepage; > > > > + if (objcg) { > > + memcg =3D get_mem_cgroup_from_objcg(objcg); > > + if (memcg_list_lru_alloc(memcg, &entry->pool->list_lru,= GFP_KERNEL)) { > > + mem_cgroup_put(memcg); > > + goto put_pool; > > + } > > + mem_cgroup_put(memcg); > > + } > > + > > /* compress */ > > acomp_ctx =3D raw_cpu_ptr(entry->pool->acomp_ctx); > >