memcg/kmem: cleanup naming and callflows

PATCH Superseded Vladimir Davydov 1 participant 9 messages Apr 25, 2014 – Apr 27, 2014

Subsystems (3)

Memory management memcg Slab allocator

lore

Share & tools

lore

Vladimir Davydov

author

2014-04-25 12:33 UTC #0 lore raw

cover memcg/kmem: cleanup naming and callflows

Hi,

In reply to "[PATCH RFC -mm v2 3/3] memcg, slab: simplify

Quoted 2 lines · synchronization scheme" Johannes

I like this patch, but the API names are confusing. Could we fix up that whole thing by any chance?

(see https://lkml.org/lkml/2014/4/18/317)

So this patch set is about cleaning up memcg/kmem naming.

While preparing it I found that some of the ugly-named functions constituting interface between memcontrol.c and slab_common.c can be neatly got rid of w/o complicating the code. Quite the contrary, w/o them call-flows look much simpler, IMO. So the first four patches do not rename anything actually - they just rework call-flows in kmem cache creation/destruction and memcg_caches arrays relocations paths. Finally, patches 5 and 6 clean up the naming.

Reviews are appreciated.

Thanks,

Vladimir Davydov (6):
memcg: get rid of memcg_create_cache_name
memcg: allocate memcg_caches array on first per memcg cache creation
memcg: cleanup memcg_caches arrays relocation path
memcg: get rid of memcg_{alloc,free}_cache_params
memcg: cleanup kmem cache creation/destruction functions naming
memcg: cleanup kmem_id-related naming

 include/linux/memcontrol.h |   40 +----
 include/linux/slab.h       |   12 +-
 mm/memcontrol.c            |  420 ++++++++++++++++++++------------------------
 mm/slab.c                  |    4 +-
 mm/slab.h                  |   24 ++-
 mm/slab_common.c           |  127 ++++++++++----
 mm/slub.c                  |   10 +-
 7 files changed, 320 insertions(+), 317 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>

Vladimir Davydov

author

2014-04-25 12:33 UTC #1 lore raw

PATCH 1/6 memcg: get rid of memcg_create_cache_name

Instead of calling back to memcontrol.c from kmem_cache_create_memcg in order to just create the name of a per memcg cache, let's allocate it in place. We only need to pass the memcg name to kmem_cache_create_memcg for that - everything else can be done in slab_common.c.

This patch also turns the buffer used to store memcg names to be statically allocated while before it was kmalloc'ed on first use. Saving those 256 bytes isn't worth complicating the code. If someone is so tight on memory, they'd better disable CONFIG_MEMCG_KMEM altogether.

Signed-off-by Vladimir Davydov <[email protected]>

---
 include/linux/memcontrol.h |    2 --
 include/linux/slab.h       |    3 ++-
 mm/memcontrol.c            |   27 +++------------------------
 mm/slab_common.c           |    7 +++++--
 4 files changed, 10 insertions(+), 29 deletions(-)

include/linux/memcontrol.h +0 -2

index 1fa23244fe37..dfc2929a3877 100644
@@ -492,8 +492,6 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order);
 
 int memcg_cache_id(struct mem_cgroup *memcg);
 
-char *memcg_create_cache_name(struct mem_cgroup *memcg,
-			      struct kmem_cache *root_cache);
 int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
 			     struct kmem_cache *root_cache);
 void memcg_free_cache_params(struct kmem_cache *s);

include/linux/slab.h +2 -1

index ecbec9ccb80d..86e5b26fbdab 100644
@@ -117,7 +117,8 @@ struct kmem_cache *kmem_cache_create(const char *, size_t, size_t,
 			void (*)(void *));
 #ifdef CONFIG_MEMCG_KMEM
 struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *,
-					   struct kmem_cache *);
+					   struct kmem_cache *,
+					   const char *);
 #endif
 void kmem_cache_destroy(struct kmem_cache *);
 int kmem_cache_shrink(struct kmem_cache *);

mm/memcontrol.c +3 ~~-24~~

index 19d620b3d69c..605d72044533 100644
@@ -3105,29 +3105,6 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
 	return 0;
 }
 
