cgroup: initialize cgrp->dentry before css_alloc()
PATCH 1/6 cgroup: initialize cgrp->dentry before css_alloc()
With this change, it's guaranteed that cgroup_path() won't see NULL cgrp->dentry, and thus we can remove the NULL check in it.
(Well, it's not true, because dummptop.dentry is always NULL)
--- kernel/cgroup.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
kernel/cgroup.c +4 -3
@@ -1764,7 +1764,7 @@ int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen)rcu_lockdep_assert (rcu_read_lock_held () ||cgroup_lock_is_held (),"cgroup_path() called without proper locking" ); -if (!dentry || cgrp == dummytop) { +if (cgrp == dummytop) {/* * Inactive subsystems have no dentry for their root * cgroup
@@ -4150,6 +4150,9 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,init_cgroup_housekeeping (cgrp); + dentry->d_fsdata = cgrp; + cgrp->dentry = dentry; + cgrp->parent = parent; cgrp->root = parent->root; cgrp->top_cgroup = parent->top_cgroup;
@@ -4187,8 +4190,6 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,lockdep_assert_held (&dentry->d_inode->i_mutex);/* allocation complete, commit to creation */ - dentry->d_fsdata = cgrp; - cgrp->dentry = dentry;list_add_tail (&cgrp->allcg_node, &root->allcg_list);list_add_tail_rcu (&cgrp->sibling, &cgrp->parent->children); root->number_of_cgroups++;
Signature · 1 line
1.8.0.2
PATCH 2/6 sched: split out tg creation/destruction to css_online/css_offline
With this change, it's safe to access tg->css.cgroup while iterating the global task_group list.
This is a preparation for later patches.
--- include/linux/sched.h | 3 +++ kernel/sched/auto_group.c | 3 +++ kernel/sched/core.c | 49 +++++++++++++++++++++++++++++++++++++---------- 3 files changed, 45 insertions(+), 10 deletions(-)
include/linux/sched.h +3 -0
@@ -2750,7 +2750,10 @@ extern void normalize_rt_tasks(void);extern struct task_group root_task_group;extern struct task_group *sched_create_group (struct task_group *parent); +extern void sched_online_group (struct task_group *tg, +struct task_group *parent);extern void sched_destroy_group (struct task_group *tg); +extern void sched_offline_group (struct task_group *tg);extern void sched_move_task (struct task_struct *tsk);#ifdef CONFIG_FAIR_GROUP_SCHED extern int sched_group_set_shares (struct task_group *tg,unsigned long shares);
kernel/sched/auto_group.c +3 -0
@@ -35,6 +35,7 @@ static inline void autogroup_destroy(struct kref *kref) ag->tg->rt_se =NULL ; ag->tg->rt_rq =NULL ;#endif +sched_offline_group (ag->tg);sched_destroy_group (ag->tg); }
@@ -76,6 +77,8 @@ static inline struct autogroup *autogroup_create(void)if (IS_ERR (tg)) goto out_free; +sched_online_group (tg, &root_task_group); +kref_init (&ag->kref);init_rwsem (&ag->lock); ag->id =atomic_inc_return (&autogroup_seq_nr);
kernel/sched/core.c +39 -10
97 lines not on this page · raw message
Signature · 1 line
1.8.0.2
PATCH 3/6 sched: remove redundant NULL cgroup check in task_group_path()
In reply to Li Zefancpu_cgroup_css_online() is called after cgroup is fully created, and it makes this check redundant.
--- kernel/sched/debug.c | 7 ------- 1 file changed, 7 deletions(-)
kernel/sched/debug.c +0 -7
@@ -110,13 +110,6 @@ static char *task_group_path(struct task_group *tg)if (autogroup_path (tg, group_path,PATH_MAX ))return group_path; -/* -* May be NULL if the underlying cgroup isn't fully-created yet -*/ -if (!tg->css.cgroup) { - group_path[0 ] ='\0' ; -return group_path; - }cgroup_path (tg->css.cgroup, group_path,PATH_MAX );return group_path; }
Signature · 1 line
1.8.0.2
PATCH 4/6 cgroup: remove duplicate RCU free on struct cgroup
In reply to Li ZefanWhen destroying cgroup, though in cgroup_diput() we've called synchronize_rcu(), we then still have to free cgroup via call_rcu().
The story is, long ago to fix a race between reading /proc/sched_debug and freeing cgroup, the code was changed to utilize call_rcu().
Now we've splitted part of cpu cgroup css_alloc/free to css_online/offline, which makes the race vanish, and this grants us to remove call_rcu().
--- kernel/cgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/cgroup.c +1 -1
@@ -892,7 +892,7 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode)simple_xattrs_free (&cgrp->xattrs);ida_simple_remove (&cgrp->root->cgroup_ida, cgrp->id); -kfree_rcu (cgrp, rcu_head); +kfree (cgrp); }else {struct cfent *cfe =__d_cfe (dentry);struct cgroup *cgrp = dentry->d_parent->d_fsdata;
Signature · 1 line
1.8.0.2
PATCH 5/6 cgroup: remove synchronize_rcu() from cgroup_diput()
In reply to Li ZefanFree cgroup via call_rcu(). The actual work is done through workqueue.
--- include/linux/cgroup.h | 1 + kernel/cgroup.c | 73 ++++++++++++++++++++++++++++++-------------------- 2 files changed, 45 insertions(+), 29 deletions(-)
include/linux/cgroup.h +1 -0
@@ -203,6 +203,7 @@ struct cgroup {/* For RCU-protected deletion */ struct rcu_head rcu_head; +struct work_struct free_work;/* List of events which userspace want to receive */ struct list_head event_list;
kernel/cgroup.c +44 -29
96 lines not on this page · raw message
Signature · 1 line
1.8.0.2
PATCH 6/6 cgroup: remove bogus comments in cgroup_diput()
In reply to Li ZefanSince commit 48ddbe194623ae089cc0576e60363f2d2e85662a ("cgroup: make css->refcnt clearing on cgroup removal optional"), each css holds a ref on cgroup's dentry, so cgroup_diput() won't be called until all css' refs go down to 0, which invalids the comments.
--- kernel/cgroup.c | 6 ------ 1 file changed, 6 deletions(-)
kernel/cgroup.c +0 -6
@@ -899,12 +899,6 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode)struct cgroup *cgrp = dentry->d_fsdata;BUG_ON (!(cgroup_is_removed (cgrp))); -/* It's possible for external users to be holding css -* reference counts on a cgroup; css_put() needs to -* be able to access the cgroup after decrementing -* the reference count in order to know if it needs to -* queue the cgroup to be handled by the release -* agent */ call_rcu (&cgrp->rcu_head, cgroup_free_rcu); }else {struct cfent *cfe =__d_cfe (dentry);
Signature · 1 line
1.8.0.2
Tejun Heo
maintainer(cc'ing Andrew as scheduler folks are difficult to get response from
these days and I can't think of anyone else to bother :)
Hello, Li.
The cgroup part looks good to me but it would be great if the descriptions are more detailed, especially, about why the change is beneficial or what it's aiming at. I take it that the shed changes are necessary to facilitate the later cgroup changes? Can you please elaborate how?
The scheduler part of changes are mostly mechanical, so it would be great if we can get ack from scheduler people and route these together.
Thanks.
Signature · 1 line
tejun
Quoted 5 lines · On Tue, 22 Jan 2013 16:27:54 -0800 Tejun Heo…
(cc'ing Andrew as scheduler folks are difficult to get response from
these days and I can't think of anyone else to bother :)Hello, Li.
Quoted 9 lines
The cgroup part looks good to me but it would be great if the descriptions are more detailed, especially, about why the change is beneficial or what it's aiming at. I take it that the shed changes are necessary to facilitate the later cgroup changes? Can you please elaborate how?
The scheduler part of changes are mostly mechanical, so it would be great if we can get ack from scheduler people and route these together.
The sched parts do look pretty innocuous and could be viewed as "cgroup stuff which happens to reside in kernel/sched/".
If you like the code, I think the best you can do is to merge it up, make sure the sched guys are fully cc'ed then send it in to Linus.
Quoted 11 lines · Tejun Heo
(cc'ing Andrew as scheduler folks are difficult to get response from
these days and I can't think of anyone else to bother :)Hello, Li.
The cgroup part looks good to me but it would be great if the descriptions are more detailed, especially, about why the change is beneficial or what it's aiming at. I take it that the shed changes are necessary to facilitate the later cgroup changes? Can you please elaborate how?
Scheduler changes made in patch #2 are preparation for #3 and #4.
- for #3:
After ss->css_alloc(), there's a small window that tg->css.cgroup is NULL. With the change, tg won't be seen before ss->css_online(), so the scheduler won't see NULL tg->css.cgroup.
- for #4:
If tg is unregistered and removed from global list in css_free(), and if we kfree cgroup right after ss->css_free(), the scheduler can access invalid tg->css.cgroup, because tg is also protected by RCU.
Without patch #2:
cgroup_rmdir()
no ss->css_offline()diput()
syncronize_rcu()
ss->css_free() <-- remove tg from global list,
and free tg via call_rcu()
kfree_rcu(cgroup) <-- wait rcu read section
With the change:
cgroup_rmdir()
ss->css_offline() <-- remove tg from global listdiput()
synchronize_rcu() <-- wait rcu read section
ss->css_free(); <-- free tg
kfree(cgroup);
Quoted 5 lines
The scheduler part of changes are mostly mechanical, so it would be great if we can get ack from scheduler people and route these together.
Thanks.
Tejun Heo
maintainerHello, Li.
Quoted 8 lines · Li Zefan
Scheduler changes made in patch #2 are preparation for #3 and #4.
- for #3:
After
ss->css_alloc(), there's a small window thattg->css.cgroupis NULL. With the change, tg won't be seen beforess->css_online(), so the scheduler won't see NULLtg->css.cgroup.
Quoted 5 lines
- for #4:
If tg is unregistered and removed from global list in
css_free(), and if we kfree cgroup right afterss->css_free(), the scheduler can access invalidtg->css.cgroup, because tg is also protected by RCU.
Can you please incorporate the above into the patch descriptions?
Thanks!
Signature · 1 line
tejun