Re: [PATCH 03/10] mm: Assign memcg-aware shrinkers bitmap to memcg

[Vladimir Davydov]

On Wed, Mar 21, 2018 at 04:21:40PM +0300, Kirill Tkhai wrote:
> Imagine a big node with many cpus, memory cgroups and containers.
> Let we have 200 containers, every container has 10 mounts,
> and 10 cgroups. All container tasks don't touch foreign
> containers mounts. If there is intensive pages write,
> and global reclaim happens, a writing task has to iterate
> over all memcgs to shrink slab, before it's able to go
> to shrink_page_list().
> 
> Iteration over all the memcg slabs is very expensive:
> the task has to visit 200 * 10 = 2000 shrinkers
> for every memcg, and since there are 2000 memcgs,
> the total calls are 2000 * 2000 = 4000000.
> 
> So, the shrinker makes 4 million do_shrink_slab() calls
> just to try to isolate SWAP_CLUSTER_MAX pages in one
> of the actively writing memcg via shrink_page_list().
> I've observed a node spending almost 100% in kernel,
> making useless iteration over already shrinked slab.
> 
> This patch adds bitmap of memcg-aware shrinkers to memcg.
> The size of the bitmap depends on bitmap_nr_ids, and during
> memcg life it's maintained to be enough to fit bitmap_nr_ids
> shrinkers. Every bit in the map is related to corresponding
> shrinker id.
> 
> Next patches will maintain set bit only for really charged
> memcg. This will allow shrink_slab() to increase its
> performance in significant way. See the last patch for
> the numbers.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@xxxxxxxxxxxxx>
> ---
>  include/linux/memcontrol.h |   20 ++++++++
>  mm/memcontrol.c            |    5 ++
>  mm/vmscan.c                |  117 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 142 insertions(+)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 4525b4404a9e..ad88a9697fb9 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -151,6 +151,11 @@ struct mem_cgroup_thresholds {
>  	struct mem_cgroup_threshold_ary *spare;
>  };
>  
> +struct shrinkers_map {

IMO better call it mem_cgroup_shrinker_map.

> +	struct rcu_head rcu;
> +	unsigned long *map[0];
> +};
> +
>  enum memcg_kmem_state {
>  	KMEM_NONE,
>  	KMEM_ALLOCATED,
> @@ -182,6 +187,9 @@ struct mem_cgroup {
>  	unsigned long low;
>  	unsigned long high;
>  
> +	/* Bitmap of shrinker ids suitable to call for this memcg */
> +	struct shrinkers_map __rcu *shrinkers_map;
> +

We keep all per-node data in mem_cgroup_per_node struct. I think this
bitmap should be defined there as well.

>  	/* Range enforcement for interrupt charges */
>  	struct work_struct high_work;
>  

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 3801ac1fcfbc..2324577c62dc 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4476,6 +4476,9 @@ static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
>  {
>  	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
>  
> +	if (alloc_shrinker_maps(memcg))
> +		return -ENOMEM;
> +

This needs a comment explaining why you can't allocate the map in
css_alloc, which seems to be a better place for it.

>  	/* Online state pins memcg ID, memcg ID pins CSS */
>  	atomic_set(&memcg->id.ref, 1);
>  	css_get(css);
> @@ -4487,6 +4490,8 @@ static void mem_cgroup_css_offline(struct cgroup_subsys_state *css)
>  	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
>  	struct mem_cgroup_event *event, *tmp;
>  
> +	free_shrinker_maps(memcg);
> +

AFAIU this can race with shrink_slab accessing the map, resulting in
use-after-free. IMO it would be safer to free the bitmap from css_free.

>  	/*
>  	 * Unregister events and notify userspace.
>  	 * Notify userspace about cgroup removing only after rmdir of cgroup
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 97ce4f342fab..9d1df5d90eca 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -165,6 +165,10 @@ static DECLARE_RWSEM(bitmap_rwsem);
>  static int bitmap_id_start;
>  static int bitmap_nr_ids;
>  static struct shrinker **mcg_shrinkers;
> +struct shrinkers_map *__rcu root_shrinkers_map;

Why do you need root_shrinkers_map? AFAIR the root memory cgroup doesn't
have kernel memory accounting enabled.

> +
> +#define SHRINKERS_MAP(memcg) \
> +	(memcg == root_mem_cgroup || !memcg ? root_shrinkers_map : memcg->shrinkers_map)
>  
>  static int expand_shrinkers_array(int old_nr, int nr)
>  {
> @@ -188,6 +192,116 @@ static int expand_shrinkers_array(int old_nr, int nr)
>  	return 0;
>  }
>  
> +static void kvfree_map_rcu(struct rcu_head *head)
> +{

> +static int memcg_expand_maps(struct mem_cgroup *memcg, int size, int old_size)
> +{

> +int alloc_shrinker_maps(struct mem_cgroup *memcg)
> +{

> +void free_shrinker_maps(struct mem_cgroup *memcg)
> +{

> +static int expand_shrinker_maps(int old_id, int id)
> +{

All these functions should be defined in memcontrol.c

The only public function should be mem_cgroup_grow_shrinker_map (I'm not
insisting on the name), which reallocates shrinker bitmap for each
cgroups so that it can accommodate the new shrinker id. To do that,
you'll probably need to keep track of the bitmap capacity in
memcontrol.c