memcg-vs-slab related fixes, improvements, cleanups

Per-memcg kmem caches are named as follows:

<global-cache-name>(<cgroup-kmem-id>:<cgroup-name>)

where <cgroup-kmem-id> is the unique id of the memcg the cache belongs to, <cgroup-name> is the relative name of the memcg on the cgroup fs. Cache names are exposed to userspace for debugging purposes (e.g. via sysfs in case of slub or via dmesg).

Using relative names makes it impossible in general (in case the cgroup hierarchy is not flat) to find out which memcg a particular cache belongs to, because <cgroup-kmem-id> is not known to the user. Since using absolute cgroup names would be an overkill, let's fix this by exporting the id of kmem-active memcg via cgroup fs file "memory.kmem.id".

Quoted 9 lines · Vladimir Davydov

Per-memcg kmem caches are named as follows:

<global-cache-name>(<cgroup-kmem-id>:<cgroup-name>)

where <cgroup-kmem-id> is the unique id of the memcg the cache belongs to, <cgroup-name> is the relative name of the memcg on the cgroup fs. Cache names are exposed to userspace for debugging purposes (e.g. via sysfs in case of slub or via dmesg).

Quoted 7 lines

Using relative names makes it impossible in general (in case the cgroup hierarchy is not flat) to find out which memcg a particular cache belongs to, because <cgroup-kmem-id> is not known to the user. Since using absolute cgroup names would be an overkill, let's fix this by exporting the id of kmem-active memcg via cgroup fs file "memory.kmem.id".

Hmm, I'm not sure exporting additional information is the best way to do it only for this purpose. I do understand the problem in naming collisions if the hierarchy isn't flat and we typically work around that by ensuring child memcgs still have a unique memcg. This isn't only a problem in slab cache naming, me also avoid printing the entire absolute names for things like the oom killer.

So it would be nice to have
consensus on how people are supposed to identify memcgs with a hierarchy:
either by exporting information like the id like you do here (but leave the oom killer still problematic) or by insisting people name their memcgs with unique names if they care to differentiate them.

Quoted 18 lines · David Rientjes

Quoted 9 lines · Vladimir Davydov

Per-memcg kmem caches are named as follows:

<global-cache-name>(<cgroup-kmem-id>:<cgroup-name>)

where <cgroup-kmem-id> is the unique id of the memcg the cache belongs to, <cgroup-name> is the relative name of the memcg on the cgroup fs. Cache names are exposed to userspace for debugging purposes (e.g. via sysfs in case of slub or via dmesg).

Quoted 7 lines

Using relative names makes it impossible in general (in case the cgroup hierarchy is not flat) to find out which memcg a particular cache belongs to, because <cgroup-kmem-id> is not known to the user. Since using absolute cgroup names would be an overkill, let's fix this by exporting the id of kmem-active memcg via cgroup fs file "memory.kmem.id".

Quoted 6 lines

Hmm, I'm not sure exporting additional information is the best way to do it only for this purpose. I do understand the problem in naming collisions if the hierarchy isn't flat and we typically work around that by ensuring child memcgs still have a unique memcg. This isn't only a problem in slab cache naming, me also avoid printing the entire absolute names for things like the oom killer.

AFAIU, cgroup identifiers dumped on oom (cgroup paths, currently) and memcg slab cache names serve for different purposes. The point is oom is a perfectly normal situation for the kernel, and info dumped to dmesg is for admin to find out the cause of the problem (a greedy user or cgroup). On the other hand, slab cache names are dumped to dmesg only on extraordinary situations - like bugs in slab implementation, or double free, or detected memory leaks - where we usually do not need the name of the memcg that triggered the problem, because the bug is likely to be in the kernel subsys using the cache. Plus, the names are exported to sysfs in case of slub, again for debugging purposes, AFAIK. So IMO the use cases for oom vs slab names are completely different - information vs debugging - and I want to export kmem.id only for the ability of debugging kmemcg and slab subsystems.

Quoted 5 lines

So it would be nice to have
consensus on how people are supposed to identify memcgs with a hierarchy:
either by exporting information like the id like you do here (but leave the oom killer still problematic) or by insisting people name their memcgs with unique names if they care to differentiate them.

Anyway, I agree with you that this needs a consensus, because this is a functional change.

Thanks.

Quoted 18 lines · David Rientjes

Quoted 9 lines · Vladimir Davydov

Per-memcg kmem caches are named as follows:

<global-cache-name>(<cgroup-kmem-id>:<cgroup-name>)

where <cgroup-kmem-id> is the unique id of the memcg the cache belongs to, <cgroup-name> is the relative name of the memcg on the cgroup fs. Cache names are exposed to userspace for debugging purposes (e.g. via sysfs in case of slub or via dmesg).

Quoted 7 lines

Using relative names makes it impossible in general (in case the cgroup hierarchy is not flat) to find out which memcg a particular cache belongs to, because <cgroup-kmem-id> is not known to the user. Since using absolute cgroup names would be an overkill, let's fix this by exporting the id of kmem-active memcg via cgroup fs file "memory.kmem.id".

Quoted 6 lines

Hmm, I'm not sure exporting additional information is the best way to do it only for this purpose. I do understand the problem in naming collisions if the hierarchy isn't flat and we typically work around that by ensuring child memcgs still have a unique memcg. This isn't only a problem in slab cache naming, me also avoid printing the entire absolute names for things like the oom killer.

AFAIU, cgroup identifiers dumped on oom (cgroup paths, currently) and memcg slab cache names serve for different purposes. The point is oom is a perfectly normal situation for the kernel, and info dumped to dmesg is for admin to find out the cause of the problem (a greedy user or cgroup). On the other hand, slab cache names are dumped to dmesg only on extraordinary situations - like bugs in slab implementation, or double free, or detected memory leaks - where we usually do not need the name of the memcg that triggered the problem, because the bug is likely to be in the kernel subsys using the cache. Plus, the names are exported to sysfs in case of slub, again for debugging purposes, AFAIK. So IMO the use cases for oom vs slab names are completely different - information vs debugging - and I want to export kmem.id only for the ability of debugging kmemcg and slab subsystems.

Quoted 5 lines

So it would be nice to have
consensus on how people are supposed to identify memcgs with a hierarchy:
either by exporting information like the id like you do here (but leave the oom killer still problematic) or by insisting people name their memcgs with unique names if they care to differentiate them.

Anyway, I agree with you that this needs a consensus, because this is a functional change.

Thanks.

Quoted 25 lines · Vladimir Davydov

Quoted 18 lines · David Rientjes

Quoted 9 lines · Vladimir Davydov

Per-memcg kmem caches are named as follows:

