Re: [PATCH] mm: memcontrol: fix possible memcg leak due to interrupted reclaim

[Michal Hocko]

On Sat 12-12-15 22:18:55, Vladimir Davydov wrote:
> On Sat, Dec 12, 2015 at 11:45:40AM -0500, Johannes Weiner wrote:
> > On Sat, Dec 12, 2015 at 04:34:02PM +0300, Vladimir Davydov wrote:
> > > Memory cgroup reclaim can be interrupted with mem_cgroup_iter_break()
> > > once enough pages have been reclaimed, in which case, in contrast to a
> > > full round-trip over a cgroup sub-tree, the current position stored in
> > > mem_cgroup_reclaim_iter of the target cgroup does not get invalidated
> > > and so is left holding the reference to the last scanned cgroup. If the
> > > target cgroup does not get scanned again (we might have just reclaimed
> > > the last page or all processes might exit and free their memory
> > > voluntary), we will leak it, because there is nobody to put the
> > > reference held by the iterator.
> > > 
> > > The problem is easy to reproduce by running the following command
> > > sequence in a loop:
> > > 
> > >     mkdir /sys/fs/cgroup/memory/test
> > >     echo 100M > /sys/fs/cgroup/memory/test/memory.limit_in_bytes
> > >     echo $$ > /sys/fs/cgroup/memory/test/cgroup.procs
> > >     memhog 150M
> > >     echo $$ > /sys/fs/cgroup/memory/cgroup.procs
> > >     rmdir test
> > > 
> > > The cgroups generated by it will never get freed.
> > > 
> > > This patch fixes this issue by making mem_cgroup_iter_break() clear
> > > mem_cgroup_reclaim_iter->position in case it points to the memory cgroup
> > > we interrupted reclaim on.
> > > 
> > > Fixes: 5ac8fb31ad2e ("mm: memcontrol: convert reclaim iterator to simple css refcounting")
> > > Signed-off-by: Vladimir Davydov <vdavydov@xxxxxxxxxxxxx>
> > > Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
> > > Cc: Michal Hocko <mhocko@xxxxxxxxxx>
> > > Cc: <stable@xxxxxxxxxxxxxxx> # 3.19+
> > 
> > Good catch!

Indeed!

[...]
> > @@ -2425,21 +2425,6 @@ static bool shrink_zone(struct zone *zone, struct scan_control *sc,
> >  				   sc->nr_scanned - scanned,
> >  				   sc->nr_reclaimed - reclaimed);
> >  
> > -			/*
> > -			 * Direct reclaim and kswapd have to scan all memory
> > -			 * cgroups to fulfill the overall scan target for the
> > -			 * zone.
> > -			 *
> > -			 * Limit reclaim, on the other hand, only cares about
> > -			 * nr_to_reclaim pages to be reclaimed and it will
> > -			 * retry with decreasing priority if one round over the
> > -			 * whole hierarchy is not sufficient.
> > -			 */
> > -			if (!global_reclaim(sc) &&
> > -					sc->nr_reclaimed >= sc->nr_to_reclaim) {
> > -				mem_cgroup_iter_break(root, memcg);
> > -				break;
> > -			}
> 
> Dunno. I like it, because it's simple and clean, but I'm unsure: can't
> it result in lags when performing memcg reclaim for deep hierarchies?

Yes I think we want to preserve this.

> For global reclaim we have kswapd, which tries to keep the system within
> bounds so as to avoid direct reclaim at all. Memcg lacks such thing, and
> interleave walks looks like a good compensation for it.

Agreed

> Alternatively, we could avoid taking reference to iter->position and
> make use of css_released cgroup callback to invalidate reclaim
> iterators. With this approach, upper level cgroups shouldn't receive
> unfairly high pressure in comparison to their children. Something like
> this, maybe?
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 87af26a24491..fcc5133210a0 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -859,14 +859,12 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>  		if (prev && reclaim->generation != iter->generation)
>  			goto out_unlock;
>  
> -		do {
> +		while (1) {
>  			pos = READ_ONCE(iter->position);
> -			/*
> -			 * A racing update may change the position and
> -			 * put the last reference, hence css_tryget(),
> -			 * or retry to see the updated position.
> -			 */
> -		} while (pos && !css_tryget(&pos->css));
> +			if (!pos || css_tryget(&pos->css))
> +				break;
> +			cmpxchg(&iter->position, pos, NULL);

This really deserves a comment.

> +		}
>  	}
>  
>  	if (pos)
> @@ -912,12 +910,7 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>  	}
>  
>  	if (reclaim) {
> -		if (cmpxchg(&iter->position, pos, memcg) == pos) {
> -			if (memcg)
> -				css_get(&memcg->css);
> -			if (pos)
> -				css_put(&pos->css);
> -		}
> +		cmpxchg(&iter->position, pos, memcg);
>  
>  		/*
>  		 * pairs with css_tryget when dereferencing iter->position
> @@ -955,6 +948,28 @@ void mem_cgroup_iter_break(struct mem_cgroup *root,
>  		css_put(&prev->css);
>  }
>  
> +static void invalidate_reclaim_iterators(struct mem_cgroup *dead_memcg)
> +{
> +	struct mem_cgroup *memcg = dead_memcg;
> +	struct mem_cgroup_reclaim_iter *iter;
> +	struct mem_cgroup_per_zone *mz;
> +	int nid, zid;
> +	int i;
> +
> +	while ((memcg = parent_mem_cgroup(memcg))) {
> +		for_each_node(nid) {
> +			for (zid = 0; zid < MAX_NR_ZONES; zid++) {
> +				mz = &memcg->nodeinfo[nid]->zoneinfo[zid];
> +				for (i = 0; i <= DEF_PRIORITY; i++) {
> +					iter = &mz->iter[i];
> +					cmpxchg(&iter->position,
> +						dead_memcg, NULL);
> +				}
> +			}
> +		}
> +	}

An earlier attempts (I cannot find the link right now) were doing this
walk but the patch was dropping a reference and this looks much better.

> +}
> +
>  /*
>   * Iteration constructs for visiting all cgroups (under a tree).  If
>   * loops are exited prematurely (break), mem_cgroup_iter_break() must
> @@ -4375,6 +4390,13 @@ static void mem_cgroup_css_offline(struct cgroup_subsys_state *css)
>  	wb_memcg_offline(memcg);
>  }
>  
> +static void mem_cgroup_css_released(struct cgroup_subsys_state *css)
> +{
> +	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
> +
> +	invalidate_reclaim_iterators(memcg);
> +}
> +
>  static void mem_cgroup_css_free(struct cgroup_subsys_state *css)
>  {
>  	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
> @@ -5229,6 +5251,7 @@ struct cgroup_subsys memory_cgrp_subsys = {
>  	.css_alloc = mem_cgroup_css_alloc,
>  	.css_online = mem_cgroup_css_online,
>  	.css_offline = mem_cgroup_css_offline,
> +	.css_released = mem_cgroup_css_released,
>  	.css_free = mem_cgroup_css_free,
>  	.css_reset = mem_cgroup_css_reset,
>  	.can_attach = mem_cgroup_can_attach,

I was not aware of css_released callback but from my reading of
7d172cc89b85 ("cgroup: add cgroup_subsys->css_released()") this looks
correct.

Feel free to add
Acked-by: Michal Hocko <mhocko@xxxxxxxx>

Thanks!
-- 
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/