diff mbox series

[v2,5/9] mm: memcontrol: add per memcg shrinker nr_deferred

Message ID 20201214223722.232537-6-shy828301@gmail.com (mailing list archive)
State New
Headers show
Series Make shrinker's nr_deferred memcg aware | expand

Commit Message

Yang Shi Dec. 14, 2020, 10:37 p.m. UTC
Currently the number of deferred objects are per shrinker, but some slabs, for example,
vfs inode/dentry cache are per memcg, this would result in poor isolation among memcgs.

The deferred objects typically are generated by __GFP_NOFS allocations, one memcg with
excessive __GFP_NOFS allocations may blow up deferred objects, then other innocent memcgs
may suffer from over shrink, excessive reclaim latency, etc.

For example, two workloads run in memcgA and memcgB respectively, workload in B is vfs
heavy workload.  Workload in A generates excessive deferred objects, then B's vfs cache
might be hit heavily (drop half of caches) by B's limit reclaim or global reclaim.

We observed this hit in our production environment which was running vfs heavy workload
shown as the below tracing log:

<...>-409454 [016] .... 28286961.747146: mm_shrink_slab_start: super_cache_scan+0x0/0x1a0 ffff9a83046f3458:
nid: 1 objects to shrink 3641681686040 gfp_flags GFP_HIGHUSER_MOVABLE|__GFP_ZERO pgs_scanned 1 lru_pgs 15721
cache items 246404277 delta 31345 total_scan 123202138
<...>-409454 [022] .... 28287105.928018: mm_shrink_slab_end: super_cache_scan+0x0/0x1a0 ffff9a83046f3458:
nid: 1 unused scan count 3641681686040 new scan count 3641798379189 total_scan 602
last shrinker return val 123186855

The vfs cache and page cache ration was 10:1 on this machine, and half of caches were dropped.
This also resulted in significant amount of page caches were dropped due to inodes eviction.

Make nr_deferred per memcg for memcg aware shrinkers would solve the unfairness and bring
better isolation.

When memcg is not enabled (!CONFIG_MEMCG or memcg disabled), the shrinker's nr_deferred
would be used.  And non memcg aware shrinkers use shrinker's nr_deferred all the time.

Signed-off-by: Yang Shi <shy828301@gmail.com>
---
 include/linux/memcontrol.h |   9 +++
 mm/memcontrol.c            | 110 ++++++++++++++++++++++++++++++++++++-
 mm/vmscan.c                |   4 ++
 3 files changed, 120 insertions(+), 3 deletions(-)

Comments

Dave Chinner Dec. 15, 2020, 2:22 a.m. UTC | #1
On Mon, Dec 14, 2020 at 02:37:18PM -0800, Yang Shi wrote:
> Currently the number of deferred objects are per shrinker, but some slabs, for example,
> vfs inode/dentry cache are per memcg, this would result in poor isolation among memcgs.
> 
> The deferred objects typically are generated by __GFP_NOFS allocations, one memcg with
> excessive __GFP_NOFS allocations may blow up deferred objects, then other innocent memcgs
> may suffer from over shrink, excessive reclaim latency, etc.
> 
> For example, two workloads run in memcgA and memcgB respectively, workload in B is vfs
> heavy workload.  Workload in A generates excessive deferred objects, then B's vfs cache
> might be hit heavily (drop half of caches) by B's limit reclaim or global reclaim.
> 
> We observed this hit in our production environment which was running vfs heavy workload
> shown as the below tracing log:
> 
> <...>-409454 [016] .... 28286961.747146: mm_shrink_slab_start: super_cache_scan+0x0/0x1a0 ffff9a83046f3458:
> nid: 1 objects to shrink 3641681686040 gfp_flags GFP_HIGHUSER_MOVABLE|__GFP_ZERO pgs_scanned 1 lru_pgs 15721
> cache items 246404277 delta 31345 total_scan 123202138
> <...>-409454 [022] .... 28287105.928018: mm_shrink_slab_end: super_cache_scan+0x0/0x1a0 ffff9a83046f3458:
> nid: 1 unused scan count 3641681686040 new scan count 3641798379189 total_scan 602
> last shrinker return val 123186855
> 
> The vfs cache and page cache ration was 10:1 on this machine, and half of caches were dropped.
> This also resulted in significant amount of page caches were dropped due to inodes eviction.
> 
> Make nr_deferred per memcg for memcg aware shrinkers would solve the unfairness and bring
> better isolation.
> 
> When memcg is not enabled (!CONFIG_MEMCG or memcg disabled), the shrinker's nr_deferred
> would be used.  And non memcg aware shrinkers use shrinker's nr_deferred all the time.
> 
> Signed-off-by: Yang Shi <shy828301@gmail.com>
> ---
>  include/linux/memcontrol.h |   9 +++
>  mm/memcontrol.c            | 110 ++++++++++++++++++++++++++++++++++++-
>  mm/vmscan.c                |   4 ++
>  3 files changed, 120 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 922a7f600465..1b343b268359 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -92,6 +92,13 @@ struct lruvec_stat {
>  	long count[NR_VM_NODE_STAT_ITEMS];
>  };
>  
> +
> +/* Shrinker::id indexed nr_deferred of memcg-aware shrinkers. */
> +struct memcg_shrinker_deferred {
> +	struct rcu_head rcu;
> +	atomic_long_t nr_deferred[];
> +};