<global-cache-name>(<cgroup-kmem-id>:<cgroup-name>)

where <cgroup-kmem-id> is the unique id of the memcg the cache belongs to, <cgroup-name> is the relative name of the memcg on the cgroup fs. Cache names are exposed to userspace for debugging purposes (e.g. via sysfs in case of slub or via dmesg).

Quoted 7 lines

Using relative names makes it impossible in general (in case the cgroup hierarchy is not flat) to find out which memcg a particular cache belongs to, because <cgroup-kmem-id> is not known to the user. Since using absolute cgroup names would be an overkill, let's fix this by exporting the id of kmem-active memcg via cgroup fs file "memory.kmem.id".

Quoted 6 lines

Hmm, I'm not sure exporting additional information is the best way to do it only for this purpose. I do understand the problem in naming collisions if the hierarchy isn't flat and we typically work around that by ensuring child memcgs still have a unique memcg. This isn't only a problem in slab cache naming, me also avoid printing the entire absolute names for things like the oom killer.

Quoted 14 lines

AFAIU, cgroup identifiers dumped on oom (cgroup paths, currently) and memcg slab cache names serve for different purposes. The point is oom is a perfectly normal situation for the kernel, and info dumped to dmesg is for admin to find out the cause of the problem (a greedy user or cgroup). On the other hand, slab cache names are dumped to dmesg only on extraordinary situations - like bugs in slab implementation, or double free, or detected memory leaks - where we usually do not need the name of the memcg that triggered the problem, because the bug is likely to be in the kernel subsys using the cache. Plus, the names are exported to sysfs in case of slub, again for debugging purposes, AFAIK. So IMO the use cases for oom vs slab names are completely different - information vs debugging - and I want to export kmem.id only for the ability of debugging kmemcg and slab subsystems.

Then maybe it is better to wrap it into some kind of CONFIG_DEBUG wrap. We already have other files like that.

Quoted 6 lines

Quoted 6 lines

So it would be nice to have
consensus on how people are supposed to identify memcgs with a hierarchy:
either by exporting information like the id like you do here (but leave the oom killer still problematic) or by insisting people name their memcgs with unique names if they care to differentiate them.

Quoted 4 lines

Anyway, I agree with you that this needs a consensus, because this is a functional change.

Thanks.

AFAIU, cgroup identifiers dumped on oom (cgroup paths, currently) and memcg slab cache names serve for different purposes.

The point is oom is
a perfectly normal situation for the kernel, and info dumped to dmesg is for admin to find out the cause of the problem (a greedy user or cgroup).

On the other hand, slab cache names are dumped to dmesg only on extraordinary situations - like bugs in slab implementation, or double free, or detected memory leaks - where we usually do not need the name of the memcg that triggered the problem, because the bug is likely to be in the kernel subsys using the cache.

Plus, the names are exported to
sysfs in case of slub, again for debugging purposes, AFAIK. So IMO the use cases for oom vs slab names are completely different - information vs debugging - and I want to export kmem.id only for the ability of debugging kmemcg and slab subsystems.

Quoted 2 lines · Vladimir Davydov

AFAIU, cgroup identifiers dumped on oom (cgroup paths, currently) and memcg slab cache names serve for different purposes.

Sure, you may dump the name for a number of legitimate reasons, but the problem still exists that it's difficult to determine what memcg is being referenced without a flat hierarchy and unique memcg names for all children.

Quoted 4 lines

The point is oom is
a perfectly normal situation for the kernel, and info dumped to dmesg is for admin to find out the cause of the problem (a greedy user or cgroup).

Hmm, so if we hand out top-level memcgs to individual jobs or users, like our userspace does, and they are able to configure their child memcgs as they wish, and then they or the admin finds in the kernel log that a memory hog was killed from the memcg with the perfectly anonymous memcg name of "memcg", how do we determine what job or user triggered that kill? User id is not going to be conclusive in a production environment with shared user accounts.

Quoted 5 lines

On the other hand, slab cache names are dumped to dmesg only on extraordinary situations - like bugs in slab implementation, or double free, or detected memory leaks - where we usually do not need the name of the memcg that triggered the problem, because the bug is likely to be in the kernel subsys using the cache.

There's certainly overlap here since slab leaks triggered by a particular workload, perhaps by usage of a particular syscall, can occur and cause oom killing but the problem remains that neither the memcg name nor the slab cache name may be conclusive to determine what job or user triggered the issue. That's why we make strict demands that memcg names are always unique and encode several key values to identify the user and job and we don't rely on the parent.

I can also see the huge maintenance burden it would be to keep around a mapping of kmem ids to {user, job} pairs just in case we later identify a problem and in 99% of the cases would be just wasted storage.

Quoted 6 lines

Plus, the names are exported to
sysfs in case of slub, again for debugging purposes, AFAIK. So IMO the use cases for oom vs slab names are completely different - information vs debugging - and I want to export kmem.id only for the ability of debugging kmemcg and slab subsystems.

Eeek, I'm not sure I agree. I've often found that reproducing rare slab issues is very difficult without knowledge of the workload so that I can reproduce it. Whereas X is a very large number of machines and we see this issue on 0.0001% of X machines, I would be required to enable this "debugging" aid unconditionally to ever be able to map the stored kmem id back to a user and job, that mapping would be extremely costly to maintain, and we've gained nothing if we had already demanded that userspace identify their memcg names with unique identifiers regardless of where they are in the hierarchy.

Quoted 18 lines · David Rientjes

Quoted 9 lines · Vladimir Davydov

Per-memcg kmem caches are named as follows:

<global-cache-name>(<cgroup-kmem-id>:<cgroup-name>)

where <cgroup-kmem-id> is the unique id of the memcg the cache belongs to, <cgroup-name> is the relative name of the memcg on the cgroup fs. Cache names are exposed to userspace for debugging purposes (e.g. via sysfs in case of slub or via dmesg).

Quoted 7 lines

Using relative names makes it impossible in general (in case the cgroup hierarchy is not flat) to find out which memcg a particular cache belongs to, because <cgroup-kmem-id> is not known to the user. Since using absolute cgroup names would be an overkill, let's fix this by exporting the id of kmem-active memcg via cgroup fs file "memory.kmem.id".

Quoted 6 lines

Hmm, I'm not sure exporting additional information is the best way to do it only for this purpose. I do understand the problem in naming collisions if the hierarchy isn't flat and we typically work around that by ensuring child memcgs still have a unique memcg. This isn't only a problem in slab cache naming, me also avoid printing the entire absolute names for things like the oom killer.