-char *memcg_create_cache_name(struct mem_cgroup *memcg,
-			      struct kmem_cache *root_cache)
-{
-	static char *buf = NULL;
-
-	/*
-	 * We need a mutex here to protect the shared buffer. Since this is
-	 * expected to be called only on cache creation, we can employ the
-	 * slab_mutex for that purpose.
-	 */
-	lockdep_assert_held(&slab_mutex);
-
-	if (!buf) {
-		buf = kmalloc(NAME_MAX + 1, GFP_KERNEL);
-		if (!buf)
-			return NULL;
-	}
-
-	cgroup_name(memcg->css.cgroup, buf, NAME_MAX + 1);
-	return kasprintf(GFP_KERNEL, "%s(%d:%s)", root_cache->name,
-			 memcg_cache_id(memcg), buf);
-}
-
 int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
 			     struct kmem_cache *root_cache)
 {

@@ -3168,6 +3145,7 @@ void memcg_free_cache_params(struct kmem_cache *s)
 static void memcg_kmem_create_cache(struct mem_cgroup *memcg,
 				    struct kmem_cache *root_cache)
 {
+	static char memcg_name[NAME_MAX + 1];
 	struct kmem_cache *cachep;
 	int id;

@@ -3183,7 +3161,8 @@ static void memcg_kmem_create_cache(struct mem_cgroup *memcg,
 	if (cache_from_memcg_idx(root_cache, id))
 		return;
 
-	cachep = kmem_cache_create_memcg(memcg, root_cache);
+	cgroup_name(memcg->css.cgroup, memcg_name, NAME_MAX + 1);
+	cachep = kmem_cache_create_memcg(memcg, root_cache, memcg_name);
 	/*
 	 * If we could not create a memcg cache, do not complain, because
 	 * that's not critical at all as we can always proceed with the root

mm/slab_common.c +5 -2

index 4f16f374392a..6b6397d2e05e 100644
@@ -264,13 +264,15 @@ EXPORT_SYMBOL(kmem_cache_create);
  * kmem_cache_create_memcg - Create a cache for a memory cgroup.
  * @memcg: The memory cgroup the new cache is for.
  * @root_cache: The parent of the new cache.
+ * @memcg_name: The name of the memory cgroup.
  *
  * This function attempts to create a kmem cache that will serve allocation
  * requests going from @memcg to @root_cache. The new cache inherits properties
  * from its parent.
  */
 struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *memcg,
-					   struct kmem_cache *root_cache)
+					   struct kmem_cache *root_cache,
+					   const char *memcg_name)
 {
 	struct kmem_cache *s = NULL;
 	char *cache_name;

@@ -280,7 +282,8 @@ struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *memcg,
 
 	mutex_lock(&slab_mutex);
 
-	cache_name = memcg_create_cache_name(memcg, root_cache);
+	cache_name = kasprintf(GFP_KERNEL, "%s(%d:%s)", root_cache->name,
+			       memcg_cache_id(memcg), memcg_name);
 	if (!cache_name)
 		goto out_unlock;

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>

Vladimir Davydov

author

2014-04-25 12:33 UTC #2 lore raw

PATCH 2/6 memcg: allocate memcg_caches array on first per memcg cache creation

In reply to Vladimir Davydov

There is no need to allocate the memcg_caches array on kmem cache creation, because we perfectly handle root caches without memcg_params everywhere. Since not all caches will necessarily be used by memory cgroups, this rather looks like a waste of memory. So let's allocate the memcg_caches array on the first per memcg cache creation.

A couple of things to note about this patch:

- after it is applied memcg_{alloc, free}_cache_params do nothing for root caches so that they can be inlined into memcg_kmem_{create, destroy}_cache;

- since memcg_limited_groups_array_size is now accessed under activate_kmem_mutex, we can move memcg_limited_groups_array_size update (memcg_update_array_size) out of slab_mutex, which is nice, because it isn't something specific to kmem caches - it will be used in per memcg list_lru in the future;

- memcg_caches allocation scheme is now spread between slab_common.c and memcontrol.c, but that'll be fixed by the next patches where I'll move it completely to slab_common.c

Signed-off-by Vladimir Davydov <[email protected]>

---
 include/linux/slab.h |    1 +
 mm/memcontrol.c      |   64 +++++++++++++++++++++++++++++++-------------------
 mm/slab_common.c     |   41 ++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 24 deletions(-)

include/linux/slab.h +1 -0

index 86e5b26fbdab..095ce8e47a36 100644
@@ -119,6 +119,7 @@ struct kmem_cache *kmem_cache_create(const char *, size_t, size_t,
 struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *,
 					   struct kmem_cache *,
 					   const char *);
+int kmem_cache_init_memcg_array(struct kmem_cache *, int);
 #endif
 void kmem_cache_destroy(struct kmem_cache *);
 int kmem_cache_shrink(struct kmem_cache *);

mm/memcontrol.c +40 ~~-24~~

112 lines not on this page · raw message

mm/slab_common.c +41 -0

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

Vladimir Davydov

author

2014-04-25 12:33 UTC #3 lore raw

PATCH 3/6 memcg: cleanup memcg_caches arrays relocation path

In reply to Vladimir Davydov

Current memcg_caches arrays relocation path looks rather awkward: on setting kmem limit we call memcg_update_all_caches located on slab's side, which walks over all root caches and calls back to memcontrol.c through memcg_update_cache_size and memcg_update_array_size, which in turn reallocate the memcg_caches array for a particular cache and update the arrays' size respectively.

The first call from memcontrol.c to slab_common.c, which is memcg_update_all_caches, is justified, because to iterate over root caches we have to take the slab_mutex. However, I don't see any reason why we can't reallocate a memcg_caches array on the slab's size or why we should call memcg_update_cache_size under the slab_mutex. So let's clean up this mess.

Signed-off-by Vladimir Davydov <[email protected]>

---
 include/linux/memcontrol.h |    3 --
 include/linux/slab.h       |    3 +-
 mm/memcontrol.c            |  125 +++++++++++++-------------------------------
 mm/slab_common.c           |   36 +++++++------
 4 files changed, 59 insertions(+), 108 deletions(-)

include/linux/memcontrol.h +0 -3

index dfc2929a3877..6e59393e03f9 100644
@@ -496,9 +496,6 @@ int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
 			     struct kmem_cache *root_cache);
 void memcg_free_cache_params(struct kmem_cache *s);
 
-int memcg_update_cache_size(struct kmem_cache *s, int num_groups);
-void memcg_update_array_size(int num_groups);
-
 struct kmem_cache *
 __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp);

