[PATCH] memcg: Free spare array to avoid memory leak

[PATCH] memcg: Free spare array to avoid memory leak

[Sha Zhengju]

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

When the last event is unregistered, there is no need to keep the spare
array anymore. So free it to avoid memory leak.

Signed-off-by: Sha Zhengju <handai.szj@taobao.com>

---
 mm/memcontrol.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 22d94f5..3c09a84 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
 swap_buffers:
 	/* Swap primary and spare array */
 	thresholds->spare = thresholds->primary;
+	/* If all events are unregistered, free the spare array */
+	if (!new) {
+		kfree(thresholds->spare);
+		thresholds->spare = NULL;
+	}
+
 	rcu_assign_pointer(thresholds->primary, new);
 
 	/* To be sure that nobody uses thresholds */
-- 
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: Free spare array to avoid memory leak

[Kirill A. Shutemov]

On Tue, Mar 06, 2012 at 08:13:24PM +0800, Sha Zhengju wrote:
> From: Sha Zhengju <handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
> 
> When the last event is unregistered, there is no need to keep the spare
> array anymore. So free it to avoid memory leak.

It's not a leak. It will be freed on next event register.

Yeah, we don't have to keep spare if primary is empty. But is it worth to
make code more complicated to save few bytes of memory?

> 
> Signed-off-by: Sha Zhengju <handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
> 
> ---
>  mm/memcontrol.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 22d94f5..3c09a84 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
>  swap_buffers:
>  	/* Swap primary and spare array */
>  	thresholds->spare = thresholds->primary;
> +	/* If all events are unregistered, free the spare array */
> +	if (!new) {
> +		kfree(thresholds->spare);
> +		thresholds->spare = NULL;
> +	}
> +
>  	rcu_assign_pointer(thresholds->primary, new);
>  
>  	/* To be sure that nobody uses thresholds */
> -- 
> 1.7.4.1
> 

-- 
 Kirill A. Shutemov

Re: [PATCH] memcg: Free spare array to avoid memory leak

[Kirill A. Shutemov]

On Tue, Mar 06, 2012 at 08:13:24PM +0800, Sha Zhengju wrote:
> From: Sha Zhengju <handai.szj@taobao.com>
> 
> When the last event is unregistered, there is no need to keep the spare
> array anymore. So free it to avoid memory leak.

It's not a leak. It will be freed on next event register.

Yeah, we don't have to keep spare if primary is empty. But is it worth to
make code more complicated to save few bytes of memory?

> 
> Signed-off-by: Sha Zhengju <handai.szj@taobao.com>
> 
> ---
>  mm/memcontrol.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 22d94f5..3c09a84 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
>  swap_buffers:
>  	/* Swap primary and spare array */
>  	thresholds->spare = thresholds->primary;
> +	/* If all events are unregistered, free the spare array */
> +	if (!new) {
> +		kfree(thresholds->spare);
> +		thresholds->spare = NULL;
> +	}
> +
>  	rcu_assign_pointer(thresholds->primary, new);
>  
>  	/* To be sure that nobody uses thresholds */
> -- 
> 1.7.4.1
> 

-- 
 Kirill A. Shutemov

--
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: Free spare array to avoid memory leak

[Sha Zhengju]

On 03/08/2012 07:08 AM, Kirill A. Shutemov wrote:
> On Tue, Mar 06, 2012 at 08:13:24PM +0800, Sha Zhengju wrote:
>> From: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
>>
>> When the last event is unregistered, there is no need to keep the spare
>> array anymore. So free it to avoid memory leak.
> It's not a leak. It will be freed on next event register.


Yeah, I noticed that. But what if it is just the last one and no more
event registering ?


Thanks,
Sha

> Yeah, we don't have to keep spare if primary is empty. But is it worth to
> make code more complicated to save few bytes of memory?
>
>> Signed-off-by: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
>>
>> ---
>>   mm/memcontrol.c |    6 ++++++
>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 22d94f5..3c09a84 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
>>   swap_buffers:
>>   	/* Swap primary and spare array */
>>   	thresholds->spare = thresholds->primary;
>> +	/* If all events are unregistered, free the spare array */
>> +	if (!new) {
>> +		kfree(thresholds->spare);
>> +		thresholds->spare = NULL;
>> +	}
>> +
>>   	rcu_assign_pointer(thresholds->primary, new);
>>
>>   	/* To be sure that nobody uses thresholds */
>> -- 
>> 1.7.4.1
>>

Re: [PATCH] memcg: Free spare array to avoid memory leak

