Re: query: [PATCH 2/2] cgroup: Remove call to synchronize_rcu incgroup_attach_task

[Paul E. McKenney]

On Mon, May 02, 2011 at 05:04:00PM +0200, Mike Galbraith wrote:
> On Mon, 2011-05-02 at 16:29 +0200, Mike Galbraith wrote:
> > On Mon, 2011-05-02 at 06:46 -0700, Paul E. McKenney wrote:
> > > On Fri, Apr 29, 2011 at 02:34:47PM +0200, Mike Galbraith wrote:
> > 
> > > > 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!
> > 
> > In these instances are a little different.
> > 
> > We have..
> >     start teardown
> >     synchronize_rcu()
> >     finish teardown
> >     call_rcu(kfree_it) 
> > ..so removal wouldn't trigger the standard "let's rummage around in
> > freed memory" kind of excitement.
> > 
> > But yeah, removing them without proof is out.
> > 
> > My box was telling me that they _are_ safe to remove, by not exploding
> > with list/slub debug enabled while I beat the snot out of it.. which is
> > evidence, but not proof :)
> 
> P.S. the explosions I was looking into were caused by that finish
> teardown being in flight via schedule_work() when android removed
> synchronize_rcu() _and synchronization on in-flight teardown_.  I became
> curious wrt the need for synchronize_rcu() at all when I fixed these
> explosions by ensuring that teardown was _not_ in flight before
> shredding the cgroup via rmdir, by doing synchronous teardown if
> possible, and only synchronizing if it wasn't possible.  Only removing
> synchronize_rcu() does essentially the same, since teardown is then done
> synchronously under the big mutex.  Freeing is still done via rcu.
> 
> So it wasn't "these things make userspace sleepy, let's remove them".

OK, but you did have me going for a bit there!  ;-)

							Thanx, Paul
--
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/