[PATCH] memcg: make threshold index in the right position

[PATCH] memcg: make threshold index in the right position

[Sha Zhengju]

From: Sha Zhengju <handai.szj@taobao.com>

Index current_threshold may point to threshold that just equal to
usage after __mem_cgroup_threshold is triggerd. But after registering
a new event, it will change (pointing to threshold just below usage).
So make it consistent here.

Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Signed-off-by: Sha Zhengju <handai.szj@taobao.com>
---
 mm/memcontrol.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 22d94f5..79f4a58 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -183,7 +183,7 @@ struct mem_cgroup_threshold {
 
 /* For threshold */
 struct mem_cgroup_threshold_ary {
-	/* An array index points to threshold just below usage. */
+	/* An array index points to threshold just below or equal to usage. */
 	int current_threshold;
 	/* Size of entries[] */
 	unsigned int size;
@@ -4319,14 +4319,15 @@ static int mem_cgroup_usage_register_event(struct cgroup *cgrp,
 	/* Find current threshold */
 	new->current_threshold = -1;
 	for (i = 0; i < size; i++) {
-		if (new->entries[i].threshold < usage) {
+		if (new->entries[i].threshold <= usage) {
 			/*
 			 * new->current_threshold will not be used until
 			 * rcu_assign_pointer(), so it's safe to increment
 			 * it here.
 			 */
 			++new->current_threshold;
-		}
+		} else
+			break;
 	}
 
 	/* Free old spare buffer and save old primary buffer as spare */
-- 
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] memcg: make threshold index in the right position

[Kirill A. Shutemov]

On Thu, Feb 02, 2012 at 05:45:19PM +0800, Sha Zhengju wrote:
> From: Sha Zhengju <handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
> 
> Index current_threshold may point to threshold that just equal to
> usage after __mem_cgroup_threshold is triggerd.

I don't see it. Could you describe conditions?

> But after registering
> a new event, it will change (pointing to threshold just below usage).
> So make it consistent here.
> 
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> Cc: Kirill A. Shutemov <kirill-oKw7cIdHH8eLwutG50LtGA@public.gmane.org>
> Signed-off-by: Sha Zhengju <handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
> ---
>  mm/memcontrol.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 22d94f5..79f4a58 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -183,7 +183,7 @@ struct mem_cgroup_threshold {
>  
>  /* For threshold */
>  struct mem_cgroup_threshold_ary {
> -	/* An array index points to threshold just below usage. */
> +	/* An array index points to threshold just below or equal to usage. */
>  	int current_threshold;
>  	/* Size of entries[] */
>  	unsigned int size;
> @@ -4319,14 +4319,15 @@ static int mem_cgroup_usage_register_event(struct cgroup *cgrp,
>  	/* Find current threshold */
>  	new->current_threshold = -1;
>  	for (i = 0; i < size; i++) {
> -		if (new->entries[i].threshold < usage) {
> +		if (new->entries[i].threshold <= usage) {
>  			/*
>  			 * new->current_threshold will not be used until
>  			 * rcu_assign_pointer(), so it's safe to increment
>  			 * it here.
>  			 */
>  			++new->current_threshold;
> -		}
> +		} else
> +			break;
>  	}
>  
>  	/* Free old spare buffer and save old primary buffer as spare */
> -- 
> 1.7.4.1
> 

-- 
 Kirill A. Shutemov

Re: [PATCH] memcg: make threshold index in the right position

[Sha Zhengju]

On 02/02/2012 06:14 PM, Kirill A. Shutemov wrote:
> On Thu, Feb 02, 2012 at 05:45:19PM +0800, Sha Zhengju wrote:
>> From: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
>>
>> Index current_threshold may point to threshold that just equal to
>> usage after __mem_cgroup_threshold is triggerd.
> I don't see it. Could you describe conditions?
>
It is because of the following code path in __mem_cgroup_threshold:
{
     ...
         i = t->current_threshold;

         for (; i >= 0 && unlikely(t->entries[i].threshold > usage); i--)
                 eventfd_signal(t->entries[i].eventfd, 1);
         i++;

         for (; i < t->size && unlikely(t->entries[i].threshold <= 
usage); i++)
                 eventfd_signal(t->entries[i].eventfd, 1);

         t->current_threshold = i - 1;
     ...
}

For example:
now:
     threshold array:  3  5  7  9   (usage = 6)
                                    ^
                                 index

next turn:
     threshold array:  3  5  7  9   (usage = 7)
                                        ^
                                     index

after registering a new event(threshold = 10):
     threshold array:  3  5  7  9  10 (usage = 7)
                                    ^
                                 index
>> But after registering
>> a new event, it will change (pointing to threshold just below usage).
>> So make it consistent here.
>>
>> Cc: KAMEZAWA Hiroyuki<kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
>> Cc: Kirill A. Shutemov<kirill-oKw7cIdHH8eLwutG50LtGA@public.gmane.org>
>> Signed-off-by: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
>> ---
>>   mm/memcontrol.c |    7 ++++---
>>   1 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 22d94f5..79f4a58 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -183,7 +183,7 @@ struct mem_cgroup_threshold {
>>
>>   /* For threshold */
>>   struct mem_cgroup_threshold_ary {
>> -	/* An array index points to threshold just below usage. */
>> +	/* An array index points to threshold just below or equal to usage. */
>>   	int current_threshold;
>>   	/* Size of entries[] */
>>   	unsigned int size;
>> @@ -4319,14 +4319,15 @@ static int mem_cgroup_usage_register_event(struct cgroup *cgrp,
>>   	/* Find current threshold */
>>   	new->current_threshold = -1;
>>   	for (i = 0; i<  size; i++) {
>> -		if (new->entries[i].threshold<  usage) {
>> +		if (new->entries[i].threshold<= usage) {
>>   			/*
>>   			 * new->current_threshold will not be used until
>>   			 * rcu_assign_pointer(), so it's safe to increment
>>   			 * it here.
>>   			 */
>>   			++new->current_threshold;
>> -		}
>> +		} else
>> +			break;
>>   	}
>>
>>   	/* Free old spare buffer and save old primary buffer as spare */
>> -- 
>> 1.7.4.1
>>

Re: [PATCH] memcg: make threshold index in the right position

[Sha Zhengju]

On 02/02/2012 06:54 PM, Sha Zhengju wrote:
> On 02/02/2012 06:14 PM, Kirill A. Shutemov wrote:
>> On Thu, Feb 02, 2012 at 05:45:19PM +0800, Sha Zhengju wrote:
>>> From: Sha Zhengju<handai.szj@taobao.com>
>>>
>>> Index current_threshold may point to threshold that just equal to
>>> usage after __mem_cgroup_threshold is triggerd.
>> I don't see it. Could you describe conditions?
>>
> It is because of the following code path in __mem_cgroup_threshold:
> {
>     ...
>         i = t->current_threshold;
>
>         for (; i >= 0 && unlikely(t->entries[i].threshold > usage); i--)
>                 eventfd_signal(t->entries[i].eventfd, 1);
>         i++;
>
>         for (; i < t->size && unlikely(t->entries[i].threshold <= 
> usage); i++)
>                 eventfd_signal(t->entries[i].eventfd, 1);
>
>         t->current_threshold = i - 1;
>     ...
> }
>
> For example:
> now:
>     threshold array:  3  5  7  9   (usage = 6)
>                                    ^
>                                 index
>
> next turn:
>     threshold array:  3  5  7  9   (usage = 7)
>                                        ^
>                                     index
>
> after registering a new event(threshold = 10):
>     threshold array:  3  5  7  9  10 (usage = 7)
>                                    ^
>                                 index
Err.. Sorry for showing inaccurate index position... (may because of the 
mail format)

now:
     threshold array:  3  [5]  7  9   (usage = 6, index = 5)

next turn:
     threshold array:  3  5  [7]  9   (usage = 7, index = 7)

after registering a new event(threshold = 10):
     threshold array:  3  [5]  7  9  10 (usage = 7, index = 5)

>>> But after registering
>>> a new event, it will change (pointing to threshold just below usage).
>>> So make it consistent here.
>>>
>>> Cc: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>>> Cc: Kirill A. Shutemov<kirill@shutemov.name>
>>> Signed-off-by: Sha Zhengju<handai.szj@taobao.com>
>>> ---
>>>   mm/memcontrol.c |    7 ++++---
>>>   1 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>> index 22d94f5..79f4a58 100644
>>> --- a/mm/memcontrol.c
>>> +++ b/mm/memcontrol.c
>>> @@ -183,7 +183,7 @@ struct mem_cgroup_threshold {
>>>
>>>   /* For threshold */
>>>   struct mem_cgroup_threshold_ary {
>>> -    /* An array index points to threshold just below usage. */
>>> +    /* An array index points to threshold just below or equal to 
>>> usage. */
>>>       int current_threshold;
>>>       /* Size of entries[] */
>>>       unsigned int size;
>>> @@ -4319,14 +4319,15 @@ static int 
>>> mem_cgroup_usage_register_event(struct cgroup *cgrp,
>>>       /* Find current threshold */
>>>       new->current_threshold = -1;
>>>       for (i = 0; i<  size; i++) {
>>> -        if (new->entries[i].threshold<  usage) {
>>> +        if (new->entries[i].threshold<= usage) {
>>>               /*
>>>                * new->current_threshold will not be used until
>>>                * rcu_assign_pointer(), so it's safe to increment
>>>                * it here.
>>>                */
>>>               ++new->current_threshold;
>>> -        }
>>> +        } else
>>> +            break;
>>>       }
>>>
>>>       /* Free old spare buffer and save old primary buffer as spare */
>>> -- 
>>> 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] memcg: make threshold index in the right position

[Kirill A. Shutemov]

On Thu, Feb 02, 2012 at 07:08:44PM +0800, Sha Zhengju wrote:
> On 02/02/2012 06:54 PM, Sha Zhengju wrote:
> >On 02/02/2012 06:14 PM, Kirill A. Shutemov wrote:
> >>On Thu, Feb 02, 2012 at 05:45:19PM +0800, Sha Zhengju wrote:
> >>>From: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
> >>>
> >>>Index current_threshold may point to threshold that just equal to
> >>>usage after __mem_cgroup_threshold is triggerd.
> >>I don't see it. Could you describe conditions?
> >>
> >It is because of the following code path in __mem_cgroup_threshold:
> >{
> >    ...
> >        i = t->current_threshold;
> >
> >        for (; i >= 0 && unlikely(t->entries[i].threshold > usage); i--)
> >                eventfd_signal(t->entries[i].eventfd, 1);
> >        i++;
> >
> >        for (; i < t->size && unlikely(t->entries[i].threshold <=
> >usage); i++)
> >                eventfd_signal(t->entries[i].eventfd, 1);
> >
> >        t->current_threshold = i - 1;
> >    ...
> >}
> >
> >For example:
> >now:
> >    threshold array:  3  5  7  9   (usage = 6)
> >                                   ^
> >                                index
> >
> >next turn:
> >    threshold array:  3  5  7  9   (usage = 7)
> >                                       ^
> >                                    index
> >
> >after registering a new event(threshold = 10):
> >    threshold array:  3  5  7  9  10 (usage = 7)
> >                                   ^
> >                                index
> Err.. Sorry for showing inaccurate index position... (may because of
> the mail format)
> 
> now:
>     threshold array:  3  [5]  7  9   (usage = 6, index = 5)
> 
> next turn:
>     threshold array:  3  5  [7]  9   (usage = 7, index = 7)
> 
> after registering a new event(threshold = 10):
>     threshold array:  3  [5]  7  9  10 (usage = 7, index = 5)

Good catch! Thank you.

Reviewed-by: Kirill A. Shutemov <kirill-oKw7cIdHH8eLwutG50LtGA@public.gmane.org>

-- 
 Kirill A. Shutemov

Re: [PATCH] memcg: make threshold index in the right position

[KAMEZAWA Hiroyuki]

On Thu,  2 Feb 2012 17:45:19 +0800
Sha Zhengju <handai.szj-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> From: Sha Zhengju <handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
> 
> Index current_threshold may point to threshold that just equal to
> usage after __mem_cgroup_threshold is triggerd. But after registering
> a new event, it will change (pointing to threshold just below usage).
> So make it consistent here.
> 
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> Cc: Kirill A. Shutemov <kirill-oKw7cIdHH8eLwutG50LtGA@public.gmane.org>
> Signed-off-by: Sha Zhengju <handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>

Could you add the explanation you did to Kirill into the patch description ?

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
Thanks,
-Kame


> ---
>  mm/memcontrol.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 22d94f5..79f4a58 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -183,7 +183,7 @@ struct mem_cgroup_threshold {
>  
>  /* For threshold */
>  struct mem_cgroup_threshold_ary {
> -	/* An array index points to threshold just below usage. */
> +	/* An array index points to threshold just below or equal to usage. */
>  	int current_threshold;
>  	/* Size of entries[] */
>  	unsigned int size;
> @@ -4319,14 +4319,15 @@ static int mem_cgroup_usage_register_event(struct cgroup *cgrp,
>  	/* Find current threshold */
>  	new->current_threshold = -1;
>  	for (i = 0; i < size; i++) {
> -		if (new->entries[i].threshold < usage) {
> +		if (new->entries[i].threshold <= usage) {
>  			/*
>  			 * new->current_threshold will not be used until
>  			 * rcu_assign_pointer(), so it's safe to increment
>  			 * it here.
>  			 */
>  			++new->current_threshold;
> -		}
> +		} else
> +			break;
>  	}
>  
>  	/* Free old spare buffer and save old primary buffer as spare */
> -- 
> 1.7.4.1
> 
> 

[PATCH] memcg: make threshold index in the right position

[Sha Zhengju]

From: Sha Zhengju <handai.szj@taobao.com>

Index current_threshold may point to threshold that just equal to
usage after last call of __mem_cgroup_threshold. But after registering
a new event, it will change (pointing to threshold just below usage).
So make it consistent here.

For example:
now:
	threshold array:  3  [5]  7  9   (usage = 6, [index] = 5)

next turn (after calling __mem_cgroup_threshold):
	threshold array:  3   5  [7]  9   (usage = 7, [index] = 7)

after registering a new event (threshold = 10):
	threshold array:  3  [5]  7  9  10 (usage = 7, [index] = 5)  

Signed-off-by: Sha Zhengju <handai.szj@taobao.com>
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Reviewed-by: Kirill A. Shutemov <kirill@shutemov.name>

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

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 22d94f5..95fc9f07 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -183,7 +183,7 @@ struct mem_cgroup_threshold {
 
 /* For threshold */
 struct mem_cgroup_threshold_ary {
-	/* An array index points to threshold just below usage. */
+	/* An array index points to threshold just below or equal to usage. */
 	int current_threshold;
 	/* Size of entries[] */
 	unsigned int size;
@@ -4193,7 +4193,7 @@ static void __mem_cgroup_threshold(struct mem_cgroup *memcg, bool swap)
 	usage = mem_cgroup_usage(memcg, swap);
 
 	/*
-	 * current_threshold points to threshold just below usage.
+	 * current_threshold points to threshold just below or equal to usage.
 	 * If it's not true, a threshold was crossed after last
 	 * call of __mem_cgroup_threshold().
 	 */
@@ -4319,14 +4319,15 @@ static int mem_cgroup_usage_register_event(struct cgroup *cgrp,
 	/* Find current threshold */
 	new->current_threshold = -1;
 	for (i = 0; i < size; i++) {
-		if (new->entries[i].threshold < usage) {
+		if (new->entries[i].threshold <= usage) {
 			/*
 			 * new->current_threshold will not be used until
 			 * rcu_assign_pointer(), so it's safe to increment
 			 * it here.
 			 */
 			++new->current_threshold;
-		}
+		} else
+			break;
 	}
 
 	/* Free old spare buffer and save old primary buffer as spare */
-- 
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] memcg: make threshold index in the right position

[Sha Zhengju]

From: Sha Zhengju <handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>

Index current_threshold may point to threshold that just equal to
usage after __mem_cgroup_threshold is triggerd. But after registering
a new event, it will change (pointing to threshold just below usage). 
So make it consistent here.

Cc: Kirill A. Shutemov <kirill-oKw7cIdHH8eLwutG50LtGA@public.gmane.org>
Signed-off-by: Sha Zhengju <handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
---
 mm/memcontrol.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 22d94f5..ba46a01 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -183,7 +183,7 @@ struct mem_cgroup_threshold {
 
 /* For threshold */
 struct mem_cgroup_threshold_ary {
-	/* An array index points to threshold just below usage. */
+	/* An array index points to threshold just below or equal to usage. */
 	int current_threshold;
 	/* Size of entries[] */
 	unsigned int size;
@@ -4319,7 +4319,7 @@ static int mem_cgroup_usage_register_event(struct cgroup *cgrp,
 	/* Find current threshold */
 	new->current_threshold = -1;
 	for (i = 0; i < size; i++) {
-		if (new->entries[i].threshold < usage) {
+		if (new->entries[i].threshold <= usage) {
 			/*
 			 * new->current_threshold will not be used until
 			 * rcu_assign_pointer(), so it's safe to increment
-- 
1.7.4.1