include/linux/slab.h +1 -2

index 095ce8e47a36..c08c9667a1e8 100644
@@ -120,6 +120,7 @@ struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *,
 					   struct kmem_cache *,
 					   const char *);
 int kmem_cache_init_memcg_array(struct kmem_cache *, int);
+int kmem_cache_grow_memcg_arrays(int);
 #endif
 void kmem_cache_destroy(struct kmem_cache *);
 int kmem_cache_shrink(struct kmem_cache *);

@@ -545,8 +546,6 @@ struct memcg_cache_params {
 	};
 };
 
-int memcg_update_all_caches(int num_memcgs);
-
 struct seq_file;
 int cache_show(struct kmem_cache *s, struct seq_file *m);
 void print_slabinfo_header(struct seq_file *m);

mm/memcontrol.c +37 ~~-88~~

162 lines not on this page · raw message

mm/slab_common.c +21 ~~-15~~

index 44bfc4b1ee09..a98922acfdc6 100644
@@ -77,10 +77,13 @@ static inline int kmem_cache_sanity_check(const char *name, size_t size)
 #endif
 
 #ifdef CONFIG_MEMCG_KMEM
-static int kmem_cache_alloc_memcg_array(struct kmem_cache *s, int nr_entries)
+static int kmem_cache_realloc_memcg_array(struct kmem_cache *s, int nr_entries)
 {
+	int i;
 	size_t size;
-	struct memcg_cache_params *new;
+	struct memcg_cache_params *new, *old;
+
+	old = s->memcg_params;
 
 	size = offsetof(struct memcg_cache_params, memcg_caches);
 	size += nr_entries * sizeof(void *);

@@ -90,10 +93,15 @@ static int kmem_cache_alloc_memcg_array(struct kmem_cache *s, int nr_entries)
 		return -ENOMEM;
 
 	new->is_root_cache = true;
+	if (old) {
+		for_each_memcg_cache_index(i)
+			new->memcg_caches[i] = old->memcg_caches[i];
+	}
 
 	/* matching rcu_dereference is in cache_from_memcg_idx */
 	rcu_assign_pointer(s->memcg_params, new);
-
+	if (old)
+		kfree_rcu(old, rcu_head);
 	return 0;
 }

@@ -111,37 +119,35 @@ int kmem_cache_init_memcg_array(struct kmem_cache *s, int nr_entries)
 
 	mutex_lock(&slab_mutex);
 	if (!s->memcg_params)
