kmemcg: simplify work-flow
Subsystems (3)
v3 cover kmemcg: simplify work-flow
Hi,
This patch-set is a part of preparations for kmemcg re-parenting. It targets at simplifying kmemcg work-flows and synchronization.
First, it removes async per memcg cache destruction (see patches 1, 2). Now caches are only destroyed on memcg offline. That means the caches that are not empty on memcg offline will be leaked. However, they are already leaked, because memcg_cache_params::nr_pages normally never drops to 0 so the destruction work is never scheduled except kmem_cache_shrink is called explicitly. In the future I'm planning reaping such dead caches on vmpressure or periodically.
Second, it substitutes per memcg slab_caches_mutex's with the global memcg_slab_mutex, which should be taken during the whole per memcg cache creation/destruction path before the slab_mutex (see patch 3). This greatly simplifies synchronization among various per memcg cache creation/destruction paths.
I'm still not quite sure about the end picture, in particular I don't know whether we should reap dead memcgs' kmem caches periodically or try to merge them with their parents (see https://lkml.org/lkml/2014/4/20/38
for more details), but whichever way we choose, this set looks like a reasonable change to me, because it greatly simplifies kmemcg work-flows and eases further development.
Changes in v3:
- rebase on top of mmotm 2014-04-22-15-20 (minor, so I preserved Johannes' ACKs)
Changes in v2:
- substitute per memcg slab_caches_mutex's with the global memcg_slab_mutex and re-split the set.
v2: https://lkml.org/lkml/2014/4/18/42
v1: https://lkml.org/lkml/2014/4/9/298
Thanks,
Vladimir Davydov (3):
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: simplify synchronization scheme
include/linux/memcontrol.h | 15 +-- include/linux/slab.h | 8 +- mm/memcontrol.c | 231 +++++++++++++++----------------------------- mm/slab.c | 2 - mm/slab.h | 28 +----- mm/slab_common.c | 23 ++--- mm/slub.c | 2 - 7 files changed, 93 insertions(+), 216 deletions(-)
Signature · 1 line
1.7.10.4
PATCH v3 1/3 memcg, slab: do not schedule cache destruction when last page goes away
After a 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.
--- 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
@@ -509,7 +509,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
@@ -524,7 +524,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.
@@ -540,7 +539,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
@@ -128,11 +128,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>
PATCH v3 2/3 memcg, slab: merge memcg_{bind,release}_pages to memcg_{un}charge_slab
In reply to Vladimir DavydovCurrently 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.
--- 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
@@ -506,8 +506,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
@@ -2951,7 +2951,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 ;
@@ -2989,7 +2989,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)
@@ -3387,6 +3387,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
@@ -1706,7 +1706,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);
@@ -1742,7 +1741,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
@@ -120,18 +120,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) {
@@ -197,8 +185,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)
@@ -207,7 +194,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)
@@ -215,14 +202,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
@@ -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
PATCH v3 3/3 memcg, slab: simplify synchronization scheme
In reply to Vladimir DavydovAt present, we have the following mutexes protecting data related to per memcg kmem caches:
- slab_mutex. This one is held during the whole kmem cache creation and destruction paths. We also take it when updating per root cache memcg_caches arrays (see memcg_update_all_caches). As a result, taking it guarantees there will be no changes to any kmem cache (including per memcg). Why do we need something else then? The point is it is private to slab implementation and has some internal dependencies with other mutexes (get_online_cpus). So we just don't want to rely upon it and prefer to introduce additional mutexes instead.
- activate_kmem_mutex. Initially it was added to synchronize initializing kmem limit (memcg_activate_kmem). However, since we can grow per root cache memcg_caches arrays only on kmem limit initialization (see memcg_update_all_caches), we also employ it to protect against memcg_caches arrays relocation (e.g. see __kmem_cache_destroy_memcg_children).
- We have a convention not to take slab_mutex in memcontrol.c, but we want to walk over per memcg memcg_slab_caches lists there (e.g. for destroying all memcg caches on offline). So we have per memcg slab_caches_mutex's protecting those lists.
The mutexes are taken in the following order:
activate_kmem_mutex -> slab_mutex -> memcg::slab_caches_mutex
Such a syncrhonization scheme has a number of flaws, for instance:
- We can't call kmem_cache_{destroy,shrink} while walking over a memcg::memcg_slab_caches list due to locking order. As a result, in mem_cgroup_destroy_all_caches we schedule the memcg_cache_params::destroy work shrinking and destroying the cache.
- We don't have a mutex to synchronize per memcg caches destruction between memcg offline (mem_cgroup_destroy_all_caches) and root cache destruction (__kmem_cache_destroy_memcg_children). Currently we just don't bother about it.
This patch simplifies it by substituting per memcg slab_caches_mutex's with the global memcg_slab_mutex. It will be held whenever a new per memcg cache is created or destroyed, so it protects per root cache memcg_caches arrays and per memcg memcg_slab_caches lists. The locking order is following:
activate_kmem_mutex -> memcg_slab_mutex -> slab_mutex
This allows us to call kmem_cache_{create,shrink,destroy} under the memcg_slab_mutex. As a result, we don't need memcg_cache_params::destroy work any more - we can simply destroy caches while iterating over a per memcg slab caches list.
Also using the global mutex simplifies synchronization between concurrent per memcg caches creation/destruction, e.g. mem_cgroup_destroy_all_caches vs __kmem_cache_destroy_memcg_children.
The downside of this is that we substitute per-memcg slab_caches_mutex's with a hummer-like global mutex, but since we already take either the slab_mutex or the cgroup_mutex along with a memcg::slab_caches_mutex, it shouldn't hurt concurrency a lot.
--- include/linux/memcontrol.h | 10 --- include/linux/slab.h | 6 +- mm/memcontrol.c | 150 +++++++++++++++++--------------------------- mm/slab_common.c | 23 +++---- 4 files changed, 69 insertions(+), 120 deletions(-)
include/linux/memcontrol.h +0 -10
@@ -497,8 +497,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);
@@ -640,14 +638,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 -4
@@ -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 *);
@@ -525,8 +526,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;
@@ -540,7 +539,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 +59 -91
264 lines not on this page · raw message
mm/slab_common.c +8 -15
@@ -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);
@@ -270,9 +269,10 @@ 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 ();
@@ -280,14 +280,6 @@ void kmem_cache_create_memcg(struct mem_cgroup *memcg, struct kmem_cache *root_cmutex_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;
@@ -296,14 +288,18 @@ 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_mems ();put_online_cpus (); + +return s; }static int kmem_cache_destroy_memcg_children (struct kmem_cache *s)
@@ -342,11 +338,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>