memcg: prevent -ENOMEM in pre_destroy()

The conditions are handled as -EBUSY, _now_.

Quoted 1 line · On Fri, 11 May 2012 18:45:18 +0900 KAMEZAWA Hiroyuki…

The conditions are handled as -EBUSY, _now_.

The changelog is poor. I rewrote it to

: hugetlb_force_memcg_empty() incorrectly returns 0 (success) when the : cgroup is found to be busy. Return -EBUSY instead.

But it still doesn't tell us the end-user-visible effects of the bug. It should.

mm/hugetlb.c int hugetlb_force_memcg_empty(struct cgroup *cgroup) · line 1928

Quoted 5 lines · KAMEZAWA Hiroyuki

mm/hugetlb.c +3 -1

-		if (cgroup_task_count(cgroup) || !list_empty(&cgroup->children))
+		if (cgroup_task_count(cgroup)
+			|| !list_empty(&cgroup->children)) {
+			ret = -EBUSY;
 			goto out;

Why break the line? It doesn't go over 80 col.

Quoted 8 lines · Tejun Heo

Review

Quoted 5 lines · KAMEZAWA Hiroyuki

(quoted diff) +3 -1

-		if (cgroup_task_count(cgroup) || !list_empty(&cgroup->children))
+		if (cgroup_task_count(cgroup)
+			|| !list_empty(&cgroup->children)) {
+			ret = -EBUSY;
 			goto out;

Why break the line? It doesn't go over 80 col.

Ooh, it does. Sorry, my bad. But still, isn't it more usual to leave the operator in the preceding line and align the start of the second line with the first? ie.

		if (cgroup_task_count(cgroup) ||
		    !list_empty(&cgroup->children)) {

Quoted 10 lines · Tejun Heo

Quoted 8 lines · Tejun Heo

mm/hugetlb.c int hugetlb_force_memcg_empty(struct cgroup *cgroup) · line 1928

Quoted 5 lines · KAMEZAWA Hiroyuki

mm/hugetlb.c +3 -1

-		if (cgroup_task_count(cgroup) || !list_empty(&cgroup->children))
+		if (cgroup_task_count(cgroup)
+			|| !list_empty(&cgroup->children)) {
+			ret = -EBUSY;
 			goto out;

Why break the line? It doesn't go over 80 col.

Quoted 7 lines

Ooh, it does. Sorry, my bad. But still, isn't it more usual to leave the operator in the preceding line and align the start of the second line with the first? ie.

		if (cgroup_task_count(cgroup) ||
		    !list_empty(&cgroup->children)) {

How about this ?
==
From: KAMEZAWA Hiroyuki <[email protected]>
Date: Fri, 27 Apr 2012 13:19:19 +0900
Subject: [PATCH] memcg: fix error code in hugetlb_force_memcg_empty()

Changelog:
- clean up.

Reviewed-by Aneesh Kumar K.V <[email protected]>

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

From: Frederic Weisbecker <[email protected]>

At killing res_counter which is a child of other counter, we need to do
res_counter_uncharge(child, xxx) res_counter_charge(parent, xxx)

This is not atomic and wasting cpu. This patch adds res_counter_uncharge_until(). This function's uncharge propagates to ancestors until specified res_counter.

res_counter_uncharge_until(child, parent, xxx)

Now, ops is atomic and efficient.

Changelog since v2
- removed unnecessary lines.
- Fixed 'From' , this patch comes from his series. Please signed-off-by if good.

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

Quoted 15 lines · On Fri, 11 May 2012 18:47:06 +0900 KAMEZAWA Hiroyuki…

From: Frederic Weisbecker <[email protected]>

At killing res_counter which is a child of other counter, we need to do
res_counter_uncharge(child, xxx) res_counter_charge(parent, xxx)

This is not atomic and wasting cpu. This patch adds res_counter_uncharge_until(). This function's uncharge propagates to ancestors until specified res_counter.

res_counter_uncharge_until(child, parent, xxx)

Now, ops is atomic and efficient.

Quoted 5 lines

Changelog since v2
- removed unnecessary lines.
- Fixed 'From' , this patch comes from his series. Please signed-off-by if good.

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

Frederic's Signed-off-by: is unavaliable?

Quoted 24 lines · Andrew Morton

Quoted 20 lines · On Fri, 11 May 2012 18:47:06 +0900 KAMEZAWA Hiroyuki…

From: Frederic Weisbecker <[email protected]>

At killing res_counter which is a child of other counter, we need to do
     res_counter_uncharge(child, xxx) res_counter_charge(parent, xxx)

This is not atomic and wasting cpu. This patch adds res_counter_uncharge_until(). This function's uncharge propagates to ancestors until specified res_counter.

     res_counter_uncharge_until(child, parent, xxx)

Now, ops is atomic and efficient.

Changelog since v2
 - removed unnecessary lines.
 - Fixed 'From' , this patch comes from his series. Please signed-off-by if good.

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

Quoted 2 lines

Frederic's Signed-off-by: is unavaliable?

I didn't add his Signed-off because I modified his orignal patch a little... I dropped res_counter_charge_until() because it's not used in this series, I have no justification for adding it.
The idea of res_counter_uncharge_until() is from his patch.

2012/5/14 KAMEZAWA Hiroyuki <[email protected]>:

Quoted 26 lines

Quoted 24 lines · Andrew Morton

Quoted 20 lines · On Fri, 11 May 2012 18:47:06 +0900 KAMEZAWA Hiroyuki…

From: Frederic Weisbecker <[email protected]>

At killing res_counter which is a child of other counter, we need to do
res_counter_uncharge(child, xxx) res_counter_charge(parent, xxx)

This is not atomic and wasting cpu. This patch adds res_counter_uncharge_until(). This function's uncharge propagates to ancestors until specified res_counter.

res_counter_uncharge_until(child, parent, xxx)

Now, ops is atomic and efficient.

Changelog since v2
- removed unnecessary lines.
- Fixed 'From' , this patch comes from his series. Please signed-off-by if good.

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

Quoted 7 lines

Quoted 2 lines

Frederic's Signed-off-by: is unavaliable?

I didn't add his Signed-off because I modified his orignal patch a little... I dropped res_counter_charge_until() because it's not used in this series, I have no justification for adding it.
The idea of res_counter_uncharge_until() is from his patch.

The property of Signed-off-by is that as long as you carry/relay/modify a patch, you add your
own signed-off-by. But you can't remove the signed off by of somebody in the chain.

Even if you did a change in the patch, you need to preserve the chain.

There may be some special cases with "Original-patch-from:" tags used when one heavily inspire from a patch without taking much of its original code.

Quoted 3 lines

There may be some special cases with "Original-patch-from:" tags used when one heavily inspire from a patch without taking much of its original code.

Is this ok ?

[PATCH 2/6] memcg: add res_counter_uncharge_until()

From: Frederic Weisbecker <[email protected]>

At killing res_counter which is a child of other counter, we need to do
res_counter_uncharge(child, xxx) res_counter_charge(parent, xxx)

This is not atomic and wasting cpu. This patch adds res_counter_uncharge_until(). This function's uncharge propagates to ancestors until specified res_counter.

res_counter_uncharge_until(child, parent, xxx)

Now, ops is atomic and efficient.

Changelog since v2
- removed unnecessary lines.
- added 'From' , this patch comes from his one.

Signed-off-by Frederic Weisbecker <[email protected]>

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

mm/memcontrol.c +1 -7

@@ -3351,9 +3339,8 @@ int mem_cgroup_move_hugetlb_parent(int idx, struct cgroup *cgroup,
 	struct page_cgroup *pc;
 	int csize,  ret = 0;
 	struct res_counter *fail_res;
-	struct cgroup *pcgrp = cgroup->parent;
-	struct mem_cgroup *parent = mem_cgroup_from_cont(pcgrp);
 	struct mem_cgroup *memcg  = mem_cgroup_from_cont(cgroup);
+	struct mem_cgroup *parent = parent_mem_cgroup(memcg);
 	struct res_counter *counter;
 
 	if (!get_page_unless_zero(page))

@@ -3366,13 +3353,11 @@ int mem_cgroup_move_hugetlb_parent(int idx, struct cgroup *cgroup,
 
 	csize = PAGE_SIZE << compound_order(page);
 	/* If parent->use_hierarchy == 0, we need to charge parent */
-	if (!parent->use_hierarchy) {
-		ret = res_counter_charge(&parent->hugepage[idx],
-					 csize, &fail_res);
-		if (ret) {
-			ret = -EBUSY;

Quoted 20 lines · KAMEZAWA Hiroyuki

mm/memcontrol.c +1 -7

@@ -3351,9 +3339,8 @@ int mem_cgroup_move_hugetlb_parent(int idx, struct cgroup *cgroup,
 	struct page_cgroup *pc;
 	int csize,  ret = 0;
 	struct res_counter *fail_res;
-	struct cgroup *pcgrp = cgroup->parent;
-	struct mem_cgroup *parent = mem_cgroup_from_cont(pcgrp);
 	struct mem_cgroup *memcg  = mem_cgroup_from_cont(cgroup);
+	struct mem_cgroup *parent = parent_mem_cgroup(memcg);
 	struct res_counter *counter;
 
 	if (!get_page_unless_zero(page))

@@ -3366,13 +3353,11 @@ int mem_cgroup_move_hugetlb_parent(int idx, struct cgroup *cgroup,
 
 	csize = PAGE_SIZE << compound_order(page);
 	/* If parent->use_hierarchy == 0, we need to charge parent */
-	if (!parent->use_hierarchy) {
-		ret = res_counter_charge(&parent->hugepage[idx],
-					 csize, &fail_res);
-		if (ret) {
-			ret = -EBUSY;

mm/memcontrol.c int mem_cgroup_move_hugetlb_parent(int idx, struct cgroup *cgroup, · line 3363

Quoted 10 lines

mm/memcontrol.c +5 -2

-			goto err_out;
-		}
+	if (!parent) {
+		parent = root_mem_cgroup;
+		/* root has no limit */
+		res_counter_charge_nofail(&parent->hugepage[idx],
+				 csize, &fail_res);
 	}
 	counter = &memcg->hugepage[idx];
 	res_counter_uncharge_until(counter, counter->parent, csize);

This function can simply return 0 now, so no point in having int return. Make it return void?

Also, follow-up patches to cleanup -ENOMEM failure handling in the callers would be nice.

I picked this up from Costa's slub memcg series. For fixing added warning by patch 4.
==
From: Glauber Costa <[email protected]>
Subject: [PATCH 6/6] remove __must_check for res_counter_charge_nofail()

Since we will succeed with the allocation no matter what, there isn't the need to use __must_check with it. It can very well be optional.

Signed-off-by Glauber Costa <[email protected]>

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

Quoted 6 lines · KAMEZAWA Hiroyuki

I picked this up from Costa's slub memcg series. For fixing added warning by patch 4.
==
From: Glauber Costa <[email protected]>
Subject: [PATCH 6/6] remove __must_check for res_counter_charge_nofail()

Quoted 6 lines

Since we will succeed with the allocation no matter what, there isn't the need to use __must_check with it. It can very well be optional.

Signed-off-by Glauber Costa <[email protected]>

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

For 3-6,

Reviewed-by Tejun Heo <[email protected]>

Thanks a lot for doing this. This doesn't solve all the failure paths tho. ie. what about -EINTR failures from lock contention? pre_destroy() would probably need delay and retry logic with WARN_ON_ONCE() on !-EINTR failures.

Hi, here is v3 based on memcg-devel tree.
git://github.com/mstsxfx/memcg-devel.git

This patch series is for avoiding -ENOMEM at calling pre_destroy() which is called at rmdir(). After this patch, charges will be moved to root (if use_hierarchy==0) or parent (if use_hierarchy==1), and we'll not see -ENOMEM in rmdir() of cgroup.

v2 included some other patches than ones for handling -ENOMEM problem, but I divided it. I'd like to post others in different series, later. No logical changes in general, maybe v3 is cleaner than v2.

0001 ....fix error code in memcg-hugetlb
0002 ....add res_counter_uncharge_until
0003 ....use res_counter_uncharge_until in memcg
0004 ....move charges to root is use_hierarchy==0
0005 ....cleanup for mem_cgroup_move_account()
0006 ....remove warning of res_counter_uncharge_nofail (from Costa's slub accounting series).

Quoted 12 lines · KAMEZAWA Hiroyuki

Hi, here is v3 based on memcg-devel tree.
git://github.com/mstsxfx/memcg-devel.git

This patch series is for avoiding -ENOMEM at calling pre_destroy() which is called at rmdir(). After this patch, charges will be moved to root (if use_hierarchy==0) or parent (if use_hierarchy==1), and we'll not see -ENOMEM in rmdir() of cgroup.

v2 included some other patches than ones for handling -ENOMEM problem, but I divided it. I'd like to post others in different series, later. No logical changes in general, maybe v3 is cleaner than v2.

Quoted 6 lines

0001 ....fix error code in memcg-hugetlb
0002 ....add res_counter_uncharge_until
0003 ....use res_counter_uncharge_until in memcg
0004 ....move charges to root is use_hierarchy==0
0005 ....cleanup for mem_cgroup_move_account()
0006 ....remove warning of res_counter_uncharge_nofail (from Costa's slub accounting series).

KAME, how is this progressing? Is it stuck on anything?

Remaining 20% of work is based on a modification to cgroup layer

How do you think this patch ? (This patch is not tested yet...so may have troubles...) I think callers of pre_destory() is not so many...

==
From a28db946f91f3509d25779e8c5db249506cc4b07 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <[email protected]>
Date: Fri, 22 Jun 2012 08:38:38 +0900
Subject: [PATCH] cgroup: keep cgroup_mutex() while calling ->pre_destroy()

In past, memcg's pre_destroy() was verrry slow because of the possibility of page reclaiming in it. So, cgroup_mutex() was released before calling pre_destroy() callbacks. Now, it's enough fast. memcg just scans the list and move pages to other cgroup, no memory reclaim happens. Then, we can keep cgroup_mutex() there.

By holding looks, we can avoid following cases
1. new task is attached while rmdir().
2. new child cgroup is created while rmdir()
3. new task is attached to cgroup and removed from cgroup before checking css's count. So, ->destroy() will be called even if some trashes by the task remains

(3. is terrible case...even if I think it will not happen in real world..)

Hello, KAME.

Quoted 24 lines · Kamezawa Hiroyuki

Remaining 20% of work is based on a modification to cgroup layer

How do you think this patch ? (This patch is not tested yet...so may have troubles...) I think callers of pre_destory() is not so many...

==
From a28db946f91f3509d25779e8c5db249506cc4b07 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <[email protected]>
Date: Fri, 22 Jun 2012 08:38:38 +0900
Subject: [PATCH] cgroup: keep cgroup_mutex() while calling ->pre_destroy()

In past, memcg's pre_destroy() was verrry slow because of the possibility of page reclaiming in it. So, cgroup_mutex() was released before calling pre_destroy() callbacks. Now, it's enough fast. memcg just scans the list and move pages to other cgroup, no memory reclaim happens. Then, we can keep cgroup_mutex() there.

By holding looks, we can avoid following cases
1. new task is attached while rmdir().
2. new child cgroup is created while rmdir()
3. new task is attached to cgroup and removed from cgroup before checking css's count. So, ->destroy() will be called even if some trashes by the task remains

Quoted 1 line

(3. is terrible case...even if I think it will not happen in real world..)

Ooh, once memcg drops the __DEPRECATED_clear_css_refs, cgroup_rmdir() will mark the cgroup dead before start calling pre_destroy() and none of the above will happen.

Quoted 4 lines

Ooh, once memcg drops the __DEPRECATED_clear_css_refs, cgroup_rmdir() will mark the cgroup dead before start calling pre_destroy() and none of the above will happen.

Hm, threads which touches memcg should hold memcg's reference count rather than css.
Right ? IIUC, one of reason is a reference from kswapd etc...hm. I'll check it.