Re: [PATCH 2/3] mm/memcg: fix endless iteration in reclaim

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu 16-01-14 09:17:38, Michal Hocko wrote:
> On Wed 15-01-14 13:24:34, Hugh Dickins wrote:
> > On Wed, 15 Jan 2014, Michal Hocko wrote:
[...]
> > > From 560924e86059947ab9418732cb329ad149dd8f6a Mon Sep 17 00:00:00 2001
> > > From: Michal Hocko <mhocko@xxxxxxx>
> > > Date: Wed, 15 Jan 2014 11:52:09 +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.
> > > 
> > > 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.
> > > 
> > > Cc: stable@xxxxxxxxxxxxxxx # 3.10+
> > > Reported-and-debugged-by: Hugh Dickins <hughd@xxxxxxxxxx>
> > 
> > Definitely not debugged by me!  Debugged and understood by you.
> 
> You still have debugged the second part of the problem (endless loop).
> But I will go with whatever tag you like.
> 
> > > Signed-off-by: Michal Hocko <mhocko@xxxxxxx>
> > > ---
> > >  mm/memcontrol.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index f016d26adfd3..dd3974c9f08d 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -1078,7 +1078,8 @@ skip_node:
> > >  	 * protected by css_get and the tree walk is rcu safe.
> > >  	 */
> > >  	if (next_css) {
> > > -		if ((next_css->flags & CSS_ONLINE) && css_tryget(next_css))
> > > +		if ((next_css->flags & CSS_ONLINE) &&
> > > +				(next_css == root || css_tryget(next_css)))
> > 
> > Not quite: next_css points to one thing and root to another.
> 
> Dohh, right you are next_css == root->css. I was wondering how I was
> able to see the leak being fixed and then realized that root->css has
> the same address as root...
> Anyway very well spotted.
> 
> > >  			return mem_cgroup_from_css(next_css);
> > >  		else {
> > >  			prev_css = next_css;
> > > -- 
> > 
> > This is how I've re-written that block, and started testing on it;
> > the unnecessary "else {" part was looking increasingly ugly to me
> > (though let loose on it, I might change it all around more...)
> > 
> > 	if (next_css) {
> > 		if ((next_css->flags & CSS_ONLINE) &&
> > 		    (next_css == &root->css || css_tryget(next_css)))
> > 			return mem_cgroup_from_css(next_css);
> > 		prev_css = next_css;
> > 		goto skip_node;
> > 	}
> 
> Yes, that looks better. Maybe put a blank line before prev_css = next_css?
> 
> > Sorry for being so slow to respond, by the way: for a couple of hours
> > I couldn't test at all, and thought I was going mad - one day I send
> > you that "cg" script, the next day it starts to break, it couldn't
> > "mkdir -p /cg/cg", claiming it already existed, wha???  Turns out the
> > fix for that has gone into yesterday's mmotm (though I've not had time
> > to move on to that yet): uninitialized ret in memcg_propagate_kmem().
> 
> Thanks a lot!

What about this? I have incorporated your root->css fix and else removal
+ added a comment to clarify importance of root css refcount exclusion.
I still prefer this going to the stable separately from CSS_ONLINE fix.
---
>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.

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]
Cc: stable@xxxxxxxxxxxxxxx # 3.10+
<Hugh's-selection>-by: Hugh Dickins <hughd@xxxxxxxxxx>
Signed-off-by: Michal Hocko <mhocko@xxxxxxx>
---
 mm/memcontrol.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f016d26adfd3..969f14d32b30 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1076,14 +1076,22 @@ skip_node:
 	 * 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.
+	 *
+	 * We do not take a reference on the root of the tree walk
+	 * because we might race with the root removal when it would
+	 * be the only node in the iterated hierarchy and mem_cgroup_iter
+	 * would end up in an endless loop because it expects that at
+	 * least one valid node will be returned. Root cannot disappear
+	 * because caller of the iterator should hold it already so
+	 * skipping css reference should be safe.
 	 */
 	if (next_css) {
-		if ((next_css->flags & CSS_ONLINE) && css_tryget(next_css))
+		if ((next_css->flags & CSS_ONLINE) &&
+				(next_css == root->css || css_tryget(next_css)))
 			return mem_cgroup_from_css(next_css);
-		else {
-			prev_css = next_css;
-			goto skip_node;
-		}
+
+		prev_css = next_css;
+		goto skip_node;
 	}
 
 	return NULL;
-- 
1.8.5.2

-- 
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>




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]