From: Roman Gushchin <guro@fb.com> To: Andrew Morton <akpm@linux-foundation.org>, <linux-mm@kvack.org> Cc: Michal Hocko <mhocko@kernel.org>, Johannes Weiner <hannes@cmpxchg.org>, <linux-kernel@vger.kernel.org>, <kernel-team@fb.com>, Roman Gushchin <guro@fb.com>, Michal Hocko <mhocko@suse.com> Subject: [PATCH] mm: memcontrol: switch to rcu protection in drain_all_stock() Date: Thu, 1 Aug 2019 16:35:13 -0700 [thread overview] Message-ID: <20190801233513.137917-1-guro@fb.com> (raw) Commit 72f0184c8a00 ("mm, memcg: remove hotplug locking from try_charge") introduced css_tryget()/css_put() calls in drain_all_stock(), which are supposed to protect the target memory cgroup from being released during the mem_cgroup_is_descendant() call. However, it's not completely safe. In theory, memcg can go away between reading stock->cached pointer and calling css_tryget(). So, let's read the stock->cached pointer and evaluate the memory cgroup inside a rcu read section, and get rid of css_tryget()/css_put() calls. Signed-off-by: Roman Gushchin <guro@fb.com> Cc: Michal Hocko <mhocko@suse.com> --- mm/memcontrol.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 5c7b9facb0eb..d856b64426b7 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2235,21 +2235,22 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) for_each_online_cpu(cpu) { struct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu); struct mem_cgroup *memcg; + bool flush = false; + rcu_read_lock(); memcg = stock->cached; - if (!memcg || !stock->nr_pages || !css_tryget(&memcg->css)) - continue; - if (!mem_cgroup_is_descendant(memcg, root_memcg)) { - css_put(&memcg->css); - continue; - } - if (!test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) { + if (memcg && stock->nr_pages && + mem_cgroup_is_descendant(memcg, root_memcg)) + flush = true; + rcu_read_unlock(); + + if (flush && + !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) { if (cpu == curcpu) drain_local_stock(&stock->work); else schedule_work_on(cpu, &stock->work); } - css_put(&memcg->css); } put_cpu(); mutex_unlock(&percpu_charge_mutex); -- 2.21.0
WARNING: multiple messages have this Message-ID (diff)
From: Hillf Danton <hdanton@sina.com> To: Roman Gushchin <guro@fb.com> Cc: Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org, Michal Hocko <mhocko@kernel.org>, Johannes Weiner <hannes@cmpxchg.org>, linux-kernel@vger.kernel.org, kernel-team@fb.com, Michal Hocko <mhocko@suse.com> Subject: Re: [PATCH] mm: memcontrol: switch to rcu protection in drain_all_stock() Date: Fri, 2 Aug 2019 11:33:33 +0800 [thread overview] Message-ID: <20190801233513.137917-1-guro@fb.com> (raw) Message-ID: <20190802033333.OO_sZQO9dpFnZ1vumT_ij3msqGe1vh_tKY8duC4mJOQ@z> (raw) On Thu, 1 Aug 2019 16:35:13 -0700 Roman Gushchin wrote: > > Commit 72f0184c8a00 ("mm, memcg: remove hotplug locking from try_charge") > introduced css_tryget()/css_put() calls in drain_all_stock(), > which are supposed to protect the target memory cgroup from being > released during the mem_cgroup_is_descendant() call. > > However, it's not completely safe. In theory, memcg can go away > between reading stock->cached pointer and calling css_tryget(). Good catch! > > So, let's read the stock->cached pointer and evaluate the memory > cgroup inside a rcu read section, and get rid of > css_tryget()/css_put() calls. You need to either adjust the boundry of the rcu-protected section, or retain the call pairs, as the memcg cache is dereferenced again in drain_stock(). > > Signed-off-by: Roman Gushchin <guro@fb.com> > Cc: Michal Hocko <mhocko@suse.com> > --- > mm/memcontrol.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 5c7b9facb0eb..d856b64426b7 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2235,21 +2235,22 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) > for_each_online_cpu(cpu) { > struct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu); > struct mem_cgroup *memcg; > + bool flush = false; > > + rcu_read_lock(); > memcg = stock->cached; > - if (!memcg || !stock->nr_pages || !css_tryget(&memcg->css)) > - continue; > - if (!mem_cgroup_is_descendant(memcg, root_memcg)) { > - css_put(&memcg->css); > - continue; > - } > - if (!test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) { > + if (memcg && stock->nr_pages && > + mem_cgroup_is_descendant(memcg, root_memcg)) > + flush = true; > + rcu_read_unlock(); > + > + if (flush && > + !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) { > if (cpu == curcpu) > drain_local_stock(&stock->work); > else > schedule_work_on(cpu, &stock->work); > } > - css_put(&memcg->css); > } > put_cpu(); > mutex_unlock(&percpu_charge_mutex); > -- > 2.21.0 >
next reply other threads:[~2019-08-01 23:35 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-01 23:35 Roman Gushchin [this message] 2019-08-02 3:33 ` [PATCH] mm: memcontrol: switch to rcu protection in drain_all_stock() Hillf Danton 2019-08-02 8:04 ` Michal Hocko 2019-08-02 8:59 ` Michal Hocko 2019-08-02 17:00 ` Roman Gushchin 2019-08-02 17:14 ` Michal Hocko 2019-08-02 17:36 ` Roman Gushchin 2019-08-02 16:55 ` Roman Gushchin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190801233513.137917-1-guro@fb.com \ --to=guro@fb.com \ --cc=akpm@linux-foundation.org \ --cc=hannes@cmpxchg.org \ --cc=kernel-team@fb.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mhocko@kernel.org \ --cc=mhocko@suse.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).