[PATCH] memcg: refactor mem_cgroup_resize_limit()

[PATCH] memcg: refactor mem_cgroup_resize_limit()

[Yu Zhao]

mem_cgroup_resize_limit() and mem_cgroup_resize_memsw_limit() have
identical logics. Refactor code so we don't need to keep two pieces
of code that does same thing.

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 mm/memcontrol.c | 71 +++++++++------------------------------------------------
 1 file changed, 11 insertions(+), 60 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 94172089f52f..a4f0daaff704 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2422,13 +2422,14 @@ static inline int mem_cgroup_move_swap_account(swp_entry_t entry,
 static DEFINE_MUTEX(memcg_limit_mutex);
 
 static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
-				   unsigned long limit)
+				   unsigned long limit, bool memsw)
 {
 	unsigned long curusage;
 	unsigned long oldusage;
 	bool enlarge = false;
 	int retry_count;
 	int ret;
+	struct page_counter *counter = memsw ? &memcg->memsw : &memcg->memory;
 
 	/*
 	 * For keeping hierarchical_reclaim simple, how long we should retry
@@ -2438,58 +2439,7 @@ static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
 	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
 		      mem_cgroup_count_children(memcg);
 
-	oldusage = page_counter_read(&memcg->memory);
-
-	do {
-		if (signal_pending(current)) {
-			ret = -EINTR;
-			break;
-		}
-
-		mutex_lock(&memcg_limit_mutex);
-		if (limit > memcg->memsw.limit) {
-			mutex_unlock(&memcg_limit_mutex);
-			ret = -EINVAL;
-			break;
-		}
-		if (limit > memcg->memory.limit)
-			enlarge = true;
-		ret = page_counter_limit(&memcg->memory, limit);
-		mutex_unlock(&memcg_limit_mutex);
-
-		if (!ret)
-			break;
-
-		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, true);
-
-		curusage = page_counter_read(&memcg->memory);
-		/* Usage is reduced ? */
-		if (curusage >= oldusage)
-			retry_count--;
-		else
-			oldusage = curusage;
-	} while (retry_count);
-
-	if (!ret && enlarge)
-		memcg_oom_recover(memcg);
-
-	return ret;
-}
-
-static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
-					 unsigned long limit)
-{
-	unsigned long curusage;
-	unsigned long oldusage;
-	bool enlarge = false;
-	int retry_count;
-	int ret;
-
-	/* see mem_cgroup_resize_res_limit */
-	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
-		      mem_cgroup_count_children(memcg);
-
-	oldusage = page_counter_read(&memcg->memsw);
+	oldusage = page_counter_read(counter);
 
 	do {
 		if (signal_pending(current)) {
@@ -2498,22 +2448,23 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
 		}
 
 		mutex_lock(&memcg_limit_mutex);
-		if (limit < memcg->memory.limit) {
+		if (memsw ? limit < memcg->memory.limit :
+			    limit > memcg->memsw.limit) {
 			mutex_unlock(&memcg_limit_mutex);
 			ret = -EINVAL;
 			break;
 		}
-		if (limit > memcg->memsw.limit)
+		if (limit > counter->limit)
 			enlarge = true;
-		ret = page_counter_limit(&memcg->memsw, limit);
+		ret = page_counter_limit(counter, limit);
 		mutex_unlock(&memcg_limit_mutex);
 
 		if (!ret)
 			break;
 
-		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, false);
+		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, !memsw);
 
-		curusage = page_counter_read(&memcg->memsw);
+		curusage = page_counter_read(counter);
 		/* Usage is reduced ? */
 		if (curusage >= oldusage)
 			retry_count--;
@@ -2975,10 +2926,10 @@ static ssize_t mem_cgroup_write(struct kernfs_open_file *of,
 		}
 		switch (MEMFILE_TYPE(of_cft(of)->private)) {
 		case _MEM:
-			ret = mem_cgroup_resize_limit(memcg, nr_pages);
+			ret = mem_cgroup_resize_limit(memcg, nr_pages, false);
 			break;
 		case _MEMSWAP:
-			ret = mem_cgroup_resize_memsw_limit(memcg, nr_pages);
+			ret = mem_cgroup_resize_limit(memcg, nr_pages, true);
 			break;
 		case _KMEM:
 			ret = memcg_update_kmem_limit(memcg, nr_pages);
-- 
2.13.0.506.g27d5fe0cd-goog

--
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] memcg: refactor mem_cgroup_resize_limit()

[Nikolay Borisov]

On  2.06.2017 02:02, Yu Zhao wrote:
> mem_cgroup_resize_limit() and mem_cgroup_resize_memsw_limit() have
> identical logics. Refactor code so we don't need to keep two pieces
> of code that does same thing.
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> ---
>  mm/memcontrol.c | 71 +++++++++------------------------------------------------
>  1 file changed, 11 insertions(+), 60 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 94172089f52f..a4f0daaff704 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2422,13 +2422,14 @@ static inline int mem_cgroup_move_swap_account(swp_entry_t entry,
>  static DEFINE_MUTEX(memcg_limit_mutex);
>  
>  static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
> -				   unsigned long limit)
> +				   unsigned long limit, bool memsw)
>  {
>  	unsigned long curusage;
>  	unsigned long oldusage;
>  	bool enlarge = false;
>  	int retry_count;
>  	int ret;
> +	struct page_counter *counter = memsw ? &memcg->memsw : &memcg->memory;
>  
>  	/*
>  	 * For keeping hierarchical_reclaim simple, how long we should retry
> @@ -2438,58 +2439,7 @@ static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
>  	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
>  		      mem_cgroup_count_children(memcg);
>  
> -	oldusage = page_counter_read(&memcg->memory);
> -
> -	do {
> -		if (signal_pending(current)) {
> -			ret = -EINTR;
> -			break;
> -		}
> -
> -		mutex_lock(&memcg_limit_mutex);
> -		if (limit > memcg->memsw.limit) {
> -			mutex_unlock(&memcg_limit_mutex);
> -			ret = -EINVAL;
> -			break;
> -		}
> -		if (limit > memcg->memory.limit)
> -			enlarge = true;
> -		ret = page_counter_limit(&memcg->memory, limit);
> -		mutex_unlock(&memcg_limit_mutex);
> -
> -		if (!ret)
> -			break;
> -
> -		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, true);
> -
> -		curusage = page_counter_read(&memcg->memory);
> -		/* Usage is reduced ? */
> -		if (curusage >= oldusage)
> -			retry_count--;
> -		else
> -			oldusage = curusage;
> -	} while (retry_count);
> -
> -	if (!ret && enlarge)
> -		memcg_oom_recover(memcg);
> -
> -	return ret;
> -}
> -
> -static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
> -					 unsigned long limit)
> -{
> -	unsigned long curusage;
> -	unsigned long oldusage;
> -	bool enlarge = false;
> -	int retry_count;
> -	int ret;
> -
> -	/* see mem_cgroup_resize_res_limit */
> -	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
> -		      mem_cgroup_count_children(memcg);
> -
> -	oldusage = page_counter_read(&memcg->memsw);
> +	oldusage = page_counter_read(counter);
>  
>  	do {
>  		if (signal_pending(current)) {
> @@ -2498,22 +2448,23 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
>  		}
>  
>  		mutex_lock(&memcg_limit_mutex);
> -		if (limit < memcg->memory.limit) {
> +		if (memsw ? limit < memcg->memory.limit :
> +			    limit > memcg->memsw.limit) {

No, just no. Please createa a local variable and use that. Using the
ternary operator in an 'if' statement is just ugly!

>  			mutex_unlock(&memcg_limit_mutex);
>  			ret = -EINVAL;
>  			break;
>  		}
> -		if (limit > memcg->memsw.limit)
> +		if (limit > counter->limit)
>  			enlarge = true;
> -		ret = page_counter_limit(&memcg->memsw, limit);
> +		ret = page_counter_limit(counter, limit);
>  		mutex_unlock(&memcg_limit_mutex);
>  
>  		if (!ret)
>  			break;
>  
> -		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, false);
> +		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, !memsw);
>  
> -		curusage = page_counter_read(&memcg->memsw);
> +		curusage = page_counter_read(counter);
>  		/* Usage is reduced ? */
>  		if (curusage >= oldusage)
>  			retry_count--;
> @@ -2975,10 +2926,10 @@ static ssize_t mem_cgroup_write(struct kernfs_open_file *of,
>  		}
>  		switch (MEMFILE_TYPE(of_cft(of)->private)) {
>  		case _MEM:
> -			ret = mem_cgroup_resize_limit(memcg, nr_pages);
> +			ret = mem_cgroup_resize_limit(memcg, nr_pages, false);
>  			break;
>  		case _MEMSWAP:
> -			ret = mem_cgroup_resize_memsw_limit(memcg, nr_pages);
> +			ret = mem_cgroup_resize_limit(memcg, nr_pages, true);
>  			break;
>  		case _KMEM:
>  			ret = memcg_update_kmem_limit(memcg, nr_pages);
> 

--
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] memcg: refactor mem_cgroup_resize_limit()

[Vladimir Davydov]

On Thu, Jun 01, 2017 at 04:02:12PM -0700, Yu Zhao wrote:
> mem_cgroup_resize_limit() and mem_cgroup_resize_memsw_limit() have
> identical logics. Refactor code so we don't need to keep two pieces
> of code that does same thing.
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> ---
>  mm/memcontrol.c | 71 +++++++++------------------------------------------------
>  1 file changed, 11 insertions(+), 60 deletions(-)

Makes sense to me.

Acked-by: Vladimir Davydov <vdavydov.dev@gmail.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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] memcg: refactor mem_cgroup_resize_limit()

