[PATCH] mm: memcg: keep root group unchanged if fail to create new

[PATCH] mm: memcg: keep root group unchanged if fail to create new

[Hillf Danton]

If the request is not to create root group and we fail to meet it, we'd leave
the root unchanged.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/mm/memcontrol.c	Fri Dec  9 21:57:40 2011
+++ b/mm/memcontrol.c	Sun Dec 11 09:04:48 2011
@@ -4849,8 +4849,10 @@ mem_cgroup_create(struct cgroup_subsys *
 		enable_swap_cgroup();
 		parent = NULL;
 		root_mem_cgroup = memcg;
-		if (mem_cgroup_soft_limit_tree_init())
+		if (mem_cgroup_soft_limit_tree_init()) {
+			root_mem_cgroup = NULL;
 			goto free_out;
+		}
 		for_each_possible_cpu(cpu) {
 			struct memcg_stock_pcp *stock =
 						&per_cpu(memcg_stock, cpu);
@@ -4888,7 +4890,6 @@ mem_cgroup_create(struct cgroup_subsys *
 	return &memcg->css;
 free_out:
 	__mem_cgroup_free(memcg);
-	root_mem_cgroup = NULL;
 	return ERR_PTR(error);
 }

--
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] mm: memcg: keep root group unchanged if fail to create new

[Hugh Dickins]

On Sun, 11 Dec 2011, Hillf Danton wrote:

> If the request is not to create root group and we fail to meet it,
> we'd leave the root unchanged.

I didn't understand that at first: please say "we should" rather
than "we'd", which I take to be an abbreviation for "we would".

> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>

Yes indeed, well caught:
Acked-by: Hugh Dickins <hughd@google.com>

I wonder what was going through the author's mind when he wrote it
that way?  I wonder if it's one of those bugs that creeps in when
you start from a perfectly functional patch, then make refinements
to suit feedback from reviewers.

On which topic: wouldn't this patch be better just to move the
"root_mem_cgroup = memcg;" two lines lower down (and of course
remove free_out's "root_mem_cgroup = NULL;" as you already did)?
I can't see mem_cgroup_soft_limit_tree_init() relying on
root_mem_cgroup at all.

Should your patch go to stable, even to 2.6.32-longterm?
Matter of taste, really: while it's not quite impossible for
alloc_mem_cgroup_per_zone_info() to fail, it is very unlikely.

> ---
> 
> --- a/mm/memcontrol.c	Fri Dec  9 21:57:40 2011
> +++ b/mm/memcontrol.c	Sun Dec 11 09:04:48 2011
> @@ -4849,8 +4849,10 @@ mem_cgroup_create(struct cgroup_subsys *
>  		enable_swap_cgroup();
>  		parent = NULL;
>  		root_mem_cgroup = memcg;
> -		if (mem_cgroup_soft_limit_tree_init())
> +		if (mem_cgroup_soft_limit_tree_init()) {
> +			root_mem_cgroup = NULL;
>  			goto free_out;
> +		}
>  		for_each_possible_cpu(cpu) {
>  			struct memcg_stock_pcp *stock =
>  						&per_cpu(memcg_stock, cpu);
> @@ -4888,7 +4890,6 @@ mem_cgroup_create(struct cgroup_subsys *
>  	return &memcg->css;
>  free_out:
>  	__mem_cgroup_free(memcg);
> -	root_mem_cgroup = NULL;
>  	return ERR_PTR(error);
>  }
> 

--
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] mm: memcg: keep root group unchanged if fail to create new

[Hillf Danton]

Hello Hugh

On Mon, Dec 12, 2011 at 7:39 AM, Hugh Dickins <hughd@google.com> wrote:
> On Sun, 11 Dec 2011, Hillf Danton wrote:
>
>> If the request is not to create root group and we fail to meet it,
>> we'd leave the root unchanged.
>
> I didn't understand that at first: please say "we should" rather
> than "we'd", which I take to be an abbreviation for "we would".
>

Thanks for correcting me.

>
> I wonder what was going through the author's mind when he wrote it
> that way?  I wonder if it's one of those bugs that creeps in when
> you start from a perfectly functional patch, then make refinements
> to suit feedback from reviewers.
>

Actually no such a perfectly functional patch, but I also wonder if
you are likely to post and/or share your current todo list if any,
then many could learn the right direction, and in turn you comment
and pull acceptable works in that direction, like Linus.

Hillf

--
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] mm: memcg: keep root group unchanged if fail to create new

[Michal Hocko]

On Sun 11-12-11 15:39:43, Hugh Dickins wrote:
> On Sun, 11 Dec 2011, Hillf Danton wrote:
> 
> > If the request is not to create root group and we fail to meet it,
> > we'd leave the root unchanged.
> 
> I didn't understand that at first: please say "we should" rather
> than "we'd", which I take to be an abbreviation for "we would".
> 
> > 
> > Signed-off-by: Hillf Danton <dhillf@gmail.com>
> 
> Yes indeed, well caught:
> Acked-by: Hugh Dickins <hughd@google.com>
> 
> I wonder what was going through the author's mind when he wrote it
> that way?  I wonder if it's one of those bugs that creeps in when
> you start from a perfectly functional patch, then make refinements
> to suit feedback from reviewers.
> 
> On which topic: wouldn't this patch be better just to move the
> "root_mem_cgroup = memcg;" two lines lower down (and of course
> remove free_out's "root_mem_cgroup = NULL;" as you already did)?

Yes would look nicer.

> I can't see mem_cgroup_soft_limit_tree_init() relying on
> root_mem_cgroup at all.

It doesn't but it still needs some love to handle error case properly
AFAICS. We do not deallocate softlimit trees for nodes that succeeded.

[...]

Hilf could you update the patch please?
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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] mm: memcg: keep root group unchanged if fail to create new

[Hillf Danton]

On Mon, Dec 12, 2011 at 9:11 PM, Michal Hocko <mhocko@suse.cz> wrote:
>
> Hillf could you update the patch please?
>
Hi Michal,

Please review again, thanks.
Hillf

===CUT HERE===
From: Hillf Danton <dhillf@gmail.com>
Subject: [PATCH] mm: memcg: keep root group unchanged if fail to create new

If the request is to create non-root group and we fail to meet it, we should
leave the root unchanged.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
Acked-by: Hugh Dickins <hughd@google.com>
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---

--- a/mm/memcontrol.c	Fri Dec  9 21:57:40 2011
+++ b/mm/memcontrol.c	Mon Dec 12 21:47:14 2011
@@ -4848,9 +4848,9 @@ mem_cgroup_create(struct cgroup_subsys *
 		int cpu;
 		enable_swap_cgroup();
 		parent = NULL;
-		root_mem_cgroup = memcg;
 		if (mem_cgroup_soft_limit_tree_init())
 			goto free_out;
+		root_mem_cgroup = memcg;
 		for_each_possible_cpu(cpu) {
 			struct memcg_stock_pcp *stock =
 						&per_cpu(memcg_stock, cpu);
@@ -4888,7 +4888,6 @@ mem_cgroup_create(struct cgroup_subsys *
 	return &memcg->css;
 free_out:
 	__mem_cgroup_free(memcg);
-	root_mem_cgroup = NULL;
 	return ERR_PTR(error);
 }

--
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] mm: memcg: keep root group unchanged if fail to create new

[Michal Hocko]

On Mon 12-12-11 21:49:18, Hillf Danton wrote:
[...]
> From: Hillf Danton <dhillf@gmail.com>
> Subject: [PATCH] mm: memcg: keep root group unchanged if fail to create new
> 
> If the request is to create non-root group and we fail to meet it, we should
> leave the root unchanged.
> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> Acked-by: Hugh Dickins <hughd@google.com>
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Acked-by: Michal Hocko <mhocko@suse.cz>

Thanks
> ---
> 
> --- a/mm/memcontrol.c	Fri Dec  9 21:57:40 2011
> +++ b/mm/memcontrol.c	Mon Dec 12 21:47:14 2011
> @@ -4848,9 +4848,9 @@ mem_cgroup_create(struct cgroup_subsys *
>  		int cpu;
>  		enable_swap_cgroup();
>  		parent = NULL;
> -		root_mem_cgroup = memcg;
>  		if (mem_cgroup_soft_limit_tree_init())
>  			goto free_out;
> +		root_mem_cgroup = memcg;
>  		for_each_possible_cpu(cpu) {
>  			struct memcg_stock_pcp *stock =
>  						&per_cpu(memcg_stock, cpu);
> @@ -4888,7 +4888,6 @@ mem_cgroup_create(struct cgroup_subsys *
>  	return &memcg->css;
>  free_out:
>  	__mem_cgroup_free(memcg);
> -	root_mem_cgroup = NULL;
>  	return ERR_PTR(error);
>  }
> 
> --
> 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>

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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: clean up soft_limit_tree properly new

[Michal Hocko]

And a follow up patch for the proper clean up:
---

Re: [PATCH] memcg: clean up soft_limit_tree properly new

[Johannes Weiner]

On Mon, Dec 12, 2011 at 03:09:35PM +0100, Michal Hocko wrote:
> And a follow up patch for the proper clean up:
> ---
> >From 4b9f5a1e88496af9f336d1ef37cfdf3754a3ba48 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Mon, 12 Dec 2011 15:04:18 +0100
> Subject: [PATCH] memcg: clean up soft_limit_tree properly
> 
> If we are not able to allocate tree nodes for all NUMA nodes then we
> should better clean up those that were allocated otherwise we will leak
> a memory.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

That being said, I think it's unlikely that the machine even boots
properly if those allocations fail.  But the code looks better this
way and one doesn't have to double take, wondering if anyone else is
taking care of the already allocated objects.

--
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: clean up soft_limit_tree properly new

[Michal Hocko]

On Tue 13-12-11 15:08:44, Johannes Weiner wrote:
> On Mon, Dec 12, 2011 at 03:09:35PM +0100, Michal Hocko wrote:
> > And a follow up patch for the proper clean up:
> > ---
> > >From 4b9f5a1e88496af9f336d1ef37cfdf3754a3ba48 Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.cz>
> > Date: Mon, 12 Dec 2011 15:04:18 +0100
> > Subject: [PATCH] memcg: clean up soft_limit_tree properly
> > 
> > If we are not able to allocate tree nodes for all NUMA nodes then we
> > should better clean up those that were allocated otherwise we will leak
> > a memory.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> 
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Thanks

> 
> That being said, I think it's unlikely that the machine even boots
> properly if those allocations fail.  But the code looks better this
> way and one doesn't have to double take, wondering if anyone else is
> taking care of the already allocated objects.

Yes, that was the point of the patch - just a cleanup. I do not think we
want to push it into stable...

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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: clean up soft_limit_tree properly new

[Andrew Morton]

On Mon, 12 Dec 2011 15:09:35 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> And a follow up patch for the proper clean up:
> ---
> >From 4b9f5a1e88496af9f336d1ef37cfdf3754a3ba48 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Mon, 12 Dec 2011 15:04:18 +0100
> Subject: [PATCH] memcg: clean up soft_limit_tree properly
> 
> If we are not able to allocate tree nodes for all NUMA nodes then we
> should better clean up those that were allocated otherwise we will leak
> a memory.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> ---
>  mm/memcontrol.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 6aff93c..838d812 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4874,7 +4874,7 @@ static int mem_cgroup_soft_limit_tree_init(void)
>  			tmp = -1;
>  		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL, tmp);
>  		if (!rtpn)
> -			return 1;
> +			goto err_cleanup;
>  
>  		soft_limit_tree.rb_tree_per_node[node] = rtpn;
>  
> @@ -4885,6 +4885,16 @@ static int mem_cgroup_soft_limit_tree_init(void)
>  		}
>  	}
>  	return 0;
> +
> +err_cleanup:
> +	for_each_node_state(node, N_POSSIBLE) {
> +		if (!soft_limit_tree.rb_tree_per_node[node])
> +			break;
> +		kfree(soft_limit_tree.rb_tree_per_node[node]);
> +		soft_limit_tree.rb_tree_per_node[node] = NULL;
> +	}
> +	return 1;
> +
>  }

afacit the kernel never frees the soft_limit_tree.rb_tree_per_node[]
entries on the mem_cgroup_destroy() path.  Bug?

--
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: clean up soft_limit_tree properly new

[KAMEZAWA Hiroyuki]

On Tue, 13 Dec 2011 17:00:12 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 12 Dec 2011 15:09:35 +0100
> Michal Hocko <mhocko@suse.cz> wrote:
> 
> > And a follow up patch for the proper clean up:
> > ---
> > >From 4b9f5a1e88496af9f336d1ef37cfdf3754a3ba48 Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.cz>
> > Date: Mon, 12 Dec 2011 15:04:18 +0100
> > Subject: [PATCH] memcg: clean up soft_limit_tree properly
> > 
> > If we are not able to allocate tree nodes for all NUMA nodes then we
> > should better clean up those that were allocated otherwise we will leak
> > a memory.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> > ---
> >  mm/memcontrol.c |   12 +++++++++++-
> >  1 files changed, 11 insertions(+), 1 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 6aff93c..838d812 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -4874,7 +4874,7 @@ static int mem_cgroup_soft_limit_tree_init(void)
> >  			tmp = -1;
> >  		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL, tmp);
> >  		if (!rtpn)
> > -			return 1;
> > +			goto err_cleanup;
> >  
> >  		soft_limit_tree.rb_tree_per_node[node] = rtpn;
> >  
> > @@ -4885,6 +4885,16 @@ static int mem_cgroup_soft_limit_tree_init(void)
> >  		}
> >  	}
> >  	return 0;
> > +
> > +err_cleanup:
> > +	for_each_node_state(node, N_POSSIBLE) {
> > +		if (!soft_limit_tree.rb_tree_per_node[node])
> > +			break;
> > +		kfree(soft_limit_tree.rb_tree_per_node[node]);
> > +		soft_limit_tree.rb_tree_per_node[node] = NULL;
> > +	}
> > +	return 1;
> > +
> >  }
> 
> afacit the kernel never frees the soft_limit_tree.rb_tree_per_node[]
> entries on the mem_cgroup_destroy() path.  Bug?
> 

