Message-ID: <1398431536-22391-1-git-send-email-vdavydov@parallels.com>
Date:	Fri, 25 Apr 2014 17:12:16 +0400
From:	Vladimir Davydov <vdavydov@...allels.com>
To:	<akpm@...ux-foundation.org>
CC:	<mhocko@...e.cz>, <hannes@...xchg.org>, <glommer@...il.com>,
	<cl@...ux-foundation.org>, <penberg@...nel.org>,
	<linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>,
	<devel@...nvz.org>
Subject: [PATCH -mm v1.1] memcg: cleanup kmem_id-related naming

The naming of mem_cgroup->kmemcg_id-related functions is rather
inconsistent. We tend to use cache_id as part of their names, but it
isn't quite right, because kmem_id isn't something specific to kmem
caches. It can be used for indexing any array that stores per memcg
data. For instance, we will use it to make list_lru per memcg in the
future. So let's clean up the names and comments related to kmem_id.

Brief change log:

** old name **                          ** new name **

mem_cgroup->kmemcg_id                   mem_cgroup->kmem_id

memcg_init_cache_id()			memcg_init_kmem_id()
memcg_cache_id()                        memcg_kmem_id()
cache_from_memcg_idx()                  kmem_cache_of_memcg_by_id()
cache_from_memcg_idx(memcg_cache_id())  kmem_cache_of_memcg()

for_each_memcg_cache_index()            for_each_possible_memcg_kmem_id()

memcg_limited_groups                    memcg_kmem_ida
memcg_limited_groups_array_size         memcg_nr_kmem_ids_max

MEMCG_CACHES_MIN_SIZE                   <constant inlined>
MEMCG_CACHES_MAX_SIZE                   MEMCG_KMEM_ID_MAX + 1

Signed-off-by: Vladimir Davydov <vdavydov@...allels.com>
---
Changes in v1.1:
 - rename memcg_init_cache_id -> memcg_init_kmem_id
   (lost that hunk somehow in v1)

 include/linux/memcontrol.h |   19 +++-----
 mm/memcontrol.c            |  114 ++++++++++++++++++++------------------------
 mm/slab.c                  |    4 +-
 mm/slab.h                  |   24 +++++++---
 mm/slab_common.c           |   10 ++--
 mm/slub.c                  |   10 ++--
 6 files changed, 90 insertions(+), 91 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 3ee73da2991b..4080a6418c72 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -458,15 +458,10 @@ static inline void sock_release_memcg(struct sock *sk)
 #ifdef CONFIG_MEMCG_KMEM
 extern struct static_key memcg_kmem_enabled_key;
 
-extern int memcg_limited_groups_array_size;
+extern int memcg_nr_kmem_ids_max;
 
-/*
- * Helper macro to loop through all memcg-specific caches. Callers must still
- * check if the cache is valid (it is either valid or NULL).
- * the slab_mutex must be held when looping through those caches
- */
-#define for_each_memcg_cache_index(_idx)	\
-	for ((_idx) = 0; (_idx) < memcg_limited_groups_array_size; (_idx)++)
+#define for_each_possible_memcg_kmem_id(id) \
+	for ((id) = 0; (id) < memcg_nr_kmem_ids_max; (id)++)
 
 static inline bool memcg_kmem_enabled(void)
 {
@@ -490,7 +485,7 @@ void __memcg_kmem_commit_charge(struct page *page,
 				       struct mem_cgroup *memcg, int order);
 void __memcg_kmem_uncharge_pages(struct page *page, int order);
 
-int memcg_cache_id(struct mem_cgroup *memcg);
+int memcg_kmem_id(struct mem_cgroup *memcg);
 
 struct kmem_cache *
 __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp);
@@ -591,8 +586,8 @@ memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
 	return __memcg_kmem_get_cache(cachep, gfp);
 }
 #else
