Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1586277 > unrolled thread

[PATCH 2/2] mm/cgroup: delay soft limit data allocation

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] mm/cgroup: delay soft limit data allocation Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2017-02-22 17:00 +0100
    Re: [PATCH 2/2] mm/cgroup: delay soft limit data allocation Michal Hocko <mhocko@kernel.org> - 2017-02-22 18:20 +0100
      Re: [PATCH 2/2] mm/cgroup: delay soft limit data allocation Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2017-02-22 19:00 +0100
        Re: [PATCH 2/2] mm/cgroup: delay soft limit data allocation Michal Hocko <mhocko@kernel.org> - 2017-02-22 19:30 +0100
          Re: [PATCH 2/2] mm/cgroup: delay soft limit data allocation Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2017-02-23 10:10 +0100
    Re: [PATCH 2/2] mm/cgroup: delay soft limit data allocation Balbir Singh <bsingharora@gmail.com> - 2017-02-23 02:30 +0100
      Re: [PATCH 2/2] mm/cgroup: delay soft limit data allocation Laurent Dufour <ldufour@linux.vnet.ibm.com> - 2017-02-23 11:50 +0100

#1586277 — [PATCH 2/2] mm/cgroup: delay soft limit data allocation

Until a soft limit is set to a cgroup, the soft limit data are useless
so delay this allocation when a limit is set.

Suggested-by: Michal Hocko <mhocko@kernel.org>
Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
---
 mm/memcontrol.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 228ac44f77e1..bc2e6ab69c0c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -142,6 +142,8 @@ struct mem_cgroup_tree {
 	struct mem_cgroup_tree_per_node *rb_tree_per_node[MAX_NUMNODES];
 };
 
+static DEFINE_MUTEX(soft_limit_mutex);
+static bool soft_limit_initialized;
 static struct mem_cgroup_tree soft_limit_tree __read_mostly;
 
 /* for OOM */
@@ -381,6 +383,36 @@ mem_cgroup_page_nodeinfo(struct mem_cgroup *memcg, struct page *page)
 	return memcg->nodeinfo[nid];
 }
 