AFAIU, cgroup identifiers dumped on oom (cgroup paths, currently) and memcg slab cache names serve for different purposes. The point is oom is a perfectly normal situation for the kernel, and info dumped to dmesg is for admin to find out the cause of the problem (a greedy user or cgroup). On the other hand, slab cache names are dumped to dmesg only on extraordinary situations - like bugs in slab implementation, or double free, or detected memory leaks - where we usually do not need the name of the memcg that triggered the problem, because the bug is likely to be in the kernel subsys using the cache. Plus, the names are exported to sysfs in case of slub, again for debugging purposes, AFAIK. So IMO the use cases for oom vs slab names are completely different - information vs debugging - and I want to export kmem.id only for the ability of debugging kmemcg and slab subsystems.

Quoted 5 lines

So it would be nice to have
consensus on how people are supposed to identify memcgs with a hierarchy:
either by exporting information like the id like you do here (but leave the oom killer still problematic) or by insisting people name their memcgs with unique names if they care to differentiate them.

Anyway, I agree with you that this needs a consensus, because this is a functional change.

Thanks.

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

Per-memcg kmem caches are named as follows:

<global-cache-name>(<cgroup-kmem-id>:<cgroup-name>)

where <cgroup-kmem-id> is the unique id of the memcg the cache belongs to, <cgroup-name> is the relative name of the memcg on the cgroup fs. Cache names are exposed to userspace for debugging purposes (e.g. via sysfs in case of slub or via dmesg).

Using relative names makes it impossible in general (in case the cgroup hierarchy is not flat) to find out which memcg a particular cache belongs to, because <cgroup-kmem-id> is not known to the user. Since using absolute cgroup names would be an overkill,

let's fix this by
exporting the id of kmem-active memcg via cgroup fs file "memory.kmem.id".

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

---
 mm/memcontrol.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

mm/memcontrol.c +12 -0

index 53385cd4e6f0..91d242707404 100644
@@ -3113,6 +3113,14 @@ int memcg_cache_id(struct mem_cgroup *memcg)
 	return memcg ? memcg->kmemcg_id : -1;
 }
 
+static s64 mem_cgroup_cache_id_read(struct cgroup_subsys_state *css,
+				    struct cftype *cft)
+{
+	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
+
+	return memcg_can_account_kmem(memcg) ? memcg_cache_id(memcg) : -1;
+}
+
 static size_t memcg_caches_array_size(int num_groups)
 {
 	ssize_t size;

@@ -6301,6 +6309,10 @@ static struct cftype mem_cgroup_files[] = {
 #endif
 #ifdef CONFIG_MEMCG_KMEM
 	{
+		.name = "kmem.id",
+		.read_s64 = mem_cgroup_cache_id_read,
+	},
+	{
 		.name = "kmem.limit_in_bytes",
 		.private = MEMFILE_PRIVATE(_KMEM, RES_LIMIT),
 		.write_string = mem_cgroup_write,

Signature · 1 line

1.7.10.4

Quoted 8 lines · Vladimir Davydov

Per-memcg kmem caches are named as follows:

<global-cache-name>(<cgroup-kmem-id>:<cgroup-name>)

where <cgroup-kmem-id> is the unique id of the memcg the cache belongs to, <cgroup-name> is the relative name of the memcg on the cgroup fs. Cache names are exposed to userspace for debugging purposes (e.g. via sysfs in case of slub or via dmesg).

If this is only for debugging purposes then it shouldn't pollute regular memcg cgroupfs namespace.

Quoted 4 lines

Using relative names makes it impossible in general (in case the cgroup hierarchy is not flat) to find out which memcg a particular cache belongs to, because <cgroup-kmem-id> is not known to the user. Since using absolute cgroup names would be an overkill,

I do not consider it an overkill. We are using the full path when dumping OOM information so I do not see any reason this should be any different.

Quoted 6 lines

let's fix this by
exporting the id of kmem-active memcg via cgroup fs file "memory.kmem.id".

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

Nacked-by Michal Hocko <[email protected]>

Quoted 29 lines

---
 mm/memcontrol.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

mm/memcontrol.c +10 -0

index 53385cd4e6f0..91d242707404 100644
@@ -3113,6 +3113,14 @@ int memcg_cache_id(struct mem_cgroup *memcg)
 	return memcg ? memcg->kmemcg_id : -1;
 }
 
+static s64 mem_cgroup_cache_id_read(struct cgroup_subsys_state *css,
+				    struct cftype *cft)
+{
+	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
+
+	return memcg_can_account_kmem(memcg) ? memcg_cache_id(memcg) : -1;
+}
+
 static size_t memcg_caches_array_size(int num_groups)
 {
 	ssize_t size;

@@ -6301,6 +6309,10 @@ static struct cftype mem_cgroup_files[] = {
 #endif
 #ifdef CONFIG_MEMCG_KMEM
 	{
+		.name = "kmem.id",
+		.read_s64 = mem_cgroup_cache_id_read,

Quoted 8 lines

mm/memcontrol.c +2 -0

+	},
+	{
 		.name = "kmem.limit_in_bytes",
 		.private = MEMFILE_PRIVATE(_KMEM, RES_LIMIT),
 		.write_string = mem_cgroup_write,

Signature · 1 line

1.7.10.4

The cgroup name is not informative at all in case the cgroup hierarchy is not flat. Besides, we can always find the memcg a particular cache belongs to by its kmemcg id, which is now exported via "memory.kmem.id" cgroup fs file for each memcg.

So let's remove the cgroup name part from kmem caches names - it will greatly simplify the call paths and make the code look clearer.

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

---
 mm/memcontrol.c  |   63 +++++++++++++-----------------------------------------
 mm/slab_common.c |    6 +++++-
 2 files changed, 20 insertions(+), 49 deletions(-)

mm/memcontrol.c +15 -48

index 91d242707404..3351c5b5486d 100644
@@ -3405,44 +3405,6 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 	schedule_work(&cachep->memcg_params->destroy);
 }
 
-static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
-						  struct kmem_cache *s)
-{
-	struct kmem_cache *new = NULL;
-	static char *tmp_name = NULL;
-	static DEFINE_MUTEX(mutex);	/* protects tmp_name */
-
-	BUG_ON(!memcg_can_account_kmem(memcg));
-
-	mutex_lock(&mutex);
-	/*
-	 * kmem_cache_create_memcg duplicates the given name and
-	 * cgroup_name for this name requires RCU context.
-	 * This static temporary buffer is used to prevent from
-	 * pointless shortliving allocation.
-	 */
-	if (!tmp_name) {
-		tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);
-		if (!tmp_name)
-			goto out;
-	}
-
-	rcu_read_lock();
-	snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name,
-			 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
-	rcu_read_unlock();
-
-	new = kmem_cache_create_memcg(memcg, tmp_name, s->object_size, s->align,
-				      (s->flags & ~SLAB_PANIC), s->ctor, s);
-	if (new)
-		new->allocflags |= __GFP_KMEMCG;
-	else
-		new = s;
-out:
-	mutex_unlock(&mutex);
-	return new;
-}
-
 void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 {
 	struct kmem_cache *c;

@@ -3489,12 +3451,6 @@ void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 	mutex_unlock(&activate_kmem_mutex);
 }
 
-struct create_work {
-	struct mem_cgroup *memcg;
-	struct kmem_cache *cachep;
-	struct work_struct work;
-};
-
 static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg)
 {
 	struct kmem_cache *cachep;

@@ -3512,13 +3468,24 @@ static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg)
 	mutex_unlock(&memcg->slab_caches_mutex);
 }
 