So you're effectively copy and pasting the memcg_shrinker_map
infrastructure and doubling the number of allocations/frees required
to set up/tear down a memcg? Why not add it to the struct
memcg_shrinker_map like this:

struct memcg_shrinker_map {
        struct rcu_head	rcu;
	unsigned long	*map;
	atomic_long_t	*nr_deferred;
};

And when you dynamically allocate the structure, set the map and
nr_deferred pointers to the correct offset in the allocated range.

Then this patch is really only changes to the size of the chunk
being allocated, setting up the pointers and copying the relevant
data from the old to new.

Cheers,

Dave.
Johannes Weiner Dec. 15, 2020, 2:45 p.m. UTC | #2
On Tue, Dec 15, 2020 at 01:22:33PM +1100, Dave Chinner wrote:
> On Mon, Dec 14, 2020 at 02:37:18PM -0800, Yang Shi wrote:
> > Currently the number of deferred objects are per shrinker, but some slabs, for example,
> > vfs inode/dentry cache are per memcg, this would result in poor isolation among memcgs.
> > 
> > The deferred objects typically are generated by __GFP_NOFS allocations, one memcg with
> > excessive __GFP_NOFS allocations may blow up deferred objects, then other innocent memcgs
> > may suffer from over shrink, excessive reclaim latency, etc.
> > 
> > For example, two workloads run in memcgA and memcgB respectively, workload in B is vfs
> > heavy workload.  Workload in A generates excessive deferred objects, then B's vfs cache
> > might be hit heavily (drop half of caches) by B's limit reclaim or global reclaim.
> > 
> > We observed this hit in our production environment which was running vfs heavy workload
> > shown as the below tracing log:
> > 
> > <...>-409454 [016] .... 28286961.747146: mm_shrink_slab_start: super_cache_scan+0x0/0x1a0 ffff9a83046f3458:
> > nid: 1 objects to shrink 3641681686040 gfp_flags GFP_HIGHUSER_MOVABLE|__GFP_ZERO pgs_scanned 1 lru_pgs 15721
> > cache items 246404277 delta 31345 total_scan 123202138
> > <...>-409454 [022] .... 28287105.928018: mm_shrink_slab_end: super_cache_scan+0x0/0x1a0 ffff9a83046f3458:
> > nid: 1 unused scan count 3641681686040 new scan count 3641798379189 total_scan 602
> > last shrinker return val 123186855
> > 
> > The vfs cache and page cache ration was 10:1 on this machine, and half of caches were dropped.
> > This also resulted in significant amount of page caches were dropped due to inodes eviction.
> > 
> > Make nr_deferred per memcg for memcg aware shrinkers would solve the unfairness and bring
> > better isolation.
> > 
> > When memcg is not enabled (!CONFIG_MEMCG or memcg disabled), the shrinker's nr_deferred
> > would be used.  And non memcg aware shrinkers use shrinker's nr_deferred all the time.
> > 
> > Signed-off-by: Yang Shi <shy828301@gmail.com>
> > ---
> >  include/linux/memcontrol.h |   9 +++
> >  mm/memcontrol.c            | 110 ++++++++++++++++++++++++++++++++++++-
> >  mm/vmscan.c                |   4 ++
> >  3 files changed, 120 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index 922a7f600465..1b343b268359 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -92,6 +92,13 @@ struct lruvec_stat {
> >  	long count[NR_VM_NODE_STAT_ITEMS];
> >  };
> >  
> > +
> > +/* Shrinker::id indexed nr_deferred of memcg-aware shrinkers. */
> > +struct memcg_shrinker_deferred {
> > +	struct rcu_head rcu;
> > +	atomic_long_t nr_deferred[];
> > +};
> 
> So you're effectively copy and pasting the memcg_shrinker_map
> infrastructure and doubling the number of allocations/frees required
> to set up/tear down a memcg? Why not add it to the struct
> memcg_shrinker_map like this:
> 
> struct memcg_shrinker_map {
>         struct rcu_head	rcu;
> 	unsigned long	*map;
> 	atomic_long_t	*nr_deferred;
> };
> 
> And when you dynamically allocate the structure, set the map and
> nr_deferred pointers to the correct offset in the allocated range.
> 
> Then this patch is really only changes to the size of the chunk
> being allocated, setting up the pointers and copying the relevant
> data from the old to new.