[Sha Zhengju]

On 03/08/2012 07:08 AM, Kirill A. Shutemov wrote:
> On Tue, Mar 06, 2012 at 08:13:24PM +0800, Sha Zhengju wrote:
>> From: Sha Zhengju<handai.szj@taobao.com>
>>
>> When the last event is unregistered, there is no need to keep the spare
>> array anymore. So free it to avoid memory leak.
> It's not a leak. It will be freed on next event register.


Yeah, I noticed that. But what if it is just the last one and no more
event registering ?


Thanks,
Sha

> Yeah, we don't have to keep spare if primary is empty. But is it worth to
> make code more complicated to save few bytes of memory?
>
>> Signed-off-by: Sha Zhengju<handai.szj@taobao.com>
>>
>> ---
>>   mm/memcontrol.c |    6 ++++++
>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 22d94f5..3c09a84 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
>>   swap_buffers:
>>   	/* Swap primary and spare array */
>>   	thresholds->spare = thresholds->primary;
>> +	/* If all events are unregistered, free the spare array */
>> +	if (!new) {
>> +		kfree(thresholds->spare);
>> +		thresholds->spare = NULL;
>> +	}
>> +
>>   	rcu_assign_pointer(thresholds->primary, new);
>>
>>   	/* To be sure that nobody uses thresholds */
>> -- 
>> 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: Free spare array to avoid memory leak

[Kirill A. Shutemov]

On Thu, Mar 08, 2012 at 10:11:32AM +0800, Sha Zhengju wrote:
> On 03/08/2012 07:08 AM, Kirill A. Shutemov wrote:
> >On Tue, Mar 06, 2012 at 08:13:24PM +0800, Sha Zhengju wrote:
> >>From: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
> >>
> >>When the last event is unregistered, there is no need to keep the spare
> >>array anymore. So free it to avoid memory leak.
> >It's not a leak. It will be freed on next event register.
> 
> 
> Yeah, I noticed that. But what if it is just the last one and no more
> event registering ?

See my question below. ;)

> >Yeah, we don't have to keep spare if primary is empty. But is it worth to
> >make code more complicated to save few bytes of memory?
> >
> >>Signed-off-by: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
> >>
> >>---
> >>  mm/memcontrol.c |    6 ++++++
> >>  1 files changed, 6 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> >>index 22d94f5..3c09a84 100644
> >>--- a/mm/memcontrol.c
> >>+++ b/mm/memcontrol.c
> >>@@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
> >>  swap_buffers:
> >>  	/* Swap primary and spare array */
> >>  	thresholds->spare = thresholds->primary;
> >>+	/* If all events are unregistered, free the spare array */
> >>+	if (!new) {
> >>+		kfree(thresholds->spare);
> >>+		thresholds->spare = NULL;
> >>+	}
> >>+
> >>  	rcu_assign_pointer(thresholds->primary, new);
> >>
> >>  	/* To be sure that nobody uses thresholds */
> >>-- 
> >>1.7.4.1
> >>
> 

-- 
 Kirill A. Shutemov

Re: [PATCH] memcg: Free spare array to avoid memory leak

[Kirill A. Shutemov]

On Thu, Mar 08, 2012 at 10:11:32AM +0800, Sha Zhengju wrote:
> On 03/08/2012 07:08 AM, Kirill A. Shutemov wrote:
> >On Tue, Mar 06, 2012 at 08:13:24PM +0800, Sha Zhengju wrote:
> >>From: Sha Zhengju<handai.szj@taobao.com>
> >>
> >>When the last event is unregistered, there is no need to keep the spare
> >>array anymore. So free it to avoid memory leak.
> >It's not a leak. It will be freed on next event register.
> 
> 
> Yeah, I noticed that. But what if it is just the last one and no more
> event registering ?

See my question below. ;)

> >Yeah, we don't have to keep spare if primary is empty. But is it worth to
> >make code more complicated to save few bytes of memory?
> >
> >>Signed-off-by: Sha Zhengju<handai.szj@taobao.com>
> >>
> >>---
> >>  mm/memcontrol.c |    6 ++++++
> >>  1 files changed, 6 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> >>index 22d94f5..3c09a84 100644
> >>--- a/mm/memcontrol.c
> >>+++ b/mm/memcontrol.c
> >>@@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
> >>  swap_buffers:
> >>  	/* Swap primary and spare array */
> >>  	thresholds->spare = thresholds->primary;
> >>+	/* If all events are unregistered, free the spare array */
> >>+	if (!new) {
> >>+		kfree(thresholds->spare);
> >>+		thresholds->spare = NULL;
> >>+	}
> >>+
> >>  	rcu_assign_pointer(thresholds->primary, new);
> >>
> >>  	/* To be sure that nobody uses thresholds */
> >>-- 
> >>1.7.4.1
> >>
> 