+struct create_work {
+	struct mem_cgroup *memcg;
+	struct kmem_cache *cachep;
+	struct work_struct work;
+};
+
 static void memcg_create_cache_work_func(struct work_struct *w)
 {
-	struct create_work *cw;
+	struct create_work *cw = container_of(w, struct create_work, work);
+	struct mem_cgroup *memcg = cw->memcg;
+	struct kmem_cache *s = cw->cachep;
+	struct kmem_cache *new;
 
-	cw = container_of(w, struct create_work, work);
-	memcg_create_kmem_cache(cw->memcg, cw->cachep);
-	css_put(&cw->memcg->css);
+	new = kmem_cache_create_memcg(memcg, s->name, s->object_size, s->align,
+				      (s->flags & ~SLAB_PANIC), s->ctor, s);
+	if (new)
+		new->allocflags |= __GFP_KMEMCG;
+	css_put(&memcg->css);
 	kfree(cw);
 }
 

mm/slab_common.c +5 -1

index 1ec3c619ba04..152d9b118b7a 100644
@@ -213,7 +213,11 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	s->align = calculate_alignment(flags, align, size);
 	s->ctor = ctor;
 
-	s->name = kstrdup(name, GFP_KERNEL);
+	if (!memcg)
+		s->name = kstrdup(name, GFP_KERNEL);
+	else
+		s->name = kasprintf(GFP_KERNEL, "%s:%d",
+				    name, memcg_cache_id(memcg));
 	if (!s->name)
 		goto out_free_cache;
 

Signature · 1 line

1.7.10.4

Quoted 9 lines · Vladimir Davydov

The cgroup name is not informative at all in case the cgroup hierarchy is not flat. Besides, we can always find the memcg a particular cache belongs to by its kmemcg id, which is now exported via "memory.kmem.id" cgroup fs file for each memcg.

So let's remove the cgroup name part from kmem caches names - it will greatly simplify the call paths and make the code look clearer.

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

I guess this one doesn't make much sense withou 1/8, right?

Quoted 114 lines

---
 mm/memcontrol.c  |   63 +++++++++++++-----------------------------------------
 mm/slab_common.c |    6 +++++-
 2 files changed, 20 insertions(+), 49 deletions(-)

mm/memcontrol.c +15 -48

index 91d242707404..3351c5b5486d 100644
@@ -3405,44 +3405,6 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 	schedule_work(&cachep->memcg_params->destroy);
 }
 
-static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
-						  struct kmem_cache *s)
-{
-	struct kmem_cache *new = NULL;
-	static char *tmp_name = NULL;
-	static DEFINE_MUTEX(mutex);	/* protects tmp_name */
-
-	BUG_ON(!memcg_can_account_kmem(memcg));
-
-	mutex_lock(&mutex);
-	/*
-	 * kmem_cache_create_memcg duplicates the given name and
-	 * cgroup_name for this name requires RCU context.
-	 * This static temporary buffer is used to prevent from
-	 * pointless shortliving allocation.
-	 */
-	if (!tmp_name) {
-		tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);
-		if (!tmp_name)
-			goto out;
-	}
-
-	rcu_read_lock();
-	snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name,
-			 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
-	rcu_read_unlock();
-
-	new = kmem_cache_create_memcg(memcg, tmp_name, s->object_size, s->align,
-				      (s->flags & ~SLAB_PANIC), s->ctor, s);
-	if (new)
-		new->allocflags |= __GFP_KMEMCG;
-	else
-		new = s;
-out:
-	mutex_unlock(&mutex);
-	return new;
-}
-
 void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 {
 	struct kmem_cache *c;

@@ -3489,12 +3451,6 @@ void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 	mutex_unlock(&activate_kmem_mutex);
 }
 
-struct create_work {
-	struct mem_cgroup *memcg;
-	struct kmem_cache *cachep;
-	struct work_struct work;
-};
-
 static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg)
 {
 	struct kmem_cache *cachep;

@@ -3512,13 +3468,24 @@ static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg)
 	mutex_unlock(&memcg->slab_caches_mutex);
 }
 
+struct create_work {
+	struct mem_cgroup *memcg;
+	struct kmem_cache *cachep;
+	struct work_struct work;
+};
+
 static void memcg_create_cache_work_func(struct work_struct *w)
 {
-	struct create_work *cw;
+	struct create_work *cw = container_of(w, struct create_work, work);
+	struct mem_cgroup *memcg = cw->memcg;
+	struct kmem_cache *s = cw->cachep;
+	struct kmem_cache *new;
 
-	cw = container_of(w, struct create_work, work);
-	memcg_create_kmem_cache(cw->memcg, cw->cachep);
-	css_put(&cw->memcg->css);
+	new = kmem_cache_create_memcg(memcg, s->name, s->object_size, s->align,
+				      (s->flags & ~SLAB_PANIC), s->ctor, s);
+	if (new)
+		new->allocflags |= __GFP_KMEMCG;
+	css_put(&memcg->css);
 	kfree(cw);
 }
 

mm/slab_common.c +5 -1

index 1ec3c619ba04..152d9b118b7a 100644
@@ -213,7 +213,11 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	s->align = calculate_alignment(flags, align, size);
 	s->ctor = ctor;
 
-	s->name = kstrdup(name, GFP_KERNEL);
+	if (!memcg)
+		s->name = kstrdup(name, GFP_KERNEL);
+	else
+		s->name = kasprintf(GFP_KERNEL, "%s:%d",
+				    name, memcg_cache_id(memcg));
 	if (!s->name)
 		goto out_free_cache;
 

Quoted 3 lines

Signature · 1 line

1.7.10.4

Quoted 9 lines · Michal Hocko

Quoted 8 lines · Vladimir Davydov

