Messages in this thread |  | | | Date | Mon, 2 May 2011 06:46:35 -0700 | | From | "Paul E. McKenney" <> | | Subject | Re: query: [PATCH 2/2] cgroup: Remove call to synchronize_rcu in cgroup_attach_task |
| |
On Fri, Apr 29, 2011 at 02:34:47PM +0200, Mike Galbraith wrote: > (ok crickets, keep the noise down please) > > On Thu, 2011-04-28 at 11:38 +0200, Mike Galbraith wrote: > > > The explosions are because the logic to snag rmdir() should anyone grab > > a reference will let us zip right through and free a cgroup while > > there's a destruction in flight. Adding a cgrp->count check before > > trying to cgroup_clear_css_refs() prevents the explosions, but that > > leaves RCU grace periods still embedded in userspace. > > > > So... > > > > I bent up put_css_set() a bit to try to destroy synchronously on final > > put if possible, so rmdir() will only be snagged if that fails. The > > thing seems to be working, so I'll show it. Readers (beware) may notice > > some gratuitous looking chicken scratches. Just ignore those, and move > > along smartly to the suggesting a much better way part, for surely one > > must exist. > > Hi Self, (*howdy*) > > You might try the below. No weird gyrations to accomplish the same > thing, and I see no slub debug gripes, no list debug gripes, nada. > > Makes one wonder what these things do for a living.
If you are adding something to an RCU-protected data structure, then you do not need synchronize_rcu(). But if you are removing something from an RCU-protected data structure, then you really do need them. If you leave them out, you can see the following type of failure:
1. CPU 0, running in an RCU read-side critical section, obtains a pointer to data item A.
2. CPU 1 removes data item A from the structure.
3. CPU 1 does not do a synchronize_rcu(). If CPU 1 had done a synchronize_rcu(), then it would have waited until CPU 0 had left its RCU read-side critical section, and thus until CPU 0 stopped using its pointer to data item A. But there was no synchronize_rcu(), so CPU 0 is still looking at data item A.
4. CPU 1 frees data item A.
This is very bad. CPU 0 has a pointer into the freelist. Worse yet, some other CPU might allocate memory and get a pointer to data item A. That CPU and CPU 0 would then have an interesting but short lived disagreement about that memory's type. Crash goes the kernel.
So please do not remove synchronize_rcu() calls unless you can prove that it is safe to do so!
Thanx, Paul
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c > index 25c7eb5..b8c64bf 100644 > --- a/kernel/cgroup.c > +++ b/kernel/cgroup.c > @@ -826,13 +826,6 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode) > struct cgroup *cgrp = dentry->d_fsdata; > struct cgroup_subsys *ss; > 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 */ > - synchronize_rcu(); > > mutex_lock(&cgroup_mutex); > /* > @@ -1822,7 +1815,6 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk) > ss->attach(ss, cgrp, oldcgrp, tsk, false); > } > set_bit(CGRP_RELEASABLE, &oldcgrp->flags); > - synchronize_rcu(); > put_css_set(cg); > > /* > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
|  |