[PATCH 0/3] cleanup memcg controller initialization

[PATCH 0/3] cleanup memcg controller initialization

[Michal Hocko]

Hi,
this is just a small cleanup I promised some time ago[1]. It just moves
all memcg controller initialization code independant on mem_cgroup into
subsystem initialization code.

There are no functional changes.

Diffstat even says that we have saved some lines.
 mm/memcontrol.c |   49 +++++++++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 28 deletions(-)

Shortlog says:
Michal Hocko (3):
      memcg: move mem_cgroup_soft_limit_tree_init to mem_cgroup_init
      memcg: move memcg_stock initialization to mem_cgroup_init
      memcg: cleanup mem_cgroup_init comment
---
[1] https://lkml.org/lkml/2012/12/18/256

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 0/3] cleanup memcg controller initialization

[Michal Hocko]

Hi,
this is just a small cleanup I promised some time ago[1]. It just moves
all memcg controller initialization code independant on mem_cgroup into
subsystem initialization code.

There are no functional changes.

Diffstat even says that we have saved some lines.
 mm/memcontrol.c |   49 +++++++++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 28 deletions(-)

Shortlog says:
Michal Hocko (3):
      memcg: move mem_cgroup_soft_limit_tree_init to mem_cgroup_init
      memcg: move memcg_stock initialization to mem_cgroup_init
      memcg: cleanup mem_cgroup_init comment
---
[1] https://lkml.org/lkml/2012/12/18/256

[PATCH 1/3] memcg: move mem_cgroup_soft_limit_tree_init to mem_cgroup_init

[Michal Hocko]

Per-node-zone soft limit tree is currently initialized when the root
cgroup is created which is OK but it pointlessly pollutes memcg
allocation code with something that can be called when the memcg
subsystem is initialized by mem_cgroup_init along with other controller
specific parts.

While we are at it let's make mem_cgroup_soft_limit_tree_init void
because it doesn't make much sense to report memory failure because if
we fail to allocate memory that early during the boot then we are
screwed anyway (this saves some code).

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c |   19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 2382fe9..b0d3339 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -6228,7 +6228,7 @@ struct mem_cgroup *parent_mem_cgroup(struct mem_cgroup *memcg)
 }
 EXPORT_SYMBOL(parent_mem_cgroup);
 
-static int mem_cgroup_soft_limit_tree_init(void)
+static void __init mem_cgroup_soft_limit_tree_init(void)
 {
 	struct mem_cgroup_tree_per_node *rtpn;
 	struct mem_cgroup_tree_per_zone *rtpz;
@@ -6239,8 +6239,7 @@ static int mem_cgroup_soft_limit_tree_init(void)
 		if (!node_state(node, N_NORMAL_MEMORY))
 			tmp = -1;
 		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL, tmp);
-		if (!rtpn)
-			goto err_cleanup;
+		BUG_ON(!rtpn);
 
 		soft_limit_tree.rb_tree_per_node[node] = rtpn;
 
@@ -6250,17 +6249,6 @@ static int mem_cgroup_soft_limit_tree_init(void)
 			spin_lock_init(&rtpz->lock);
 		}
 	}
-	return 0;
-
-err_cleanup:
-	for_each_node(node) {
-		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;
-
 }
 
 static struct cgroup_subsys_state * __ref
@@ -6282,8 +6270,6 @@ mem_cgroup_css_alloc(struct cgroup *cont)
 	if (cont->parent == NULL) {
 		int cpu;
 
-		if (mem_cgroup_soft_limit_tree_init())
-			goto free_out;
 		root_mem_cgroup = memcg;
 		for_each_possible_cpu(cpu) {
 			struct memcg_stock_pcp *stock =
@@ -7027,6 +7013,7 @@ static int __init mem_cgroup_init(void)
 {
 	hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
 	enable_swap_cgroup();
+	mem_cgroup_soft_limit_tree_init();
 	return 0;
 }
 subsys_initcall(mem_cgroup_init);
-- 
1.7.10.4

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/3] memcg: move mem_cgroup_soft_limit_tree_init to mem_cgroup_init

[Michal Hocko]

Per-node-zone soft limit tree is currently initialized when the root
cgroup is created which is OK but it pointlessly pollutes memcg
allocation code with something that can be called when the memcg
subsystem is initialized by mem_cgroup_init along with other controller
specific parts.

While we are at it let's make mem_cgroup_soft_limit_tree_init void
because it doesn't make much sense to report memory failure because if
we fail to allocate memory that early during the boot then we are
screwed anyway (this saves some code).

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c |   19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 2382fe9..b0d3339 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -6228,7 +6228,7 @@ struct mem_cgroup *parent_mem_cgroup(struct mem_cgroup *memcg)
 }
 EXPORT_SYMBOL(parent_mem_cgroup);
 
