Skip to contentSkip to conversation

kmemcg shrinkers

PATCH Superseded Vladimir Davydov 4 participants 26 messages Nov 25, 2013 – Nov 27, 2013

Subsystems (9)
Memory management VFS MM core cgroup +5 more subsystems

v11 of 15 Superseded Latest is v15 →

lore
Share & tools
lore
#0 lore raw

v11 cover kmemcg shrinkers

This patchset implements targeted shrinking for memcg when kmem limits are present. So far, we've been accounting kernel objects but failing allocations when short of memory. This is because our only option would be to call the global shrinker, depleting objects from all caches and breaking isolation.

The main idea is to associate per-memcg lists with each of the LRUs. The main LRU still provides a single entry point and when adding or removing an element from the LRU, we use the page information to figure out which memcg it belongs to and relay it to the right list.

The bulk of the code is written by Glauber Costa. The only change I introduced myself in this iteration is reworking per-memcg LRU lists. Instead of extending the existing list_lru structure, which seems to be neat as is, I introduced a new one, memcg_list_lru, which aggregates list_lru objects for each kmem-active memcg and keeps them uptodate as memcgs are created/destroyed. I hope this simplified call paths and made the code easier to read.

The patchset is based on top of Linux 3.13-rc1.

Any comments and proposals are appreciated.

== Known issues ==

* In case kmem limit is less than sum mem limit, reaching memcg kmem limit will result in an attempt to shrink all memcg slabs (see try_to_free_mem_cgroup_kmem()). Although this is better than simply failing allocation as it works now, it is still to be improved.

* Since FS shrinkers can't be executed on __GFP_FS allocations, such allocations will fail if memcg kmem limit is less than sum mem limit and the memcg kmem usage is close to its limit. Glauber proposed to schedule a worker which would shrink kmem in the background on such allocations. However, this approach does not eliminate failures completely, it just makes them rarer. I'm thinking on implementing soft limits for memcg kmem so that striking the soft limit will trigger the reclaimer, but won't fail the allocation. I would appreciate any other proposals on how this can be fixed.

* Only dcache and icache are reclaimed on memcg pressure. Other FS objects are left for global pressure only. However, it should not be a serious problem to make them reclaimable too by passing on memcg to the FS-layer and letting each FS decide if its internal objects are shrinkable on memcg pressure.

== Changes from v10 ==

* Rework per-memcg list_lru infrastructure.

Previous iteration (with full changelog) can be found here:

http://www.spinics.net/lists/linux-fsdevel/msg66632.html

Glauber Costa (12):
memcg: make cache index determination more robust
memcg: consolidate callers of memcg_cache_id
vmscan: also shrink slab in memcg pressure
memcg: move initialization to memcg creation
memcg: move stop and resume accounting functions
memcg: per-memcg kmem shrinking
memcg: scan cache objects hierarchically
vmscan: take at least one pass with shrinkers
memcg: allow kmem limit to be resized down
vmpressure: in-kernel notifications
memcg: reap dead memcgs upon global memory pressure
memcg: flush memcg items upon memcg destruction

Vladimir Davydov (3):
memcg,list_lru: add per-memcg LRU list infrastructure memcg,list_lru: add function walking over all lists of a per-memcg
LRU
super: make icache, dcache shrinkers memcg-aware

 fs/dcache.c                |   25 +-
 fs/inode.c                 |   16 +-
 fs/internal.h              |    9 +-
 fs/super.c                 |   45 +--
 include/linux/fs.h         |    4 +-
 include/linux/list_lru.h   |   77 +++++
 include/linux/memcontrol.h |   23 ++
 include/linux/shrinker.h   |    6 +-
 include/linux/swap.h       |    2 +
 include/linux/vmpressure.h |    5 +
 mm/memcontrol.c            |  709 +++++++++++++++++++++++++++++++++++++++-----
 mm/vmpressure.c            |   53 +++-
 mm/vmscan.c                |  178 +++++++++--
 13 files changed, 1018 insertions(+), 134 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 v11 1/15 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 Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
---
 mm/memcontrol.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
mm/memcontrol.c +3 -1
index f1a0ae6..02b5176 100644
@@ -3073,7 +3073,9 @@ void memcg_cache_list_add(struct mem_cgroup *memcg, struct kmem_cache *cachep)
  */
 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;
 }
 
 /*
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 v11 2/15 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 accesses in a negative index.

Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
---
 mm/memcontrol.c |   49 +++++++++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 20 deletions(-)
mm/memcontrol.c +29 -20
index 02b5176..144cb4c 100644
@@ -2960,6 +2960,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.
  */
@@ -2969,7 +2993,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
@@ -3067,18 +3091,6 @@ void memcg_cache_list_add(struct mem_cgroup *memcg, struct kmem_cache *cachep)
 }
 
 /*
- * 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 ends up being protected by the set_limit mutex, during normal
  * operation, because that is its main call site.
  *
@@ -3240,7 +3252,7 @@ void memcg_release_cache(struct kmem_cache *s)
 		goto out;
 
 	memcg = s->memcg_params->memcg;
-	id  = memcg_cache_id(memcg);
+	id = memcg_cache_idx(memcg);
 
 	root = s->memcg_params->root_cache;
 	root->memcg_params->memcg_caches[id] = NULL;
@@ -3403,9 +3415,7 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 	struct kmem_cache *new_cachep;
 	int idx;
 
-	BUG_ON(!memcg_can_account_kmem(memcg));
-
-	idx = memcg_cache_id(memcg);
+	idx = memcg_cache_idx(memcg);
 
 	mutex_lock(&memcg_cache_mutex);
 	new_cachep = cache_from_memcg_idx(cachep, idx);
@@ -3578,10 +3588,9 @@ 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))
-		goto out;
-
 	idx = memcg_cache_id(memcg);
+	if (idx < 0)
+		goto out;
 
 	/*
 	 * barrier to mare sure we're always seeing the up to date value.  The
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 v11 3/15 vmscan: also shrink slab in memcg pressure

In reply to Vladimir Davydov

From: Glauber Costa <[email protected]>

Without the surrounding infrastructure, this patch is a bit of a hammer:
it will basically shrink objects from all memcgs under memcg pressure. At least, however, we will keep the scan limited to the shrinkers marked as per-memcg.

Future patches will implement the in-shrinker logic to filter objects based on its memcg association.

Cc Dave Chinner <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/linux/memcontrol.h |   17 +++++++++++++++
 include/linux/shrinker.h   |    6 +++++-
 mm/memcontrol.c            |   16 +++++++++++++-
 mm/vmscan.c                |   50 +++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 82 insertions(+), 7 deletions(-)
include/linux/memcontrol.h +17 -0
index b3e7a66..d16ba51 100644
@@ -231,6 +231,9 @@ void mem_cgroup_split_huge_fixup(struct page *head);
 bool mem_cgroup_bad_page_check(struct page *page);
 void mem_cgroup_print_bad_page(struct page *page);
 #endif
+
+unsigned long
+memcg_zone_reclaimable_pages(struct mem_cgroup *memcg, struct zone *zone);
 #else /* CONFIG_MEMCG */
 struct mem_cgroup;
 
@@ -427,6 +430,12 @@ static inline void mem_cgroup_replace_page_cache(struct page *oldpage,
 				struct page *newpage)
 {
 }
+
+static inline unsigned long
+memcg_zone_reclaimable_pages(struct mem_cgroup *memcg, struct zone *zone)
+{
+	return 0;
+}
 #endif /* CONFIG_MEMCG */
 
 #if !defined(CONFIG_MEMCG) || !defined(CONFIG_DEBUG_VM)
@@ -479,6 +488,8 @@ static inline bool memcg_kmem_enabled(void)
 	return static_key_false(&memcg_kmem_enabled_key);
 }
 
+bool memcg_kmem_is_active(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
@@ -612,6 +623,12 @@ memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
 	return __memcg_kmem_get_cache(cachep, gfp);
 }
 #else
+
+static inline bool memcg_kmem_is_active(struct mem_cgroup *memcg)
+{
+	return false;
+}
+
 #define for_each_memcg_cache_index(_idx)	\
 	for (; NULL; )
 
include/linux/shrinker.h +5 -1
index 68c0970..7d462b1 100644
@@ -22,6 +22,9 @@ struct shrink_control {
 	nodemask_t nodes_to_scan;
 	/* current node being shrunk (for NUMA aware shrinkers) */
 	int nid;
+
+	/* reclaim from this memcg only (if not NULL) */
+	struct mem_cgroup *target_mem_cgroup;
 };
 
 #define SHRINK_STOP (~0UL)
@@ -63,7 +66,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 +15 -1
index 144cb4c..8924ff1 100644
@@ -358,7 +358,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);
 }
@@ -958,6 +958,20 @@ mem_cgroup_zone_nr_lru_pages(struct mem_cgroup *memcg, int nid, int zid,
 	return ret;
 }
 
+unsigned long
+memcg_zone_reclaimable_pages(struct mem_cgroup *memcg, struct zone *zone)
+{
+	int nid = zone_to_nid(zone);
+	int zid = zone_idx(zone);
+	unsigned long val;
+
+	val = mem_cgroup_zone_nr_lru_pages(memcg, nid, zid, LRU_ALL_FILE);
+	if (do_swap_account)
+		val += mem_cgroup_zone_nr_lru_pages(memcg, nid, zid,
+						    LRU_ALL_ANON);
+	return val;
+}
+
 static unsigned long
 mem_cgroup_node_nr_lru_pages(struct mem_cgroup *memcg,
 			int nid, unsigned int lru_mask)
mm/vmscan.c +45 -5