[Yu Zhao]

On Fri, Jun 02, 2017 at 10:32:52AM +0300, Nikolay Borisov wrote:
> 
> 
> On  2.06.2017 02:02, Yu Zhao wrote:
> > mem_cgroup_resize_limit() and mem_cgroup_resize_memsw_limit() have
> > identical logics. Refactor code so we don't need to keep two pieces
> > of code that does same thing.
> > 
> > Signed-off-by: Yu Zhao <yuzhao@google.com>
> > ---
> >  mm/memcontrol.c | 71 +++++++++------------------------------------------------
> >  1 file changed, 11 insertions(+), 60 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 94172089f52f..a4f0daaff704 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -2422,13 +2422,14 @@ static inline int mem_cgroup_move_swap_account(swp_entry_t entry,
> >  static DEFINE_MUTEX(memcg_limit_mutex);
> >  
> >  static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
> > -				   unsigned long limit)
> > +				   unsigned long limit, bool memsw)
> >  {
> >  	unsigned long curusage;
> >  	unsigned long oldusage;
> >  	bool enlarge = false;
> >  	int retry_count;
> >  	int ret;
> > +	struct page_counter *counter = memsw ? &memcg->memsw : &memcg->memory;
> >  
> >  	/*
> >  	 * For keeping hierarchical_reclaim simple, how long we should retry
> > @@ -2438,58 +2439,7 @@ static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
> >  	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
> >  		      mem_cgroup_count_children(memcg);
> >  
> > -	oldusage = page_counter_read(&memcg->memory);
> > -
> > -	do {
> > -		if (signal_pending(current)) {
> > -			ret = -EINTR;
> > -			break;
> > -		}
> > -
> > -		mutex_lock(&memcg_limit_mutex);
> > -		if (limit > memcg->memsw.limit) {
> > -			mutex_unlock(&memcg_limit_mutex);
> > -			ret = -EINVAL;
> > -			break;
> > -		}
> > -		if (limit > memcg->memory.limit)
> > -			enlarge = true;
> > -		ret = page_counter_limit(&memcg->memory, limit);
> > -		mutex_unlock(&memcg_limit_mutex);
> > -
> > -		if (!ret)
> > -			break;
> > -
> > -		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, true);
> > -
> > -		curusage = page_counter_read(&memcg->memory);
> > -		/* Usage is reduced ? */
> > -		if (curusage >= oldusage)
> > -			retry_count--;
> > -		else
> > -			oldusage = curusage;
> > -	} while (retry_count);
> > -
> > -	if (!ret && enlarge)
> > -		memcg_oom_recover(memcg);
> > -
> > -	return ret;
> > -}
> > -
> > -static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
> > -					 unsigned long limit)
> > -{
> > -	unsigned long curusage;
> > -	unsigned long oldusage;
> > -	bool enlarge = false;
> > -	int retry_count;
> > -	int ret;
> > -
> > -	/* see mem_cgroup_resize_res_limit */
> > -	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
> > -		      mem_cgroup_count_children(memcg);
> > -
> > -	oldusage = page_counter_read(&memcg->memsw);
> > +	oldusage = page_counter_read(counter);
> >  
> >  	do {
> >  		if (signal_pending(current)) {
> > @@ -2498,22 +2448,23 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
> >  		}
> >  
> >  		mutex_lock(&memcg_limit_mutex);
> > -		if (limit < memcg->memory.limit) {
> > +		if (memsw ? limit < memcg->memory.limit :
> > +			    limit > memcg->memsw.limit) {
> 
> No, just no. Please createa a local variable and use that. Using the
> ternary operator in an 'if' statement is just ugly!

Thanks. It is uncommon but seems no aesthetic difference to me. I'll
replace it with an extra variable and resend the patch.

--
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 v2] memcg: refactor mem_cgroup_resize_limit()

[Yu Zhao]

mem_cgroup_resize_limit() and mem_cgroup_resize_memsw_limit() have
identical logics. Refactor code so we don't need to keep two pieces
of code that does same thing.

Signed-off-by: Yu Zhao <yuzhao@google.com>
Acked-by: Vladimir Davydov <vdavydov.dev@gmail.com>
---
Changelog since v1:
* minor style change

 mm/memcontrol.c | 73 ++++++++++-----------------------------------------------
 1 file changed, 13 insertions(+), 60 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 94172089f52f..08a97a381b2a 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2422,13 +2422,15 @@ static inline int mem_cgroup_move_swap_account(swp_entry_t entry,
 static DEFINE_MUTEX(memcg_limit_mutex);
 
 static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
-				   unsigned long limit)
+				   unsigned long limit, bool memsw)
 {
 	unsigned long curusage;
 	unsigned long oldusage;
 	bool enlarge = false;
 	int retry_count;
 	int ret;
+	bool inverted;
+	struct page_counter *counter = memsw ? &memcg->memsw : &memcg->memory;
 
 	/*
 	 * For keeping hierarchical_reclaim simple, how long we should retry
@@ -2438,58 +2440,7 @@ static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
 	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
 		      mem_cgroup_count_children(memcg);
 
-	oldusage = page_counter_read(&memcg->memory);
-
-	do {
-		if (signal_pending(current)) {
-			ret = -EINTR;
-			break;
-		}
-
-		mutex_lock(&memcg_limit_mutex);
-		if (limit > memcg->memsw.limit) {
-			mutex_unlock(&memcg_limit_mutex);
-			ret = -EINVAL;
-			break;
-		}
-		if (limit > memcg->memory.limit)
-			enlarge = true;
-		ret = page_counter_limit(&memcg->memory, limit);
-		mutex_unlock(&memcg_limit_mutex);
-
-		if (!ret)
-			break;
-
-		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, true);
-
-		curusage = page_counter_read(&memcg->memory);
-		/* Usage is reduced ? */
-		if (curusage >= oldusage)
-			retry_count--;
-		else
-			oldusage = curusage;
-	} while (retry_count);
-
-	if (!ret && enlarge)
-		memcg_oom_recover(memcg);
-
-	return ret;
-}
-
-static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
-					 unsigned long limit)
-{
-	unsigned long curusage;
-	unsigned long oldusage;
-	bool enlarge = false;
-	int retry_count;
-	int ret;
-
-	/* see mem_cgroup_resize_res_limit */
-	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
-		      mem_cgroup_count_children(memcg);
-
-	oldusage = page_counter_read(&memcg->memsw);
+	oldusage = page_counter_read(counter);
 
 	do {
 		if (signal_pending(current)) {
@@ -2498,22 +2449,24 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
 		}
 
 		mutex_lock(&memcg_limit_mutex);
-		if (limit < memcg->memory.limit) {
+		inverted = memsw ? limit < memcg->memory.limit :
+				   limit > memcg->memsw.limit;
+		if (inverted)
 			mutex_unlock(&memcg_limit_mutex);
 			ret = -EINVAL;
 			break;
 		}
-		if (limit > memcg->memsw.limit)
+		if (limit > counter->limit)
 			enlarge = true;
-		ret = page_counter_limit(&memcg->memsw, limit);
+		ret = page_counter_limit(counter, limit);
 		mutex_unlock(&memcg_limit_mutex);
 
 		if (!ret)
 			break;
 
-		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, false);
+		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, !memsw);
 
-		curusage = page_counter_read(&memcg->memsw);
+		curusage = page_counter_read(counter);
 		/* Usage is reduced ? */
 		if (curusage >= oldusage)
 			retry_count--;
@@ -2975,10 +2928,10 @@ static ssize_t mem_cgroup_write(struct kernfs_open_file *of,
 		}
 		switch (MEMFILE_TYPE(of_cft(of)->private)) {
 		case _MEM:
-			ret = mem_cgroup_resize_limit(memcg, nr_pages);
+			ret = mem_cgroup_resize_limit(memcg, nr_pages, false);
 			break;
 		case _MEMSWAP:
-			ret = mem_cgroup_resize_memsw_limit(memcg, nr_pages);
+			ret = mem_cgroup_resize_limit(memcg, nr_pages, true);
 			break;
 		case _KMEM:
 			ret = memcg_update_kmem_limit(memcg, nr_pages);
-- 
2.13.0.506.g27d5fe0cd-goog

--
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 v2] memcg: refactor mem_cgroup_resize_limit()

[Vladimir Davydov]

On Sun, Jun 04, 2017 at 01:04:37PM -0700, Yu Zhao wrote:
> @@ -2498,22 +2449,24 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
>  		}
>  
>  		mutex_lock(&memcg_limit_mutex);
> -		if (limit < memcg->memory.limit) {
> +		inverted = memsw ? limit < memcg->memory.limit :
> +				   limit > memcg->memsw.limit;
> +		if (inverted)
>  			mutex_unlock(&memcg_limit_mutex);
>  			ret = -EINVAL;
>  			break;
>  		}

For some reason, I liked this patch more without this extra variable :-)

--
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 v2] memcg: refactor mem_cgroup_resize_limit()

[Yu Zhao]

On Sun, Jun 04, 2017 at 11:09:42PM +0300, Vladimir Davydov wrote:
> On Sun, Jun 04, 2017 at 01:04:37PM -0700, Yu Zhao wrote:
> > @@ -2498,22 +2449,24 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
> >  		}
> >  
> >  		mutex_lock(&memcg_limit_mutex);
> > -		if (limit < memcg->memory.limit) {
> > +		inverted = memsw ? limit < memcg->memory.limit :
> > +				   limit > memcg->memsw.limit;
> > +		if (inverted)
> >  			mutex_unlock(&memcg_limit_mutex);
> >  			ret = -EINVAL;
> >  			break;
> >  		}
> 
> For some reason, I liked this patch more without this extra variable :-)
Well, I'll refrain myself from commenting more because we are now at
the risk of starting a coding style war over this.

--
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 v2] memcg: refactor mem_cgroup_resize_limit()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5418 bytes --]

Hi Yu,

[auto build test ERROR on mmotm/master]
[also build test ERROR on v4.12-rc3 next-20170602]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Yu-Zhao/memcg-refactor-mem_cgroup_resize_limit/20170605-041444
base:   git://git.cmpxchg.org/linux-mmotm.git master
config: x86_64-randconfig-x002-201723 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   In file included from arch/x86/include/asm/atomic.h:4:0,
                    from include/linux/atomic.h:4,
                    from include/linux/page_counter.h:4,
                    from mm/memcontrol.c:34:
   mm/memcontrol.c: In function 'mem_cgroup_resize_limit':
   include/linux/compiler.h:156:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
     ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
>> mm/memcontrol.c:2453:3: note: in expansion of macro 'if'
      if (inverted)
      ^~
   mm/memcontrol.c:2455:4: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
       ret = -EINVAL;
       ^~~
   In file included from arch/x86/include/asm/atomic.h:4:0,
                    from include/linux/atomic.h:4,
                    from include/linux/page_counter.h:4,
                    from mm/memcontrol.c:34:
>> include/linux/compiler.h:156:2: error: expected 'while' before 'if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
     ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   mm/memcontrol.c:2458:3: note: in expansion of macro 'if'
      if (limit > counter->limit)
      ^~
>> include/linux/compiler.h:170:3: error: expected statement before ')' token
     }))
      ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   mm/memcontrol.c:2458:3: note: in expansion of macro 'if'
      if (limit > counter->limit)
      ^~
   include/linux/compiler.h:170:4: error: expected statement before ')' token
     }))
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   mm/memcontrol.c:2458:3: note: in expansion of macro 'if'
      if (limit > counter->limit)
      ^~
   mm/memcontrol.c:2464:4: error: break statement not within loop or switch
       break;
       ^~~~~
   mm/memcontrol.c:2474:2: warning: no return statement in function returning non-void [-Wreturn-type]
     } while (retry_count);
     ^
   mm/memcontrol.c: At top level:
   mm/memcontrol.c:2474:4: error: expected identifier or '(' before 'while'
     } while (retry_count);
       ^~~~~
   In file included from arch/x86/include/asm/atomic.h:4:0,
                    from include/linux/atomic.h:4,
                    from include/linux/page_counter.h:4,
                    from mm/memcontrol.c:34:
>> include/linux/compiler.h:156:2: error: expected identifier or '(' before 'if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
     ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   mm/memcontrol.c:2476:2: note: in expansion of macro 'if'
     if (!ret && enlarge)
     ^~
>> include/linux/compiler.h:170:3: error: expected identifier or '(' before ')' token
     }))
      ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   mm/memcontrol.c:2476:2: note: in expansion of macro 'if'
     if (!ret && enlarge)
     ^~
   mm/memcontrol.c:2479:2: error: expected identifier or '(' before 'return'
     return ret;
     ^~~~~~
   mm/memcontrol.c:2480:1: error: expected identifier or '(' before '}' token
    }
    ^

vim +/if +2453 mm/memcontrol.c

  2437		 * of # of children which we should visit in this loop.
  2438		 */
  2439		retry_count = MEM_CGROUP_RECLAIM_RETRIES *
  2440			      mem_cgroup_count_children(memcg);
  2441	
  2442		oldusage = page_counter_read(counter);
  2443	
  2444		do {
  2445			if (signal_pending(current)) {
  2446				ret = -EINTR;
  2447				break;
  2448			}
  2449	
  2450			mutex_lock(&memcg_limit_mutex);
  2451			inverted = memsw ? limit < memcg->memory.limit :
  2452					   limit > memcg->memsw.limit;
> 2453			if (inverted)
  2454				mutex_unlock(&memcg_limit_mutex);
  2455				ret = -EINVAL;
  2456				break;
  2457			}
  2458			if (limit > counter->limit)
  2459				enlarge = true;
  2460			ret = page_counter_limit(counter, limit);
  2461			mutex_unlock(&memcg_limit_mutex);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28905 bytes --]

Re: [PATCH v2] memcg: refactor mem_cgroup_resize_limit()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3957 bytes --]

Hi Yu,

[auto build test ERROR on mmotm/master]
[also build test ERROR on v4.12-rc3 next-20170602]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Yu-Zhao/memcg-refactor-mem_cgroup_resize_limit/20170605-041444
base:   git://git.cmpxchg.org/linux-mmotm.git master
config: x86_64-randconfig-x019-201723 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   mm/memcontrol.c: In function 'mem_cgroup_resize_limit':
>> mm/memcontrol.c:2453:3: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
      if (inverted)
      ^~
   mm/memcontrol.c:2455:4: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
       ret = -EINVAL;
       ^~~
>> mm/memcontrol.c:2458:3: error: expected 'while' before 'if'
      if (limit > counter->limit)
      ^~
>> mm/memcontrol.c:2464:4: error: break statement not within loop or switch
       break;
       ^~~~~
   mm/memcontrol.c:2428:7: warning: unused variable 'enlarge' [-Wunused-variable]
     bool enlarge = false;
          ^~~~~~~
>> mm/memcontrol.c:2474:2: warning: no return statement in function returning non-void [-Wreturn-type]
     } while (retry_count);
     ^
   mm/memcontrol.c: At top level:
>> mm/memcontrol.c:2474:4: error: expected identifier or '(' before 'while'
     } while (retry_count);
       ^~~~~
>> mm/memcontrol.c:2476:2: error: expected identifier or '(' before 'if'
     if (!ret && enlarge)
     ^~
>> mm/memcontrol.c:2479:2: error: expected identifier or '(' before 'return'
     return ret;
     ^~~~~~
>> mm/memcontrol.c:2480:1: error: expected identifier or '(' before '}' token
    }
    ^

vim +2458 mm/memcontrol.c

  2422	
  2423	static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
  2424					   unsigned long limit, bool memsw)
  2425	{
  2426		unsigned long curusage;
  2427		unsigned long oldusage;
> 2428		bool enlarge = false;
  2429		int retry_count;
  2430		int ret;
  2431		bool inverted;
  2432		struct page_counter *counter = memsw ? &memcg->memsw : &memcg->memory;
  2433	
  2434		/*
  2435		 * For keeping hierarchical_reclaim simple, how long we should retry
  2436		 * is depends on callers. We set our retry-count to be function
  2437		 * of # of children which we should visit in this loop.
  2438		 */
  2439		retry_count = MEM_CGROUP_RECLAIM_RETRIES *
  2440			      mem_cgroup_count_children(memcg);
  2441	
  2442		oldusage = page_counter_read(counter);
  2443	
  2444		do {
  2445			if (signal_pending(current)) {
  2446				ret = -EINTR;
  2447				break;
  2448			}
  2449	
  2450			mutex_lock(&memcg_limit_mutex);
  2451			inverted = memsw ? limit < memcg->memory.limit :
  2452					   limit > memcg->memsw.limit;
> 2453			if (inverted)
  2454				mutex_unlock(&memcg_limit_mutex);
  2455				ret = -EINVAL;
  2456				break;
  2457			}
> 2458			if (limit > counter->limit)
  2459				enlarge = true;
  2460			ret = page_counter_limit(counter, limit);
  2461			mutex_unlock(&memcg_limit_mutex);
  2462	
  2463			if (!ret)
> 2464				break;
  2465	
  2466			try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, !memsw);
  2467	
  2468			curusage = page_counter_read(counter);
  2469			/* Usage is reduced ? */
  2470			if (curusage >= oldusage)
  2471				retry_count--;
  2472			else
  2473				oldusage = curusage;
> 2474		} while (retry_count);
  2475	
> 2476		if (!ret && enlarge)
  2477			memcg_oom_recover(memcg);
  2478	
> 2479		return ret;
> 2480	}
  2481	
  2482	unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
  2483						    gfp_t gfp_mask,

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31818 bytes --]

[PATCH v3] memcg: refactor mem_cgroup_resize_limit()

[Yu Zhao]

mem_cgroup_resize_limit() and mem_cgroup_resize_memsw_limit() have
identical logics. Refactor code so we don't need to keep two pieces
of code that does same thing.

Signed-off-by: Yu Zhao <yuzhao@google.com>
Acked-by: Vladimir Davydov <vdavydov.dev@gmail.com>
---
Changelog since v1:
* minor style change
Changelog since v2:
* fix build error

 mm/memcontrol.c | 73 ++++++++++-----------------------------------------------
 1 file changed, 13 insertions(+), 60 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 94172089f52f..08a97a381b2a 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2422,13 +2422,15 @@ static inline int mem_cgroup_move_swap_account(swp_entry_t entry,
 static DEFINE_MUTEX(memcg_limit_mutex);
 
 static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
-				   unsigned long limit)
+				   unsigned long limit, bool memsw)
 {
 	unsigned long curusage;
 	unsigned long oldusage;
 	bool enlarge = false;
 	int retry_count;
 	int ret;
+	bool inverted;
+	struct page_counter *counter = memsw ? &memcg->memsw : &memcg->memory;
 
 	/*
 	 * For keeping hierarchical_reclaim simple, how long we should retry
@@ -2438,58 +2440,7 @@ static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
 	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
 		      mem_cgroup_count_children(memcg);
 
-	oldusage = page_counter_read(&memcg->memory);
-
-	do {
-		if (signal_pending(current)) {
-			ret = -EINTR;
-			break;
-		}
-
-		mutex_lock(&memcg_limit_mutex);
-		if (limit > memcg->memsw.limit) {
-			mutex_unlock(&memcg_limit_mutex);
-			ret = -EINVAL;
-			break;
-		}
-		if (limit > memcg->memory.limit)
-			enlarge = true;
-		ret = page_counter_limit(&memcg->memory, limit);
-		mutex_unlock(&memcg_limit_mutex);
-
-		if (!ret)
-			break;
-
-		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, true);
-
-		curusage = page_counter_read(&memcg->memory);
-		/* Usage is reduced ? */
-		if (curusage >= oldusage)
-			retry_count--;
-		else
-			oldusage = curusage;
-	} while (retry_count);
-
-	if (!ret && enlarge)
-		memcg_oom_recover(memcg);
-
-	return ret;
-}
-
-static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
-					 unsigned long limit)
-{
-	unsigned long curusage;
-	unsigned long oldusage;
-	bool enlarge = false;
-	int retry_count;
-	int ret;
-
-	/* see mem_cgroup_resize_res_limit */
-	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
-		      mem_cgroup_count_children(memcg);
-
-	oldusage = page_counter_read(&memcg->memsw);
+	oldusage = page_counter_read(counter);
 
 	do {
 		if (signal_pending(current)) {
@@ -2498,22 +2449,24 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
 		}
 
 		mutex_lock(&memcg_limit_mutex);
-		if (limit < memcg->memory.limit) {
+		inverted = memsw ? limit < memcg->memory.limit :
+				   limit > memcg->memsw.limit;
+		if (inverted) {
 			mutex_unlock(&memcg_limit_mutex);
 			ret = -EINVAL;
 			break;
 		}
-		if (limit > memcg->memsw.limit)
+		if (limit > counter->limit)
 			enlarge = true;
-		ret = page_counter_limit(&memcg->memsw, limit);
+		ret = page_counter_limit(counter, limit);
 		mutex_unlock(&memcg_limit_mutex);
 
 		if (!ret)
 			break;
 
-		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, false);
+		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, !memsw);
 
-		curusage = page_counter_read(&memcg->memsw);
+		curusage = page_counter_read(counter);
 		/* Usage is reduced ? */
 		if (curusage >= oldusage)
 			retry_count--;
@@ -2975,10 +2928,10 @@ static ssize_t mem_cgroup_write(struct kernfs_open_file *of,
 		}
 		switch (MEMFILE_TYPE(of_cft(of)->private)) {
 		case _MEM:
-			ret = mem_cgroup_resize_limit(memcg, nr_pages);
+			ret = mem_cgroup_resize_limit(memcg, nr_pages, false);
 			break;
 		case _MEMSWAP:
-			ret = mem_cgroup_resize_memsw_limit(memcg, nr_pages);
+			ret = mem_cgroup_resize_limit(memcg, nr_pages, true);
 			break;
 		case _KMEM:
 			ret = memcg_update_kmem_limit(memcg, nr_pages);
-- 
2.13.0.506.g27d5fe0cd-goog

--
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 v3] memcg: refactor mem_cgroup_resize_limit()

[Michal Hocko]

[Sorry for a late reponse]

On Sun 04-06-17 14:18:07, Yu Zhao wrote:
> mem_cgroup_resize_limit() and mem_cgroup_resize_memsw_limit() have
> identical logics. Refactor code so we don't need to keep two pieces
> of code that does same thing.
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> Acked-by: Vladimir Davydov <vdavydov.dev@gmail.com>

It is nice to see removal of the code duplication. I have one comment
though

[...]

> @@ -2498,22 +2449,24 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
>  		}
>  
>  		mutex_lock(&memcg_limit_mutex);
> -		if (limit < memcg->memory.limit) {
> +		inverted = memsw ? limit < memcg->memory.limit :
> +				   limit > memcg->memsw.limit;
> +		if (inverted) {
>  			mutex_unlock(&memcg_limit_mutex);
>  			ret = -EINVAL;
>  			break;
>  		}

This is just too ugly and hard to understand. inverted just doesn't give
you a good clue what is going on. What do you think about something like

		/*
		 * Make sure that the new limit (memsw or hard limit) doesn't
		 * break our basic invariant that memory.limit <= memsw.limit
		 */
		limits_invariant = memsw ? limit >= memcg->memory.limit :
					limit <= mmecg->memsw.limit;
		if (!limits_invariant) {
			mutex_unlock(&memcg_limit_mutex);
			ret = -EINVAL;
			break;
		}

with that feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>
-- 
Michal Hocko
SUSE Labs

--
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 v4] memcg: refactor mem_cgroup_resize_limit()