-- 
 Kirill A. Shutemov

--
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: Free spare array to avoid memory leak

[Sha Zhengju]

On 03/08/2012 06:35 PM, Kirill A. Shutemov wrote:
> On Thu, Mar 08, 2012 at 10:11:32AM +0800, Sha Zhengju wrote:
>> On 03/08/2012 07:08 AM, Kirill A. Shutemov wrote:
>>> On Tue, Mar 06, 2012 at 08:13:24PM +0800, Sha Zhengju wrote:
>>>> From: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
>>>>
>>>> When the last event is unregistered, there is no need to keep the spare
>>>> array anymore. So free it to avoid memory leak.
>>> It's not a leak. It will be freed on next event register.
>>
>> Yeah, I noticed that. But what if it is just the last one and no more
>> event registering ?
> See my question below. ;)
>
>>> Yeah, we don't have to keep spare if primary is empty. But is it worth to
>>> make code more complicated to save few bytes of memory?
>>>
If we unregister the last event and *don't* register a new event anymore,
the primary is freed but the spare is still kept which has no chance to
free.

IMHO, it's obvious not a problem of saving bytes but *memory leak*.

>>>> Signed-off-by: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
>>>>
>>>> ---
>>>>   mm/memcontrol.c |    6 ++++++
>>>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>>> index 22d94f5..3c09a84 100644
>>>> --- a/mm/memcontrol.c
>>>> +++ b/mm/memcontrol.c
>>>> @@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
>>>>   swap_buffers:
>>>>   	/* Swap primary and spare array */
>>>>   	thresholds->spare = thresholds->primary;
>>>> +	/* If all events are unregistered, free the spare array */
>>>> +	if (!new) {
>>>> +		kfree(thresholds->spare);
>>>> +		thresholds->spare = NULL;
>>>> +	}
>>>> +
>>>>   	rcu_assign_pointer(thresholds->primary, new);
>>>>
>>>>   	/* To be sure that nobody uses thresholds */
>>>> -- 
>>>> 1.7.4.1
>>>>

Re: [PATCH] memcg: Free spare array to avoid memory leak

[Sha Zhengju]

On 03/08/2012 06:35 PM, Kirill A. Shutemov wrote:
> On Thu, Mar 08, 2012 at 10:11:32AM +0800, Sha Zhengju wrote:
>> On 03/08/2012 07:08 AM, Kirill A. Shutemov wrote:
>>> On Tue, Mar 06, 2012 at 08:13:24PM +0800, Sha Zhengju wrote:
>>>> From: Sha Zhengju<handai.szj@taobao.com>
>>>>
>>>> When the last event is unregistered, there is no need to keep the spare
>>>> array anymore. So free it to avoid memory leak.
>>> It's not a leak. It will be freed on next event register.
>>
>> Yeah, I noticed that. But what if it is just the last one and no more
>> event registering ?
> See my question below. ;)
>
>>> Yeah, we don't have to keep spare if primary is empty. But is it worth to
>>> make code more complicated to save few bytes of memory?
>>>
If we unregister the last event and *don't* register a new event anymore,
the primary is freed but the spare is still kept which has no chance to
free.

IMHO, it's obvious not a problem of saving bytes but *memory leak*.

>>>> Signed-off-by: Sha Zhengju<handai.szj@taobao.com>
>>>>
>>>> ---
>>>>   mm/memcontrol.c |    6 ++++++
>>>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>>> index 22d94f5..3c09a84 100644
>>>> --- a/mm/memcontrol.c
>>>> +++ b/mm/memcontrol.c
>>>> @@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
>>>>   swap_buffers:
>>>>   	/* Swap primary and spare array */
>>>>   	thresholds->spare = thresholds->primary;
>>>> +	/* If all events are unregistered, free the spare array */
>>>> +	if (!new) {
>>>> +		kfree(thresholds->spare);
>>>> +		thresholds->spare = NULL;
>>>> +	}
>>>> +
>>>>   	rcu_assign_pointer(thresholds->primary, new);
>>>>
>>>>   	/* To be sure that nobody uses thresholds */
>>>> -- 
>>>> 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: Free spare array to avoid memory leak

[KAMEZAWA Hiroyuki]

