memcg: fix infinite loop

1. task p1 is in /memcg/0
2. p1 does mmap(4096*2, MAP_LOCKED)
3. echo 4096 > /memcg/0/memory.limit_in_bytes

The above 'echo' will never return, unless p1 exited or freed the memory. The cause is we can't reclaim memory from p1, so the while loop in mem_cgroup_resize_limit() won't break.

This patch fixes it by decrementing retry_count regardless the return value of mem_cgroup_hierarchical_reclaim().

Quoted 8 lines · Li Zefan

1. task p1 is in /memcg/0
2. p1 does mmap(4096*2, MAP_LOCKED)
3. echo 4096 > /memcg/0/memory.limit_in_bytes

The above 'echo' will never return, unless p1 exited or freed the memory. The cause is we can't reclaim memory from p1, so the while loop in mem_cgroup_resize_limit() won't break.

Quoted 3 lines

This patch fixes it by decrementing retry_count regardless the return value of mem_cgroup_hierarchical_reclaim().

The problem definitely seems to exist, shouldn't we fix reclaim to return 0, so that we know progress is not made and retry count decrements?

Quoted 13 lines · On Thu, 15 Jan 2009 11:45:57 +0530 Balbir Singh <[email protected]>…

Quoted 11 lines · Li Zefan

1. task p1 is in /memcg/0
2. p1 does mmap(4096*2, MAP_LOCKED)
3. echo 4096 > /memcg/0/memory.limit_in_bytes

The above 'echo' will never return, unless p1 exited or freed the memory. The cause is we can't reclaim memory from p1, so the while loop in mem_cgroup_resize_limit() won't break.

This patch fixes it by decrementing retry_count regardless the return value of mem_cgroup_hierarchical_reclaim().

Quoted 4 lines

The problem definitely seems to exist, shouldn't we fix reclaim to return 0, so that we know progress is not made and retry count decrements?

The behavior is correct. And we already check signal_pending() in the loop. Ctrl-C or SIGALARM will works better than checking retry count.

But adding a new control file, memory.resize_timeout to check timeout is a choice.

Second thought is.
thanks to Kosaki at el, LRU for locked pages is now visible in memory.stat file. So, we may able to have clever way.

==
unevictable = mem_cgroup_get_all_zonestat(mem, LRU_UNEVICLABLE); if (newlimit < unevictable)

	break;

==
But considering hierarchy, this can be complex.
please don't modify current behavior for a while, I'll try to write "hierarchical stat" with CSS_ID patch set's easy hierarchy walk.

Thanks,
-Kame

Quoted 21 lines · KAMEZAWA Hiroyuki

Quoted 17 lines · On Thu, 15 Jan 2009 11:45:57 +0530 Balbir Singh <[email protected]>…

Quoted 11 lines · Li Zefan

1. task p1 is in /memcg/0
2. p1 does mmap(4096*2, MAP_LOCKED)
3. echo 4096 > /memcg/0/memory.limit_in_bytes

The above 'echo' will never return, unless p1 exited or freed the memory. The cause is we can't reclaim memory from p1, so the while loop in mem_cgroup_resize_limit() won't break.

This patch fixes it by decrementing retry_count regardless the return value of mem_cgroup_hierarchical_reclaim().

The problem definitely seems to exist, shouldn't we fix reclaim to return 0, so that we know progress is not made and retry count decrements?

Quoted 2 lines

The behavior is correct. And we already check signal_pending() in the loop. Ctrl-C or SIGALARM will works better than checking retry count.

But this behavior seems like a regression. Please try it in 2.6.28, you'll see it returns EBUSY immediately.

Looks like the return value of mem_cgroup_hierarchical_reclaim() is buggy ?

Quoted 21 lines · On Thu, 15 Jan 2009 15:14:38 +0800 Li Zefan <[email protected]> wrote:

Quoted 20 lines · KAMEZAWA Hiroyuki

Quoted 17 lines · On Thu, 15 Jan 2009 11:45:57 +0530 Balbir Singh <[email protected]>…

Quoted 11 lines · Li Zefan

1. task p1 is in /memcg/0
2. p1 does mmap(4096*2, MAP_LOCKED)
3. echo 4096 > /memcg/0/memory.limit_in_bytes

The above 'echo' will never return, unless p1 exited or freed the memory. The cause is we can't reclaim memory from p1, so the while loop in mem_cgroup_resize_limit() won't break.

