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

[Michal Hocko]

On Mon 13-01-14 17:52:30, Hugh Dickins wrote:
> On one home machine I can easily reproduce (by rmdir of memcgdir during
> reclaim) multiple processes stuck looping forever in mem_cgroup_iter():
> __mem_cgroup_iter_next() keeps selecting the memcg being destroyed, fails
> to tryget it, returns NULL to mem_cgroup_iter(), which goes around again.

So you had a single memcg (without any children) and a limit-reclaim
on it when you removed it, right? This is nasty because
__mem_cgroup_iter_next will try to skip it but there is nothing else so
it returns NULL. We update iter->generation++ but that doesn't help us
as prev = NULL as this is the first iteration so
		if (prev && reclaim->generation != iter->generation)

break out will not help us. You patch will surely help I am just not
sure it is the right thing to do. Let me think about this.

Anyway very well spotted!

> It's better to err on the side of leaving the loop too soon than never
> when such races occur: once we've served prev (using root if none),
> get out the next time __mem_cgroup_iter_next() cannot deliver.
> 
> Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx>
> ---
> Securing the tree iterator against such races is difficult, I've
> certainly got it wrong myself before.  Although the bug is real, and
> deserves a Cc stable, you may want to play around with other solutions
> before committing to this one.  The current iterator goes back to v3.12:
> I'm really not sure if v3.11 was good or not - I never saw the problem
> in the vanilla kernel, but with Google mods in we also had to make an
> adjustment, there to stop __mem_cgroup_iter() being called endlessly
> from the reclaim level.
> 
>  mm/memcontrol.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> --- mmotm/mm/memcontrol.c	2014-01-10 18:25:02.236448954 -0800
> +++ linux/mm/memcontrol.c	2014-01-12 22:21:10.700570471 -0800
> @@ -1254,8 +1252,11 @@ struct mem_cgroup *mem_cgroup_iter(struc
>  				reclaim->generation = iter->generation;
>  		}
>  
> -		if (prev && !memcg)
> +		if (!memcg) {
> +			if (!prev)
> +				memcg = root;
>  			goto out_unlock;
> +		}
>  	}
>  out_unlock:
>  	rcu_read_unlock();

-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/