On Thu, 08 Mar 2012 18:46:13 +0800
Sha Zhengju <handai.szj-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On 03/08/2012 06:35 PM, Kirill A. Shutemov wrote:
> > On Thu, Mar 08, 2012 at 10:11:32AM +0800, Sha Zhengju wrote:
> >> On 03/08/2012 07:08 AM, Kirill A. Shutemov wrote:
> >>> On Tue, Mar 06, 2012 at 08:13:24PM +0800, Sha Zhengju wrote:
> >>>> From: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
> >>>>
> >>>> When the last event is unregistered, there is no need to keep the spare
> >>>> array anymore. So free it to avoid memory leak.
> >>> It's not a leak. It will be freed on next event register.
> >>
> >> Yeah, I noticed that. But what if it is just the last one and no more
> >> event registering ?
> > See my question below. ;)
> >
> >>> Yeah, we don't have to keep spare if primary is empty. But is it worth to
> >>> make code more complicated to save few bytes of memory?
> >>>
> If we unregister the last event and *don't* register a new event anymore,
> the primary is freed but the spare is still kept which has no chance to
> free.
> 
> IMHO, it's obvious not a problem of saving bytes but *memory leak*.
> 

IMHO, it's cached. It will be freed when a memcg is destroyed.

Thanks,
-Kame

Re: [PATCH] memcg: Free spare array to avoid memory leak

[KAMEZAWA Hiroyuki]

On Thu, 08 Mar 2012 18:46:13 +0800
Sha Zhengju <handai.szj@gmail.com> wrote:

> On 03/08/2012 06:35 PM, Kirill A. Shutemov wrote:
> > On Thu, Mar 08, 2012 at 10:11:32AM +0800, Sha Zhengju wrote:
> >> On 03/08/2012 07:08 AM, Kirill A. Shutemov wrote:
> >>> On Tue, Mar 06, 2012 at 08:13:24PM +0800, Sha Zhengju wrote:
> >>>> From: Sha Zhengju<handai.szj@taobao.com>
> >>>>
> >>>> When the last event is unregistered, there is no need to keep the spare
> >>>> array anymore. So free it to avoid memory leak.
> >>> It's not a leak. It will be freed on next event register.
> >>
> >> Yeah, I noticed that. But what if it is just the last one and no more
> >> event registering ?
> > See my question below. ;)
> >
> >>> Yeah, we don't have to keep spare if primary is empty. But is it worth to
> >>> make code more complicated to save few bytes of memory?
> >>>
> If we unregister the last event and *don't* register a new event anymore,
> the primary is freed but the spare is still kept which has no chance to
> free.
> 
> IMHO, it's obvious not a problem of saving bytes but *memory leak*.
> 

IMHO, it's cached. It will be freed when a memcg is destroyed.

Thanks,
-Kame

--
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: Free spare array to avoid memory leak

[Sha Zhengju]

On 03/09/2012 09:24 AM, KAMEZAWA Hiroyuki wrote:
> On Thu, 08 Mar 2012 18:46:13 +0800
> Sha Zhengju<handai.szj-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>  wrote:
>
>> On 03/08/2012 06:35 PM, Kirill A. Shutemov wrote:
>>> On Thu, Mar 08, 2012 at 10:11:32AM +0800, Sha Zhengju wrote:
>>>> On 03/08/2012 07:08 AM, Kirill A. Shutemov wrote:
>>>>> On Tue, Mar 06, 2012 at 08:13:24PM +0800, Sha Zhengju wrote:
>>>>>> From: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
>>>>>>
>>>>>> When the last event is unregistered, there is no need to keep the spare
>>>>>> array anymore. So free it to avoid memory leak.
>>>>> It's not a leak. It will be freed on next event register.
>>>> Yeah, I noticed that. But what if it is just the last one and no more
>>>> event registering ?
>>> See my question below. ;)
>>>
>>>>> Yeah, we don't have to keep spare if primary is empty. But is it worth to
>>>>> make code more complicated to save few bytes of memory?
>>>>>
>> If we unregister the last event and *don't* register a new event anymore,
>> the primary is freed but the spare is still kept which has no chance to
>> free.
>>
>> IMHO, it's obvious not a problem of saving bytes but *memory leak*.
>>
> IMHO, it's cached. It will be freed when a memcg is destroyed.

I didn't see that behavior.  Could you point it out ? :-)

Re: [PATCH] memcg: Free spare array to avoid memory leak

[Sha Zhengju]

