Re: [PATCH 4/4] memcg: replace cgroup_lock with memcg specific memcg_lock

[Tejun Heo <tj@kernel.org>]

Hello, Michal.

On Wed, Dec 05, 2012 at 03:35:37PM +0100, Michal Hocko wrote:
> On Tue 04-12-12 07:22:25, Tejun Heo wrote:
> > Hello, Michal.
> > 
> > On Tue, Dec 04, 2012 at 04:14:20PM +0100, Michal Hocko wrote:
> > > OK, I read this as "generic helper doesn't make much sense". Then I
> > > would just ask. Does cgroup core really care whether we do
> > > list_empty test? Is this something that we have to care about in memcg
> > > and should fix? If yes then just try to do it as simple as possible.
> > 
> > The thing is, what does the test mean when it doesn't have proper
> > synchronization?  list_empty(&cgroup->children) doesn't really have a
> > precise meaning if you're not synchronized. 
> 
> For the cases memcg use this test it is perfectly valid because the only
> important information is whether there is a child group. We do not care
> about its current state. The test is rather strict because we could set
> use_hierarchy to 1 even if there is child which is not online yet (after
> the value is copied in css_online of course). But do we care about this
> race? If yes, patches with the use case are welcome.

Please just implement properly synchronized onlineness.  There is
absoluately *NO* reason not to do it.  It's gonna be error/race-prone
like hell if memcg keeps trying to dance around it.

> > There could be cases where such correct-most-of-the-time results are
> > okay but depending on stuff like that is a sure-fire way to subtle
> > bugs.
> > 
> > So, my recommendation would be to bite the bullet and implement
> > properly synchronized on/offline state and teach the memcg iterator
> > about it so that memcg can definitively tell what's online and what's
> > not while holding memcg_mutex, and use such knowledge consistently.
> 
> I would rather not complicate the iterators with even more logic but if
> it turns out being useful then why not.

It's gonna be as simple as the following.  I doubt it's gonna add much
to the complexity.

	if (!memcg_online(pos))
		continue; // or goto next; whatever

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>