86 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>
maintainer
#4 lore raw

Re: [PATCH v11 03/15] vmscan: also shrink slab in memcg pressure

Quoted 113 lines · Vladimir Davydov

From: Glauber Costa <[email protected]>

Without the surrounding infrastructure, this patch is a bit of a hammer:
it will basically shrink objects from all memcgs under memcg pressure. At least, however, we will keep the scan limited to the shrinkers marked as per-memcg.

Future patches will implement the in-shrinker logic to filter objects based on its memcg association.

Cc Dave Chinner <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/linux/memcontrol.h |   17 +++++++++++++++
 include/linux/shrinker.h   |    6 +++++-
 mm/memcontrol.c            |   16 +++++++++++++-
 mm/vmscan.c                |   50 +++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 82 insertions(+), 7 deletions(-)
include/linux/memcontrol.h +17 -0
index b3e7a66..d16ba51 100644
@@ -231,6 +231,9 @@ void mem_cgroup_split_huge_fixup(struct page *head);
 bool mem_cgroup_bad_page_check(struct page *page);
 void mem_cgroup_print_bad_page(struct page *page);
 #endif
+
+unsigned long
+memcg_zone_reclaimable_pages(struct mem_cgroup *memcg, struct zone *zone);
 #else /* CONFIG_MEMCG */
 struct mem_cgroup;
 
@@ -427,6 +430,12 @@ static inline void mem_cgroup_replace_page_cache(struct page *oldpage,
 				struct page *newpage)
 {
 }
+
+static inline unsigned long
+memcg_zone_reclaimable_pages(struct mem_cgroup *memcg, struct zone *zone)
+{
+	return 0;
+}
 #endif /* CONFIG_MEMCG */
 
 #if !defined(CONFIG_MEMCG) || !defined(CONFIG_DEBUG_VM)
@@ -479,6 +488,8 @@ static inline bool memcg_kmem_enabled(void)
 	return static_key_false(&memcg_kmem_enabled_key);
 }
 
+bool memcg_kmem_is_active(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
@@ -612,6 +623,12 @@ memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
 	return __memcg_kmem_get_cache(cachep, gfp);
 }
 #else
+
+static inline bool memcg_kmem_is_active(struct mem_cgroup *memcg)
+{
+	return false;
+}
+
 #define for_each_memcg_cache_index(_idx)	\
 	for (; NULL; )
 
include/linux/shrinker.h +5 -1
index 68c0970..7d462b1 100644
@@ -22,6 +22,9 @@ struct shrink_control {
 	nodemask_t nodes_to_scan;
 	/* current node being shrunk (for NUMA aware shrinkers) */
 	int nid;
+
+	/* reclaim from this memcg only (if not NULL) */
+	struct mem_cgroup *target_mem_cgroup;
 };
 
 #define SHRINK_STOP (~0UL)
@@ -63,7 +66,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 +1 -1
index 144cb4c..8924ff1 100644
@@ -358,7 +358,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);
 }