On 03/09/2012 09:24 AM, KAMEZAWA Hiroyuki wrote:
> On Thu, 08 Mar 2012 18:46:13 +0800
> Sha Zhengju<handai.szj@gmail.com>  wrote:
>
>> On 03/08/2012 06:35 PM, Kirill A. Shutemov wrote:
>>> On Thu, Mar 08, 2012 at 10:11:32AM +0800, Sha Zhengju wrote:
>>>> On 03/08/2012 07:08 AM, Kirill A. Shutemov wrote:
>>>>> On Tue, Mar 06, 2012 at 08:13:24PM +0800, Sha Zhengju wrote:
>>>>>> From: Sha Zhengju<handai.szj@taobao.com>
>>>>>>
>>>>>> When the last event is unregistered, there is no need to keep the spare
>>>>>> array anymore. So free it to avoid memory leak.
>>>>> It's not a leak. It will be freed on next event register.
>>>> Yeah, I noticed that. But what if it is just the last one and no more
>>>> event registering ?
>>> See my question below. ;)
>>>
>>>>> Yeah, we don't have to keep spare if primary is empty. But is it worth to
>>>>> make code more complicated to save few bytes of memory?
>>>>>
>> If we unregister the last event and *don't* register a new event anymore,
>> the primary is freed but the spare is still kept which has no chance to
>> free.
>>
>> IMHO, it's obvious not a problem of saving bytes but *memory leak*.
>>
> IMHO, it's cached. It will be freed when a memcg is destroyed.

I didn't see that behavior.  Could you point it out ? :-)

--
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: Free spare array to avoid memory leak

[KAMEZAWA Hiroyuki]

On Fri, 09 Mar 2012 11:24:22 +0800
Sha Zhengju <handai.szj-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On 03/09/2012 09:24 AM, KAMEZAWA Hiroyuki wrote:
> > On Thu, 08 Mar 2012 18:46:13 +0800
> > Sha Zhengju<handai.szj-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>  wrote:
> >
> >> On 03/08/2012 06:35 PM, Kirill A. Shutemov wrote:
> >>> On Thu, Mar 08, 2012 at 10:11:32AM +0800, Sha Zhengju wrote:
> >>>> On 03/08/2012 07:08 AM, Kirill A. Shutemov wrote:
> >>>>> On Tue, Mar 06, 2012 at 08:13:24PM +0800, Sha Zhengju wrote:
> >>>>>> From: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
> >>>>>>
> >>>>>> When the last event is unregistered, there is no need to keep the spare
> >>>>>> array anymore. So free it to avoid memory leak.
> >>>>> It's not a leak. It will be freed on next event register.
> >>>> Yeah, I noticed that. But what if it is just the last one and no more
> >>>> event registering ?
> >>> See my question below. ;)
> >>>
> >>>>> Yeah, we don't have to keep spare if primary is empty. But is it worth to
> >>>>> make code more complicated to save few bytes of memory?
> >>>>>
> >> If we unregister the last event and *don't* register a new event anymore,
> >> the primary is freed but the spare is still kept which has no chance to
> >> free.
> >>
> >> IMHO, it's obvious not a problem of saving bytes but *memory leak*.
> >>
> > IMHO, it's cached. It will be freed when a memcg is destroyed.
> 
> I didn't see that behavior.  Could you point it out ? :-)
> 

I'm sorry I misundersttood the behavior.
I'll read your patch again.


Thanks,
-Kame

Re: [PATCH] memcg: Free spare array to avoid memory leak

[KAMEZAWA Hiroyuki]

On Fri, 09 Mar 2012 11:24:22 +0800
Sha Zhengju <handai.szj@gmail.com> wrote:

> On 03/09/2012 09:24 AM, KAMEZAWA Hiroyuki wrote:
> > On Thu, 08 Mar 2012 18:46:13 +0800
> > Sha Zhengju<handai.szj@gmail.com>  wrote:
> >
> >> On 03/08/2012 06:35 PM, Kirill A. Shutemov wrote:
> >>> On Thu, Mar 08, 2012 at 10:11:32AM +0800, Sha Zhengju wrote:
> >>>> On 03/08/2012 07:08 AM, Kirill A. Shutemov wrote:
> >>>>> On Tue, Mar 06, 2012 at 08:13:24PM +0800, Sha Zhengju wrote:
> >>>>>> From: Sha Zhengju<handai.szj@taobao.com>
> >>>>>>
> >>>>>> When the last event is unregistered, there is no need to keep the spare
> >>>>>> array anymore. So free it to avoid memory leak.
> >>>>> It's not a leak. It will be freed on next event register.
> >>>> Yeah, I noticed that. But what if it is just the last one and no more
> >>>> event registering ?
> >>> See my question below. ;)
> >>>
> >>>>> Yeah, we don't have to keep spare if primary is empty. But is it worth to
> >>>>> make code more complicated to save few bytes of memory?
> >>>>>
> >> If we unregister the last event and *don't* register a new event anymore,
> >> the primary is freed but the spare is still kept which has no chance to
> >> free.
> >>
> >> IMHO, it's obvious not a problem of saving bytes but *memory leak*.
> >>
> > IMHO, it's cached. It will be freed when a memcg is destroyed.
> 
> I didn't see that behavior.  Could you point it out ? :-)
> 

