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:

quoted

Hello, Michal.

On Tue, Dec 04, 2012 at 04:14:20PM +0100, Michal Hocko wrote:

quoted

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.

quoted

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