Fully agreed.

In the longer-term, it may be nice to further expand this and make
this the generalized intersection between cgroup, node and shrinkers.

There is large overlap with list_lru e.g. - with data of identical
scope and lifetime, but duplicative callbacks and management. If we
folded list_lru_memcg into the above data structure, we could also
generalize and reuse the existing callbacks.
Yang Shi Dec. 15, 2020, 9:57 p.m. UTC | #3
On Tue, Dec 15, 2020 at 6:47 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Tue, Dec 15, 2020 at 01:22:33PM +1100, Dave Chinner wrote:
> > On Mon, Dec 14, 2020 at 02:37:18PM -0800, Yang Shi wrote:
> > > Currently the number of deferred objects are per shrinker, but some slabs, for example,
> > > vfs inode/dentry cache are per memcg, this would result in poor isolation among memcgs.
> > >
> > > The deferred objects typically are generated by __GFP_NOFS allocations, one memcg with
> > > excessive __GFP_NOFS allocations may blow up deferred objects, then other innocent memcgs
> > > may suffer from over shrink, excessive reclaim latency, etc.
> > >
> > > For example, two workloads run in memcgA and memcgB respectively, workload in B is vfs
> > > heavy workload.  Workload in A generates excessive deferred objects, then B's vfs cache
> > > might be hit heavily (drop half of caches) by B's limit reclaim or global reclaim.
> > >
> > > We observed this hit in our production environment which was running vfs heavy workload
> > > shown as the below tracing log:
> > >
> > > <...>-409454 [016] .... 28286961.747146: mm_shrink_slab_start: super_cache_scan+0x0/0x1a0 ffff9a83046f3458:
> > > nid: 1 objects to shrink 3641681686040 gfp_flags GFP_HIGHUSER_MOVABLE|__GFP_ZERO pgs_scanned 1 lru_pgs 15721
> > > cache items 246404277 delta 31345 total_scan 123202138
> > > <...>-409454 [022] .... 28287105.928018: mm_shrink_slab_end: super_cache_scan+0x0/0x1a0 ffff9a83046f3458:
> > > nid: 1 unused scan count 3641681686040 new scan count 3641798379189 total_scan 602
> > > last shrinker return val 123186855
> > >
> > > The vfs cache and page cache ration was 10:1 on this machine, and half of caches were dropped.
> > > This also resulted in significant amount of page caches were dropped due to inodes eviction.
> > >
> > > Make nr_deferred per memcg for memcg aware shrinkers would solve the unfairness and bring
> > > better isolation.
> > >
> > > When memcg is not enabled (!CONFIG_MEMCG or memcg disabled), the shrinker's nr_deferred
> > > would be used.  And non memcg aware shrinkers use shrinker's nr_deferred all the time.
> > >
> > > Signed-off-by: Yang Shi <shy828301@gmail.com>
> > > ---
> > >  include/linux/memcontrol.h |   9 +++
> > >  mm/memcontrol.c            | 110 ++++++++++++++++++++++++++++++++++++-
> > >  mm/vmscan.c                |   4 ++
> > >  3 files changed, 120 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > > index 922a7f600465..1b343b268359 100644
> > > --- a/include/linux/memcontrol.h
> > > +++ b/include/linux/memcontrol.h
> > > @@ -92,6 +92,13 @@ struct lruvec_stat {
> > >     long count[NR_VM_NODE_STAT_ITEMS];
> > >  };
> > >
> > > +
> > > +/* Shrinker::id indexed nr_deferred of memcg-aware shrinkers. */
> > > +struct memcg_shrinker_deferred {
> > > +   struct rcu_head rcu;
> > > +   atomic_long_t nr_deferred[];
> > > +};
> >
> > So you're effectively copy and pasting the memcg_shrinker_map
> > infrastructure and doubling the number of allocations/frees required
> > to set up/tear down a memcg? Why not add it to the struct
> > memcg_shrinker_map like this:
> >
> > struct memcg_shrinker_map {
> >         struct rcu_head       rcu;
> >       unsigned long   *map;
> >       atomic_long_t   *nr_deferred;
> > };
> >
> > And when you dynamically allocate the structure, set the map and
> > nr_deferred pointers to the correct offset in the allocated range.
> >
> > Then this patch is really only changes to the size of the chunk
> > being allocated, setting up the pointers and copying the relevant
> > data from the old to new.
>
> Fully agreed.

