On Thu 19-12-13 10:31:43, Vladimir Davydov wrote:
> On 12/18/2013 08:56 PM, Michal Hocko wrote:
> > On Wed 18-12-13 17:16:52, Vladimir Davydov wrote:
> >> Signed-off-by: Vladimir Davydov <vdavydov@xxxxxxxxxxxxx>
> >> Cc: Michal Hocko <mhocko@xxxxxxx>
> >> Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
> >> Cc: Glauber Costa <glommer@xxxxxxxxx>
> >> Cc: Christoph Lameter <cl@xxxxxxxxx>
> >> Cc: Pekka Enberg <penberg@xxxxxxxxxx>
> >> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Dunno, is this really better to be worth the code churn?
> >
> > It even makes the generated code tiny bit bigger:
> > text data bss dec hex filename
> > 4355 171 236 4762 129a mm/slab_common.o.after
> > 4342 171 236 4749 128d mm/slab_common.o.before
> >
> > Or does it make the further changes much more easier? Be explicit in the
> > patch description if so.
>
> Hi, Michal
>
> IMO, undoing under labels looks better than inside conditionals, because
> we don't have to repeat the same deinitialization code then, like this
> (note three calls to kmem_cache_free()):
Agreed but the resulting code is far from doing nice undo on different
conditions. You have out_free_cache which frees everything regardless
whether name or cache registration failed. So it doesn't help with
readability much IMO.
>
> 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;
> }
>
> 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;
>
> The next patch, which fixes the memcg_params leakage on error, would
> make it even worse introducing two calls to memcg_free_cache_params()
> after kstrdup and __kmem_cache_create.
>
> If you think it isn't worthwhile applying this patch, just let me know,
> I don't mind dropping it.
As I've said if it helps with the later patches then I do not mind but
on its own it doesn't sound like a huge improvement.
Btw. you do not have to set err = -ENOMEM before goto out_locked. Just
set before kmem_cache_zalloc. You also do not need to initialize it to 0
because kmem_cache_sanity_check will set it.
> Anyway, I'll improve the comment and resend.
Thanks!
> Thanks.
>
> >
> >> ---
> >> mm/slab_common.c | 66 +++++++++++++++++++++++++++---------------------------
> >> 1 file changed, 33 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/mm/slab_common.c b/mm/slab_common.c
> >> index 0b7bb39..5d6f743 100644
> >> --- a/mm/slab_common.c
> >> +++ b/mm/slab_common.c
> >> @@ -176,8 +176,9 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
> >> 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,41 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
> >>
> >> s = __kmem_cache_alias(memcg, name, size, align, flags, ctor);
> >> if (s)
> >> - goto out_locked;
> >> + goto out_unlock;
> >>
> >> 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;
> >> - }
> >> + if (!s) {
> >> + err = -ENOMEM;
> >> + goto out_unlock;
> >> + }
> >>
> >> - s->name = kstrdup(name, GFP_KERNEL);
> >> - if (!s->name) {
> >> - kmem_cache_free(kmem_cache, s);
> >> - err = -ENOMEM;
> >> - goto out_locked;
> >> - }
> >> + s->object_size = s->size = size;
> >> + s->align = calculate_alignment(flags, align, size);
> >> + s->ctor = ctor;
> >>
> >> - 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
> >> + s->name = kstrdup(name, GFP_KERNEL);
> >> + if (!s->name) {
> >> err = -ENOMEM;
> >> + goto out_free_cache;
> >> + }
> >> +
> >> + err = memcg_register_cache(memcg, s, parent_cache);
> >> + if (err)
> >> + goto out_free_cache;
> >>
> >> -out_locked:
> >> + 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_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 +233,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 *
> >> --
> >> 1.7.10.4
> >>
>
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>