-		ret = kmem_cache_alloc_memcg_array(s, nr_entries);
+		ret = kmem_cache_realloc_memcg_array(s, nr_entries);
 	mutex_unlock(&slab_mutex);
 	return ret;
 }
 
-int memcg_update_all_caches(int num_memcgs)
+int kmem_cache_grow_memcg_arrays(int nr_entries)
 {
 	struct kmem_cache *s;
 	int ret = 0;
-	mutex_lock(&slab_mutex);
 
+	mutex_lock(&slab_mutex);
 	list_for_each_entry(s, &slab_caches, list) {
 		if (!is_root_cache(s))
 			continue;
 
-		ret = memcg_update_cache_size(s, num_memcgs);
+		ret = kmem_cache_realloc_memcg_array(s, nr_entries);
 		/*
-		 * See comment in memcontrol.c, memcg_update_cache_size:
-		 * Instead of freeing the memory, we'll just leave the caches
-		 * up to this point in an updated state.
+		 * We won't shrink the arrays back to the initial size on
+		 * failure, because it isn't a big deal if some caches are left
+		 * with a size greater than others. Further updates will reset
+		 * this anyway.
 		 */
 		if (ret)
-			goto out;
+			break;
 	}
-
-	memcg_update_array_size(num_memcgs);
-out:
 	mutex_unlock(&slab_mutex);
 	return ret;
 }
-#endif
+#endif /* CONFIG_MEMCG_KMEM */
 
 /*
  * Figure out what the alignment of the objects will be given a set of

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>

Vladimir Davydov

author

2014-04-25 12:33 UTC #4 lore raw

PATCH 4/6 memcg: get rid of memcg_{alloc,free}_cache_params

In reply to Vladimir Davydov

Let's inline them to memcg_kmem_{create,destroy}_cache to keep the interface between slab_common.c and memcontrol.c clean.

Note, to properly bail out in memcg_kmem_destroy_cache, I need to know if the cache was actually destroyed (it won't if it has leaked objects). That's why I add the return value to kmem_cache_destroy.

Signed-off-by Vladimir Davydov <[email protected]>

---
 include/linux/memcontrol.h |   14 ---------
 include/linux/slab.h       |    7 +++--
 mm/memcontrol.c            |   74 +++++++++++++++++++++++---------------------
 mm/slab_common.c           |   41 +++++++++++++++---------
 4 files changed, 69 insertions(+), 67 deletions(-)

include/linux/memcontrol.h +0 ~~-14~~

index 6e59393e03f9..3aee79fc7876 100644
@@ -492,10 +492,6 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order);
 
 int memcg_cache_id(struct mem_cgroup *memcg);
 
-int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
-			     struct kmem_cache *root_cache);
-void memcg_free_cache_params(struct kmem_cache *s);
-
 struct kmem_cache *
 __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp);

@@ -623,16 +619,6 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg)
 	return -1;
 }
 
-static inline int memcg_alloc_cache_params(struct mem_cgroup *memcg,
-		struct kmem_cache *s, struct kmem_cache *root_cache)
-{
-	return 0;
-}
-
-static inline void memcg_free_cache_params(struct kmem_cache *s)
-{
-}
-
 static inline struct kmem_cache *
 memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
 {

include/linux/slab.h +4 -3

index c08c9667a1e8..22cc9cab4279 100644
@@ -106,6 +106,8 @@
 #include <linux/kmemleak.h>
 
 struct mem_cgroup;
+struct memcg_cache_params;
+
 /*
  * struct kmem_cache related prototypes
  */

@@ -116,13 +118,12 @@ struct kmem_cache *kmem_cache_create(const char *, size_t, size_t,
 			unsigned long,
 			void (*)(void *));
 #ifdef CONFIG_MEMCG_KMEM
-struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *,
-					   struct kmem_cache *,
+struct kmem_cache *kmem_cache_create_memcg(struct memcg_cache_params *,
 					   const char *);
 int kmem_cache_init_memcg_array(struct kmem_cache *, int);
 int kmem_cache_grow_memcg_arrays(int);
 #endif
-void kmem_cache_destroy(struct kmem_cache *);
+int kmem_cache_destroy(struct kmem_cache *);
 int kmem_cache_shrink(struct kmem_cache *);
 void kmem_cache_free(struct kmem_cache *, void *);

mm/memcontrol.c +39 ~~-35~~

127 lines not on this page · raw message

