kmemcg shrinkers

I need to move this up a bit, and I am doing in a separate patch just to reduce churn in the patch that needs it.

Signed-off-by Vladimir Davydov <[email protected]>

This reduces the indentation level of do_try_to_free_pages() and removes extra loop over all eligible zones counting the number of on-LRU pages.

To enable targeted reclaim, the list_lru structure distributes its elements among several LRU lists. Currently, there is one LRU per NUMA node, and the elements from different nodes are placed to different LRUs. As a result there are two versions of count and walk functions:

- list_lru_count, list_lru_walk - count, walk items from all nodes;
- list_lru_count_node, list_lru_walk_node - count, walk items from a particular node specified in an additional argument.

We are going to make the list_lru structure per-memcg in addition to being per-node. This would allow us to reclaim slab not only on global memory shortage, but also on memcg pressure. If we followed the current list_lru interface notation, we would have to add a bunch of new functions taking a memcg and a node in additional arguments, which would look cumbersome.

To avoid this, we remove the *_node functions and make list_lru_count and list_lru_walk require a shrink_control argument so that they will

fs/dcache.c +2 -4

@@ -972,8 +972,8 @@ 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_walk()
+ * @nr_to_scan: number of entries to try to free
  *
  * 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

@@ -982,14 +982,14 @@ dentry_lru_isolate(struct list_head *item, spinlock_t *lru_lock, void *arg)
  * 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)

fs/xfs/xfs_buf.c +5 -3

