* [PATCH -mm 1/2] memcg: get rid of memcg_create_cache_name @ 2014-05-07 8:15 Vladimir Davydov 2014-05-07 8:15 ` [PATCH -mm 2/2] memcg: cleanup kmem cache creation/destruction functions naming Vladimir Davydov 2014-05-07 9:51 ` [PATCH -mm 1/2] memcg: get rid of memcg_create_cache_name Michal Hocko 0 siblings, 2 replies; 10+ messages in thread From: Vladimir Davydov @ 2014-05-07 8:15 UTC (permalink / raw) To: akpm; +Cc: hannes, mhocko, linux-kernel, linux-mm Instead of calling back to memcontrol.c from kmem_cache_create_memcg in order to just create the name of a per memcg cache, let's allocate it in place. We only need to pass the memcg name to kmem_cache_create_memcg for that - everything else can be done in slab_common.c. Signed-off-by: Vladimir Davydov <vdavydov@parallels.com> --- Initially this was a part of "memcg/kmem: cleanup naming and callflows" patch set (https://lkml.org/lkml/2014/4/27/345). include/linux/memcontrol.h | 2 -- include/linux/slab.h | 3 ++- mm/memcontrol.c | 33 +++++++++------------------------ mm/slab_common.c | 7 +++++-- 4 files changed, 16 insertions(+), 29 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 6c59056f4bc6..7b639ab48aa8 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -501,8 +501,6 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order); int memcg_cache_id(struct mem_cgroup *memcg); -char *memcg_create_cache_name(struct mem_cgroup *memcg, - struct kmem_cache *root_cache); int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s, struct kmem_cache *root_cache); void memcg_free_cache_params(struct kmem_cache *s); diff --git a/include/linux/slab.h b/include/linux/slab.h index ecbec9ccb80d..86e5b26fbdab 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -117,7 +117,8 @@ struct kmem_cache *kmem_cache_create(const char *, size_t, size_t, void (*)(void *)); #ifdef CONFIG_MEMCG_KMEM struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *, - struct kmem_cache *); + struct kmem_cache *, + const char *); #endif void kmem_cache_destroy(struct kmem_cache *); int kmem_cache_shrink(struct kmem_cache *); diff --git a/mm/memcontrol.c b/mm/memcontrol.c index f381239ab402..f401f227a099 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -3101,29 +3101,6 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups) return 0; } -char *memcg_create_cache_name(struct mem_cgroup *memcg, - struct kmem_cache *root_cache) -{ - static char *buf; - - /* - * We need a mutex here to protect the shared buffer. Since this is - * expected to be called only on cache creation, we can employ the - * slab_mutex for that purpose. - */ - lockdep_assert_held(&slab_mutex); - - if (!buf) { - buf = kmalloc(NAME_MAX + 1, GFP_KERNEL); - if (!buf) - return NULL; - } - - cgroup_name(memcg->css.cgroup, buf, NAME_MAX + 1); - return kasprintf(GFP_KERNEL, "%s(%d:%s)", root_cache->name, - memcg_cache_id(memcg), buf); -} - int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s, struct kmem_cache *root_cache) { @@ -3164,6 +3141,7 @@ void memcg_free_cache_params(struct kmem_cache *s) static void memcg_kmem_create_cache(struct mem_cgroup *memcg, struct kmem_cache *root_cache) { + static char *memcg_name_buf; struct kmem_cache *cachep; int id; @@ -3179,7 +3157,14 @@ static void memcg_kmem_create_cache(struct mem_cgroup *memcg, if (cache_from_memcg_idx(root_cache, id)) return; - cachep = kmem_cache_create_memcg(memcg, root_cache); + if (!memcg_name_buf) { + memcg_name_buf = kmalloc(NAME_MAX + 1, GFP_KERNEL); + if (!memcg_name_buf) + return; + } + + cgroup_name(memcg->css.cgroup, memcg_name_buf, NAME_MAX + 1); + cachep = kmem_cache_create_memcg(memcg, root_cache, memcg_name_buf); /* * If we could not create a memcg cache, do not complain, because * that's not critical at all as we can always proceed with the root diff --git a/mm/slab_common.c b/mm/slab_common.c index 7e348cff814d..32175617cb75 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -264,13 +264,15 @@ EXPORT_SYMBOL(kmem_cache_create); * kmem_cache_create_memcg - Create a cache for a memory cgroup. * @memcg: The memory cgroup the new cache is for. * @root_cache: The parent of the new cache. + * @memcg_name: The name of the memory cgroup (used for naming the new cache). * * This function attempts to create a kmem cache that will serve allocation * requests going from @memcg to @root_cache. The new cache inherits properties * from its parent. */ struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *memcg, - struct kmem_cache *root_cache) + struct kmem_cache *root_cache, + const char *memcg_name) { struct kmem_cache *s = NULL; char *cache_name; @@ -280,7 +282,8 @@ struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *memcg, mutex_lock(&slab_mutex); - cache_name = memcg_create_cache_name(memcg, root_cache); + cache_name = kasprintf(GFP_KERNEL, "%s(%d:%s)", root_cache->name, + memcg_cache_id(memcg), memcg_name); if (!cache_name) goto out_unlock; -- 1.7.10.4 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH -mm 2/2] memcg: cleanup kmem cache creation/destruction functions naming 2014-05-07 8:15 [PATCH -mm 1/2] memcg: get rid of memcg_create_cache_name Vladimir Davydov @ 2014-05-07 8:15 ` Vladimir Davydov 2014-05-07 12:45 ` Michal Hocko 2014-05-07 9:51 ` [PATCH -mm 1/2] memcg: get rid of memcg_create_cache_name Michal Hocko 1 sibling, 1 reply; 10+ messages in thread From: Vladimir Davydov @ 2014-05-07 8:15 UTC (permalink / raw) To: akpm; +Cc: hannes, mhocko, linux-kernel, linux-mm Current names are rather inconsistent. Let's try to improve them. Brief change log: ** old name ** ** new name ** kmem_cache_create_memcg kmem_cache_request_memcg_copy memcg_kmem_create_cache memcg_copy_kmem_cache memcg_kmem_destroy_cache memcg_destroy_kmem_cache_copy __kmem_cache_destroy_memcg_children __kmem_cache_destroy_memcg_copies kmem_cache_destroy_memcg_children kmem_cache_destroy_memcg_copies mem_cgroup_destroy_all_caches memcg_destroy_kmem_cache_copies create_work memcg_kmem_cache_copy_work memcg_create_cache_work_func memcg_kmem_cache_copy_work_func memcg_create_cache_enqueue memcg_schedule_kmem_cache_copy Signed-off-by: Vladimir Davydov <vdavydov@parallels.com> --- Initially this was a part of "memcg/kmem: cleanup naming and callflows" patch set (https://lkml.org/lkml/2014/4/27/345). include/linux/memcontrol.h | 2 +- include/linux/slab.h | 6 ++--- mm/memcontrol.c | 58 +++++++++++++++++++++----------------------- mm/slab_common.c | 29 +++++++++++----------- 4 files changed, 45 insertions(+), 50 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 7b639ab48aa8..0958f0361af0 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -514,7 +514,7 @@ __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp); int __memcg_charge_slab(struct kmem_cache *cachep, gfp_t gfp, int order); void __memcg_uncharge_slab(struct kmem_cache *cachep, int order); -int __kmem_cache_destroy_memcg_children(struct kmem_cache *s); +int __kmem_cache_destroy_memcg_copies(struct kmem_cache *s); /** * memcg_kmem_newpage_charge: verify if a new kmem allocation is allowed. diff --git a/include/linux/slab.h b/include/linux/slab.h index 86e5b26fbdab..e08e369d42ac 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -116,9 +116,9 @@ struct kmem_cache *kmem_cache_create(const char *, size_t, size_t, unsigned long, void (*)(void *)); #ifdef CONFIG_MEMCG_KMEM -struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *, - struct kmem_cache *, - const char *); +struct kmem_cache *kmem_cache_request_memcg_copy(struct kmem_cache *, + struct mem_cgroup *, + const char *); #endif void kmem_cache_destroy(struct kmem_cache *); int kmem_cache_shrink(struct kmem_cache *); diff --git a/mm/memcontrol.c b/mm/memcontrol.c index f401f227a099..52a31f6876ce 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -3138,8 +3138,8 @@ void memcg_free_cache_params(struct kmem_cache *s) kfree(s->memcg_params); } -static void memcg_kmem_create_cache(struct mem_cgroup *memcg, - struct kmem_cache *root_cache) +static void memcg_copy_kmem_cache(struct mem_cgroup *memcg, + struct kmem_cache *root_cache) { static char *memcg_name_buf; struct kmem_cache *cachep; @@ -3164,7 +3164,7 @@ static void memcg_kmem_create_cache(struct mem_cgroup *memcg, } cgroup_name(memcg->css.cgroup, memcg_name_buf, NAME_MAX + 1); - cachep = kmem_cache_create_memcg(memcg, root_cache, memcg_name_buf); + cachep = kmem_cache_request_memcg_copy(root_cache, memcg, memcg_name_buf); /* * If we could not create a memcg cache, do not complain, because * that's not critical at all as we can always proceed with the root @@ -3186,7 +3186,7 @@ static void memcg_kmem_create_cache(struct mem_cgroup *memcg, root_cache->memcg_params->memcg_caches[id] = cachep; } -static void memcg_kmem_destroy_cache(struct kmem_cache *cachep) +static void memcg_destroy_kmem_cache_copy(struct kmem_cache *cachep) { struct kmem_cache *root_cache; struct mem_cgroup *memcg; @@ -3239,7 +3239,7 @@ static inline void memcg_resume_kmem_account(void) current->memcg_kmem_skip_account--; } -int __kmem_cache_destroy_memcg_children(struct kmem_cache *s) +int __kmem_cache_destroy_memcg_copies(struct kmem_cache *s) { struct kmem_cache *c; int i, failed = 0; @@ -3250,7 +3250,7 @@ int __kmem_cache_destroy_memcg_children(struct kmem_cache *s) if (!c) continue; - memcg_kmem_destroy_cache(c); + memcg_destroy_kmem_cache_copy(c); if (cache_from_memcg_idx(s, i)) failed++; @@ -3259,7 +3259,7 @@ int __kmem_cache_destroy_memcg_children(struct kmem_cache *s) return failed; } -static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg) +static void memcg_destroy_kmem_cache_copies(struct mem_cgroup *memcg) { struct kmem_cache *cachep; struct memcg_cache_params *params, *tmp; @@ -3272,25 +3272,26 @@ static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg) cachep = memcg_params_to_cache(params); kmem_cache_shrink(cachep); if (atomic_read(&cachep->memcg_params->nr_pages) == 0) - memcg_kmem_destroy_cache(cachep); + memcg_destroy_kmem_cache_copy(cachep); } mutex_unlock(&memcg_slab_mutex); } -struct create_work { +struct memcg_kmem_cache_copy_work { struct mem_cgroup *memcg; struct kmem_cache *cachep; struct work_struct work; }; -static void memcg_create_cache_work_func(struct work_struct *w) +static void memcg_kmem_cache_copy_work_func(struct work_struct *w) { - struct create_work *cw = container_of(w, struct create_work, work); + struct memcg_kmem_cache_copy_work *cw = container_of(w, + struct memcg_kmem_cache_copy_work, work); struct mem_cgroup *memcg = cw->memcg; struct kmem_cache *cachep = cw->cachep; mutex_lock(&memcg_slab_mutex); - memcg_kmem_create_cache(memcg, cachep); + memcg_copy_kmem_cache(memcg, cachep); mutex_unlock(&memcg_slab_mutex); css_put(&memcg->css); @@ -3300,12 +3301,12 @@ static void memcg_create_cache_work_func(struct work_struct *w) /* * Enqueue the creation of a per-memcg kmem_cache. */ -static void __memcg_create_cache_enqueue(struct mem_cgroup *memcg, - struct kmem_cache *cachep) +static void __memcg_schedule_kmem_cache_copy(struct mem_cgroup *memcg, + struct kmem_cache *cachep) { - struct create_work *cw; + struct memcg_kmem_cache_copy_work *cw; - cw = kmalloc(sizeof(struct create_work), GFP_NOWAIT); + cw = kmalloc(sizeof(*cw), GFP_NOWAIT); if (cw == NULL) { css_put(&memcg->css); return; @@ -3314,17 +3315,17 @@ static void __memcg_create_cache_enqueue(struct mem_cgroup *memcg, cw->memcg = memcg; cw->cachep = cachep; - INIT_WORK(&cw->work, memcg_create_cache_work_func); + INIT_WORK(&cw->work, memcg_kmem_cache_copy_work_func); schedule_work(&cw->work); } -static void memcg_create_cache_enqueue(struct mem_cgroup *memcg, - struct kmem_cache *cachep) +static void memcg_schedule_kmem_cache_copy(struct mem_cgroup *memcg, + struct kmem_cache *cachep) { /* * We need to stop accounting when we kmalloc, because if the * corresponding kmalloc cache is not yet created, the first allocation - * in __memcg_create_cache_enqueue will recurse. + * in __memcg_schedule_kmem_cache_copy will recurse. * * However, it is better to enclose the whole function. Depending on * the debugging options enabled, INIT_WORK(), for instance, can @@ -3333,7 +3334,7 @@ static void memcg_create_cache_enqueue(struct mem_cgroup *memcg, * the safest choice is to do it like this, wrapping the whole function. */ memcg_stop_kmem_account(); - __memcg_create_cache_enqueue(memcg, cachep); + __memcg_schedule_kmem_cache_copy(memcg, cachep); memcg_resume_kmem_account(); } @@ -3404,16 +3405,11 @@ struct kmem_cache *__memcg_kmem_get_cache(struct kmem_cache *cachep, * * However, there are some clashes that can arrive from locking. * For instance, because we acquire the slab_mutex while doing - * kmem_cache_dup, this means no further allocation could happen - * with the slab_mutex held. - * - * Also, because cache creation issue get_online_cpus(), this - * creates a lock chain: memcg_slab_mutex -> cpu_hotplug_mutex, - * that ends up reversed during cpu hotplug. (cpuset allocates - * a bunch of GFP_KERNEL memory during cpuup). Due to all that, + * kmem_cache_request_memcg_copy, this means no further + * allocation could happen with the slab_mutex held. So it's * better to defer everything. */ - memcg_create_cache_enqueue(memcg, cachep); + memcg_schedule_kmem_cache_copy(memcg, cachep); return cachep; out: rcu_read_unlock(); @@ -3537,7 +3533,7 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order) memcg_uncharge_kmem(memcg, PAGE_SIZE << order); } #else -static inline void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg) +static inline void memcg_destroy_kmem_cache_copies(struct mem_cgroup *memcg) { } #endif /* CONFIG_MEMCG_KMEM */ @@ -6415,7 +6411,7 @@ static void mem_cgroup_css_offline(struct cgroup_subsys_state *css) css_for_each_descendant_post(iter, css) mem_cgroup_reparent_charges(mem_cgroup_from_css(iter)); - mem_cgroup_destroy_all_caches(memcg); + memcg_destroy_kmem_cache_copies(memcg); vmpressure_cleanup(&memcg->vmpressure); } diff --git a/mm/slab_common.c b/mm/slab_common.c index 32175617cb75..802466c7e736 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -261,18 +261,18 @@ EXPORT_SYMBOL(kmem_cache_create); #ifdef CONFIG_MEMCG_KMEM /* - * kmem_cache_create_memcg - Create a cache for a memory cgroup. + * kmem_cache_request_memcg_copy - Create copy of a cache for a memcg. + * @cachep: The cache to make a copy of. * @memcg: The memory cgroup the new cache is for. - * @root_cache: The parent of the new cache. * @memcg_name: The name of the memory cgroup (used for naming the new cache). * * This function attempts to create a kmem cache that will serve allocation - * requests going from @memcg to @root_cache. The new cache inherits properties + * requests going from @memcg to @cachep. The new cache inherits properties * from its parent. */ -struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *memcg, - struct kmem_cache *root_cache, - const char *memcg_name) +struct kmem_cache *kmem_cache_request_memcg_copy(struct kmem_cache *cachep, + struct mem_cgroup *memcg, + const char *memcg_name) { struct kmem_cache *s = NULL; char *cache_name; @@ -282,15 +282,14 @@ struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *memcg, mutex_lock(&slab_mutex); - cache_name = kasprintf(GFP_KERNEL, "%s(%d:%s)", root_cache->name, + cache_name = kasprintf(GFP_KERNEL, "%s(%d:%s)", cachep->name, memcg_cache_id(memcg), memcg_name); if (!cache_name) goto out_unlock; - s = do_kmem_cache_create(cache_name, root_cache->object_size, - root_cache->size, root_cache->align, - root_cache->flags, root_cache->ctor, - memcg, root_cache); + s = do_kmem_cache_create(cache_name, cachep->object_size, cachep->size, + cachep->align, cachep->flags, cachep->ctor, + memcg, cachep); if (IS_ERR(s)) { kfree(cache_name); s = NULL; @@ -305,7 +304,7 @@ out_unlock: return s; } -static int kmem_cache_destroy_memcg_children(struct kmem_cache *s) +static int kmem_cache_destroy_memcg_copies(struct kmem_cache *s) { int rc; @@ -314,13 +313,13 @@ static int kmem_cache_destroy_memcg_children(struct kmem_cache *s) return 0; mutex_unlock(&slab_mutex); - rc = __kmem_cache_destroy_memcg_children(s); + rc = __kmem_cache_destroy_memcg_copies(s); mutex_lock(&slab_mutex); return rc; } #else -static int kmem_cache_destroy_memcg_children(struct kmem_cache *s) +static int kmem_cache_destroy_memcg_copies(struct kmem_cache *s) { return 0; } @@ -343,7 +342,7 @@ void kmem_cache_destroy(struct kmem_cache *s) if (s->refcount) goto out_unlock; - if (kmem_cache_destroy_memcg_children(s) != 0) + if (kmem_cache_destroy_memcg_copies(s) != 0) goto out_unlock; list_del(&s->list); -- 1.7.10.4 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 2/2] memcg: cleanup kmem cache creation/destruction functions naming 2014-05-07 8:15 ` [PATCH -mm 2/2] memcg: cleanup kmem cache creation/destruction functions naming Vladimir Davydov @ 2014-05-07 12:45 ` Michal Hocko 2014-05-13 15:05 ` [PATCH -mm v2] " Vladimir Davydov 0 siblings, 1 reply; 10+ messages in thread From: Michal Hocko @ 2014-05-07 12:45 UTC (permalink / raw) To: Vladimir Davydov; +Cc: akpm, hannes, linux-kernel, linux-mm On Wed 07-05-14 12:15:30, Vladimir Davydov wrote: > Current names are rather inconsistent. Let's try to improve them. Yes the old names are a giant mess. I am not sure the new ones are that much better however. > Brief change log: > > ** old name ** ** new name ** > > kmem_cache_create_memcg kmem_cache_request_memcg_copy Both are bad because the first suggests we are creating memcg and the second that we are requesting a copy of memcg. memcg_alloc_kmem_cache? _copy suffix is a bit confusing. E.g. copy_mm and others either to shallow or deep copy depending on the context. This one always creats a deep copy. Also why it is imporatant to treat the created caches as copies? > memcg_kmem_create_cache memcg_copy_kmem_cache memcg_register_kmem_cache? It also allocates so this name is a bit awkward as well. > memcg_kmem_destroy_cache memcg_destroy_kmem_cache_copy memcg_unregister_kmem_cache to match the above? > __kmem_cache_destroy_memcg_children __kmem_cache_destroy_memcg_copies > kmem_cache_destroy_memcg_children kmem_cache_destroy_memcg_copies _children suffix is really confusing because they have different meaning in memcg and refer to children groups. memcg_cleanup_kmem_chache_memcg_params? It doesn't have to live in the kmem_cache namespace because it only does only memcg kmem specific stuff. > mem_cgroup_destroy_all_caches memcg_destroy_kmem_cache_copies > > create_work memcg_kmem_cache_copy_work memcg_register_cache_work? > memcg_create_cache_work_func memcg_kmem_cache_copy_work_func memcg_register_cache_func? > memcg_create_cache_enqueue memcg_schedule_kmem_cache_copy memcg_schedule_register_cache? -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH -mm v2] memcg: cleanup kmem cache creation/destruction functions naming 2014-05-07 12:45 ` Michal Hocko @ 2014-05-13 15:05 ` Vladimir Davydov 2014-05-14 9:40 ` Michal Hocko 0 siblings, 1 reply; 10+ messages in thread From: Vladimir Davydov @ 2014-05-13 15:05 UTC (permalink / raw) To: mhocko; +Cc: akpm, hannes, linux-kernel, linux-mm Current names are rather inconsistent. Let's try to improve them. Brief change log: ** old name ** ** new name ** kmem_cache_create_memcg memcg_create_kmem_cache memcg_kmem_create_cache memcg_regsiter_cache memcg_kmem_destroy_cache memcg_unregister_cache kmem_cache_destroy_memcg_children memcg_cleanup_cache_params mem_cgroup_destroy_all_caches memcg_unregister_all_caches create_work memcg_register_cache_work memcg_create_cache_work_func memcg_register_cache_func memcg_create_cache_enqueue memcg_schedule_register_cache Signed-off-by: Vladimir Davydov <vdavydov@parallels.com> --- include/linux/memcontrol.h | 2 +- include/linux/slab.h | 2 +- mm/memcontrol.c | 60 +++++++++++++++++++++----------------------- mm/slab_common.c | 12 ++++----- 4 files changed, 36 insertions(+), 40 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index aa429de275cc..c3a53cbb88eb 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -514,7 +514,7 @@ __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp); int __memcg_charge_slab(struct kmem_cache *cachep, gfp_t gfp, int order); void __memcg_uncharge_slab(struct kmem_cache *cachep, int order); -int __kmem_cache_destroy_memcg_children(struct kmem_cache *s); +int __memcg_cleanup_cache_params(struct kmem_cache *s); /** * memcg_kmem_newpage_charge: verify if a new kmem allocation is allowed. diff --git a/include/linux/slab.h b/include/linux/slab.h index 86e5b26fbdab..1d9abb7d22a0 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -116,7 +116,7 @@ struct kmem_cache *kmem_cache_create(const char *, size_t, size_t, unsigned long, void (*)(void *)); #ifdef CONFIG_MEMCG_KMEM -struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *, +struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *, struct kmem_cache *, const char *); #endif diff --git a/mm/memcontrol.c b/mm/memcontrol.c index f5ea266f4d9a..b12a05049f2a 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -3127,8 +3127,8 @@ void memcg_free_cache_params(struct kmem_cache *s) kfree(s->memcg_params); } -static void memcg_kmem_create_cache(struct mem_cgroup *memcg, - struct kmem_cache *root_cache) +static void memcg_register_cache(struct mem_cgroup *memcg, + struct kmem_cache *root_cache) { static char memcg_name_buf[NAME_MAX + 1]; /* protected by memcg_slab_mutex */ @@ -3148,7 +3148,7 @@ static void memcg_kmem_create_cache(struct mem_cgroup *memcg, return; cgroup_name(memcg->css.cgroup, memcg_name_buf, NAME_MAX + 1); - cachep = kmem_cache_create_memcg(memcg, root_cache, memcg_name_buf); + cachep = memcg_create_kmem_cache(memcg, root_cache, memcg_name_buf); /* * If we could not create a memcg cache, do not complain, because * that's not critical at all as we can always proceed with the root @@ -3170,7 +3170,7 @@ static void memcg_kmem_create_cache(struct mem_cgroup *memcg, root_cache->memcg_params->memcg_caches[id] = cachep; } -static void memcg_kmem_destroy_cache(struct kmem_cache *cachep) +static void memcg_unregister_cache(struct kmem_cache *cachep) { struct kmem_cache *root_cache; struct mem_cgroup *memcg; @@ -3223,7 +3223,7 @@ static inline void memcg_resume_kmem_account(void) current->memcg_kmem_skip_account--; } -int __kmem_cache_destroy_memcg_children(struct kmem_cache *s) +int __memcg_cleanup_cache_params(struct kmem_cache *s) { struct kmem_cache *c; int i, failed = 0; @@ -3234,7 +3234,7 @@ int __kmem_cache_destroy_memcg_children(struct kmem_cache *s) if (!c) continue; - memcg_kmem_destroy_cache(c); + memcg_unregister_cache(c); if (cache_from_memcg_idx(s, i)) failed++; @@ -3243,7 +3243,7 @@ int __kmem_cache_destroy_memcg_children(struct kmem_cache *s) return failed; } -static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg) +static void memcg_unregister_all_caches(struct mem_cgroup *memcg) { struct kmem_cache *cachep; struct memcg_cache_params *params, *tmp; @@ -3256,25 +3256,26 @@ static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg) cachep = memcg_params_to_cache(params); kmem_cache_shrink(cachep); if (atomic_read(&cachep->memcg_params->nr_pages) == 0) - memcg_kmem_destroy_cache(cachep); + memcg_unregister_cache(cachep); } mutex_unlock(&memcg_slab_mutex); } -struct create_work { +struct memcg_register_cache_work { struct mem_cgroup *memcg; struct kmem_cache *cachep; struct work_struct work; }; -static void memcg_create_cache_work_func(struct work_struct *w) +static void memcg_register_cache_func(struct work_struct *w) { - struct create_work *cw = container_of(w, struct create_work, work); + struct memcg_register_cache_work *cw = + container_of(w, struct memcg_register_cache_work, work); struct mem_cgroup *memcg = cw->memcg; struct kmem_cache *cachep = cw->cachep; mutex_lock(&memcg_slab_mutex); - memcg_kmem_create_cache(memcg, cachep); + memcg_register_cache(memcg, cachep); mutex_unlock(&memcg_slab_mutex); css_put(&memcg->css); @@ -3284,12 +3285,12 @@ static void memcg_create_cache_work_func(struct work_struct *w) /* * Enqueue the creation of a per-memcg kmem_cache. */ -static void __memcg_create_cache_enqueue(struct mem_cgroup *memcg, - struct kmem_cache *cachep) +static void __memcg_schedule_register_cache(struct mem_cgroup *memcg, + struct kmem_cache *cachep) { - struct create_work *cw; + struct memcg_register_cache_work *cw; - cw = kmalloc(sizeof(struct create_work), GFP_NOWAIT); + cw = kmalloc(sizeof(*cw), GFP_NOWAIT); if (cw == NULL) { css_put(&memcg->css); return; @@ -3298,17 +3299,17 @@ static void __memcg_create_cache_enqueue(struct mem_cgroup *memcg, cw->memcg = memcg; cw->cachep = cachep; - INIT_WORK(&cw->work, memcg_create_cache_work_func); + INIT_WORK(&cw->work, memcg_register_cache_func); schedule_work(&cw->work); } -static void memcg_create_cache_enqueue(struct mem_cgroup *memcg, - struct kmem_cache *cachep) +static void memcg_schedule_register_cache(struct mem_cgroup *memcg, + struct kmem_cache *cachep) { /* * We need to stop accounting when we kmalloc, because if the * corresponding kmalloc cache is not yet created, the first allocation - * in __memcg_create_cache_enqueue will recurse. + * in __memcg_schedule_register_cache will recurse. * * However, it is better to enclose the whole function. Depending on * the debugging options enabled, INIT_WORK(), for instance, can @@ -3317,7 +3318,7 @@ static void memcg_create_cache_enqueue(struct mem_cgroup *memcg, * the safest choice is to do it like this, wrapping the whole function. */ memcg_stop_kmem_account(); - __memcg_create_cache_enqueue(memcg, cachep); + __memcg_schedule_register_cache(memcg, cachep); memcg_resume_kmem_account(); } @@ -3388,16 +3389,11 @@ struct kmem_cache *__memcg_kmem_get_cache(struct kmem_cache *cachep, * * However, there are some clashes that can arrive from locking. * For instance, because we acquire the slab_mutex while doing - * kmem_cache_dup, this means no further allocation could happen - * with the slab_mutex held. - * - * Also, because cache creation issue get_online_cpus(), this - * creates a lock chain: memcg_slab_mutex -> cpu_hotplug_mutex, - * that ends up reversed during cpu hotplug. (cpuset allocates - * a bunch of GFP_KERNEL memory during cpuup). Due to all that, - * better to defer everything. + * memcg_create_kmem_cache, this means no further allocation + * could happen with the slab_mutex held. So it's better to + * defer everything. */ - memcg_create_cache_enqueue(memcg, cachep); + memcg_schedule_register_cache(memcg, cachep); return cachep; out: rcu_read_unlock(); @@ -3521,7 +3517,7 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order) memcg_uncharge_kmem(memcg, PAGE_SIZE << order); } #else -static inline void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg) +static inline void memcg_unregister_all_caches(struct mem_cgroup *memcg) { } #endif /* CONFIG_MEMCG_KMEM */ @@ -6399,7 +6395,7 @@ static void mem_cgroup_css_offline(struct cgroup_subsys_state *css) css_for_each_descendant_post(iter, css) mem_cgroup_reparent_charges(mem_cgroup_from_css(iter)); - mem_cgroup_destroy_all_caches(memcg); + memcg_unregister_all_caches(memcg); vmpressure_cleanup(&memcg->vmpressure); } diff --git a/mm/slab_common.c b/mm/slab_common.c index 32175617cb75..48fafb61f35e 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -261,7 +261,7 @@ EXPORT_SYMBOL(kmem_cache_create); #ifdef CONFIG_MEMCG_KMEM /* - * kmem_cache_create_memcg - Create a cache for a memory cgroup. + * memcg_create_kmem_cache - Create a cache for a memory cgroup. * @memcg: The memory cgroup the new cache is for. * @root_cache: The parent of the new cache. * @memcg_name: The name of the memory cgroup (used for naming the new cache). @@ -270,7 +270,7 @@ EXPORT_SYMBOL(kmem_cache_create); * requests going from @memcg to @root_cache. The new cache inherits properties * from its parent. */ -struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *memcg, +struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, struct kmem_cache *root_cache, const char *memcg_name) { @@ -305,7 +305,7 @@ out_unlock: return s; } -static int kmem_cache_destroy_memcg_children(struct kmem_cache *s) +static int memcg_cleanup_cache_params(struct kmem_cache *s) { int rc; @@ -314,13 +314,13 @@ static int kmem_cache_destroy_memcg_children(struct kmem_cache *s) return 0; mutex_unlock(&slab_mutex); - rc = __kmem_cache_destroy_memcg_children(s); + rc = __memcg_cleanup_cache_params(s); mutex_lock(&slab_mutex); return rc; } #else -static int kmem_cache_destroy_memcg_children(struct kmem_cache *s) +static int memcg_cleanup_cache_params(struct kmem_cache *s) { return 0; } @@ -343,7 +343,7 @@ void kmem_cache_destroy(struct kmem_cache *s) if (s->refcount) goto out_unlock; - if (kmem_cache_destroy_memcg_children(s) != 0) + if (memcg_cleanup_cache_params(s) != 0) goto out_unlock; list_del(&s->list); -- 1.7.10.4 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH -mm v2] memcg: cleanup kmem cache creation/destruction functions naming 2014-05-13 15:05 ` [PATCH -mm v2] " Vladimir Davydov @ 2014-05-14 9:40 ` Michal Hocko 0 siblings, 0 replies; 10+ messages in thread From: Michal Hocko @ 2014-05-14 9:40 UTC (permalink / raw) To: Vladimir Davydov; +Cc: akpm, hannes, linux-kernel, linux-mm On Tue 13-05-14 19:05:51, Vladimir Davydov wrote: > Current names are rather inconsistent. Let's try to improve them. > > Brief change log: > > ** old name ** ** new name ** > > kmem_cache_create_memcg memcg_create_kmem_cache > memcg_kmem_create_cache memcg_regsiter_cache > memcg_kmem_destroy_cache memcg_unregister_cache > > kmem_cache_destroy_memcg_children memcg_cleanup_cache_params > mem_cgroup_destroy_all_caches memcg_unregister_all_caches > > create_work memcg_register_cache_work > memcg_create_cache_work_func memcg_register_cache_func > memcg_create_cache_enqueue memcg_schedule_register_cache > > Signed-off-by: Vladimir Davydov <vdavydov@parallels.com> Acked-by: Michal Hocko <mhocko@suse.cz> Thanks! > --- > include/linux/memcontrol.h | 2 +- > include/linux/slab.h | 2 +- > mm/memcontrol.c | 60 +++++++++++++++++++++----------------------- > mm/slab_common.c | 12 ++++----- > 4 files changed, 36 insertions(+), 40 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index aa429de275cc..c3a53cbb88eb 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -514,7 +514,7 @@ __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp); > int __memcg_charge_slab(struct kmem_cache *cachep, gfp_t gfp, int order); > void __memcg_uncharge_slab(struct kmem_cache *cachep, int order); > > -int __kmem_cache_destroy_memcg_children(struct kmem_cache *s); > +int __memcg_cleanup_cache_params(struct kmem_cache *s); > > /** > * memcg_kmem_newpage_charge: verify if a new kmem allocation is allowed. > diff --git a/include/linux/slab.h b/include/linux/slab.h > index 86e5b26fbdab..1d9abb7d22a0 100644 > --- a/include/linux/slab.h > +++ b/include/linux/slab.h > @@ -116,7 +116,7 @@ struct kmem_cache *kmem_cache_create(const char *, size_t, size_t, > unsigned long, > void (*)(void *)); > #ifdef CONFIG_MEMCG_KMEM > -struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *, > +struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *, > struct kmem_cache *, > const char *); > #endif > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index f5ea266f4d9a..b12a05049f2a 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -3127,8 +3127,8 @@ void memcg_free_cache_params(struct kmem_cache *s) > kfree(s->memcg_params); > } > > -static void memcg_kmem_create_cache(struct mem_cgroup *memcg, > - struct kmem_cache *root_cache) > +static void memcg_register_cache(struct mem_cgroup *memcg, > + struct kmem_cache *root_cache) > { > static char memcg_name_buf[NAME_MAX + 1]; /* protected by > memcg_slab_mutex */ > @@ -3148,7 +3148,7 @@ static void memcg_kmem_create_cache(struct mem_cgroup *memcg, > return; > > cgroup_name(memcg->css.cgroup, memcg_name_buf, NAME_MAX + 1); > - cachep = kmem_cache_create_memcg(memcg, root_cache, memcg_name_buf); > + cachep = memcg_create_kmem_cache(memcg, root_cache, memcg_name_buf); > /* > * If we could not create a memcg cache, do not complain, because > * that's not critical at all as we can always proceed with the root > @@ -3170,7 +3170,7 @@ static void memcg_kmem_create_cache(struct mem_cgroup *memcg, > root_cache->memcg_params->memcg_caches[id] = cachep; > } > > -static void memcg_kmem_destroy_cache(struct kmem_cache *cachep) > +static void memcg_unregister_cache(struct kmem_cache *cachep) > { > struct kmem_cache *root_cache; > struct mem_cgroup *memcg; > @@ -3223,7 +3223,7 @@ static inline void memcg_resume_kmem_account(void) > current->memcg_kmem_skip_account--; > } > > -int __kmem_cache_destroy_memcg_children(struct kmem_cache *s) > +int __memcg_cleanup_cache_params(struct kmem_cache *s) > { > struct kmem_cache *c; > int i, failed = 0; > @@ -3234,7 +3234,7 @@ int __kmem_cache_destroy_memcg_children(struct kmem_cache *s) > if (!c) > continue; > > - memcg_kmem_destroy_cache(c); > + memcg_unregister_cache(c); > > if (cache_from_memcg_idx(s, i)) > failed++; > @@ -3243,7 +3243,7 @@ int __kmem_cache_destroy_memcg_children(struct kmem_cache *s) > return failed; > } > > -static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg) > +static void memcg_unregister_all_caches(struct mem_cgroup *memcg) > { > struct kmem_cache *cachep; > struct memcg_cache_params *params, *tmp; > @@ -3256,25 +3256,26 @@ static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg) > cachep = memcg_params_to_cache(params); > kmem_cache_shrink(cachep); > if (atomic_read(&cachep->memcg_params->nr_pages) == 0) > - memcg_kmem_destroy_cache(cachep); > + memcg_unregister_cache(cachep); > } > mutex_unlock(&memcg_slab_mutex); > } > > -struct create_work { > +struct memcg_register_cache_work { > struct mem_cgroup *memcg; > struct kmem_cache *cachep; > struct work_struct work; > }; > > -static void memcg_create_cache_work_func(struct work_struct *w) > +static void memcg_register_cache_func(struct work_struct *w) > { > - struct create_work *cw = container_of(w, struct create_work, work); > + struct memcg_register_cache_work *cw = > + container_of(w, struct memcg_register_cache_work, work); > struct mem_cgroup *memcg = cw->memcg; > struct kmem_cache *cachep = cw->cachep; > > mutex_lock(&memcg_slab_mutex); > - memcg_kmem_create_cache(memcg, cachep); > + memcg_register_cache(memcg, cachep); > mutex_unlock(&memcg_slab_mutex); > > css_put(&memcg->css); > @@ -3284,12 +3285,12 @@ static void memcg_create_cache_work_func(struct work_struct *w) > /* > * Enqueue the creation of a per-memcg kmem_cache. > */ > -static void __memcg_create_cache_enqueue(struct mem_cgroup *memcg, > - struct kmem_cache *cachep) > +static void __memcg_schedule_register_cache(struct mem_cgroup *memcg, > + struct kmem_cache *cachep) > { > - struct create_work *cw; > + struct memcg_register_cache_work *cw; > > - cw = kmalloc(sizeof(struct create_work), GFP_NOWAIT); > + cw = kmalloc(sizeof(*cw), GFP_NOWAIT); > if (cw == NULL) { > css_put(&memcg->css); > return; > @@ -3298,17 +3299,17 @@ static void __memcg_create_cache_enqueue(struct mem_cgroup *memcg, > cw->memcg = memcg; > cw->cachep = cachep; > > - INIT_WORK(&cw->work, memcg_create_cache_work_func); > + INIT_WORK(&cw->work, memcg_register_cache_func); > schedule_work(&cw->work); > } > > -static void memcg_create_cache_enqueue(struct mem_cgroup *memcg, > - struct kmem_cache *cachep) > +static void memcg_schedule_register_cache(struct mem_cgroup *memcg, > + struct kmem_cache *cachep) > { > /* > * We need to stop accounting when we kmalloc, because if the > * corresponding kmalloc cache is not yet created, the first allocation > - * in __memcg_create_cache_enqueue will recurse. > + * in __memcg_schedule_register_cache will recurse. > * > * However, it is better to enclose the whole function. Depending on > * the debugging options enabled, INIT_WORK(), for instance, can > @@ -3317,7 +3318,7 @@ static void memcg_create_cache_enqueue(struct mem_cgroup *memcg, > * the safest choice is to do it like this, wrapping the whole function. > */ > memcg_stop_kmem_account(); > - __memcg_create_cache_enqueue(memcg, cachep); > + __memcg_schedule_register_cache(memcg, cachep); > memcg_resume_kmem_account(); > } > > @@ -3388,16 +3389,11 @@ struct kmem_cache *__memcg_kmem_get_cache(struct kmem_cache *cachep, > * > * However, there are some clashes that can arrive from locking. > * For instance, because we acquire the slab_mutex while doing > - * kmem_cache_dup, this means no further allocation could happen > - * with the slab_mutex held. > - * > - * Also, because cache creation issue get_online_cpus(), this > - * creates a lock chain: memcg_slab_mutex -> cpu_hotplug_mutex, > - * that ends up reversed during cpu hotplug. (cpuset allocates > - * a bunch of GFP_KERNEL memory during cpuup). Due to all that, > - * better to defer everything. > + * memcg_create_kmem_cache, this means no further allocation > + * could happen with the slab_mutex held. So it's better to > + * defer everything. > */ > - memcg_create_cache_enqueue(memcg, cachep); > + memcg_schedule_register_cache(memcg, cachep); > return cachep; > out: > rcu_read_unlock(); > @@ -3521,7 +3517,7 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order) > memcg_uncharge_kmem(memcg, PAGE_SIZE << order); > } > #else > -static inline void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg) > +static inline void memcg_unregister_all_caches(struct mem_cgroup *memcg) > { > } > #endif /* CONFIG_MEMCG_KMEM */ > @@ -6399,7 +6395,7 @@ static void mem_cgroup_css_offline(struct cgroup_subsys_state *css) > css_for_each_descendant_post(iter, css) > mem_cgroup_reparent_charges(mem_cgroup_from_css(iter)); > > - mem_cgroup_destroy_all_caches(memcg); > + memcg_unregister_all_caches(memcg); > vmpressure_cleanup(&memcg->vmpressure); > } > > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 32175617cb75..48fafb61f35e 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -261,7 +261,7 @@ EXPORT_SYMBOL(kmem_cache_create); > > #ifdef CONFIG_MEMCG_KMEM > /* > - * kmem_cache_create_memcg - Create a cache for a memory cgroup. > + * memcg_create_kmem_cache - Create a cache for a memory cgroup. > * @memcg: The memory cgroup the new cache is for. > * @root_cache: The parent of the new cache. > * @memcg_name: The name of the memory cgroup (used for naming the new cache). > @@ -270,7 +270,7 @@ EXPORT_SYMBOL(kmem_cache_create); > * requests going from @memcg to @root_cache. The new cache inherits properties > * from its parent. > */ > -struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *memcg, > +struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, > struct kmem_cache *root_cache, > const char *memcg_name) > { > @@ -305,7 +305,7 @@ out_unlock: > return s; > } > > -static int kmem_cache_destroy_memcg_children(struct kmem_cache *s) > +static int memcg_cleanup_cache_params(struct kmem_cache *s) > { > int rc; > > @@ -314,13 +314,13 @@ static int kmem_cache_destroy_memcg_children(struct kmem_cache *s) > return 0; > > mutex_unlock(&slab_mutex); > - rc = __kmem_cache_destroy_memcg_children(s); > + rc = __memcg_cleanup_cache_params(s); > mutex_lock(&slab_mutex); > > return rc; > } > #else > -static int kmem_cache_destroy_memcg_children(struct kmem_cache *s) > +static int memcg_cleanup_cache_params(struct kmem_cache *s) > { > return 0; > } > @@ -343,7 +343,7 @@ void kmem_cache_destroy(struct kmem_cache *s) > if (s->refcount) > goto out_unlock; > > - if (kmem_cache_destroy_memcg_children(s) != 0) > + if (memcg_cleanup_cache_params(s) != 0) > goto out_unlock; > > list_del(&s->list); > -- > 1.7.10.4 > -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 1/2] memcg: get rid of memcg_create_cache_name 2014-05-07 8:15 [PATCH -mm 1/2] memcg: get rid of memcg_create_cache_name Vladimir Davydov 2014-05-07 8:15 ` [PATCH -mm 2/2] memcg: cleanup kmem cache creation/destruction functions naming Vladimir Davydov @ 2014-05-07 9:51 ` Michal Hocko 2014-05-07 10:45 ` Vladimir Davydov 1 sibling, 1 reply; 10+ messages in thread From: Michal Hocko @ 2014-05-07 9:51 UTC (permalink / raw) To: Vladimir Davydov; +Cc: akpm, hannes, linux-kernel, linux-mm On Wed 07-05-14 12:15:29, Vladimir Davydov wrote: > Instead of calling back to memcontrol.c from kmem_cache_create_memcg in > order to just create the name of a per memcg cache, let's allocate it in > place. We only need to pass the memcg name to kmem_cache_create_memcg > for that - everything else can be done in slab_common.c. > > Signed-off-by: Vladimir Davydov <vdavydov@parallels.com> Seems good to me. I would keep the comment about the static buffer as mentioned below. Other than that Acked-by: Michal Hocko <mhocko@suse.cz> [...] > -char *memcg_create_cache_name(struct mem_cgroup *memcg, > - struct kmem_cache *root_cache) > -{ > - static char *buf; > - > - /* > - * We need a mutex here to protect the shared buffer. Since this is > - * expected to be called only on cache creation, we can employ the > - * slab_mutex for that purpose. > - */ > - lockdep_assert_held(&slab_mutex); > - > - if (!buf) { > - buf = kmalloc(NAME_MAX + 1, GFP_KERNEL); > - if (!buf) > - return NULL; > - } > - > - cgroup_name(memcg->css.cgroup, buf, NAME_MAX + 1); > - return kasprintf(GFP_KERNEL, "%s(%d:%s)", root_cache->name, > - memcg_cache_id(memcg), buf); > -} > - > int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s, > struct kmem_cache *root_cache) > { > @@ -3164,6 +3141,7 @@ void memcg_free_cache_params(struct kmem_cache *s) > static void memcg_kmem_create_cache(struct mem_cgroup *memcg, > struct kmem_cache *root_cache) > { > + static char *memcg_name_buf; > struct kmem_cache *cachep; > int id; So we are relying on memcg_slab_mutex now, right? Worth a comment I suppose. -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 1/2] memcg: get rid of memcg_create_cache_name 2014-05-07 9:51 ` [PATCH -mm 1/2] memcg: get rid of memcg_create_cache_name Michal Hocko @ 2014-05-07 10:45 ` Vladimir Davydov 2014-05-07 11:19 ` Michal Hocko 2014-05-07 20:53 ` Andrew Morton 0 siblings, 2 replies; 10+ messages in thread From: Vladimir Davydov @ 2014-05-07 10:45 UTC (permalink / raw) To: Michal Hocko; +Cc: akpm, hannes, linux-kernel, linux-mm On Wed, May 07, 2014 at 11:51:27AM +0200, Michal Hocko wrote: > On Wed 07-05-14 12:15:29, Vladimir Davydov wrote: > [...] > > static void memcg_kmem_create_cache(struct mem_cgroup *memcg, > > struct kmem_cache *root_cache) > > { > > + static char *memcg_name_buf; > > struct kmem_cache *cachep; > > int id; > > So we are relying on memcg_slab_mutex now, right? Worth a comment I > suppose. Sure. The updated version is attached below. Thanks. -- From: Vladimir Davydov <vdavydov@parallels.com> Subject: [PATCH] memcg: get rid of memcg_create_cache_name Instead of calling back to memcontrol.c from kmem_cache_create_memcg in order to just create the name of a per memcg cache, let's allocate it in place. We only need to pass the memcg name to kmem_cache_create_memcg for that - everything else can be done in slab_common.c. Signed-off-by: Vladimir Davydov <vdavydov@parallels.com> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 6c59056f4bc6..7b639ab48aa8 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -501,8 +501,6 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order); int memcg_cache_id(struct mem_cgroup *memcg); -char *memcg_create_cache_name(struct mem_cgroup *memcg, - struct kmem_cache *root_cache); int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s, struct kmem_cache *root_cache); void memcg_free_cache_params(struct kmem_cache *s); diff --git a/include/linux/slab.h b/include/linux/slab.h index ecbec9ccb80d..86e5b26fbdab 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -117,7 +117,8 @@ struct kmem_cache *kmem_cache_create(const char *, size_t, size_t, void (*)(void *)); #ifdef CONFIG_MEMCG_KMEM struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *, - struct kmem_cache *); + struct kmem_cache *, + const char *); #endif void kmem_cache_destroy(struct kmem_cache *); int kmem_cache_shrink(struct kmem_cache *); diff --git a/mm/memcontrol.c b/mm/memcontrol.c index f381239ab402..9ff3742f4154 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -3101,29 +3101,6 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups) return 0; } -char *memcg_create_cache_name(struct mem_cgroup *memcg, - struct kmem_cache *root_cache) -{ - static char *buf; - - /* - * We need a mutex here to protect the shared buffer. Since this is - * expected to be called only on cache creation, we can employ the - * slab_mutex for that purpose. - */ - lockdep_assert_held(&slab_mutex); - - if (!buf) { - buf = kmalloc(NAME_MAX + 1, GFP_KERNEL); - if (!buf) - return NULL; - } - - cgroup_name(memcg->css.cgroup, buf, NAME_MAX + 1); - return kasprintf(GFP_KERNEL, "%s(%d:%s)", root_cache->name, - memcg_cache_id(memcg), buf); -} - int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s, struct kmem_cache *root_cache) { @@ -3164,6 +3141,7 @@ void memcg_free_cache_params(struct kmem_cache *s) static void memcg_kmem_create_cache(struct mem_cgroup *memcg, struct kmem_cache *root_cache) { + static char *memcg_name_buf; /* protected by memcg_slab_mutex */ struct kmem_cache *cachep; int id; @@ -3179,7 +3157,14 @@ static void memcg_kmem_create_cache(struct mem_cgroup *memcg, if (cache_from_memcg_idx(root_cache, id)) return; - cachep = kmem_cache_create_memcg(memcg, root_cache); + if (!memcg_name_buf) { + memcg_name_buf = kmalloc(NAME_MAX + 1, GFP_KERNEL); + if (!memcg_name_buf) + return; + } + + cgroup_name(memcg->css.cgroup, memcg_name_buf, NAME_MAX + 1); + cachep = kmem_cache_create_memcg(memcg, root_cache, memcg_name_buf); /* * If we could not create a memcg cache, do not complain, because * that's not critical at all as we can always proceed with the root diff --git a/mm/slab_common.c b/mm/slab_common.c index 7e348cff814d..32175617cb75 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -264,13 +264,15 @@ EXPORT_SYMBOL(kmem_cache_create); * kmem_cache_create_memcg - Create a cache for a memory cgroup. * @memcg: The memory cgroup the new cache is for. * @root_cache: The parent of the new cache. + * @memcg_name: The name of the memory cgroup (used for naming the new cache). * * This function attempts to create a kmem cache that will serve allocation * requests going from @memcg to @root_cache. The new cache inherits properties * from its parent. */ struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *memcg, - struct kmem_cache *root_cache) + struct kmem_cache *root_cache, + const char *memcg_name) { struct kmem_cache *s = NULL; char *cache_name; @@ -280,7 +282,8 @@ struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *memcg, mutex_lock(&slab_mutex); - cache_name = memcg_create_cache_name(memcg, root_cache); + cache_name = kasprintf(GFP_KERNEL, "%s(%d:%s)", root_cache->name, + memcg_cache_id(memcg), memcg_name); if (!cache_name) goto out_unlock; -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 1/2] memcg: get rid of memcg_create_cache_name 2014-05-07 10:45 ` Vladimir Davydov @ 2014-05-07 11:19 ` Michal Hocko 2014-05-07 20:53 ` Andrew Morton 1 sibling, 0 replies; 10+ messages in thread From: Michal Hocko @ 2014-05-07 11:19 UTC (permalink / raw) To: Vladimir Davydov; +Cc: akpm, hannes, linux-kernel, linux-mm On Wed 07-05-14 14:45:16, Vladimir Davydov wrote: [...] > From: Vladimir Davydov <vdavydov@parallels.com> > Subject: [PATCH] memcg: get rid of memcg_create_cache_name > > Instead of calling back to memcontrol.c from kmem_cache_create_memcg in > order to just create the name of a per memcg cache, let's allocate it in > place. We only need to pass the memcg name to kmem_cache_create_memcg > for that - everything else can be done in slab_common.c. > > Signed-off-by: Vladimir Davydov <vdavydov@parallels.com> Acked-by: Michal Hocko <mhocko@suse.cz> > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 6c59056f4bc6..7b639ab48aa8 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -501,8 +501,6 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order); > > int memcg_cache_id(struct mem_cgroup *memcg); > > -char *memcg_create_cache_name(struct mem_cgroup *memcg, > - struct kmem_cache *root_cache); > int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s, > struct kmem_cache *root_cache); > void memcg_free_cache_params(struct kmem_cache *s); > diff --git a/include/linux/slab.h b/include/linux/slab.h > index ecbec9ccb80d..86e5b26fbdab 100644 > --- a/include/linux/slab.h > +++ b/include/linux/slab.h > @@ -117,7 +117,8 @@ struct kmem_cache *kmem_cache_create(const char *, size_t, size_t, > void (*)(void *)); > #ifdef CONFIG_MEMCG_KMEM > struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *, > - struct kmem_cache *); > + struct kmem_cache *, > + const char *); > #endif > void kmem_cache_destroy(struct kmem_cache *); > int kmem_cache_shrink(struct kmem_cache *); > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index f381239ab402..9ff3742f4154 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -3101,29 +3101,6 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups) > return 0; > } > > -char *memcg_create_cache_name(struct mem_cgroup *memcg, > - struct kmem_cache *root_cache) > -{ > - static char *buf; > - > - /* > - * We need a mutex here to protect the shared buffer. Since this is > - * expected to be called only on cache creation, we can employ the > - * slab_mutex for that purpose. > - */ > - lockdep_assert_held(&slab_mutex); > - > - if (!buf) { > - buf = kmalloc(NAME_MAX + 1, GFP_KERNEL); > - if (!buf) > - return NULL; > - } > - > - cgroup_name(memcg->css.cgroup, buf, NAME_MAX + 1); > - return kasprintf(GFP_KERNEL, "%s(%d:%s)", root_cache->name, > - memcg_cache_id(memcg), buf); > -} > - > int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s, > struct kmem_cache *root_cache) > { > @@ -3164,6 +3141,7 @@ void memcg_free_cache_params(struct kmem_cache *s) > static void memcg_kmem_create_cache(struct mem_cgroup *memcg, > struct kmem_cache *root_cache) > { > + static char *memcg_name_buf; /* protected by memcg_slab_mutex */ > struct kmem_cache *cachep; > int id; > > @@ -3179,7 +3157,14 @@ static void memcg_kmem_create_cache(struct mem_cgroup *memcg, > if (cache_from_memcg_idx(root_cache, id)) > return; > > - cachep = kmem_cache_create_memcg(memcg, root_cache); > + if (!memcg_name_buf) { > + memcg_name_buf = kmalloc(NAME_MAX + 1, GFP_KERNEL); > + if (!memcg_name_buf) > + return; > + } > + > + cgroup_name(memcg->css.cgroup, memcg_name_buf, NAME_MAX + 1); > + cachep = kmem_cache_create_memcg(memcg, root_cache, memcg_name_buf); > /* > * If we could not create a memcg cache, do not complain, because > * that's not critical at all as we can always proceed with the root > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 7e348cff814d..32175617cb75 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -264,13 +264,15 @@ EXPORT_SYMBOL(kmem_cache_create); > * kmem_cache_create_memcg - Create a cache for a memory cgroup. > * @memcg: The memory cgroup the new cache is for. > * @root_cache: The parent of the new cache. > + * @memcg_name: The name of the memory cgroup (used for naming the new cache). > * > * This function attempts to create a kmem cache that will serve allocation > * requests going from @memcg to @root_cache. The new cache inherits properties > * from its parent. > */ > struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *memcg, > - struct kmem_cache *root_cache) > + struct kmem_cache *root_cache, > + const char *memcg_name) > { > struct kmem_cache *s = NULL; > char *cache_name; > @@ -280,7 +282,8 @@ struct kmem_cache *kmem_cache_create_memcg(struct mem_cgroup *memcg, > > mutex_lock(&slab_mutex); > > - cache_name = memcg_create_cache_name(memcg, root_cache); > + cache_name = kasprintf(GFP_KERNEL, "%s(%d:%s)", root_cache->name, > + memcg_cache_id(memcg), memcg_name); > if (!cache_name) > goto out_unlock; > -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 1/2] memcg: get rid of memcg_create_cache_name 2014-05-07 10:45 ` Vladimir Davydov 2014-05-07 11:19 ` Michal Hocko @ 2014-05-07 20:53 ` Andrew Morton 2014-05-08 7:08 ` Vladimir Davydov 1 sibling, 1 reply; 10+ messages in thread From: Andrew Morton @ 2014-05-07 20:53 UTC (permalink / raw) To: Vladimir Davydov; +Cc: Michal Hocko, hannes, linux-kernel, linux-mm On Wed, 7 May 2014 14:45:16 +0400 Vladimir Davydov <vdavydov@parallels.com> wrote: > @@ -3164,6 +3141,7 @@ void memcg_free_cache_params(struct kmem_cache *s) > static void memcg_kmem_create_cache(struct mem_cgroup *memcg, > struct kmem_cache *root_cache) > { > + static char *memcg_name_buf; /* protected by memcg_slab_mutex */ > struct kmem_cache *cachep; > int id; > > @@ -3179,7 +3157,14 @@ static void memcg_kmem_create_cache(struct mem_cgroup *memcg, > if (cache_from_memcg_idx(root_cache, id)) > return; > > - cachep = kmem_cache_create_memcg(memcg, root_cache); > + if (!memcg_name_buf) { > + memcg_name_buf = kmalloc(NAME_MAX + 1, GFP_KERNEL); > + if (!memcg_name_buf) > + return; > + } Does this have any meaningful advantage over the simpler static char memcg_name_buf[NAME_MAX + 1]; ? I guess it saves a scrap of memory if the machine never uses memcg's. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 1/2] memcg: get rid of memcg_create_cache_name 2014-05-07 20:53 ` Andrew Morton @ 2014-05-08 7:08 ` Vladimir Davydov 0 siblings, 0 replies; 10+ messages in thread From: Vladimir Davydov @ 2014-05-08 7:08 UTC (permalink / raw) To: Andrew Morton; +Cc: Michal Hocko, hannes, linux-kernel, linux-mm On Wed, May 07, 2014 at 01:53:52PM -0700, Andrew Morton wrote: > On Wed, 7 May 2014 14:45:16 +0400 Vladimir Davydov <vdavydov@parallels.com> wrote: > > > @@ -3164,6 +3141,7 @@ void memcg_free_cache_params(struct kmem_cache *s) > > static void memcg_kmem_create_cache(struct mem_cgroup *memcg, > > struct kmem_cache *root_cache) > > { > > + static char *memcg_name_buf; /* protected by memcg_slab_mutex */ > > struct kmem_cache *cachep; > > int id; > > > > @@ -3179,7 +3157,14 @@ static void memcg_kmem_create_cache(struct mem_cgroup *memcg, > > if (cache_from_memcg_idx(root_cache, id)) > > return; > > > > - cachep = kmem_cache_create_memcg(memcg, root_cache); > > + if (!memcg_name_buf) { > > + memcg_name_buf = kmalloc(NAME_MAX + 1, GFP_KERNEL); > > + if (!memcg_name_buf) > > + return; > > + } > > Does this have any meaningful advantage over the simpler > > static char memcg_name_buf[NAME_MAX + 1]; > > ? Don't think so. In case nobody has objections, the patch is attached below. Thanks. -- From: Vladimir Davydov <vdavydov@parallels.com> Subject: [PATCH] memcg: memcg_kmem_create_cache: make memcg_name_buf statically allocated It isn't worth complicating the code by allocating it on the first access, because it only takes 256 bytes. Signed-off-by: Vladimir Davydov <vdavydov@parallels.com> diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 9ff3742f4154..01fda17a2566 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -3141,7 +3141,8 @@ void memcg_free_cache_params(struct kmem_cache *s) static void memcg_kmem_create_cache(struct mem_cgroup *memcg, struct kmem_cache *root_cache) { - static char *memcg_name_buf; /* protected by memcg_slab_mutex */ + static char memcg_name_buf[NAME_MAX + 1]; /* protected by + memcg_slab_mutex */ struct kmem_cache *cachep; int id; @@ -3157,12 +3158,6 @@ static void memcg_kmem_create_cache(struct mem_cgroup *memcg, if (cache_from_memcg_idx(root_cache, id)) return; - if (!memcg_name_buf) { - memcg_name_buf = kmalloc(NAME_MAX + 1, GFP_KERNEL); - if (!memcg_name_buf) - return; - } - cgroup_name(memcg->css.cgroup, memcg_name_buf, NAME_MAX + 1); cachep = kmem_cache_create_memcg(memcg, root_cache, memcg_name_buf); /* -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-05-14 9:40 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-05-07 8:15 [PATCH -mm 1/2] memcg: get rid of memcg_create_cache_name Vladimir Davydov 2014-05-07 8:15 ` [PATCH -mm 2/2] memcg: cleanup kmem cache creation/destruction functions naming Vladimir Davydov 2014-05-07 12:45 ` Michal Hocko 2014-05-13 15:05 ` [PATCH -mm v2] " Vladimir Davydov 2014-05-14 9:40 ` Michal Hocko 2014-05-07 9:51 ` [PATCH -mm 1/2] memcg: get rid of memcg_create_cache_name Michal Hocko 2014-05-07 10:45 ` Vladimir Davydov 2014-05-07 11:19 ` Michal Hocko 2014-05-07 20:53 ` Andrew Morton 2014-05-08 7:08 ` Vladimir Davydov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).