[Devel] [PATCH rh7] mm/memcg: use seqlock to protect reclaim_iter updates.

[Andrey Ryabinin]

Currently mem_cgroup_iter() uses complicated, odd, weak reference scheme
to iterate memcgs during reclaim. The basic scheme looks like this:

 	if (iter->last_dead_count == *sequence) {
		smp_rmb();
		position = iter->last_visited;
...
	new_position = __mem_cgroup_iter_next(root, last_visited);
...
 	iter->last_visited = new_position;
	smp_wmb();
 	iter->last_dead_count = sequence;

The problem is that all this code could run in parallel. E.g.
we may have several threads simmulatniously updating
"iter->last_visited", "iter->last_dead_count" fields to different
values. In result we may have iter in inconsistent state - last_visited
from one writer, and last_dead_count from another.

It seems to may cause use-afte-frees in mem_cgroup_iter(), although
I'm not entirely sure about that. I still can't understand how this
mess should work.

Use seqlock to protect iter updates.

https://jira.sw.ru/browse/PSBM-83369
Signed-off-by: Andrey Ryabinin <aryabinin at virtuozzo.com>
---
 mm/memcontrol.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 99d5da15b377..e0303b428ac1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -188,6 +188,7 @@ struct mem_cgroup_reclaim_iter {
 	 */
 	struct mem_cgroup *last_visited;
 	unsigned long last_dead_count;
+	seqlock_t last_visited_lock;
 
 	/* scan generation, increased every round-trip */
 	unsigned int generation;
@@ -1279,6 +1280,8 @@ mem_cgroup_iter_load(struct mem_cgroup_reclaim_iter *iter,
 		     int *sequence)
 {
 	struct mem_cgroup *position = NULL;
+	unsigned seq;
+
 	/*
 	 * A cgroup destruction happens in two stages: offlining and
 	 * release.  They are separated by a RCU grace period.
@@ -1288,9 +1291,13 @@ mem_cgroup_iter_load(struct mem_cgroup_reclaim_iter *iter,
 	 * released, tryget will fail if we lost the race.
 	 */
 	*sequence = atomic_read(&root->dead_count);
+retry:
+	seq = read_seqbegin(&iter->last_visited_lock);
 	if (iter->last_dead_count == *sequence) {
-		smp_rmb();
-		position = iter->last_visited;
+		position = READ_ONCE(iter->last_visited);
+
+		if (read_seqretry(&iter->last_visited_lock, seq))
+			goto retry;
 
 		/*
 		 * We cannot take a reference to root because we might race
@@ -1321,9 +1328,10 @@ static void mem_cgroup_iter_update(struct mem_cgroup_reclaim_iter *iter,
 	 * don't lose destruction events in between.  We could have
 	 * raced with the destruction of @new_position after all.
 	 */
+	write_seqlock(&iter->last_visited_lock);
 	iter->last_visited = new_position;
-	smp_wmb();
 	iter->last_dead_count = sequence;
+	write_sequnlock(&iter->last_visited_lock);
 }
 
 /**
@@ -5912,11 +5920,15 @@ static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg, int node)
 		return 1;
 
 	for (zone = 0; zone < MAX_NR_ZONES; zone++) {
+		int i;
+
 		mz = &pn->zoneinfo[zone];
 		lruvec_init(&mz->lruvec);
 		mz->usage_in_excess = 0;
 		mz->on_tree = false;
 		mz->memcg = memcg;
+		for (i = 0; i < ARRAY_SIZE(mz->reclaim_iter); i++)
+			seqlock_init(&mz->reclaim_iter[i].last_visited_lock);
 	}
 	memcg->info.nodeinfo[node] = pn;
 	return 0;
-- 
2.16.1