I'm sorry I misundersttood the behavior.
I'll read your patch again.


Thanks,
-Kame

--
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: Free spare array to avoid memory leak

[KAMEZAWA Hiroyuki]

On Tue,  6 Mar 2012 20:13:24 +0800
Sha Zhengju <handai.szj@gmail.com> wrote:

> From: Sha Zhengju <handai.szj@taobao.com>
> 
> When the last event is unregistered, there is no need to keep the spare
> array anymore. So free it to avoid memory leak.
> 
> Signed-off-by: Sha Zhengju <handai.szj@taobao.com>
> 
> ---
>  mm/memcontrol.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 22d94f5..3c09a84 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
>  swap_buffers:
>  	/* Swap primary and spare array */
>  	thresholds->spare = thresholds->primary;
> +	/* If all events are unregistered, free the spare array */
> +	if (!new) {
> +		kfree(thresholds->spare);
> +		thresholds->spare = NULL;
> +	}
> +

Could you clear thresholds->primary ? I don't like a pointer points to freed memory.

Thanks,
-Kame

--
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: Free spare array to avoid memory leak

[Sha Zhengju]

On 03/09/2012 11:40 AM, KAMEZAWA Hiroyuki wrote:
> On Tue,  6 Mar 2012 20:13:24 +0800
> Sha Zhengju<handai.szj-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>  wrote:
>
>> From: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
>>
>> When the last event is unregistered, there is no need to keep the spare
>> array anymore. So free it to avoid memory leak.
>>
>> Signed-off-by: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
>>
>> ---
>>   mm/memcontrol.c |    6 ++++++
>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 22d94f5..3c09a84 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
>>   swap_buffers:
>>   	/* Swap primary and spare array */
>>   	thresholds->spare = thresholds->primary;
>> +	/* If all events are unregistered, free the spare array */
>> +	if (!new) {
>> +		kfree(thresholds->spare);
>> +		thresholds->spare = NULL;
>> +	}
>> +
> Could you clear thresholds->primary ? I don't like a pointer points to freed memory.
Do you meaning I should set ‘thresholds->primary = NULL‘ ？
But the following rcu_assign_pointer will do this :

+	/* If all events are unregistered, free the spare array */
+	if (!new) {
+		kfree(thresholds->spare);
+		thresholds->spare = NULL;
+	}
+
  	rcu_assign_pointer(thresholds->primary, new);<---------*HERE*

  	/* To be sure that nobody uses thresholds */

Re: [PATCH] memcg: Free spare array to avoid memory leak

[Sha Zhengju]

On 03/09/2012 11:40 AM, KAMEZAWA Hiroyuki wrote:
> On Tue,  6 Mar 2012 20:13:24 +0800
> Sha Zhengju<handai.szj@gmail.com>  wrote:
>
>> From: Sha Zhengju<handai.szj@taobao.com>
>>
>> When the last event is unregistered, there is no need to keep the spare
>> array anymore. So free it to avoid memory leak.
>>
>> Signed-off-by: Sha Zhengju<handai.szj@taobao.com>
>>
>> ---
>>   mm/memcontrol.c |    6 ++++++
>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 22d94f5..3c09a84 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
>>   swap_buffers:
>>   	/* Swap primary and spare array */
>>   	thresholds->spare = thresholds->primary;
>> +	/* If all events are unregistered, free the spare array */
>> +	if (!new) {
>> +		kfree(thresholds->spare);
>> +		thresholds->spare = NULL;
>> +	}
>> +
> Could you clear thresholds->primary ? I don't like a pointer points to freed memory.
Do you meaning I should set a??thresholds->primary = NULLa?? i 1/4 ?
But the following rcu_assign_pointer will do this :

