Re: [PATCH v10 26/35] memcg,list_lru: duplicate LRUs upon kmemcg creation

[Andrew Morton <akpm@linux-foundation.org>]

On Mon,  3 Jun 2013 23:29:55 +0400 Glauber Costa <glommer@openvz.org> wrote:

> When a new memcg is created, we need to open up room for its descriptors
> in all of the list_lrus that are marked per-memcg. The process is quite
> similar to the one we are using for the kmem caches: we initialize the
> new structures in an array indexed by kmemcg_id, and grow the array if
> needed. Key data like the size of the array will be shared between the
> kmem cache code and the list_lru code (they basically describe the same
> thing)

Gee this is a big patchset.

>
> ...
>
> --- a/include/linux/list_lru.h
> +++ b/include/linux/list_lru.h
> @@ -24,6 +24,23 @@ struct list_lru_node {
>  	long			nr_items;
>  } ____cacheline_aligned_in_smp;
>  
> +/*
> + * This is supposed to be M x N matrix, where M is kmem-limited memcg, and N is
> + * the number of nodes. Both dimensions are likely to be very small, but are
> + * potentially very big. Therefore we will allocate or grow them dynamically.
> + *
> + * The size of M will increase as new memcgs appear and can be 0 if no memcgs
> + * are being used. This is done in mm/memcontrol.c in a way quite similar than

"similar to"

> + * the way we use for the slab cache management.
> + *
> + * The size o N can't be determined at compile time, but won't increase once we

"value of N"

> + * determine it. It is nr_node_ids, the firmware-provided maximum number of
> + * nodes in a system.


> + */
> +struct list_lru_array {
> +	struct list_lru_node node[1];
> +};
> +
>  struct list_lru {
>  	/*
>  	 * Because we use a fixed-size array, this struct can be very big if
> @@ -37,9 +54,38 @@ struct list_lru {
>  	 */
>  	struct list_lru_node	node[MAX_NUMNODES];
>  	nodemask_t		active_nodes;
> +#ifdef CONFIG_MEMCG_KMEM
> +	/* All memcg-aware LRUs will be chained in the lrus list */
> +	struct list_head	lrus;
> +	/* M x N matrix as described above */
> +	struct list_lru_array	**memcg_lrus;
> +#endif
>  };

It's here where I decided "this code shouldn't be in lib/" ;)

> -int list_lru_init(struct list_lru *lru);
> +struct mem_cgroup;
> +#ifdef CONFIG_MEMCG_KMEM
> +struct list_lru_array *lru_alloc_array(void);

Experience teaches it that it is often a mistake for callees to assume
they will always be called in GFP_KERNEL context.  For high-level init
code we can usually get away with it, but I do think that the decision
to not provide a gfp_t argument should be justfied up-front, and that
this restriction should be mentioned in the interface documentation
(when it is written ;)).