index c7f0b77..5b2a49c 100644
@@ -1508,9 +1508,11 @@ xfs_wait_buftarg(
 	int loop = 0;
 
 	/* loop until there is nothing left on the lru list. */
-	while (list_lru_count(&btp->bt_lru)) {
-		list_lru_walk(&btp->bt_lru, xfs_buftarg_wait_rele,
-			      &dispose, LONG_MAX);
+	while (list_lru_count(&btp->bt_lru, NULL)) {
+		unsigned long nr_to_scan = ULONG_MAX;
+
+		list_lru_walk(&btp->bt_lru, NULL, xfs_buftarg_wait_rele,
+			      &dispose, &nr_to_scan);
 
 		while (!list_empty(&dispose)) {
 			struct xfs_buf *bp;

fs/xfs/xfs_buf.c +1 -1

 	while (!list_empty(&dispose)) {
 		struct xfs_buf *bp;

@@ -1585,7 +1587,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_count(&btp->bt_lru, sc);
 }
 
 void

fs/xfs/xfs_qm.c +0 -0

index 14a4996..aaacf8f 100644

include/linux/list_lru.h +4 -1

-unsigned long
+unsigned long list_lru_count(struct list_lru *lru, struct shrink_control *sc)
+{
+	long count = 0;
+	int nid;

include/linux/list_lru.h +13 -1

+
+static 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)
 {

@@ -112,7 +127,27 @@ restart:
 	spin_unlock(&nlru->lock);
 	return isolated;
 }
-EXPORT_SYMBOL_GPL(list_lru_walk_node);
+
+unsigned long list_lru_walk(struct list_lru *lru, struct shrink_control *sc,
+			    list_lru_walk_cb isolate, void *cb_arg,
+			    unsigned long *nr_to_walk)
+{
+	long isolated = 0;
+	int nid;
+
+	if (sc)
+		return list_lru_walk_node(lru, sc->nid, isolate,
+					  cb_arg, nr_to_walk);

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.

Signed-off-by Vladimir Davydov <[email protected]>

Cc Glauber Costa <[email protected]>

Cc Dave Chinner <[email protected]>

Cc Al Viro <viro-RmSDqhL/[email protected]>

---
 fs/super.c         |    8 +++-----
 fs/xfs/xfs_super.c |    6 +++---
 include/linux/fs.h |    6 ++++--
 3 files changed, 10 insertions(+), 10 deletions(-)

fs/super.c +1 -1

index a039dba..8f9a81b 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_count(&sb->s_inode_lru, sc);
 	dentries = list_lru_count(&sb->s_dentry_lru, sc);

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.

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

mm/vmscan.c +40 -0

@@ -311,6 +311,58 @@ shrink_slab_node(struct shrink_control *shrinkctl, struct shrinker *shrinker,
 	return freed;
 }
 
+static unsigned long
+run_shrinker(struct shrink_control *shrinkctl, struct shrinker *shrinker,
+	     unsigned long nr_pages_scanned, unsigned long lru_pages)
+{
+	unsigned long freed = 0;
+
+	/*
+	 * If we don't have a target mem cgroup, we scan them all. Otherwise
+	 * we will limit our scan to shrinkers marked as memcg aware.
+	 */
+	if (!(shrinker->flags & SHRINKER_MEMCG_AWARE) &&
+	    shrinkctl->target_mem_cgroup != NULL)
+		return 0;
+	/*
+	 * In a hierarchical chain, it might be that not all memcgs are kmem
+	 * active. kmemcg design mandates that when one memcg is active, its
+	 * children will be active as well. But it is perfectly possible that
+	 * its parent is not.
+	 *
+	 * We also need to make sure we scan at least once, for the global
+	 * case. So if we don't have a target memcg, we proceed normally and
+	 * expect to break in the next round.
+	 */
+	shrinkctl->memcg = shrinkctl->target_mem_cgroup;
+	do {
+		if (shrinkctl->memcg && !memcg_kmem_is_active(shrinkctl->memcg))
+			goto next;
+
+		if (!(shrinker->flags & SHRINKER_NUMA_AWARE)) {
+			shrinkctl->nid = 0;
+			freed += shrink_slab_node(shrinkctl, shrinker,
+					nr_pages_scanned, lru_pages);
+			goto next;
+		}
+
+		for_each_node_mask(shrinkctl->nid, shrinkctl->nodes_to_scan) {
+			if (node_online(shrinkctl->nid))
+				freed += shrink_slab_node(shrinkctl, shrinker,
+						nr_pages_scanned, lru_pages);
+

mm/vmscan.c +1 -3

+
 /*
  * Call the shrink functions to age shrinkable caches
  *

@@ -352,20 +404,10 @@ unsigned long shrink_slab(struct shrink_control *shrinkctl,
 	}
 
 	list_for_each_entry(shrinker, &shrinker_list, list) {
-		if (!(shrinker->flags & SHRINKER_NUMA_AWARE)) {
-			shrinkctl->nid = 0;
-			freed += shrink_slab_node(shrinkctl, shrinker,

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.

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

Signed-off-by Vladimir Davydov <[email protected]>

Cc Glauber Costa <[email protected]>

Cc Dave Chinner <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Andrew Morton <akpm-de/[email protected]>

Cc Al Viro <viro-RmSDqhL/[email protected]>

Cc Balbir Singh <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

---
 include/linux/list_lru.h   |   44 +++++++-
 include/linux/memcontrol.h |   13 +++
 mm/list_lru.c              |  242 ++++++++++++++++++++++++++++++++++++++------
 mm/memcontrol.c            |  158 ++++++++++++++++++++++++++++-
 4 files changed, 416 insertions(+), 41 deletions(-)

include/linux/list_lru.h +5 -1

index 34e57af..e8add3d 100644
@@ -28,11 +28,47 @@ struct list_lru_node {
 	long			nr_items;
 } ____cacheline_aligned_in_smp;
 
+struct list_lru_one {
+	struct list_lru_node *node;
+	nodemask_t active_nodes;
+};
+
 struct list_lru {
-	struct list_lru_node	*node;

include/linux/list_lru.h +8 -0

+int list_lru_init(struct list_lru *lru)
+{
+	int err;
+
+	err = list_lru_init_one(&lru->global);
+	if (err)
+		goto fail;
+

include/linux/list_lru.h +1 -1

 EXPORT_SYMBOL_GPL(list_lru_init);
 
 void list_lru_destroy(struct list_lru *lru)
 {
-	kfree(lru->node);
+	/*

include/linux/list_lru.h +44 -0

+	if (!lru->global.node)
+		return;
+
+	list_lru_destroy_one(&lru->global);
+	memcg_list_lru_destroy(lru);
 }
 EXPORT_SYMBOL_GPL(list_lru_destroy);
+
+#ifdef CONFIG_MEMCG_KMEM
+int list_lru_memcg_alloc(struct list_lru *lru, int memcg_id)
+{
+	int err;
+	struct list_lru_one *olru;
+
+	olru = kmalloc(sizeof(*olru), GFP_KERNEL);
+	if (!olru)
+		return -ENOMEM;
+
+	err = list_lru_init_one(olru);
+	if (err) {
+		kfree(olru);
+		return err;
+	}
+
+	VM_BUG_ON(lru->memcg[memcg_id]);
+	lru->memcg[memcg_id] = olru;
+	return 0;
+}
+
+void list_lru_memcg_free(struct list_lru *lru, int memcg_id)
+{
+	struct list_lru_one *olru;
+
+	olru = lru->memcg[memcg_id];
+	if (olru) {
+		list_lru_destroy_one(olru);
+		kfree(olru);
+		lru->memcg[memcg_id] = NULL;
+	}
+}
+
+int list_lru_grow_memcg(struct list_lru *lru, size_t new_array_size)
+{
+	int i;
+	struct list_lru_one **memcg_lrus;
+

include/linux/list_lru.h +27 -0

+/*
+ * This function allocates LRUs for a memcg in all list_lru structures. It is
+ * called under memcg_create_mutex when a new kmem-active memcg is added.
+ */
+static int memcg_init_all_lrus(int new_memcg_id)
+{
+	int err = 0;
+	int num_memcgs = new_memcg_id + 1;
+	int grow = (num_memcgs > memcg_limited_groups_array_size);
+	size_t new_array_size = memcg_caches_array_size(num_memcgs);
+	struct list_lru *lru;
+
+	if (grow) {
+		list_for_each_entry(lru, &all_memcg_lrus, list) {
+			err = list_lru_grow_memcg(lru, new_array_size);
+			if (err)
+				goto out;
+		}
+	}
+
+	list_for_each_entry(lru, &all_memcg_lrus, list) {
+		err = list_lru_memcg_alloc(lru, new_memcg_id);
+		if (err) {
+			__memcg_destroy_all_lrus(new_memcg_id);
+			break;
+		}
+	}

include/linux/list_lru.h +36 -0

+
+int memcg_list_lru_init(struct list_lru *lru)
+{
+	int err = 0;
+	int i;
+	struct mem_cgroup *memcg;
+
+	lru->memcg = NULL;
+	lru->memcg_old = NULL;
+
+	mutex_lock(&memcg_create_mutex);
+	if (!memcg_kmem_enabled())
+		goto out_list_add;
+
+	lru->memcg = kcalloc(memcg_limited_groups_array_size,
+			     sizeof(*lru->memcg), GFP_KERNEL);
+	if (!lru->memcg) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	for_each_mem_cgroup(memcg) {
+		int memcg_id;
+
+		memcg_id = memcg_cache_id(memcg);
+		if (memcg_id < 0)
+			continue;
+
+		err = list_lru_memcg_alloc(lru, memcg_id);
+		if (err) {
+			mem_cgroup_iter_break(NULL, memcg);
+			goto out_free_lru_memcg;
+		}
+	}
+out_list_add:
+	list_add(&lru->list, &all_memcg_lrus);

Quoted 19 lines · Vladimir Davydov

include/linux/list_lru.h +8 -2

index 34e57af..e8add3d 100644
@@ -28,11 +28,47 @@ struct list_lru_node {
 	long			nr_items;
 } ____cacheline_aligned_in_smp;
 
+struct list_lru_one {
+	struct list_lru_node *node;
+	nodemask_t active_nodes;
+};
+
 struct list_lru {
-	struct list_lru_node	*node;
-	nodemask_t		active_nodes;
+	struct list_lru_one	global;
+#ifdef CONFIG_MEMCG_KMEM
+	/*

include/linux/list_lru.h struct list_lru_node { · line 45

Quoted 6 lines

include/linux/list_lru.h +6 -0

+	 * 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_one	**memcg;

OK, as far as I can tell, this is introducing a per-node, per-memcg LRU lists. Is that correct?

If so, then that is not what Glauber and I originally intended for memcg LRUs. per-node LRUs are expensive in terms of memory and cross multiplying them by the number of memcgs in a system was not a good use of memory.

According to Glauber, most memcgs are small and typically confined to a single node or two by external means and therefore don't need the scalability numa aware LRUs provide. Hence the idea was that the memcg LRUs would just be a single LRU list, just like a non-numa aware list_lru instantiation. IOWs, this is the structure that we had decided on as the best compromise between memory usage, complexity and memcg awareness:

global list --- node 0 lru
node 1 lru
.....
node nr_nodes lru
memcg lists memcg 0 lru
memcg 1 lru
.....
memcg nr_memcgs lru

and the LRU code internally would select either a node or memcg LRU to iterated based on the scan information coming in from the shrinker. i.e.:

struct list_lru {
	struct list_lru_node	*node;
	nodemask_t		active_nodes;
#ifdef MEMCG
	struct list_lru_node	**memcg;
	....

include/linux/list_lru.h struct list_lru_node { · line 51

Quoted 8 lines

include/linux/list_lru.h +4 -2

 bool list_lru_add(struct list_lru *lru, struct list_head *item)
 {
-	int nid = page_to_nid(virt_to_page(item));
-	struct list_lru_node *nlru = &lru->node[nid];
+	struct page *page = virt_to_page(item);
+	int nid = page_to_nid(page);
+	struct list_lru_one *olru = lru_of_page(lru, page);
+	struct list_lru_node *nlru = &olru->node[nid];

Yeah, that's per-memcg, per-node dereferencing. And, FWIW, olru/nlru are bad names - that's the convention typically used for "old <foo>" and "new <foo>" pointers....

As it is, it shouldn't be necessary - lru_of_page() should just return a struct list_lru_node....

Quoted 15 lines

include/linux/list_lru.h +15 -0

+int list_lru_init(struct list_lru *lru)
+{
+	int err;
+
+	err = list_lru_init_one(&lru->global);
+	if (err)
+		goto fail;
+
+	err = memcg_list_lru_init(lru);
+	if (err)
+		goto fail;
+
+	return 0;
+fail:
+	list_lru_destroy_one(&lru->global);

include/linux/list_lru.h struct list_lru_node { · line 69

Quoted 3 lines

include/linux/list_lru.h +3 -0

+	lru->global.node = NULL; /* see list_lru_destroy() */
+	return err;
+}

I suspect we have users of list_lru that don't want memcg bits added to them. Hence I think we want to leave list_lru_init() alone and add a list_lru_init_memcg() variant that makes the LRU memcg aware. i.e. if the shrinker is not going to be memcg aware, then we don't want the LRU to be memcg aware, either....

Quoted 8 lines

include/linux/list_lru.h +8 -0

+int list_lru_grow_memcg(struct list_lru *lru, size_t new_array_size)
+{
+	int i;
+	struct list_lru_one **memcg_lrus;
+
+	memcg_lrus = kcalloc(new_array_size, sizeof(*memcg_lrus), GFP_KERNEL);
+	if (!memcg_lrus)
+		return -ENOMEM;

include/linux/list_lru.h

Quoted 7 lines

include/linux/list_lru.h +7 -0

+
+	if (lru->memcg) {
+		for_each_memcg_cache_index(i) {
+			if (lru->memcg[i])
+				memcg_lrus[i] = lru->memcg[i];
+		}
+	}

Um, krealloc()?

Quoted 27 lines

include/linux/list_lru.h +27 -0

+/*
+ * This function allocates LRUs for a memcg in all list_lru structures. It is
+ * called under memcg_create_mutex when a new kmem-active memcg is added.
+ */
+static int memcg_init_all_lrus(int new_memcg_id)
+{
+	int err = 0;
+	int num_memcgs = new_memcg_id + 1;
+	int grow = (num_memcgs > memcg_limited_groups_array_size);
+	size_t new_array_size = memcg_caches_array_size(num_memcgs);
+	struct list_lru *lru;
+
+	if (grow) {
+		list_for_each_entry(lru, &all_memcg_lrus, list) {
+			err = list_lru_grow_memcg(lru, new_array_size);
+			if (err)
+				goto out;
+		}
+	}
+
+	list_for_each_entry(lru, &all_memcg_lrus, list) {
+		err = list_lru_memcg_alloc(lru, new_memcg_id);
+		if (err) {
+			__memcg_destroy_all_lrus(new_memcg_id);
+			break;
+		}
+	}

include/linux/list_lru.h

Quoted 10 lines

include/linux/list_lru.h +10 -0

+out:
+	if (grow) {
+		synchronize_rcu();
+		list_for_each_entry(lru, &all_memcg_lrus, list) {
+			kfree(lru->memcg_old);
+			lru->memcg_old = NULL;
+		}
+	}
+	return err;
+}

Urk. That won't scale very well.

Quoted 42 lines

include/linux/list_lru.h +42 -0

+
+int memcg_list_lru_init(struct list_lru *lru)
+{
+	int err = 0;
+	int i;
+	struct mem_cgroup *memcg;
+
+	lru->memcg = NULL;
+	lru->memcg_old = NULL;
+
+	mutex_lock(&memcg_create_mutex);
+	if (!memcg_kmem_enabled())
+		goto out_list_add;
+
+	lru->memcg = kcalloc(memcg_limited_groups_array_size,
+			     sizeof(*lru->memcg), GFP_KERNEL);
+	if (!lru->memcg) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	for_each_mem_cgroup(memcg) {
+		int memcg_id;
+
+		memcg_id = memcg_cache_id(memcg);
+		if (memcg_id < 0)
+			continue;
+
+		err = list_lru_memcg_alloc(lru, memcg_id);
+		if (err) {
+			mem_cgroup_iter_break(NULL, memcg);
+			goto out_free_lru_memcg;
+		}
+	}
+out_list_add:
+	list_add(&lru->list, &all_memcg_lrus);
+out:
+	mutex_unlock(&memcg_create_mutex);
+	return err;
+
+out_free_lru_memcg:
+	for (i = 0; i < memcg_limited_groups_array_size; i++)

include/linux/list_lru.h

Quoted 4 lines

include/linux/list_lru.h +4 -0

+		list_lru_memcg_free(lru, i);
+	kfree(lru->memcg);
+	goto out;
+}

That will probably scale even worse. Think about what happens when we try to mount a bunch of filesystems in parallel - they will now serialise completely on this memcg_create_mutex instantiating memcg lists inside list_lru_init().

Hi, David

First of all, let me thank you for such a thorough review. It is really helpful. As usual, I can't help agreeing with most of your comments, but there are a couple of things I'd like to clarify. Please, see comments inline.

I agree that such a setup would not only reduce memory consumption, but also make the code look much clearer removing these ugly "list_lru_one" and "olru" I had to introduce. However, it would also make us scan memcg LRUs more aggressively than usual NUMA-aware LRUs on global pressure (I mean kswapd's would scan them on each node). I don't think it's much of concern though, because this is what we had for all shrinkers before NUMA-awareness was introduced. Besides, prioritizing memcg LRUs reclaim over global LRUs sounds sane. That said I like this idea. Thanks.

Quoted 15 lines

Quoted 15 lines

mm/list_lru.c +15 -0

+int list_lru_init(struct list_lru *lru)
+{
+	int err;
+
+	err = list_lru_init_one(&lru->global);
+	if (err)
+		goto fail;
+
+	err = memcg_list_lru_init(lru);
+	if (err)
+		goto fail;
+
+	return 0;
+fail:
+	list_lru_destroy_one(&lru->global);

Quoted 8 lines

mm/list_lru.c unsigned long list_lru_walk(struct list_lru *lru, struct shrink_control *sc, · line 275

Quoted 3 lines

mm/list_lru.c +3 -0

+	lru->global.node = NULL; /* see list_lru_destroy() */
+	return err;
+}

I suspect we have users of list_lru that don't want memcg bits added to them. Hence I think we want to leave list_lru_init() alone and add a list_lru_init_memcg() variant that makes the LRU memcg aware. i.e. if the shrinker is not going to be memcg aware, then we don't want the LRU to be memcg aware, either....

I though that we want to make all LRUs per-memcg automatically, just like it was with NUMA awareness. After your explanation about some FS-specific caches (gfs2/xfs dquot), I admit I was wrong, and not all caches require per-memcg shrinking. I'll add a flag to list_lru_init() specifying if we want memcg awareness.

Quoted 8 lines

Quoted 8 lines

mm/list_lru.c +8 -0

+int list_lru_grow_memcg(struct list_lru *lru, size_t new_array_size)
+{
+	int i;
+	struct list_lru_one **memcg_lrus;
+
+	memcg_lrus = kcalloc(new_array_size, sizeof(*memcg_lrus), GFP_KERNEL);
+	if (!memcg_lrus)
+		return -ENOMEM;

Quoted 8 lines

mm/list_lru.c unsigned long list_lru_walk(struct list_lru *lru, struct shrink_control *sc, · line 346

Quoted 7 lines

mm/list_lru.c +7 -0

+
+	if (lru->memcg) {
+		for_each_memcg_cache_index(i) {
+			if (lru->memcg[i])
+				memcg_lrus[i] = lru->memcg[i];
+		}
+	}

Um, krealloc()?

Not exactly. We have to keep the old version until we call sync_rcu.

Quoted 41 lines

Quoted 41 lines

mm/memcontrol.c +41 -0

+int memcg_list_lru_init(struct list_lru *lru)
+{
+	int err = 0;
+	int i;
+	struct mem_cgroup *memcg;
+
+	lru->memcg = NULL;
+	lru->memcg_old = NULL;
+
+	mutex_lock(&memcg_create_mutex);
+	if (!memcg_kmem_enabled())
+		goto out_list_add;
+
+	lru->memcg = kcalloc(memcg_limited_groups_array_size,
+			     sizeof(*lru->memcg), GFP_KERNEL);
+	if (!lru->memcg) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	for_each_mem_cgroup(memcg) {
+		int memcg_id;
+
+		memcg_id = memcg_cache_id(memcg);
+		if (memcg_id < 0)
+			continue;
+
+		err = list_lru_memcg_alloc(lru, memcg_id);
+		if (err) {
+			mem_cgroup_iter_break(NULL, memcg);
+			goto out_free_lru_memcg;
+		}
+	}
+out_list_add:
+	list_add(&lru->list, &all_memcg_lrus);
+out:
+	mutex_unlock(&memcg_create_mutex);
+	return err;
+
+out_free_lru_memcg:
+	for (i = 0; i < memcg_limited_groups_array_size; i++)

Quoted 8 lines

mm/memcontrol.c static size_t memcg_caches_array_size(int num_groups) · line 3150

Quoted 4 lines

mm/memcontrol.c +4 -0

+		list_lru_memcg_free(lru, i);
+	kfree(lru->memcg);
+	goto out;
+}

That will probably scale even worse. Think about what happens when we try to mount a bunch of filesystems in parallel - they will now serialise completely on this memcg_create_mutex instantiating memcg lists inside list_lru_init().

Yes, the scalability seems to be the main problem here. I have a couple of thoughts on how it could be improved. Here they go:

1) We can turn memcg_create_mutex to rw-semaphore (or introduce an rw-semaphore, which we would take for modifying list_lru's) and take it for reading in memcg_list_lru_init() and for writing when we create a new memcg (memcg_init_all_lrus()).
This would remove the bottleneck from the mount path, but every memcg creation would still iterate over all LRUs under a memcg mutex. So I guess it is not an option, isn't it?

2) We could use cmpxchg() instead of a mutex in list_lru_init_memcg() and memcg_init_all_lrus() to assure a memcg LRU is initialized only once. But again, this would not remove iteration over all LRUs from memcg_init_all_lrus().

3) We can try to initialize per-memcg LRUs lazily only when we actually need them, similar to how we now handle per-memcg kmem caches creation. If list_lru_add() cannot find appropriate LRU, it will schedule a background worker for its initialization.

The benefits of this approach are clear: we do not introduce any bottlenecks, and we lower memory consumption in case different memcgs use different mounts exclusively.
However, there is one thing that bothers me. Some objects accounted to a memcg will go into the global LRU, which will postpone actual memcg destruction until global reclaim.

Quoted 16 lines

Quoted 16 lines

Quoted 5 lines

mm/list_lru.c +5 -0

+int list_lru_grow_memcg(struct list_lru *lru, size_t new_array_size)
+{
+	int i;
+	struct list_lru_one **memcg_lrus;
+

mm/list_lru.c unsigned long list_lru_walk(struct list_lru *lru, struct shrink_control *sc, · line 346

Quoted 10 lines

mm/list_lru.c +10 -0

+	memcg_lrus = kcalloc(new_array_size, sizeof(*memcg_lrus), GFP_KERNEL);
+	if (!memcg_lrus)
+		return -ENOMEM;
+
+	if (lru->memcg) {
+		for_each_memcg_cache_index(i) {
+			if (lru->memcg[i])
+				memcg_lrus[i] = lru->memcg[i];
+		}
+	}

Um, krealloc()?

Quoted 1 line

Not exactly. We have to keep the old version until we call sync_rcu.

Ah, of course. Could you add a big comment explaining this so that the next reader doesn't suggest replacing it with krealloc(), too?

Quoted 52 lines

Quoted 41 lines

Quoted 41 lines

mm/memcontrol.c +41 -0

+int memcg_list_lru_init(struct list_lru *lru)
+{
+	int err = 0;
+	int i;
+	struct mem_cgroup *memcg;
+
+	lru->memcg = NULL;
+	lru->memcg_old = NULL;
+
+	mutex_lock(&memcg_create_mutex);
+	if (!memcg_kmem_enabled())
+		goto out_list_add;
+
+	lru->memcg = kcalloc(memcg_limited_groups_array_size,
+			     sizeof(*lru->memcg), GFP_KERNEL);
+	if (!lru->memcg) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	for_each_mem_cgroup(memcg) {
+		int memcg_id;
+
+		memcg_id = memcg_cache_id(memcg);
+		if (memcg_id < 0)
+			continue;
+
+		err = list_lru_memcg_alloc(lru, memcg_id);
+		if (err) {
+			mem_cgroup_iter_break(NULL, memcg);
+			goto out_free_lru_memcg;
+		}
+	}
+out_list_add:
+	list_add(&lru->list, &all_memcg_lrus);
+out:
+	mutex_unlock(&memcg_create_mutex);
+	return err;
+
+out_free_lru_memcg:
+	for (i = 0; i < memcg_limited_groups_array_size; i++)

Quoted 8 lines

mm/memcontrol.c static size_t memcg_caches_array_size(int num_groups) · line 3150

Quoted 4 lines

mm/memcontrol.c +4 -0

+		list_lru_memcg_free(lru, i);
+	kfree(lru->memcg);
+	goto out;
+}

That will probably scale even worse. Think about what happens when we try to mount a bunch of filesystems in parallel - they will now serialise completely on this memcg_create_mutex instantiating memcg lists inside list_lru_init().

Yes, the scalability seems to be the main problem here. I have a couple of thoughts on how it could be improved. Here they go:

Quoted 7 lines

1) We can turn memcg_create_mutex to rw-semaphore (or introduce an rw-semaphore, which we would take for modifying list_lru's) and take it for reading in memcg_list_lru_init() and for writing when we create a new memcg (memcg_init_all_lrus()).
This would remove the bottleneck from the mount path, but every memcg creation would still iterate over all LRUs under a memcg mutex. So I guess it is not an option, isn't it?

Right - it's not so much that there is a mutex to protect the init, it's how long it's held that will be the issue. I mean, we don't need to hold the memcg_create_mutex until we've completely initialised the lru structure and are ready to add it to the all_memcg_lrus list, right?

i.e. restructing it so that you don't need to hold the mutex until you make the LRU list globally visible would solve the problem just as well. if we can iterate the memcgs lists without holding a lock, then we can init the per-memcg lru lists without holding a lock because nobody will access them through the list_lru structure because it's not yet been published.

That keeps the locking simple, and we get scalability because we've reduced the lock's scope to just a few instructures instead of a memcg iteration and a heap of memory allocation....

Quoted 10 lines

2) We could use cmpxchg() instead of a mutex in list_lru_init_memcg() and memcg_init_all_lrus() to assure a memcg LRU is initialized only once. But again, this would not remove iteration over all LRUs from memcg_init_all_lrus().

3) We can try to initialize per-memcg LRUs lazily only when we actually need them, similar to how we now handle per-memcg kmem caches creation. If list_lru_add() cannot find appropriate LRU, it will schedule a background worker for its initialization.

I'd prefer not to add complexity to the list_lru_add() path here. It's frequently called, so it's a code hot path and so we should keep it as simply as possible.

Quoted 6 lines

The benefits of this approach are clear: we do not introduce any bottlenecks, and we lower memory consumption in case different memcgs use different mounts exclusively.
However, there is one thing that bothers me. Some objects accounted to a memcg will go into the global LRU, which will postpone actual memcg destruction until global reclaim.

Yeah, that's messy. best to avoid it by doing the work at list init time, IMO.

Quoted 72 lines

Quoted 52 lines

Quoted 41 lines

Quoted 41 lines

mm/memcontrol.c +41 -0

+int memcg_list_lru_init(struct list_lru *lru)
+{
+	int err = 0;
+	int i;
+	struct mem_cgroup *memcg;
+
+	lru->memcg = NULL;
+	lru->memcg_old = NULL;
+
+	mutex_lock(&memcg_create_mutex);
+	if (!memcg_kmem_enabled())
+		goto out_list_add;
+
+	lru->memcg = kcalloc(memcg_limited_groups_array_size,
+			     sizeof(*lru->memcg), GFP_KERNEL);
+	if (!lru->memcg) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	for_each_mem_cgroup(memcg) {
+		int memcg_id;
+
+		memcg_id = memcg_cache_id(memcg);
+		if (memcg_id < 0)
+			continue;
+
+		err = list_lru_memcg_alloc(lru, memcg_id);
+		if (err) {
+			mem_cgroup_iter_break(NULL, memcg);
+			goto out_free_lru_memcg;
+		}
+	}
+out_list_add:
+	list_add(&lru->list, &all_memcg_lrus);
+out:
+	mutex_unlock(&memcg_create_mutex);
+	return err;
+
+out_free_lru_memcg:
+	for (i = 0; i < memcg_limited_groups_array_size; i++)

Quoted 8 lines

mm/memcontrol.c static size_t memcg_caches_array_size(int num_groups) · line 3150

Quoted 4 lines

mm/memcontrol.c +4 -0

+		list_lru_memcg_free(lru, i);
+	kfree(lru->memcg);
+	goto out;
+}

That will probably scale even worse. Think about what happens when we try to mount a bunch of filesystems in parallel - they will now serialise completely on this memcg_create_mutex instantiating memcg lists inside list_lru_init().

Yes, the scalability seems to be the main problem here. I have a couple of thoughts on how it could be improved. Here they go:

Quoted 7 lines

1) We can turn memcg_create_mutex to rw-semaphore (or introduce an rw-semaphore, which we would take for modifying list_lru's) and take it for reading in memcg_list_lru_init() and for writing when we create a new memcg (memcg_init_all_lrus()).
This would remove the bottleneck from the mount path, but every memcg creation would still iterate over all LRUs under a memcg mutex. So I guess it is not an option, isn't it?

Right - it's not so much that there is a mutex to protect the init, it's how long it's held that will be the issue. I mean, we don't need to hold the memcg_create_mutex until we've completely initialised the lru structure and are ready to add it to the all_memcg_lrus list, right?

i.e. restructing it so that you don't need to hold the mutex until you make the LRU list globally visible would solve the problem just as well. if we can iterate the memcgs lists without holding a lock, then we can init the per-memcg lru lists without holding a lock because nobody will access them through the list_lru structure because it's not yet been published.

Quoted 3 lines

That keeps the locking simple, and we get scalability because we've reduced the lock's scope to just a few instructures instead of a memcg iteration and a heap of memory allocation....

Unfortunately that's not that easy as it seems to be :-(

Currently I hold the memcg_create_mutex while initializing per-memcg LRUs in memcg_list_lru_init() in order to be sure that I won't miss a memcg that is added during initialization.

I mean, let's try to move per-memcg LRUs allocation outside the lock and only register the LRU there:

memcg_list_lru_init():
1) allocate lru->memcg array
2) for_each_kmem_active_memcg(m)
allocate lru->memcg[m]
3) lock memcg_create_mutex
add lru to all_memcg_lrus_list
unlock memcg_create_mutex

Then if a new kmem-active memcg is added after step 2 and before step 3, it won't see the new lru, because it has not been registered yet, and thus won't initialize its list in this lru. As a result, we will end up with a partially initialized list_lru. Note that this will happen even if the whole memcg initialization proceeds under the memcg_create_mutex.

Provided we could freeze memcg_limited_groups_array_size, it would be possible to fix this problem by swapping steps 2 and 3 and making step 2 initialize lru->memcg[m] using cmpxchg() only if it was not initialized. However we still have to hold the memcg_create_mutex during the whole kmemcg activation path (memcg_init_all_lrus()).

Let's see if we can get rid of the lock in memcg_init_all_lrus() by making the all_memcg_lrus RCU-protected so that we could iterate over all list_lrus w/o holding any locks and turn memcg_init_all_lrus() to something like this:

memcg_init_all_lrus():
1) for_each_list_lru_rcu(lru)
allocate lru->memcg[new_memcg_id]
2) mark new_memcg as kmem-active

The problem is that if memcg_list_lru_init(new_lru) starts and completes between steps 1 and 2, we will not initialize new_lru->memcg[new_memcg_id] neither in memcg_init_all_lrus() nor in memcg_list_lru_init().

The problem here is that on kmemcg creation (memcg_init_all_lrus()) we have to iterate over all list_lrus while on list_lru creation (memcg_list_lru_init()) we have to iterate over all memcgs. Currently I can't figure out how we can do it w/o holding any mutexes at least while calling one of these functions, but I'm keep thinking on it.

Quoted 119 lines · Vladimir Davydov

Quoted 72 lines

Quoted 52 lines

Quoted 41 lines

Quoted 41 lines

mm/memcontrol.c +41 -0

+int memcg_list_lru_init(struct list_lru *lru)
+{
+	int err = 0;
+	int i;
+	struct mem_cgroup *memcg;
+
+	lru->memcg = NULL;
+	lru->memcg_old = NULL;
+
+	mutex_lock(&memcg_create_mutex);
+	if (!memcg_kmem_enabled())
+		goto out_list_add;
+
+	lru->memcg = kcalloc(memcg_limited_groups_array_size,
+			     sizeof(*lru->memcg), GFP_KERNEL);
+	if (!lru->memcg) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	for_each_mem_cgroup(memcg) {
+		int memcg_id;
+
+		memcg_id = memcg_cache_id(memcg);
+		if (memcg_id < 0)
+			continue;
+
+		err = list_lru_memcg_alloc(lru, memcg_id);
+		if (err) {
+			mem_cgroup_iter_break(NULL, memcg);
+			goto out_free_lru_memcg;
+		}
+	}
+out_list_add:
+	list_add(&lru->list, &all_memcg_lrus);
+out:
+	mutex_unlock(&memcg_create_mutex);
+	return err;
+
+out_free_lru_memcg:
+	for (i = 0; i < memcg_limited_groups_array_size; i++)

Quoted 8 lines

mm/memcontrol.c static size_t memcg_caches_array_size(int num_groups) · line 3150

Quoted 4 lines

mm/memcontrol.c +4 -0

+		list_lru_memcg_free(lru, i);
+	kfree(lru->memcg);
+	goto out;
+}

That will probably scale even worse. Think about what happens when we try to mount a bunch of filesystems in parallel - they will now serialise completely on this memcg_create_mutex instantiating memcg lists inside list_lru_init().

Yes, the scalability seems to be the main problem here. I have a couple of thoughts on how it could be improved. Here they go:

Quoted 7 lines

1) We can turn memcg_create_mutex to rw-semaphore (or introduce an rw-semaphore, which we would take for modifying list_lru's) and take it for reading in memcg_list_lru_init() and for writing when we create a new memcg (memcg_init_all_lrus()).
This would remove the bottleneck from the mount path, but every memcg creation would still iterate over all LRUs under a memcg mutex. So I guess it is not an option, isn't it?

Right - it's not so much that there is a mutex to protect the init, it's how long it's held that will be the issue. I mean, we don't need to hold the memcg_create_mutex until we've completely initialised the lru structure and are ready to add it to the all_memcg_lrus list, right?

i.e. restructing it so that you don't need to hold the mutex until you make the LRU list globally visible would solve the problem just as well. if we can iterate the memcgs lists without holding a lock, then we can init the per-memcg lru lists without holding a lock because nobody will access them through the list_lru structure because it's not yet been published.

Quoted 3 lines

That keeps the locking simple, and we get scalability because we've reduced the lock's scope to just a few instructures instead of a memcg iteration and a heap of memory allocation....

Unfortunately that's not that easy as it seems to be :-(

Currently I hold the memcg_create_mutex while initializing per-memcg LRUs in memcg_list_lru_init() in order to be sure that I won't miss a memcg that is added during initialization.

I mean, let's try to move per-memcg LRUs allocation outside the lock and only register the LRU there:

memcg_list_lru_init():
1) allocate lru->memcg array
2) for_each_kmem_active_memcg(m)
allocate lru->memcg[m]
3) lock memcg_create_mutex
add lru to all_memcg_lrus_list
unlock memcg_create_mutex

Then if a new kmem-active memcg is added after step 2 and before step 3, it won't see the new lru, because it has not been registered yet, and thus won't initialize its list in this lru. As a result, we will end up with a partially initialized list_lru. Note that this will happen even if the whole memcg initialization proceeds under the memcg_create_mutex.

Provided we could freeze memcg_limited_groups_array_size, it would be possible to fix this problem by swapping steps 2 and 3 and making step 2 initialize lru->memcg[m] using cmpxchg() only if it was not initialized. However we still have to hold the memcg_create_mutex during the whole kmemcg activation path (memcg_init_all_lrus()).

Let's see if we can get rid of the lock in memcg_init_all_lrus() by making the all_memcg_lrus RCU-protected so that we could iterate over all list_lrus w/o holding any locks and turn memcg_init_all_lrus() to something like this:

memcg_init_all_lrus():
1) for_each_list_lru_rcu(lru)
allocate lru->memcg[new_memcg_id]
2) mark new_memcg as kmem-active

The problem is that if memcg_list_lru_init(new_lru) starts and completes between steps 1 and 2, we will not initialize new_lru->memcg[new_memcg_id] neither in memcg_init_all_lrus() nor in memcg_list_lru_init().

Quoted 6 lines

The problem here is that on kmemcg creation (memcg_init_all_lrus()) we have to iterate over all list_lrus while on list_lru creation (memcg_list_lru_init()) we have to iterate over all memcgs. Currently I can't figure out how we can do it w/o holding any mutexes at least while calling one of these functions, but I'm keep thinking on it.

Seems I got it. We could add a memcg state bit, say "activating", meaning that a memcg is going to become kmem-active, but it is not yet, so it should not be accounted to; keep all list_lrus in a rcu-protected list; and implement memcg_init_all_lrus() and memcg_list_lru_init() as follows:

memcg_init_all_lrus():

    set activating
    for_each_list_lru_rcu(lru)
        cmpxchg(&lru->memcg[new_memcg_id], NULL, new list_lru_node);
    set active

memcg_list_lru_init():
add the new_lru to the all_memcg_lrus list for_each_memcg(memcg):
if memcg is activating or active:
cmpxchg(&new_lru->memcg[memcg_id], NULL, new list_lru_node)

At first glance, it looks correct, because:

If we skip an lru while iterating over all_memcg_lrus in memcg_init_all_lrus(), it means it was created after the "activating" bit had been set and thus the per-memcg lru will be initialized in memcg_list_lru_init(). If we skip a memcg in memcg_list_lru_init(), this memg will "see" this lru while iterating over the all_memcg_lrus list, because the lru must have been created before the "activating" bit set.

Although it is to be elaborated, because I haven't examined the destruction paths yet, and this doesn't take into account the fact that memcg_limited_groups_array_size is not a constant (I guess I'll have to introduce an rw semaphore for it, but it'll be OK, because its updates are very-very rare), I guess I am on the right way.

OK, as far as I can tell, this is introducing a per-node, per-memcg LRU lists. Is that correct?

If so, then that is not what Glauber and I originally intended for memcg LRUs. per-node LRUs are expensive in terms of memory and cross multiplying them by the number of memcgs in a system was not a good use of memory.

According to Glauber, most memcgs are small and typically confined to a single node or two by external means and therefore don't need the scalability numa aware LRUs provide. Hence the idea was that the memcg LRUs would just be a single LRU list, just like a non-numa aware list_lru instantiation. IOWs, this is the structure that we had decided on as the best compromise between memory usage, complexity and memcg awareness:

Since now list_lru automatically distributes objects among per-memcg lists and list_lru_{count,walk} employ information passed in the shrink_control argument to scan appropriate list, all shrinkers that keep objects in the list_lru structure can already work as memcg-aware. Let us mark them so.

Signed-off-by Vladimir Davydov <[email protected]>

Cc Glauber Costa <[email protected]>

Cc Dave Chinner <[email protected]>

Cc Al Viro <[email protected]>

---
 fs/gfs2/quota.c  |    2 +-
 fs/super.c       |    2 +-
 fs/xfs/xfs_buf.c |    2 +-
 fs/xfs/xfs_qm.c  |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

fs/gfs2/quota.c +0 -0

index f0435da..6cf6114 100644

Quoted 22 lines · Vladimir Davydov

Since now list_lru automatically distributes objects among per-memcg lists and list_lru_{count,walk} employ information passed in the shrink_control argument to scan appropriate list, all shrinkers that keep objects in the list_lru structure can already work as memcg-aware. Let us mark them so.

Signed-off-by Vladimir Davydov <[email protected]>

Cc Glauber Costa <[email protected]>

Cc Dave Chinner <[email protected]>

Cc Al Viro <[email protected]>

---
 fs/gfs2/quota.c  |    2 +-
 fs/super.c       |    2 +-
 fs/xfs/xfs_buf.c |    2 +-
 fs/xfs/xfs_qm.c  |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

fs/gfs2/quota.c +0 -0

index f0435da..6cf6114 100644

fs/gfs2/quota.c struct shrinker gfs2_qd_shrinker = { · line 154

Quoted 6 lines

fs/gfs2/quota.c +1 -1

@@ -150,7 +150,7 @@ struct shrinker gfs2_qd_shrinker = {
 	.count_objects = gfs2_qd_shrink_count,
 	.scan_objects = gfs2_qd_shrink_scan,
 	.seeks = DEFAULT_SEEKS,
-	.flags = SHRINKER_NUMA_AWARE,
+	.flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE,
 };

I'll leave it for Steve to have the final say, but this cache tracks objects that have contexts that span multiple memcgs (i.e. global scope) and so is not a candidate for memcg based shrinking.

e.g. a single user can have processes running in multiple concurrent memcgs, and so the user quota dquot needs to be accessed from all those memcg contexts. Same for group quota objects - they can span multiple memcgs that different users have instantiated, simply because they all belong to the same group and hence are subject to the group quota accounting.

And for XFS, there's also project quotas, which means you can have files that are unique to both users and groups, but shared the same project quota and hence span memcgs that way....

From: Glauber Costa <[email protected]>

In very low free kernel memory situations, it may be the case that we have less objects to free than our initial batch size. If this is the case, it is better to shrink those, and open space for the new workload then to keep them and fail the new allocations.

In particular, we are concerned with the direct reclaim case for memcg. Although this same technique can be applied to other situations just as well, we will start conservative and apply it for that case, which is the one that matters the most.

Quoted 7 lines · Vladimir Davydov

From: Glauber Costa <[email protected]>

In very low free kernel memory situations, it may be the case that we have less objects to free than our initial batch size. If this is the case, it is better to shrink those, and open space for the new workload then to keep them and fail the new allocations.

Quoted 4 lines

In particular, we are concerned with the direct reclaim case for memcg. Although this same technique can be applied to other situations just as well, we will start conservative and apply it for that case, which is the one that matters the most.

This should be at the start of the series.

Quoted 12 lines · Dave Chinner

Quoted 11 lines · Vladimir Davydov

From: Glauber Costa <[email protected]>

In very low free kernel memory situations, it may be the case that we have less objects to free than our initial batch size. If this is the case, it is better to shrink those, and open space for the new workload then to keep them and fail the new allocations.

In particular, we are concerned with the direct reclaim case for memcg. Although this same technique can be applied to other situations just as well, we will start conservative and apply it for that case, which is the one that matters the most.

Quoted 1 line

This should be at the start of the series.

Since Glauber wanted to introduce this only for memcg-reclaim first, this can't be at the start of the series, but I'll move it to go immediately after per-memcg shrinking core in the next iteration.

Thanks.

From: Glauber Costa <[email protected]>

During the past weeks, it became clear to us that the shrinker interface

Please note that in contrast to previous versions 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. This is, because the implementation of kmem-only reclaim was rather incomplete - we had to fail GFP_NOFS allocations since everything we could reclaim was only FS data. I will try to improve this and send 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.