This patch fixes it by decrementing retry_count regardless the return value of mem_cgroup_hierarchical_reclaim().

The problem definitely seems to exist, shouldn't we fix reclaim to return 0, so that we know progress is not made and retry count decrements?

Quoted 7 lines

Quoted 2 lines

The behavior is correct. And we already check signal_pending() in the loop. Ctrl-C or SIGALARM will works better than checking retry count.

But this behavior seems like a regression. Please try it in 2.6.28, you'll see it returns EBUSY immediately.

Looks like the return value of mem_cgroup_hierarchical_reclaim() is buggy ?

This is intentional behavior change by
==
memcg-make-oom-less-frequently.patch
==

try_to_free_page() returns positive value if try_to_free_page() reclaims at least 1 pages. It itself doesn't seem to be buggy.

What buggy is resize_limit's retry-out check code, I think.

How about following ?

==

	while (1) {
		if (signal_pending())
			break;
		try to set limit ....
		...
		ret = mem_cgroup_hierarchical_reclaim(memcg,  GFP_KERNEL, false);
		total_progress += ret;	

		if (total_progress > (memcg->res.usage - val) * 2) {
			/*
			 * It seems we reclaimed twice of necessary
			 * pages...this memcg is busy
			 */
			ret = -EBUSY;
			break;
		}
	}

==

Thanks,
-Kame

Quoted 3 lines · KAMEZAWA Hiroyuki

try_to_free_page() returns positive value if try_to_free_page() reclaims at least 1 pages. It itself doesn't seem to be buggy.

Quoted 3 lines

What buggy is resize_limit's retry-out check code, I think.

How about following ?

Not sure.

I didn't look into the reclaim code, so I'd rather let you and Balbir decide if this is a bug and (if yes) how to fix it.

Quoted 22 lines · On Thu, 15 Jan 2009 15:14:38 +0800 Li Zefan <[email protected]> wrote:

Quoted 21 lines · KAMEZAWA Hiroyuki

Quoted 17 lines · On Thu, 15 Jan 2009 11:45:57 +0530 Balbir Singh <[email protected]>…

Quoted 11 lines · Li Zefan

1. task p1 is in /memcg/0
2. p1 does mmap(4096*2, MAP_LOCKED)
3. echo 4096 > /memcg/0/memory.limit_in_bytes

The above 'echo' will never return, unless p1 exited or freed the memory. The cause is we can't reclaim memory from p1, so the while loop in mem_cgroup_resize_limit() won't break.

This patch fixes it by decrementing retry_count regardless the return value of mem_cgroup_hierarchical_reclaim().

The problem definitely seems to exist, shouldn't we fix reclaim to return 0, so that we know progress is not made and retry count decrements?

Quoted 8 lines

Quoted 2 lines

The behavior is correct. And we already check signal_pending() in the loop. Ctrl-C or SIGALARM will works better than checking retry count.

But this behavior seems like a regression. Please try it in 2.6.28, you'll see it returns EBUSY immediately.

Looks like the return value of mem_cgroup_hierarchical_reclaim() is buggy ?

This is intentional behavior change by
==
memcg-make-oom-less-frequently.patch
==

try_to_free_page() returns positive value if try_to_free_page() reclaims at least 1 pages. It itself doesn't seem to be buggy.

What buggy is resize_limit's retry-out check code, I think.

How about following ?
==

	while (1) {
		if (signal_pending())
			break;
		try to set limit ....
		...
		ret = mem_cgroup_hierarchical_reclaim(memcg,  GFP_KERNEL, false);
		total_progress += ret;	

		if (total_progress > (memcg->res.usage - val) * 2) {
			/*
			 * It seems we reclaimed twice of necessary
			 * pages...this memcg is busy
			 */
			ret = -EBUSY;
			break;

		}
	}

==

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

1. task p1 is in /memcg/0
2. p1 does mmap(4096*2, MAP_LOCKED)
3. echo 4096 > /memcg/0/memory.limit_in_bytes

The above 'echo' will never return, unless p1 exited or freed the memory. The cause is we can't reclaim memory from p1, so the while loop in mem_cgroup_resize_limit() won't break.

This patch fixes it by decrementing retry_count regardless the return value of mem_cgroup_hierarchical_reclaim().

Signed-off-by Li Zefan <[email protected]>

---
 mm/memcontrol.c |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

