Skip to contentSkip to conversation

memcg-vs-slab cleanup

PATCH Superseded Vladimir Davydov 3 participants 10 messages Apr 9, 2014 – Apr 15, 2014

Subsystems (3)
Memory management memcg Slab allocator

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Vladimir Davydov

author
#0 lore raw

cover memcg-vs-slab cleanup

Hi,

This patchset does a bit of cleanup in the kmemcg implementation hopefully making it more readable.

It requires the following two patches applied on top of the mmotm tree:
[PATCH -mm v2 1/2] sl[au]b: charge slabs to kmemcg explicitly [PATCH -mm v2.2] mm: get rid of __GFP_KMEMCG
(see https://lkml.org/lkml/2014/4/1/100)

Thanks,

Vladimir Davydov (4):
memcg, slab: do not schedule cache destruction when last page goes
away
memcg, slab: merge memcg_{bind,release}_pages to
memcg_{un}charge_slab
memcg, slab: change memcg::slab_caches_mutex vs slab_mutex locking
order
memcg, slab: remove memcg_cache_params::destroy work

 include/linux/memcontrol.h |   15 +---
 include/linux/slab.h       |    8 +-
 mm/memcontrol.c            |  209 +++++++++++++++++---------------------------
 mm/slab.c                  |    2 -
 mm/slab.h                  |   28 +-----
 mm/slab_common.c           |   22 ++---
 mm/slub.c                  |    2 -
 7 files changed, 94 insertions(+), 192 deletions(-)
Signature · 1 line
1.7.10.4
author
#1 lore raw

PATCH 1/4 memcg, slab: do not schedule cache destruction when last page goes away

After the memcg is offlined, we mark its kmem caches that cannot be deleted right now due to pending objects as dead by setting the memcg_cache_params::dead flag, so that memcg_release_pages will schedule cache destruction (memcg_cache_params::destroy) as soon as the last slab of the cache is freed (memcg_cache_params::nr_pages drops to zero).

I guess the idea was to destroy the caches as soon as possible, i.e. immediately after freeing the last object. However, it just doesn't work that way, because kmem caches always preserve some pages for the sake of performance, so that nr_pages never gets to zero unless the cache is shrunk explicitly using kmem_cache_shrink. Of course, we could account the total number of objects on the cache or check if all the slabs allocated for the cache are empty on kmem_cache_free and schedule destruction if so, but that would be too costly.

Thus we have a piece of code that works only when we explicitly call kmem_cache_shrink, but complicates the whole picture a lot. Moreover, it's racy in fact. For instance, kmem_cache_shrink may free the last slab and thus schedule cache destruction before it finishes checking that the cache is empty, which can lead to use-after-free.

So I propose to remove this async cache destruction from memcg_release_pages, and check if the cache is empty explicitly after calling kmem_cache_shrink instead. This will simplify things a lot w/o introducing any functional changes.

And regarding dead memcg caches (i.e. those that are left hanging around after memcg offline for they have objects), I suppose we should reap them either periodically or on vmpressure as Glauber suggested initially. I'm going to implement this later.

Signed-off-by Vladimir Davydov <[email protected]>
---
 include/linux/memcontrol.h |    1 -
 include/linux/slab.h       |    2 --
 mm/memcontrol.c            |   63 ++------------------------------------------
 mm/slab.h                  |    7 ++---
 4 files changed, 4 insertions(+), 69 deletions(-)
include/linux/memcontrol.h +0 -1
index 13acdb5259f5..41b381412367 100644
@@ -515,7 +515,6 @@ __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp);
 int memcg_charge_kmem(struct mem_cgroup *memcg, gfp_t gfp, u64 size);
 void memcg_uncharge_kmem(struct mem_cgroup *memcg, u64 size);
 
