On Mon 21-01-13 11:58:49, Glauber Costa wrote: > On 01/18/2013 08:06 PM, Michal Hocko wrote: > >> + /* bounce at first found */ > >> > + for_each_mem_cgroup_tree(iter, memcg) { > > This will not work. Consider you will see a !online memcg. What happens? > > mem_cgroup_iter will css_get group that it returns and css_put it when > > it visits another one or finishes the loop. So your poor iter will be > > released before it gets born. Not good. > > > Reading this again, I don't really follow. The iterator is not supposed > to put() anything it hasn't get()'d before, so we will never release the > group. Note that if it ever appears in here, the css refcnt is expected > to be at least 1 already. > > The online test relies on the memcg refcnt, not on the css refcnt. Bahh, yeah, sorry about the confusion. Damn, it's not the first time I managed to mix those two... > Actually, now that the value setting is all done in css_online, the css > refcnt should be enough to denote if the cgroup already has children, > without a memcg-specific test. The css refcnt is bumped somewhere > between alloc and online. Yes, in init_cgroup_css. > Unless Tejun objects it, I think I will just get rid of the online > test, and rely on the fact that if the iterator sees any children, we > should already online. Which means that we are back to list_empty(&cgrp->children) test, aren't we. We just call it a different name. If you really insist on not using children directly then do something like: struct cgroup *pos; if (!memcg->use_hierarchy) cgroup_for_each_child(pos, memcg->css.cgroup) return true; return false; This still has an issue that a change (e.g. vm_swappiness) that requires this check will fail even though the child creation fails after it is made visible (e.g. during css_online). -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>