Thanks folks. Such idea has been discussed with Roman in the earlier
emails. I agree this would make the code neater. Will do it in v3.

>
> In the longer-term, it may be nice to further expand this and make
> this the generalized intersection between cgroup, node and shrinkers.
>
> There is large overlap with list_lru e.g. - with data of identical
> scope and lifetime, but duplicative callbacks and management. If we
> folded list_lru_memcg into the above data structure, we could also
> generalize and reuse the existing callbacks.

Yes, agree we should look further to combine and deduplicate all the
pieces for the long run.
diff mbox series

Patch

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 922a7f600465..1b343b268359 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -92,6 +92,13 @@  struct lruvec_stat {
 	long count[NR_VM_NODE_STAT_ITEMS];
 };
 
+
+/* Shrinker::id indexed nr_deferred of memcg-aware shrinkers. */
+struct memcg_shrinker_deferred {
+	struct rcu_head rcu;
+	atomic_long_t nr_deferred[];
+};
+
 /*
  * Bitmap of shrinker::id corresponding to memcg-aware shrinkers,
  * which have elements charged to this memcg.
@@ -119,6 +126,7 @@  struct mem_cgroup_per_node {
 	struct mem_cgroup_reclaim_iter	iter;
 
 	struct memcg_shrinker_map __rcu	*shrinker_map;
+	struct memcg_shrinker_deferred __rcu	*shrinker_deferred;
 
 	struct rb_node		tree_node;	/* RB tree node */
 	unsigned long		usage_in_excess;/* Set to the value by which */
@@ -1489,6 +1497,7 @@  static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg)
 }
 
 extern int memcg_expand_shrinker_maps(int new_id);
+extern int memcg_expand_shrinker_deferred(int new_id);
 
 extern void memcg_set_shrinker_bit(struct mem_cgroup *memcg,
 				   int nid, int shrinker_id);
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 3d4ddbb84a01..321d1818ce3d 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -394,14 +394,20 @@  DEFINE_STATIC_KEY_FALSE(memcg_kmem_enabled_key);
 EXPORT_SYMBOL(memcg_kmem_enabled_key);
 #endif
 
-/* It is only can be changed with holding shrinker_rwsem exclusively */
+/* They are only can be changed with holding shrinker_rwsem exclusively */
 static int memcg_shrinker_map_size;
+static int memcg_shrinker_deferred_size;
 
 static void memcg_free_shrinker_map_rcu(struct rcu_head *head)
 {
 	kvfree(container_of(head, struct memcg_shrinker_map, rcu));
 }
 
+static void memcg_free_shrinker_deferred_rcu(struct rcu_head *head)
+{
+	kvfree(container_of(head, struct memcg_shrinker_deferred, rcu));
+}
+
 static int memcg_expand_one_shrinker_map(struct mem_cgroup *memcg,
 					 int size, int old_size)
 {
@@ -430,6 +436,34 @@  static int memcg_expand_one_shrinker_map(struct mem_cgroup *memcg,
 	return 0;
 }
 
+static int memcg_expand_one_shrinker_deferred(struct mem_cgroup *memcg,
+					      int size, int old_size)
+{
+	struct memcg_shrinker_deferred *new, *old;
+	int nid;
+
+	for_each_node(nid) {
+		old = rcu_dereference_protected(
+			mem_cgroup_nodeinfo(memcg, nid)->shrinker_deferred, true);
+		/* Not yet online memcg */
+		if (!old)
+			return 0;
+
+		new = kvmalloc_node(sizeof(*new) + size, GFP_KERNEL, nid);
+		if (!new)
+			return -ENOMEM;
+
+		/* Copy all old values, and clear all new ones */
+		memcpy((void *)new->nr_deferred, (void *)old->nr_deferred, old_size);
+		memset((void *)new->nr_deferred + old_size, 0, size - old_size);
+
+		rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_deferred, new);
+		call_rcu(&old->rcu, memcg_free_shrinker_deferred_rcu);
+	}
+
+	return 0;
+}
+
 static void memcg_free_shrinker_maps(struct mem_cgroup *memcg)
 {
 	struct mem_cgroup_per_node *pn;
@@ -448,6 +482,21 @@  static void memcg_free_shrinker_maps(struct mem_cgroup *memcg)
 	}
 }
 
