| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAKEwX=PU3z7CseAZHE6v-q_yKQn0PtZqtfsfyKy5KOJpnNiE9Q@mail.gmail.com>
Date: Mon, 6 Nov 2023 12:54:43 -0800
From: Nhat Pham <nphamcs@...il.com>
To: Yosry Ahmed <yosryahmed@...gle.com>
Cc: akpm@...ux-foundation.org, hannes@...xchg.org,
cerasuolodomenico@...il.com, sjenning@...hat.com,
ddstreet@...e.org, vitaly.wool@...sulko.com, mhocko@...nel.org,
roman.gushchin@...ux.dev, shakeelb@...gle.com,
muchun.song@...ux.dev, chrisl@...nel.org, linux-mm@...ck.org,
kernel-team@...a.com, linux-kernel@...r.kernel.org,
cgroups@...r.kernel.org, linux-doc@...r.kernel.org,
linux-kselftest@...r.kernel.org, shuah@...nel.org
Subject: Re: [PATCH v5 3/6] zswap: make shrinking memcg-aware
On Mon, Nov 6, 2023 at 12:26 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
>
> On Mon, Nov 6, 2023 at 10:32 AM Nhat Pham <nphamcs@...il.com> wrote:
> >
> > From: Domenico Cerasuolo <cerasuolodomenico@...il.com>
> >
> > 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 <cerasuolodomenico@...il.com>
> > Co-developed-by: Nhat Pham <nphamcs@...il.com>
> > Signed-off-by: Nhat Pham <nphamcs@...il.com>
> > ---
> > 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_check(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 *memcg) {}
> >
> > #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 = total;
> > }
> >
> > +/* should be called under RCU */
> > +static inline struct mem_cgroup *get_mem_cgroup_from_entry(struct zswap_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 == memcg)
> > + pool->next_shrink =
> > + 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 = kmem_cache_alloc(zswap_entry_cache, gfp);
> > + entry = kmem_cache_alloc_node(zswap_entry_cache, gfp, nid);
> > if (!entry)
> > return NULL;
> > entry->refcount = 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 = get_obj_cgroup_from_folio(folio);
> > - if (objcg && !obj_cgroup_may_zswap(objcg))
> > - goto reject;
> > + if (objcg && !obj_cgroup_may_zswap(objcg)) {
> > + memcg = 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 = zswap_entry_cache_alloc(GFP_KERNEL);
> > + entry = 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 = 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 = raw_cpu_ptr(entry->pool->acomp_ctx);
> >
Powered by blists - more mailing lists