The cgroup name is not informative at all in case the cgroup hierarchy is not flat. Besides, we can always find the memcg a particular cache belongs to by its kmemcg id, which is now exported via "memory.kmem.id" cgroup fs file for each memcg.

So let's remove the cgroup name part from kmem caches names - it will greatly simplify the call paths and make the code look clearer.

Quoted 2 lines

Quoted 1 line

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

I guess this one doesn't make much sense withou 1/8, right?

Actually, the rest of the patchset does not depend on the logic of the first two - they only have a couple of hunks overlapped. However, there is v2 already.

Thanks.

Quoted 117 lines

Quoted 117 lines

---
 mm/memcontrol.c  |   63 +++++++++++++-----------------------------------------
 mm/slab_common.c |    6 +++++-
 2 files changed, 20 insertions(+), 49 deletions(-)

mm/memcontrol.c +15 -48

index 91d242707404..3351c5b5486d 100644
@@ -3405,44 +3405,6 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 	schedule_work(&cachep->memcg_params->destroy);
 }
 
-static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
-						  struct kmem_cache *s)
-{
-	struct kmem_cache *new = NULL;
-	static char *tmp_name = NULL;
-	static DEFINE_MUTEX(mutex);	/* protects tmp_name */
-
-	BUG_ON(!memcg_can_account_kmem(memcg));
-
-	mutex_lock(&mutex);
-	/*
-	 * kmem_cache_create_memcg duplicates the given name and
-	 * cgroup_name for this name requires RCU context.
-	 * This static temporary buffer is used to prevent from
-	 * pointless shortliving allocation.
-	 */
-	if (!tmp_name) {
-		tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);
-		if (!tmp_name)
-			goto out;
-	}
-
-	rcu_read_lock();
-	snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name,
-			 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
-	rcu_read_unlock();
-
-	new = kmem_cache_create_memcg(memcg, tmp_name, s->object_size, s->align,
-				      (s->flags & ~SLAB_PANIC), s->ctor, s);
-	if (new)
-		new->allocflags |= __GFP_KMEMCG;
-	else
-		new = s;
-out:
-	mutex_unlock(&mutex);
-	return new;
-}
-
 void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 {
 	struct kmem_cache *c;

@@ -3489,12 +3451,6 @@ void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 	mutex_unlock(&activate_kmem_mutex);
 }
 
-struct create_work {
-	struct mem_cgroup *memcg;
-	struct kmem_cache *cachep;
-	struct work_struct work;
-};
-
 static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg)
 {
 	struct kmem_cache *cachep;

@@ -3512,13 +3468,24 @@ static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg)
 	mutex_unlock(&memcg->slab_caches_mutex);
 }
 
+struct create_work {
+	struct mem_cgroup *memcg;
+	struct kmem_cache *cachep;
+	struct work_struct work;
+};
+
 static void memcg_create_cache_work_func(struct work_struct *w)
 {
-	struct create_work *cw;
+	struct create_work *cw = container_of(w, struct create_work, work);
+	struct mem_cgroup *memcg = cw->memcg;
+	struct kmem_cache *s = cw->cachep;
+	struct kmem_cache *new;
 
-	cw = container_of(w, struct create_work, work);
-	memcg_create_kmem_cache(cw->memcg, cw->cachep);
-	css_put(&cw->memcg->css);
+	new = kmem_cache_create_memcg(memcg, s->name, s->object_size, s->align,
+				      (s->flags & ~SLAB_PANIC), s->ctor, s);
+	if (new)
+		new->allocflags |= __GFP_KMEMCG;
+	css_put(&memcg->css);
 	kfree(cw);
 }
 

mm/slab_common.c +5 -1

index 1ec3c619ba04..152d9b118b7a 100644
@@ -213,7 +213,11 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	s->align = calculate_alignment(flags, align, size);
 	s->ctor = ctor;
 
-	s->name = kstrdup(name, GFP_KERNEL);
+	if (!memcg)
+		s->name = kstrdup(name, GFP_KERNEL);
+	else
+		s->name = kasprintf(GFP_KERNEL, "%s:%d",
+				    name, memcg_cache_id(memcg));
 	if (!s->name)
 		goto out_free_cache;
 

Signature · 1 line

1.7.10.4

Suppose we are creating memcg cache A that could be merged with cache B of the same memcg. Since any memcg cache has the same parameters as its parent cache, parent caches PA and PB of memcg caches A and B must be mergeable too. That means PA was merged with PB on creation or vice versa, i.e. PA = PB. From that it follows that A = B, and we couldn't even try to create cache B, because it already exists - a contradiction.

So let's remove unused code responsible for merging memcg caches.

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

---
 mm/slab.h        |   21 ++++-----------------
 mm/slab_common.c |    8 +++++---
 mm/slub.c        |   19 +++++++++----------
 3 files changed, 18 insertions(+), 30 deletions(-)

mm/slab.h +4 -17

index 8184a7cde272..3045316b7c9d 100644
@@ -55,12 +55,12 @@ extern void create_boot_cache(struct kmem_cache *, const char *name,
 struct mem_cgroup;
 #ifdef CONFIG_SLUB
 struct kmem_cache *
-__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
-		   size_t align, unsigned long flags, void (*ctor)(void *));
+__kmem_cache_alias(const char *name, size_t size, size_t align,
+		   unsigned long flags, void (*ctor)(void *));
 #else
 static inline struct kmem_cache *
-__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
-		   size_t align, unsigned long flags, void (*ctor)(void *))
+__kmem_cache_alias(const char *name, size_t size, size_t align,
+		   unsigned long flags, void (*ctor)(void *))
 { return NULL; }
 #endif
 

@@ -119,13 +119,6 @@ static inline bool is_root_cache(struct kmem_cache *s)
 	return !s->memcg_params || s->memcg_params->is_root_cache;
 }
 
-static inline bool cache_match_memcg(struct kmem_cache *cachep,
-				     struct mem_cgroup *memcg)
-{
-	return (is_root_cache(cachep) && !memcg) ||
-				(cachep->memcg_params->memcg == memcg);
-}
-
 static inline void memcg_bind_pages(struct kmem_cache *s, int order)
 {
 	if (!is_root_cache(s))

@@ -204,12 +197,6 @@ static inline bool is_root_cache(struct kmem_cache *s)
 	return true;
 }
 
-static inline bool cache_match_memcg(struct kmem_cache *cachep,
-				     struct mem_cgroup *memcg)
-{
-	return true;
-}
-
 static inline void memcg_bind_pages(struct kmem_cache *s, int order)
 {
 }

mm/slab_common.c +5 -3

index 152d9b118b7a..a75834bb966d 100644
@@ -200,9 +200,11 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	 */
 	flags &= CACHE_CREATE_MASK;
 