mm/memcontrol.c mem_cgroup_zone_nr_lru_pages(struct mem_cgroup *memcg, int nid, int zid, · line 962

Quoted 5 lines
mm/memcontrol.c +2 -0
@@ -958,6 +958,20 @@ mem_cgroup_zone_nr_lru_pages(struct mem_cgroup *memcg, int nid, int zid,
 	return ret;
 }
 
+unsigned long
+memcg_zone_reclaimable_pages(struct mem_cgroup *memcg, struct zone *zone)

If the prototype does not fit, please wrap the argument list, not the return value. We are not consistent, but most functions are like this in memcontrol.c and vmscan.c.

Quoted 9 lines
mm/vmscan.c +1 -0
index eea668d..652dfa3 100644
@@ -140,11 +140,41 @@ static bool global_reclaim(struct scan_control *sc)
 {
 	return !sc->target_mem_cgroup;
 }
+

mm/vmscan.c static bool global_reclaim(struct scan_control *sc) · line 153

Quoted 10 lines
mm/vmscan.c +10 -0
+/*
+ * kmem reclaim should usually not be triggered when we are doing targetted
+ * reclaim. It is only valid when global reclaim is triggered, or when the
+ * underlying memcg has kmem objects.
+ */
+static bool has_kmem_reclaim(struct scan_control *sc)
+{
+	return !sc->target_mem_cgroup ||
+		memcg_kmem_is_active(sc->target_mem_cgroup);
+}

Please opencode these checks in the callsite, they are more descriptive than the name of this function.

mm/vmscan.c static bool global_reclaim(struct scan_control *sc) · line 161

Quoted 7 lines
mm/vmscan.c +7 -0
+static unsigned long
+zone_nr_reclaimable_pages(struct scan_control *sc, struct zone *zone)
+{
+	if (global_reclaim(sc))
+		return zone_reclaimable_pages(zone);
+	return memcg_zone_reclaimable_pages(sc->target_mem_cgroup, zone);
+}

So we have zone_reclaimable_pages() and zone_nr_reclaimable_pages() with completely different signatures and usecases. Not good.

The intersection between a zone and a memcg is called an lruvec, please use that. Look up an lruvec as early as possible, then implement lruvec_reclaimable_pages() etc. for use during reclaim.

Quoted 42 lines
mm/vmscan.c +14 -5
@@ -352,6 +382,15 @@ unsigned long shrink_slab(struct shrink_control *shrinkctl,
 	}
 
 	list_for_each_entry(shrinker, &shrinker_list, list) {
+		/*
+		 * 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 (shrinkctl->target_mem_cgroup &&
+		    !(shrinker->flags & SHRINKER_MEMCG_AWARE))
+			continue;
+
 		for_each_node_mask(shrinkctl->nid, shrinkctl->nodes_to_scan) {
 			if (!node_online(shrinkctl->nid))
 				continue;
@@ -2399,11 +2438,11 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
 
 		/*
 		 * Don't shrink slabs when reclaiming memory from over limit
-		 * cgroups but do shrink slab at least once when aborting
-		 * reclaim for compaction to avoid unevenly scanning file/anon
-		 * LRU pages over slab pages.
+		 * cgroups unless we know they have kmem objects. But do shrink
+		 * slab at least once when aborting reclaim for compaction to
+		 * avoid unevenly scanning file/anon LRU pages over slab pages.
 		 */
-		if (global_reclaim(sc)) {
+		if (has_kmem_reclaim(sc)) {
 			unsigned long lru_pages = 0;
 
 			nodes_clear(shrink->nodes_to_scan);
@@ -2412,7 +2451,7 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
 				if (!cpuset_zone_allowed_hardwall(zone, GFP_KERNEL))
 					continue;
 
-				lru_pages += zone_reclaimable_pages(zone);
+				lru_pages += zone_nr_reclaimable_pages(sc, zone);
 				node_set(zone_to_nid(zone),
 					 shrink->nodes_to_scan);
 			}

mm/vmscan.c unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg, · line 2714

Quoted 7 lines
mm/vmscan.c +1 -0
@@ -2669,6 +2708,7 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
 	};
 	struct shrink_control shrink = {
 		.gfp_mask = sc.gfp_mask,
+		.target_mem_cgroup = memcg,
 	};
 
 	/*

--
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 v11 4/15 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 Dave Chinner <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
Cc Andrew Morton <[email protected]>
---
 mm/memcontrol.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
mm/memcontrol.c +2 -2
index 8924ff1..9ba9975 100644
@@ -3136,8 +3136,6 @@ int memcg_update_cache_sizes(struct mem_cgroup *memcg)
 	}
 
 	memcg->kmemcg_id = num;
-	INIT_LIST_HEAD(&memcg->memcg_slab_caches);
-	mutex_init(&memcg->slab_caches_mutex);
 	return 0;
 }
 
@@ -5923,6 +5921,8 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
 {
 	int ret;
 
+	INIT_LIST_HEAD(&memcg->memcg_slab_caches);
+	mutex_init(&memcg->slab_caches_mutex);
 	memcg->kmemcg_id = -1;
 	ret = memcg_propagate_kmem(memcg);
 	if (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>
#6 lore raw

PATCH v11 5/15 memcg: move stop and resume accounting functions

In reply to Vladimir Davydov

From: Glauber Costa <[email protected]>

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

Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
Cc Andrew Morton <akpm-de/[email protected]>
---
 mm/memcontrol.c |   62 +++++++++++++++++++++++++++----------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)
mm/memcontrol.c +31 -31
index 9ba9975..e9bdcf3 100644
@@ -3010,6 +3010,37 @@ static struct kmem_cache *memcg_params_to_cache(struct memcg_cache_params *p)
 	return cache_from_memcg_idx(cachep, memcg_cache_idx(p->memcg));
 }
 
+/*
+ * During the creation a new cache, we need to disable our accounting mechanism
+ * altogether. This is true even if we are not creating, but rather just
+ * enqueing new caches to be created.
+ *
+ * This is because that process will trigger allocations; some visible, like
+ * explicit kmallocs to auxiliary data structures, name strings and internal
+ * cache structures; some well concealed, like INIT_WORK() that can allocate
+ * objects during debug.
+ *
+ * If any allocation happens during memcg_kmem_get_cache, we will recurse back
+ * to it. This may not be a bounded recursion: since the first cache creation
+ * failed to complete (waiting on the allocation), we'll just try to create the
+ * cache again, failing at the same point.
+ *
+ * memcg_kmem_get_cache is prepared to abort after seeing a positive count of
+ * memcg_kmem_skip_account. So we enclose anything that might allocate memory
+ * inside the following two functions.
+ */
+static inline void memcg_stop_kmem_account(void)
+{
+	VM_BUG_ON(!current->mm);
+	current->memcg_kmem_skip_account++;
+}
+
+static inline void memcg_resume_kmem_account(void)
+{
+	VM_BUG_ON(!current->mm);
+	current->memcg_kmem_skip_account--;
+}
+
 #ifdef CONFIG_SLABINFO
 static int mem_cgroup_slabinfo_read(struct cgroup_subsys_state *css,
 				    struct cftype *cft, struct seq_file *m)
@@ -3278,37 +3309,6 @@ out:
 	kfree(s->memcg_params);
 }
 
-/*
- * During the creation a new cache, we need to disable our accounting mechanism
- * altogether. This is true even if we are not creating, but rather just
- * enqueing new caches to be created.
- *
- * This is because that process will trigger allocations; some visible, like
- * explicit kmallocs to auxiliary data structures, name strings and internal
- * cache structures; some well concealed, like INIT_WORK() that can allocate
- * objects during debug.
- *
- * If any allocation happens during memcg_kmem_get_cache, we will recurse back
- * to it. This may not be a bounded recursion: since the first cache creation
- * failed to complete (waiting on the allocation), we'll just try to create the
- * cache again, failing at the same point.
- *
- * memcg_kmem_get_cache is prepared to abort after seeing a positive count of
- * memcg_kmem_skip_account. So we enclose anything that might allocate memory
- * inside the following two functions.
- */
-static inline void memcg_stop_kmem_account(void)
-{
-	VM_BUG_ON(!current->mm);
-	current->memcg_kmem_skip_account++;
-}
-
-static inline void memcg_resume_kmem_account(void)
-{
-	VM_BUG_ON(!current->mm);
-	current->memcg_kmem_skip_account--;
-}
-
 static void kmem_cache_destroy_work_func(struct work_struct *w)
 {
 	struct kmem_cache *cachep;
Signature · 1 line
1.7.10.4
#7 lore raw

PATCH v11 6/15 memcg: per-memcg kmem shrinking

In reply to Vladimir Davydov

From: Glauber Costa <[email protected]>

If the kernel limit is smaller than the user limit, we will have situations in which our allocations fail but freeing user pages will buy us nothing. In those, we would like to call a specialized memcg reclaimer that only frees kernel memory and leave the user memory alone. Those are also expected to fail when we account memcg->kmem, instead of when we account memcg->res. Based on that, this patch implements a memcg-specific reclaimer, that only shrinks kernel objects, withouth touching user pages.

There might be situations in which there are plenty of objects to shrink, but we can't do it because the __GFP_FS flag is not set. Although they can happen with user pages, they are a lot more common with fs-metadata: this is the case with almost all inode allocation.

For those cases, the best we can do is to spawn a worker and fail the current allocation.

Cc Dave Chinner <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/linux/swap.h |    2 +
 mm/memcontrol.c      |  118 +++++++++++++++++++++++++++++++++++++++++++++++---
 mm/vmscan.c          |   44 ++++++++++++++++++-
 3 files changed, 157 insertions(+), 7 deletions(-)
include/linux/swap.h +2 -0
index 46ba0c6..367a773 100644
@@ -309,6 +309,8 @@ extern unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 extern int __isolate_lru_page(struct page *page, isolate_mode_t mode);
 extern unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *mem,
 						  gfp_t gfp_mask, bool noswap);
+extern unsigned long try_to_free_mem_cgroup_kmem(struct mem_cgroup *mem,
+						 gfp_t gfp_mask);
 extern unsigned long mem_cgroup_shrink_node_zone(struct mem_cgroup *mem,
 						gfp_t gfp_mask, bool noswap,
 						struct zone *zone,
mm/memcontrol.c +112 -6

189 lines not on this page · raw message

mm/vmscan.c +43 -1

50 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>
maintainer
#8 lore raw

Re: [PATCH v11 06/15] memcg: per-memcg kmem shrinking

Quoted 90 lines · Vladimir Davydov

From: Glauber Costa <[email protected]>

If the kernel limit is smaller than the user limit, we will have situations in which our allocations fail but freeing user pages will buy us nothing. In those, we would like to call a specialized memcg reclaimer that only frees kernel memory and leave the user memory alone. Those are also expected to fail when we account memcg->kmem, instead of when we account memcg->res. Based on that, this patch implements a memcg-specific reclaimer, that only shrinks kernel objects, withouth touching user pages.

There might be situations in which there are plenty of objects to shrink, but we can't do it because the __GFP_FS flag is not set. Although they can happen with user pages, they are a lot more common with fs-metadata: this is the case with almost all inode allocation.

For those cases, the best we can do is to spawn a worker and fail the current allocation.

Cc Dave Chinner <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
Cc Andrew Morton <akpm-de/[email protected]>
---
 include/linux/swap.h |    2 +
 mm/memcontrol.c      |  118 +++++++++++++++++++++++++++++++++++++++++++++++---
 mm/vmscan.c          |   44 ++++++++++++++++++-
 3 files changed, 157 insertions(+), 7 deletions(-)
include/linux/swap.h +2 -0
index 46ba0c6..367a773 100644
@@ -309,6 +309,8 @@ extern unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 extern int __isolate_lru_page(struct page *page, isolate_mode_t mode);
 extern unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *mem,
 						  gfp_t gfp_mask, bool noswap);
+extern unsigned long try_to_free_mem_cgroup_kmem(struct mem_cgroup *mem,
+						 gfp_t gfp_mask);
 extern unsigned long mem_cgroup_shrink_node_zone(struct mem_cgroup *mem,
 						gfp_t gfp_mask, bool noswap,
 						struct zone *zone,
mm/memcontrol.c +16 -0
index e9bdcf3..9be1e8b 100644
@@ -330,6 +330,8 @@ struct mem_cgroup {
 	atomic_t	numainfo_events;
 	atomic_t	numainfo_updating;
 #endif
+	/* when kmem shrinkers can sleep but can't proceed due to context */
+	struct work_struct kmemcg_shrink_work;
 
 	struct mem_cgroup_per_node *nodeinfo[0];
 	/* WARNING: nodeinfo must be the last member here */
@@ -341,11 +343,14 @@ static size_t memcg_size(void)
 		nr_node_ids * sizeof(struct mem_cgroup_per_node);
 }
 
+static DEFINE_MUTEX(set_limit_mutex);
+
 /* internal only representation about the status of kmem accounting. */
 enum {
 	KMEM_ACCOUNTED_ACTIVE = 0, /* accounted by this cgroup itself */
 	KMEM_ACCOUNTED_ACTIVATED, /* static key enabled. */
 	KMEM_ACCOUNTED_DEAD, /* dead memcg with pending kmem charges */
+	KMEM_MAY_SHRINK, /* kmem limit < mem limit, shrink kmem only */
 };
 
 /* We account when limit is on, but only after call sites are patched */
@@ -389,6 +394,31 @@ static bool memcg_kmem_test_and_clear_dead(struct mem_cgroup *memcg)
 	return test_and_clear_bit(KMEM_ACCOUNTED_DEAD,
 				  &memcg->kmem_account_flags);
 }
+
+/*
+ * If the kernel limit is smaller than the user limit, we will have situations
+ * in which our allocations fail but freeing user pages will buy us nothing.
+ * In those, we would like to call a specialized memcg reclaimer that only
+ * frees kernel memory and leaves the user memory alone.
+ *
+ * This test exists so we can differentiate between those. Every time one of the
+ * limits is updated, we need to run it. The set_limit_mutex must be held, so
+ * they don't change again.
+ */

mm/memcontrol.c static bool memcg_kmem_test_and_clear_dead(struct mem_cgroup *memcg) · line 417

Quoted 10 lines
mm/memcontrol.c +10 -0
+static void memcg_update_shrink_status(struct mem_cgroup *memcg)
+{
+	mutex_lock(&set_limit_mutex);
+	if (res_counter_read_u64(&memcg->kmem, RES_LIMIT) <
+		res_counter_read_u64(&memcg->res, RES_LIMIT))
+		set_bit(KMEM_MAY_SHRINK, &memcg->kmem_account_flags);
+	else
+		clear_bit(KMEM_MAY_SHRINK, &memcg->kmem_account_flags);
+	mutex_unlock(&set_limit_mutex);
+}

Fold this into the limit updating function.

Quoted 77 lines
mm/memcontrol.c +45 -6
@@ -2964,8 +2994,6 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
 	memcg_check_events(memcg, page);
 }
 