+	/* If all events are unregistered, free the spare array */
+	if (!new) {
+		kfree(thresholds->spare);
+		thresholds->spare = NULL;
+	}
+
  	rcu_assign_pointer(thresholds->primary, new);<---------*HERE*

  	/* To be sure that nobody uses thresholds */

--
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: Free spare array to avoid memory leak

[KAMEZAWA Hiroyuki]

On Fri, 09 Mar 2012 12:07:32 +0800
Sha Zhengju <handai.szj-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On 03/09/2012 11:40 AM, KAMEZAWA Hiroyuki wrote:
> > On Tue,  6 Mar 2012 20:13:24 +0800
> > Sha Zhengju<handai.szj-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>  wrote:
> >
> >> From: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
> >>
> >> When the last event is unregistered, there is no need to keep the spare
> >> array anymore. So free it to avoid memory leak.
> >>
> >> Signed-off-by: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
> >>
> >> ---
> >>   mm/memcontrol.c |    6 ++++++
> >>   1 files changed, 6 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> >> index 22d94f5..3c09a84 100644
> >> --- a/mm/memcontrol.c
> >> +++ b/mm/memcontrol.c
> >> @@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
> >>   swap_buffers:
> >>   	/* Swap primary and spare array */
> >>   	thresholds->spare = thresholds->primary;
> >> +	/* If all events are unregistered, free the spare array */
> >> +	if (!new) {
> >> +		kfree(thresholds->spare);
> >> +		thresholds->spare = NULL;
> >> +	}
> >> +
> > Could you clear thresholds->primary ? I don't like a pointer points to freed memory.
> Do you meaning I should set ‘thresholds->primary = NULL‘ ？
> But the following rcu_assign_pointer will do this :
> 
> +	/* If all events are unregistered, free the spare array */
> +	if (!new) {
> +		kfree(thresholds->spare);
> +		thresholds->spare = NULL;
> +	}
> +
>   	rcu_assign_pointer(thresholds->primary, new);<---------*HERE*
> 

Hm, ok.

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


BTW, can memory cgroup be destroyed while there are registered events ?

Re: [PATCH] memcg: Free spare array to avoid memory leak

[KAMEZAWA Hiroyuki]

On Fri, 09 Mar 2012 12:07:32 +0800
Sha Zhengju <handai.szj@gmail.com> wrote:

> On 03/09/2012 11:40 AM, KAMEZAWA Hiroyuki wrote:
> > On Tue,  6 Mar 2012 20:13:24 +0800
> > Sha Zhengju<handai.szj@gmail.com>  wrote:
> >
> >> From: Sha Zhengju<handai.szj@taobao.com>
> >>
> >> When the last event is unregistered, there is no need to keep the spare
> >> array anymore. So free it to avoid memory leak.
> >>
> >> Signed-off-by: Sha Zhengju<handai.szj@taobao.com>
> >>
> >> ---
> >>   mm/memcontrol.c |    6 ++++++
> >>   1 files changed, 6 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> >> index 22d94f5..3c09a84 100644
> >> --- a/mm/memcontrol.c
> >> +++ b/mm/memcontrol.c
> >> @@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
> >>   swap_buffers:
> >>   	/* Swap primary and spare array */
> >>   	thresholds->spare = thresholds->primary;
> >> +	/* If all events are unregistered, free the spare array */
> >> +	if (!new) {
> >> +		kfree(thresholds->spare);
> >> +		thresholds->spare = NULL;
> >> +	}
> >> +
> > Could you clear thresholds->primary ? I don't like a pointer points to freed memory.
> Do you meaning I should set ‘thresholds->primary = NULL‘ ？
> But the following rcu_assign_pointer will do this :
> 
> +	/* If all events are unregistered, free the spare array */
> +	if (!new) {
> +		kfree(thresholds->spare);
> +		thresholds->spare = NULL;
> +	}
> +
>   	rcu_assign_pointer(thresholds->primary, new);<---------*HERE*
> 

Hm, ok.

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>


BTW, can memory cgroup be destroyed while there are registered events ?



--
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: Free spare array to avoid memory leak

[Kirill A. Shutemov]