>
> ...
>
> @@ -163,18 +168,97 @@ list_lru_dispose_all(
>  	return total;
>  }
>  
> -int
> -list_lru_init(
> -	struct list_lru	*lru)
> +/*
> + * This protects the list of all LRU in the system. One only needs
> + * to take when registering an LRU, or when duplicating the list of lrus.

That isn't very grammatical.

> + * Transversing an LRU can and should be done outside the lock
> + */
> +static DEFINE_MUTEX(all_memcg_lrus_mutex);
> +static LIST_HEAD(all_memcg_lrus);
> +
> +static void list_lru_init_one(struct list_lru_node *lru)
>  {
> +	spin_lock_init(&lru->lock);
> +	INIT_LIST_HEAD(&lru->list);
> +	lru->nr_items = 0;
> +}
> +
> +struct list_lru_array *lru_alloc_array(void)
> +{
> +	struct list_lru_array *lru_array;
>  	int i;
>  
> -	nodes_clear(lru->active_nodes);
> -	for (i = 0; i < MAX_NUMNODES; i++) {
> -		spin_lock_init(&lru->node[i].lock);
> -		INIT_LIST_HEAD(&lru->node[i].list);
> -		lru->node[i].nr_items = 0;
> +	lru_array = kzalloc(nr_node_ids * sizeof(struct list_lru_node),
> +				GFP_KERNEL);

Could use kcalloc() here.

> +	if (!lru_array)
> +		return NULL;
> +
> +	for (i = 0; i < nr_node_ids; i++)
> +		list_lru_init_one(&lru_array->node[i]);
> +
> +	return lru_array;
> +}
> +
> +#ifdef CONFIG_MEMCG_KMEM
> +int __memcg_init_lru(struct list_lru *lru)
> +{
> +	int ret;
> +
> +	INIT_LIST_HEAD(&lru->lrus);
> +	mutex_lock(&all_memcg_lrus_mutex);
> +	list_add(&lru->lrus, &all_memcg_lrus);
> +	ret = memcg_new_lru(lru);
> +	mutex_unlock(&all_memcg_lrus_mutex);
> +	return ret;
> +}
> +
> +int memcg_update_all_lrus(unsigned long num)
> +{
> +	int ret = 0;
> +	struct list_lru *lru;
> +
> +	mutex_lock(&all_memcg_lrus_mutex);
> +	list_for_each_entry(lru, &all_memcg_lrus, lrus) {
> +		ret = memcg_kmem_update_lru_size(lru, num, false);
> +		if (ret)
> +			goto out;
> +	}
> +out:
> +	mutex_unlock(&all_memcg_lrus_mutex);
> +	return ret;
> +}
> +
> +void list_lru_destroy(struct list_lru *lru)

This is a memcg-specific function (which lives in lib/list_lru.c!) and
hence should be called, say, memcg_list_lru_destroy().

> +{
> +	mutex_lock(&all_memcg_lrus_mutex);
> +	list_del(&lru->lrus);
> +	mutex_unlock(&all_memcg_lrus_mutex);
> +}
> +
> +void memcg_destroy_all_lrus(struct mem_cgroup *memcg)
> +{
> +	struct list_lru *lru;
> +	mutex_lock(&all_memcg_lrus_mutex);
> +	list_for_each_entry(lru, &all_memcg_lrus, lrus) {
> +		kfree(lru->memcg_lrus[memcg_cache_id(memcg)]);
> +		lru->memcg_lrus[memcg_cache_id(memcg)] = NULL;

Some common-subexpression-elimination-by-hand would probably improve
the output code here.

> +		/* everybody must beaware that this memcg is no longer valid */

"be aware"

> +		wmb();

The code implies that other code paths can come in here and start
playing with the pointer without taking all_memcg_lrus_mutex?  If so,
where, how why, etc?

I'd be more confortable if the sequence was something like

	lru->memcg_lrus[memcg_cache_id(memcg)] = NULL;
	wmb();
	kfree(lru->memcg_lrus[memcg_cache_id(memcg)]);

but that still has holes and is still scary.


What's going on here?

>  	}
> +	mutex_unlock(&all_memcg_lrus_mutex);
> +}
> +#endif
> +
> +int __list_lru_init(struct list_lru *lru, bool memcg_enabled)
> +{
> +	int i;
> +
> +	nodes_clear(lru->active_nodes);
> +	for (i = 0; i < MAX_NUMNODES; i++)
> +		list_lru_init_one(&lru->node[i]);
> +
> +	if (memcg_enabled)
> +		return memcg_init_lru(lru);

OK, this is weird.  list_lru.c calls into a memcg initialisation
function!  That memcg initialisation function then calls into
list_lru.c stuff, as expected.

Seems screwed up.  What's going on here?

>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(list_lru_init);
> +EXPORT_SYMBOL_GPL(__list_lru_init);
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 27af2d1..5d31b4a 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3163,16 +3163,30 @@ int memcg_update_cache_sizes(struct mem_cgroup *memcg)
>  	memcg_kmem_set_activated(memcg);
>  
>  	ret = memcg_update_all_caches(num+1);
> -	if (ret) {
> -		ida_simple_remove(&kmem_limited_groups, num);
> -		memcg_kmem_clear_activated(memcg);
> -		return ret;
> -	}
> +	if (ret)
> +		goto out;
> +
> +	/*
> +	 * We should make sure that the array size is not updated until we are
> +	 * done; otherwise we have no easy way to know whether or not we should
> +	 * grow the array.
> +	 */

What's the locking here, to prevent concurrent array-resizers?

> +	ret = memcg_update_all_lrus(num + 1);
> +	if (ret)
> +		goto out;
>  
>  	memcg->kmemcg_id = num;
> +
> +	memcg_update_array_size(num + 1);
> +
>  	INIT_LIST_HEAD(&memcg->memcg_slab_caches);
>  	mutex_init(&memcg->slab_caches_mutex);
> +
>  	return 0;
> +out:
> +	ida_simple_remove(&kmem_limited_groups, num);
> +	memcg_kmem_clear_activated(memcg);
> +	return ret;
>  }
>  
>  static size_t memcg_caches_array_size(int num_groups)
> @@ -3254,6 +3268,129 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
>  	return 0;
>  }
>  
> +/*
> + * memcg_kmem_update_lru_size - fill in kmemcg info into a list_lru
> + *
> + * @lru: the lru we are operating with
> + * @num_groups: how many kmem-limited cgroups we have
> + * @new_lru: true if this is a new_lru being created, false if this
> + * was triggered from the memcg side
> + *
> + * Returns 0 on success, and an error code otherwise.
> + *
> + * This function can be called either when a new kmem-limited memcg appears,
> + * or when a new list_lru is created. The work is roughly the same in two cases,

"both cases"

> + * but in the later we never have to expand the array size.

"latter"

> + *
> + * This is always protected by the all_lrus_mutex from the list_lru side.  But
> + * a race can still exists if a new memcg becomes kmem limited at the same time

"exist"

> + * that we are registering a new memcg. Creation is protected by the
> + * memcg_mutex, so the creation of a new lru have to be protected by that as

"has"

> + * well.
> + *
> + * The lock ordering is that the memcg_mutex needs to be acquired before the
> + * lru-side mutex.

It's nice to provide the C name of this "lru-side mutex".

> + */

