Re: [PATCH 3/8] cgroup: use cgroup_lock_live_group(parent) incgroup_create()

[Michal Hocko]

On Thu 01-11-12 16:05:32, Michal Hocko wrote:
> On Thu 01-11-12 07:52:24, Tejun Heo wrote:
> > Hey, Michal.
> > 
> > On Thu, Nov 1, 2012 at 2:16 AM, Michal Hocko <mhocko@xxxxxxx> wrote:
> > > I am not sure I understand. What does deactivate_super has to do with
> > > the above suggestion? cgroup_lock_live_group will take the cgroup_mutex
> > > on the success or frees the previously allocated&unused memory. The
> > > only thing I was suggesting is to do cgroup_lock_live_group first and
> > > allocate the cgroup only if it doesn't fail.
> > 
> > It complicates updates to the error exit path.
> 
> Still don't get it, sorry. What prevents us to do:
> static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
>                              umode_t mode)
> {
>         struct cgroup *cgrp;
>         struct cgroupfs_root *root = parent->root;
>         int err = 0;
>         struct cgroup_subsys *ss;
>         struct super_block *sb = root->sb;
> 
> 	if (!cgroup_lock_live_group(parent))
> 		return -ENODEV;
> 
>         cgrp = kzalloc(sizeof(*cgrp), GFP_KERNEL);
>         if (!cgrp)
>                 return -ENOMEM;

this needs to drop the lock of course but it doesn't make it much more
complicated...
 
> > You end up having to update a lot more and it's not like grabbing lock
> > first is substantially better in any way, so why bother?
> 
> Yes the allocation can sleep if we are short on memory so this can
> potentially take long which is not entirely nice but a pointless
> allocation is not nice either. Anyway I am asking because I am trying to
> understand what is the motivation behind and your explanation about the
> error exit path doesn't make much sense to me. So I am either missing
> something or we are talking about two different things.

-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/