-#define for_each_memcg_cache_index(_idx)	\
-	for (; NULL; )
+#define for_each_possible_memcg_kmem_id(id) \
+	for ((id) = 0; 0; )
 
 static inline bool memcg_kmem_enabled(void)
 {
@@ -614,7 +609,7 @@ memcg_kmem_commit_charge(struct page *page, struct mem_cgroup *memcg, int order)
 {
 }
 
-static inline int memcg_cache_id(struct mem_cgroup *memcg)
+static inline int memcg_kmem_id(struct mem_cgroup *memcg)
 {
 	return -1;
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index bdd3d373cdca..7ecbae9b7800 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -357,11 +357,22 @@ struct mem_cgroup {
 	struct cg_proto tcp_mem;
 #endif
 #if defined(CONFIG_MEMCG_KMEM)
+	/*
+	 * Each kmem-limited memory cgroup has a unique id. We use it for
+	 * indexing the arrays that store per cgroup data. An example of such
+	 * an array is kmem_cache->memcg_params->memcg_caches.
+	 *
+	 * We introduce a separate id instead of using cgroup->id to avoid
+	 * waste of memory in sparse environments, where we have a lot of
+	 * memory cgroups, but only a few of them are kmem-limited.
+	 *
+	 * For unlimited cgroups kmem_id equals -1.
+	 */
+	int kmem_id;
+
 	/* analogous to slab_common's slab_caches list, but per-memcg;
 	 * protected by memcg_slab_mutex */
 	struct list_head memcg_slab_caches;
-        /* Index in the kmem_cache->memcg_params->memcg_caches array */
-	int kmemcg_id;
 #endif
 
 	int last_scanned_node;
@@ -610,35 +621,25 @@ static void disarm_sock_keys(struct mem_cgroup *memcg)
 #endif
 
 #ifdef CONFIG_MEMCG_KMEM
+/* used for mem_cgroup->kmem_id allocations */
+static DEFINE_IDA(memcg_kmem_ida);
+
 /*
- * This will be the memcg's index in each cache's ->memcg_params->memcg_caches.
- * The main reason for not using cgroup id for this:
- *  this works better in sparse environments, where we have a lot of memcgs,
- *  but only a few kmem-limited. Or also, if we have, for instance, 200
- *  memcgs, and none but the 200th is kmem-limited, we'd have to have a
- *  200 entry array for that.
- *
- * The current size of the caches array is stored in
- * memcg_limited_groups_array_size.  It will double each time we have to
- * increase it.
+ * Max kmem id should be as large as max cgroup id so that we could enable
+ * kmem-accounting for each memory cgroup.
  */
-static DEFINE_IDA(kmem_limited_groups);
-int memcg_limited_groups_array_size;
+#define MEMCG_KMEM_ID_MAX	MEM_CGROUP_ID_MAX
 
 /*
- * MIN_SIZE is different than 1, because we would like to avoid going through
- * the alloc/free process all the time. In a small machine, 4 kmem-limited
- * cgroups is a reasonable guess. In the future, it could be a parameter or
- * tunable, but that is strictly not necessary.
+ * We keep the maximal number of kmem ids that may exist in the system in the
+ * memcg_nr_kmem_ids_max variable. We use it for the size of the arrays indexed
+ * by kmem id (see the mem_cgroup->kmem_id definition).
  *
- * MAX_SIZE should be as large as the number of cgrp_ids. Ideally, we could get
- * this constant directly from cgroup, but it is understandable that this is
- * better kept as an internal representation in cgroup.c. In any case, the
- * cgrp_id space is not getting any smaller, and we don't have to necessarily
- * increase ours as well if it increases.
+ * If a newly allocated kmem id is greater or equal to memcg_nr_kmem_ids_max,
+ * we double it and reallocate the arrays so that they have enough space to
+ * store data for the new cgroup.
  */
-#define MEMCG_CACHES_MIN_SIZE 4
-#define MEMCG_CACHES_MAX_SIZE MEM_CGROUP_ID_MAX
+int memcg_nr_kmem_ids_max;
 
 /*
  * A lot of the calls to the cache allocation functions are expected to be
@@ -653,7 +654,7 @@ static void disarm_kmem_keys(struct mem_cgroup *memcg)
 {
 	if (memcg_kmem_is_active(memcg)) {
 		static_key_slow_dec(&memcg_kmem_enabled_key);
-		ida_simple_remove(&kmem_limited_groups, memcg->kmemcg_id);
+		ida_simple_remove(&memcg_kmem_ida, memcg->kmem_id);
 	}
 	/*
 	 * This check can't live in kmem destruction function,
@@ -2930,11 +2931,8 @@ static inline bool memcg_can_account_kmem(struct mem_cgroup *memcg)
  */
 static struct kmem_cache *memcg_params_to_cache(struct memcg_cache_params *p)
 {
-	struct kmem_cache *cachep;
-
 	VM_BUG_ON(p->is_root_cache);
-	cachep = p->root_cache;
-	return cache_from_memcg_idx(cachep, memcg_cache_id(p->memcg));
+	return kmem_cache_of_memcg(p->root_cache, p->memcg);
 }
 
 #ifdef CONFIG_SLABINFO
@@ -3017,29 +3015,24 @@ static void memcg_uncharge_kmem(struct mem_cgroup *memcg, u64 size)
 		css_put(&memcg->css);
 }
 
-/*
- * 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)
+int memcg_kmem_id(struct mem_cgroup *memcg)
 {
-	return memcg ? memcg->kmemcg_id : -1;
+	return memcg ? memcg->kmem_id : -1;
 }
 
-static int memcg_init_cache_id(struct mem_cgroup *memcg)
+static int memcg_init_kmem_id(struct mem_cgroup *memcg)
 {
 	int err = 0;
 	int id, size;
 
 	lockdep_assert_held(&activate_kmem_mutex);
 
-	id = ida_simple_get(&kmem_limited_groups,
-			    0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL);
+	id = ida_simple_get(&memcg_kmem_ida,
+			    0, MEMCG_KMEM_ID_MAX + 1, GFP_KERNEL);
 	if (id < 0)
 		return id;
 
-	if (id < memcg_limited_groups_array_size)
+	if (id < memcg_nr_kmem_ids_max)
 		goto out_setid;
 
 	/*
@@ -3047,10 +3040,10 @@ static int memcg_init_cache_id(struct mem_cgroup *memcg)
 	 * per memcg data. Let's try to grow them then.
 	 */
 	size = id * 2;
-	if (size < MEMCG_CACHES_MIN_SIZE)
-		size = MEMCG_CACHES_MIN_SIZE;
-	else if (size > MEMCG_CACHES_MAX_SIZE)
-		size = MEMCG_CACHES_MAX_SIZE;
+	if (size < 4)
+		size = 4; /* a good number to start with */
+	if (size > MEMCG_KMEM_ID_MAX + 1)
+		size = MEMCG_KMEM_ID_MAX + 1;
 
 	mutex_lock(&memcg_slab_mutex);
 	err = kmem_cache_grow_memcg_arrays(size);
@@ -3064,14 +3057,14 @@ static int memcg_init_cache_id(struct mem_cgroup *memcg)
 	 * walking over such an array won't get an index out of range provided
 	 * they use an appropriate mutex to protect the array's elements.
 	 */
-	memcg_limited_groups_array_size = size;
+	memcg_nr_kmem_ids_max = size;
 
 out_setid:
-	memcg->kmemcg_id = id;
+	memcg->kmem_id = id;
 	return 0;
 
 out_rmid:
-	ida_simple_remove(&kmem_limited_groups, id);
+	ida_simple_remove(&memcg_kmem_ida, id);
 	return err;
 }
 
@@ -3089,11 +3082,10 @@ static int memcg_prepare_kmem_cache(struct kmem_cache *cachep)
 		return 0;
 
 	/* activate_kmem_mutex guarantees a stable value of
-	 * memcg_limited_groups_array_size */
+	 * memcg_nr_kmem_ids_max */
 	mutex_lock(&activate_kmem_mutex);
 	mutex_lock(&memcg_slab_mutex);
-	ret = kmem_cache_init_memcg_array(cachep,
-			memcg_limited_groups_array_size);
+	ret = kmem_cache_init_memcg_array(cachep, memcg_nr_kmem_ids_max);
 	mutex_unlock(&memcg_slab_mutex);
 	mutex_unlock(&activate_kmem_mutex);
 	return ret;
@@ -3113,14 +3105,14 @@ static void memcg_copy_kmem_cache(struct mem_cgroup *memcg,
 
 	lockdep_assert_held(&memcg_slab_mutex);
 
-	id = memcg_cache_id(memcg);
+	id = memcg_kmem_id(memcg);
 
 	/*
 	 * Since per-memcg caches are created asynchronously on first
 	 * allocation (see memcg_kmem_get_cache()), several threads can try to
 	 * create the same cache, but only one of them may succeed.
 	 */
-	if (cache_from_memcg_idx(root_cache, id))
+	if (kmem_cache_of_memcg_by_id(root_cache, id))
 		return;
 
 	params = kzalloc(sizeof(*params), GFP_KERNEL);
@@ -3146,8 +3138,8 @@ static void memcg_copy_kmem_cache(struct mem_cgroup *memcg,
 	list_add(&cachep->memcg_params->list, &memcg->memcg_slab_caches);
 
 	/*
-	 * Since readers won't lock (see cache_from_memcg_idx()), we need a
-	 * barrier here to ensure nobody will see the kmem_cache partially
+	 * Since readers won't lock (see kmem_cache_of_memcg_by_id()), we need
+	 * a barrier here to ensure nobody will see the kmem_cache partially
 	 * initialized.
 	 */
 	smp_wmb();
@@ -3171,7 +3163,7 @@ static int memcg_destroy_kmem_cache_copy(struct kmem_cache *cachep)
 	params = cachep->memcg_params;
 	root_cache = params->root_cache;
 	memcg = params->memcg;
-	id = memcg_cache_id(memcg);
+	id = memcg_kmem_id(memcg);
 
 	/*
 	 * Since memcg_caches arrays can be accessed using only slab_mutex for
@@ -3243,8 +3235,8 @@ int kmem_cache_destroy_memcg_copies(struct kmem_cache *cachep)
 	BUG_ON(!is_root_cache(cachep));
 
 	mutex_lock(&memcg_slab_mutex);
-	for_each_memcg_cache_index(i) {
-		c = cache_from_memcg_idx(cachep, i);
+	for_each_possible_memcg_kmem_id(i) {
+		c = kmem_cache_of_memcg_by_id(cachep, i);
 		if (!c)
 			continue;
 
@@ -3388,7 +3380,7 @@ struct kmem_cache *__memcg_kmem_get_cache(struct kmem_cache *cachep,
 	if (!memcg_can_account_kmem(memcg))
 		goto out;
 
-	memcg_cachep = cache_from_memcg_idx(cachep, memcg_cache_id(memcg));
+	memcg_cachep = kmem_cache_of_memcg(cachep, memcg);
 	if (likely(memcg_cachep)) {
 		cachep = memcg_cachep;
 		goto out;
@@ -4962,7 +4954,7 @@ static int __memcg_activate_kmem(struct mem_cgroup *memcg,
 	if (err)
 		goto out;
 
-	err = memcg_init_cache_id(memcg);
+	err = memcg_init_kmem_id(memcg);
 	if (err)
 		goto out;
 
@@ -5693,7 +5685,7 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
 {
 	int ret;
 
-	memcg->kmemcg_id = -1;
+	memcg->kmem_id = -1;
 	ret = memcg_propagate_kmem(memcg);
 	if (ret)
 		return ret;
diff --git a/mm/slab.c b/mm/slab.c
index 25317fd1daa2..194322a634e2 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3844,8 +3844,8 @@ static int do_tune_cpucache(struct kmem_cache *cachep, int limit,
 		return ret;
 
 	VM_BUG_ON(!mutex_is_locked(&slab_mutex));
-	for_each_memcg_cache_index(i) {
-		c = cache_from_memcg_idx(cachep, i);
+	for_each_possible_memcg_kmem_id(i) {
+		c = kmem_cache_of_memcg_by_id(cachep, i);
 		if (c)
 			/* return value determined by the parent cache only */
 			__do_tune_cpucache(c, limit, batchcount, shared, gfp);
diff --git a/mm/slab.h b/mm/slab.h
index ba834860fbfd..61f833c569e7 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -145,11 +145,11 @@ static inline const char *cache_name(struct kmem_cache *s)
  * created a memcg's cache is destroyed only along with the root cache, it is
  * true if we are going to allocate from the cache or hold a reference to the
  * root cache by other means. Otherwise, we should hold either the slab_mutex
- * or the memcg's slab_caches_mutex while calling this function and accessing
- * the returned value.
+ * or the memcg_slab_mutex while calling this function and accessing the
+ * returned value.
  */
 static inline struct kmem_cache *
-cache_from_memcg_idx(struct kmem_cache *s, int idx)
+kmem_cache_of_memcg_by_id(struct kmem_cache *s, int id)
 {
 	struct kmem_cache *cachep;
 	struct memcg_cache_params *params;
@@ -159,18 +159,24 @@ cache_from_memcg_idx(struct kmem_cache *s, int idx)
 
 	rcu_read_lock();
 	params = rcu_dereference(s->memcg_params);
-	cachep = params->memcg_caches[idx];
+	cachep = params->memcg_caches[id];
 	rcu_read_unlock();
 
 	/*
 	 * Make sure we will access the up-to-date value. The code updating
 	 * memcg_caches issues a write barrier to match this (see
-	 * memcg_register_cache()).
+	 * memcg_copy_kmem_cache()).
 	 */
 	smp_read_barrier_depends();
 	return cachep;
 }
 
+static inline struct kmem_cache *
+kmem_cache_of_memcg(struct kmem_cache *s, struct mem_cgroup *memcg)
+{
+	return kmem_cache_of_memcg_by_id(s, memcg_kmem_id(memcg));
+}
+
 static inline struct kmem_cache *memcg_root_cache(struct kmem_cache *s)
 {
 	if (is_root_cache(s))
@@ -214,7 +220,13 @@ static inline const char *cache_name(struct kmem_cache *s)
 }
 
 static inline struct kmem_cache *
-cache_from_memcg_idx(struct kmem_cache *s, int idx)
+kmem_cache_of_memcg_by_id(struct kmem_cache *s, int id)
+{
+	return NULL;
+}
+
+static inline struct kmem_cache *
+kmem_cache_of_memcg(struct kmem_cache *s, struct mem_cgroup *memcg)
 {
 	return NULL;
 }
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 36d9b866a3ab..beaaaecd35f4 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -94,11 +94,11 @@ static int kmem_cache_realloc_memcg_array(struct kmem_cache *s, int nr_entries)
 
 	new->is_root_cache = true;
 	if (old) {
-		for_each_memcg_cache_index(i)
+		for_each_possible_memcg_kmem_id(i)
 			new->memcg_caches[i] = old->memcg_caches[i];
 	}
 
-	/* matching rcu_dereference is in cache_from_memcg_idx */
+	/* matching rcu_dereference is in kmem_cache_of_memcg_by_id */
 	rcu_assign_pointer(s->memcg_params, new);
 	if (old)
 		kfree_rcu(old, rcu_head);
@@ -327,7 +327,7 @@ kmem_cache_request_memcg_copy(struct memcg_cache_params *memcg_params,
 	mutex_lock(&slab_mutex);
 
 	cache_name = kasprintf(GFP_KERNEL, "%s(%d:%s)", root_cache->name,
-			       memcg_cache_id(memcg), memcg_name);
+			       memcg_kmem_id(memcg), memcg_name);
 	if (!cache_name)
 		goto out_unlock;
 
@@ -748,8 +748,8 @@ memcg_accumulate_slabinfo(struct kmem_cache *s, struct slabinfo *info)
 	if (!is_root_cache(s))
 		return;
 
-	for_each_memcg_cache_index(i) {
-		c = cache_from_memcg_idx(s, i);
+	for_each_possible_memcg_kmem_id(i) {
+		c = kmem_cache_of_memcg_by_id(s, i);
 		if (!c)
 			continue;
 
diff --git a/mm/slub.c b/mm/slub.c
index aa30932c5190..006e6bfe257c 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3772,8 +3772,8 @@ __kmem_cache_alias(const char *name, size_t size, size_t align,
 		s->object_size = max(s->object_size, (int)size);
 		s->inuse = max_t(int, s->inuse, ALIGN(size, sizeof(void *)));
 
-		for_each_memcg_cache_index(i) {
-			c = cache_from_memcg_idx(s, i);
+		for_each_possible_memcg_kmem_id(i) {
+			c = kmem_cache_of_memcg_by_id(s, i);
 			if (!c)
 				continue;
 			c->object_size = s->object_size;
@@ -5062,8 +5062,8 @@ static ssize_t slab_attr_store(struct kobject *kobj,
 		 * directly either failed or succeeded, in which case we loop
 		 * through the descendants with best-effort propagation.
 		 */
-		for_each_memcg_cache_index(i) {
-			struct kmem_cache *c = cache_from_memcg_idx(s, i);
+		for_each_possible_memcg_kmem_id(i) {
+			struct kmem_cache *c = kmem_cache_of_memcg_by_id(s, i);
 			if (c)
 				attribute->store(c, buf, len);
 		}
@@ -5198,7 +5198,7 @@ static char *create_unique_id(struct kmem_cache *s)
 #ifdef CONFIG_MEMCG_KMEM
 	if (!is_root_cache(s))
 		p += sprintf(p, "-%08d",
-				memcg_cache_id(s->memcg_params->memcg));
+			     memcg_kmem_id(s->memcg_params->memcg));
 #endif
 
 	BUG_ON(p > name + ID_STR_LENGTH - 1);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists