linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] memcg: Free spare array to avoid memory leak
@ 2012-03-06 12:13 Sha Zhengju
  2012-03-07 23:08 ` Kirill A. Shutemov
  2012-03-09  3:40 ` KAMEZAWA Hiroyuki
  0 siblings, 2 replies; 12+ messages in thread
From: Sha Zhengju @ 2012-03-06 12:13 UTC (permalink / raw)
  To: linux-mm, cgroups; +Cc: kamezawa.hiroyu, kirill, 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>

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] memcg: Free spare array to avoid memory leak
  2012-03-06 12:13 [PATCH] memcg: Free spare array to avoid memory leak Sha Zhengju
@ 2012-03-07 23:08 ` Kirill A. Shutemov
  2012-03-08  2:11   ` Sha Zhengju
  2012-03-09  3:40 ` KAMEZAWA Hiroyuki
  1 sibling, 1 reply; 12+ messages in thread
From: Kirill A. Shutemov @ 2012-03-07 23:08 UTC (permalink / raw)
  To: Sha Zhengju; +Cc: linux-mm, cgroups, kamezawa.hiroyu, Sha Zhengju

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] memcg: Free spare array to avoid memory leak
  2012-03-07 23:08 ` Kirill A. Shutemov
@ 2012-03-08  2:11   ` Sha Zhengju
  2012-03-08 10:35     ` Kirill A. Shutemov
  0 siblings, 1 reply; 12+ messages in thread
From: Sha Zhengju @ 2012-03-08  2:11 UTC (permalink / raw)
  To: Kirill A. Shutemov; +Cc: linux-mm, cgroups, kamezawa.hiroyu, 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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] memcg: Free spare array to avoid memory leak
  2012-03-08  2:11   ` Sha Zhengju
@ 2012-03-08 10:35     ` Kirill A. Shutemov
  2012-03-08 10:46       ` Sha Zhengju
  0 siblings, 1 reply; 12+ messages in thread
From: Kirill A. Shutemov @ 2012-03-08 10:35 UTC (permalink / raw)
  To: Sha Zhengju; +Cc: linux-mm, cgroups, kamezawa.hiroyu, Sha Zhengju

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] memcg: Free spare array to avoid memory leak
  2012-03-08 10:35     ` Kirill A. Shutemov
@ 2012-03-08 10:46       ` Sha Zhengju
  2012-03-09  1:24         ` KAMEZAWA Hiroyuki
  0 siblings, 1 reply; 12+ messages in thread
From: Sha Zhengju @ 2012-03-08 10:46 UTC (permalink / raw)
  To: Kirill A. Shutemov; +Cc: linux-mm, cgroups, kamezawa.hiroyu, 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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] memcg: Free spare array to avoid memory leak
  2012-03-08 10:46       ` Sha Zhengju
@ 2012-03-09  1:24         ` KAMEZAWA Hiroyuki
  2012-03-09  3:24           ` Sha Zhengju
  0 siblings, 1 reply; 12+ messages in thread
From: KAMEZAWA Hiroyuki @ 2012-03-09  1:24 UTC (permalink / raw)
  To: Sha Zhengju; +Cc: Kirill A. Shutemov, linux-mm, cgroups, Sha Zhengju

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] memcg: Free spare array to avoid memory leak
  2012-03-09  1:24         ` KAMEZAWA Hiroyuki
@ 2012-03-09  3:24           ` Sha Zhengju
  2012-03-09  3:36             ` KAMEZAWA Hiroyuki
  0 siblings, 1 reply; 12+ messages in thread
From: Sha Zhengju @ 2012-03-09  3:24 UTC (permalink / raw)
  To: KAMEZAWA Hiroyuki; +Cc: Kirill A. Shutemov, linux-mm, cgroups, 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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] memcg: Free spare array to avoid memory leak
  2012-03-09  3:24           ` Sha Zhengju
@ 2012-03-09  3:36             ` KAMEZAWA Hiroyuki
  0 siblings, 0 replies; 12+ messages in thread
From: KAMEZAWA Hiroyuki @ 2012-03-09  3:36 UTC (permalink / raw)
  To: Sha Zhengju; +Cc: Kirill A. Shutemov, linux-mm, cgroups, Sha Zhengju

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] memcg: Free spare array to avoid memory leak
  2012-03-06 12:13 [PATCH] memcg: Free spare array to avoid memory leak Sha Zhengju
  2012-03-07 23:08 ` Kirill A. Shutemov
@ 2012-03-09  3:40 ` KAMEZAWA Hiroyuki
  2012-03-09  4:07   ` Sha Zhengju
  1 sibling, 1 reply; 12+ messages in thread
From: KAMEZAWA Hiroyuki @ 2012-03-09  3:40 UTC (permalink / raw)
  To: Sha Zhengju; +Cc: linux-mm, cgroups, kirill, Sha Zhengju

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] memcg: Free spare array to avoid memory leak
  2012-03-09  3:40 ` KAMEZAWA Hiroyuki
@ 2012-03-09  4:07   ` Sha Zhengju
  2012-03-09  4:20     ` KAMEZAWA Hiroyuki
  0 siblings, 1 reply; 12+ messages in thread
From: Sha Zhengju @ 2012-03-09  4:07 UTC (permalink / raw)
  To: KAMEZAWA Hiroyuki; +Cc: linux-mm, cgroups, kirill, 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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] memcg: Free spare array to avoid memory leak
  2012-03-09  4:07   ` Sha Zhengju
@ 2012-03-09  4:20     ` KAMEZAWA Hiroyuki
  2012-03-09  9:38       ` Kirill A. Shutemov
  0 siblings, 1 reply; 12+ messages in thread
From: KAMEZAWA Hiroyuki @ 2012-03-09  4:20 UTC (permalink / raw)
  To: Sha Zhengju; +Cc: linux-mm, cgroups, kirill, Sha Zhengju

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] memcg: Free spare array to avoid memory leak
  2012-03-09  4:20     ` KAMEZAWA Hiroyuki
@ 2012-03-09  9:38       ` Kirill A. Shutemov
  0 siblings, 0 replies; 12+ messages in thread
From: Kirill A. Shutemov @ 2012-03-09  9:38 UTC (permalink / raw)
  To: KAMEZAWA Hiroyuki; +Cc: Sha Zhengju, linux-mm, cgroups, Sha Zhengju

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2012-03-09  8:46 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-06 12:13 [PATCH] memcg: Free spare array to avoid memory leak Sha Zhengju
2012-03-07 23:08 ` Kirill A. Shutemov
2012-03-08  2:11   ` Sha Zhengju
2012-03-08 10:35     ` Kirill A. Shutemov
2012-03-08 10:46       ` Sha Zhengju
2012-03-09  1:24         ` KAMEZAWA Hiroyuki
2012-03-09  3:24           ` Sha Zhengju
2012-03-09  3:36             ` KAMEZAWA Hiroyuki
2012-03-09  3:40 ` KAMEZAWA Hiroyuki
2012-03-09  4:07   ` Sha Zhengju
2012-03-09  4:20     ` KAMEZAWA Hiroyuki
2012-03-09  9:38       ` Kirill A. Shutemov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).