-static DEFINE_MUTEX(set_limit_mutex);
-
 #ifdef CONFIG_MEMCG_KMEM
 static inline bool memcg_can_account_kmem(struct mem_cgroup *memcg)
 {
@@ -3062,15 +3090,54 @@ static int mem_cgroup_slabinfo_read(struct cgroup_subsys_state *css,
 }
 #endif
 
+static int memcg_try_charge_kmem(struct mem_cgroup *memcg, gfp_t gfp, u64 size)
+{
+	int retries = MEM_CGROUP_RECLAIM_RETRIES;
+	struct res_counter *fail_res;
+	int ret;
+
+	do {
+		ret = res_counter_charge(&memcg->kmem, size, &fail_res);
+		if (!ret)
+			return ret;
+
+		/*
+		 * We will try to shrink kernel memory present in caches.  If
+		 * we can't wait, we will have no option rather than fail the
+		 * current allocation and make room in the background hoping
+		 * the next one will succeed.
+		 *
+		 * If we are in FS context, then although we can wait,
+		 * we cannot call the shrinkers. Most fs shrinkers will not run
+		 * without __GFP_FS since they can deadlock.
+		 */
+		if (!(gfp & __GFP_WAIT) || !(gfp & __GFP_FS)) {
+			/*
+			 * we are already short on memory, every queue
+			 * allocation is likely to fail.
+			 */
+			memcg_stop_kmem_account();
+			schedule_work(&memcg->kmemcg_shrink_work);
+			memcg_resume_kmem_account();
+		} else
+			try_to_free_mem_cgroup_kmem(memcg, gfp);
+	} while (retries--);
+
+	return ret;
+}
+
 static int memcg_charge_kmem(struct mem_cgroup *memcg, gfp_t gfp, u64 size)
 {
 	struct res_counter *fail_res;
 	struct mem_cgroup *_memcg;
 	int ret = 0;
+	bool kmem_first = test_bit(KMEM_MAY_SHRINK, &memcg->kmem_account_flags);
 
-	ret = res_counter_charge(&memcg->kmem, size, &fail_res);
-	if (ret)
-		return ret;
+	if (kmem_first) {
+		ret = memcg_try_charge_kmem(memcg, gfp, size);
+		if (ret)
+			return ret;
+	}
 
 	_memcg = memcg;
 	ret = __mem_cgroup_try_charge(NULL, gfp, size >> PAGE_SHIFT,
@@ -3096,13 +3163,47 @@ static int memcg_charge_kmem(struct mem_cgroup *memcg, gfp_t gfp, u64 size)
 		if (do_swap_account)
 			res_counter_charge_nofail(&memcg->memsw, size,
 						  &fail_res);
+		if (!kmem_first)
+			res_counter_charge_nofail(&memcg->kmem, size, &fail_res);
 		ret = 0;
-	} else if (ret)
+	} else if (ret && kmem_first)
 		res_counter_uncharge(&memcg->kmem, size);

mm/memcontrol.c static int memcg_charge_kmem(struct mem_cgroup *memcg, gfp_t gfp, u64 size) · line 3180

Quoted 10 lines
mm/memcontrol.c +9 -0
 
+	if (!ret && !kmem_first) {
+		ret = res_counter_charge(&memcg->kmem, size, &fail_res);
+		if (!ret)
+			return ret;
+
+		res_counter_uncharge(&memcg->res, size);
+		if (do_swap_account)
+			res_counter_uncharge(&memcg->memsw, size);
+	}

This is no longer readable. We have 3 res counters with non-descript names, charged in a certain combination in a certain order depending on a bunch of non-descript states, and no documentation. Please fix this.

Quoted 91 lines
mm/memcontrol.c +28 -0
+
 	return ret;
 }
 
