Skip to contentSkip to conversation

kmemcg shrinkers

PATCH In review Vladimir Davydov 3 participants 22 messages Feb 5, 2014 – Feb 14, 2014

Subsystems (9)
lore
Share & tools
lore
#0 lore raw

v15 cover kmemcg shrinkers

Hi,

This is the 15th iteration of Glauber Costa's patch-set implementing slab shrinking on memcg pressure. The main idea is to make the list_lru structure used by most FS shrinkers per-memcg. When adding or removing an element from a list_lru, we use the page information to figure out which memcg it belongs to and relay it to the appropriate list. This allows scanning kmem objects accounted to different memcgs independently.

Please note that this patch-set implements slab shrinking only when we hit the user memory limit so that kmem allocations will still fail if we are below the user memory limit, but close to the kmem limit. I am going to fix this in a separate patch-set, but currently it is only worthwhile setting the kmem limit to be greater than the user mem limit just to enable per-memcg slab accounting and reclaim.

The patch-set is based on top of v3.14-rc1-mmots-2014-02-04-16-48 (there are some vmscan cleanups that I need committed there) and organized as follows:
- patches 1-4 introduce some minor changes to memcg needed for this set;
- patches 5-7 prepare fs for per-memcg list_lru;
- patch 8 implement kmemcg reclaim core;
- patch 9 make list_lru per-memcg and patch 10 marks sb shrinker memcg-aware;
- patch 10 is trivial - it issues shrinkers on memcg destruction;
- patches 12 and 13 introduce shrinking of dead kmem caches to facilitate memcg destruction.

Changes in v15:
- remove patches that have been merged to -mm;
- fix memory barrier usage in per-memcg list_lru implementation;
- fix list_lru_destroy(), which might sleep for per-memcg lrus, called from atomic context (__put_super()).

Previous iterations of this patch-set can be found here:
- https://lkml.org/lkml/2013/12/16/206 (v14)
- https://lkml.org/lkml/2013/12/9/103 (v13)
- https://lkml.org/lkml/2013/12/2/141 (v12)
- https://lkml.org/lkml/2013/11/25/214 (v11)

Comments are highly appreciated.

Thanks.

Glauber Costa (6):
memcg: make cache index determination more robust
memcg: consolidate callers of memcg_cache_id
memcg: move initialization to memcg creation
memcg: flush memcg items upon memcg destruction
vmpressure: in-kernel notifications
memcg: reap dead memcgs upon global memory pressure

Vladimir Davydov (7):
memcg: make for_each_mem_cgroup macros public list_lru, shrinkers: introduce list_lru_shrink_{count,walk}
fs: consolidate {nr,free}_cached_objects args in shrink_control
fs: do not call destroy_super() in atomic context
vmscan: shrink slab on memcg pressure
list_lru: add per-memcg lists
fs: make shrinker memcg aware

 fs/dcache.c                |   14 +-
 fs/gfs2/quota.c            |    6 +-
 fs/inode.c                 |    7 +-
 fs/internal.h              |    7 +-
 fs/super.c                 |   44 ++---
 fs/xfs/xfs_buf.c           |    7 +-
 fs/xfs/xfs_qm.c            |    7 +-
 fs/xfs/xfs_super.c         |    7 +-
 include/linux/fs.h         |    8 +-
 include/linux/list_lru.h   |  112 ++++++++++---
 include/linux/memcontrol.h |   50 ++++++
 include/linux/shrinker.h   |   10 +-
 include/linux/vmpressure.h |    5 +
 mm/list_lru.c              |  271 +++++++++++++++++++++++++++---
 mm/memcontrol.c            |  399 ++++++++++++++++++++++++++++++++++++++------
 mm/vmpressure.c            |   53 +++++-
 mm/vmscan.c                |   94 ++++++++---
 17 files changed, 926 insertions(+), 175 deletions(-)
Signature · 7 lines
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#1 lore raw

PATCH v15 1/13 memcg: make cache index determination more robust

From: Glauber Costa <[email protected]>

I caught myself doing something like the following outside memcg core:

	memcg_id = -1;
	if (memcg && memcg_kmem_is_active(memcg))
		memcg_id = memcg_cache_id(memcg);

to be able to handle all possible memcgs in a sane manner. In particular, the root cache will have kmemcg_id = -1 (just because we don't call memcg_kmem_init to the root cache since it is not limitable). We have always coped with that by making sure we sanitize which cache is passed to memcg_cache_id. Although this example is given for root, what we really need to know is whether or not a cache is kmem active.

But outside the memcg core testing for root, for instance, is not trivial since we don't export mem_cgroup_is_root. I ended up realizing that this tests really belong inside memcg_cache_id. This patch moves a similar but stronger test inside memcg_cache_id and make sure it always return a meaningful value.

Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Balbir Singh <[email protected]>
Cc KAMEZAWA Hiroyuki <[email protected]>
---
 mm/memcontrol.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
mm/memcontrol.c +3 -1
index 53385cd4e6f0..75758fc5c50c 100644
@@ -3110,7 +3110,9 @@ static void memcg_uncharge_kmem(struct mem_cgroup *memcg, u64 size)
  */
 int memcg_cache_id(struct mem_cgroup *memcg)
 {
-	return memcg ? memcg->kmemcg_id : -1;
+	if (!memcg || !memcg_can_account_kmem(memcg))
+		return -1;
+	return memcg->kmemcg_id;
 }
 
 static size_t memcg_caches_array_size(int num_groups)
Signature · 7 lines
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#2 lore raw

PATCH v15 2/13 memcg: consolidate callers of memcg_cache_id

In reply to Vladimir Davydov

From: Glauber Costa <[email protected]>

Each caller of memcg_cache_id ends up sanitizing its parameters in its own way. Now that the memcg_cache_id itself is more robust, we can consolidate this.

Also, as suggested by Michal, a special helper memcg_cache_idx is used when the result is expected to be used directly as an array index to make sure we never access in a negative index.

Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Balbir Singh <[email protected]>
Cc KAMEZAWA Hiroyuki <[email protected]>
---
 mm/memcontrol.c |   48 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 17 deletions(-)
mm/memcontrol.c +31 -17
index 75758fc5c50c..9d1245dc993a 100644
@@ -3008,6 +3008,30 @@ static inline bool memcg_can_account_kmem(struct mem_cgroup *memcg)
 }
 
 /*
+ * helper for acessing a memcg's index. It will be used as an index in the
+ * child cache array in kmem_cache, and also to derive its name. This function
+ * will return -1 when this is not a kmem-limited memcg.
+ */
+int memcg_cache_id(struct mem_cgroup *memcg)
+{
+	if (!memcg || !memcg_can_account_kmem(memcg))
+		return -1;
+	return memcg->kmemcg_id;
+}
+
+/*
+ * This helper around memcg_cache_id is not intented for use outside memcg
+ * core. It is meant for places where the cache id is used directly as an array
+ * index
+ */
+static int memcg_cache_idx(struct mem_cgroup *memcg)
+{
+	int ret = memcg_cache_id(memcg);
+	BUG_ON(ret < 0);
+	return ret;
+}
+
+/*
  * This is a bit cumbersome, but it is rarely used and avoids a backpointer
  * in the memcg_cache_params struct.
  */
@@ -3017,7 +3041,7 @@ static struct kmem_cache *memcg_params_to_cache(struct memcg_cache_params *p)
 
 	VM_BUG_ON(p->is_root_cache);
 	cachep = p->root_cache;
-	return cache_from_memcg_idx(cachep, memcg_cache_id(p->memcg));
+	return cache_from_memcg_idx(cachep, memcg_cache_idx(p->memcg));
 }
 
 #ifdef CONFIG_SLABINFO
@@ -3103,18 +3127,6 @@ static void memcg_uncharge_kmem(struct mem_cgroup *memcg, u64 size)
 		css_put(&memcg->css);
 }
 