-void mem_cgroup_destroy_cache(struct kmem_cache *cachep);
 int __kmem_cache_destroy_memcg_children(struct kmem_cache *s);
 
 /**
include/linux/slab.h +0 -2
index eaaf2f69c54d..0217e05e1d83 100644
@@ -513,7 +513,6 @@ static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
  * @memcg: pointer to the memcg this cache belongs to
  * @list: list_head for the list of all caches in this memcg
  * @root_cache: pointer to the global, root cache, this cache was derived from
- * @dead: set to true after the memcg dies; the cache may still be around.
  * @nr_pages: number of pages that belongs to this cache.
  * @destroy: worker to be called whenever we are ready, or believe we may be
  *           ready, to destroy this cache.

@@ -529,7 +528,6 @@ struct memcg_cache_params {
 			struct mem_cgroup *memcg;
 			struct list_head list;
 			struct kmem_cache *root_cache;
-			bool dead;
 			atomic_t nr_pages;
 			struct work_struct destroy;
 		};
mm/memcontrol.c +2 -61

85 lines not on this page · raw message

mm/slab.h +2 -5
index 3db3c52f80a2..efe14d420010 100644
@@ -127,11 +127,8 @@ static inline void memcg_bind_pages(struct kmem_cache *s, int order)
 
 static inline void memcg_release_pages(struct kmem_cache *s, int order)
 {
-	if (is_root_cache(s))
-		return;
-
-	if (atomic_sub_and_test((1 << order), &s->memcg_params->nr_pages))
-		mem_cgroup_destroy_cache(s);
+	if (!is_root_cache(s))
+		atomic_sub(1 << order, &s->memcg_params->nr_pages);
 }
 
 static inline bool slab_equal_or_root(struct kmem_cache *s,
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>
maintainer
#2 lore raw

Re: [PATCH -mm 1/4] memcg, slab: do not schedule cache destruction when last page goes away

Quoted 26 lines · Vladimir Davydov

After the memcg is offlined, we mark its kmem caches that cannot be deleted right now due to pending objects as dead by setting the memcg_cache_params::dead flag, so that memcg_release_pages will schedule cache destruction (memcg_cache_params::destroy) as soon as the last slab of the cache is freed (memcg_cache_params::nr_pages drops to zero).

I guess the idea was to destroy the caches as soon as possible, i.e. immediately after freeing the last object. However, it just doesn't work that way, because kmem caches always preserve some pages for the sake of performance, so that nr_pages never gets to zero unless the cache is shrunk explicitly using kmem_cache_shrink. Of course, we could account the total number of objects on the cache or check if all the slabs allocated for the cache are empty on kmem_cache_free and schedule destruction if so, but that would be too costly.

Thus we have a piece of code that works only when we explicitly call kmem_cache_shrink, but complicates the whole picture a lot. Moreover, it's racy in fact. For instance, kmem_cache_shrink may free the last slab and thus schedule cache destruction before it finishes checking that the cache is empty, which can lead to use-after-free.

So I propose to remove this async cache destruction from memcg_release_pages, and check if the cache is empty explicitly after calling kmem_cache_shrink instead. This will simplify things a lot w/o introducing any functional changes.

Quoted 4 lines

And regarding dead memcg caches (i.e. those that are left hanging around after memcg offline for they have objects), I suppose we should reap them either periodically or on vmpressure as Glauber suggested initially. I'm going to implement this later.

memcg_release_pages() can be called after cgroup destruction, and thus it *must* ensure that the now-empty cache is destroyed - or we'll leak it.

There is no excuse to downgrade to periodic reaping when we already directly hook into the event that makes the cache empty. If slab needs to hold on to the cache for slightly longer than the final memcg_release_pages(), then it should grab a refcount to it.

author
#3 lore raw

Re: [PATCH -mm 1/4] memcg, slab: do not schedule cache destruction when last page goes away

Hi Johannes,

Quoted 27 lines · Johannes Weiner
Quoted 26 lines · Vladimir Davydov

After the memcg is offlined, we mark its kmem caches that cannot be deleted right now due to pending objects as dead by setting the memcg_cache_params::dead flag, so that memcg_release_pages will schedule cache destruction (memcg_cache_params::destroy) as soon as the last slab of the cache is freed (memcg_cache_params::nr_pages drops to zero).

I guess the idea was to destroy the caches as soon as possible, i.e. immediately after freeing the last object. However, it just doesn't work that way, because kmem caches always preserve some pages for the sake of performance, so that nr_pages never gets to zero unless the cache is shrunk explicitly using kmem_cache_shrink. Of course, we could account the total number of objects on the cache or check if all the slabs allocated for the cache are empty on kmem_cache_free and schedule destruction if so, but that would be too costly.

Thus we have a piece of code that works only when we explicitly call kmem_cache_shrink, but complicates the whole picture a lot. Moreover, it's racy in fact. For instance, kmem_cache_shrink may free the last slab and thus schedule cache destruction before it finishes checking that the cache is empty, which can lead to use-after-free.

So I propose to remove this async cache destruction from memcg_release_pages, and check if the cache is empty explicitly after calling kmem_cache_shrink instead. This will simplify things a lot w/o introducing any functional changes.

Quoted 7 lines
Quoted 4 lines

And regarding dead memcg caches (i.e. those that are left hanging around after memcg offline for they have objects), I suppose we should reap them either periodically or on vmpressure as Glauber suggested initially. I'm going to implement this later.

memcg_release_pages() can be called after cgroup destruction, and thus it *must* ensure that the now-empty cache is destroyed - or we'll leak it.

But the problem is that we already leak *all* per memcg caches that are not empty by the time memcg is offlined, because even when all objects of such a cache are freed, it will still have some pages cached per-cpu/node in both slab and slub implementations. That said, at present the code scheduling cache destruction from memcg_release_pages only works when we call kmem_cache_shrink.

I propose to remove this piece of code from memcg_release_pages and instead call kmem_cache_destroy explicitly after kmem_cache_shrink if the cache becomes empty. The caches that still have objects after memcg offline should be shrunk either periodically or on vmpressure. That would greatly simplify synchronization.

Quoted 4 lines

There is no excuse to downgrade to periodic reaping when we already directly hook into the event that makes the cache empty. If slab needs to hold on to the cache for slightly longer than the final memcg_release_pages(), then it should grab a refcount to it.

IMO that wouldn't be a downgrade - the code in memcg_release_pages already does not work as expected, at least it doesn't initiate cache destruction when the last object goes away.

If we really want to destroy caches as soon as possible we could:

1) Count active objects per cache instead of pages as we do now. That would be too costly IMO.

2) When freeing an object of a dead memcg cache, initiate thorough check if the cache is really empty and destroy it then. That could be implemented by poking the reaping thread on kfree, and actually does not require the schedule_work in memcg_release_pages IMO.

Thanks.

#4 lore raw

Re: [PATCH -mm 1/4] memcg, slab: do not schedule cache destruction when last page goes away

Quoted 4 lines · Vladimir Davydov

2) When freeing an object of a dead memcg cache, initiate thorough check if the cache is really empty and destroy it then. That could be implemented by poking the reaping thread on kfree, and actually does not require the schedule_work in memcg_release_pages IMO.

There is already logic in both slub and slab that does that on cache close.

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

author
#5 lore raw

Re: [PATCH -mm 1/4] memcg, slab: do not schedule cache destruction when last page goes away

Hi Christoph,

15.04.2014 19:17, Christoph Lameter:

Quoted 9 lines · Vladimir Davydov
Quoted 5 lines

2) When freeing an object of a dead memcg cache, initiate thorough check if the cache is really empty and destroy it then. That could be implemented by poking the reaping thread on kfree, and actually does not require the schedule_work in memcg_release_pages IMO.

There is already logic in both slub and slab that does that on cache close.

Yeah, but here the question is when we should close caches left after memcg offline. Obviously we should do it after all objects of such a cache have gone, but when exactly? Do it immediately after the last kfree (have to count objects per cache then AFAIU) or may be check periodically (or on vmpressure) that the cache is empty by issuing kmem_cache_shrink and looking if memcg_params::nr_pages = 0?

Thanks.

#6 lore raw

Re: [PATCH -mm 1/4] memcg, slab: do not schedule cache destruction when last page goes away

Quoted 9 lines · Vladimir Davydov
Quoted 2 lines

There is already logic in both slub and slab that does that on cache close.

Yeah, but here the question is when we should close caches left after memcg offline. Obviously we should do it after all objects of such a cache have gone, but when exactly? Do it immediately after the last kfree (have to count objects per cache then AFAIU) or may be check periodically (or on vmpressure) that the cache is empty by issuing kmem_cache_shrink and looking if memcg_params::nr_pages = 0?

Guess check once in a while if you have no other way to determine this. A hook in kfree() would impact all users.

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

author
#7 lore raw

PATCH 2/4 memcg, slab: merge memcg_{bind,release}_pages to memcg_{un}charge_slab

In reply to Vladimir Davydov

Currently we have two pairs of kmemcg-related functions that are called on slab alloc/free. The first is memcg_{bind,release}_pages that count the total number of pages allocated on a kmem cache. The second is memcg_{un}charge_slab that {un}charge slab pages to kmemcg resource counter. Let's just merge them to keep the code clean.

Signed-off-by Vladimir Davydov <[email protected]>
---
 include/linux/memcontrol.h |    4 ++--
 mm/memcontrol.c            |   22 ++++++++++++++++++++--
 mm/slab.c                  |    2 --
 mm/slab.h                  |   25 ++-----------------------
 mm/slub.c                  |    2 --
 5 files changed, 24 insertions(+), 31 deletions(-)
include/linux/memcontrol.h +2 -2
index 41b381412367..da87fa2124cf 100644
@@ -512,8 +512,8 @@ void memcg_update_array_size(int num_groups);
 struct kmem_cache *
 __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp);
 
-int memcg_charge_kmem(struct mem_cgroup *memcg, gfp_t gfp, u64 size);
-void memcg_uncharge_kmem(struct mem_cgroup *memcg, u64 size);
+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);
mm/memcontrol.c +20 -2
index 78431620d225..253d6a064d89 100644
@@ -2944,7 +2944,7 @@ static int mem_cgroup_slabinfo_read(struct seq_file *m, void *v)
 }
 #endif
 
-int memcg_charge_kmem(struct mem_cgroup *memcg, gfp_t gfp, u64 size)
+static int memcg_charge_kmem(struct mem_cgroup *memcg, gfp_t gfp, u64 size)
 {
 	struct res_counter *fail_res;
 	int ret = 0;

@@ -2982,7 +2982,7 @@ int memcg_charge_kmem(struct mem_cgroup *memcg, gfp_t gfp, u64 size)
 	return ret;
 }
 
-void memcg_uncharge_kmem(struct mem_cgroup *memcg, u64 size)
+static void memcg_uncharge_kmem(struct mem_cgroup *memcg, u64 size)
 {
 	res_counter_uncharge(&memcg->res, size);
 	if (do_swap_account)

@@ -3380,6 +3380,24 @@ static void memcg_create_cache_enqueue(struct mem_cgroup *memcg,
 	__memcg_create_cache_enqueue(memcg, cachep);
 	memcg_resume_kmem_account();
 }
+
+int __memcg_charge_slab(struct kmem_cache *cachep, gfp_t gfp, int order)
+{
+	int res;
+
+	res = memcg_charge_kmem(cachep->memcg_params->memcg, gfp,
+				PAGE_SIZE << order);
+	if (!res)
+		atomic_add(1 << order, &cachep->memcg_params->nr_pages);
+	return res;
+}
+
+void __memcg_uncharge_slab(struct kmem_cache *cachep, int order)
+{
+	memcg_uncharge_kmem(cachep->memcg_params->memcg, PAGE_SIZE << order);
+	atomic_sub(1 << order, &cachep->memcg_params->nr_pages);
+}
+
 /*
  * Return the kmem_cache we're supposed to use for a slab allocation.
  * We try to use the current memcg's version of the cache.
mm/slab.c +0 -2
index af126a37dafd..cd645dbaab0a 100644
@@ -1689,7 +1689,6 @@ static struct page *kmem_getpages(struct kmem_cache *cachep, gfp_t flags,
 	__SetPageSlab(page);
 	if (page->pfmemalloc)
 		SetPageSlabPfmemalloc(page);
-	memcg_bind_pages(cachep, cachep->gfporder);
 
 	if (kmemcheck_enabled && !(cachep->flags & SLAB_NOTRACK)) {
 		kmemcheck_alloc_shadow(page, cachep->gfporder, flags, nodeid);

@@ -1725,7 +1724,6 @@ static void kmem_freepages(struct kmem_cache *cachep, struct page *page)
 	page_mapcount_reset(page);
 	page->mapping = NULL;
 
-	memcg_release_pages(cachep, cachep->gfporder);
 	if (current->reclaim_state)
 		current->reclaim_state->reclaimed_slab += nr_freed;
 	__free_pages(page, cachep->gfporder);
mm/slab.h +2 -23
index efe14d420010..11eb623f0e61 100644
@@ -119,18 +119,6 @@ static inline bool is_root_cache(struct kmem_cache *s)
 	return !s->memcg_params || s->memcg_params->is_root_cache;
 }
 
-static inline void memcg_bind_pages(struct kmem_cache *s, int order)
-{
-	if (!is_root_cache(s))
-		atomic_add(1 << order, &s->memcg_params->nr_pages);
-}
-
-static inline void memcg_release_pages(struct kmem_cache *s, int order)
-{
-	if (!is_root_cache(s))
-		atomic_sub(1 << order, &s->memcg_params->nr_pages);
-}
-
 static inline bool slab_equal_or_root(struct kmem_cache *s,
 					struct kmem_cache *p)
 {

@@ -196,8 +184,7 @@ static __always_inline int memcg_charge_slab(struct kmem_cache *s,
 		return 0;
 	if (is_root_cache(s))
 		return 0;
-	return memcg_charge_kmem(s->memcg_params->memcg, gfp,
-				 PAGE_SIZE << order);
+	return __memcg_charge_slab(s, gfp, order);
 }
 
 static __always_inline void memcg_uncharge_slab(struct kmem_cache *s, int order)

@@ -206,7 +193,7 @@ static __always_inline void memcg_uncharge_slab(struct kmem_cache *s, int order)
 		return;
 	if (is_root_cache(s))
 		return;
-	memcg_uncharge_kmem(s->memcg_params->memcg, PAGE_SIZE << order);
+	__memcg_uncharge_slab(s, order);
 }
 #else
 static inline bool is_root_cache(struct kmem_cache *s)

@@ -214,14 +201,6 @@ static inline bool is_root_cache(struct kmem_cache *s)
 	return true;
 }
 
-static inline void memcg_bind_pages(struct kmem_cache *s, int order)
-{
-}
-
-static inline void memcg_release_pages(struct kmem_cache *s, int order)
-{
-}
-
 static inline bool slab_equal_or_root(struct kmem_cache *s,
 				      struct kmem_cache *p)
 {
mm/slub.c +0 -2
index fa7a1817835e..c17d8b9975be 100644
@@ -1427,7 +1427,6 @@ static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
 
 	order = compound_order(page);
 	inc_slabs_node(s, page_to_nid(page), page->objects);
-	memcg_bind_pages(s, order);
 	page->slab_cache = s;
 	__SetPageSlab(page);
 	if (page->pfmemalloc)

@@ -1478,7 +1477,6 @@ static void __free_slab(struct kmem_cache *s, struct page *page)
 	__ClearPageSlabPfmemalloc(page);
 	__ClearPageSlab(page);
 
-	memcg_release_pages(s, order);
 	page_mapcount_reset(page);
 	if (current->reclaim_state)
 		current->reclaim_state->reclaimed_slab += pages;
Signature · 1 line
1.7.10.4
author
#8 lore raw

PATCH 3/4 memcg, slab: change memcg::slab_caches_mutex vs slab_mutex locking order

In reply to Vladimir Davydov

When creating/destroying a kmem cache for a memcg we must hold two locks
- memcg::slab_caches_mutex, which protects memcg caches list, and the slab_mutex taken by kmem_cache_create/destroy. Currently we first take the slab_mutex and then in memcg_{un}register_cache take the memcg's slab_caches_mutex in order to synchronize changes to memcg caches list.

Such a locking order create the ugly dependency, which prevents us from calling kmem_cache_shrink/destroy while iterating per-memcg caches list, because we must hold memcg::slab_caches_mutex to protect against changes to the list. As a result, in mem_cgroup_destroy_all_caches we can't just shrink and destroy a memcg cache if it's get emptied. Instead we schedule an async work that does the trick.

This patch changes the locking order of the two mutexes to opposite so that we could get rid of the memcg cache destruction work (this is done in the next patch).

Signed-off-by Vladimir Davydov <[email protected]>
---
 include/linux/memcontrol.h |   10 -----
 include/linux/slab.h       |    3 +-
 mm/memcontrol.c            |   89 +++++++++++++++++++++++---------------------
 mm/slab_common.c           |   22 ++++-------
 4 files changed, 56 insertions(+), 68 deletions(-)
include/linux/memcontrol.h +0 -10
index da87fa2124cf..d815b26f8086 100644
@@ -503,8 +503,6 @@ char *memcg_create_cache_name(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);
-void memcg_register_cache(struct kmem_cache *s);
-void memcg_unregister_cache(struct kmem_cache *s);
 
 int memcg_update_cache_size(struct kmem_cache *s, int num_groups);
 void memcg_update_array_size(int num_groups);

@@ -646,14 +644,6 @@ static inline void memcg_free_cache_params(struct kmem_cache *s)
 {
 }
 
-static inline void memcg_register_cache(struct kmem_cache *s)
-{
-}
-
-static inline void memcg_unregister_cache(struct kmem_cache *s)
-{
-}
-
 static inline struct kmem_cache *
 memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
 {
include/linux/slab.h +2 -1
index 0217e05e1d83..22ebcf475814 100644
@@ -116,7 +116,8 @@ struct kmem_cache *kmem_cache_create(const char *, size_t, size_t,
 			unsigned long,
 			void (*)(void *));
 #ifdef CONFIG_MEMCG_KMEM
-void kmem_cache_create_memcg(struct mem_cgroup *, struct kmem_cache *);
+struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *,
+					   struct kmem_cache *);
 #endif
 void kmem_cache_destroy(struct kmem_cache *);
 int kmem_cache_shrink(struct kmem_cache *);
mm/memcontrol.c +47 -42

145 lines not on this page · raw message

mm/slab_common.c +7 -15
index cab4c49b3e8c..b3968ca1e55d 100644
@@ -160,7 +160,6 @@ do_kmem_cache_create(char *name, size_t object_size, size_t size, size_t align,
 
 	s->refcount = 1;
 	list_add(&s->list, &slab_caches);
-	memcg_register_cache(s);
 out:
 	if (err)
 		return ERR_PTR(err);

@@ -266,22 +265,15 @@ EXPORT_SYMBOL(kmem_cache_create);
  * requests going from @memcg to @root_cache. The new cache inherits properties
  * from its parent.
  */