-static int mem_cgroup_soft_limit_tree_init(void)
+static void __init mem_cgroup_soft_limit_tree_init(void)
 {
 	struct mem_cgroup_tree_per_node *rtpn;
 	struct mem_cgroup_tree_per_zone *rtpz;
@@ -6239,8 +6239,7 @@ static int mem_cgroup_soft_limit_tree_init(void)
 		if (!node_state(node, N_NORMAL_MEMORY))
 			tmp = -1;
 		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL, tmp);
-		if (!rtpn)
-			goto err_cleanup;
+		BUG_ON(!rtpn);
 
 		soft_limit_tree.rb_tree_per_node[node] = rtpn;
 
@@ -6250,17 +6249,6 @@ static int mem_cgroup_soft_limit_tree_init(void)
 			spin_lock_init(&rtpz->lock);
 		}
 	}
-	return 0;
-
-err_cleanup:
-	for_each_node(node) {
-		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;
-
 }
 
 static struct cgroup_subsys_state * __ref
@@ -6282,8 +6270,6 @@ mem_cgroup_css_alloc(struct cgroup *cont)
 	if (cont->parent == NULL) {
 		int cpu;
 
-		if (mem_cgroup_soft_limit_tree_init())
-			goto free_out;
 		root_mem_cgroup = memcg;
 		for_each_possible_cpu(cpu) {
 			struct memcg_stock_pcp *stock =
@@ -7027,6 +7013,7 @@ static int __init mem_cgroup_init(void)
 {
 	hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
 	enable_swap_cgroup();
+	mem_cgroup_soft_limit_tree_init();
 	return 0;
 }
 subsys_initcall(mem_cgroup_init);
-- 
1.7.10.4

Re: [PATCH 1/3] memcg: move mem_cgroup_soft_limit_tree_init to mem_cgroup_init

[Johannes Weiner]

On Tue, Feb 05, 2013 at 05:23:59PM +0100, Michal Hocko wrote:
> Per-node-zone soft limit tree is currently initialized when the root
> cgroup is created which is OK but it pointlessly pollutes memcg
> allocation code with something that can be called when the memcg
> subsystem is initialized by mem_cgroup_init along with other controller
> specific parts.
> 
> While we are at it let's make mem_cgroup_soft_limit_tree_init void
> because it doesn't make much sense to report memory failure because if
> we fail to allocate memory that early during the boot then we are
> screwed anyway (this saves some code).
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/3] memcg: move mem_cgroup_soft_limit_tree_init to mem_cgroup_init

[Johannes Weiner]

On Tue, Feb 05, 2013 at 05:23:59PM +0100, Michal Hocko wrote:
> Per-node-zone soft limit tree is currently initialized when the root
> cgroup is created which is OK but it pointlessly pollutes memcg
> allocation code with something that can be called when the memcg
> subsystem is initialized by mem_cgroup_init along with other controller
> specific parts.
> 
> While we are at it let's make mem_cgroup_soft_limit_tree_init void
> because it doesn't make much sense to report memory failure because if
> we fail to allocate memory that early during the boot then we are
> screwed anyway (this saves some code).
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

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

[PATCH 2/3] memcg: move memcg_stock initialization to mem_cgroup_init

[Michal Hocko]

memcg_stock are currently initialized during the root cgroup allocation
which is OK but it pointlessly pollutes memcg allocation code with
something that can be called when the memcg subsystem is initialized by
mem_cgroup_init along with other controller specific parts.

This patch wrappes the current memcg_stock initialization code into a
helper calls it from the controller subsystem initialization code.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b0d3339..e9c1690 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2362,6 +2362,17 @@ static void drain_local_stock(struct work_struct *dummy)
 	clear_bit(FLUSHING_CACHED_CHARGE, &stock->flags);
 }
 
