Subject: + slab-clean-up-kmem_cache_create_memcg-error-handling.patch added to -mm tree To: vdavydov@xxxxxxxxxxxxx,cl@xxxxxxxxx,glommer@xxxxxxxxx,hannes@xxxxxxxxxxx,mhocko@xxxxxxx,penberg@xxxxxxxxxx From: akpm@xxxxxxxxxxxxxxxxxxxx Date: Mon, 06 Jan 2014 14:56:16 -0800 The patch titled Subject: slab: clean up kmem_cache_create_memcg() error handling has been added to the -mm tree. Its filename is slab-clean-up-kmem_cache_create_memcg-error-handling.patch This patch should soon appear at http://ozlabs.org/~akpm/mmots/broken-out/slab-clean-up-kmem_cache_create_memcg-error-handling.patch and later at http://ozlabs.org/~akpm/mmotm/broken-out/slab-clean-up-kmem_cache_create_memcg-error-handling.patch Before you just go and hit "reply", please: a) Consider who else should be cc'ed b) Prefer to cc a suitable mailing list as well c) Ideally: find the original patch on the mailing list and do a reply-to-all to that, adding suitable additional cc's *** Remember to use Documentation/SubmitChecklist when testing your code *** The -mm tree is included into linux-next and is updated there every 3-4 working days ------------------------------------------------------ From: Vladimir Davydov <vdavydov@xxxxxxxxxxxxx> Subject: slab: clean up kmem_cache_create_memcg() error handling Currently kmem_cache_create_memcg() backoffs on failure inside conditionals, without using gotos. This results in the rollback code duplication, which makes the function look cumbersome even though on error we should only free the allocated cache. Since in the next patch I am going to add yet another rollback function call on error path there, let's employ labels instead of conditionals for undoing any changes on failure to keep things clean. Signed-off-by: Vladimir Davydov <vdavydov@xxxxxxxxxxxxx> Reviewed-by: Pekka Enberg <penberg@xxxxxxxxxx> Cc: Michal Hocko <mhocko@xxxxxxx> Cc: Glauber Costa <glommer@xxxxxxxxx> Cc: Johannes Weiner <hannes@xxxxxxxxxxx> Cc: Christoph Lameter <cl@xxxxxxxxx> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> --- mm/slab_common.c | 69 +++++++++++++++++++++------------------------ 1 file changed, 33 insertions(+), 36 deletions(-) diff -puN mm/slab_common.c~slab-clean-up-kmem_cache_create_memcg-error-handling mm/slab_common.c --- a/mm/slab_common.c~slab-clean-up-kmem_cache_create_memcg-error-handling +++ a/mm/slab_common.c @@ -171,13 +171,14 @@ kmem_cache_create_memcg(struct mem_cgrou struct kmem_cache *parent_cache) { struct kmem_cache *s = NULL; - int err = 0; + int err; get_online_cpus(); mutex_lock(&slab_mutex); - if (!kmem_cache_sanity_check(memcg, name, size) == 0) - goto out_locked; + err = kmem_cache_sanity_check(memcg, name, size); + if (err) + goto out_unlock; /* * Some allocators will constraint the set of valid flags to a subset @@ -189,45 +190,38 @@ kmem_cache_create_memcg(struct mem_cgrou s = __kmem_cache_alias(memcg, name, size, align, flags, ctor); if (s) - goto out_locked; + goto out_unlock; + err = -ENOMEM; s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL); - if (s) { - s->object_size = s->size = size; - s->align = calculate_alignment(flags, align, size); - s->ctor = ctor; - - if (memcg_register_cache(memcg, s, parent_cache)) { - kmem_cache_free(kmem_cache, s); - err = -ENOMEM; - goto out_locked; - } - - s->name = kstrdup(name, GFP_KERNEL); - if (!s->name) { - kmem_cache_free(kmem_cache, s); - err = -ENOMEM; - goto out_locked; - } + if (!s) + goto out_unlock; - err = __kmem_cache_create(s, flags); - if (!err) { - s->refcount = 1; - list_add(&s->list, &slab_caches); - memcg_cache_list_add(memcg, s); - } else { - kfree(s->name); - kmem_cache_free(kmem_cache, s); - } - } else - err = -ENOMEM; + s->object_size = s->size = size; + s->align = calculate_alignment(flags, align, size); + s->ctor = ctor; + + s->name = kstrdup(name, GFP_KERNEL); + if (!s->name) + goto out_free_cache; + + err = memcg_register_cache(memcg, s, parent_cache); + if (err) + goto out_free_cache; + + err = __kmem_cache_create(s, flags); + if (err) + goto out_free_cache; + + s->refcount = 1; + list_add(&s->list, &slab_caches); + memcg_cache_list_add(memcg, s); -out_locked: +out_unlock: mutex_unlock(&slab_mutex); put_online_cpus(); if (err) { - if (flags & SLAB_PANIC) panic("kmem_cache_create: Failed to create slab '%s'. Error %d\n", name, err); @@ -236,11 +230,14 @@ out_locked: name, err); dump_stack(); } - return NULL; } - return s; + +out_free_cache: + kfree(s->name); + kmem_cache_free(kmem_cache, s); + goto out_unlock; } struct kmem_cache * _ Patches currently in -mm which might be from vdavydov@xxxxxxxxxxxxx are origin.patch memcg-fix-kmem_account_flags-check-in-memcg_can_account_kmem.patch memcg-make-memcg_update_cache_sizes-static.patch memcg-do-not-use-vmalloc-for-mem_cgroup-allocations.patch slab-clean-up-kmem_cache_create_memcg-error-handling.patch memcg-slab-kmem_cache_create_memcg-fix-memleak-on-fail-path.patch memcg-slab-clean-up-memcg-cache-initialization-destruction.patch memcg-slab-fix-barrier-usage-when-accessing-memcg_caches.patch memcg-fix-possible-null-deref-while-traversing-memcg_slab_caches-list.patch memcg-slab-fix-races-in-per-memcg-cache-creation-destruction.patch memcg-get-rid-of-kmem_cache_dup.patch slab-do-not-panic-if-we-fail-to-create-memcg-cache.patch memcg-slab-rcu-protect-memcg_params-for-root-caches.patch memcg-remove-kmem_accounted_activated-flag.patch memcg-rework-memcg_update_kmem_limit-synchronization.patch linux-next.patch -- To unsubscribe from this list: send the line "unsubscribe mm-commits" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html