-void kmem_cache_create_memcg(struct mem_cgroup *memcg, struct kmem_cache *root_cache)
+struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *memcg,
+					   struct kmem_cache *root_cache)
 {
-	struct kmem_cache *s;
+	struct kmem_cache *s = NULL;
 	char *cache_name;
 
 	get_online_cpus();
 	mutex_lock(&slab_mutex);
 
-	/*
-	 * Since per-memcg caches are created asynchronously on first
-	 * allocation (see memcg_kmem_get_cache()), several threads can try to
-	 * create the same cache, but only one of them may succeed.
-	 */
-	if (cache_from_memcg_idx(root_cache, memcg_cache_id(memcg)))
-		goto out_unlock;
-
 	cache_name = memcg_create_cache_name(memcg, root_cache);
 	if (!cache_name)
 		goto out_unlock;

@@ -290,12 +282,15 @@ void kmem_cache_create_memcg(struct mem_cgroup *memcg, struct kmem_cache *root_c
 				 root_cache->size, root_cache->align,
 				 root_cache->flags, root_cache->ctor,
 				 memcg, root_cache);
-	if (IS_ERR(s))
+	if (IS_ERR(s)) {
 		kfree(cache_name);
+		s = NULL;
+	}
 
 out_unlock:
 	mutex_unlock(&slab_mutex);
 	put_online_cpus();
+	return s;
 }
 
 static int kmem_cache_destroy_memcg_children(struct kmem_cache *s)

