[trivial PATCH] Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim

[trivial PATCH] Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim

[Michal Hocko]

Hi,
while reading the code I have encountered the following thing. It is no
biggie but...
---
From: Michal Hocko <mhocko@suse.cz>
Subject: Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim

next_mz is assigned to NULL if __mem_cgroup_largest_soft_limit_node selects
the same mz. This doesn't make much sense as we assign to the variable
right in the next loop.

Compiler will probably optimize this out but it is little bit confusing for
the code reading.

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

Index: linux-2.6.38-rc8/mm/memcontrol.c
===================================================================
--- linux-2.6.38-rc8.orig/mm/memcontrol.c	2011-03-28 11:25:14.000000000 +0200
+++ linux-2.6.38-rc8/mm/memcontrol.c	2011-03-29 15:24:08.000000000 +0200
@@ -3349,7 +3349,6 @@ unsigned long mem_cgroup_soft_limit_recl
 				__mem_cgroup_largest_soft_limit_node(mctz);
 				if (next_mz == mz) {
 					css_put(&next_mz->mem->css);
-					next_mz = NULL;
 				} else /* next_mz == NULL or other memcg */
 					break;
 			} while (1);
-- 
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: [trivial PATCH] Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim

[Zhu Yanhai]

Michal,
IIUC it's to prevent the infinite loop, as in the end of the do-while
there's
if (!nr_reclaimed &&
    (next_mz == NULL ||
    loop > MEM_CGROUP_MAX_SOFT_LIMIT_RECLAIM_LOOPS))
		break;
so the loop will break earlier if all groups are iterated once and no
pages are freed.

Thanks,
Zhu Yanhai

2011/3/29 Michal Hocko <mhocko@suse.cz>:
> Hi,
> while reading the code I have encountered the following thing. It is no
> biggie but...
> ---
> From: Michal Hocko <mhocko@suse.cz>
> Subject: Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim
>
> next_mz is assigned to NULL if __mem_cgroup_largest_soft_limit_node selects
> the same mz. This doesn't make much sense as we assign to the variable
> right in the next loop.
>
> Compiler will probably optimize this out but it is little bit confusing for
> the code reading.
>
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
>
> Index: linux-2.6.38-rc8/mm/memcontrol.c
> ===================================================================
> --- linux-2.6.38-rc8.orig/mm/memcontrol.c       2011-03-28 11:25:14.000000000 +0200
> +++ linux-2.6.38-rc8/mm/memcontrol.c    2011-03-29 15:24:08.000000000 +0200
> @@ -3349,7 +3349,6 @@ unsigned long mem_cgroup_soft_limit_recl
>                                __mem_cgroup_largest_soft_limit_node(mctz);
>                                if (next_mz == mz) {
>                                        css_put(&next_mz->mem->css);
> -                                       next_mz = NULL;
>                                } else /* next_mz == NULL or other memcg */
>                                        break;
>                        } while (1);
> --
> 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>
>

--
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: [trivial PATCH] Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim

[Michal Hocko]

On Tue 29-03-11 21:40:13, Zhu Yanhai wrote:
> Michal,
> IIUC it's to prevent the infinite loop, as in the end of the do-while
> there's
> if (!nr_reclaimed &&
>     (next_mz == NULL ||
>     loop > MEM_CGROUP_MAX_SOFT_LIMIT_RECLAIM_LOOPS))
> 		break;

> so the loop will break earlier if all groups are iterated once and no
> pages are freed.

The code (in mmotm 2011-03-10-16-42) reads:
			do {
				[skipped comments]
                                next_mz =
                                __mem_cgroup_largest_soft_limit_node(mctz);
                                if (next_mz == mz) {
                                        css_put(&next_mz->mem->css);
					next_mz = NULL;
                                } else /* next_mz == NULL or other memcg */
                                        break;
                        } while (1);

So we do not break out of the loop and start a new iteration if next_mz == mz
and assign next_mz again.
Am I missing something?
-- 
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: [trivial PATCH] Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim

[Zhu Yanhai]

Got it. Sorry, my fault :)

-zyh

2011/3/29 Michal Hocko <mhocko@suse.cz>:
> On Tue 29-03-11 21:40:13, Zhu Yanhai wrote:
>> Michal,
>> IIUC it's to prevent the infinite loop, as in the end of the do-while
>> there's
>> if (!nr_reclaimed &&
>>     (next_mz == NULL ||
>>     loop > MEM_CGROUP_MAX_SOFT_LIMIT_RECLAIM_LOOPS))
>>               break;
>
>> so the loop will break earlier if all groups are iterated once and no
>> pages are freed.
>
> The code (in mmotm 2011-03-10-16-42) reads:
>                        do {
>                                [skipped comments]
>                                next_mz =
>                                __mem_cgroup_largest_soft_limit_node(mctz);
>                                if (next_mz == mz) {
>                                        css_put(&next_mz->mem->css);
>                                        next_mz = NULL;
>                                } else /* next_mz == NULL or other memcg */
>                                        break;
>                        } while (1);
>
> So we do not break out of the loop and start a new iteration if next_mz == mz
> and assign next_mz again.
> Am I missing something?
> --
> Michal Hocko
> SUSE Labs
> SUSE LINUX s.r.o.
> Lihovarska 1060/12
> 190 00 Praha 9
> Czech Republic
>

Re: [trivial PATCH] Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim

[Daisuke Nishimura]

On Tue, 29 Mar 2011 15:28:00 +0200
Michal Hocko <mhocko@suse.cz> wrote:

