Re: [PATCH 3/8] cgroup: use cgroup_lock_live_group(parent) in cgroup_create()
From: Michal Hocko <hidden>
Date: 2012-10-31 15:55:20
Also in:
lkml
On Tue 30-10-12 21:22:40, Tejun Heo wrote:
This patch makes cgroup_create() fail if @parent is marked removed. This is to prepare for further updates to cgroup_rmdir() path. Note that this change isn't strictly necessary. cgroup can only be created via mkdir and the removed marking and dentry removal happen without releasing cgroup_mutex, so cgroup_create() can never race with cgroup_rmdir(). Even after the scheduled updates to cgroup_rmdir(), cgroup_mkdir() and cgroup_rmdir() are synchronized by i_mutex rendering the added liveliness check unnecessary. Do it anyway such that locking is contained inside cgroup proper and we don't get nasty surprises if we ever grow another caller of cgroup_create(). Signed-off-by: Tejun Heo <redacted>
Looks good. Just a nit bellow Reviewed-by: Michal Hocko <redacted>
quoted hunk ↗ jump to hunk
--- kernel/cgroup.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)diff --git a/kernel/cgroup.c b/kernel/cgroup.c index a49cdbc..b3010ae 100644 --- a/kernel/cgroup.c +++ b/kernel/cgroup.c@@ -3906,6 +3906,18 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry, if (!cgrp) return -ENOMEM; + /* + * Only live parents can have children. Note that the liveliness + * check isn't strictly necessary because cgroup_mkdir() and + * cgroup_rmdir() are fully synchronized by i_mutex; however, do it + * anyway so that locking is contained inside cgroup proper and we + * don't get nasty surprises if we ever grow another caller. + */ + if (!cgroup_lock_live_group(parent)) { + err = -ENODEV; + goto err_free; + } +
I think this should be moved up before we try to allocate any memory. Or is your motivation to keep cgroup_lock held for shorter time? I could agree with that but a small comment would be helpful. -- Michal Hocko SUSE Labs