soft_limit_tree.rb_tree_per_node[] is a global object and allocated once
at creating root cgroup.

Nodes of rb_tree for a memcg are contained in struct mem_cgroup_per_zone
and it's freed at mem_cgroup_destroy().

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] mm: memcg: keep root group unchanged if fail to create new

[Johannes Weiner]

On Mon, Dec 12, 2011 at 09:49:18PM +0800, Hillf Danton wrote:
> On Mon, Dec 12, 2011 at 9:11 PM, Michal Hocko <mhocko@suse.cz> wrote:
> >
> > Hillf could you update the patch please?
> >
> Hi Michal,
> 
> Please review again, thanks.
> Hillf
> 
> ===CUT HERE===
> From: Hillf Danton <dhillf@gmail.com>
> Subject: [PATCH] mm: memcg: keep root group unchanged if fail to create new
> 
> If the request is to create non-root group and we fail to meet it, we should
> leave the root unchanged.
> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> Acked-by: Hugh Dickins <hughd@google.com>
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

--
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] mm: memcg: keep root group unchanged if fail to create new

[KAMEZAWA Hiroyuki]

On Sun, 11 Dec 2011 09:19:12 +0800
Hillf Danton <dhillf@gmail.com> wrote:

> If the request is not to create root group and we fail to meet it, we'd leave
> the root unchanged.
> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>

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

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