@@ -332,11 +327,8 @@ void kmem_cache_destroy(struct kmem_cache *s)
 		goto out_unlock;
 
 	list_del(&s->list);
-	memcg_unregister_cache(s);
-
 	if (__kmem_cache_shutdown(s) != 0) {
 		list_add(&s->list, &slab_caches);
-		memcg_register_cache(s);
 		printk(KERN_ERR "kmem_cache_destroy %s: "
 		       "Slab cache still has objects\n", s->name);
 		dump_stack();
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>
author
#9 lore raw

PATCH 4/4 memcg, slab: remove memcg_cache_params::destroy work

In reply to Vladimir Davydov

There is no need to schedule a work for kmem_cache_shrink/destroy from mem_cgroup_destroy_all_caches any more, because we can now call them directly under memcg's slab_caches_mutex.

Signed-off-by Vladimir Davydov <[email protected]>
---
 include/linux/slab.h |    3 ---
 mm/memcontrol.c      |   45 +++++++++++++++++----------------------------
 2 files changed, 17 insertions(+), 31 deletions(-)
include/linux/slab.h +0 -3
index 22ebcf475814..5b48ef7c0cea 100644
@@ -515,8 +515,6 @@ static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
  * @list: list_head for the list of all caches in this memcg
  * @root_cache: pointer to the global, root cache, this cache was derived from
  * @nr_pages: number of pages that belongs to this cache.
- * @destroy: worker to be called whenever we are ready, or believe we may be
- *           ready, to destroy this cache.
  */
 struct memcg_cache_params {
 	bool is_root_cache;

@@ -530,7 +528,6 @@ struct memcg_cache_params {
 			struct list_head list;
 			struct kmem_cache *root_cache;
 			atomic_t nr_pages;
-			struct work_struct destroy;
 		};
 	};
 };
mm/memcontrol.c +17 -28

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