[Yu Zhao]

mem_cgroup_resize_limit() and mem_cgroup_resize_memsw_limit() have
identical logics. Refactor code so we don't need to keep two pieces
of code that does same thing.

Signed-off-by: Yu Zhao <yuzhao@google.com>
Acked-by: Vladimir Davydov <vdavydov.dev@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>
---
Changelog since v1:
* minor style change
Changelog since v2:
* fix build error
Changelog since v3:
* minor style change

 mm/memcontrol.c | 77 +++++++++++++--------------------------------------------
 1 file changed, 17 insertions(+), 60 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 94172089f52f..401f64a3dda1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2422,13 +2422,15 @@ static inline int mem_cgroup_move_swap_account(swp_entry_t entry,
 static DEFINE_MUTEX(memcg_limit_mutex);
 
 static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
-				   unsigned long limit)
+				   unsigned long limit, bool memsw)
 {
 	unsigned long curusage;
 	unsigned long oldusage;
 	bool enlarge = false;
 	int retry_count;
 	int ret;
+	bool limits_invariant;
+	struct page_counter *counter = memsw ? &memcg->memsw : &memcg->memory;
 
 	/*
 	 * For keeping hierarchical_reclaim simple, how long we should retry
@@ -2438,7 +2440,7 @@ static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
 	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
 		      mem_cgroup_count_children(memcg);
 
-	oldusage = page_counter_read(&memcg->memory);
+	oldusage = page_counter_read(counter);
 
 	do {
 		if (signal_pending(current)) {
@@ -2447,73 +2449,28 @@ static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
 		}
 
 		mutex_lock(&memcg_limit_mutex);
-		if (limit > memcg->memsw.limit) {
-			mutex_unlock(&memcg_limit_mutex);
-			ret = -EINVAL;
-			break;
-		}
-		if (limit > memcg->memory.limit)
-			enlarge = true;
-		ret = page_counter_limit(&memcg->memory, limit);
-		mutex_unlock(&memcg_limit_mutex);
-
-		if (!ret)
-			break;
-
-		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, true);
-
-		curusage = page_counter_read(&memcg->memory);
-		/* Usage is reduced ? */
-		if (curusage >= oldusage)
-			retry_count--;
-		else
-			oldusage = curusage;
-	} while (retry_count);
-
-	if (!ret && enlarge)
-		memcg_oom_recover(memcg);
-
-	return ret;
-}
-
-static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
-					 unsigned long limit)
-{
-	unsigned long curusage;
-	unsigned long oldusage;
-	bool enlarge = false;
-	int retry_count;
-	int ret;
-
-	/* see mem_cgroup_resize_res_limit */
-	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
-		      mem_cgroup_count_children(memcg);
-
-	oldusage = page_counter_read(&memcg->memsw);
-
-	do {
-		if (signal_pending(current)) {
-			ret = -EINTR;
-			break;
-		}
-
-		mutex_lock(&memcg_limit_mutex);
-		if (limit < memcg->memory.limit) {
+		/*
+		 * Make sure that the new limit (memsw or memory limit) doesn't
+		 * break our basic invariant rule memory.limit <= memsw.limit.
+		 */
+		limits_invariant = memsw ? limit >= memcg->memory.limit :
+					   limit <= memcg->memsw.limit;
+		if (!limits_invariant) {
 			mutex_unlock(&memcg_limit_mutex);
 			ret = -EINVAL;
 			break;
 		}
-		if (limit > memcg->memsw.limit)
+		if (limit > counter->limit)
 			enlarge = true;
-		ret = page_counter_limit(&memcg->memsw, limit);
+		ret = page_counter_limit(counter, limit);
 		mutex_unlock(&memcg_limit_mutex);
 
 		if (!ret)
 			break;
 
-		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, false);
+		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, !memsw);
 
-		curusage = page_counter_read(&memcg->memsw);
+		curusage = page_counter_read(counter);
 		/* Usage is reduced ? */
 		if (curusage >= oldusage)
 			retry_count--;
@@ -2975,10 +2932,10 @@ static ssize_t mem_cgroup_write(struct kernfs_open_file *of,
 		}
 		switch (MEMFILE_TYPE(of_cft(of)->private)) {
 		case _MEM:
-			ret = mem_cgroup_resize_limit(memcg, nr_pages);
+			ret = mem_cgroup_resize_limit(memcg, nr_pages, false);
 			break;
 		case _MEMSWAP:
-			ret = mem_cgroup_resize_memsw_limit(memcg, nr_pages);
+			ret = mem_cgroup_resize_limit(memcg, nr_pages, true);
 			break;
 		case _KMEM:
 			ret = memcg_update_kmem_limit(memcg, nr_pages);
-- 
2.13.1.508.gb3defc5cc-goog

--
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 v4] memcg: refactor mem_cgroup_resize_limit()

[Yu Zhao]

mem_cgroup_resize_limit() and mem_cgroup_resize_memsw_limit() have
identical logics. Refactor code so we don't need to keep two pieces
of code that does same thing.

Signed-off-by: Yu Zhao <yuzhao@google.com>
Acked-by: Vladimir Davydov <vdavydov.dev@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>
---
 mm/memcontrol.c | 77 +++++++++++++--------------------------------------------
 1 file changed, 17 insertions(+), 60 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ac2ffd5e02b9..9af733fa9381 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2467,13 +2467,15 @@ static inline int mem_cgroup_move_swap_account(swp_entry_t entry,
 static DEFINE_MUTEX(memcg_limit_mutex);
 
 static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
-				   unsigned long limit)
+				   unsigned long limit, bool memsw)
 {
 	unsigned long curusage;
 	unsigned long oldusage;
 	bool enlarge = false;
 	int retry_count;
 	int ret;
+	bool limits_invariant;
+	struct page_counter *counter = memsw ? &memcg->memsw : &memcg->memory;
 
 	/*
 	 * For keeping hierarchical_reclaim simple, how long we should retry
@@ -2483,7 +2485,7 @@ static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
 	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
 		      mem_cgroup_count_children(memcg);
 
-	oldusage = page_counter_read(&memcg->memory);
+	oldusage = page_counter_read(counter);
 
 	do {
 		if (signal_pending(current)) {
@@ -2492,73 +2494,28 @@ static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
 		}
 
 		mutex_lock(&memcg_limit_mutex);
-		if (limit > memcg->memsw.limit) {
-			mutex_unlock(&memcg_limit_mutex);
-			ret = -EINVAL;
-			break;
-		}
-		if (limit > memcg->memory.limit)
-			enlarge = true;
-		ret = page_counter_limit(&memcg->memory, limit);
-		mutex_unlock(&memcg_limit_mutex);
-
-		if (!ret)
-			break;
-
-		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, true);
-
-		curusage = page_counter_read(&memcg->memory);
-		/* Usage is reduced ? */
-		if (curusage >= oldusage)
-			retry_count--;
-		else
-			oldusage = curusage;
-	} while (retry_count);
-
-	if (!ret && enlarge)
-		memcg_oom_recover(memcg);
-
-	return ret;
-}
-
-static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
-					 unsigned long limit)
-{
-	unsigned long curusage;
-	unsigned long oldusage;
-	bool enlarge = false;
-	int retry_count;
-	int ret;
-
-	/* see mem_cgroup_resize_res_limit */
-	retry_count = MEM_CGROUP_RECLAIM_RETRIES *
-		      mem_cgroup_count_children(memcg);
-
-	oldusage = page_counter_read(&memcg->memsw);
-
-	do {
-		if (signal_pending(current)) {
-			ret = -EINTR;
-			break;
-		}
-
-		mutex_lock(&memcg_limit_mutex);
-		if (limit < memcg->memory.limit) {
+		/*
+		 * Make sure that the new limit (memsw or memory limit) doesn't
+		 * break our basic invariant rule memory.limit <= memsw.limit.
+		 */
+		limits_invariant = memsw ? limit >= memcg->memory.limit :
+					   limit <= memcg->memsw.limit;
+		if (!limits_invariant) {
 			mutex_unlock(&memcg_limit_mutex);
 			ret = -EINVAL;
 			break;
 		}
-		if (limit > memcg->memsw.limit)
+		if (limit > counter->limit)
 			enlarge = true;
-		ret = page_counter_limit(&memcg->memsw, limit);
+		ret = page_counter_limit(counter, limit);
 		mutex_unlock(&memcg_limit_mutex);
 
 		if (!ret)
 			break;
 
-		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, false);
+		try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL, !memsw);
 
-		curusage = page_counter_read(&memcg->memsw);
+		curusage = page_counter_read(counter);
 		/* Usage is reduced ? */
 		if (curusage >= oldusage)
 			retry_count--;
@@ -3020,10 +2977,10 @@ static ssize_t mem_cgroup_write(struct kernfs_open_file *of,
 		}
 		switch (MEMFILE_TYPE(of_cft(of)->private)) {
 		case _MEM:
-			ret = mem_cgroup_resize_limit(memcg, nr_pages);
+			ret = mem_cgroup_resize_limit(memcg, nr_pages, false);
 			break;
 		case _MEMSWAP:
-			ret = mem_cgroup_resize_memsw_limit(memcg, nr_pages);
+			ret = mem_cgroup_resize_limit(memcg, nr_pages, true);
 			break;
 		case _KMEM:
 			ret = memcg_update_kmem_limit(memcg, nr_pages);
-- 
2.16.0.rc0.223.g4a4ac83678-goog

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