[PATCH v2] mm: memcontrol: switch to rcu protection in drain_all_stock()

* [PATCH v2] mm: memcontrol: switch to rcu protection in drain_all_stock()
@ 2019-08-02 19:22 Roman Gushchin
  2019-08-05 11:11 ` Michal Hocko
  0 siblings, 1 reply; 3+ messages in thread
From: Roman Gushchin @ 2019-08-02 19:22 UTC (permalink / raw)
  To: Andrew Morton, linux-mm
  Cc: Michal Hocko, Johannes Weiner, linux-kernel, kernel-team,
	Roman Gushchin, Michal Hocko, Hillf Danton

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().

This can happen if drain_all_stock() races with drain_local_stock()
performed on the remote cpu as a result of a work, scheduled
by the previous invocation of drain_all_stock().

The race is a bit theoretical and there are few chances to trigger
it, but the current code looks a bit confusing, so it makes sense
to fix it anyway. The code looks like as if css_tryget() and
css_put() are used to protect stocks drainage. It's not necessary
because stocked pages are holding references to the cached cgroup.
And it obviously won't work for works, scheduled on other cpus.

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.

v2: added some explanations to the commit message, no code changes

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Hillf Danton <hdanton@sina.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


^ permalink raw reply related	[flat|nested] 3+ messages in thread