This purports to be a kerneldoc comment, but it doesn't start with the
kerneldoc /** token.  Please review the entire patchset for this
(common) oddity.

> +int memcg_kmem_update_lru_size(struct list_lru *lru, int num_groups,
> +			       bool new_lru)
> +{
> +	struct list_lru_array **new_lru_array;
> +	struct list_lru_array *lru_array;
> +
> +	lru_array = lru_alloc_array();
> +	if (!lru_array)
> +		return -ENOMEM;
> +
> +	/*
> +	 * When a new LRU is created, we still need to update all data for that
> +	 * LRU. The procedure for late LRUs and new memcgs are quite similar, we

"procedures"

> +	 * only need to make sure we get into the loop even if num_groups <
> +	 * memcg_limited_groups_array_size.

This sentence is hard to follow.  Particularly the "even if" part. 
Rework it?

> +	 */
> +	if ((num_groups > memcg_limited_groups_array_size) || new_lru) {
> +		int i;
> +		struct list_lru_array **old_array;
> +		size_t size = memcg_caches_array_size(num_groups);
> +		int num_memcgs = memcg_limited_groups_array_size;
> +
> +		new_lru_array = kzalloc(size * sizeof(void *), GFP_KERNEL);

Could use kcalloc().

What are the implications of that GFP_KERNEL?  That we cannot take
memcg_mutex and "the lru-side mutex" on the direct reclaim -> shrink
codepaths.  Is that honoured?  Any other potential problems here?

> +		if (!new_lru_array) {
> +			kfree(lru_array);
> +			return -ENOMEM;
> +		}
> +
> +		for (i = 0; lru->memcg_lrus && (i < num_memcgs); i++) {
> +			if (lru->memcg_lrus && lru->memcg_lrus[i])
> +				continue;
> +			new_lru_array[i] =  lru->memcg_lrus[i];
> +		}
> +
> +		old_array = lru->memcg_lrus;
> +		lru->memcg_lrus = new_lru_array;
> +		/*
> +		 * We don't need a barrier here because we are just copying
> +		 * information over. Anybody operating in memcg_lrus will

s/in/on/

> +		 * either follow the new array or the old one and they contain
> +		 * exactly the same information. The new space in the end is

s/in/at/

> +		 * always empty anyway.
> +		 */
> +		if (lru->memcg_lrus)
> +			kfree(old_array);
> +	}
> +
> +	if (lru->memcg_lrus) {
> +		lru->memcg_lrus[num_groups - 1] = lru_array;
> +		/*
> +		 * Here we do need the barrier, because of the state transition
> +		 * implied by the assignment of the array. All users should be
> +		 * able to see it
> +		 */
> +		wmb();

Am worried about this lockless concurrency stuff.  Perhaps putting a
description of the overall design somewhere would be sensible.

> +	}
> +	return 0;
> +}
> +
> +/*
> + * This is called with the LRU-mutex being held.

That's "all_memcg_lrus_mutex", yes?  Not "all_lrus_mutex".  Clear as mud :(

> + */
> +int memcg_new_lru(struct list_lru *lru)
> +{
> +	struct mem_cgroup *iter;
> +
> +	if (!memcg_kmem_enabled())
> +		return 0;

So the caller took all_memcg_lrus_mutex needlessly in this case.  Could
be optimised.

> +	for_each_mem_cgroup(iter) {
> +		int ret;
> +		int memcg_id = memcg_cache_id(iter);
> +		if (memcg_id < 0)
> +			continue;
> +
> +		ret = memcg_kmem_update_lru_size(lru, memcg_id + 1, true);
> +		if (ret) {
> +			mem_cgroup_iter_break(root_mem_cgroup, iter);
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
> +/*
> + * We need to call back and forth from memcg to LRU because of the lock
> + * ordering.  This complicates the flow a little bit, but since the memcg mutex

"the memcg mutex" is named... what?

> + * is held through the whole duration of memcg creation, we need to hold it
> + * before we hold the LRU-side mutex in the case of a new list creation as

"LRU-side mutex" has a name?

> + * well.
> + */
>
> ...
>

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