-	s = __kmem_cache_alias(memcg, name, size, align, flags, ctor);
-	if (s)
-		goto out_unlock;
+	if (!memcg) {
+		s = __kmem_cache_alias(name, size, align, flags, ctor);
+		if (s)
+			goto out_unlock;
+	}
 
 	err = -ENOMEM;
 	s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL);

mm/slub.c +9 -10

index 2b1a6970e46f..962abfdfde06 100644
@@ -3686,9 +3686,8 @@ static int slab_unmergeable(struct kmem_cache *s)
 	return 0;
 }
 
-static struct kmem_cache *find_mergeable(struct mem_cgroup *memcg, size_t size,
-		size_t align, unsigned long flags, const char *name,
-		void (*ctor)(void *))
+static struct kmem_cache *find_mergeable(size_t size, size_t align,
+		unsigned long flags, const char *name, void (*ctor)(void *))
 {
 	struct kmem_cache *s;
 

@@ -3707,11 +3706,14 @@ static struct kmem_cache *find_mergeable(struct mem_cgroup *memcg, size_t size,
 		if (slab_unmergeable(s))
 			continue;
 
+		if (!is_root_cache(s))
+			continue;
+
 		if (size > s->size)
 			continue;
 
 		if ((flags & SLUB_MERGE_SAME) != (s->flags & SLUB_MERGE_SAME))
-				continue;
+			continue;
 		/*
 		 * Check if alignment is compatible.
 		 * Courtesy of Adrian Drzewiecki

@@ -3722,21 +3724,18 @@ static struct kmem_cache *find_mergeable(struct mem_cgroup *memcg, size_t size,
 		if (s->size - size >= sizeof(void *))
 			continue;
 
-		if (!cache_match_memcg(s, memcg))
-			continue;
-
 		return s;
 	}
 	return NULL;
 }
 
 struct kmem_cache *
-__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
-		   size_t align, unsigned long flags, void (*ctor)(void *))
+__kmem_cache_alias(const char *name, size_t size, size_t align,
+		   unsigned long flags, void (*ctor)(void *))
 {
 	struct kmem_cache *s;
 
-	s = find_mergeable(memcg, size, align, flags, name, ctor);
+	s = find_mergeable(size, align, flags, name, ctor);
 	if (s) {
 		s->refcount++;
 		/*

Signature · 1 line

1.7.10.4

Quoted 6 lines · Vladimir Davydov

Suppose we are creating memcg cache A that could be merged with cache B of the same memcg. Since any memcg cache has the same parameters as its parent cache, parent caches PA and PB of memcg caches A and B must be mergeable too. That means PA was merged with PB on creation or vice versa, i.e. PA = PB. From that it follows that A = B, and we couldn't even try to create cache B, because it already exists - a contradiction.

I cannot tell I understand the above but I am totally not sure about the statement bellow.

Quoted 1 line

So let's remove unused code responsible for merging memcg caches.

How come the code was unused? find_mergeable called cache_match_memcg...

Quoted 127 lines

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

---
 mm/slab.h        |   21 ++++-----------------
 mm/slab_common.c |    8 +++++---
 mm/slub.c        |   19 +++++++++----------
 3 files changed, 18 insertions(+), 30 deletions(-)

mm/slab.h +4 -17

index 8184a7cde272..3045316b7c9d 100644
@@ -55,12 +55,12 @@ extern void create_boot_cache(struct kmem_cache *, const char *name,
 struct mem_cgroup;
 #ifdef CONFIG_SLUB
 struct kmem_cache *
-__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
-		   size_t align, unsigned long flags, void (*ctor)(void *));
+__kmem_cache_alias(const char *name, size_t size, size_t align,
+		   unsigned long flags, void (*ctor)(void *));
 #else
 static inline struct kmem_cache *
-__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
-		   size_t align, unsigned long flags, void (*ctor)(void *))
+__kmem_cache_alias(const char *name, size_t size, size_t align,
+		   unsigned long flags, void (*ctor)(void *))
 { return NULL; }
 #endif
 

@@ -119,13 +119,6 @@ static inline bool is_root_cache(struct kmem_cache *s)
 	return !s->memcg_params || s->memcg_params->is_root_cache;
 }
 
-static inline bool cache_match_memcg(struct kmem_cache *cachep,
-				     struct mem_cgroup *memcg)
-{
-	return (is_root_cache(cachep) && !memcg) ||
-				(cachep->memcg_params->memcg == memcg);
-}
-
 static inline void memcg_bind_pages(struct kmem_cache *s, int order)
 {
 	if (!is_root_cache(s))

@@ -204,12 +197,6 @@ static inline bool is_root_cache(struct kmem_cache *s)
 	return true;
 }
 
-static inline bool cache_match_memcg(struct kmem_cache *cachep,
-				     struct mem_cgroup *memcg)
-{
-	return true;
-}
-
 static inline void memcg_bind_pages(struct kmem_cache *s, int order)
 {
 }

mm/slab_common.c +5 -3

index 152d9b118b7a..a75834bb966d 100644
@@ -200,9 +200,11 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	 */
 	flags &= CACHE_CREATE_MASK;
 
-	s = __kmem_cache_alias(memcg, name, size, align, flags, ctor);
-	if (s)
-		goto out_unlock;
+	if (!memcg) {
+		s = __kmem_cache_alias(name, size, align, flags, ctor);
+		if (s)
+			goto out_unlock;
+	}
 
 	err = -ENOMEM;
 	s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL);

mm/slub.c +8 -9

index 2b1a6970e46f..962abfdfde06 100644
@@ -3686,9 +3686,8 @@ static int slab_unmergeable(struct kmem_cache *s)
 	return 0;
 }
 
