On Thu 19-12-13 12:51:38, Vladimir Davydov wrote: > 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. I disagree. It is much easier to review code for mem leaks when you have explicit cleanup gotos. But this is a matter of taste I guess. -- 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>