On Tue 04-12-12 11:58:48, Glauber Costa wrote:
> On 12/03/2012 09:15 PM, Michal Hocko wrote:
> > On Fri 30-11-12 17:31:26, Glauber Costa wrote:
> > [...]
> >> +/*
> >> + * must be called with memcg_lock held, unless the cgroup is guaranteed to be
> >> + * already dead (like in mem_cgroup_force_empty, for instance).
> >> + */
> >> +static inline bool memcg_has_children(struct mem_cgroup *memcg)
> >> +{
> >> + return mem_cgroup_count_children(memcg) != 1;
> >> +}
> >
> > Why not just keep list_empty(&cgrp->children) which is much simpler much
> > more effective and correct here as well because cgroup cannot vanish
> > while we are at the call because all callers come from cgroup fs?
> >
> Because it depends on cgroup's internal representation, which I think
> we're better off not depending upon, even if this is not as serious a
> case as the locking stuff. But also, technically, cgrp->children is
> protected by the cgroup_lock(), while since we'll hold the memcg_lock
> during creation and also around the iterators, we cover everything with
> the same lock.
The list is RCU safe so we do not have to use cgroup_lock there for this
kind of test.
> That said, of course we don't need to do the full iteration here, and
> mem_cgroup_count_children is indeed overkill. We could just as easily
> verify if any child exist - it is just an emptiness test after all. But
> it is not living in any fast path, though, and I just assumed code reuse
> to win over efficiency in this particular case -
> mem_cgroup_count_children already existed...
Yes but the function name suggests a more generic usage and the test is
really an overkill. Maybe we can get a cgroup generic helper
cgroup_as_children which would do the thing without exhibiting cgroup
internals. What do you think?
--
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>