On 12/19/2013 12:44 PM, Michal Hocko wrote:
> 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.
AFAIK it's common practice not to split kfree's to be called under
different labels on fail paths, because kfree(NULL) results in a no-op.
Since on undo, we only call kfree, I introduce the only label. Of course
I could do something like
s->name=...
if (!s->name)
goto out_free_name;
err = __kmem_new_cache(...)
if (err)
goto out_free_name;
<...>
out_free_name:
kfree(s->name);
out_free_cache:
kfree(s);
goto out_unlock;
But I think using only out_free_cache makes the code look clearer.
>
>> 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.
OK, thanks.
--
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>