mm/slab_common.c +26 ~~-15~~

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

Vladimir Davydov

author

2014-04-25 12:33 UTC #5 lore raw

PATCH 5/6 memcg: cleanup kmem cache creation/destruction functions naming

In reply to Vladimir Davydov

Current names are rather confusing. Let's try to improve them.

Brief change log:

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

kmem_cache_create_memcg                 kmem_cache_request_memcg_copy
memcg_kmem_create_cache                 memcg_copy_kmem_cache
memcg_kmem_destroy_cache                memcg_destroy_kmem_cache_copy

__kmem_cache_destroy_memcg_children     kmem_cache_destroy_memcg_copies
kmem_cache_destroy_memcg_children       kmem_cache_destroy_memcg_array
mem_cgroup_destroy_all_caches           memcg_destroy_kmem_cache_copies

create_work                             memcg_kmem_cache_copy_work
memcg_create_cache_work_func            memcg_kmem_cache_copy_work_func
memcg_create_cache_enqueue              memcg_schedule_kmem_cache_copy

Signed-off-by Vladimir Davydov <[email protected]>

---
 include/linux/memcontrol.h |    2 +-
 include/linux/slab.h       |    4 +-
 mm/memcontrol.c            |   92 +++++++++++++++++++++++++-------------------
 mm/slab_common.c           |   14 +++----
 4 files changed, 62 insertions(+), 50 deletions(-)

include/linux/memcontrol.h +1 -1

index 3aee79fc7876..3ee73da2991b 100644
@@ -498,7 +498,7 @@ __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp);
 int __memcg_charge_slab(struct kmem_cache *cachep, gfp_t gfp, int order);
 void __memcg_uncharge_slab(struct kmem_cache *cachep, int order);
 
-int __kmem_cache_destroy_memcg_children(struct kmem_cache *s);
+int kmem_cache_destroy_memcg_copies(struct kmem_cache *cachep);
 
 /**
  * memcg_kmem_newpage_charge: verify if a new kmem allocation is allowed.

include/linux/slab.h +2 -2

index 22cc9cab4279..1f22c6130f1a 100644
@@ -118,8 +118,8 @@ struct kmem_cache *kmem_cache_create(const char *, size_t, size_t,
 			unsigned long,
 			void (*)(void *));
 #ifdef CONFIG_MEMCG_KMEM
-struct kmem_cache *kmem_cache_create_memcg(struct memcg_cache_params *,
-					   const char *);
+struct kmem_cache *kmem_cache_request_memcg_copy(struct memcg_cache_params *,
+						 const char *);
 int kmem_cache_init_memcg_array(struct kmem_cache *, int);
 int kmem_cache_grow_memcg_arrays(int);
 #endif

mm/memcontrol.c +52 ~~-40~~

202 lines not on this page · raw message

mm/slab_common.c +7 -7

index cb4e2293ec46..36d9b866a3ab 100644
@@ -304,7 +304,7 @@ EXPORT_SYMBOL(kmem_cache_create);
 
 #ifdef CONFIG_MEMCG_KMEM
 /*
- * kmem_cache_create_memcg - Create a cache for a memory cgroup.
+ * kmem_cache_request_memcg_copy - Create a cache for a memory cgroup.
  * @memcg_params: The memcg params to initialize the cache with.
  * @memcg_name: The name of the memory cgroup.
  *

@@ -313,8 +313,8 @@ EXPORT_SYMBOL(kmem_cache_create);
  * from its parent.
  */
 struct kmem_cache *
-kmem_cache_create_memcg(struct memcg_cache_params *memcg_params,
-			const char *memcg_name)
+kmem_cache_request_memcg_copy(struct memcg_cache_params *memcg_params,
+			      const char *memcg_name)
 {
 	struct mem_cgroup *memcg = memcg_params->memcg;
 	struct kmem_cache *root_cache = memcg_params->root_cache;

@@ -349,7 +349,7 @@ out_unlock:
 	return s;
 }
 
-static int kmem_cache_destroy_memcg_children(struct kmem_cache *s)
+static int kmem_cache_destroy_memcg_array(struct kmem_cache *s)
 {
 	int rc;

@@ -358,7 +358,7 @@ static int kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 		return 0;
 
 	mutex_unlock(&slab_mutex);
-	rc = __kmem_cache_destroy_memcg_children(s);
+	rc = kmem_cache_destroy_memcg_copies(s);
 	mutex_lock(&slab_mutex);
 
 	if (rc == 0)

@@ -366,7 +366,7 @@ static int kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 	return rc;
 }
 #else