+/*
+ * There might be situations in which there are plenty of objects to shrink,
+ * but we can't do it because the __GFP_FS flag is not set.  This is the case
+ * with almost all inode allocation. Unfortunately we have no idea which fs
+ * locks we are holding to put ourselves in this situation, so the best we
+ * can do is to spawn a worker, fail the current allocation and hope that
+ * the next one succeeds.
+ *
+ * One way to make it better is to introduce some sort of implicit soft-limit
+ * that would trigger background reclaim for memcg when we are close to the
+ * kmem limit, so that we would never have to be faced with direct reclaim
+ * potentially lacking __GFP_FS.
+ */
+static void kmemcg_shrink_work_fn(struct work_struct *w)
+{
+	struct mem_cgroup *memcg;
+
+	memcg = container_of(w, struct mem_cgroup, kmemcg_shrink_work);
+	try_to_free_mem_cgroup_kmem(memcg, GFP_KERNEL);
+}
+
+
 static void memcg_uncharge_kmem(struct mem_cgroup *memcg, u64 size)
 {
 	res_counter_uncharge(&memcg->res, size);
@@ -5289,6 +5390,8 @@ static int mem_cgroup_write(struct cgroup_subsys_state *css, struct cftype *cft,
 			ret = memcg_update_kmem_limit(css, val);
 		else
 			return -EINVAL;
+		if (!ret)
+			memcg_update_shrink_status(memcg);
 		break;
 	case RES_SOFT_LIMIT:
 		ret = res_counter_memparse_write_strategy(buffer, &val);
@@ -5922,6 +6025,7 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
 	int ret;
 
 	INIT_LIST_HEAD(&memcg->memcg_slab_caches);
+	INIT_WORK(&memcg->kmemcg_shrink_work, kmemcg_shrink_work_fn);
 	mutex_init(&memcg->slab_caches_mutex);
 	memcg->kmemcg_id = -1;
 	ret = memcg_propagate_kmem(memcg);
@@ -5941,6 +6045,8 @@ static void kmem_cgroup_css_offline(struct mem_cgroup *memcg)
 	if (!memcg_kmem_is_active(memcg))
 		return;
 
+	cancel_work_sync(&memcg->kmemcg_shrink_work);
+
 	/*
 	 * kmem charges can outlive the cgroup. In the case of slab
 	 * pages, for instance, a page contain objects from various
mm/vmscan.c +27 -1
index 652dfa3..cdfc364 100644
@@ -2730,7 +2730,49 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
 
 	return nr_reclaimed;
 }
-#endif
+
+#ifdef CONFIG_MEMCG_KMEM
+/*
+ * This function is called when we are under kmem-specific pressure.  It will
+ * only trigger in environments with kmem.limit_in_bytes < limit_in_bytes, IOW,
+ * with a lower kmem allowance than the memory allowance.
+ *
+ * In this situation, freeing user pages from the cgroup won't do us any good.
+ * What we really need is to call the memcg-aware shrinkers, in the hope of
+ * freeing pages holding kmem objects. It may also be that we won't be able to
+ * free any pages, but will get rid of old objects opening up space for new
+ * ones.
+ */
+unsigned long try_to_free_mem_cgroup_kmem(struct mem_cgroup *memcg,
+					  gfp_t gfp_mask)
+{
+	long freed;
+
+	struct shrink_control shrink = {
+		.gfp_mask = gfp_mask,
+		.target_mem_cgroup = memcg,
+	};
+
+	if (!(gfp_mask & __GFP_WAIT))
+		return 0;
+
+	/*

mm/vmscan.c unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg, · line 2769

Quoted 10 lines
mm/vmscan.c +10 -0
+	 * memcg pressure is always global */
+	nodes_setall(shrink.nodes_to_scan);
+
+	/*
+	 * We haven't scanned any user LRU, so we basically come up with
+	 * crafted values of nr_scanned and LRU page (1 and 0 respectively).
+	 * This should be enough to tell shrink_slab that the freeing
+	 * responsibility is all on himself.
+	 */
+	freed = shrink_slab(&shrink, 1, 0);

Never make up phony values to be able to use a function that was written for a completely separate use case. We own the whole source tree, refactor the code so it makes sense in all paths.

#9 lore raw

PATCH v11 7/15 memcg: scan cache objects hierarchically

In reply to Vladimir Davydov

From: Glauber Costa <[email protected]>

When reaching shrink_slab, we should descent in children memcg searching for objects that could be shrunk. This is true even if the memcg does not have kmem limits on, since the kmem res_counter will also be billed against the user res_counter of the parent.

It is possible that we will free objects and not free any pages, that will just harm the child groups without helping the parent group at all. But at this point, we basically are prepared to pay the price.

Cc Dave Chinner <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/linux/memcontrol.h |    6 ++++
 mm/memcontrol.c            |   13 +++++++++
 mm/vmscan.c                |   65 ++++++++++++++++++++++++++++++++++++--------
 3 files changed, 73 insertions(+), 11 deletions(-)
include/linux/memcontrol.h +6 -0
index d16ba51..a513fad 100644
@@ -488,6 +488,7 @@ static inline bool memcg_kmem_enabled(void)
 	return static_key_false(&memcg_kmem_enabled_key);
 }
 
+bool memcg_kmem_should_reclaim(struct mem_cgroup *memcg);
 bool memcg_kmem_is_active(struct mem_cgroup *memcg);
 
 /*
@@ -624,6 +625,11 @@ memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
 }
 #else
 
+static inline bool memcg_kmem_should_reclaim(struct mem_cgroup *memcg)
+{
+	return false;
+}
+
 static inline bool memcg_kmem_is_active(struct mem_cgroup *memcg)
 {
 	return false;
mm/memcontrol.c +13 -0
index 9be1e8b..f5d7128 100644
@@ -2995,6 +2995,19 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
 }
 
 #ifdef CONFIG_MEMCG_KMEM
+bool memcg_kmem_should_reclaim(struct mem_cgroup *memcg)
+{
+	struct mem_cgroup *iter;
+
+	for_each_mem_cgroup_tree(iter, memcg) {
+		if (memcg_kmem_is_active(iter)) {
+			mem_cgroup_iter_break(memcg, iter);
+			return true;
+		}
+	}
+	return false;
+}
+
 static inline bool memcg_can_account_kmem(struct mem_cgroup *memcg)
 {
 	return !mem_cgroup_disabled() && !mem_cgroup_is_root(memcg) &&
mm/vmscan.c +54 -11

92 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>
maintainer
#10 lore raw

Re: [PATCH v11 07/15] memcg: scan cache objects hierarchically

Quoted 88 lines · Vladimir Davydov

From: Glauber Costa <[email protected]>

When reaching shrink_slab, we should descent in children memcg searching for objects that could be shrunk. This is true even if the memcg does not have kmem limits on, since the kmem res_counter will also be billed against the user res_counter of the parent.

It is possible that we will free objects and not free any pages, that will just harm the child groups without helping the parent group at all. But at this point, we basically are prepared to pay the price.

Cc Dave Chinner <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/linux/memcontrol.h |    6 ++++
 mm/memcontrol.c            |   13 +++++++++
 mm/vmscan.c                |   65 ++++++++++++++++++++++++++++++++++++--------
 3 files changed, 73 insertions(+), 11 deletions(-)
include/linux/memcontrol.h +6 -0
index d16ba51..a513fad 100644
@@ -488,6 +488,7 @@ static inline bool memcg_kmem_enabled(void)
 	return static_key_false(&memcg_kmem_enabled_key);
 }
 
+bool memcg_kmem_should_reclaim(struct mem_cgroup *memcg);
 bool memcg_kmem_is_active(struct mem_cgroup *memcg);
 
 /*
@@ -624,6 +625,11 @@ memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
 }
 #else
 
+static inline bool memcg_kmem_should_reclaim(struct mem_cgroup *memcg)
+{
+	return false;
+}
+
 static inline bool memcg_kmem_is_active(struct mem_cgroup *memcg)
 {
 	return false;
mm/memcontrol.c +13 -0
index 9be1e8b..f5d7128 100644
@@ -2995,6 +2995,19 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
 }
 
 #ifdef CONFIG_MEMCG_KMEM
+bool memcg_kmem_should_reclaim(struct mem_cgroup *memcg)
+{
+	struct mem_cgroup *iter;
+
+	for_each_mem_cgroup_tree(iter, memcg) {
+		if (memcg_kmem_is_active(iter)) {
+			mem_cgroup_iter_break(memcg, iter);
+			return true;
+		}
+	}
+	return false;
+}
+
 static inline bool memcg_can_account_kmem(struct mem_cgroup *memcg)
 {
 	return !mem_cgroup_disabled() && !mem_cgroup_is_root(memcg) &&
mm/vmscan.c +1 -1
index cdfc364..36fc133 100644
@@ -149,7 +149,7 @@ static bool global_reclaim(struct scan_control *sc)
 static bool has_kmem_reclaim(struct scan_control *sc)
 {
 	return !sc->target_mem_cgroup ||
-		memcg_kmem_is_active(sc->target_mem_cgroup);
+		memcg_kmem_should_reclaim(sc->target_mem_cgroup);
 }
 
 static unsigned long

mm/vmscan.c shrink_slab_node(struct shrink_control *shrinkctl, struct shrinker *shrinker, · line 365

Quoted 6 lines
mm/vmscan.c +3 -0
@@ -360,12 +360,35 @@ shrink_slab_node(struct shrink_control *shrinkctl, struct shrinker *shrinker,
  *
  * Returns the number of slab objects which we shrunk.
  */
+static unsigned long
+shrink_slab_one(struct shrink_control *shrinkctl, struct shrinker *shrinker,
+		unsigned long nr_pages_scanned, unsigned long lru_pages)

one what?

Quoted 54 lines
mm/vmscan.c +34 -6
+{
+	unsigned long freed = 0;
+
+	for_each_node_mask(shrinkctl->nid, shrinkctl->nodes_to_scan) {
+		if (!node_online(shrinkctl->nid))
+			continue;
+
+		if (!(shrinker->flags & SHRINKER_NUMA_AWARE) &&
+		    (shrinkctl->nid != 0))
+			break;
+
+		freed += shrink_slab_node(shrinkctl, shrinker,
+			 nr_pages_scanned, lru_pages);
+
+	}
+
+	return freed;
+}
+
 unsigned long shrink_slab(struct shrink_control *shrinkctl,
 			  unsigned long nr_pages_scanned,
 			  unsigned long lru_pages)
 {
 	struct shrinker *shrinker;
 	unsigned long freed = 0;
+	struct mem_cgroup *root = shrinkctl->target_mem_cgroup;
 
 	if (nr_pages_scanned == 0)
 		nr_pages_scanned = SWAP_CLUSTER_MAX;
@@ -390,19 +413,39 @@ unsigned long shrink_slab(struct shrink_control *shrinkctl,
 		if (shrinkctl->target_mem_cgroup &&
 		    !(shrinker->flags & SHRINKER_MEMCG_AWARE))
 			continue;
+		/*
+		 * 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 (saved in
+		 * root), we proceed normally and expect to break in the next
+		 * round.
+		 */
+		do {
+			struct mem_cgroup *memcg = shrinkctl->target_mem_cgroup;
 
-		for_each_node_mask(shrinkctl->nid, shrinkctl->nodes_to_scan) {
-			if (!node_online(shrinkctl->nid))
-				continue;
-
-			if (!(shrinker->flags & SHRINKER_NUMA_AWARE) &&
-			    (shrinkctl->nid != 0))
+			if (!memcg || memcg_kmem_is_active(memcg))

mm/vmscan.c unsigned long shrink_slab(struct shrink_control *shrinkctl, · line 440

Quoted 10 lines
mm/vmscan.c +10 -0
+				freed += shrink_slab_one(shrinkctl, shrinker,
+					 nr_pages_scanned, lru_pages);
+			/*
+			 * For non-memcg aware shrinkers, we will arrive here
+			 * at first pass because we need to scan the root
+			 * memcg.  We need to bail out, since exactly because
+			 * they are not memcg aware, instead of noticing they
+			 * have nothing to shrink, they will just shrink again,
+			 * and deplete too many objects.
+			 */

I actually found the code easier to understand without this comment.

mm/vmscan.c unsigned long shrink_slab(struct shrink_control *shrinkctl, · line 444

Quoted 4 lines
mm/vmscan.c +3 -0
+			if (!(shrinker->flags & SHRINKER_MEMCG_AWARE))
 				break;
+			shrinkctl->target_mem_cgroup =
+				mem_cgroup_iter(root, memcg, NULL);

The target memcg is always the same, don't change this. Look at the lru scan code for reference. Iterate zones (nodes in this case) first, then iterate the memcgs in each zone (node), look up the lruvec and then call shrink_slab_lruvec(lruvec, ...).

--
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 v11 8/15 vmscan: take at least one pass with shrinkers

In reply to 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.

CC Dave Chinner <[email protected]>
CC Carlos Maiolino <[email protected]>
CC "Theodore Ts'o" <[email protected]>
CC Al Viro <[email protected]>
---
 mm/vmscan.c |   23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)
mm/vmscan.c +18 -5
index 36fc133..bfedcdc 100644
@@ -311,20 +311,33 @@ shrink_slab_node(struct shrink_control *shrinkctl, struct shrinker *shrinker,
 				nr_pages_scanned, lru_pages,
 				max_pass, delta, total_scan);
 
-	while (total_scan >= batch_size) {
+	do {
 		unsigned long ret;
+		unsigned long nr_to_scan = min(batch_size, total_scan);
+		struct mem_cgroup *memcg = shrinkctl->target_mem_cgroup;
+
+		/*
+		 * Differentiate between "few objects" and "no objects"
+		 * as returned by the count step.
+		 */
+		if (!total_scan)
+			break;
+
+		if ((total_scan < batch_size) &&
+		   !(memcg && memcg_kmem_is_active(memcg)))
+			break;
 
-		shrinkctl->nr_to_scan = batch_size;
+		shrinkctl->nr_to_scan = nr_to_scan;
 		ret = shrinker->scan_objects(shrinker, shrinkctl);
 		if (ret == SHRINK_STOP)
 			break;
 		freed += ret;
 
-		count_vm_events(SLABS_SCANNED, batch_size);
-		total_scan -= batch_size;
+		count_vm_events(SLABS_SCANNED, nr_to_scan);
+		total_scan -= nr_to_scan;
 
 		cond_resched();
-	}
+	} while (total_scan >= batch_size);
 
 	/*
 	 * move the unused scan count back into the shrinker in a
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 v11 9/15 memcg,list_lru: add per-memcg LRU list infrastructure

In reply to Vladimir Davydov

FS-shrinkers, which shrink dcaches and icaches, keep dentries and inodes in list_lru structures in order to evict least recently used objects. With per-memcg kmem shrinking infrastructure introduced, we have to make those LRU lists per-memcg in order to allow shrinking FS caches that belong to different memory cgroups independently.

This patch addresses the issue by introducing struct memcg_list_lru. This struct aggregates list_lru objects for each kmem-active memcg, and keeps it uptodate whenever a memcg is created or destroyed. Its interface is very simple: it only allows to get the pointer to the appropriate list_lru object from a memcg or a kmem ptr, which should be further operated with conventional list_lru methods.

Cc Glauber Costa <[email protected]>
Cc Dave Chinner <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/linux/list_lru.h |   56 ++++++++++
 mm/memcontrol.c          |  256 ++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 306 insertions(+), 6 deletions(-)
include/linux/list_lru.h +56 -0
index 3ce5417..b3b3b86 100644
@@ -10,6 +10,8 @@
 #include <linux/list.h>
 #include <linux/nodemask.h>
 
+struct mem_cgroup;
+
 /* list_lru_walk_cb has to always return one of those */
 enum lru_status {
 	LRU_REMOVED,		/* item removed from list */
@@ -31,6 +33,27 @@ struct list_lru {
 	nodemask_t		active_nodes;
 };
 
+struct memcg_list_lru {
+	struct list_lru global_lru;
+
+#ifdef CONFIG_MEMCG_KMEM
+	struct list_lru **memcg_lrus;	/* rcu-protected array of per-memcg
+					   lrus, indexed by memcg_cache_id() */
+
+	struct list_head list;		/* list of all memcg-aware lrus */
+
+	/*
+	 * The memcg_lrus array is rcu protected, so we can only free it after
+	 * a call to synchronize_rcu(). To avoid multiple calls to
+	 * synchronize_rcu() when many 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 old_lrus variable for each lru, and then
+	 * free them all at once after a single call to synchronize_rcu().
+	 */
+	void *old_lrus;
+#endif
+};
+
 void list_lru_destroy(struct list_lru *lru);
 int list_lru_init(struct list_lru *lru);
 
@@ -128,4 +151,37 @@ list_lru_walk(struct list_lru *lru, list_lru_walk_cb isolate,
 	}
 	return isolated;
 }
+
+#ifdef CONFIG_MEMCG_KMEM
+int memcg_list_lru_init(struct memcg_list_lru *lru);
+void memcg_list_lru_destroy(struct memcg_list_lru *lru);
+
+struct list_lru *
+mem_cgroup_list_lru(struct memcg_list_lru *lru, struct mem_cgroup *memcg);
+struct list_lru *
+mem_cgroup_kmem_list_lru(struct memcg_list_lru *lru, void *ptr);
+#else
+static inline int memcg_list_lru_init(struct memcg_list_lru *lru)
+{
+	return list_lru_init(&lru->global_lru);
+}
+
+static inline void memcg_list_lru_destroy(struct memcg_list_lru *lru)
+{
+	list_lru_destroy(&lru->global_lru);
+}
+
+static inline struct list_lru *
+mem_cgroup_list_lru(struct memcg_list_lru *lru, struct mem_cgroup *memcg)
+{
+	return &lru->global_lru;
+}
+
+static inline struct list_lru *
+mem_cgroup_kmem_list_lru(struct memcg_list_lru *lru, void *ptr)
+{
+	return &lru->global_lru;
+}
+#endif /* CONFIG_MEMCG_KMEM */
+
 #endif /* _LRU_LIST_H */
mm/memcontrol.c +250 -6

289 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>
maintainer
#13 lore raw

Re: [PATCH v11 09/15] memcg,list_lru: add per-memcg LRU list infrastructure

Quoted 120 lines · Vladimir Davydov

FS-shrinkers, which shrink dcaches and icaches, keep dentries and inodes in list_lru structures in order to evict least recently used objects. With per-memcg kmem shrinking infrastructure introduced, we have to make those LRU lists per-memcg in order to allow shrinking FS caches that belong to different memory cgroups independently.

This patch addresses the issue by introducing struct memcg_list_lru. This struct aggregates list_lru objects for each kmem-active memcg, and keeps it uptodate whenever a memcg is created or destroyed. Its interface is very simple: it only allows to get the pointer to the appropriate list_lru object from a memcg or a kmem ptr, which should be further operated with conventional list_lru methods.

Cc Glauber Costa <[email protected]>
Cc Dave Chinner <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
Cc Andrew Morton <akpm-de/[email protected]>
---
 include/linux/list_lru.h |   56 ++++++++++
 mm/memcontrol.c          |  256 ++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 306 insertions(+), 6 deletions(-)
include/linux/list_lru.h +56 -0
index 3ce5417..b3b3b86 100644
@@ -10,6 +10,8 @@
 #include <linux/list.h>
 #include <linux/nodemask.h>
 
+struct mem_cgroup;
+
 /* list_lru_walk_cb has to always return one of those */
 enum lru_status {
 	LRU_REMOVED,		/* item removed from list */
@@ -31,6 +33,27 @@ struct list_lru {
 	nodemask_t		active_nodes;
 };
 
+struct memcg_list_lru {
+	struct list_lru global_lru;
+
+#ifdef CONFIG_MEMCG_KMEM
+	struct list_lru **memcg_lrus;	/* rcu-protected array of per-memcg
+					   lrus, indexed by memcg_cache_id() */
+
+	struct list_head list;		/* list of all memcg-aware lrus */
+
+	/*
+	 * The memcg_lrus array is rcu protected, so we can only free it after
+	 * a call to synchronize_rcu(). To avoid multiple calls to
+	 * synchronize_rcu() when many 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 old_lrus variable for each lru, and then
+	 * free them all at once after a single call to synchronize_rcu().
+	 */
+	void *old_lrus;
+#endif
+};
+
 void list_lru_destroy(struct list_lru *lru);
 int list_lru_init(struct list_lru *lru);
 
@@ -128,4 +151,37 @@ list_lru_walk(struct list_lru *lru, list_lru_walk_cb isolate,
 	}
 	return isolated;
 }
+
+#ifdef CONFIG_MEMCG_KMEM
+int memcg_list_lru_init(struct memcg_list_lru *lru);
+void memcg_list_lru_destroy(struct memcg_list_lru *lru);
+
+struct list_lru *
+mem_cgroup_list_lru(struct memcg_list_lru *lru, struct mem_cgroup *memcg);
+struct list_lru *
+mem_cgroup_kmem_list_lru(struct memcg_list_lru *lru, void *ptr);
+#else
+static inline int memcg_list_lru_init(struct memcg_list_lru *lru)
+{
+	return list_lru_init(&lru->global_lru);
+}
+
+static inline void memcg_list_lru_destroy(struct memcg_list_lru *lru)
+{
+	list_lru_destroy(&lru->global_lru);
+}
+
+static inline struct list_lru *
+mem_cgroup_list_lru(struct memcg_list_lru *lru, struct mem_cgroup *memcg)
+{
+	return &lru->global_lru;
+}
+
+static inline struct list_lru *
+mem_cgroup_kmem_list_lru(struct memcg_list_lru *lru, void *ptr)
+{
+	return &lru->global_lru;
+}
+#endif /* CONFIG_MEMCG_KMEM */
+
 #endif /* _LRU_LIST_H */
mm/memcontrol.c +1 -0
index f5d7128..84f1ca3 100644
@@ -55,6 +55,7 @@
 #include <linux/cpu.h>
 #include <linux/oom.h>
 #include <linux/lockdep.h>
+#include <linux/list_lru.h>
 #include "internal.h"
 #include <net/sock.h>
 #include <net/ip.h>

mm/memcontrol.c void memcg_cache_list_add(struct mem_cgroup *memcg, struct kmem_cache *cachep) · line 3253

Quoted 4 lines
mm/memcontrol.c +1 -0
@@ -3249,6 +3250,8 @@ void memcg_cache_list_add(struct mem_cgroup *memcg, struct kmem_cache *cachep)
 	mutex_unlock(&memcg->slab_caches_mutex);
 }
 
+static int memcg_update_all_lrus(int num_groups);

This name is a red flag. It does not say what the function does, and return value and parameter are unexpected.

#14 lore raw

PATCH v11 10/15 memcg,list_lru: add function walking over all lists of a per-memcg LRU

In reply to Vladimir Davydov

Sometimes it can be necessary to iterate over all memcgs' lists of the same memcg-aware LRU. For example shrink_dcache_sb() should prune all dentries no matter what memory cgroup they belong to. Current interface to struct memcg_list_lru, however, only allows per-memcg LRU walks. This patch adds the special method memcg_list_lru_walk_all() which provides the required functionality. Note that this function does not guarantee that all the elements will be processed in the true least-recently-used order, in fact it simply enumerates all kmem-active memcgs and for each of them calls list_lru_walk(), but shrink_dcache_sb(), which is going to be the only user of this function, does not need it.

Cc Glauber Costa <[email protected]>
Cc Dave Chinner <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/linux/list_lru.h |   21 ++++++++++++++++++
 mm/memcontrol.c          |   55 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)
include/linux/list_lru.h +21 -0
index b3b3b86..ce815cc 100644
@@ -40,6 +40,16 @@ struct memcg_list_lru {
 	struct list_lru **memcg_lrus;	/* rcu-protected array of per-memcg
 					   lrus, indexed by memcg_cache_id() */
 
+	/*
+	 * When a memory cgroup is removed, all pointers to its list_lru
+	 * objects stored in memcg_lrus arrays are first marked as dead by
+	 * setting the lowest bit of the address while the actual data free
+	 * happens only after an rcu grace period. If a memcg_lrus reader,
+	 * which should be rcu-protected, faces a dead pointer, it won't
+	 * dereference it. This ensures there will be no use-after-free.
+	 */
+#define MEMCG_LIST_LRU_DEAD		1
+
 	struct list_head list;		/* list of all memcg-aware lrus */
 
 	/*
@@ -160,6 +170,10 @@ struct list_lru *
 mem_cgroup_list_lru(struct memcg_list_lru *lru, struct mem_cgroup *memcg);
 struct list_lru *
 mem_cgroup_kmem_list_lru(struct memcg_list_lru *lru, void *ptr);
+
+unsigned long
+memcg_list_lru_walk_all(struct memcg_list_lru *lru, list_lru_walk_cb isolate,
+			void *cb_arg, unsigned long nr_to_walk);
 #else
 static inline int memcg_list_lru_init(struct memcg_list_lru *lru)
 {
@@ -182,6 +196,13 @@ mem_cgroup_kmem_list_lru(struct memcg_list_lru *lru, void *ptr)
 {
 	return &lru->global_lru;
 }
+
+static inline unsigned long
+memcg_list_lru_walk_all(struct memcg_list_lru *lru, list_lru_walk_cb isolate,
+			void *cb_arg, unsigned long nr_to_walk)
+{
+	return list_lru_walk(&lru->global_lru, isolate, cb_arg, nr_to_walk);
+}
 #endif /* CONFIG_MEMCG_KMEM */
 
 #endif /* _LRU_LIST_H */
mm/memcontrol.c +55 -0

83 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>
#15 lore raw

PATCH v11 11/15 super: make icache, dcache shrinkers memcg-aware

In reply to Vladimir Davydov

Using the per-memcg LRU infrastructure introduced by previous patches, this patch makes dcache and icache shrinkers memcg-aware. To achieve that, it converts s_dentry_lru and s_inode_lru from list_lru to memcg_list_lru and restricts the reclaim to per-memcg parts of the lists in case of memcg pressure.

Other FS objects are currently ignored and only reclaimed on global pressure, because their shrinkers are heavily FS-specific and can't be converted to be memcg-aware so easily. However, we can pass on target memcg to the FS layer and let it decide if per-memcg objects should be reclaimed.

Note that with this patch applied we lose global LRU order, but it does not appear to be a critical drawback, because global pressure should try to balance the amount reclaimed from all memcgs. On the other hand, preserving global LRU order would require an extra list_head added to each dentry and inode, which seems to be too costly.

Cc Glauber Costa <[email protected]>
Cc Dave Chinner <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
Cc Andrew Morton <[email protected]>
---
 fs/dcache.c        |   25 +++++++++++++++----------
 fs/inode.c         |   16 ++++++++++------
 fs/internal.h      |    9 +++++----
 fs/super.c         |   45 ++++++++++++++++++++++++++++-----------------
 include/linux/fs.h |    4 ++--
 5 files changed, 60 insertions(+), 39 deletions(-)
fs/dcache.c +15 -10
index 4bdb300..e8499db 100644
@@ -343,18 +343,24 @@ static void dentry_unlink_inode(struct dentry * dentry)
 #define D_FLAG_VERIFY(dentry,x) WARN_ON_ONCE(((dentry)->d_flags & (DCACHE_LRU_LIST | DCACHE_SHRINK_LIST)) != (x))
 static void d_lru_add(struct dentry *dentry)
 {
+	struct list_lru *lru =
+		mem_cgroup_kmem_list_lru(&dentry->d_sb->s_dentry_lru, dentry);
+
 	D_FLAG_VERIFY(dentry, 0);
 	dentry->d_flags |= DCACHE_LRU_LIST;
 	this_cpu_inc(nr_dentry_unused);
-	WARN_ON_ONCE(!list_lru_add(&dentry->d_sb->s_dentry_lru, &dentry->d_lru));
+	WARN_ON_ONCE(!list_lru_add(lru, &dentry->d_lru));
 }
 
 static void d_lru_del(struct dentry *dentry)
 {
+	struct list_lru *lru =
+		mem_cgroup_kmem_list_lru(&dentry->d_sb->s_dentry_lru, dentry);
+
 	D_FLAG_VERIFY(dentry, DCACHE_LRU_LIST);
 	dentry->d_flags &= ~DCACHE_LRU_LIST;
 	this_cpu_dec(nr_dentry_unused);
-	WARN_ON_ONCE(!list_lru_del(&dentry->d_sb->s_dentry_lru, &dentry->d_lru));
+	WARN_ON_ONCE(!list_lru_del(lru, &dentry->d_lru));
 }
 
 static void d_shrink_del(struct dentry *dentry)
@@ -970,9 +976,9 @@ 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
+ * prune_dcache_lru - shrink the dcache
+ * @lru: dentry lru list
+ * @nr_to_scan: number of entries to try to free
  * @nid: which node to scan for freeable entities
  *
  * Attempt to shrink the superblock dcache LRU by @nr_to_scan entries. This is
@@ -982,14 +988,13 @@ 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)
+long prune_dcache_lru(struct list_lru *lru, unsigned long nr_to_scan, int nid)
 {
 	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_walk_node(lru, nid, dentry_lru_isolate,
+				   &dispose, &nr_to_scan);
 	shrink_dentry_list(&dispose);
 	return freed;
 }
@@ -1029,7 +1034,7 @@ void shrink_dcache_sb(struct super_block *sb)
 	do {
 		LIST_HEAD(dispose);
 
-		freed = list_lru_walk(&sb->s_dentry_lru,
+		freed = memcg_list_lru_walk_all(&sb->s_dentry_lru,
 			dentry_lru_isolate_shrink, &dispose, UINT_MAX);
 
 		this_cpu_sub(nr_dentry_unused, freed);
fs/inode.c +10 -6
index 4bcdad3..f06a963 100644
@@ -402,7 +402,10 @@ EXPORT_SYMBOL(ihold);
 
 static void inode_lru_list_add(struct inode *inode)
 {
-	if (list_lru_add(&inode->i_sb->s_inode_lru, &inode->i_lru))
+	struct list_lru *lru =
+		mem_cgroup_kmem_list_lru(&inode->i_sb->s_inode_lru, inode);
+
+	if (list_lru_add(lru, &inode->i_lru))
 		this_cpu_inc(nr_unused);
 }
 
@@ -421,8 +424,10 @@ void inode_add_lru(struct inode *inode)
 
 static void inode_lru_list_del(struct inode *inode)
 {
+	struct list_lru *lru =
+		mem_cgroup_kmem_list_lru(&inode->i_sb->s_inode_lru, inode);
 
-	if (list_lru_del(&inode->i_sb->s_inode_lru, &inode->i_lru))
+	if (list_lru_del(lru, &inode->i_lru))
 		this_cpu_dec(nr_unused);
 }
 
@@ -748,14 +753,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_lru(struct list_lru *lru, unsigned long nr_to_scan, int nid)
 {
 	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_walk_node(lru, nid, inode_lru_isolate,
+				   &freeable, &nr_to_scan);
 	dispose_list(&freeable);
 	return freed;
 }
fs/internal.h +5 -4
index 4657424..5d977f3 100644
@@ -14,6 +14,7 @@ struct file_system_type;
 struct linux_binprm;
 struct path;
 struct mount;
+struct list_lru;
 
 /*
  * block_dev.c
@@ -107,8 +108,8 @@ 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_lru(struct list_lru *lru,
+			     unsigned long nr_to_scan, int nid);
 extern void inode_add_lru(struct inode *inode);
 
 /*
@@ -125,8 +126,8 @@ 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_lru(struct list_lru *lru,
+			     unsigned long nr_to_scan, int nid);
 
 /*
  * read_write.c
fs/super.c +28 -17

104 lines not on this page · raw message

include/linux/fs.h +2 -2
index 121f11f..8256a7e 100644
@@ -1322,8 +1322,8 @@ struct super_block {
 	 * Keep the lru lists last in the structure so they always sit on their
 	 * own individual cachelines.
 	 */
-	struct list_lru		s_dentry_lru ____cacheline_aligned_in_smp;
-	struct list_lru		s_inode_lru ____cacheline_aligned_in_smp;
+	struct memcg_list_lru s_dentry_lru ____cacheline_aligned_in_smp;
+	struct memcg_list_lru s_inode_lru ____cacheline_aligned_in_smp;
 	struct rcu_head		rcu;
 };
 
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>
#16 lore raw

PATCH v11 12/15 memcg: allow kmem limit to be resized down

In reply to Vladimir Davydov

From: Glauber Costa <[email protected]>

The userspace memory limit can be freely resized down. Upon attempt, reclaim will be called to flush the pages away until we either reach the limit we want or give up.

It wasn't possible so far with the kmem limit, since we had no way to shrink the kmem buffers other than using the big hammer of shrink_slab, that effectively frees data around the whole system.

The situation flips now that we have a per-memcg shrinker infrastructure. We will proceed analogously to our user memory counterpart and try to shrink our buffers until we either reach the limit we want or give up.

Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
---
 mm/memcontrol.c |   43 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 5 deletions(-)
mm/memcontrol.c +38 -5
index 7b4f420..a605eb0 100644
@@ -5581,10 +5581,39 @@ static ssize_t mem_cgroup_read(struct cgroup_subsys_state *css,
 	return simple_read_from_buffer(buf, nbytes, ppos, str, len);
 }
 
+#ifdef CONFIG_MEMCG_KMEM
+/*
+ * This is slightly different than res or memsw reclaim.  We already have
+ * vmscan behind us to drive the reclaim, so we can basically keep trying until
+ * all buffers that can be flushed are flushed. We have a very clear signal
+ * about it in the form of the return value of try_to_free_mem_cgroup_kmem.
+ */
+static int mem_cgroup_resize_kmem_limit(struct mem_cgroup *memcg,
+					unsigned long long val)
+{
+	int ret = -EBUSY;
+
+	for (;;) {
+		if (signal_pending(current)) {
+			ret = -EINTR;
+			break;
+		}
+
+		ret = res_counter_set_limit(&memcg->kmem, val);
+		if (!ret)
+			break;
+
+		/* Can't free anything, pointless to continue */
+		if (!try_to_free_mem_cgroup_kmem(memcg, GFP_KERNEL))
+			break;
+	}
+
+	return ret;
+}
+
 static int memcg_update_kmem_limit(struct cgroup_subsys_state *css, u64 val)
 {
 	int ret = -EINVAL;
-#ifdef CONFIG_MEMCG_KMEM
 	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
 	/*
 	 * For simplicity, we won't allow this to be disabled.  It also can't
@@ -5619,16 +5648,15 @@ static int memcg_update_kmem_limit(struct cgroup_subsys_state *css, u64 val)
 		 * starts accounting before all call sites are patched
 		 */
 		memcg_kmem_set_active(memcg);
-	} else
-		ret = res_counter_set_limit(&memcg->kmem, val);
+	} else {
+		ret = mem_cgroup_resize_kmem_limit(memcg, val);
+	}
 out:
 	mutex_unlock(&set_limit_mutex);
 	mutex_unlock(&memcg_create_mutex);
-#endif
 	return ret;
 }
 
-#ifdef CONFIG_MEMCG_KMEM
 static int memcg_propagate_kmem(struct mem_cgroup *memcg)
 {
 	int ret = 0;
@@ -5665,6 +5693,11 @@ static int memcg_propagate_kmem(struct mem_cgroup *memcg)
 out:
 	return ret;
 }
+#else
+static int memcg_update_kmem_limit(struct cgroup *cont, u64 val)
+{
+	return -EINVAL;
+}
 #endif /* CONFIG_MEMCG_KMEM */
 
 /*
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>
#17 lore raw

PATCH v11 13/15 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 later 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.

Cc Dave Chinner <[email protected]>
Cc John Stultz <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Joonsoo Kim <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
Cc Johannes Weiner <[email protected]>
---
 include/linux/vmpressure.h |    5 +++++
 mm/vmpressure.c            |   53 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 3 deletions(-)
include/linux/vmpressure.h +5 -0
index 3f3788d..9102e53 100644
@@ -19,6 +19,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;
 };
 
@@ -38,6 +41,8 @@ extern int vmpressure_register_event(struct cgroup_subsys_state *css,
 				     struct cftype *cft,
 				     struct eventfd_ctx *eventfd,
 				     const char *args);
+extern int vmpressure_register_kernel_event(struct cgroup_subsys_state *css,
+					    void (*fn)(void));
 extern void vmpressure_unregister_event(struct cgroup_subsys_state *css,
 					struct cftype *cft,
 					struct eventfd_ctx *eventfd);
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>
#18 lore raw

PATCH v11 14/15 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 |   80 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 77 insertions(+), 3 deletions(-)
mm/memcontrol.c +77 -3
index a605eb0..3533d33 100644
@@ -287,8 +287,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
@@ -338,6 +346,29 @@ struct mem_cgroup {
 	/* WARNING: nodeinfo must be the last member here */
 };
 
+#if defined(CONFIG_MEMCG_KMEM) || defined(CONFIG_MEMCG_SWAP)
+static LIST_HEAD(dangling_memcgs);
+static DEFINE_MUTEX(dangling_memcgs_mutex);
+
+static inline void memcg_dangling_del(struct mem_cgroup *memcg)
+{
+	mutex_lock(&dangling_memcgs_mutex);
+	list_del(&memcg->dead);
+	mutex_unlock(&dangling_memcgs_mutex);
+}
+
+static inline void memcg_dangling_add(struct mem_cgroup *memcg)
+{
+	INIT_LIST_HEAD(&memcg->dead);
+	mutex_lock(&dangling_memcgs_mutex);
+	list_add(&memcg->dead, &dangling_memcgs);
+	mutex_unlock(&dangling_memcgs_mutex);
+}
+#else
+static inline void memcg_dangling_free(struct mem_cgroup *memcg) {}
+static inline void memcg_dangling_add(struct mem_cgroup *memcg) {}
+#endif
+
 static size_t memcg_size(void)
 {
 	return sizeof(struct mem_cgroup) +
@@ -6364,6 +6395,41 @@ static int mem_cgroup_oom_control_write(struct cgroup_subsys_state *css,
 }
 
 #ifdef CONFIG_MEMCG_KMEM
+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 cgroup_subsys_state *css)
+{
+	vmpressure_register_kernel_event(css, memcg_vmpressure_shrink_dead);
+}
+
 static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
 {
 	int ret;
@@ -6421,6 +6487,10 @@ static void kmem_cgroup_css_offline(struct mem_cgroup *memcg)
 		css_put(&memcg->css);
 }
 #else
+static inline void memcg_register_kmem_events(struct cgroup *cont)
+{
+}
+
 static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
 {
 	return 0;
@@ -6759,8 +6829,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(css);
 		return 0;
+	}
 
 	mutex_lock(&memcg_create_mutex);
 
@@ -6822,6 +6894,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);
 }
 
@@ -6830,6 +6903,7 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css)
 	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
 
 	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>