> Hi,
> while reading the code I have encountered the following thing. It is no
> biggie but...
> ---
> From: Michal Hocko <mhocko@suse.cz>
> Subject: Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim
> 
> next_mz is assigned to NULL if __mem_cgroup_largest_soft_limit_node selects
> the same mz. This doesn't make much sense as we assign to the variable
> right in the next loop.
> 
> Compiler will probably optimize this out but it is little bit confusing for
> the code reading.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> 
> Index: linux-2.6.38-rc8/mm/memcontrol.c
> ===================================================================
> --- linux-2.6.38-rc8.orig/mm/memcontrol.c	2011-03-28 11:25:14.000000000 +0200
> +++ linux-2.6.38-rc8/mm/memcontrol.c	2011-03-29 15:24:08.000000000 +0200
> @@ -3349,7 +3349,6 @@ unsigned long mem_cgroup_soft_limit_recl
>  				__mem_cgroup_largest_soft_limit_node(mctz);
>  				if (next_mz == mz) {
>  					css_put(&next_mz->mem->css);
> -					next_mz = NULL;
>  				} else /* next_mz == NULL or other memcg */
>  					break;
>  			} while (1);
hmm, make sense.

Can you remove the braces of the if-else statement too ?

Thanks,
Daisuke Nishimura.

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

[trivial PATCH v2] Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim

[Michal Hocko]

On Wed 30-03-11 11:09:53, Daisuke Nishimura wrote:
[...]
> > Index: linux-2.6.38-rc8/mm/memcontrol.c
> > ===================================================================
> > --- linux-2.6.38-rc8.orig/mm/memcontrol.c	2011-03-28 11:25:14.000000000 +0200
> > +++ linux-2.6.38-rc8/mm/memcontrol.c	2011-03-29 15:24:08.000000000 +0200
> > @@ -3349,7 +3349,6 @@ unsigned long mem_cgroup_soft_limit_recl
> >  				__mem_cgroup_largest_soft_limit_node(mctz);
> >  				if (next_mz == mz) {
> >  					css_put(&next_mz->mem->css);
> > -					next_mz = NULL;
> >  				} else /* next_mz == NULL or other memcg */
> >  					break;
> >  			} while (1);
> hmm, make sense.
> 
> Can you remove the braces of the if-else statement too ?

Sure, makes sense and the diff is even nicer because now we can see that
nezt_mz is assigned right before.
Thanks
--- 
From: Michal Hocko <mhocko@suse.cz>
Subject: Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim

next_mz is assigned to NULL if __mem_cgroup_largest_soft_limit_node selects
the same mz. This doesn't make much sense as we assign to the variable
right in the next loop.

Compiler will probably optimize this out but it is little bit confusing for
the code reading.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
Index: linux-2.6.38-rc8/mm/memcontrol.c
===================================================================
--- linux-2.6.38-rc8.orig/mm/memcontrol.c	2011-03-28 11:25:14.000000000 +0200
+++ linux-2.6.38-rc8/mm/memcontrol.c	2011-03-30 08:57:52.000000000 +0200
@@ -3347,10 +3347,9 @@ unsigned long mem_cgroup_soft_limit_recl
 				 */
 				next_mz =
 				__mem_cgroup_largest_soft_limit_node(mctz);
-				if (next_mz == mz) {
+				if (next_mz == mz)
 					css_put(&next_mz->mem->css);
-					next_mz = NULL;
-				} else /* next_mz == NULL or other memcg */
+				else /* next_mz == NULL or other memcg */
 					break;
 			} while (1);
 		}
-- 
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: [trivial PATCH v2] Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim

[Daisuke Nishimura]

> From: Michal Hocko <mhocko@suse.cz>
> Subject: Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim
> 
> next_mz is assigned to NULL if __mem_cgroup_largest_soft_limit_node selects
> the same mz. This doesn't make much sense as we assign to the variable
> right in the next loop.
> 
> Compiler will probably optimize this out but it is little bit confusing for
> the code reading.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

Acked-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>

--
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: [trivial PATCH v2] Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim

[Michal Hocko]

On Wed 30-03-11 16:18:00, Daisuke Nishimura wrote:
> > From: Michal Hocko <mhocko@suse.cz>
> > Subject: Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim
> > 
> > next_mz is assigned to NULL if __mem_cgroup_largest_soft_limit_node selects
> > the same mz. This doesn't make much sense as we assign to the variable
> > right in the next loop.
> > 
> > Compiler will probably optimize this out but it is little bit confusing for
> > the code reading.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> 
> Acked-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>

Thanks Daisuke.

Andrew, should this go though your mm tree?
--- 
From: Michal Hocko <mhocko@suse.cz>
Subject: Remove pointless next_mz nullification in mem_cgroup_soft_limit_reclaim

next_mz is assigned to NULL if __mem_cgroup_largest_soft_limit_node selects
the same mz. This doesn't make much sense as we assign to the variable
right in the next loop.

Compiler will probably optimize this out but it is little bit confusing for
the code reading.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
Acked-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
Index: linux-2.6.38-rc8/mm/memcontrol.c
===================================================================
--- linux-2.6.38-rc8.orig/mm/memcontrol.c	2011-03-28 11:25:14.000000000 +0200
+++ linux-2.6.38-rc8/mm/memcontrol.c	2011-03-30 08:57:52.000000000 +0200
@@ -3347,10 +3347,9 @@ unsigned long mem_cgroup_soft_limit_recl
 				 */
 				next_mz =
 				__mem_cgroup_largest_soft_limit_node(mctz);
-				if (next_mz == mz) {
+				if (next_mz == mz)
 					css_put(&next_mz->mem->css);
-					next_mz = NULL;
-				} else /* next_mz == NULL or other memcg */
+				else /* next_mz == NULL or other memcg */
 					break;
 			} while (1);
 		}
-- 
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>