On Mon, Nov 6, 2023 at 10:32 AM Nhat Pham <nphamcs@xxxxxxxxx> wrote: > > From: Domenico Cerasuolo <cerasuolodomenico@xxxxxxxxx> > > 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@xxxxxxxxx/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@xxxxxxxxx> > Co-developed-by: Nhat Pham <nphamcs@xxxxxxxxx> > Signed-off-by: Nhat Pham <nphamcs@xxxxxxxxx> > --- > 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. > + > 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()? > +{ > + 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()). > + 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). > + } > > /* 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); >