+static void __init memcg_stock_init(void)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		struct memcg_stock_pcp *stock =
+					&per_cpu(memcg_stock, cpu);
+		INIT_WORK(&stock->work, drain_local_stock);
+	}
+}
+
 /*
  * Cache charges(val) which is from res_counter, to local per_cpu area.
  * This will be consumed by consume_stock() function, later.
@@ -6268,15 +6279,7 @@ mem_cgroup_css_alloc(struct cgroup *cont)
 
 	/* root ? */
 	if (cont->parent == NULL) {
-		int cpu;
-
 		root_mem_cgroup = memcg;
-		for_each_possible_cpu(cpu) {
-			struct memcg_stock_pcp *stock =
-						&per_cpu(memcg_stock, cpu);
-			INIT_WORK(&stock->work, drain_local_stock);
-		}
-
 		res_counter_init(&memcg->res, NULL);
 		res_counter_init(&memcg->memsw, NULL);
 		res_counter_init(&memcg->kmem, NULL);
@@ -7014,6 +7017,7 @@ static int __init mem_cgroup_init(void)
 	hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
 	enable_swap_cgroup();
 	mem_cgroup_soft_limit_tree_init();
+	memcg_stock_init();
 	return 0;
 }
 subsys_initcall(mem_cgroup_init);
-- 
1.7.10.4

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/3] memcg: move memcg_stock initialization to mem_cgroup_init

[Michal Hocko]

memcg_stock are currently initialized during the root cgroup allocation
which is OK but it pointlessly pollutes memcg allocation code with
something that can be called when the memcg subsystem is initialized by
mem_cgroup_init along with other controller specific parts.

This patch wrappes the current memcg_stock initialization code into a
helper calls it from the controller subsystem initialization code.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b0d3339..e9c1690 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2362,6 +2362,17 @@ static void drain_local_stock(struct work_struct *dummy)
 	clear_bit(FLUSHING_CACHED_CHARGE, &stock->flags);
 }
 
+static void __init memcg_stock_init(void)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		struct memcg_stock_pcp *stock =
+					&per_cpu(memcg_stock, cpu);
+		INIT_WORK(&stock->work, drain_local_stock);
+	}
+}
+
 /*
  * Cache charges(val) which is from res_counter, to local per_cpu area.
  * This will be consumed by consume_stock() function, later.
@@ -6268,15 +6279,7 @@ mem_cgroup_css_alloc(struct cgroup *cont)
 
 	/* root ? */
 	if (cont->parent == NULL) {
-		int cpu;
-
 		root_mem_cgroup = memcg;
-		for_each_possible_cpu(cpu) {
-			struct memcg_stock_pcp *stock =
-						&per_cpu(memcg_stock, cpu);
-			INIT_WORK(&stock->work, drain_local_stock);
-		}
-
 		res_counter_init(&memcg->res, NULL);
 		res_counter_init(&memcg->memsw, NULL);
 		res_counter_init(&memcg->kmem, NULL);
@@ -7014,6 +7017,7 @@ static int __init mem_cgroup_init(void)
 	hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
 	enable_swap_cgroup();
 	mem_cgroup_soft_limit_tree_init();
+	memcg_stock_init();
 	return 0;
 }
 subsys_initcall(mem_cgroup_init);
-- 
1.7.10.4

Re: [PATCH 2/3] memcg: move memcg_stock initialization to mem_cgroup_init

[Johannes Weiner]

On Tue, Feb 05, 2013 at 05:24:00PM +0100, Michal Hocko wrote:
> memcg_stock are currently initialized during the root cgroup allocation
> which is OK but it pointlessly pollutes memcg allocation code with
> something that can be called when the memcg subsystem is initialized by
> mem_cgroup_init along with other controller specific parts.
> 
> This patch wrappes the current memcg_stock initialization code into a
> helper calls it from the controller subsystem initialization code.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/3] memcg: move memcg_stock initialization to mem_cgroup_init

[Johannes Weiner]

On Tue, Feb 05, 2013 at 05:24:00PM +0100, Michal Hocko wrote:
> memcg_stock are currently initialized during the root cgroup allocation
> which is OK but it pointlessly pollutes memcg allocation code with
> something that can be called when the memcg subsystem is initialized by
> mem_cgroup_init along with other controller specific parts.
> 
> This patch wrappes the current memcg_stock initialization code into a
> helper calls it from the controller subsystem initialization code.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

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