#19 lore raw

PATCH v11 15/15 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 Mel Gorman <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Kamezawa Hiroyuki <[email protected]>
---
 mm/memcontrol.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)
mm/memcontrol.c +17 -0
index 3533d33..471b544 100644
@@ -6453,12 +6453,29 @@ static void memcg_destroy_kmem(struct mem_cgroup *memcg)
 
 static void kmem_cgroup_css_offline(struct mem_cgroup *memcg)
 {
+	int ret;
 	if (!memcg_kmem_is_active(memcg))
 		return;
 
 	cancel_work_sync(&memcg->kmemcg_shrink_work);
 
 	/*
+	 * 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.
+	 */
+	do {
+		ret = try_to_free_mem_cgroup_kmem(memcg, GFP_KERNEL);
+	} while (ret);
+
+	/*
 	 * 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>
maintainer
#20 lore raw
In reply to Vladimir Davydov

I ran out of steam reviewing these because there were too many things that should be changed in the first couple patches.

I realize this is frustrating to see these type of complaints in v11 of a patch series, but the review bandwidth was simply exceeded back when Glauber submitted this along with the kmem accounting patches. A lot of the kmemcg commits themselves don't even have review tags or acks, but it all got merged anyway, and the author has moved on to different projects...

Too much stuff slips past the only two people that have more than one usecase on their agenda and are willing to maintain this code base - which is in desparate need of rework and pushback against even more drive-by feature dumps. I have repeatedly asked to split the memcg tree out of the memory tree to better deal with the vastly different developmental stages of memcg and the rest of the mm code, to no avail. So I don't know what to do anymore, but this is not working.

Thoughts?

#21 lore raw

Hi,

Thank you for the review. I agree with all your comments and I'll resend the fixed version soon.

If anyone still has something to say about the patchset, I'd be glad to hear from them.

Quoted 18 lines · Johannes Weiner

I ran out of steam reviewing these because there were too many things that should be changed in the first couple patches.

I realize this is frustrating to see these type of complaints in v11 of a patch series, but the review bandwidth was simply exceeded back when Glauber submitted this along with the kmem accounting patches. A lot of the kmemcg commits themselves don't even have review tags or acks, but it all got merged anyway, and the author has moved on to different projects...

Too much stuff slips past the only two people that have more than one usecase on their agenda and are willing to maintain this code base - which is in desparate need of rework and pushback against even more drive-by feature dumps. I have repeatedly asked to split the memcg tree out of the memory tree to better deal with the vastly different developmental stages of memcg and the rest of the mm code, to no avail. So I don't know what to do anymore, but this is not working.

Quoted 1 line

Thoughts?

That's a pity, because w/o this patchset kmemcg is in fact useless. Perhaps, it's worth trying to split it? (not sure if it'll help much though since first 11 patches are rather essential :-( )

Thanks.

--
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>

#22 lore raw
Quoted 25 lines · Vladimir Davydov

Hi,

Thank you for the review. I agree with all your comments and I'll resend the fixed version soon.

If anyone still has something to say about the patchset, I'd be glad to hear from them.

Quoted 18 lines · Johannes Weiner

I ran out of steam reviewing these because there were too many things that should be changed in the first couple patches.

I realize this is frustrating to see these type of complaints in v11 of a patch series, but the review bandwidth was simply exceeded back when Glauber submitted this along with the kmem accounting patches. A lot of the kmemcg commits themselves don't even have review tags or acks, but it all got merged anyway, and the author has moved on to different projects...

Too much stuff slips past the only two people that have more than one usecase on their agenda and are willing to maintain this code base - which is in desparate need of rework and pushback against even more drive-by feature dumps. I have repeatedly asked to split the memcg tree out of the memory tree to better deal with the vastly different developmental stages of memcg and the rest of the mm code, to no avail. So I don't know what to do anymore, but this is not working.

Quoted 3 lines
Quoted 1 line

Thoughts?

That's a pity, because w/o this patchset kmemcg is in fact useless. Perhaps, it's worth trying to split it? (not sure if it'll help much though since first 11 patches are rather essential :-( )

What do you think about splitting this set into two main series as follows:

1) Prepare vmscan to kmemcg-aware shrinkers; would include patches 1-7 of this set.
2) Make fs shrinkers memcg-aware; would include patches 9-11 of this set

and leave other patches, which are rather for optimization/extending functionality, for future?

Thanks.

maintainer
#23 lore raw
Quoted 5 lines · On Tue, 26 Nov 2013 16:55:43 +0400 Vladimir Davydov <[email protected]>…

What do you think about splitting this set into two main series as follows:

1) Prepare vmscan to kmemcg-aware shrinkers; would include patches 1-7 of this set.
2) Make fs shrinkers memcg-aware; would include patches 9-11 of this set

Please just resend everything.

Quoted 2 lines

and leave other patches, which are rather for optimization/extending functionality, for future?

It will be helpful to describe such splitting opportunities in the [0/n] changelog.

--
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
#24 lore raw
In reply to Vladimir Davydov
Quoted 7 lines · Vladimir Davydov

Hi,

Thank you for the review. I agree with all your comments and I'll resend the fixed version soon.

If anyone still has something to say about the patchset, I'd be glad to hear from them.

Please CC me on all the shrinker/list-lru changes being made as I am the original author of the list-lru code and the shrinker integration and have more than a passing interest in ensuring it doesn't get broken or crippled....

Cheers,

Dave.

Signature · 8 lines
Dave Chinner
[email protected]

--
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>
#25 lore raw
Quoted 6 lines · Dave Chinner
Quoted 5 lines · Vladimir Davydov

Hi,

Thank you for the review. I agree with all your comments and I'll resend the fixed version soon.

Quoted 6 lines
Quoted 2 lines

If anyone still has something to say about the patchset, I'd be glad to hear from them.

Please CC me on all the shrinker/list-lru changes being made as I am the original author of the list-lru code and the shrinker integration and have more than a passing interest in ensuring it doesn't get broken or crippled....

Sure, thanks.