On Fri, Mar 09, 2012 at 01:20:16PM +0900, KAMEZAWA Hiroyuki wrote:
> On Fri, 09 Mar 2012 12:07:32 +0800
> Sha Zhengju <handai.szj-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > On 03/09/2012 11:40 AM, KAMEZAWA Hiroyuki wrote:
> > > On Tue,  6 Mar 2012 20:13:24 +0800
> > > Sha Zhengju<handai.szj-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>  wrote:
> > >
> > >> From: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
> > >>
> > >> When the last event is unregistered, there is no need to keep the spare
> > >> array anymore. So free it to avoid memory leak.
> > >>
> > >> Signed-off-by: Sha Zhengju<handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org>
> > >>
> > >> ---
> > >>   mm/memcontrol.c |    6 ++++++
> > >>   1 files changed, 6 insertions(+), 0 deletions(-)
> > >>
> > >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > >> index 22d94f5..3c09a84 100644
> > >> --- a/mm/memcontrol.c
> > >> +++ b/mm/memcontrol.c
> > >> @@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
> > >>   swap_buffers:
> > >>   	/* Swap primary and spare array */
> > >>   	thresholds->spare = thresholds->primary;
> > >> +	/* If all events are unregistered, free the spare array */
> > >> +	if (!new) {
> > >> +		kfree(thresholds->spare);
> > >> +		thresholds->spare = NULL;
> > >> +	}
> > >> +
> > > Could you clear thresholds->primary ? I don't like a pointer points to freed memory.
> > Do you meaning I should set ‘thresholds->primary = NULL‘ ？
> > But the following rcu_assign_pointer will do this :
> > 
> > +	/* If all events are unregistered, free the spare array */
> > +	if (!new) {
> > +		kfree(thresholds->spare);
> > +		thresholds->spare = NULL;
> > +	}
> > +
> >   	rcu_assign_pointer(thresholds->primary, new);<---------*HERE*
> > 
> 
> Hm, ok.
> 
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> 
> 
> BTW, can memory cgroup be destroyed while there are registered events ?

Yes, it can. All eventfds will be closed first. See cgroup_rmdir().

And here's possibility of leak. If we have an eventfd with >1 threasholds
attached to it, mem_cgroup_usage_unregister_event() will leave spare
not freed. And then we destroy cgroup...

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

-- 
 Kirill A. Shutemov

Re: [PATCH] memcg: Free spare array to avoid memory leak

[Kirill A. Shutemov]

On Fri, Mar 09, 2012 at 01:20:16PM +0900, KAMEZAWA Hiroyuki wrote:
> On Fri, 09 Mar 2012 12:07:32 +0800
> Sha Zhengju <handai.szj@gmail.com> wrote:
> 
> > On 03/09/2012 11:40 AM, KAMEZAWA Hiroyuki wrote:
> > > On Tue,  6 Mar 2012 20:13:24 +0800
> > > Sha Zhengju<handai.szj@gmail.com>  wrote:
> > >
> > >> From: Sha Zhengju<handai.szj@taobao.com>
> > >>
> > >> When the last event is unregistered, there is no need to keep the spare
> > >> array anymore. So free it to avoid memory leak.
> > >>
> > >> Signed-off-by: Sha Zhengju<handai.szj@taobao.com>
> > >>
> > >> ---
> > >>   mm/memcontrol.c |    6 ++++++
> > >>   1 files changed, 6 insertions(+), 0 deletions(-)
> > >>
> > >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > >> index 22d94f5..3c09a84 100644
> > >> --- a/mm/memcontrol.c
> > >> +++ b/mm/memcontrol.c
> > >> @@ -4412,6 +4412,12 @@ static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
> > >>   swap_buffers:
> > >>   	/* Swap primary and spare array */
> > >>   	thresholds->spare = thresholds->primary;
> > >> +	/* If all events are unregistered, free the spare array */
> > >> +	if (!new) {
> > >> +		kfree(thresholds->spare);
> > >> +		thresholds->spare = NULL;
> > >> +	}
> > >> +
> > > Could you clear thresholds->primary ? I don't like a pointer points to freed memory.
> > Do you meaning I should set a??thresholds->primary = NULLa?? i 1/4 ?
> > But the following rcu_assign_pointer will do this :
> > 
> > +	/* If all events are unregistered, free the spare array */
> > +	if (!new) {
> > +		kfree(thresholds->spare);
> > +		thresholds->spare = NULL;
> > +	}
> > +
> >   	rcu_assign_pointer(thresholds->primary, new);<---------*HERE*
> > 
> 
> Hm, ok.
> 
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> 
> BTW, can memory cgroup be destroyed while there are registered events ?

Yes, it can. All eventfds will be closed first. See cgroup_rmdir().

And here's possibility of leak. If we have an eventfd with >1 threasholds
attached to it, mem_cgroup_usage_unregister_event() will leave spare
not freed. And then we destroy cgroup...

Reviewed-by: Kirill A. Shutemov <kirill@shutemov.name>

-- 
 Kirill A. Shutemov

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