-/*
- * helper for acessing a memcg's index. It will be used as an index in the
- * child cache array in kmem_cache, and also to derive its name. This function
- * will return -1 when this is not a kmem-limited memcg.
- */
-int memcg_cache_id(struct mem_cgroup *memcg)
-{
-	if (!memcg || !memcg_can_account_kmem(memcg))
-		return -1;
-	return memcg->kmemcg_id;
-}
-
 static size_t memcg_caches_array_size(int num_groups)
 {
 	ssize_t size;
@@ -3246,7 +3258,7 @@ void memcg_register_cache(struct kmem_cache *s)
 
 	root = s->memcg_params->root_cache;
 	memcg = s->memcg_params->memcg;
-	id = memcg_cache_id(memcg);
+	id = memcg_cache_idx(memcg);
 
 	css_get(&memcg->css);
 
@@ -3288,7 +3300,7 @@ void memcg_unregister_cache(struct kmem_cache *s)
 
 	root = s->memcg_params->root_cache;
 	memcg = s->memcg_params->memcg;
-	id = memcg_cache_id(memcg);
+	id = memcg_cache_idx(memcg);
 
 	mutex_lock(&memcg->slab_caches_mutex);
 	list_del(&s->memcg_params->list);
@@ -3573,6 +3585,7 @@ struct kmem_cache *__memcg_kmem_get_cache(struct kmem_cache *cachep,
 {
 	struct mem_cgroup *memcg;
 	struct kmem_cache *memcg_cachep;
+	int id;
 
 	VM_BUG_ON(!cachep->memcg_params);
 	VM_BUG_ON(!cachep->memcg_params->is_root_cache);
@@ -3583,10 +3596,11 @@ struct kmem_cache *__memcg_kmem_get_cache(struct kmem_cache *cachep,
 	rcu_read_lock();
 	memcg = mem_cgroup_from_task(rcu_dereference(current->mm->owner));
 
-	if (!memcg_can_account_kmem(memcg))
+	id = memcg_cache_id(memcg);
+	if (id < 0)
 		goto out;
 
-	memcg_cachep = cache_from_memcg_idx(cachep, memcg_cache_id(memcg));
+	memcg_cachep = cache_from_memcg_idx(cachep, id);
 	if (likely(memcg_cachep)) {
 		cachep = memcg_cachep;
 		goto out;
Signature · 7 lines
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#3 lore raw

PATCH v15 3/13 memcg: move initialization to memcg creation

In reply to Vladimir Davydov

From: Glauber Costa <[email protected]>

Those structures are only used for memcgs that are effectively using kmemcg. However, in a later patch I intend to use scan that list inconditionally (list empty meaning no kmem caches present), which simplifies the code a lot.

So move the initialization to early kmem creation.

Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Balbir Singh <[email protected]>
Cc KAMEZAWA Hiroyuki <[email protected]>
---
 mm/memcontrol.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
mm/memcontrol.c +3 -2
index 9d1245dc993a..deb5b9bb6188 100644
@@ -5210,8 +5210,6 @@ static int __memcg_activate_kmem(struct mem_cgroup *memcg,
 		goto out_rmid;
 
 	memcg->kmemcg_id = memcg_id;
-	INIT_LIST_HEAD(&memcg->memcg_slab_caches);
-	mutex_init(&memcg->slab_caches_mutex);
 
 	/*
 	 * We couldn't have accounted to this cgroup, because it hasn't got the
@@ -5958,6 +5956,9 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
 	int ret;
 
 	memcg->kmemcg_id = -1;
+	INIT_LIST_HEAD(&memcg->memcg_slab_caches);
+	mutex_init(&memcg->slab_caches_mutex);
+
 	ret = memcg_propagate_kmem(memcg);
 	if (ret)
 		return ret;
Signature · 7 lines
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#4 lore raw

PATCH v15 4/13 memcg: make for_each_mem_cgroup macros public

In reply to Vladimir Davydov

I am going to use these macros in next patches, so let's move them to the header. These macros are very handy and they depend only on mem_cgroup_iter(), which is already public, so I guess it's worth it.

Cc Glauber Costa <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Balbir Singh <[email protected]>
Cc KAMEZAWA Hiroyuki <[email protected]>
---
 include/linux/memcontrol.h |   15 +++++++++++++++
 mm/memcontrol.c            |   15 ---------------
 2 files changed, 15 insertions(+), 15 deletions(-)
include/linux/memcontrol.h +15 -0
index abd0113b6620..0503b59c3fad 100644
@@ -53,6 +53,21 @@ struct mem_cgroup_reclaim_cookie {
 	unsigned int generation;
 };
 
+/*
+ * Iteration constructs for visiting all cgroups (under a tree).  If
+ * loops are exited prematurely (break), mem_cgroup_iter_break() must
+ * be used for reference counting.
+ */
+#define for_each_mem_cgroup_tree(iter, root)		\
+	for (iter = mem_cgroup_iter(root, NULL, NULL);	\
+	     iter != NULL;				\
+	     iter = mem_cgroup_iter(root, iter, NULL))
+
+#define for_each_mem_cgroup(iter)			\
+	for (iter = mem_cgroup_iter(NULL, NULL, NULL);	\
+	     iter != NULL;				\
+	     iter = mem_cgroup_iter(NULL, iter, NULL))
+
 #ifdef CONFIG_MEMCG
 /*
  * All "charge" functions with gfp_mask should use GFP_KERNEL or
mm/memcontrol.c +0 -15
index deb5b9bb6188..854d0b8e3c45 100644
@@ -1297,21 +1297,6 @@ void mem_cgroup_iter_break(struct mem_cgroup *root,
 		css_put(&prev->css);
 }
 
-/*
- * Iteration constructs for visiting all cgroups (under a tree).  If
- * loops are exited prematurely (break), mem_cgroup_iter_break() must
- * be used for reference counting.
- */
-#define for_each_mem_cgroup_tree(iter, root)		\
-	for (iter = mem_cgroup_iter(root, NULL, NULL);	\
-	     iter != NULL;				\
-	     iter = mem_cgroup_iter(root, iter, NULL))
-
-#define for_each_mem_cgroup(iter)			\
-	for (iter = mem_cgroup_iter(NULL, NULL, NULL);	\
-	     iter != NULL;				\
-	     iter = mem_cgroup_iter(NULL, iter, NULL))
-
 void __mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx)
 {
 	struct mem_cgroup *memcg;
Signature · 7 lines
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#5 lore raw

PATCH v15 5/13 list_lru, shrinkers: introduce list_lru_shrink_{count,walk}

In reply to Vladimir Davydov

NUMA aware slab shrinkers use the list_lru structure to distribute objects coming from different NUMA nodes to different lists. Whenever such a shrinker needs to count or scan objects from a particular node, it issues commands like this:

        count = list_lru_count_node(lru, sc->nid);
        freed = list_lru_walk_node(lru, sc->nid, isolate_func,
                                   isolate_arg, &sc->nr_to_scan);

where sc is an instance of the shrink_control structure passed to it from vmscan.

To simplify this, let's add the special list_lru functions to be used by shrinkers, list_lru_shrink_count() and list_lru_shrink_walk(), which consolidate the nid and nr_to_scan arguments in the shrink_control structure.

This will also allow us to avoid patching shrinkers that use list_lru when we make shrink_slab() per-memcg - all we will have to do is extend the shrink_control structure to include the memcg to scan from and make list_lru_shrink_{count,walk} handle this appropriately.

Thanks to David Chinner for the tip.

Cc Glauber Costa <[email protected]>
Cc Dave Chinner <[email protected]>
Cc Al Viro <[email protected]>
---
 fs/dcache.c              |   14 ++++++--------
 fs/gfs2/quota.c          |    6 +++---
 fs/inode.c               |    7 +++----
 fs/internal.h            |    7 +++----
 fs/super.c               |   22 ++++++++++------------
 fs/xfs/xfs_buf.c         |    7 +++----
 fs/xfs/xfs_qm.c          |    7 +++----
 include/linux/list_lru.h |   16 ++++++++++++++++
 8 files changed, 47 insertions(+), 39 deletions(-)
fs/dcache.c +6 -8
index 265e0ce9769c..4bc85f96a87d 100644
@@ -972,24 +972,22 @@ dentry_lru_isolate(struct list_head *item, spinlock_t *lru_lock, void *arg)
 /**
  * prune_dcache_sb - shrink the dcache
  * @sb: superblock
- * @nr_to_scan : number of entries to try to free
- * @nid: which node to scan for freeable entities
+ * @sc: shrink control, passed to list_lru_shrink_walk()
  *
- * Attempt to shrink the superblock dcache LRU by @nr_to_scan entries. This is
- * done when we need more memory an called from the superblock shrinker
+ * Attempt to shrink the superblock dcache LRU by @sc->nr_to_scan entries. This
+ * is done when we need more memory and called from the superblock shrinker
  * function.
  *
  * This function may fail to free any resources if all the dentries are in
  * use.
  */
-long prune_dcache_sb(struct super_block *sb, unsigned long nr_to_scan,
-		     int nid)
+long prune_dcache_sb(struct super_block *sb, struct shrink_control *sc)
 {
 	LIST_HEAD(dispose);
 	long freed;
 
-	freed = list_lru_walk_node(&sb->s_dentry_lru, nid, dentry_lru_isolate,
-				       &dispose, &nr_to_scan);
+	freed = list_lru_shrink_walk(&sb->s_dentry_lru, sc,
+				     dentry_lru_isolate, &dispose);
 	shrink_dentry_list(&dispose);
 	return freed;
 }
fs/gfs2/quota.c +3 -3
index 8bec0e3192dd..8746393aed88 100644
@@ -169,8 +169,8 @@ static unsigned long gfs2_qd_shrink_scan(struct shrinker *shrink,
 	if (!(sc->gfp_mask & __GFP_FS))
 		return SHRINK_STOP;
 
-	freed = list_lru_walk_node(&gfs2_qd_lru, sc->nid, gfs2_qd_isolate,
-				   &dispose, &sc->nr_to_scan);
+	freed = list_lru_shrink_walk(&gfs2_qd_lru, sc,
+				     gfs2_qd_isolate, &dispose);
 
 	gfs2_qd_dispose(&dispose);
 
@@ -180,7 +180,7 @@ static unsigned long gfs2_qd_shrink_scan(struct shrinker *shrink,
 static unsigned long gfs2_qd_shrink_count(struct shrinker *shrink,
 					  struct shrink_control *sc)
 {
-	return vfs_pressure_ratio(list_lru_count_node(&gfs2_qd_lru, sc->nid));
+	return vfs_pressure_ratio(list_lru_shrink_count(&gfs2_qd_lru, sc));
 }
 
 struct shrinker gfs2_qd_shrinker = {
fs/inode.c +3 -4
index e6905152c39f..890e4f9b1590 100644
@@ -748,14 +748,13 @@ inode_lru_isolate(struct list_head *item, spinlock_t *lru_lock, void *arg)
  * to trim from the LRU. Inodes to be freed are moved to a temporary list and
  * then are freed outside inode_lock by dispose_list().
  */
-long prune_icache_sb(struct super_block *sb, unsigned long nr_to_scan,
-		     int nid)
+long prune_icache_sb(struct super_block *sb, struct shrink_control *sc)
 {
 	LIST_HEAD(freeable);
 	long freed;
 
-	freed = list_lru_walk_node(&sb->s_inode_lru, nid, inode_lru_isolate,
-				       &freeable, &nr_to_scan);
+	freed = list_lru_shrink_walk(&sb->s_inode_lru, sc,
+				     inode_lru_isolate, &freeable);
 	dispose_list(&freeable);
 	return freed;
 }
fs/internal.h +3 -4
index 465742407466..3db5f6e41cd7 100644
@@ -14,6 +14,7 @@ struct file_system_type;
 struct linux_binprm;
 struct path;
 struct mount;
+struct shrink_control;
 
 /*
  * block_dev.c
@@ -107,8 +108,7 @@ extern int open_check_o_direct(struct file *f);
  * inode.c
  */
 extern spinlock_t inode_sb_list_lock;
-extern long prune_icache_sb(struct super_block *sb, unsigned long nr_to_scan,
-			    int nid);
+extern long prune_icache_sb(struct super_block *sb, struct shrink_control *sc);
 extern void inode_add_lru(struct inode *inode);
 
 /*
@@ -125,8 +125,7 @@ extern int invalidate_inodes(struct super_block *, bool);
  */
 extern struct dentry *__d_alloc(struct super_block *, const struct qstr *);
 extern int d_set_mounted(struct dentry *dentry);
-extern long prune_dcache_sb(struct super_block *sb, unsigned long nr_to_scan,
-			    int nid);
+extern long prune_dcache_sb(struct super_block *sb, struct shrink_control *sc);
 
 /*
  * read_write.c
fs/super.c +10 -12
index 80d5cf2ca765..0688f3eaf012 100644
@@ -78,27 +78,27 @@ static unsigned long super_cache_scan(struct shrinker *shrink,
 	if (sb->s_op->nr_cached_objects)
 		fs_objects = sb->s_op->nr_cached_objects(sb, sc->nid);
 
-	inodes = list_lru_count_node(&sb->s_inode_lru, sc->nid);
-	dentries = list_lru_count_node(&sb->s_dentry_lru, sc->nid);
+	inodes = list_lru_shrink_count(&sb->s_inode_lru, sc);
+	dentries = list_lru_shrink_count(&sb->s_dentry_lru, sc);
 	total_objects = dentries + inodes + fs_objects + 1;
 
 	/* proportion the scan between the caches */
 	dentries = mult_frac(sc->nr_to_scan, dentries, total_objects);
 	inodes = mult_frac(sc->nr_to_scan, inodes, total_objects);
+	fs_objects = mult_frac(sc->nr_to_scan, fs_objects, total_objects);
 
 	/*
 	 * prune the dcache first as the icache is pinned by it, then
 	 * prune the icache, followed by the filesystem specific caches
 	 */
-	freed = prune_dcache_sb(sb, dentries, sc->nid);
-	freed += prune_icache_sb(sb, inodes, sc->nid);
+	sc->nr_to_scan = dentries;
+	freed = prune_dcache_sb(sb, sc);
+	sc->nr_to_scan = inodes;
+	freed += prune_icache_sb(sb, sc);
 
-	if (fs_objects) {
-		fs_objects = mult_frac(sc->nr_to_scan, fs_objects,
-								total_objects);
+	if (fs_objects)
 		freed += sb->s_op->free_cached_objects(sb, fs_objects,
 						       sc->nid);
-	}
 
 	drop_super(sb);
 	return freed;
@@ -119,10 +119,8 @@ static unsigned long super_cache_count(struct shrinker *shrink,
 		total_objects = sb->s_op->nr_cached_objects(sb,
 						 sc->nid);
 
-	total_objects += list_lru_count_node(&sb->s_dentry_lru,
-						 sc->nid);
-	total_objects += list_lru_count_node(&sb->s_inode_lru,
-						 sc->nid);
+	total_objects += list_lru_shrink_count(&sb->s_dentry_lru, sc);
+	total_objects += list_lru_shrink_count(&sb->s_inode_lru, sc);
 
 	total_objects = vfs_pressure_ratio(total_objects);
 	drop_super(sb);
fs/xfs/xfs_buf.c +3 -4
index 9c061ef2b0d9..b52ea989a2a4 100644
@@ -1554,10 +1554,9 @@ xfs_buftarg_shrink_scan(
 					struct xfs_buftarg, bt_shrinker);
 	LIST_HEAD(dispose);
 	unsigned long		freed;
-	unsigned long		nr_to_scan = sc->nr_to_scan;
 
-	freed = list_lru_walk_node(&btp->bt_lru, sc->nid, xfs_buftarg_isolate,
-				       &dispose, &nr_to_scan);
+	freed = list_lru_shrink_walk(&btp->bt_lru, sc,
+				     xfs_buftarg_isolate, &dispose);
 
 	while (!list_empty(&dispose)) {
 		struct xfs_buf *bp;
@@ -1576,7 +1575,7 @@ xfs_buftarg_shrink_count(
 {
 	struct xfs_buftarg	*btp = container_of(shrink,
 					struct xfs_buftarg, bt_shrinker);
-	return list_lru_count_node(&btp->bt_lru, sc->nid);
+	return list_lru_shrink_count(&btp->bt_lru, sc);
 }
 
 void
fs/xfs/xfs_qm.c +3 -4
index 348e4d2ed6e6..b4a33b7ab597 100644
@@ -790,7 +790,6 @@ xfs_qm_shrink_scan(
 	struct xfs_qm_isolate	isol;
 	unsigned long		freed;
 	int			error;
-	unsigned long		nr_to_scan = sc->nr_to_scan;
 
 	if ((sc->gfp_mask & (__GFP_FS|__GFP_WAIT)) != (__GFP_FS|__GFP_WAIT))
 		return 0;
@@ -798,8 +797,8 @@ xfs_qm_shrink_scan(
 	INIT_LIST_HEAD(&isol.buffers);
 	INIT_LIST_HEAD(&isol.dispose);
 
-	freed = list_lru_walk_node(&qi->qi_lru, sc->nid, xfs_qm_dquot_isolate, &isol,
-					&nr_to_scan);
+	freed = list_lru_shrink_walk(&qi->qi_lru, sc,
+				     xfs_qm_dquot_isolate, &isol);
 
 	error = xfs_buf_delwri_submit(&isol.buffers);
 	if (error)
@@ -824,7 +823,7 @@ xfs_qm_shrink_count(
 	struct xfs_quotainfo	*qi = container_of(shrink,
 					struct xfs_quotainfo, qi_shrinker);
 
-	return list_lru_count_node(&qi->qi_lru, sc->nid);
+	return list_lru_shrink_count(&qi->qi_lru, sc);
 }
 
 /*
include/linux/list_lru.h +16 -0
index b02fc233eadd..6ca43b2486fc 100644
@@ -9,6 +9,7 @@
 
 #include <linux/list.h>
 #include <linux/nodemask.h>
+#include <linux/shrinker.h>
 
 /* list_lru_walk_cb has to always return one of those */
 enum lru_status {
@@ -77,6 +78,13 @@ bool list_lru_del(struct list_lru *lru, struct list_head *item);
  * Callers that want such a guarantee need to provide an outer lock.
  */
 unsigned long list_lru_count_node(struct list_lru *lru, int nid);
+
+static inline unsigned long list_lru_shrink_count(struct list_lru *lru,
+						  struct shrink_control *sc)
+{
+	return list_lru_count_node(lru, sc->nid);
+}
+
 static inline unsigned long list_lru_count(struct list_lru *lru)
 {
 	long count = 0;
@@ -116,6 +124,14 @@ unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
 				 unsigned long *nr_to_walk);
 
 static inline unsigned long
+list_lru_shrink_walk(struct list_lru *lru, struct shrink_control *sc,
+		     list_lru_walk_cb isolate, void *cb_arg)
+{
+	return list_lru_walk_node(lru, sc->nid, isolate, cb_arg,
+				  &sc->nr_to_scan);
+}
+
+static inline unsigned long
 list_lru_walk(struct list_lru *lru, list_lru_walk_cb isolate,
 	      void *cb_arg, unsigned long nr_to_walk)
 {
Signature · 7 lines
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#6 lore raw

PATCH v15 6/13 fs: consolidate {nr,free}_cached_objects args in shrink_control

In reply to Vladimir Davydov

We are going to make the FS shrinker memcg-aware. To achieve that, we will have to pass the memcg to scan to the nr_cached_objects and free_cached_objects VFS methods, which currently take only the NUMA node to scan. Since the shrink_control structure already holds the node, and the memcg to scan will be added to it as we introduce memcg-aware vmscan, let us consolidate the methods' arguments in this structure to keep things clean.

Thanks to David Chinner for the tip.

Cc Glauber Costa <[email protected]>
Cc Dave Chinner <[email protected]>
Cc Al Viro <[email protected]>
---
 fs/super.c         |   12 ++++++------
 fs/xfs/xfs_super.c |    7 +++----
 include/linux/fs.h |    6 ++++--
 3 files changed, 13 insertions(+), 12 deletions(-)
fs/super.c +6 -6
index 0688f3eaf012..ff9ff5fad70c 100644
@@ -76,7 +76,7 @@ static unsigned long super_cache_scan(struct shrinker *shrink,
 		return SHRINK_STOP;
 
 	if (sb->s_op->nr_cached_objects)
-		fs_objects = sb->s_op->nr_cached_objects(sb, sc->nid);
+		fs_objects = sb->s_op->nr_cached_objects(sb, sc);
 
 	inodes = list_lru_shrink_count(&sb->s_inode_lru, sc);
 	dentries = list_lru_shrink_count(&sb->s_dentry_lru, sc);
@@ -96,9 +96,10 @@ static unsigned long super_cache_scan(struct shrinker *shrink,
 	sc->nr_to_scan = inodes;
 	freed += prune_icache_sb(sb, sc);
 
-	if (fs_objects)
-		freed += sb->s_op->free_cached_objects(sb, fs_objects,
-						       sc->nid);
+	if (fs_objects) {
+		sc->nr_to_scan = fs_objects;
+		freed += sb->s_op->free_cached_objects(sb, sc);
+	}
 
 	drop_super(sb);
 	return freed;
@@ -116,8 +117,7 @@ static unsigned long super_cache_count(struct shrinker *shrink,
 		return 0;
 
 	if (sb->s_op && sb->s_op->nr_cached_objects)
-		total_objects = sb->s_op->nr_cached_objects(sb,
-						 sc->nid);
+		total_objects = sb->s_op->nr_cached_objects(sb, sc);
 
 	total_objects += list_lru_shrink_count(&sb->s_dentry_lru, sc);
 	total_objects += list_lru_shrink_count(&sb->s_inode_lru, sc);
fs/xfs/xfs_super.c +3 -4
index 01ee44444885..4bc182b29c8f 100644
@@ -1524,7 +1524,7 @@ xfs_fs_mount(
 static long
 xfs_fs_nr_cached_objects(
 	struct super_block	*sb,
-	int			nid)
+	struct shrink_control	*sc)
 {
 	return xfs_reclaim_inodes_count(XFS_M(sb));
 }
@@ -1532,10 +1532,9 @@ xfs_fs_nr_cached_objects(
 static long
 xfs_fs_free_cached_objects(
 	struct super_block	*sb,
-	long			nr_to_scan,
-	int			nid)
+	struct shrink_control	*sc)
 {
-	return xfs_reclaim_inodes_nr(XFS_M(sb), nr_to_scan);
+	return xfs_reclaim_inodes_nr(XFS_M(sb), sc->nr_to_scan);
 }
 
 static const struct super_operations xfs_super_operations = {
include/linux/fs.h +4 -2
index 9b613a828b53..3a87b0254408 100644
@@ -1624,8 +1624,10 @@ struct super_operations {
 	ssize_t (*quota_write)(struct super_block *, int, const char *, size_t, loff_t);
 #endif
 	int (*bdev_try_to_free_page)(struct super_block*, struct page*, gfp_t);
-	long (*nr_cached_objects)(struct super_block *, int);
-	long (*free_cached_objects)(struct super_block *, long, int);
+	long (*nr_cached_objects)(struct super_block *,
+				  struct shrink_control *);
+	long (*free_cached_objects)(struct super_block *,
+				    struct shrink_control *);
 };
 
 /*
Signature · 7 lines
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#7 lore raw

PATCH v15 7/13 fs: do not call destroy_super() in atomic context

In reply to Vladimir Davydov

To make list_lru per-memcg, I'll have to add some code that might sleep to list_lru_destroy(), but it is impossible to do now, because list_lru_destroy() is currently called by destroy_super(), which can be called in atomic context from __put_super().

To overcome this, in this patch I make __put_super() schedule the super block destruction in an asynchronous work instead of calling destroy_super() directly.

Cc Glauber Costa <[email protected]>
Cc Dave Chinner <[email protected]>
Cc Al Viro <[email protected]>
---
 fs/super.c         |   10 +++++++++-
 include/linux/fs.h |    2 ++
 2 files changed, 11 insertions(+), 1 deletion(-)
fs/super.c +9 -1
index ff9ff5fad70c..33cbff3769e7 100644
@@ -147,6 +147,13 @@ static void destroy_super(struct super_block *s)
 	kfree_rcu(s, rcu);
 }
 
+static void destroy_super_work_func(struct work_struct *w)
+{
+	struct super_block *s = container_of(w, struct super_block, destroy);
+
+	destroy_super(s);
+}
+
 /**
  *	alloc_super	-	create new superblock
  *	@type:	filesystem type superblock should belong to
@@ -182,6 +189,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags)
 	INIT_HLIST_NODE(&s->s_instances);
 	INIT_HLIST_BL_HEAD(&s->s_anon);
 	INIT_LIST_HEAD(&s->s_inodes);
+	INIT_WORK(&s->destroy, destroy_super_work_func);
 
 	if (list_lru_init(&s->s_dentry_lru))
 		goto fail;
@@ -239,7 +247,7 @@ static void __put_super(struct super_block *sb)
 {
 	if (!--sb->s_count) {
 		list_del_init(&sb->s_list);
-		destroy_super(sb);
+		schedule_work(&sb->destroy);
 	}
 }
 
include/linux/fs.h +2 -0
index 3a87b0254408..200cbf804335 100644
@@ -1328,7 +1328,9 @@ struct super_block {
 	 */
 	struct list_lru		s_dentry_lru ____cacheline_aligned_in_smp;
 	struct list_lru		s_inode_lru ____cacheline_aligned_in_smp;
+
 	struct rcu_head		rcu;
+	struct work_struct	destroy;
 };
 
 extern struct timespec current_fs_time(struct super_block *sb);
Signature · 7 lines
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#8 lore raw

PATCH v15 8/13 vmscan: shrink slab on memcg pressure

In reply to Vladimir Davydov

This patch makes direct reclaim path shrink slab not only on global memory pressure, but also when we reach the user memory limit of a memcg. To achieve that, it makes shrink_slab() walk over the memcg hierarchy and run shrinkers marked as memcg-aware on the target memcg and all its descendants. The memcg to scan is passed in a shrink_control structure; memcg-unaware shrinkers are still called only on global memory pressure with memcg=NULL. It is up to the shrinker how to organize the objects it is responsible for to achieve per-memcg reclaim.

Note that we do not intend to have true per memcg per node reclaim. Since most memcgs are small and typically confined to a single NUMA node or two by external means and therefore do not need the scalability NUMA aware shrinkers provide, we actually call per node shrinking only for the global list (memcg=NULL), while per-memcg lists are always scanned only once irrespective of the nodemask with nid=0.

The idea lying behind the patch as well as the initial implementation belong to Glauber Costa.

Cc Glauber Costa <[email protected]>
Cc Dave Chinner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Al Viro <[email protected]>
Cc Balbir Singh <[email protected]>
Cc KAMEZAWA Hiroyuki <[email protected]>
---
 include/linux/memcontrol.h |   22 +++++++++++
 include/linux/shrinker.h   |   10 ++++-
 mm/memcontrol.c            |   37 ++++++++++++++++-
 mm/vmscan.c                |   94 ++++++++++++++++++++++++++++++++++----------
 4 files changed, 141 insertions(+), 22 deletions(-)
include/linux/memcontrol.h +22 -0
index 0503b59c3fad..fc4a24d31e99 100644
@@ -95,6 +95,9 @@ extern int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
 struct lruvec *mem_cgroup_zone_lruvec(struct zone *, struct mem_cgroup *);
 struct lruvec *mem_cgroup_page_lruvec(struct page *, struct zone *);
 
+unsigned long mem_cgroup_zone_reclaimable_pages(struct zone *,
+						struct mem_cgroup *);
+
 /* For coalescing uncharge for reducing memcg' overhead*/
 extern void mem_cgroup_uncharge_start(void);
 extern void mem_cgroup_uncharge_end(void);
@@ -304,6 +307,12 @@ static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page,
 	return &zone->lruvec;
 }
 
+static inline unsigned long mem_cgroup_zone_reclaimable_pages(struct zone *zone,
+							struct mem_cgroup *)
+{
+	return 0;
+}
+
 static inline struct mem_cgroup *try_get_mem_cgroup_from_page(struct page *page)
 {
 	return NULL;
@@ -494,6 +503,9 @@ static inline bool memcg_kmem_enabled(void)
 	return static_key_false(&memcg_kmem_enabled_key);
 }
 
+bool memcg_kmem_is_active(struct mem_cgroup *memcg);
+bool memcg_kmem_should_reclaim(struct mem_cgroup *memcg);
+
 /*
  * In general, we'll do everything in our power to not incur in any overhead
  * for non-memcg users for the kmem functions. Not even a function call, if we
@@ -636,6 +648,16 @@ static inline bool memcg_kmem_enabled(void)
 	return false;
 }
 
+static inline bool memcg_kmem_is_active(struct mem_cgroup *memcg)
+{
+	return false;
+}
+
+static inline bool memcg_kmem_should_reclaim(struct mem_cgroup *memcg)
+{
+	return false;
+}
+
 static inline bool
 memcg_kmem_newpage_charge(gfp_t gfp, struct mem_cgroup **memcg, int order)
 {
include/linux/shrinker.h +9 -1
index 68c097077ef0..ab79b174bfbe 100644
@@ -20,8 +20,15 @@ struct shrink_control {
 
 	/* shrink from these nodes */
 	nodemask_t nodes_to_scan;
+
+	/* shrink from this memory cgroup hierarchy (if not NULL) */
+	struct mem_cgroup *target_mem_cgroup;
+
 	/* current node being shrunk (for NUMA aware shrinkers) */
 	int nid;
+
+	/* current memcg being shrunk (for memcg aware shrinkers) */
+	struct mem_cgroup *memcg;
 };
 
 #define SHRINK_STOP (~0UL)
@@ -63,7 +70,8 @@ struct shrinker {
 #define DEFAULT_SEEKS 2 /* A good number if you don't know better. */
 
 /* Flags */
-#define SHRINKER_NUMA_AWARE (1 << 0)
+#define SHRINKER_NUMA_AWARE	(1 << 0)
+#define SHRINKER_MEMCG_AWARE	(1 << 1)
 
 extern int register_shrinker(struct shrinker *);
 extern void unregister_shrinker(struct shrinker *);
mm/memcontrol.c +36 -1
index 854d0b8e3c45..24557d09213c 100644
@@ -392,7 +392,7 @@ static inline void memcg_kmem_set_active(struct mem_cgroup *memcg)
 	set_bit(KMEM_ACCOUNTED_ACTIVE, &memcg->kmem_account_flags);
 }
 
-static bool memcg_kmem_is_active(struct mem_cgroup *memcg)
+bool memcg_kmem_is_active(struct mem_cgroup *memcg)
 {
 	return test_bit(KMEM_ACCOUNTED_ACTIVE, &memcg->kmem_account_flags);
 }
@@ -1354,6 +1354,26 @@ out:
 	return lruvec;
 }
 
+unsigned long mem_cgroup_zone_reclaimable_pages(struct zone *zone,
+						struct mem_cgroup *memcg)
+{
+	int nid = zone_to_nid(zone);
+	int zid = zone_idx(zone);
+	unsigned long nr = 0;
+	struct mem_cgroup *iter;
+
+	iter = memcg;
+	do {
+		nr += mem_cgroup_zone_nr_lru_pages(iter, nid, zid,
+						   LRU_ALL_FILE);
+		if (do_swap_account)
+			nr += mem_cgroup_zone_nr_lru_pages(iter, nid, zid,
+							   LRU_ALL_ANON);
+		iter = mem_cgroup_iter(memcg, iter, NULL);
+	} while (iter);
+	return nr;
+}
+
 /*
  * Following LRU functions are allowed to be used without PCG_LOCK.
  * Operations are called by routine of global LRU independently from memcg.
@@ -2992,6 +3012,21 @@ static inline bool memcg_can_account_kmem(struct mem_cgroup *memcg)
 		memcg_kmem_is_active(memcg);
 }
 
+bool memcg_kmem_should_reclaim(struct mem_cgroup *memcg)
+{
+	struct mem_cgroup *iter;
+
+	iter = memcg;
+	do {
+		if (memcg_kmem_is_active(iter)) {
+			mem_cgroup_iter_break(memcg, iter);
+			return true;
+		}
+		iter = mem_cgroup_iter(memcg, iter, NULL);
+	} while (iter);
+	return false;
+}
+
 /*
  * helper for acessing a memcg's index. It will be used as an index in the
  * child cache array in kmem_cache, and also to derive its name. This function
mm/vmscan.c +74 -20

153 lines not on this page · raw message

Signature · 7 lines
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#9 lore raw

PATCH v15 9/13 list_lru: add per-memcg lists

In reply to Vladimir Davydov

There are several FS shrinkers, including super_block::s_shrink, that keep reclaimable objects in the list_lru structure. That said, to turn them to memcg-aware shrinkers, it is enough to make list_lru per-memcg.

This patch does the trick. It adds an array of LRU lists to the list_lru structure, one for each kmem-active memcg, and dispatches every item addition or removal operation to the list corresponding to the memcg the item is accounted to.

Since we already pass a shrink_control object to count and walk list_lru functions to specify the NUMA node to scan, and the target memcg is held in this structure, there is no need in changing the list_lru interface.

To make sure each kmem-active memcg has its list initialized in each memcg-enabled list_lru, we keep all memcg-enabled list_lrus in a linked list, which we iterate over allocating per-memcg LRUs whenever a new kmem-active memcg is added. To synchronize this with creation of new list_lrus, we have to take activate_kmem_mutex. Since using this mutex as is would make all mounts proceed serially, we turn it to an rw semaphore and take it for writing whenever a new kmem-active memcg is created and for reading when we are going to create a list_lru. This still does not allow mount_fs() proceed concurrently with creation of a kmem-active memcg, but since creation of memcgs is rather a rare event, this is not that critical.

The idea lying behind the patch as well as the initial implementation belong to Glauber Costa.

Cc Glauber Costa <[email protected]>
Cc Dave Chinner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Al Viro <[email protected]>
Cc Balbir Singh <[email protected]>
Cc KAMEZAWA Hiroyuki <[email protected]>
---
 include/linux/list_lru.h   |  112 ++++++++++++------
 include/linux/memcontrol.h |   13 +++
 mm/list_lru.c              |  271 +++++++++++++++++++++++++++++++++++++++-----
 mm/memcontrol.c            |  186 ++++++++++++++++++++++++++++--
 4 files changed, 511 insertions(+), 71 deletions(-)
include/linux/list_lru.h +78 -34
index 6ca43b2486fc..92d29cd790b2 100644
@@ -11,6 +11,8 @@
 #include <linux/nodemask.h>
 #include <linux/shrinker.h>
 
+struct mem_cgroup;
+
 /* list_lru_walk_cb has to always return one of those */
 enum lru_status {
 	LRU_REMOVED,		/* item removed from list */
@@ -32,10 +34,52 @@ struct list_lru_node {
 struct list_lru {
 	struct list_lru_node	*node;
 	nodemask_t		active_nodes;
+#ifdef CONFIG_MEMCG_KMEM
+	/*
+	 * In order to provide ability of scanning objects from different
+	 * memory cgroups independently, we keep a separate LRU list for each
+	 * kmem-active memcg in this array. The array is RCU-protected and
+	 * indexed by memcg_cache_id().
+	 */
+	struct list_lru_node	**memcg;
+	/*
+	 * Every time a kmem-active memcg is created or destroyed, we have to
+	 * update the array of per-memcg LRUs in each memcg enabled list_lru
+	 * structure. To achieve that, we keep all memcg enabled list_lru
+	 * structures in the all_memcg_lrus list.
+	 */
+	struct list_head	memcg_lrus_list;
+	/*
+	 * Since the array of per-memcg LRUs is RCU-protected, we can only free
+	 * it after a call to synchronize_rcu(). To avoid multiple calls to
+	 * synchronize_rcu() when a lot of LRUs get updated at the same time,
+	 * which is a typical scenario, we will store the pointer to the
+	 * previous version of the array in the memcg_old field for each
+	 * list_lru structure, and then free them all at once after a single
+	 * call to synchronize_rcu().
+	 */
+	void			*memcg_old;
+#endif /* CONFIG_MEMCG_KMEM */
 };
 
+#ifdef CONFIG_MEMCG_KMEM
+int list_lru_memcg_alloc(struct list_lru *lru, int memcg_id);
+void list_lru_memcg_free(struct list_lru *lru, int memcg_id);
+int list_lru_grow_memcg(struct list_lru *lru, size_t new_array_size);
+#endif
+
 void list_lru_destroy(struct list_lru *lru);
-int list_lru_init(struct list_lru *lru);
+int __list_lru_init(struct list_lru *lru, bool memcg_enabled);
+
+static inline int list_lru_init(struct list_lru *lru)
+{
+	return __list_lru_init(lru, false);
+}
+
+static inline int list_lru_init_memcg(struct list_lru *lru)
+{
+	return __list_lru_init(lru, true);
+}
 
 /**
  * list_lru_add: add an element to the lru list's tail
@@ -69,39 +113,41 @@ bool list_lru_add(struct list_lru *lru, struct list_head *item);
 bool list_lru_del(struct list_lru *lru, struct list_head *item);
 
 /**
- * list_lru_count_node: return the number of objects currently held by @lru
+ * list_lru_count_node_memcg: return the number of objects currently held by a
+ *  list_lru.
  * @lru: the lru pointer.
  * @nid: the node id to count from.
+ * @memcg: the memcg to count from.
  *
  * Always return a non-negative number, 0 for empty lists. There is no
  * guarantee that the list is not updated while the count is being computed.
  * Callers that want such a guarantee need to provide an outer lock.
  */
-unsigned long list_lru_count_node(struct list_lru *lru, int nid);
+unsigned long list_lru_count_node_memcg(struct list_lru *lru,
+					int nid, struct mem_cgroup *memcg);
 
-static inline unsigned long list_lru_shrink_count(struct list_lru *lru,
-						  struct shrink_control *sc)
+unsigned long list_lru_count(struct list_lru *lru);
+
+static inline unsigned long list_lru_count_node(struct list_lru *lru, int nid)
 {
-	return list_lru_count_node(lru, sc->nid);
+	return list_lru_count_node_memcg(lru, nid, NULL);
 }
 
-static inline unsigned long list_lru_count(struct list_lru *lru)
+static inline unsigned long list_lru_shrink_count(struct list_lru *lru,
+						  struct shrink_control *sc)
 {
-	long count = 0;
-	int nid;
-
-	for_each_node_mask(nid, lru->active_nodes)
-		count += list_lru_count_node(lru, nid);
-
-	return count;
+	return list_lru_count_node_memcg(lru, sc->nid, sc->memcg);
 }
 
 typedef enum lru_status
 (*list_lru_walk_cb)(struct list_head *item, spinlock_t *lock, void *cb_arg);
+
 /**
- * list_lru_walk_node: walk a list_lru, isolating and disposing freeable items.
+ * list_lru_walk_node_memcg: walk a list_lru, isolating and disposing freeable
+ *  items.
  * @lru: the lru pointer.
  * @nid: the node id to scan from.
+ * @memcg: the memcg to scan from.
  * @isolate: callback function that is resposible for deciding what to do with
  *  the item currently being scanned
  * @cb_arg: opaque type that will be passed to @isolate
@@ -119,31 +165,29 @@ typedef enum lru_status
  *
  * Return value: the number of objects effectively removed from the LRU.
  */
-unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
-				 list_lru_walk_cb isolate, void *cb_arg,
-				 unsigned long *nr_to_walk);
+unsigned long list_lru_walk_node_memcg(struct list_lru *lru,
+				       int nid, struct mem_cgroup *memcg,
+				       list_lru_walk_cb isolate, void *cb_arg,
+				       unsigned long *nr_to_walk);
+
+unsigned long list_lru_walk(struct list_lru *lru,
+			    list_lru_walk_cb isolate, void *cb_arg,
+			    unsigned long nr_to_walk);
 
 static inline unsigned long
-list_lru_shrink_walk(struct list_lru *lru, struct shrink_control *sc,
-		     list_lru_walk_cb isolate, void *cb_arg)
+list_lru_walk_node(struct list_lru *lru, int nid,
+		   list_lru_walk_cb isolate, void *cb_arg,
+		   unsigned long *nr_to_walk)
 {
-	return list_lru_walk_node(lru, sc->nid, isolate, cb_arg,
-				  &sc->nr_to_scan);
+	return list_lru_walk_node_memcg(lru, nid, NULL,
+					isolate, cb_arg, nr_to_walk);
 }
 
 static inline unsigned long
-list_lru_walk(struct list_lru *lru, list_lru_walk_cb isolate,
-	      void *cb_arg, unsigned long nr_to_walk)
+list_lru_shrink_walk(struct list_lru *lru, struct shrink_control *sc,
+		     list_lru_walk_cb isolate, void *cb_arg)
 {
-	long isolated = 0;
-	int nid;
-
-	for_each_node_mask(nid, lru->active_nodes) {
-		isolated += list_lru_walk_node(lru, nid, isolate,
-					       cb_arg, &nr_to_walk);
-		if (nr_to_walk <= 0)
-			break;
-	}
-	return isolated;
+	return list_lru_walk_node_memcg(lru, sc->nid, sc->memcg,
+					isolate, cb_arg, &sc->nr_to_scan);
 }
 #endif /* _LRU_LIST_H */
include/linux/memcontrol.h +13 -0
index fc4a24d31e99..3b310c58822a 100644
@@ -29,6 +29,7 @@ struct page_cgroup;
 struct page;
 struct mm_struct;
 struct kmem_cache;
+struct list_lru;
 
 /*
  * The corresponding mem_cgroup_stat_names is defined in mm/memcontrol.c,
@@ -539,6 +540,9 @@ __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp);
 void mem_cgroup_destroy_cache(struct kmem_cache *cachep);
 void kmem_cache_destroy_memcg_children(struct kmem_cache *s);
 
+int memcg_list_lru_init(struct list_lru *lru, bool memcg_enabled);
+void memcg_list_lru_destroy(struct list_lru *lru);
+
 /**
  * memcg_kmem_newpage_charge: verify if a new kmem allocation is allowed.
  * @gfp: the gfp allocation flags.
@@ -705,6 +709,15 @@ memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
 static inline void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 {
 }
+
+static inline int memcg_list_lru_init(struct list_lru *lru, bool memcg_enabled)
+{
+	return 0;
+}
+
+static inline void memcg_list_lru_destroy(struct list_lru *lru)
+{
+}
 #endif /* CONFIG_MEMCG_KMEM */
 #endif /* _LINUX_MEMCONTROL_H */
 
mm/list_lru.c +245 -26

340 lines not on this page · raw message

mm/memcontrol.c +175 -11

265 lines not on this page · raw message

Signature · 7 lines
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#10 lore raw

PATCH v15 10/13 fs: make shrinker memcg aware

In reply to Vladimir Davydov

Now, to make any list_lru-based shrinker memcg aware we should only initialize its list_lru as memcg-enabled. Let's do it for the general FS shrinker (super_block::s_shrink) and mark it as memcg aware.

There are other FS-specific shrinkers that use list_lru for storing objects, such as XFS and GFS2 dquot cache shrinkers, but since they reclaim objects that may be shared among different cgroups, there is no point making them memcg aware.

Cc Glauber Costa <[email protected]>
Cc Dave Chinner <[email protected]>
Cc Al Viro <[email protected]>
---
 fs/super.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
fs/super.c +3 -3
index 33cbff3769e7..6a58a7196fb2 100644
@@ -191,9 +191,9 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags)
 	INIT_LIST_HEAD(&s->s_inodes);
 	INIT_WORK(&s->destroy, destroy_super_work_func);
 
-	if (list_lru_init(&s->s_dentry_lru))
+	if (list_lru_init_memcg(&s->s_dentry_lru))
 		goto fail;
-	if (list_lru_init(&s->s_inode_lru))
+	if (list_lru_init_memcg(&s->s_inode_lru))
 		goto fail;
 
 	init_rwsem(&s->s_umount);
@@ -230,7 +230,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags)
 	s->s_shrink.scan_objects = super_cache_scan;
 	s->s_shrink.count_objects = super_cache_count;
 	s->s_shrink.batch = 1024;
-	s->s_shrink.flags = SHRINKER_NUMA_AWARE;
+	s->s_shrink.flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE;
 	return s;
 
 fail:
Signature · 7 lines
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#11 lore raw

PATCH v15 11/13 memcg: flush memcg items upon memcg destruction

In reply to Vladimir Davydov

From: Glauber Costa <[email protected]>

When a memcg is destroyed, it won't be imediately released until all objects are gone. This means that if a memcg is restarted with the very same workload - a very common case, the objects already cached won't be billed to the new memcg. This is mostly undesirable since a container can exploit this by restarting itself every time it reaches its limit, and then coming up again with a fresh new limit.

Since now we have targeted reclaim, I sustain that we should assume that a memcg that is destroyed should be flushed away. It makes perfect sense if we assume that a memcg that goes away most likely indicates an isolated workload that is terminated.

Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Balbir Singh <[email protected]>
Cc KAMEZAWA Hiroyuki <[email protected]>
---
 mm/memcontrol.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
mm/memcontrol.c +28 -0
index 27f6d795090a..aed1456015cf 100644
@@ -6155,12 +6155,40 @@ static void memcg_destroy_kmem(struct mem_cgroup *memcg)
 	memcg_destroy_all_lrus(memcg);
 }
 
+static void memcg_drop_slab(struct mem_cgroup *memcg)
+{
+	struct shrink_control shrink = {
+		.gfp_mask = GFP_KERNEL,
+		.target_mem_cgroup = memcg,
+	};
+	unsigned long nr_objects;
+
+	nodes_setall(shrink.nodes_to_scan);
+	do {
+		nr_objects = shrink_slab(&shrink, 1000, 1000);
+	} while (nr_objects > 10);
+}
+
 static void kmem_cgroup_css_offline(struct mem_cgroup *memcg)
 {
 	if (!memcg_kmem_is_active(memcg))
 		return;
 
 	/*
+	 * When a memcg is destroyed, it won't be imediately released until all
+	 * objects are gone. This means that if a memcg is restarted with the
+	 * very same workload - a very common case, the objects already cached
+	 * won't be billed to the new memcg. This is mostly undesirable since a
+	 * container can exploit this by restarting itself every time it
+	 * reaches its limit, and then coming up again with a fresh new limit.
+	 *
+	 * Therefore a memcg that is destroyed should be flushed away. It makes
+	 * perfect sense if we assume that a memcg that goes away indicates an
+	 * isolated workload that is terminated.
+	 */
+	memcg_drop_slab(memcg);
+
+	/*
 	 * kmem charges can outlive the cgroup. In the case of slab
 	 * pages, for instance, a page contain objects from various
 	 * processes. As we prevent from taking a reference for every
Signature · 7 lines
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#12 lore raw

PATCH v15 12/13 vmpressure: in-kernel notifications

In reply to Vladimir Davydov

From: Glauber Costa <[email protected]>

During the past weeks, it became clear to us that the shrinker interface we have right now works very well for some particular types of users, but not that well for others. The latter are usually people interested in one-shot notifications, that were forced to adapt themselves to the count+scan behavior of shrinkers. To do so, they had no choice than to greatly abuse the shrinker interface producing little monsters all over.

During LSF/MM, one of the proposals that popped out during our session was to reuse Anton Voronstsov's vmpressure for this. They are designed for userspace consumption, but also provide a well-stablished, cgroup-aware entry point for notifications.

This patch extends that to also support in-kernel users. Events that should be generated for in-kernel consumption will be marked as such, and for those, we will call a registered function instead of triggering an eventfd notification.

Please note that due to my lack of understanding of each shrinker user, I will stay away from converting the actual users, you are all welcome to do so.

---
 include/linux/vmpressure.h |    5 +++++
 mm/vmpressure.c            |   53 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 3 deletions(-)
include/linux/vmpressure.h +5 -0
index 3e4535876d37..67f0fbe52c3e 100644
@@ -20,6 +20,9 @@ struct vmpressure {
 	/* Have to grab the lock on events traversal or modifications. */
 	struct mutex events_lock;
 
+	/* False if only kernel users want to be notified, true otherwise. */
+	bool notify_userspace;
+
 	struct work_struct work;
 };
 
@@ -37,6 +40,8 @@ extern struct cgroup_subsys_state *vmpressure_to_css(struct vmpressure *vmpr);
 extern int vmpressure_register_event(struct mem_cgroup *memcg,
 				     struct eventfd_ctx *eventfd,
 				     const char *args);
+extern int vmpressure_register_kernel_event(struct mem_cgroup *memcg,
+					    void (*fn)(void));
 extern void vmpressure_unregister_event(struct mem_cgroup *memcg,
 					struct eventfd_ctx *eventfd);
 #else
mm/vmpressure.c +50 -3

91 lines not on this page · raw message

Signature · 7 lines
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#13 lore raw

PATCH v15 13/13 memcg: reap dead memcgs upon global memory pressure

In reply to Vladimir Davydov

From: Glauber Costa <[email protected]>

When we delete kmem-enabled memcgs, they can still be zombieing around for a while. The reason is that the objects may still be alive, and we won't be able to delete them at destruction time.

The only entry point for that, though, are the shrinkers. The shrinker interface, however, is not exactly tailored to our needs. It could be a little bit better by using the API Dave Chinner proposed, but it is still not ideal since we aren't really a count-and-scan event, but more a one-off flush-all-you-can event that would have to abuse that somehow.

Cc Anton Vorontsov <[email protected]>
Cc John Stultz <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
Cc Johannes Weiner <[email protected]>
---
 mm/memcontrol.c |   82 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 79 insertions(+), 3 deletions(-)
mm/memcontrol.c +79 -3
index aed1456015cf..14b152f4c69b 100644
@@ -328,8 +328,16 @@ struct mem_cgroup {
 	/* thresholds for mem+swap usage. RCU-protected */
 	struct mem_cgroup_thresholds memsw_thresholds;
 
-	/* For oom notifier event fd */
-	struct list_head oom_notify;
+	union {
+		/* For oom notifier event fd */
+		struct list_head oom_notify;
+		/*
+		 * we can only trigger an oom event if the memcg is alive.
+		 * so we will reuse this field to hook the memcg in the list
+		 * of dead memcgs.
+		 */
+		struct list_head dead;
+	};
 
 	/*
 	 * Should we move charges of a task when a task is moved into this
@@ -6134,6 +6142,58 @@ static int mem_cgroup_oom_control_write(struct cgroup_subsys_state *css,
 }
 
 #ifdef CONFIG_MEMCG_KMEM
+static LIST_HEAD(dangling_memcgs);
+static DEFINE_MUTEX(dangling_memcgs_mutex);
+
+static void memcg_dangling_add(struct mem_cgroup *memcg)
+{
+	mutex_lock(&dangling_memcgs_mutex);
+	list_add(&memcg->dead, &dangling_memcgs);
+	mutex_unlock(&dangling_memcgs_mutex);
+}
+
+static void memcg_dangling_del(struct mem_cgroup *memcg)
+{
+	mutex_lock(&dangling_memcgs_mutex);
+	list_del(&memcg->dead);
+	mutex_unlock(&dangling_memcgs_mutex);
+}
+
+static void memcg_vmpressure_shrink_dead(void)
+{
+	struct memcg_cache_params *params, *tmp;
+	struct kmem_cache *cachep;
+	struct mem_cgroup *memcg;
+
+	mutex_lock(&dangling_memcgs_mutex);
+	list_for_each_entry(memcg, &dangling_memcgs, dead) {
+		mutex_lock(&memcg->slab_caches_mutex);
+		/* The element may go away as an indirect result of shrink */
+		list_for_each_entry_safe(params, tmp,
+					 &memcg->memcg_slab_caches, list) {
+			cachep = memcg_params_to_cache(params);
+			/*
+			 * the cpu_hotplug lock is taken in kmem_cache_create
+			 * outside the slab_caches_mutex manipulation. It will
+			 * be taken by kmem_cache_shrink to flush the cache.
+			 * So we need to drop the lock. It is all right because
+			 * the lock only protects elements moving in and out the
+			 * list.
+			 */
+			mutex_unlock(&memcg->slab_caches_mutex);
+			kmem_cache_shrink(cachep);
+			mutex_lock(&memcg->slab_caches_mutex);
+		}
+		mutex_unlock(&memcg->slab_caches_mutex);
+	}
+	mutex_unlock(&dangling_memcgs_mutex);
+}
+
+static void memcg_register_kmem_events(struct mem_cgroup *memcg)
+{
+	vmpressure_register_kernel_event(memcg, memcg_vmpressure_shrink_dead);
+}
+
 static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
 {
 	int ret;
@@ -6217,6 +6277,18 @@ static void kmem_cgroup_css_offline(struct mem_cgroup *memcg)
 		css_put(&memcg->css);
 }
 #else
+static void memcg_dangling_add(struct mem_cgroup *memcg)
+{
+}
+
+static void memcg_dangling_del(struct mem_cgroup *memcg)
+{
+}
+
+static void memcg_register_kmem_events(struct mem_cgroup *memcg)
+{
+}
+
 static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
 {
 	return 0;
@@ -6767,8 +6839,10 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
 	if (css->cgroup->id > MEM_CGROUP_ID_MAX)
 		return -ENOSPC;
 
-	if (!parent)
+	if (!parent) {
+		memcg_register_kmem_events(memcg);
 		return 0;
+	}
 
 	mutex_lock(&memcg_create_mutex);
 
@@ -6842,6 +6916,7 @@ static void mem_cgroup_css_offline(struct cgroup_subsys_state *css)
 	mem_cgroup_invalidate_reclaim_iterators(memcg);
 	mem_cgroup_reparent_charges(memcg);
 	mem_cgroup_destroy_all_caches(memcg);
+	memcg_dangling_add(memcg);
 	vmpressure_cleanup(&memcg->vmpressure);
 }
 
@@ -6886,6 +6961,7 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css)
 	mem_cgroup_reparent_charges(memcg);
 
 	memcg_destroy_kmem(memcg);
+	memcg_dangling_del(memcg);
 	__mem_cgroup_free(memcg);
 }
 
Signature · 7 lines
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#14 lore raw
In reply to Vladimir Davydov

Hi Michal, Johannes, David,

Could you please take a look at this if you have time? Without your review, it'll never get committed.

Thank you.

Quoted 78 lines · Vladimir Davydov

Hi,

This is the 15th iteration of Glauber Costa's patch-set implementing slab shrinking on memcg pressure. The main idea is to make the list_lru structure used by most FS shrinkers per-memcg. When adding or removing an element from a list_lru, we use the page information to figure out which memcg it belongs to and relay it to the appropriate list. This allows scanning kmem objects accounted to different memcgs independently.

Please note that this patch-set implements slab shrinking only when we hit the user memory limit so that kmem allocations will still fail if we are below the user memory limit, but close to the kmem limit. I am going to fix this in a separate patch-set, but currently it is only worthwhile setting the kmem limit to be greater than the user mem limit just to enable per-memcg slab accounting and reclaim.

The patch-set is based on top of v3.14-rc1-mmots-2014-02-04-16-48 (there are some vmscan cleanups that I need committed there) and organized as follows:
- patches 1-4 introduce some minor changes to memcg needed for this set;
- patches 5-7 prepare fs for per-memcg list_lru;
- patch 8 implement kmemcg reclaim core;
- patch 9 make list_lru per-memcg and patch 10 marks sb shrinker memcg-aware;
- patch 10 is trivial - it issues shrinkers on memcg destruction;
- patches 12 and 13 introduce shrinking of dead kmem caches to facilitate memcg destruction.

Changes in v15:
- remove patches that have been merged to -mm;
- fix memory barrier usage in per-memcg list_lru implementation;
- fix list_lru_destroy(), which might sleep for per-memcg lrus, called from atomic context (__put_super()).

Previous iterations of this patch-set can be found here:
- https://lkml.org/lkml/2013/12/16/206 (v14)
- https://lkml.org/lkml/2013/12/9/103 (v13)
- https://lkml.org/lkml/2013/12/2/141 (v12)
- https://lkml.org/lkml/2013/11/25/214 (v11)

Comments are highly appreciated.

Thanks.

Glauber Costa (6):
memcg: make cache index determination more robust
memcg: consolidate callers of memcg_cache_id
memcg: move initialization to memcg creation
memcg: flush memcg items upon memcg destruction
vmpressure: in-kernel notifications
memcg: reap dead memcgs upon global memory pressure

Vladimir Davydov (7):
memcg: make for_each_mem_cgroup macros public list_lru, shrinkers: introduce list_lru_shrink_{count,walk}
fs: consolidate {nr,free}_cached_objects args in shrink_control
fs: do not call destroy_super() in atomic context
vmscan: shrink slab on memcg pressure
list_lru: add per-memcg lists
fs: make shrinker memcg aware

 fs/dcache.c                |   14 +-
 fs/gfs2/quota.c            |    6 +-
 fs/inode.c                 |    7 +-
 fs/internal.h              |    7 +-
 fs/super.c                 |   44 ++---
 fs/xfs/xfs_buf.c           |    7 +-
 fs/xfs/xfs_qm.c            |    7 +-
 fs/xfs/xfs_super.c         |    7 +-
 include/linux/fs.h         |    8 +-
 include/linux/list_lru.h   |  112 ++++++++++---
 include/linux/memcontrol.h |   50 ++++++
 include/linux/shrinker.h   |   10 +-
 include/linux/vmpressure.h |    5 +
 mm/list_lru.c              |  271 +++++++++++++++++++++++++++---
 mm/memcontrol.c            |  399 ++++++++++++++++++++++++++++++++++++++------
 mm/vmpressure.c            |   53 +++++-
 mm/vmscan.c                |   94 ++++++++---
 17 files changed, 926 insertions(+), 175 deletions(-)
#15 lore raw
Quoted 4 lines · Vladimir Davydov

Hi Michal, Johannes, David,

Could you please take a look at this if you have time? Without your review, it'll never get committed.

Yes, it is on my todo list. I could barely catch up with discussions on the previous versions and felt that David was quite concerned about some high level decisions. I have to check, re-read whether there are still there.

I am sorry that it takes so long but I am really busy with internal things recently.

Signature · 8 lines
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
maintainer
#16 lore raw
In reply to Vladimir Davydov
Quoted 4 lines · Vladimir Davydov

Hi Michal, Johannes, David,

Could you please take a look at this if you have time? Without your review, it'll never get committed.

There is simply no review bandwidth for new features as long as we are fixing fundamental bugs in memcg.

Quoted 17 lines · Vladimir Davydov
Quoted 16 lines

Hi,

This is the 15th iteration of Glauber Costa's patch-set implementing slab shrinking on memcg pressure. The main idea is to make the list_lru structure used by most FS shrinkers per-memcg. When adding or removing an element from a list_lru, we use the page information to figure out which memcg it belongs to and relay it to the appropriate list. This allows scanning kmem objects accounted to different memcgs independently.

Please note that this patch-set implements slab shrinking only when we hit the user memory limit so that kmem allocations will still fail if we are below the user memory limit, but close to the kmem limit. I am going to fix this in a separate patch-set, but currently it is only worthwhile setting the kmem limit to be greater than the user mem limit just to enable per-memcg slab accounting and reclaim.

Quoted 9 lines
Quoted 9 lines

The patch-set is based on top of v3.14-rc1-mmots-2014-02-04-16-48 (there are some vmscan cleanups that I need committed there) and organized as follows:
- patches 1-4 introduce some minor changes to memcg needed for this set;
- patches 5-7 prepare fs for per-memcg list_lru;
- patch 8 implement kmemcg reclaim core;
- patch 9 make list_lru per-memcg and patch 10 marks sb shrinker memcg-aware;
- patch 10 is trivial - it issues shrinkers on memcg destruction;
- patches 12 and 13 introduce shrinking of dead kmem caches to facilitate memcg destruction.

In the context of the ongoing discussions about charge reparenting I was curious how you deal with charges becoming unreclaimable after a memcg has been offlined.

Patch #11 drops all charged objects at offlining by just invoking shrink_slab() in a loop until "only a few" (10) objects are remaining. How long is this going to take? And why is it okay to destroy these caches when somebody else might still be using them?

That still leaves you with the free objects that slab caches retain for allocation efficiency, so now you put all dead memcgs in the system on a global list, and on a vmpressure event on root_mem_cgroup you walk the global list and drain the freelist of all remaining caches.

This is a lot of complexity and scalability problems for less than desirable behavior.

Please think about how we can properly reparent kmemcg charges during memcg teardown. That would simplify your code immensely and help clean up this unholy mess of css pinning.

Slab caches are already collected in the memcg and on destruction could be reassigned to the parent. Kmemcg uncharge from slab freeing would have to be updated to use the memcg from the cache, not from the individual page, but I don't see why this wouldn't work right now.

Charged thread stack pages could be reassigned when the task itself is migrated out of a cgroup.

It would mean that you can't simply use __GFP_KMEMCG and just pin the css until you can be bothered to return it. There must be a way for any memcg charge to be reparented on demand.

#17 lore raw
Quoted 34 lines · Johannes Weiner
Quoted 4 lines · Vladimir Davydov

Hi Michal, Johannes, David,

Could you please take a look at this if you have time? Without your review, it'll never get committed.

There is simply no review bandwidth for new features as long as we are fixing fundamental bugs in memcg.

Quoted 17 lines · Vladimir Davydov
Quoted 16 lines

Hi,

This is the 15th iteration of Glauber Costa's patch-set implementing slab shrinking on memcg pressure. The main idea is to make the list_lru structure used by most FS shrinkers per-memcg. When adding or removing an element from a list_lru, we use the page information to figure out which memcg it belongs to and relay it to the appropriate list. This allows scanning kmem objects accounted to different memcgs independently.

Please note that this patch-set implements slab shrinking only when we hit the user memory limit so that kmem allocations will still fail if we are below the user memory limit, but close to the kmem limit. I am going to fix this in a separate patch-set, but currently it is only worthwhile setting the kmem limit to be greater than the user mem limit just to enable per-memcg slab accounting and reclaim.

Quoted 9 lines
Quoted 9 lines

The patch-set is based on top of v3.14-rc1-mmots-2014-02-04-16-48 (there are some vmscan cleanups that I need committed there) and organized as follows:
- patches 1-4 introduce some minor changes to memcg needed for this set;
- patches 5-7 prepare fs for per-memcg list_lru;
- patch 8 implement kmemcg reclaim core;
- patch 9 make list_lru per-memcg and patch 10 marks sb shrinker memcg-aware;
- patch 10 is trivial - it issues shrinkers on memcg destruction;
- patches 12 and 13 introduce shrinking of dead kmem caches to facilitate memcg destruction.

Quoted 8 lines

In the context of the ongoing discussions about charge reparenting I was curious how you deal with charges becoming unreclaimable after a memcg has been offlined.

Patch #11 drops all charged objects at offlining by just invoking shrink_slab() in a loop until "only a few" (10) objects are remaining. How long is this going to take? And why is it okay to destroy these caches when somebody else might still be using them?

IMHO, on container destruction we have to drop as many objects accounted to this container as we can, because otherwise any container will be able to get access to any number of unaccounted objects by fetching them and then rebooting.

Quoted 9 lines

That still leaves you with the free objects that slab caches retain for allocation efficiency, so now you put all dead memcgs in the system on a global list, and on a vmpressure event on root_mem_cgroup you walk the global list and drain the freelist of all remaining caches.

This is a lot of complexity and scalability problems for less than desirable behavior.

Quoted 8 lines

Please think about how we can properly reparent kmemcg charges during memcg teardown. That would simplify your code immensely and help clean up this unholy mess of css pinning.

Slab caches are already collected in the memcg and on destruction could be reassigned to the parent. Kmemcg uncharge from slab freeing would have to be updated to use the memcg from the cache, not from the individual page, but I don't see why this wouldn't work right now.

I don't think I understand what you mean by reassigning slab caches to the parent.

If you mean moving all pages (slabs) from the cache of the memcg being destroyed to the corresponding root cache (or the parent memcg's cache) and then destroying the memcg's cache, I don't think this is feasible, because slub free's fast path is lockless, so AFAIU we can't remove a partial slab from a cache w/o risking to race with kmem_cache_free.

If you mean clearing all pointers from the memcg's cache to the memcg (changing them to the parent or root memcg), then AFAIU this won't solve the problem with "dangling" caches - we will still have to shrink them on vmpressure. So although this would allow us to put the reference to the memcg from kmem caches on memcg's death, it wouldn't simplify the code at all, in fact, it would even make it more complicated, because we would have to handle various corner cases like reparenting vs list_lru_{add,remove}.

Quoted 2 lines

Charged thread stack pages could be reassigned when the task itself is migrated out of a cgroup.

Thread info pages are only a part of the problem. If a process kmalloc's an object of size >= KMALLOC_MAX_CACHE_SIZE, it will be given a compound page accounted to kmemcg, and we won't be able to find this page given the memcg it is accounted to (except for walking the whole page range). Thus we will have to organize those pages in per-memcg lists, won't we? Again, even more complexity.

Although I agree with you that it would be nice to reparent kmem on memcg destruction, currently I don't see a way to implement this w/o significantly complicating the code, but I keep thinking.

Thanks.

Quoted 3 lines

It would mean that you can't simply use __GFP_KMEMCG and just pin the css until you can be bothered to return it. There must be a way for any memcg charge to be reparented on demand.

maintainer
#18 lore raw
Quoted 44 lines · Vladimir Davydov
Quoted 34 lines · Johannes Weiner
Quoted 4 lines · Vladimir Davydov

Hi Michal, Johannes, David,

Could you please take a look at this if you have time? Without your review, it'll never get committed.

There is simply no review bandwidth for new features as long as we are fixing fundamental bugs in memcg.

Quoted 17 lines · Vladimir Davydov
Quoted 16 lines

Hi,

This is the 15th iteration of Glauber Costa's patch-set implementing slab shrinking on memcg pressure. The main idea is to make the list_lru structure used by most FS shrinkers per-memcg. When adding or removing an element from a list_lru, we use the page information to figure out which memcg it belongs to and relay it to the appropriate list. This allows scanning kmem objects accounted to different memcgs independently.

Please note that this patch-set implements slab shrinking only when we hit the user memory limit so that kmem allocations will still fail if we are below the user memory limit, but close to the kmem limit. I am going to fix this in a separate patch-set, but currently it is only worthwhile setting the kmem limit to be greater than the user mem limit just to enable per-memcg slab accounting and reclaim.

Quoted 9 lines
Quoted 9 lines

The patch-set is based on top of v3.14-rc1-mmots-2014-02-04-16-48 (there are some vmscan cleanups that I need committed there) and organized as follows:
- patches 1-4 introduce some minor changes to memcg needed for this set;
- patches 5-7 prepare fs for per-memcg list_lru;
- patch 8 implement kmemcg reclaim core;
- patch 9 make list_lru per-memcg and patch 10 marks sb shrinker memcg-aware;
- patch 10 is trivial - it issues shrinkers on memcg destruction;
- patches 12 and 13 introduce shrinking of dead kmem caches to facilitate memcg destruction.

Quoted 8 lines

In the context of the ongoing discussions about charge reparenting I was curious how you deal with charges becoming unreclaimable after a memcg has been offlined.

Patch #11 drops all charged objects at offlining by just invoking shrink_slab() in a loop until "only a few" (10) objects are remaining. How long is this going to take? And why is it okay to destroy these caches when somebody else might still be using them?

Quoted 4 lines

IMHO, on container destruction we have to drop as many objects accounted to this container as we can, because otherwise any container will be able to get access to any number of unaccounted objects by fetching them and then rebooting.

They're accounted to and subject to the limit of the parent. I don't see how this is different than page cache.

Quoted 27 lines
Quoted 9 lines

That still leaves you with the free objects that slab caches retain for allocation efficiency, so now you put all dead memcgs in the system on a global list, and on a vmpressure event on root_mem_cgroup you walk the global list and drain the freelist of all remaining caches.

This is a lot of complexity and scalability problems for less than desirable behavior.

Quoted 8 lines

Please think about how we can properly reparent kmemcg charges during memcg teardown. That would simplify your code immensely and help clean up this unholy mess of css pinning.

Slab caches are already collected in the memcg and on destruction could be reassigned to the parent. Kmemcg uncharge from slab freeing would have to be updated to use the memcg from the cache, not from the individual page, but I don't see why this wouldn't work right now.

I don't think I understand what you mean by reassigning slab caches to the parent.

If you mean moving all pages (slabs) from the cache of the memcg being destroyed to the corresponding root cache (or the parent memcg's cache) and then destroying the memcg's cache, I don't think this is feasible, because slub free's fast path is lockless, so AFAIU we can't remove a partial slab from a cache w/o risking to race with kmem_cache_free.

Quoted 8 lines

If you mean clearing all pointers from the memcg's cache to the memcg (changing them to the parent or root memcg), then AFAIU this won't solve the problem with "dangling" caches - we will still have to shrink them on vmpressure. So although this would allow us to put the reference to the memcg from kmem caches on memcg's death, it wouldn't simplify the code at all, in fact, it would even make it more complicated, because we would have to handle various corner cases like reparenting vs list_lru_{add,remove}.

I think we have different concepts of what's complicated. There is an existing model of what to do with left-over cache memory when a cgroup is destroyed, which is reparenting. The rough steps will be the same, the object lifetime will be the same, the css refcounting will be the same, the user-visible behavior will be the same. Any complexity from charge vs. reparent races will be contained to a few lines of code.

Weird refcounting tricks during offline, trashing kmem caches instead of moving them to the parent like other memory, a global list of dead memcgs and sudden freelist thrashing on a vmpressure event, that's what adds complexity and what makes this code unpredictable, fragile, and insanely hard to work with. It's not acceptable.

By reparenting I meant reassigning the memcg cache parameter pointer from the slab cache such that it points to the parent. This should be an atomic operation. All css lookups already require RCU (I think slab does not follow this yet because we guarantee that css reference, but it should be changed). So switch the cache param pointer, insert an RCU graceperiod to wait for all the ongoing charges and uncharges until nobody sees the memcg anymore, then safely reparent all the remaining memcg objects to the parent. Maybe individually, maybe we can just splice the lists to the parent's list_lru lists.

I'm not sure I understand how the dangling cache problem pertains to this, isn't this an entirely separate issue?

Quoted 9 lines
Quoted 2 lines

Charged thread stack pages could be reassigned when the task itself is migrated out of a cgroup.

Thread info pages are only a part of the problem. If a process kmalloc's an object of size >= KMALLOC_MAX_CACHE_SIZE, it will be given a compound page accounted to kmemcg, and we won't be able to find this page given the memcg it is accounted to (except for walking the whole page range). Thus we will have to organize those pages in per-memcg lists, won't we? Again, even more complexity.

Why do we track them in the first place? We don't track any random page allocation, so we shouldn't track kmalloc() that falls back to the page allocator. In fact we shouldn't track any random slab allocation.

The types of allocations we primarily want to track are the ones that directly scale with user behavior. This is a finite set, which on a global level is covered mostly by ulimits. After all, an unprivileged user interfering with other users is not a new problem and existed long before memcg.

It was a mistake to provide __GFP_KMEMCG and allow charging any random allocation, without giving memcg the means to actually manage that memory. I don't see that such flexibility even needed, and it clearly hurts us now. It was a choice we made to keep things simple in the beginning, before we knew how all this is going to turn out. We should rectify this mistake before building on top of it.

Here is the much bigger issue behind this:

Memcg should be a thin layer of accounting and limiting between the VM and cgroup core code, but look at the line count. It's more code than all of the page reclaim logic combined, including the page replacement algorithm, rmap, LRU list handling - all of which already include a good deal of memcg specifics. It's the same size as the scheduler core, which includes the entire cpu cgroup controller. And all this other code is of better quality and has more eyes on it than memcg.

Please demonstrate that you try to see the bigger picture behind memcg and make an effort to keep things simple beyond the code you introduce and the niche you care about, otherwise I'm not willing to take any patches from you that don't straight-up delete stuff.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#19 lore raw
Quoted 98 lines · Johannes Weiner
Quoted 39 lines · Vladimir Davydov
Quoted 38 lines · Johannes Weiner
Quoted 4 lines · Vladimir Davydov

Hi Michal, Johannes, David,

Could you please take a look at this if you have time? Without your review, it'll never get committed.

There is simply no review bandwidth for new features as long as we are fixing fundamental bugs in memcg.

Quoted 17 lines · Vladimir Davydov
Quoted 16 lines

Hi,

This is the 15th iteration of Glauber Costa's patch-set implementing slab shrinking on memcg pressure. The main idea is to make the list_lru structure used by most FS shrinkers per-memcg. When adding or removing an element from a list_lru, we use the page information to figure out which memcg it belongs to and relay it to the appropriate list. This allows scanning kmem objects accounted to different memcgs independently.

Please note that this patch-set implements slab shrinking only when we hit the user memory limit so that kmem allocations will still fail if we are below the user memory limit, but close to the kmem limit. I am going to fix this in a separate patch-set, but currently it is only worthwhile setting the kmem limit to be greater than the user mem limit just to enable per-memcg slab accounting and reclaim.

Quoted 9 lines
Quoted 9 lines

The patch-set is based on top of v3.14-rc1-mmots-2014-02-04-16-48 (there are some vmscan cleanups that I need committed there) and organized as follows:
- patches 1-4 introduce some minor changes to memcg needed for this set;
- patches 5-7 prepare fs for per-memcg list_lru;
- patch 8 implement kmemcg reclaim core;
- patch 9 make list_lru per-memcg and patch 10 marks sb shrinker memcg-aware;
- patch 10 is trivial - it issues shrinkers on memcg destruction;
- patches 12 and 13 introduce shrinking of dead kmem caches to facilitate memcg destruction.

In the context of the ongoing discussions about charge reparenting I was curious how you deal with charges becoming unreclaimable after a memcg has been offlined.

Quoted 8 lines
Quoted 4 lines

Patch #11 drops all charged objects at offlining by just invoking shrink_slab() in a loop until "only a few" (10) objects are remaining. How long is this going to take? And why is it okay to destroy these caches when somebody else might still be using them?

IMHO, on container destruction we have to drop as many objects accounted to this container as we can, because otherwise any container will be able to get access to any number of unaccounted objects by fetching them and then rebooting.

They're accounted to and subject to the limit of the parent. I don't see how this is different than page cache.

Quoted 26 lines
Quoted 9 lines

That still leaves you with the free objects that slab caches retain for allocation efficiency, so now you put all dead memcgs in the system on a global list, and on a vmpressure event on root_mem_cgroup you walk the global list and drain the freelist of all remaining caches.

This is a lot of complexity and scalability problems for less than desirable behavior.

Quoted 8 lines

Please think about how we can properly reparent kmemcg charges during memcg teardown. That would simplify your code immensely and help clean up this unholy mess of css pinning.

Slab caches are already collected in the memcg and on destruction could be reassigned to the parent. Kmemcg uncharge from slab freeing would have to be updated to use the memcg from the cache, not from the individual page, but I don't see why this wouldn't work right now.

I don't think I understand what you mean by reassigning slab caches to the parent.

If you mean moving all pages (slabs) from the cache of the memcg being destroyed to the corresponding root cache (or the parent memcg's cache) and then destroying the memcg's cache, I don't think this is feasible, because slub free's fast path is lockless, so AFAIU we can't remove a partial slab from a cache w/o risking to race with kmem_cache_free.

Quoted 8 lines

If you mean clearing all pointers from the memcg's cache to the memcg (changing them to the parent or root memcg), then AFAIU this won't solve the problem with "dangling" caches - we will still have to shrink them on vmpressure. So although this would allow us to put the reference to the memcg from kmem caches on memcg's death, it wouldn't simplify the code at all, in fact, it would even make it more complicated, because we would have to handle various corner cases like reparenting vs list_lru_{add,remove}.

I think we have different concepts of what's complicated. There is an existing model of what to do with left-over cache memory when a cgroup is destroyed, which is reparenting. The rough steps will be the same, the object lifetime will be the same, the css refcounting will be the same, the user-visible behavior will be the same. Any complexity from charge vs. reparent races will be contained to a few lines of code.

Weird refcounting tricks during offline, trashing kmem caches instead of moving them to the parent like other memory, a global list of dead memcgs and sudden freelist thrashing on a vmpressure event, that's what adds complexity and what makes this code unpredictable, fragile, and insanely hard to work with. It's not acceptable.

Quoted 9 lines

By reparenting I meant reassigning the memcg cache parameter pointer from the slab cache such that it points to the parent. This should be an atomic operation. All css lookups already require RCU (I think slab does not follow this yet because we guarantee that css reference, but it should be changed). So switch the cache param pointer, insert an RCU graceperiod to wait for all the ongoing charges and uncharges until nobody sees the memcg anymore, then safely reparent all the remaining memcg objects to the parent. Maybe individually, maybe we can just splice the lists to the parent's list_lru lists.

But what should we do with objects that do not reside on any list_lru? How can we reparent them?

Let's forget about per-memcg list_lru for a while, because implementation of kmem reparenting requires rework of the existing code handling per-memcg kmem caches. I would really appreciate if you could share your vision on how we should get rid of per-memcg caches on memcg destruction. Currently they are left hanging around until all memcg's objects are freed. Should we try to move individual slab pages to the parent's cache and destroy the memcg's cache? Or perhaps you mean that all accounted kmem objects should be tracked in memcg by some means (list_lru in case dcache/icache)?

Please excuse me if I annoy you, but before trying to change anything I'd like to ensure I understand you right.

Thank you.

Quoted 42 lines

I'm not sure I understand how the dangling cache problem pertains to this, isn't this an entirely separate issue?

Quoted 8 lines
Quoted 2 lines

Charged thread stack pages could be reassigned when the task itself is migrated out of a cgroup.

Thread info pages are only a part of the problem. If a process kmalloc's an object of size >= KMALLOC_MAX_CACHE_SIZE, it will be given a compound page accounted to kmemcg, and we won't be able to find this page given the memcg it is accounted to (except for walking the whole page range). Thus we will have to organize those pages in per-memcg lists, won't we? Again, even more complexity.

Why do we track them in the first place? We don't track any random page allocation, so we shouldn't track kmalloc() that falls back to the page allocator. In fact we shouldn't track any random slab allocation.

The types of allocations we primarily want to track are the ones that directly scale with user behavior. This is a finite set, which on a global level is covered mostly by ulimits. After all, an unprivileged user interfering with other users is not a new problem and existed long before memcg.

It was a mistake to provide __GFP_KMEMCG and allow charging any random allocation, without giving memcg the means to actually manage that memory. I don't see that such flexibility even needed, and it clearly hurts us now. It was a choice we made to keep things simple in the beginning, before we knew how all this is going to turn out. We should rectify this mistake before building on top of it.

Here is the much bigger issue behind this:

Memcg should be a thin layer of accounting and limiting between the VM and cgroup core code, but look at the line count. It's more code than all of the page reclaim logic combined, including the page replacement algorithm, rmap, LRU list handling - all of which already include a good deal of memcg specifics. It's the same size as the scheduler core, which includes the entire cpu cgroup controller. And all this other code is of better quality and has more eyes on it than memcg.

Please demonstrate that you try to see the bigger picture behind memcg and make an effort to keep things simple beyond the code you introduce and the niche you care about, otherwise I'm not willing to take any patches from you that don't straight-up delete stuff.

maintainer
#20 lore raw
Quoted 109 lines · Vladimir Davydov
Quoted 98 lines · Johannes Weiner
Quoted 39 lines · Vladimir Davydov
Quoted 38 lines · Johannes Weiner
Quoted 4 lines · Vladimir Davydov

Hi Michal, Johannes, David,

Could you please take a look at this if you have time? Without your review, it'll never get committed.

There is simply no review bandwidth for new features as long as we are fixing fundamental bugs in memcg.

Quoted 17 lines · Vladimir Davydov
Quoted 16 lines

Hi,

This is the 15th iteration of Glauber Costa's patch-set implementing slab shrinking on memcg pressure. The main idea is to make the list_lru structure used by most FS shrinkers per-memcg. When adding or removing an element from a list_lru, we use the page information to figure out which memcg it belongs to and relay it to the appropriate list. This allows scanning kmem objects accounted to different memcgs independently.

Please note that this patch-set implements slab shrinking only when we hit the user memory limit so that kmem allocations will still fail if we are below the user memory limit, but close to the kmem limit. I am going to fix this in a separate patch-set, but currently it is only worthwhile setting the kmem limit to be greater than the user mem limit just to enable per-memcg slab accounting and reclaim.

Quoted 9 lines
Quoted 9 lines

The patch-set is based on top of v3.14-rc1-mmots-2014-02-04-16-48 (there are some vmscan cleanups that I need committed there) and organized as follows:
- patches 1-4 introduce some minor changes to memcg needed for this set;
- patches 5-7 prepare fs for per-memcg list_lru;
- patch 8 implement kmemcg reclaim core;
- patch 9 make list_lru per-memcg and patch 10 marks sb shrinker memcg-aware;
- patch 10 is trivial - it issues shrinkers on memcg destruction;
- patches 12 and 13 introduce shrinking of dead kmem caches to facilitate memcg destruction.

In the context of the ongoing discussions about charge reparenting I was curious how you deal with charges becoming unreclaimable after a memcg has been offlined.

Quoted 8 lines
Quoted 4 lines

Patch #11 drops all charged objects at offlining by just invoking shrink_slab() in a loop until "only a few" (10) objects are remaining. How long is this going to take? And why is it okay to destroy these caches when somebody else might still be using them?

IMHO, on container destruction we have to drop as many objects accounted to this container as we can, because otherwise any container will be able to get access to any number of unaccounted objects by fetching them and then rebooting.

They're accounted to and subject to the limit of the parent. I don't see how this is different than page cache.

Quoted 26 lines
Quoted 9 lines

That still leaves you with the free objects that slab caches retain for allocation efficiency, so now you put all dead memcgs in the system on a global list, and on a vmpressure event on root_mem_cgroup you walk the global list and drain the freelist of all remaining caches.

This is a lot of complexity and scalability problems for less than desirable behavior.

Quoted 8 lines

Please think about how we can properly reparent kmemcg charges during memcg teardown. That would simplify your code immensely and help clean up this unholy mess of css pinning.

Slab caches are already collected in the memcg and on destruction could be reassigned to the parent. Kmemcg uncharge from slab freeing would have to be updated to use the memcg from the cache, not from the individual page, but I don't see why this wouldn't work right now.

I don't think I understand what you mean by reassigning slab caches to the parent.

If you mean moving all pages (slabs) from the cache of the memcg being destroyed to the corresponding root cache (or the parent memcg's cache) and then destroying the memcg's cache, I don't think this is feasible, because slub free's fast path is lockless, so AFAIU we can't remove a partial slab from a cache w/o risking to race with kmem_cache_free.

Quoted 8 lines

If you mean clearing all pointers from the memcg's cache to the memcg (changing them to the parent or root memcg), then AFAIU this won't solve the problem with "dangling" caches - we will still have to shrink them on vmpressure. So although this would allow us to put the reference to the memcg from kmem caches on memcg's death, it wouldn't simplify the code at all, in fact, it would even make it more complicated, because we would have to handle various corner cases like reparenting vs list_lru_{add,remove}.

I think we have different concepts of what's complicated. There is an existing model of what to do with left-over cache memory when a cgroup is destroyed, which is reparenting. The rough steps will be the same, the object lifetime will be the same, the css refcounting will be the same, the user-visible behavior will be the same. Any complexity from charge vs. reparent races will be contained to a few lines of code.

Weird refcounting tricks during offline, trashing kmem caches instead of moving them to the parent like other memory, a global list of dead memcgs and sudden freelist thrashing on a vmpressure event, that's what adds complexity and what makes this code unpredictable, fragile, and insanely hard to work with. It's not acceptable.

Quoted 9 lines

By reparenting I meant reassigning the memcg cache parameter pointer from the slab cache such that it points to the parent. This should be an atomic operation. All css lookups already require RCU (I think slab does not follow this yet because we guarantee that css reference, but it should be changed). So switch the cache param pointer, insert an RCU graceperiod to wait for all the ongoing charges and uncharges until nobody sees the memcg anymore, then safely reparent all the remaining memcg objects to the parent. Maybe individually, maybe we can just splice the lists to the parent's list_lru lists.

Quoted 2 lines

But what should we do with objects that do not reside on any list_lru? How can we reparent them?

If there are no actual list_lru objects, we only have to make sure that any allocations are properly uncharged against the parent when they get freed later.

If the slab freeing path would uncharge against the per-memcg cache's backpointer (s->memcg_params->memcg) instead of the per-page memcg association, then we could reparent whole caches with a single pointer update, without touching each individual slab page. The kmemcg interface for slab would have to be reworked to not use lookup_page_cgroup() but have slab pass s->memcg_params->memcg.

Once that is in place, css_free() can walk memcg->memcg_slab_caches and move all the items to the parent's memcg_slab_caches, and while doing that change the memcg pointer of each item, memcg_params->memcg, to point to the parent. The cache is now owned by the parent and will stay alive until the last page is freed.

There won't be any new allocations in these caches because there are no tasks in the group anymore, so no races from that side, and the perfect time to shrink the freelists.

There will be racing frees of outstanding allocations, but we can deal with that. Frees use page->slab_cache to look up the proper per-memcg cache, which may or may not have been reparented at this time. If it has not, the cache's memcg backpointer (s->memcg_params->memcg) will point to the dying child (rcu protected) and uncharge the child's res_counter and the parent's. If the backpointer IS already pointing to the parent, it will uncharge the res_counter of the parent without the child's but we don't care, it's dying anyway.

If there are no list_lru tracked objects, we are done at this point. The css can be freed, the freelists have been purged, and any pages still in the cache will get unaccounted properly from the parent.

If there are list_lru objects, they have to be moved to the parent's list_lru so that they can be reclaimed properly on memory pressure.

Does this make sense?

#21 lore raw
Quoted 141 lines · Johannes Weiner
Quoted 108 lines · Vladimir Davydov
Quoted 98 lines · Johannes Weiner
Quoted 39 lines · Vladimir Davydov
Quoted 38 lines · Johannes Weiner
Quoted 4 lines · Vladimir Davydov

Hi Michal, Johannes, David,

Could you please take a look at this if you have time? Without your review, it'll never get committed.

There is simply no review bandwidth for new features as long as we are fixing fundamental bugs in memcg.

Quoted 17 lines · Vladimir Davydov
Quoted 16 lines

Hi,

This is the 15th iteration of Glauber Costa's patch-set implementing slab shrinking on memcg pressure. The main idea is to make the list_lru structure used by most FS shrinkers per-memcg. When adding or removing an element from a list_lru, we use the page information to figure out which memcg it belongs to and relay it to the appropriate list. This allows scanning kmem objects accounted to different memcgs independently.

Please note that this patch-set implements slab shrinking only when we hit the user memory limit so that kmem allocations will still fail if we are below the user memory limit, but close to the kmem limit. I am going to fix this in a separate patch-set, but currently it is only worthwhile setting the kmem limit to be greater than the user mem limit just to enable per-memcg slab accounting and reclaim.

Quoted 9 lines
Quoted 9 lines

The patch-set is based on top of v3.14-rc1-mmots-2014-02-04-16-48 (there are some vmscan cleanups that I need committed there) and organized as follows:
- patches 1-4 introduce some minor changes to memcg needed for this set;
- patches 5-7 prepare fs for per-memcg list_lru;
- patch 8 implement kmemcg reclaim core;
- patch 9 make list_lru per-memcg and patch 10 marks sb shrinker memcg-aware;
- patch 10 is trivial - it issues shrinkers on memcg destruction;
- patches 12 and 13 introduce shrinking of dead kmem caches to facilitate memcg destruction.

In the context of the ongoing discussions about charge reparenting I was curious how you deal with charges becoming unreclaimable after a memcg has been offlined.

Quoted 8 lines
Quoted 4 lines

Patch #11 drops all charged objects at offlining by just invoking shrink_slab() in a loop until "only a few" (10) objects are remaining. How long is this going to take? And why is it okay to destroy these caches when somebody else might still be using them?

IMHO, on container destruction we have to drop as many objects accounted to this container as we can, because otherwise any container will be able to get access to any number of unaccounted objects by fetching them and then rebooting.

They're accounted to and subject to the limit of the parent. I don't see how this is different than page cache.

Quoted 26 lines
Quoted 9 lines

That still leaves you with the free objects that slab caches retain for allocation efficiency, so now you put all dead memcgs in the system on a global list, and on a vmpressure event on root_mem_cgroup you walk the global list and drain the freelist of all remaining caches.

This is a lot of complexity and scalability problems for less than desirable behavior.

Quoted 8 lines

Please think about how we can properly reparent kmemcg charges during memcg teardown. That would simplify your code immensely and help clean up this unholy mess of css pinning.

Slab caches are already collected in the memcg and on destruction could be reassigned to the parent. Kmemcg uncharge from slab freeing would have to be updated to use the memcg from the cache, not from the individual page, but I don't see why this wouldn't work right now.

I don't think I understand what you mean by reassigning slab caches to the parent.

If you mean moving all pages (slabs) from the cache of the memcg being destroyed to the corresponding root cache (or the parent memcg's cache) and then destroying the memcg's cache, I don't think this is feasible, because slub free's fast path is lockless, so AFAIU we can't remove a partial slab from a cache w/o risking to race with kmem_cache_free.

Quoted 8 lines

If you mean clearing all pointers from the memcg's cache to the memcg (changing them to the parent or root memcg), then AFAIU this won't solve the problem with "dangling" caches - we will still have to shrink them on vmpressure. So although this would allow us to put the reference to the memcg from kmem caches on memcg's death, it wouldn't simplify the code at all, in fact, it would even make it more complicated, because we would have to handle various corner cases like reparenting vs list_lru_{add,remove}.

I think we have different concepts of what's complicated. There is an existing model of what to do with left-over cache memory when a cgroup is destroyed, which is reparenting. The rough steps will be the same, the object lifetime will be the same, the css refcounting will be the same, the user-visible behavior will be the same. Any complexity from charge vs. reparent races will be contained to a few lines of code.

Weird refcounting tricks during offline, trashing kmem caches instead of moving them to the parent like other memory, a global list of dead memcgs and sudden freelist thrashing on a vmpressure event, that's what adds complexity and what makes this code unpredictable, fragile, and insanely hard to work with. It's not acceptable.

Quoted 9 lines

By reparenting I meant reassigning the memcg cache parameter pointer from the slab cache such that it points to the parent. This should be an atomic operation. All css lookups already require RCU (I think slab does not follow this yet because we guarantee that css reference, but it should be changed). So switch the cache param pointer, insert an RCU graceperiod to wait for all the ongoing charges and uncharges until nobody sees the memcg anymore, then safely reparent all the remaining memcg objects to the parent. Maybe individually, maybe we can just splice the lists to the parent's list_lru lists.

Quoted 2 lines

But what should we do with objects that do not reside on any list_lru? How can we reparent them?

If there are no actual list_lru objects, we only have to make sure that any allocations are properly uncharged against the parent when they get freed later.

If the slab freeing path would uncharge against the per-memcg cache's backpointer (s->memcg_params->memcg) instead of the per-page memcg association, then we could reparent whole caches with a single pointer update, without touching each individual slab page. The kmemcg interface for slab would have to be reworked to not use lookup_page_cgroup() but have slab pass s->memcg_params->memcg.

Once that is in place, css_free() can walk memcg->memcg_slab_caches and move all the items to the parent's memcg_slab_caches, and while doing that change the memcg pointer of each item, memcg_params->memcg, to point to the parent. The cache is now owned by the parent and will stay alive until the last page is freed.

There won't be any new allocations in these caches because there are no tasks in the group anymore, so no races from that side, and the perfect time to shrink the freelists.

There will be racing frees of outstanding allocations, but we can deal with that. Frees use page->slab_cache to look up the proper per-memcg cache, which may or may not have been reparented at this time. If it has not, the cache's memcg backpointer (s->memcg_params->memcg) will point to the dying child (rcu protected) and uncharge the child's res_counter and the parent's. If the backpointer IS already pointing to the parent, it will uncharge the res_counter of the parent without the child's but we don't care, it's dying anyway.

Quoted 8 lines

If there are no list_lru tracked objects, we are done at this point. The css can be freed, the freelists have been purged, and any pages still in the cache will get unaccounted properly from the parent.

If there are list_lru objects, they have to be moved to the parent's list_lru so that they can be reclaimed properly on memory pressure.

Does this make sense?

Definitely. Thank you very much for such a detailed explanation! I guess I'll try to implement this.