On Thu 16-01-14 11:15:36, Hugh Dickins wrote:
> On Thu, 16 Jan 2014, Michal Hocko wrote:
> > From 543df5c82f6eec622f669ea322ba6ff03924fded Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@xxxxxxx>
> > Date: Thu, 16 Jan 2014 16:17:13 +0100
> > Subject: [PATCH] memcg: fix css reference leak from mem_cgroup_iter
> >
> > 19f39402864e (memcg: simplify mem_cgroup_iter) has introduced a css
> > refrence leak (thus memory leak) because mem_cgroup_iter makes sure it
> > doesn't put a css reference on the root of the tree walk. The mentioned
> > commit however dropped the root check when the css reference is taken
> > while it keept the css_put optimization fora the root in place.
>
> I don't think that's quite right, actually - and I think it's all
> so confusing that we do need to be pedantic and set it down right.
You are right!
> I spent quite a while yesterday trying out my "cg m" on 3.10, 3.11,
I have done the same now (with a different test - simple mem_eater
with hard_limit really low to trigger reclaim and trace_printk in both
mem_cgroup_css_{alloc,free}) and you are right that 3.10 and 3.11 were
OK regarding the leak. Which is a relief...
3.12 resp. mmotm which I was testing on previously has the leak though.
So there must have been some other escape part which didn't allow
css_tryget on the root.
> 3.12 and 3.13-rc8 on this laptop: first just counting mem_cgroup_allocs
> and frees (if I could get that far without hanging or crashing), then
> also with your patch in (on 3.12 and 3.13-rc8) or the completely
> different patch appended at the bottom (on 3.10 and 3.11), checking
> for leftover mem_cgroups afterwards.
>
> I saw no evidence of mem_cgroup leakage on 3.10 and 3.11, which had
> /*
> * Root is not visited by cgroup iterators so it needs an
> * explicit visit.
> */
> if (!last_visited)
> return root;
> at the head of __mem_cgroup_iter_next(), removed around the same
> time as changeover from prev_cgroup etc to prev_css etc in 3.12.
Ohh, now I get it. Cgroup iterators originally didn't visit the root and
all the callers had to special case it. Then Tejun changed them to visit
root as well by bd8815a6d802 (cgroup: make css_for_each_descendant() and
friends include the origin css in the iteration) which was a good change
but I didn't realize it would be a problem when I reviewed it. Now it
makes sense again.
> I don't believe 19f39402864e was responsible for a reference leak,
> that came later. But I think it was responsible for the original
> endless iteration (shrink_zone going around and around getting root
> again and again from mem_cgroup_iter).
So your hang is not within mem_cgroup_iter but you are getting root all
the time without any way out?
[3.10 code base]
shrink_zone
[rmdir root]
mem_cgroup_iter(root, NULL, reclaim)
// prev = NULL
rcu_read_lock()
last_visited = iter->last_visited // gets root || NULL
css_tryget(last_visited) // failed
last_visited = NULL [1]
memcg = root = __mem_cgroup_iter_next(root, NULL)
iter->last_visited = root;
reclaim->generation = iter->generation
mem_cgroup_iter(root, root, reclaim)
// prev = root
rcu_read_lock
last_visited = iter->last_visited // gets root
css_tryget(last_visited) // failed
[1]
So we indeed can loop here without any progress. I just fail
to see how my patch could help. We even do not get down to
cgroup_next_descendant_pre.
Or am I missing something?
The following should fix this kind of endless loop:
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 194721839cf5..168e5abcca92 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1221,7 +1221,8 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
smp_rmb();
last_visited = iter->last_visited;
if (last_visited &&
- !css_tryget(&last_visited->css))
+ last_visited != root &&
+ !css_tryget(&last_visited->css))
last_visited = NULL;
}
}
@@ -1229,7 +1230,7 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
memcg = __mem_cgroup_iter_next(root, last_visited);
if (reclaim) {
- if (last_visited)
+ if (last_visited && last_visited != root)
css_put(&last_visited->css);
iter->last_visited = memcg;
Not that I like it much :/
> But beware of my conclusion, please check for yourself: with my
> separate kbuilds in separate /cg/cg/? memcgs, what "cg m" is doing
> is very simple and segregated, can hardly be called testing reclaim
> iteration, so I hope you have something better to check it. Plus
> I was testing on 3.10 and 3.11 vanilla, not latest stable versions.
>
> (If I'm very honest, I'll admit that I still did not see that hang
> on 3.11 vanilla:
But I assume you can still reproduce it with 3.10, right?
I am sorry but I didn't get to run your script yet.
> what I hit was a crash in kfree instead, but the
> same patch got rid of that too.
Care to post an oops?
> Of course I ought to investigate
> further, but at some point I just have to give up and move on,
> there's just too much breakage to chase all over the kernel...)
>
> > This means that css_put is not called and so css along with mem_cgroup
> > and other cgroup internal object tied by css lifetime are never freed.
> >
> > Fix the issue by reintroducing root check in __mem_cgroup_iter_next.
> >
> > This patch also fixes issue reported by Hugh Dickins when
> > mem_cgroup_iter might end up in an endless loop because a group which is
> > under hard limit reclaim is removed in parallel with iteration.
> > __mem_cgroup_iter_next would always return NULL because css_tryget on
> > the root (reclaimed memcg) would fail and there are no other memcg in
> > the hierarchy. prev == NULL in mem_cgroup_iter would prevent break out
> > from the root and so the while (!memcg) loop would never terminate.
> > as css_tryget is no longer called for the root of the tree walk this
> > doesn't happen anymore.
> >
> > [hughd@xxxxxxxxxx: Fixed root vs. root->css fix]
> > [hughd@xxxxxxxxxx: Get rid of else branch because it is ugly]
>
> Thanks for your courtesy! But let's not clutter it with those two.
>
> > <Hugh's-selection>-by: Hugh Dickins <hughd@xxxxxxxxxx>
>
> You already credited me above, but "Reported-by:" here if you insist.
>
> > Cc: stable@xxxxxxxxxxxxxxx # 3.10+
>
> Well, I'm okay with that, if we use that as a way to shoehorn in the
> patch at the bottom instead for 3.10 and 3.11 stables.
So far I do not see how it would make a change for those two kernels as
they have the special handling for root.
[...]
> "Equivalent" patch for 3.10 or 3.11: fixing similar hangs but no leakage.
>
> Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx>
>
> --- v3.10/mm/memcontrol.c 2013-06-30 15:13:29.000000000 -0700
> +++ linux/mm/memcontrol.c 2014-01-15 18:18:24.476566659 -0800
> @@ -1226,7 +1226,8 @@ struct mem_cgroup *mem_cgroup_iter(struc
> }
> }
>
> - memcg = __mem_cgroup_iter_next(root, last_visited);
> + if (!prev || last_visited)
> + memcg = __mem_cgroup_iter_next(root, last_visited);
I am confused. What would change between those two calls to change the
outcome? The function doesn't have any internal state.
>
> if (reclaim) {
> if (last_visited)
--
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>