+static void soft_limit_initialize(void)
+{
+	int node;
+
+	mutex_lock(&soft_limit_mutex);
+	if (soft_limit_initialized)
+		goto bail;
+
+	for_each_node(node) {
+		struct mem_cgroup_tree_per_node *rtpn;
+
+		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL,
+				    node_online(node) ? node : NUMA_NO_NODE);
+		/* Best effort, but should we warn if allocation failed */
+		if (rtpn) {
+			rtpn->rb_root = RB_ROOT;
+			spin_lock_init(&rtpn->lock);
+			/*
+			 * We don't want the compiler to set rb_tree_per_node
+			 * before rb_root and lock are initialized.
+			 */
+			WRITE_ONCE(soft_limit_tree.rb_tree_per_node[node],
+				   rtpn);
+		}
+	}
+	soft_limit_initialized = true;
+bail:
+	mutex_unlock(&soft_limit_mutex);
+}
+
 static struct mem_cgroup_tree_per_node *
 soft_limit_tree_node(int nid)
 {
@@ -465,6 +497,8 @@ static void mem_cgroup_update_tree(struct mem_cgroup *memcg, struct page *page)
 	struct mem_cgroup_tree_per_node *mctz;
 
 	mctz = soft_limit_tree_from_page(page);
+	if (!mctz)
+		return;
 	/*
 	 * Necessary to update all ancestors when hierarchy is used.
 	 * because their event counter is not touched.
@@ -502,7 +536,8 @@ static void mem_cgroup_remove_from_trees(struct mem_cgroup *memcg)
 	for_each_node(nid) {
 		mz = mem_cgroup_nodeinfo(memcg, nid);
 		mctz = soft_limit_tree_node(nid);
-		mem_cgroup_remove_exceeded(mz, mctz);
+		if (mctz)
+			mem_cgroup_remove_exceeded(mz, mctz);
 	}
 }
 
@@ -3000,6 +3035,8 @@ static ssize_t mem_cgroup_write(struct kernfs_open_file *of,
 		}
 		break;
 	case RES_SOFT_LIMIT:
+		if (!soft_limit_initialized)
+			soft_limit_initialize();
 		memcg->soft_limit = nr_pages;
 		ret = 0;
 		break;
@@ -5774,7 +5811,7 @@ __setup("cgroup.memory=", cgroup_memory);
  */
 static int __init mem_cgroup_init(void)
 {
-	int cpu, node;
+	int cpu;
 
 #ifndef CONFIG_SLOB
 	/*
@@ -5794,17 +5831,6 @@ static int __init mem_cgroup_init(void)
 		INIT_WORK(&per_cpu_ptr(&memcg_stock, cpu)->work,
 			  drain_local_stock);
 
-	for_each_node(node) {
-		struct mem_cgroup_tree_per_node *rtpn;
-
-		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL,
-				    node_online(node) ? node : NUMA_NO_NODE);
-
-		rtpn->rb_root = RB_ROOT;
-		spin_lock_init(&rtpn->lock);
-		soft_limit_tree.rb_tree_per_node[node] = rtpn;
-	}
-
 	return 0;
 }
 subsys_initcall(mem_cgroup_init);
-- 
2.7.4

[toc] | [next] | [standalone]

#1586326

On Wed 22-02-17 16:58:11, Laurent Dufour wrote:
[...]
>  static struct mem_cgroup_tree_per_node *
>  soft_limit_tree_node(int nid)
>  {
> @@ -465,6 +497,8 @@ static void mem_cgroup_update_tree(struct mem_cgroup *memcg, struct page *page)
>  	struct mem_cgroup_tree_per_node *mctz;
>  
>  	mctz = soft_limit_tree_from_page(page);
> +	if (!mctz)
> +		return;
>  	/*
>  	 * Necessary to update all ancestors when hierarchy is used.
>  	 * because their event counter is not touched.
> @@ -502,7 +536,8 @@ static void mem_cgroup_remove_from_trees(struct mem_cgroup *memcg)
>  	for_each_node(nid) {
>  		mz = mem_cgroup_nodeinfo(memcg, nid);
>  		mctz = soft_limit_tree_node(nid);
> -		mem_cgroup_remove_exceeded(mz, mctz);
> +		if (mctz)
> +			mem_cgroup_remove_exceeded(mz, mctz);
>  	}
>  }
>  

this belongs to the previous patch, right?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1586351

On 22/02/2017 18:11, Michal Hocko wrote:
> On Wed 22-02-17 16:58:11, Laurent Dufour wrote:
> [...]
>>  static struct mem_cgroup_tree_per_node *
>>  soft_limit_tree_node(int nid)
>>  {
>> @@ -465,6 +497,8 @@ static void mem_cgroup_update_tree(struct mem_cgroup *memcg, struct page *page)
>>  	struct mem_cgroup_tree_per_node *mctz;
>>  
>>  	mctz = soft_limit_tree_from_page(page);
>> +	if (!mctz)
>> +		return;
>>  	/*
>>  	 * Necessary to update all ancestors when hierarchy is used.
>>  	 * because their event counter is not touched.
>> @@ -502,7 +536,8 @@ static void mem_cgroup_remove_from_trees(struct mem_cgroup *memcg)
>>  	for_each_node(nid) {
>>  		mz = mem_cgroup_nodeinfo(memcg, nid);
>>  		mctz = soft_limit_tree_node(nid);
>> -		mem_cgroup_remove_exceeded(mz, mctz);
>> +		if (mctz)
>> +			mem_cgroup_remove_exceeded(mz, mctz);
>>  	}
>>  }
>>  
> 
> this belongs to the previous patch, right?

It may. I made the first patch fixing the panic I saw but if you prefer
this to be part of the first one, fair enough.
Tell me what you like.

[toc] | [prev] | [next] | [standalone]

#1586381

On Wed 22-02-17 18:50:19, Laurent Dufour wrote:
> On 22/02/2017 18:11, Michal Hocko wrote:
> > On Wed 22-02-17 16:58:11, Laurent Dufour wrote:
> > [...]
> >>  static struct mem_cgroup_tree_per_node *
> >>  soft_limit_tree_node(int nid)
> >>  {
> >> @@ -465,6 +497,8 @@ static void mem_cgroup_update_tree(struct mem_cgroup *memcg, struct page *page)
> >>  	struct mem_cgroup_tree_per_node *mctz;
> >>  
> >>  	mctz = soft_limit_tree_from_page(page);
> >> +	if (!mctz)
> >> +		return;
> >>  	/*
> >>  	 * Necessary to update all ancestors when hierarchy is used.
> >>  	 * because their event counter is not touched.
> >> @@ -502,7 +536,8 @@ static void mem_cgroup_remove_from_trees(struct mem_cgroup *memcg)
> >>  	for_each_node(nid) {
> >>  		mz = mem_cgroup_nodeinfo(memcg, nid);
> >>  		mctz = soft_limit_tree_node(nid);
> >> -		mem_cgroup_remove_exceeded(mz, mctz);
> >> +		if (mctz)
> >> +			mem_cgroup_remove_exceeded(mz, mctz);
> >>  	}
> >>  }
> >>  
> > 
> > this belongs to the previous patch, right?
> 
> It may. I made the first patch fixing the panic I saw but if you prefer
> this to be part of the first one, fair enough.

Without these you would just blow up later AFAICS so the fix is not
complete. Also this patch is not complete because the initialization
code should clean up if the allocation fails half way. I have tried to
do that and it blows the code size a bit. I am not convinced this is
worth the savings after all...

Here is what I ended up:
--- 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 44fb1e80701a..54d73c20124e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -141,7 +141,7 @@ struct mem_cgroup_tree {
 	struct mem_cgroup_tree_per_node *rb_tree_per_node[MAX_NUMNODES];
 };
 
-static struct mem_cgroup_tree soft_limit_tree __read_mostly;
+static struct mem_cgroup_tree *soft_limit_tree __read_mostly;
 
 /* for OOM */
 struct mem_cgroup_eventfd_list {
@@ -381,7 +381,9 @@ mem_cgroup_page_nodeinfo(struct mem_cgroup *memcg, struct page *page)
 static struct mem_cgroup_tree_per_node *
 soft_limit_tree_node(int nid)
 {
-	return soft_limit_tree.rb_tree_per_node[nid];
+	if (!soft_limit_tree_node)
+		return NULL;
+	return soft_limit_tree->rb_tree_per_node[nid];
 }
 
 static struct mem_cgroup_tree_per_node *
@@ -389,7 +391,9 @@ soft_limit_tree_from_page(struct page *page)
 {
 	int nid = page_to_nid(page);
 
-	return soft_limit_tree.rb_tree_per_node[nid];
+	if (!soft_limit_tree_node)
+		return NULL;
+	return soft_limit_tree->rb_tree_per_node[nid];
 }
 
 static void __mem_cgroup_insert_exceeded(struct mem_cgroup_per_node *mz,
@@ -2969,6 +2973,46 @@ static int memcg_update_tcp_limit(struct mem_cgroup *memcg, unsigned long limit)
 	return ret;
 }
 
+static bool soft_limit_initialize(void)
+{
+	static DEFINE_MUTEX(soft_limit_mutex);
+	struct mem_cgroup_tree *tree;
+	bool ret = true;
+	int node;
+
+	mutex_lock(&soft_limit_mutex);
+	if (soft_limit_tree)
+		goto out_unlock;
+
+	tree = kmalloc(sizeof(*soft_limit_tree), GFP_KERNEL);
+	if (!tree) {
+		ret = false;
+		goto out;
+	}
+	for_each_node(node) {
+		struct mem_cgroup_tree_per_node *rtpn;
+
+		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL,
+				    node_online(node) ? node : NUMA_NO_NODE);
+		if (!rtpn)
+			goto out_free;
+
+		rtpn->rb_root = RB_ROOT;
+		spin_lock_init(&rtpn->lock);
+		tree->rb_tree_per_node[node] = rtpn;
+	}
+	WRITE_ONCE(soft_limit_tree, tree);
+out_unlock:
+	mutex_unlock(&soft_limit_tree);
+	return ret;
+out_free:
+	for_each_node(node)
+		kfree(tree->rb_tree_per_node[node]);
+	kfree(tree);
+	ret = false;
+	goto out_unlock;
+}
+
 /*
  * The user of this function is...
  * RES_LIMIT.
@@ -3007,6 +3051,11 @@ static ssize_t mem_cgroup_write(struct kernfs_open_file *of,
 		}
 		break;
 	case RES_SOFT_LIMIT:
+		if (!soft_limit_initialize()) {
+			ret = -ENOMEM;
+			break;
+		}
+
 		memcg->soft_limit = nr_pages;
 		ret = 0;
 		break;
@@ -5800,17 +5849,6 @@ static int __init mem_cgroup_init(void)
 		INIT_WORK(&per_cpu_ptr(&memcg_stock, cpu)->work,
 			  drain_local_stock);
 
-	for_each_node(node) {
-		struct mem_cgroup_tree_per_node *rtpn;
-
-		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL,
-				    node_online(node) ? node : NUMA_NO_NODE);
-
-		rtpn->rb_root = RB_ROOT;
-		spin_lock_init(&rtpn->lock);
-		soft_limit_tree.rb_tree_per_node[node] = rtpn;
-	}
-
 	return 0;
 }
 subsys_initcall(mem_cgroup_init);
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1586759

On 22/02/2017 19:24, Michal Hocko wrote:
> On Wed 22-02-17 18:50:19, Laurent Dufour wrote:
>> On 22/02/2017 18:11, Michal Hocko wrote:
>>> On Wed 22-02-17 16:58:11, Laurent Dufour wrote:
>>> [...]
>>>>  static struct mem_cgroup_tree_per_node *
>>>>  soft_limit_tree_node(int nid)
>>>>  {
>>>> @@ -465,6 +497,8 @@ static void mem_cgroup_update_tree(struct mem_cgroup *memcg, struct page *page)
>>>>  	struct mem_cgroup_tree_per_node *mctz;
>>>>  
>>>>  	mctz = soft_limit_tree_from_page(page);
>>>> +	if (!mctz)
>>>> +		return;
>>>>  	/*
>>>>  	 * Necessary to update all ancestors when hierarchy is used.
>>>>  	 * because their event counter is not touched.
>>>> @@ -502,7 +536,8 @@ static void mem_cgroup_remove_from_trees(struct mem_cgroup *memcg)
>>>>  	for_each_node(nid) {
>>>>  		mz = mem_cgroup_nodeinfo(memcg, nid);
>>>>  		mctz = soft_limit_tree_node(nid);
>>>> -		mem_cgroup_remove_exceeded(mz, mctz);
>>>> +		if (mctz)
>>>> +			mem_cgroup_remove_exceeded(mz, mctz);
>>>>  	}
>>>>  }
>>>>  
>>>
>>> this belongs to the previous patch, right?
>>
>> It may. I made the first patch fixing the panic I saw but if you prefer
>> this to be part of the first one, fair enough.
> 
> Without these you would just blow up later AFAICS so the fix is not
> complete. Also this patch is not complete because the initialization
> code should clean up if the allocation fails half way. I have tried to
> do that and it blows the code size a bit. I am not convinced this is
> worth the savings after all...

I do agree, we will have more code than the data we don't want to allocate.

Bur your proposal sounds to be the cleanest way to handle that, despite
the larger size of the code.
I'll send a new series in that way.

> 
> Here is what I ended up:
> --- 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 44fb1e80701a..54d73c20124e 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -141,7 +141,7 @@ struct mem_cgroup_tree {
>  	struct mem_cgroup_tree_per_node *rb_tree_per_node[MAX_NUMNODES];
>  };
> 
> -static struct mem_cgroup_tree soft_limit_tree __read_mostly;
> +static struct mem_cgroup_tree *soft_limit_tree __read_mostly;
> 
>  /* for OOM */
>  struct mem_cgroup_eventfd_list {
> @@ -381,7 +381,9 @@ mem_cgroup_page_nodeinfo(struct mem_cgroup *memcg, struct page *page)
>  static struct mem_cgroup_tree_per_node *
>  soft_limit_tree_node(int nid)
>  {
> -	return soft_limit_tree.rb_tree_per_node[nid];
> +	if (!soft_limit_tree_node)
> +		return NULL;
> +	return soft_limit_tree->rb_tree_per_node[nid];
>  }
> 
>  static struct mem_cgroup_tree_per_node *
> @@ -389,7 +391,9 @@ soft_limit_tree_from_page(struct page *page)
>  {
>  	int nid = page_to_nid(page);
> 
> -	return soft_limit_tree.rb_tree_per_node[nid];
> +	if (!soft_limit_tree_node)
> +		return NULL;
> +	return soft_limit_tree->rb_tree_per_node[nid];
>  }
> 
>  static void __mem_cgroup_insert_exceeded(struct mem_cgroup_per_node *mz,
> @@ -2969,6 +2973,46 @@ static int memcg_update_tcp_limit(struct mem_cgroup *memcg, unsigned long limit)
>  	return ret;
>  }
> 
> +static bool soft_limit_initialize(void)
> +{
> +	static DEFINE_MUTEX(soft_limit_mutex);
> +	struct mem_cgroup_tree *tree;
> +	bool ret = true;
> +	int node;
> +
> +	mutex_lock(&soft_limit_mutex);
> +	if (soft_limit_tree)
> +		goto out_unlock;
> +
> +	tree = kmalloc(sizeof(*soft_limit_tree), GFP_KERNEL);
> +	if (!tree) {
> +		ret = false;
> +		goto out;
> +	}
> +	for_each_node(node) {
> +		struct mem_cgroup_tree_per_node *rtpn;
> +
> +		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL,
> +				    node_online(node) ? node : NUMA_NO_NODE);
> +		if (!rtpn)
> +			goto out_free;
> +
> +		rtpn->rb_root = RB_ROOT;
> +		spin_lock_init(&rtpn->lock);
> +		tree->rb_tree_per_node[node] = rtpn;
> +	}
> +	WRITE_ONCE(soft_limit_tree, tree);
> +out_unlock:
> +	mutex_unlock(&soft_limit_tree);
> +	return ret;
> +out_free:
> +	for_each_node(node)
> +		kfree(tree->rb_tree_per_node[node]);
> +	kfree(tree);
> +	ret = false;
> +	goto out_unlock;
> +}
> +
>  /*
>   * The user of this function is...
>   * RES_LIMIT.
> @@ -3007,6 +3051,11 @@ static ssize_t mem_cgroup_write(struct kernfs_open_file *of,
>  		}
>  		break;
>  	case RES_SOFT_LIMIT:
> +		if (!soft_limit_initialize()) {
> +			ret = -ENOMEM;
> +			break;
> +		}
> +
>  		memcg->soft_limit = nr_pages;
>  		ret = 0;
>  		break;
> @@ -5800,17 +5849,6 @@ static int __init mem_cgroup_init(void)
>  		INIT_WORK(&per_cpu_ptr(&memcg_stock, cpu)->work,
>  			  drain_local_stock);
> 
> -	for_each_node(node) {
> -		struct mem_cgroup_tree_per_node *rtpn;
> -
> -		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL,
> -				    node_online(node) ? node : NUMA_NO_NODE);
> -
> -		rtpn->rb_root = RB_ROOT;
> -		spin_lock_init(&rtpn->lock);
> -		soft_limit_tree.rb_tree_per_node[node] = rtpn;
> -	}
> -
>  	return 0;
>  }
>  subsys_initcall(mem_cgroup_init);
> 

[toc] | [prev] | [next] | [standalone]

#1586594

On Wed, Feb 22, 2017 at 04:58:11PM +0100, Laurent Dufour wrote:
> Until a soft limit is set to a cgroup, the soft limit data are useless
> so delay this allocation when a limit is set.
> 
> Suggested-by: Michal Hocko <mhocko@kernel.org>
> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> ---
<snip>
> @@ -3000,6 +3035,8 @@ static ssize_t mem_cgroup_write(struct kernfs_open_file *of,
>  		}
>  		break;
>  	case RES_SOFT_LIMIT:
> +		if (!soft_limit_initialized)
> +			soft_limit_initialize();

What happens if this fails? Do we disable this interface?
It's a good idea, but I wonder if we can deal with certain
memory cgroups not supporting soft limits due to memory
shortage at the time of using them.

>  		memcg->soft_limit = nr_pages;
>  		ret = 0;
>  		break;

Balbir Singh.

[toc] | [prev] | [next] | [standalone]

#1586809

On 23/02/2017 02:16, Balbir Singh wrote:
> On Wed, Feb 22, 2017 at 04:58:11PM +0100, Laurent Dufour wrote:
>> Until a soft limit is set to a cgroup, the soft limit data are useless
>> so delay this allocation when a limit is set.
>>
>> Suggested-by: Michal Hocko <mhocko@kernel.org>
>> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> ---
> <snip>
>> @@ -3000,6 +3035,8 @@ static ssize_t mem_cgroup_write(struct kernfs_open_file *of,
>>  		}
>>  		break;
>>  	case RES_SOFT_LIMIT:
>> +		if (!soft_limit_initialized)
>> +			soft_limit_initialize();
> 
> What happens if this fails? Do we disable this interface?
> It's a good idea, but I wonder if we can deal with certain
> memory cgroups not supporting soft limits due to memory
> shortage at the time of using them.

Thanks Balbir for the review.

Regarding this point, Michal sent a new proposal which will return
-ENOMEM in the case the initialization failed. I'll send a new series in
that way.

> 
>>  		memcg->soft_limit = nr_pages;
>>  		ret = 0;
>>  		break;
> 
> Balbir Singh.
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web