[PATCH 3/3] memcg: cleanup mem_cgroup_init comment

[Michal Hocko]

We should encourage all memcg controller initialization independent on
a specific mem_cgroup to be done here rather than exploit css_alloc
callback and assume that nothing happens before root cgroup is created.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index e9c1690..b97008c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -7007,10 +7007,12 @@ static void __init enable_swap_cgroup(void)
 #endif
 
 /*
- * The rest of init is performed during ->css_alloc() for root css which
- * happens before initcalls.  hotcpu_notifier() can't be done together as
- * it would introduce circular locking by adding cgroup_lock -> cpu hotplug
- * dependency.  Do it from a subsys_initcall().
+ * subsys_initcall() for memory controller.
+ *
+ * Some parts like hotcpu_notifier() have to be initialized from this context
+ * because of lock dependencies (cgroup_lock -> cpu hotplug) but basically
+ * everything that doesn't depend on a specific mem_cgroup structure should
+ * be initialized from here.
  */
 static int __init mem_cgroup_init(void)
 {
-- 
1.7.10.4

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/3] memcg: cleanup mem_cgroup_init comment

[Michal Hocko]

We should encourage all memcg controller initialization independent on
a specific mem_cgroup to be done here rather than exploit css_alloc
callback and assume that nothing happens before root cgroup is created.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index e9c1690..b97008c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -7007,10 +7007,12 @@ static void __init enable_swap_cgroup(void)
 #endif
 
 /*
- * The rest of init is performed during ->css_alloc() for root css which
- * happens before initcalls.  hotcpu_notifier() can't be done together as
- * it would introduce circular locking by adding cgroup_lock -> cpu hotplug
- * dependency.  Do it from a subsys_initcall().
+ * subsys_initcall() for memory controller.
+ *
+ * Some parts like hotcpu_notifier() have to be initialized from this context
+ * because of lock dependencies (cgroup_lock -> cpu hotplug) but basically
+ * everything that doesn't depend on a specific mem_cgroup structure should
+ * be initialized from here.
  */
 static int __init mem_cgroup_init(void)
 {
-- 
1.7.10.4

Re: [PATCH 3/3] memcg: cleanup mem_cgroup_init comment

[Johannes Weiner]

On Tue, Feb 05, 2013 at 05:24:01PM +0100, Michal Hocko wrote:
> We should encourage all memcg controller initialization independent on
> a specific mem_cgroup to be done here rather than exploit css_alloc
> callback and assume that nothing happens before root cgroup is created.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

It seems a little strange to document that the subsystem init function
should be used for initializing the subsystem.  But your new comment
is better than the old comment :-)

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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/3] memcg: cleanup mem_cgroup_init comment

[Johannes Weiner]

On Tue, Feb 05, 2013 at 05:24:01PM +0100, Michal Hocko wrote:
> We should encourage all memcg controller initialization independent on
> a specific mem_cgroup to be done here rather than exploit css_alloc
> callback and assume that nothing happens before root cgroup is created.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

It seems a little strange to document that the subsystem init function
should be used for initializing the subsystem.  But your new comment
is better than the old comment :-)

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

Re: [PATCH 0/3] cleanup memcg controller initialization

[Tejun Heo]

On Tue, Feb 05, 2013 at 05:23:58PM +0100, Michal Hocko wrote:
> Hi,
> this is just a small cleanup I promised some time ago[1]. It just moves
> all memcg controller initialization code independant on mem_cgroup into
> subsystem initialization code.
> 
> There are no functional changes.

Looks good to me.  Thanks for doing this. :)

-- 
tejun

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

Re: [PATCH 0/3] cleanup memcg controller initialization

[Tejun Heo]

On Tue, Feb 05, 2013 at 05:23:58PM +0100, Michal Hocko wrote:
> Hi,
> this is just a small cleanup I promised some time ago[1]. It just moves
> all memcg controller initialization code independant on mem_cgroup into
> subsystem initialization code.
> 
> There are no functional changes.

Looks good to me.  Thanks for doing this. :)

-- 
tejun