-static struct kmem_cache *find_mergeable(struct mem_cgroup *memcg, size_t size,
-		size_t align, unsigned long flags, const char *name,
-		void (*ctor)(void *))
+static struct kmem_cache *find_mergeable(size_t size, size_t align,
+		unsigned long flags, const char *name, void (*ctor)(void *))
 {
 	struct kmem_cache *s;
 

@@ -3707,11 +3706,14 @@ static struct kmem_cache *find_mergeable(struct mem_cgroup *memcg, size_t size,
 		if (slab_unmergeable(s))
 			continue;
 
+		if (!is_root_cache(s))
+			continue;
+
 		if (size > s->size)
 			continue;
 
 		if ((flags & SLUB_MERGE_SAME) != (s->flags & SLUB_MERGE_SAME))
-				continue;
+			continue;
 		/*
 		 * Check if alignment is compatible.
 		 * Courtesy of Adrian Drzewiecki

@@ -3722,21 +3724,18 @@ static struct kmem_cache *find_mergeable(struct mem_cgroup *memcg, size_t size,
 		if (s->size - size >= sizeof(void *))
 			continue;
 
-		if (!cache_match_memcg(s, memcg))
-			continue;
-
 		return s;
 	}
 	return NULL;
 }
 
 struct kmem_cache *
-__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
-		   size_t align, unsigned long flags, void (*ctor)(void *))
+__kmem_cache_alias(const char *name, size_t size, size_t align,
+		   unsigned long flags, void (*ctor)(void *))
 {
 	struct kmem_cache *s;
 

Quoted 8 lines

mm/slub.c +1 -1

-	s = find_mergeable(memcg, size, align, flags, name, ctor);
+	s = find_mergeable(size, align, flags, name, ctor);
 	if (s) {
 		s->refcount++;
 		/*

Signature · 1 line

1.7.10.4

Quoted 11 lines · Michal Hocko

Quoted 6 lines · Vladimir Davydov

Suppose we are creating memcg cache A that could be merged with cache B of the same memcg. Since any memcg cache has the same parameters as its parent cache, parent caches PA and PB of memcg caches A and B must be mergeable too. That means PA was merged with PB on creation or vice versa, i.e. PA = PB. From that it follows that A = B, and we couldn't even try to create cache B, because it already exists - a contradiction.

I cannot tell I understand the above but I am totally not sure about the statement bellow.

Quoted 1 line

So let's remove unused code responsible for merging memcg caches.

Quoted 1 line

How come the code was unused? find_mergeable called cache_match_memcg...

Oh, sorry for misleading comment. I mean the code handling merging of per-memcg caches is useless, AFAIU: if we find an alias for a per-memcg cache on kmem_cache_create_memcg(), the parent of the found alias must be the same as the parent_cache passed to kmem_cache_create_memcg(), but if it were so, we would never proceed to the memcg cache creation, because the cache we want to create already exists.

Quoted 11 lines · Vladimir Davydov

Quoted 10 lines · Michal Hocko

Quoted 6 lines · Vladimir Davydov

Suppose we are creating memcg cache A that could be merged with cache B of the same memcg. Since any memcg cache has the same parameters as its parent cache, parent caches PA and PB of memcg caches A and B must be mergeable too. That means PA was merged with PB on creation or vice versa, i.e. PA = PB. From that it follows that A = B, and we couldn't even try to create cache B, because it already exists - a contradiction.

I cannot tell I understand the above but I am totally not sure about the statement bellow.

Quoted 8 lines

Quoted 2 lines

Quoted 1 line

So let's remove unused code responsible for merging memcg caches.

How come the code was unused? find_mergeable called cache_match_memcg...

Oh, sorry for misleading comment. I mean the code handling merging of per-memcg caches is useless, AFAIU: if we find an alias for a per-memcg cache on kmem_cache_create_memcg(), the parent of the found alias must be the same as the parent_cache passed to kmem_cache_create_memcg(), but if it were so, we would never proceed to the memcg cache creation, because the cache we want to create already exists.

I am still not sure I understand this correctly. So the outcome of this patch is that compatible caches of different memcgs can be merged together? Sorry if this is a stupid question but I am not that familiar with this area much I am just seeing that cache_match_memcg goes away and my understanding of the function is that it should prevent from different memcg's caches merging.

Quoted 20 lines · Michal Hocko

Quoted 11 lines · Vladimir Davydov

Quoted 10 lines · Michal Hocko

Quoted 6 lines · Vladimir Davydov

Suppose we are creating memcg cache A that could be merged with cache B of the same memcg. Since any memcg cache has the same parameters as its parent cache, parent caches PA and PB of memcg caches A and B must be mergeable too. That means PA was merged with PB on creation or vice versa, i.e. PA = PB. From that it follows that A = B, and we couldn't even try to create cache B, because it already exists - a contradiction.

I cannot tell I understand the above but I am totally not sure about the statement bellow.

Quoted 8 lines

Quoted 2 lines

Quoted 1 line

So let's remove unused code responsible for merging memcg caches.

How come the code was unused? find_mergeable called cache_match_memcg...

Oh, sorry for misleading comment. I mean the code handling merging of per-memcg caches is useless, AFAIU: if we find an alias for a per-memcg cache on kmem_cache_create_memcg(), the parent of the found alias must be the same as the parent_cache passed to kmem_cache_create_memcg(), but if it were so, we would never proceed to the memcg cache creation, because the cache we want to create already exists.

Quoted 6 lines

I am still not sure I understand this correctly. So the outcome of this patch is that compatible caches of different memcgs can be merged together? Sorry if this is a stupid question but I am not that familiar with this area much I am just seeing that cache_match_memcg goes away and my understanding of the function is that it should prevent from different memcg's caches merging.

Let me try to explain how I understand it.

What is cache merging/aliasing? When we create a cache (kmem_cache_create()), we first try to find a compatible cache that already exists and can handle requests from the new cache. If it is, we do not create any new caches, instead we simply increment the old cache refcount and return it.

What about memcgs? Currently, it operates in the same way, i.e. on memcg cache creation we also try to find a compatible cache of the same memcg first. But if there were such a cache, they parents would have been merged (i.e. it would be the same cache). That means we would not even get to this memcg cache creation, because it already exists. That's why the code handling memcg caches merging seems pointless to me.

Quoted 35 lines · Vladimir Davydov

Quoted 20 lines · Michal Hocko

Quoted 11 lines · Vladimir Davydov

Quoted 10 lines · Michal Hocko

Quoted 6 lines · Vladimir Davydov

Suppose we are creating memcg cache A that could be merged with cache B of the same memcg. Since any memcg cache has the same parameters as its parent cache, parent caches PA and PB of memcg caches A and B must be mergeable too. That means PA was merged with PB on creation or vice versa, i.e. PA = PB. From that it follows that A = B, and we couldn't even try to create cache B, because it already exists - a contradiction.

I cannot tell I understand the above but I am totally not sure about the statement bellow.

Quoted 8 lines

Quoted 2 lines

Quoted 1 line

So let's remove unused code responsible for merging memcg caches.

How come the code was unused? find_mergeable called cache_match_memcg...

Oh, sorry for misleading comment. I mean the code handling merging of per-memcg caches is useless, AFAIU: if we find an alias for a per-memcg cache on kmem_cache_create_memcg(), the parent of the found alias must be the same as the parent_cache passed to kmem_cache_create_memcg(), but if it were so, we would never proceed to the memcg cache creation, because the cache we want to create already exists.

Quoted 6 lines

I am still not sure I understand this correctly. So the outcome of this patch is that compatible caches of different memcgs can be merged together? Sorry if this is a stupid question but I am not that familiar with this area much I am just seeing that cache_match_memcg goes away and my understanding of the function is that it should prevent from different memcg's caches merging.

Let me try to explain how I understand it.

What is cache merging/aliasing? When we create a cache (kmem_cache_create()), we first try to find a compatible cache that already exists and can handle requests from the new cache. If it is, we do not create any new caches, instead we simply increment the old cache refcount and return it.

Quoted 7 lines

What about memcgs? Currently, it operates in the same way, i.e. on memcg cache creation we also try to find a compatible cache of the same memcg first. But if there were such a cache, they parents would have been merged (i.e. it would be the same cache). That means we would not even get to this memcg cache creation, because it already exists. That's why the code handling memcg caches merging seems pointless to me.

IIRC, this may not always hold. Some of the properties are configurable via sysfs, and it might be that you haven't merged two parent caches because they properties differ, but would be fine merging the child caches.

If all properties we check are compile-time parameters, then it should be okay.

Quoted 43 lines · Glauber Costa

Quoted 35 lines · Vladimir Davydov

Quoted 20 lines · Michal Hocko

Quoted 11 lines · Vladimir Davydov

Quoted 10 lines · Michal Hocko

Quoted 6 lines · Vladimir Davydov

Suppose we are creating memcg cache A that could be merged with cache B of the same memcg. Since any memcg cache has the same parameters as its parent cache, parent caches PA and PB of memcg caches A and B must be mergeable too. That means PA was merged with PB on creation or vice versa, i.e. PA = PB. From that it follows that A = B, and we couldn't even try to create cache B, because it already exists - a contradiction.

I cannot tell I understand the above but I am totally not sure about the statement bellow.

Quoted 8 lines

Quoted 2 lines

Quoted 1 line

So let's remove unused code responsible for merging memcg caches.

How come the code was unused? find_mergeable called cache_match_memcg...

Oh, sorry for misleading comment. I mean the code handling merging of per-memcg caches is useless, AFAIU: if we find an alias for a per-memcg cache on kmem_cache_create_memcg(), the parent of the found alias must be the same as the parent_cache passed to kmem_cache_create_memcg(), but if it were so, we would never proceed to the memcg cache creation, because the cache we want to create already exists.

Quoted 6 lines

I am still not sure I understand this correctly. So the outcome of this patch is that compatible caches of different memcgs can be merged together? Sorry if this is a stupid question but I am not that familiar with this area much I am just seeing that cache_match_memcg goes away and my understanding of the function is that it should prevent from different memcg's caches merging.

Let me try to explain how I understand it.

What is cache merging/aliasing? When we create a cache (kmem_cache_create()), we first try to find a compatible cache that already exists and can handle requests from the new cache. If it is, we do not create any new caches, instead we simply increment the old cache refcount and return it.

Quoted 7 lines

What about memcgs? Currently, it operates in the same way, i.e. on memcg cache creation we also try to find a compatible cache of the same memcg first. But if there were such a cache, they parents would have been merged (i.e. it would be the same cache). That means we would not even get to this memcg cache creation, because it already exists. That's why the code handling memcg caches merging seems pointless to me.

Quoted 5 lines

IIRC, this may not always hold. Some of the properties are configurable via sysfs, and it might be that you haven't merged two parent caches because they properties differ, but would be fine merging the child caches.

If all properties we check are compile-time parameters, then it should be okay.

AFAIK, we decide if a cache should be merged only basing on its internal parameters, such as size, ctor, flags, align (see find_mergeable()), but they are the same for root and memcg caches.

The only way to disable slub merging is via the "slub_nomerge" kernel parameter, so it is impossible to get a situation when parents can not be merged, while children can.

The only point of concern may be so called boot caches (create_boot_cache()), which are forcefully not allowed to be merged by setting refcount = -1. There are actually only two of them kmem_cache and kmem_cache_node used for internal slub allocations. I guess it was done preliminary, and we should not merge them for memcgs neither.

To sum it up, if a particular root cache is allowed to be merged, it was allowed to be merged since its creation and all its children caches are also allowed to be merged. If merging was not allowed for a root cache when it was created, we should not merge its children caches.

Thanks.

What does this patch change? Actually, it introduces no functional changes - it only remove the code trying to find an alias for a memcg cache, because it will fail anyway. So this is rather a cleanup.

On Tue 04-02-14 19:27:19, Vladimir Davydov wrote:
[...]

Quoted 3 lines

What does this patch change? Actually, it introduces no functional changes - it only remove the code trying to find an alias for a memcg cache, because it will fail anyway. So this is rather a cleanup.

But this also means that two different memcgs might share the same cache and so the pages for that cache, no?

Actually it would depend on timing
because a new page would be chaged for the current allocator.

cachep->memcg_params->memcg == memcg would prevent from such a merge previously AFAICS, or am I still confused?

Quoted 7 lines · Michal Hocko

On Tue 04-02-14 19:27:19, Vladimir Davydov wrote:
[...]

Quoted 3 lines

What does this patch change? Actually, it introduces no functional changes - it only remove the code trying to find an alias for a memcg cache, because it will fail anyway. So this is rather a cleanup.

But this also means that two different memcgs might share the same cache and so the pages for that cache, no?

No, because in this patch I explicitly forbid to merge memcg caches by this hunk:

(quoted diff) +0 -0

@@ -200,9 +200,11 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg,
 const char *name, size_t size,
      */

Quoted 16 lines · Vladimir Davydov

Quoted 7 lines · Michal Hocko

On Tue 04-02-14 19:27:19, Vladimir Davydov wrote:
[...]

Quoted 3 lines

What does this patch change? Actually, it introduces no functional changes - it only remove the code trying to find an alias for a memcg cache, because it will fail anyway. So this is rather a cleanup.

But this also means that two different memcgs might share the same cache and so the pages for that cache, no?

No, because in this patch I explicitly forbid to merge memcg caches by this hunk:

(quoted diff) +0 -0

@@ -200,9 +200,11 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg,
 const char *name, size_t size,
      */
     flags &= CACHE_CREATE_MASK;
 

Review kmem_cache_create_memcg(struct mem_cgroup *memcg, · line 208

Quoted 8 lines

(quoted diff) +5 -3

-    s = __kmem_cache_alias(memcg, name, size, align, flags, ctor);
-    if (s)
-        goto out_unlock;
+    if (!memcg) {
+        s = __kmem_cache_alias(name, size, align, flags, ctor);
+        if (s)
+            goto out_unlock;
+    }

Ohh, that was the missing part. Thanks and sorry I have missed it.

Maybe it is worth mentioning in the changelog?