Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964837AbbLOJ6u (ORCPT ); Tue, 15 Dec 2015 04:58:50 -0500 Received: from mail-wm0-f50.google.com ([74.125.82.50]:36746 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933271AbbLOJ6q (ORCPT ); Tue, 15 Dec 2015 04:58:46 -0500 Date: Tue, 15 Dec 2015 10:58:41 +0100 From: Michal Hocko To: Vladimir Davydov Cc: Johannes Weiner , Andrew Morton , stable@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm: memcontrol: fix possible memcg leak due to interrupted reclaim Message-ID: <20151215095841.GC14350@dhcp22.suse.cz> References: <1449927242-9608-1-git-send-email-vdavydov@virtuozzo.com> <20151212164540.GA7107@cmpxchg.org> <20151212191855.GE28521@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151212191855.GE28521@esperanza> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6660 Lines: 190 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 > > > Cc: Johannes Weiner > > > Cc: Michal Hocko > > > Cc: # 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 Thanks! -- Michal Hocko SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/