mm/memcontrol.c +4 -11

index fb62b43..1995098 100644
@@ -1524,11 +1524,10 @@ static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
 {
 
 	int retry_count = MEM_CGROUP_RECLAIM_RETRIES;
-	int progress;
 	u64 memswlimit;
 	int ret = 0;
 
-	while (retry_count) {
+	while (retry_count--) {
 		if (signal_pending(current)) {
 			ret = -EINTR;
 			break;

@@ -1551,9 +1550,7 @@ static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
 		if (!ret)
 			break;
 
-		progress = mem_cgroup_hierarchical_reclaim(memcg, GFP_KERNEL,
-							   false);
-  		if (!progress)			retry_count--;
+		mem_cgroup_hierarchical_reclaim(memcg, GFP_KERNEL, false);
 	}
 
 	return ret;

@@ -1563,13 +1560,13 @@ int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
 				unsigned long long val)
 {
 	int retry_count = MEM_CGROUP_RECLAIM_RETRIES;
-	u64 memlimit, oldusage, curusage;
+	u64 memlimit;
 	int ret;
 
 	if (!do_swap_account)
 		return -EINVAL;
 
-	while (retry_count) {
+	while (retry_count--) {
 		if (signal_pending(current)) {
 			ret = -EINTR;
 			break;

@@ -1592,11 +1589,7 @@ int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
 		if (!ret)
 			break;
 
-		oldusage = res_counter_read_u64(&memcg->memsw, RES_USAGE);
 		mem_cgroup_hierarchical_reclaim(memcg, GFP_KERNEL, true);
-		curusage = res_counter_read_u64(&memcg->memsw, RES_USAGE);
-		if (curusage >= oldusage)
-			retry_count--;
 	}
 	return ret;
 }

Signature · 1 line

1.5.4.rc3

Quoted 8 lines · Li Zefan

1. task p1 is in /memcg/0
2. p1 does mmap(4096*2, MAP_LOCKED)
3. echo 4096 > /memcg/0/memory.limit_in_bytes

The above 'echo' will never return, unless p1 exited or freed the memory. The cause is we can't reclaim memory from p1, so the while loop in mem_cgroup_resize_limit() won't break.

But it can be interrupted, right ?

I don't think this would be a big problem.

1. task p1 is in /memcg/0
2. p1 does mmap(4096*2, MAP_LOCKED)
3. echo 4096 > /memcg/0/memory.limit_in_bytes

The above 'echo' will never return, unless p1 exited or freed the memory. The cause is we can't reclaim memory from p1, so the while loop in mem_cgroup_resize_limit() won't break.

This patch fixes it by decrementing retry_count regardless the return value of mem_cgroup_hierarchical_reclaim().

Quoted 11 lines · On Thu, 15 Jan 2009 14:07:51 +0800 Li Zefan <[email protected]> wrote:

1. task p1 is in /memcg/0
2. p1 does mmap(4096*2, MAP_LOCKED)
3. echo 4096 > /memcg/0/memory.limit_in_bytes

The above 'echo' will never return, unless p1 exited or freed the memory. The cause is we can't reclaim memory from p1, so the while loop in mem_cgroup_resize_limit() won't break.

This patch fixes it by decrementing retry_count regardless the return value of mem_cgroup_hierarchical_reclaim().

Maybe a patch like this is necessary. But details are not fixed yet. Any comments are welcome.

(This is base on my CSS ID patch set.)

-Kame
==

From: KAMEZAWA Hiroyuki <[email protected]>

As Li Zefan pointed out, shrinking memcg's limit should return -EBUSY after reasonable retries. This patch tries to fix the current behavior of shrink_usage.

Before looking into "shrink should return -EBUSY" problem, we should fix hierarchical reclaim code. It compares current usage and current limit, but it only makes sense when the kernel reclaims memory because hit limits. This is also a problem.

What this patch does are.

1. add new argument "shrink" to hierarchical reclaim. If "shrink==true", hierarchical reclaim returns immediately and the caller checks the kernel should shrink more or not.
(At shrinking memory, usage is always smaller than limit. So check for
usage < limit is useless.)

2. For adjusting to above change, 2 changes in "shrink"'s retry path. 2-a. retry_count depends on # of children because the kernel visits the children under hierarchy one by one.
2-b. rather than checking return value of hierarchical_reclaim's progress,
compares usage-before-shrink and usage-after-shrink.
If usage-before-shrink > usage-after-shrink, retry_count is decremented.

Reported-by Li Zefan <[email protected]>

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 mm/memcontrol.c |   71 +++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 60 insertions(+), 11 deletions(-)

Index: mmotm-2.6.29-Jan16/mm/memcontrol.c
===================================================================

mm/memcontrol.c +56 -11

@@ -696,6 +696,23 @@ static int mem_cgroup_walk_tree(struct m
 	return ret;
 }
 
+static int mem_cgroup_count_children_cb(struct mem_cgroup *mem, void *data)
+{
+	int *val = data;
+	(*val)++;
+	return 0;
+}
+
+static int mem_cgroup_count_children(struct mem_cgroup *mem)
+{
+	int num = 0;
+	rcu_read_lock();
+ 	mem_cgroup_walk_tree(mem, &num, mem_cgroup_count_children_cb);
+	rcu_read_unlock();
+
+	return num;
+}
+
 /*
  * Visit the first child (need not be the first child as per the ordering
  * of the cgroup list, since we track last_scanned_child) of @mem and use

@@ -752,9 +769,12 @@ mem_cgroup_select_victim(struct mem_cgro
  * We give up and return to the caller when scan_age is increased by 2. This
  * means try_to_free_mem_cgroup_pages() is called against all children cgroup,
  * at least once. The caller itself will do further retry if necessary.
+ *
+ * If shrink==true, this routine doesn't check usage < limit, and just return
+ * quickly. It depends on callers whether shrinking is enough or not.
  */
 static int mem_cgroup_hierarchical_reclaim(struct mem_cgroup *root_mem,
-						gfp_t gfp_mask, bool noswap)
+				gfp_t gfp_mask, bool noswap, bool shrink)
 {
 	struct mem_cgroup *victim;
 	unsigned long start_age;

@@ -782,6 +802,13 @@ static int mem_cgroup_hierarchical_recla
 		ret = try_to_free_mem_cgroup_pages(victim, gfp_mask, noswap,
 						   get_swappiness(victim));
 		css_put(&victim->css);
+		/*
+		 * At shrinking usage, we can't check we should stop here or
+		 * reclaim more. It's depends on callers. last_scanned_child
+		 * will work enough for doing round-robin.
+		 */
+		if (shrink)
+			return ret;
 		total += ret;
 		if (mem_cgroup_check_under_limit(root_mem))
 			return 1 + total;

@@ -867,7 +894,7 @@ static int __mem_cgroup_try_charge(struc
 			goto nomem;
 
 		ret = mem_cgroup_hierarchical_reclaim(mem_over_limit, gfp_mask,
-							noswap);
+							noswap, false);
 		if (ret)
 			continue;
 

@@ -1488,6 +1515,7 @@ int mem_cgroup_shrink_usage(struct page 
 	struct mem_cgroup *mem = NULL;
 	int progress = 0;
 	int retry = MEM_CGROUP_RECLAIM_RETRIES;
+	int children;
 
 	if (mem_cgroup_disabled())
 		return 0;

@@ -1499,7 +1527,8 @@ int mem_cgroup_shrink_usage(struct page 
 		return 0;
 
 	do {
-		progress = mem_cgroup_hierarchical_reclaim(mem, gfp_mask, true);
+		progress = mem_cgroup_hierarchical_reclaim(mem,
+					gfp_mask, true, false);
 		progress += mem_cgroup_check_under_limit(mem);
 	} while (!progress && --retry);
 

@@ -1514,11 +1543,21 @@ static DEFINE_MUTEX(set_limit_mutex);
 static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
 				unsigned long long val)
 {
-
-	int retry_count = MEM_CGROUP_RECLAIM_RETRIES;
+	int retry_count;
 	int progress;
 	u64 memswlimit;
 	int ret = 0;
+	int children = mem_cgroup_count_children(memcg);
+	u64 curusage, oldusage, minusage;
+
+	/*
+	 * For keeping hierarchical_reclaim simple, how long we should retry
+	 * is depends on callers. We set our retry-count to be function
+	 * of # of children which we should visit in this loop.
+	 */
+	retry_count = MEM_CGROUP_RECLAIM_RETRIES * children;
+
+	oldusage = res_counter_read_u64(&memcg->res, RES_USAGE);
 
 	while (retry_count) {
 		if (signal_pending(current)) {

@@ -1544,8 +1583,13 @@ static int mem_cgroup_resize_limit(struc
 			break;
 
 		progress = mem_cgroup_hierarchical_reclaim(memcg, GFP_KERNEL,
-							   false);
-  		if (!progress)			retry_count--;
+						   false, true);
+		curusage = res_counter_read_u64(&memcg->res, RES_USAGE);
+		/* Usage is reduced ? */
+  		if (curusage >= oldusage)
+			retry_count--;
+		else
+			oldusage = curusage;
 	}
 
 	return ret;

@@ -1554,13 +1598,16 @@ static int mem_cgroup_resize_limit(struc
 int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
 				unsigned long long val)
 {
-	int retry_count = MEM_CGROUP_RECLAIM_RETRIES;
+	int retry_count;
 	u64 memlimit, oldusage, curusage;
+	int children = mem_cgroup_count_children(memcg);
 	int ret;
 
 	if (!do_swap_account)
 		return -EINVAL;
-
+	/* see mem_cgroup_resize_res_limit */
+ 	retry_count = children * MEM_CGROUP_RECLAIM_RETRIES;
+	oldusage = res_counter_read_u64(&memcg->memsw, RES_USAGE);
 	while (retry_count) {
 		if (signal_pending(current)) {
 			ret = -EINTR;

@@ -1584,11 +1631,13 @@ int mem_cgroup_resize_memsw_limit(struct
 		if (!ret)
 			break;
 
-		oldusage = res_counter_read_u64(&memcg->memsw, RES_USAGE);
-		mem_cgroup_hierarchical_reclaim(memcg, GFP_KERNEL, true);

Quoted 42 lines · KAMEZAWA Hiroyuki

Quoted 11 lines · On Thu, 15 Jan 2009 14:07:51 +0800 Li Zefan <[email protected]> wrote:

1. task p1 is in /memcg/0
2. p1 does mmap(4096*2, MAP_LOCKED)
3. echo 4096 > /memcg/0/memory.limit_in_bytes

The above 'echo' will never return, unless p1 exited or freed the memory. The cause is we can't reclaim memory from p1, so the while loop in mem_cgroup_resize_limit() won't break.

This patch fixes it by decrementing retry_count regardless the return value of mem_cgroup_hierarchical_reclaim().

Maybe a patch like this is necessary. But details are not fixed yet. Any comments are welcome.

(This is base on my CSS ID patch set.)

-Kame
==

From: KAMEZAWA Hiroyuki <[email protected]>

As Li Zefan pointed out, shrinking memcg's limit should return -EBUSY after reasonable retries. This patch tries to fix the current behavior of shrink_usage.

Before looking into "shrink should return -EBUSY" problem, we should fix hierarchical reclaim code. It compares current usage and current limit, but it only makes sense when the kernel reclaims memory because hit limits. This is also a problem.

What this patch does are.

1. add new argument "shrink" to hierarchical reclaim. If "shrink==true", hierarchical reclaim returns immediately and the caller checks the kernel should shrink more or not.
(At shrinking memory, usage is always smaller than limit. So check for
usage < limit is useless.)

Quoted 7 lines

2. For adjusting to above change, 2 changes in "shrink"'s retry path. 2-a. retry_count depends on # of children because the kernel visits the children under hierarchy one by one.
2-b. rather than checking return value of hierarchical_reclaim's progress,
compares usage-before-shrink and usage-after-shrink.
If usage-before-shrink > usage-after-shrink, retry_count is decremented.

The code seems to do the reverse, it checks for
if (currusage >= oldusage)

Quoted 6 lines

mm/memcontrol.c +2 -2

-		oldusage = res_counter_read_u64(&memcg->memsw, RES_USAGE);
-		mem_cgroup_hierarchical_reclaim(memcg, GFP_KERNEL, true);
+		mem_cgroup_hierarchical_reclaim(memcg, GFP_KERNEL, true, true);
 		curusage = res_counter_read_u64(&memcg->memsw, RES_USAGE);
+		/* Usage is reduced ? */
 		if (curusage >= oldusage)

mm/memcontrol.c int mem_cgroup_resize_memsw_limit(struct · line 1643

Quoted 6 lines

mm/memcontrol.c +2 -0

 			retry_count--;
+		else
+			oldusage = curusage;
 	}
 	return ret;
 }

Has this been tested? It seems OK to the naked eye :)