+static void memcg_free_shrinker_deferred(struct mem_cgroup *memcg)
+{
+	struct mem_cgroup_per_node *pn;
+	struct memcg_shrinker_deferred *deferred;
+	int nid;
+
+	for_each_node(nid) {
+		pn = mem_cgroup_nodeinfo(memcg, nid);
+		deferred = rcu_dereference_protected(pn->shrinker_deferred, true);
+		if (deferred)
+			kvfree(deferred);
+		rcu_assign_pointer(pn->shrinker_deferred, NULL);
+	}
+}
+
 static int memcg_alloc_shrinker_maps(struct mem_cgroup *memcg)
 {
 	struct memcg_shrinker_map *map;
@@ -472,6 +521,27 @@  static int memcg_alloc_shrinker_maps(struct mem_cgroup *memcg)
 	return ret;
 }
 
+static int memcg_alloc_shrinker_deferred(struct mem_cgroup *memcg)
+{
+	struct memcg_shrinker_deferred *deferred;
+	int nid, size, ret = 0;
+
+	down_read(&shrinker_rwsem);
+	size = memcg_shrinker_deferred_size;
+	for_each_node(nid) {
+		deferred = kvzalloc_node(sizeof(*deferred) + size, GFP_KERNEL, nid);
+		if (!deferred) {
+			memcg_free_shrinker_deferred(memcg);
+			ret = -ENOMEM;
+			break;
+		}
+		rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_deferred, deferred);
+	}
+	up_read(&shrinker_rwsem);
+
+	return ret;
+}
+
 int memcg_expand_shrinker_maps(int new_id)
 {
 	int size, old_size, ret = 0;
@@ -501,6 +571,33 @@  int memcg_expand_shrinker_maps(int new_id)
 	return ret;
 }
 
+int memcg_expand_shrinker_deferred(int new_id)
+{
+	int size, old_size, ret = 0;
+	struct mem_cgroup *memcg;
+
+	size = (new_id + 1) * sizeof(atomic_long_t);
+	old_size = memcg_shrinker_deferred_size;
+	if (size <= old_size)
+		return 0;
+
+	if (!root_mem_cgroup)
+		goto out;
+
+	for_each_mem_cgroup(memcg) {
+		ret = memcg_expand_one_shrinker_deferred(memcg, size, old_size);
+		if (ret) {
+			mem_cgroup_iter_break(NULL, memcg);
+			goto out;
+		}
+	}
+out:
+	if (!ret)
+		memcg_shrinker_deferred_size = size;
+
+	return ret;
+}
+
 void memcg_set_shrinker_bit(struct mem_cgroup *memcg, int nid, int shrinker_id)
 {
 	if (shrinker_id >= 0 && memcg && !mem_cgroup_is_root(memcg)) {
@@ -5397,8 +5494,8 @@  static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
 	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
 
 	/*
-	 * A memcg must be visible for memcg_expand_shrinker_maps()
-	 * by the time the maps are allocated. So, we allocate maps
+	 * A memcg must be visible for memcg_expand_shrinker_{maps|deferred}()
+	 * by the time the maps are allocated. So, we allocate maps and deferred
 	 * here, when for_each_mem_cgroup() can't skip it.
 	 */
 	if (memcg_alloc_shrinker_maps(memcg)) {
@@ -5406,6 +5503,12 @@  static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
 		return -ENOMEM;
 	}
 
+	if (memcg_alloc_shrinker_deferred(memcg)) {
+		memcg_free_shrinker_maps(memcg);
+		mem_cgroup_id_remove(memcg);
+		return -ENOMEM;
+	}
+
 	/*
 	 * Barrier for CSS_ONLINE, so that shrink_slab_memcg() sees shirnker_maps
 	 * and shrinker_deferred before CSS_ONLINE. It pairs with the read barrier
@@ -5473,6 +5576,7 @@  static void mem_cgroup_css_free(struct cgroup_subsys_state *css)
 	cancel_work_sync(&memcg->high_work);
 	mem_cgroup_remove_from_trees(memcg);
 	memcg_free_shrinker_maps(memcg);
+	memcg_free_shrinker_deferred(memcg);
 	memcg_free_kmem(memcg);
 	mem_cgroup_free(memcg);
 }
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 16c9d2aeeb26..bf34167dd67e 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -219,6 +219,10 @@  static int prealloc_memcg_shrinker(struct shrinker *shrinker)
 			goto unlock;
 		}
 
+		if (memcg_expand_shrinker_deferred(id)) {
+			idr_remove(&shrinker_idr, id);
+			goto unlock;
+		}
 		shrinker_nr_max = id + 1;
 	}
 	shrinker->id = id;