[PATCH v3][0/6] memcg: prevent -ENOMEM in pre_destroy()

[PATCH v3][0/6] memcg: prevent -ENOMEM in pre_destroy()

[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.

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

Regards,
-Kame

[PATCH v3 1/6] memcg: fix error code in hugetlb_force_memcg_empty()

[KAMEZAWA Hiroyuki]

The conditions are handled as -EBUSY, _now_.

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 mm/hugetlb.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 1d3c8ea9..824f07b 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1922,8 +1922,11 @@ int hugetlb_force_memcg_empty(struct cgroup *cgroup)
 	int ret = 0, idx = 0;
 
 	do {
-		if (cgroup_task_count(cgroup) || !list_empty(&cgroup->children))
+		if (cgroup_task_count(cgroup)
+			|| !list_empty(&cgroup->children)) {
+			ret = -EBUSY;
 			goto out;
+		}
 		/*
 		 * If the task doing the cgroup_rmdir got a signal
 		 * we don't really need to loop till the hugetlb resource
-- 
1.7.4.1


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v3 1/6] memcg: fix error code in hugetlb_force_memcg_empty()

[Andrew Morton]

On Fri, 11 May 2012 18:45:18 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:

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

Re: [PATCH v3 1/6] memcg: fix error code in hugetlb_force_memcg_empty()

[KAMEZAWA Hiroyuki]

(2012/05/12 6:17), Andrew Morton wrote:

> On Fri, 11 May 2012 18:45:18 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:
> 
>> 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.
> 


Ah, sorry. How about this ?



The force_empty interface allows to make the memcg only when the cgroup
doesn't include any tasks.

	# echo 0 > /cgroup/xxxx/memory.force_empty

If cgroup isn't empty, force_empty does nothing and retruns -EBUSY in usual
memcg, memcontrol.c. But hugetlb implementation has inconsitency with
it and returns 0 and do nothing. Fix it to return -EBUSY.

Thanks,
-Kame

Re: [PATCH v3 1/6] memcg: fix error code in hugetlb_force_memcg_empty()

[Tejun Heo]

On Fri, May 11, 2012 at 06:45:18PM +0900, KAMEZAWA Hiroyuki wrote:
> -		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.

Thanks.

-- 
tejun

Re: [PATCH v3 1/6] memcg: fix error code in hugetlb_force_memcg_empty()

[Tejun Heo]

On Mon, May 14, 2012 at 11:15:56AM -0700, Tejun Heo wrote:
> On Fri, May 11, 2012 at 06:45:18PM +0900, KAMEZAWA Hiroyuki wrote:
> > -		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)) {

Thanks.

-- 
tejun

Re: [PATCH v3 1/6] memcg: fix error code in hugetlb_force_memcg_empty()

[KAMEZAWA Hiroyuki]

(2012/05/15 3:32), Tejun Heo wrote:

> On Mon, May 14, 2012 at 11:15:56AM -0700, Tejun Heo wrote:
>> On Fri, May 11, 2012 at 06:45:18PM +0900, KAMEZAWA Hiroyuki wrote:
>>> -		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)) {
> 


How about this ?
==
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
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 <aneesh.kumar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
---
 mm/hugetlb.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 1d3c8ea9..82ec623 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1922,8 +1922,11 @@ int hugetlb_force_memcg_empty(struct cgroup *cgroup)
 	int ret = 0, idx = 0;
 
 	do {
-		if (cgroup_task_count(cgroup) || !list_empty(&cgroup->children))
+		if (cgroup_task_count(cgroup) ||
+		    !list_empty(&cgroup->children)){
+			ret = -EBUSY;
 			goto out;
+		}
 		/*
 		 * If the task doing the cgroup_rmdir got a signal
 		 * we don't really need to loop till the hugetlb resource
-- 
1.7.4.1

Re: [PATCH v3 1/6] memcg: fix error code in hugetlb_force_memcg_empty()

[Tejun Heo]

On Tue, May 15, 2012 at 10:10:34AM +0900, KAMEZAWA Hiroyuki wrote:
> (2012/05/15 3:32), Tejun Heo wrote:
> 
> > On Mon, May 14, 2012 at 11:15:56AM -0700, Tejun Heo wrote:
> >> On Fri, May 11, 2012 at 06:45:18PM +0900, KAMEZAWA Hiroyuki wrote:
> >>> -		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)) {
> > 
> 
> 
> How about this ?
> ==
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 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 <aneesh.kumar@linux.vnet.ibm.com>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Heh, it was a nitpick anyway.  Please feel free to add my reviewed-by
for the whole series.

Thank you!

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/6] add res_counter_uncharge_until()

[KAMEZAWA Hiroyuki]

From: Frederic Weisbecker <fweisbec@gmail.com>

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 <kamezawa.hiroyu@jp.fujitsu.com>
---
 Documentation/cgroups/resource_counter.txt |    8 ++++++++
 include/linux/res_counter.h                |    3 +++
 kernel/res_counter.c                       |   10 ++++++++--
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/Documentation/cgroups/resource_counter.txt b/Documentation/cgroups/resource_counter.txt
index 95b24d7..703103a 100644
--- a/Documentation/cgroups/resource_counter.txt
+++ b/Documentation/cgroups/resource_counter.txt
@@ -92,6 +92,14 @@ to work with it.
 
 	The _locked routines imply that the res_counter->lock is taken.
 
+ f. void res_counter_uncharge_until
+		(struct res_counter *rc, struct res_counter *top,
+		 unsinged long val)
+
+	Almost same as res_cunter_uncharge() but propagation of uncharge
+	stops when rc == top. This is useful when kill a res_coutner in
+	child cgroup.
+
  2.1 Other accounting routines
 
     There are more routines that may help you with common needs, like
diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
index da81af0..d11c1cd 100644
--- a/include/linux/res_counter.h
+++ b/include/linux/res_counter.h
@@ -135,6 +135,9 @@ int __must_check res_counter_charge_nofail(struct res_counter *counter,
 void res_counter_uncharge_locked(struct res_counter *counter, unsigned long val);
 void res_counter_uncharge(struct res_counter *counter, unsigned long val);
 
+void res_counter_uncharge_until(struct res_counter *counter,
+				struct res_counter *top,
+				unsigned long val);
 /**
  * res_counter_margin - calculate chargeable space of a counter
  * @cnt: the counter
diff --git a/kernel/res_counter.c b/kernel/res_counter.c
index d508363..d9ea45e 100644
--- a/kernel/res_counter.c
+++ b/kernel/res_counter.c
@@ -99,13 +99,15 @@ void res_counter_uncharge_locked(struct res_counter *counter, unsigned long val)
 	counter->usage -= val;
 }
 
-void res_counter_uncharge(struct res_counter *counter, unsigned long val)
+void res_counter_uncharge_until(struct res_counter *counter,
+				struct res_counter *top,
+				unsigned long val)
 {
 	unsigned long flags;
 	struct res_counter *c;
 
 	local_irq_save(flags);
-	for (c = counter; c != NULL; c = c->parent) {
+	for (c = counter; c != top; c = c->parent) {
 		spin_lock(&c->lock);
 		res_counter_uncharge_locked(c, val);
 		spin_unlock(&c->lock);
@@ -113,6 +115,10 @@ void res_counter_uncharge(struct res_counter *counter, unsigned long val)
 	local_irq_restore(flags);
 }
 
+void res_counter_uncharge(struct res_counter *counter, unsigned long val)
+{
+	res_counter_uncharge_until(counter, NULL, val);
+}
 
 static inline unsigned long long *
 res_counter_member(struct res_counter *counter, int member)
-- 
1.7.4.1


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Andrew Morton]

On Fri, 11 May 2012 18:47:06 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:

> From: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> 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 <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>

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

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

[KAMEZAWA Hiroyuki]

(2012/05/12 6:19), Andrew Morton wrote:

> On Fri, 11 May 2012 18:47:06 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:
> 
>> From: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> 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 <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> 
> 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.

Thanks,
-Kame

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

[Frederic Weisbecker]

2012/5/14 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>:
> (2012/05/12 6:19), Andrew Morton wrote:
>
>> On Fri, 11 May 2012 18:47:06 +0900
>> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>>
>>> From: Frederic Weisbecker <fweisbec@gmail.com>
>>>
>>> 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 <kamezawa.hiroyu@jp.fujitsu.com>
>>
>> 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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[KAMEZAWA Hiroyuki]

(2012/05/14 19:08), Frederic Weisbecker wrote:

> 2012/5/14 KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>:
>> (2012/05/12 6:19), Andrew Morton wrote:
>>
>>> On Fri, 11 May 2012 18:47:06 +0900
>>> KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:
>>>
>>>> From: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>
>>>> 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 <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
>>>
>>> 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.
> 

Oh, sorry. 

> 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 <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

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 <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
---
 Documentation/cgroups/resource_counter.txt |    8 ++++++++
 include/linux/res_counter.h                |    3 +++
 kernel/res_counter.c                       |   10 ++++++++--
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/Documentation/cgroups/resource_counter.txt b/Documentation/cgroups/resource_counter.txt
index 95b24d7..703103a 100644
--- a/Documentation/cgroups/resource_counter.txt
+++ b/Documentation/cgroups/resource_counter.txt
@@ -92,6 +92,14 @@ to work with it.
 
 	The _locked routines imply that the res_counter->lock is taken.
 
+ f. void res_counter_uncharge_until
+		(struct res_counter *rc, struct res_counter *top,
+		 unsinged long val)
+
+	Almost same as res_cunter_uncharge() but propagation of uncharge
+	stops when rc == top. This is useful when kill a res_coutner in
+	child cgroup.
+
  2.1 Other accounting routines
 
     There are more routines that may help you with common needs, like
diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
index da81af0..d11c1cd 100644
--- a/include/linux/res_counter.h
+++ b/include/linux/res_counter.h
@@ -135,6 +135,9 @@ int __must_check res_counter_charge_nofail(struct res_counter *counter,
 void res_counter_uncharge_locked(struct res_counter *counter, unsigned long val);
 void res_counter_uncharge(struct res_counter *counter, unsigned long val);
 
+void res_counter_uncharge_until(struct res_counter *counter,
+				struct res_counter *top,
+				unsigned long val);
 /**
  * res_counter_margin - calculate chargeable space of a counter
  * @cnt: the counter
diff --git a/kernel/res_counter.c b/kernel/res_counter.c
index d508363..d9ea45e 100644
--- a/kernel/res_counter.c
+++ b/kernel/res_counter.c
@@ -99,13 +99,15 @@ void res_counter_uncharge_locked(struct res_counter *counter, unsigned long val)
 	counter->usage -= val;
 }
 
-void res_counter_uncharge(struct res_counter *counter, unsigned long val)
+void res_counter_uncharge_until(struct res_counter *counter,
+				struct res_counter *top,
+				unsigned long val)
 {
 	unsigned long flags;
 	struct res_counter *c;
 
 	local_irq_save(flags);
-	for (c = counter; c != NULL; c = c->parent) {
+	for (c = counter; c != top; c = c->parent) {
 		spin_lock(&c->lock);
 		res_counter_uncharge_locked(c, val);
 		spin_unlock(&c->lock);
@@ -113,6 +115,10 @@ void res_counter_uncharge(struct res_counter *counter, unsigned long val)
 	local_irq_restore(flags);
 }
 
+void res_counter_uncharge(struct res_counter *counter, unsigned long val)
+{
+	res_counter_uncharge_until(counter, NULL, val);
+}
 
 static inline unsigned long long *
 res_counter_member(struct res_counter *counter, int member)
-- 
1.7.4.1

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

[Frederic Weisbecker]

2012/5/14 KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>:
>> 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 ?

Yep, or even better since I plan to use my company's address now to
sign my patches:

Signed-off-by: Frederic Weisbecker <fweisbec-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Thanks!

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

[Tejun Heo]

On Mon, May 14, 2012 at 07:32:42PM +0900, KAMEZAWA Hiroyuki wrote:
> [PATCH 2/6] memcg: add res_counter_uncharge_until()
> 
> From: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> 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 <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>

Acked-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Thanks.

-- 
tejun

[PATCH v3 3/6] memcg: use res_counter_uncharge_until in move_parent()

[KAMEZAWA Hiroyuki]

By using res_counter_uncharge_until(), we can avoid race and
unnecessary charging.

Changelog since v2:
 - a coding style chanve in __mem_cgroup_cancel_local_charge()
 - fixed typos.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 mm/memcontrol.c |   63 ++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 42 insertions(+), 21 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 09109be..cb90be1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2446,6 +2446,24 @@ static void __mem_cgroup_cancel_charge(struct mem_cgroup *memcg,
 }
 
 /*
+ * Cancel chrages in this cgroup....doesn't propagate to parent cgroup.
+ * This is useful when moving usage to parent cgroup.
+ */
+static void __mem_cgroup_cancel_local_charge(struct mem_cgroup *memcg,
+					unsigned int nr_pages)
+{
+	unsigned long bytes = nr_pages * PAGE_SIZE;
+
+	if (mem_cgroup_is_root(memcg))
+		return;
+
+	res_counter_uncharge_until(&memcg->res, memcg->res.parent, bytes);
+	if (do_swap_account)
+		res_counter_uncharge_until(&memcg->memsw,
+						memcg->memsw.parent, bytes);
+}
+
+/*
  * A helper function to get mem_cgroup from ID. must be called under
  * rcu_read_lock(). The caller must check css_is_removed() or some if
  * it's concern. (dropping refcnt from swap can be called against removed
@@ -2711,16 +2729,28 @@ static int mem_cgroup_move_parent(struct page *page,
 	nr_pages = hpage_nr_pages(page);
 
 	parent = mem_cgroup_from_cont(pcg);
-	ret = __mem_cgroup_try_charge(NULL, gfp_mask, nr_pages, &parent, false);
-	if (ret)
-		goto put_back;
+	if (!parent->use_hierarchy) {
+		ret = __mem_cgroup_try_charge(NULL,
+					gfp_mask, nr_pages, &parent, false);
+		if (ret)
+			goto put_back;
+	}
 
 	if (nr_pages > 1)
 		flags = compound_lock_irqsave(page);
 
-	ret = mem_cgroup_move_account(page, nr_pages, pc, child, parent, true);
-	if (ret)
-		__mem_cgroup_cancel_charge(parent, nr_pages);
+	if (parent->use_hierarchy) {
+		ret = mem_cgroup_move_account(page, nr_pages,
+					pc, child, parent, false);
+		if (!ret)
+			__mem_cgroup_cancel_local_charge(child, nr_pages);
+	} else {
+		ret = mem_cgroup_move_account(page, nr_pages,
+					pc, child, parent, true);
+
+		if (ret)
+			__mem_cgroup_cancel_charge(parent, nr_pages);
+	}
 
 	if (nr_pages > 1)
 		compound_unlock_irqrestore(page, flags);
@@ -3324,6 +3354,7 @@ int mem_cgroup_move_hugetlb_parent(int idx, struct cgroup *cgroup,
 	struct cgroup *pcgrp = cgroup->parent;
 	struct mem_cgroup *parent = mem_cgroup_from_cont(pcgrp);
 	struct mem_cgroup *memcg  = mem_cgroup_from_cont(cgroup);
+	struct res_counter *counter;
 
 	if (!get_page_unless_zero(page))
 		goto out;
@@ -3334,28 +3365,18 @@ int mem_cgroup_move_hugetlb_parent(int idx, struct cgroup *cgroup,
 		goto err_out;
 
 	csize = PAGE_SIZE << compound_order(page);
-	/*
-	 * If we have use_hierarchy set we can never fail here. So instead of
-	 * using res_counter_uncharge use the open-coded variant which just
-	 * uncharge the child res_counter. The parent will retain the charge.
-	 */
-	if (parent->use_hierarchy) {
-		unsigned long flags;
-		struct res_counter *counter;
-
-		counter = &memcg->hugepage[idx];
-		spin_lock_irqsave(&counter->lock, flags);
-		res_counter_uncharge_locked(counter, csize);
-		spin_unlock_irqrestore(&counter->lock, flags);
-	} else {
+	/* 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;
 			goto err_out;
 		}
-		res_counter_uncharge(&memcg->hugepage[idx], csize);
 	}
+	counter = &memcg->hugepage[idx];
+	res_counter_uncharge_until(counter, counter->parent, csize);
+
 	pc->mem_cgroup = parent;
 err_out:
 	unlock_page_cgroup(pc);
-- 
1.7.4.1


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v3 4/6] memcg: move charges to root cgroup if use_hierarchy=0.

[KAMEZAWA Hiroyuki]

Now, at removal of cgroup, ->pre_destroy() is called and move charges
to the parent cgroup. A major reason of -EBUSY returned by ->pre_destroy()
is that the 'moving' hits parent's resource limitation. It happens only
when use_hierarchy=0.

Considering use_hierarchy=0, all cgroups should be flat. So, no one
cannot justify moving charges to parent...parent and children are in
flat configuration, not hierarchical.

This patch modifes to move charges to root cgroup at rmdir/force_empty
if use_hierarchy==0. This will much simplify rmdir() and reduce error
in ->pre_destroy.

Changelog since v2:
  - use parent_mem_cgroup()
  - updated Documenation

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
---
 Documentation/cgroups/memory.txt |   13 ++++++----
 mm/memcontrol.c                  |   49 +++++++++++++------------------------
 2 files changed, 25 insertions(+), 37 deletions(-)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index 730e222a..8d0de70 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -393,14 +393,15 @@ cgroup might have some charge associated with it, even though all
 tasks have migrated away from it. (because we charge against pages, not
 against tasks.)
 
-Such charges are freed or moved to their parent. At moving, both of RSS
-and CACHES are moved to parent.
-rmdir() may return -EBUSY if freeing/moving fails. See 5.1 also.
+We move the stats to root (if use_hierarchy==0) or parent (if
+use_hierarchy==1), and no change on the charge except uncharging
+from the child.
 
 Charges recorded in swap information is not updated at removal of cgroup.
 Recorded information is discarded and a cgroup which uses swap (swapcache)
 will be charged as a new owner of it.
 
+About use_hierarchy, see Section 6.
 
 5. Misc. interfaces.
 
@@ -413,13 +414,15 @@ will be charged as a new owner of it.
 
   Almost all pages tracked by this memory cgroup will be unmapped and freed.
   Some pages cannot be freed because they are locked or in-use. Such pages are
-  moved to parent and this cgroup will be empty. This may return -EBUSY if
-  VM is too busy to free/move all pages immediately.
+  moved to parent(if use_hierarchy==1) or root (if use_hierarchy==0) and this
+  cgroup will be empty.
 
   Typical use case of this interface is that calling this before rmdir().
   Because rmdir() moves all pages to parent, some out-of-use page caches can be
   moved to the parent. If you want to avoid that, force_empty will be useful.
 
+  About use_hierarchy, see Section 6.
+
 5.2 stat file
 
 memory.stat file includes following statistics
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index cb90be1..f007c17 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2709,15 +2709,13 @@ static int mem_cgroup_move_parent(struct page *page,
 				  struct mem_cgroup *child,
 				  gfp_t gfp_mask)
 {
-	struct cgroup *cg = child->css.cgroup;
-	struct cgroup *pcg = cg->parent;
 	struct mem_cgroup *parent;
 	unsigned int nr_pages;
 	unsigned long uninitialized_var(flags);
 	int ret;
 
 	/* Is ROOT ? */
-	if (!pcg)
+	if (mem_cgroup_is_root(child))
 		return -EINVAL;
 
 	ret = -EBUSY;
@@ -2728,33 +2726,23 @@ static int mem_cgroup_move_parent(struct page *page,
 
 	nr_pages = hpage_nr_pages(page);
 
-	parent = mem_cgroup_from_cont(pcg);
-	if (!parent->use_hierarchy) {
-		ret = __mem_cgroup_try_charge(NULL,
-					gfp_mask, nr_pages, &parent, false);
-		if (ret)
-			goto put_back;
-	}
+	parent = parent_mem_cgroup(child);
+	/*
+	 * If no parent, move charges to root cgroup.
+	 */
+	if (!parent)
+		parent = root_mem_cgroup;
 
 	if (nr_pages > 1)
 		flags = compound_lock_irqsave(page);
 
-	if (parent->use_hierarchy) {
-		ret = mem_cgroup_move_account(page, nr_pages,
-					pc, child, parent, false);
-		if (!ret)
-			__mem_cgroup_cancel_local_charge(child, nr_pages);
-	} else {
-		ret = mem_cgroup_move_account(page, nr_pages,
-					pc, child, parent, true);
-
-		if (ret)
-			__mem_cgroup_cancel_charge(parent, nr_pages);
-	}
+	ret = mem_cgroup_move_account(page, nr_pages,
+				pc, child, parent, false);
+	if (!ret)
+		__mem_cgroup_cancel_local_charge(child, nr_pages);
 
 	if (nr_pages > 1)
 		compound_unlock_irqrestore(page, flags);
-put_back:
 	putback_lru_page(page);
 put:
 	put_page(page);
@@ -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;
-			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);
-- 
1.7.4.1

Re: [PATCH v3 4/6] memcg: move charges to root cgroup if use_hierarchy=0.

[Tejun Heo]

On Fri, May 11, 2012 at 06:49:22PM +0900, KAMEZAWA Hiroyuki wrote:
> @@ -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;
> -			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.

Thanks.

-- 
tejun

Re: [PATCH v3 4/6] memcg: move charges to root cgroup if use_hierarchy=0.

[KAMEZAWA Hiroyuki]

(2012/05/15 5:14), Tejun Heo wrote:

> On Fri, May 11, 2012 at 06:49:22PM +0900, KAMEZAWA Hiroyuki wrote:
>> @@ -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;
>> -			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.
> 
Sure, I'll check it.


Thanks,
-Kame

[PATCH v3 5/6] memcg: don't uncharge in mem_cgroup_move_account

[KAMEZAWA Hiroyuki]

Now, all caller passes 'false' for 'bool uncharge', remove the argument.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
---
 mm/memcontrol.c |   20 ++++++--------------
 1 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f007c17..fcb0095 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2615,23 +2615,19 @@ void mem_cgroup_split_huge_fixup(struct page *head)
  * @pc:	page_cgroup of the page.
  * @from: mem_cgroup which the page is moved from.
  * @to:	mem_cgroup which the page is moved to. @from != @to.
- * @uncharge: whether we should call uncharge and css_put against @from.
  *
  * The caller must confirm following.
  * - page is not on LRU (isolate_page() is useful.)
  * - compound_lock is held when nr_pages > 1
  *
- * This function doesn't do "charge" nor css_get to new cgroup. It should be
- * done by a caller(__mem_cgroup_try_charge would be useful). If @uncharge is
- * true, this function does "uncharge" from old cgroup, but it doesn't if
- * @uncharge is false, so a caller should do "uncharge".
+ * This function doesn't do "charge" to new cgroup and doesn't do "uncharge"
+ * from old cgroup.
  */
 static int mem_cgroup_move_account(struct page *page,
 				   unsigned int nr_pages,
 				   struct page_cgroup *pc,
 				   struct mem_cgroup *from,
-				   struct mem_cgroup *to,
-				   bool uncharge)
+				   struct mem_cgroup *to)
 {
 	unsigned long flags;
 	int ret;
@@ -2673,9 +2669,6 @@ static int mem_cgroup_move_account(struct page *page,
 		preempt_enable();
 	}
 	mem_cgroup_charge_statistics(from, anon, -nr_pages);
-	if (uncharge)
-		/* This is not "cancel", but cancel_charge does all we need. */
-		__mem_cgroup_cancel_charge(from, nr_pages);
 
 	/* caller should have done css_get */
 	pc->mem_cgroup = to;
@@ -2737,7 +2730,7 @@ static int mem_cgroup_move_parent(struct page *page,
 		flags = compound_lock_irqsave(page);
 
 	ret = mem_cgroup_move_account(page, nr_pages,
-				pc, child, parent, false);
+				pc, child, parent);
 	if (!ret)
 		__mem_cgroup_cancel_local_charge(child, nr_pages);
 
@@ -5757,8 +5750,7 @@ static int mem_cgroup_move_charge_pte_range(pmd_t *pmd,
 			if (!isolate_lru_page(page)) {
 				pc = lookup_page_cgroup(page);
 				if (!mem_cgroup_move_account(page, HPAGE_PMD_NR,
-							     pc, mc.from, mc.to,
-							     false)) {
+							pc, mc.from, mc.to)) {
 					mc.precharge -= HPAGE_PMD_NR;
 					mc.moved_charge += HPAGE_PMD_NR;
 				}
@@ -5788,7 +5780,7 @@ retry:
 				goto put;
 			pc = lookup_page_cgroup(page);
 			if (!mem_cgroup_move_account(page, 1, pc,
-						     mc.from, mc.to, false)) {
+						     mc.from, mc.to)) {
 				mc.precharge--;
 				/* we uncharge from mc.from later. */
 				mc.moved_charge++;
-- 
1.7.4.1

[PATCH v3 6/6] remove __must_check for res_counter_charge_nofail()

[KAMEZAWA Hiroyuki]

I picked this up from Costa's slub memcg series. For fixing added warning
by patch 4.
==
From: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
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 <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
---
 include/linux/res_counter.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
index d11c1cd..c6b0368 100644
--- a/include/linux/res_counter.h
+++ b/include/linux/res_counter.h
@@ -119,7 +119,7 @@ int __must_check res_counter_charge_locked(struct res_counter *counter,
 		unsigned long val);
 int __must_check res_counter_charge(struct res_counter *counter,
 		unsigned long val, struct res_counter **limit_fail_at);
-int __must_check res_counter_charge_nofail(struct res_counter *counter,
+int res_counter_charge_nofail(struct res_counter *counter,
 		unsigned long val, struct res_counter **limit_fail_at);
 
 /*
-- 
1.7.4.1

Re: [PATCH v3 6/6] remove __must_check for res_counter_charge_nofail()

[Tejun Heo]

On Fri, May 11, 2012 at 06:53:08PM +0900, KAMEZAWA Hiroyuki wrote:
> I picked this up from Costa's slub memcg series. For fixing added warning
> by patch 4.
> ==
> From: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> 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 <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>

For 3-6,

 Reviewed-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

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.

Thank you.

-- 
tejun

Re: [PATCH v3 6/6] remove __must_check for res_counter_charge_nofail()

[KAMEZAWA Hiroyuki]

(2012/05/15 5:09), Tejun Heo wrote:

> On Fri, May 11, 2012 at 06:53:08PM +0900, KAMEZAWA Hiroyuki wrote:
>> I picked this up from Costa's slub memcg series. For fixing added warning
>> by patch 4.
>> ==
>> From: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
>> 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 <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
>> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> 
> For 3-6,
> 
>  Reviewed-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> 
> 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.
> 


Yes, I'll do more work. I tend to split series, sorry.

Thanks,
-Kame

Re: [PATCH v3][0/6] memcg: prevent -ENOMEM in pre_destroy()

[Tejun Heo]

On Fri, May 11, 2012 at 06:41:36PM +0900, KAMEZAWA Hiroyuki wrote:
> 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).

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

Thank you very much.

-- 
tejun

Re: [PATCH v3][0/6] memcg: prevent -ENOMEM in pre_destroy()

[Kamezawa Hiroyuki]

(2012/06/22 5:20), Tejun Heo wrote:
> On Fri, May 11, 2012 at 06:41:36PM +0900, KAMEZAWA Hiroyuki wrote:
>> 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).
>
> KAME, how is this progressing?  Is it stuck on anything?
>

I think I finished 80% of works and patches are in -mm stack now.
They'll be visible in -next, soon.

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 <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
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..)

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
---
  kernel/cgroup.c |    3 +--
  1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index caff6a1..a5b6df1 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4171,7 +4171,6 @@ again:
  		mutex_unlock(&cgroup_mutex);
  		return -EBUSY;
  	}
-	mutex_unlock(&cgroup_mutex);
  
  	/*
  	 * In general, subsystem has no css->refcnt after pre_destroy(). But
@@ -4190,11 +4189,11 @@ again:
  	 */
  	ret = cgroup_call_pre_destroy(cgrp);
  	if (ret) {
+		mutex_unlock(&cgroup_mutex);
  		clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
  		return ret;
  	}
  
-	mutex_lock(&cgroup_mutex);
  	parent = cgrp->parent;
  	if (atomic_read(&cgrp->count) || !list_empty(&cgrp->children)) {
  		clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
-- 
1.7.4.1

Re: [PATCH v3][0/6] memcg: prevent -ENOMEM in pre_destroy()

[Tejun Heo]

Hello, KAME.

On Fri, Jun 22, 2012 at 08:27:25AM +0900, Kamezawa Hiroyuki wrote:
> 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 <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> 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..)

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.

Thanks.

-- 
tejun

Re: [PATCH v3][0/6] memcg: prevent -ENOMEM in pre_destroy()

[Kamezawa Hiroyuki]

(2012/06/28 2:58), Tejun Heo wrote:
> Hello, KAME.
>
> On Fri, Jun 22, 2012 at 08:27:25AM +0900, Kamezawa Hiroyuki wrote:
>> 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 <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
>> 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..)
>
> 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.

Thanks,
-Kame

Re: [PATCH v3][0/6] memcg: prevent -ENOMEM in pre_destroy()

[Tejun Heo]

Hello, KAME.

On Thu, Jun 28, 2012 at 1:33 AM, Kamezawa Hiroyuki
<kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:
>> 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.

Not sure I'm following. I meant that css_tryget() will always fail
once pre_destroy() calls being for the cgroup, so no new child or
reference can be created for it after that point.

Thanks.

-- 
tejun