On Sun 09-12-12 09:01:48, Ying Han wrote:
> On Mon, Nov 26, 2012 at 10:47 AM, Michal Hocko <mho...@suse.cz> wrote:
> > Current implementation of mem_cgroup_iter has to consider both css and
> > memcg to find out whether no group has been found (css==NULL - aka the
> > loop is completed) and that no memcg is associated with the found node
> > (!memcg - aka css_tryget failed because the group is no longer alive).
> > This leads to awkward tweaks like tests for css && !memcg to skip the
> > current node.
> >
> > It will be much easier if we got rid off css variable altogether and
> > only rely on memcg. In order to do that the iteration part has to skip
> > dead nodes. This sounds natural to me and as a nice side effect we will
> > get a simple invariant that memcg is always alive when non-NULL and all
> > nodes have been visited otherwise.
> >
> > We could get rid of the surrounding while loop but keep it in for now to
> > make review easier. It will go away in the following patch.
> >
> > Signed-off-by: Michal Hocko <mho...@suse.cz>
> > ---
> > mm/memcontrol.c | 56
> > +++++++++++++++++++++++++++----------------------------
> > 1 file changed, 27 insertions(+), 29 deletions(-)
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 6bcc97b..d1bc0e8 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -1086,7 +1086,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup
> > *root,
> > rcu_read_lock();
^^^^^^^^
here
> > while (!memcg) {
> > struct mem_cgroup_reclaim_iter *uninitialized_var(iter);
> > - struct cgroup_subsys_state *css = NULL;
> >
> > if (reclaim) {
> > int nid = zone_to_nid(reclaim->zone);
[...]
> > +skip_node:
> > + next_cgroup = cgroup_next_descendant_pre(
> > + prev_cgroup, root->css.cgroup);
> >
> > - /*
> > - * Even if we found a group we have to make sure it is
> > alive.
> > - * css && !memcg means that the groups should be skipped and
> > - * we should continue the tree walk.
> > - * last_visited css is safe to use because it is protected
> > by
> > - * css_get and the tree walk is rcu safe.
> > - */
> > - if (css == &root->css || (css && css_tryget(css)))
> > - memcg = mem_cgroup_from_css(css);
> > + /*
> > + * Even if we found a group we have to make sure it
> > is
> > + * alive. css && !memcg means that the groups
> > should be
> > + * skipped and we should continue the tree walk.
> > + * last_visited css is safe to use because it is
> > + * protected by css_get and the tree walk is rcu
> > safe.
> > + */
> > + if (next_cgroup) {
> > + struct mem_cgroup *mem =
> > mem_cgroup_from_cont(
> > + next_cgroup);
> > + if (css_tryget(&mem->css))
> > + memcg = mem;
> > + else {
> > + prev_cgroup = next_cgroup;
>
> I might be missing something here, but the comment says the
> last_visited is safe to use but not the next_cgroup. What is
> preventing it to be
> removed ?
rcu_read_lock. cgroup cannot disappear inside rcu.
>
> --Ying
[...]
--
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