-static int kmem_cache_destroy_memcg_children(struct kmem_cache *s)
+static int kmem_cache_destroy_memcg_array(struct kmem_cache *s)
 {
 	return 0;
 }

@@ -393,7 +393,7 @@ int kmem_cache_destroy(struct kmem_cache *s)
 		goto out_unlock;
 
 	ret = -EBUSY;
-	if (kmem_cache_destroy_memcg_children(s) != 0)
+	if (kmem_cache_destroy_memcg_array(s) != 0)
 		goto out_unlock;
 
 	list_del(&s->list);

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>

Vladimir Davydov

author

2014-04-25 12:33 UTC #6 lore raw

PATCH 6/6 memcg: cleanup kmem_id-related naming

In reply to Vladimir Davydov

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 <[email protected]>

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

include/linux/memcontrol.h +7 ~~-12~~

index 3ee73da2991b..4080a6418c72 100644
@@ -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;
 }

mm/memcontrol.c +51 ~~-59~~

227 lines not on this page · raw message

mm/slab.c +2 -2

index 25317fd1daa2..194322a634e2 100644
@@ -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);

mm/slab.h +18 -6

index ba834860fbfd..61f833c569e7 100644
@@ -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;
 }

mm/slab_common.c +5 -5

index 36d9b866a3ab..beaaaecd35f4 100644
@@ -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;

mm/slub.c +5 -5

index aa30932c5190..006e6bfe257c 100644
@@ -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);

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>

Vladimir Davydov

author

2014-04-25 13:12 UTC #7 lore raw

PATCH 1/1 memcg: cleanup kmem_id-related naming

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 <[email protected]>

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

include/linux/memcontrol.h +7 ~~-12~~

index 3ee73da2991b..4080a6418c72 100644
@@ -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;
 }

mm/memcontrol.c +53 ~~-61~~

239 lines not on this page · raw message

mm/slab.c +2 -2

index 25317fd1daa2..194322a634e2 100644
@@ -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);

mm/slab.h +18 -6

index ba834860fbfd..61f833c569e7 100644
@@ -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;
 }

mm/slab_common.c +5 -5

index 36d9b866a3ab..beaaaecd35f4 100644
@@ -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;

mm/slub.c +5 -5

index aa30932c5190..006e6bfe257c 100644
@@ -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);

Signature · 1 line

1.7.10.4

Vladimir Davydov

author

2014-04-27 08:56 UTC #8 lore raw

In reply to Vladimir Davydov

Please ignore this set - I found that something can be improved, will send v2 soon. Sorry for the noise.

Thanks.

Quoted 44 lines · Vladimir Davydov

Hi,
In reply to "[PATCH RFC -mm v2 3/3] memcg, slab: simplify
Quoted 2 lines · synchronization scheme" Johannes
I like this patch, but the API names are confusing. Could we fix up that whole thing by any chance?
(see https://lkml.org/lkml/2014/4/18/317)
So this patch set is about cleaning up memcg/kmem naming.
While preparing it I found that some of the ugly-named functions constituting interface between memcontrol.c and slab_common.c can be neatly got rid of w/o complicating the code. Quite the contrary, w/o them call-flows look much simpler, IMO. So the first four patches do not rename anything actually - they just rework call-flows in kmem cache creation/destruction and memcg_caches arrays relocations paths. Finally, patches 5 and 6 clean up the naming.
Reviews are appreciated.
Thanks,
Vladimir Davydov (6):
memcg: get rid of memcg_create_cache_name
memcg: allocate memcg_caches array on first per memcg cache creation
memcg: cleanup memcg_caches arrays relocation path
memcg: get rid of memcg_{alloc,free}_cache_params
memcg: cleanup kmem cache creation/destruction functions naming
memcg: cleanup kmem_id-related naming
 include/linux/memcontrol.h |   40 +----
 include/linux/slab.h       |   12 +-
 mm/memcontrol.c            |  420 ++++++++++++++++++++------------------------
 mm/slab.c                  |    4 +-
 mm/slab.h                  |   24 ++-
 mm/slab_common.c           |  127 ++++++++++----
 mm/slub.c                  |   10 +-
 7 files changed, 320 insertions(+), 317 deletions(-)
Signature · 1 line
1.7.10.4