[patch 3/3]vmscan: cleanup kswapd_try_to_sleep

[patch 3/3]vmscan: cleanup kswapd_try_to_sleep

[Shaohua Li]

cleanup kswapd_try_to_sleep() a little bit. Sometimes kswapd doesn't
really sleep. In such case, don't call prepare_to_wait/finish_wait.
It just wastes CPU.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
 mm/vmscan.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Index: linux/mm/vmscan.c
===================================================================
--- linux.orig/mm/vmscan.c	2011-07-28 15:52:35.000000000 +0800
+++ linux/mm/vmscan.c	2011-07-28 15:55:56.000000000 +0800
@@ -2709,13 +2709,11 @@ static void kswapd_try_to_sleep(pg_data_
 	if (freezing(current) || kthread_should_stop())
 		return;
 
-	prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
-
 	/* Try to sleep for a short interval */
 	if (!sleeping_prematurely(pgdat, order, remaining, classzone_idx)) {
+		prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
 		remaining = schedule_timeout(HZ/10);
 		finish_wait(&pgdat->kswapd_wait, &wait);
-		prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
 	}
 
 	/*
@@ -2734,7 +2732,9 @@ static void kswapd_try_to_sleep(pg_data_
 		 * them before going back to sleep.
 		 */
 		set_pgdat_percpu_threshold(pgdat, calculate_normal_threshold);
+		prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
 		schedule();
+		finish_wait(&pgdat->kswapd_wait, &wait);
 		set_pgdat_percpu_threshold(pgdat, calculate_pressure_threshold);
 	} else {
 		if (remaining)
@@ -2742,7 +2742,6 @@ static void kswapd_try_to_sleep(pg_data_
 		else
 			count_vm_event(KSWAPD_HIGH_WMARK_HIT_QUICKLY);
 	}
-	finish_wait(&pgdat->kswapd_wait, &wait);
 }
 
 /*


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

Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep

[Mel Gorman]

On Thu, Jul 28, 2011 at 04:13:09PM +0800, Shaohua Li wrote:
> cleanup kswapd_try_to_sleep() a little bit. Sometimes kswapd doesn't
> really sleep. In such case, don't call prepare_to_wait/finish_wait.
> It just wastes CPU.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>

Out of curiousity, under what circumstances is the CPU usage
noticeable? The patch itself seems fine so;

Acked-by: Mel Gorman <mgorman@suse.de>

-- 
Mel Gorman
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep

[Shaohua Li]

On Thu, 2011-07-28 at 19:03 +0800, Mel Gorman wrote:
> On Thu, Jul 28, 2011 at 04:13:09PM +0800, Shaohua Li wrote:
> > cleanup kswapd_try_to_sleep() a little bit. Sometimes kswapd doesn't
> > really sleep. In such case, don't call prepare_to_wait/finish_wait.
> > It just wastes CPU.
> > 
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> 
> Out of curiousity, under what circumstances is the CPU usage
> noticeable? The patch itself seems fine so;
no, as the title says, this is a cleanup.

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

Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep

[Minchan Kim]

On Thu, Jul 28, 2011 at 04:13:09PM +0800, Shaohua Li wrote:
> cleanup kswapd_try_to_sleep() a little bit. Sometimes kswapd doesn't
> really sleep. In such case, don't call prepare_to_wait/finish_wait.
> It just wastes CPU.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>

And it increases just code size a little bit even without big benefit of CPU.
You said it's cleanup but I doubt how it helps readability.
So code itself dosn't have a problem but I don't like it.

barrios@barrios-desktop:~/linux-mmotm$ size mm/vmscan.o.old
   text	   data	    bss	    dec	    hex	filename
  10271	     30	      8	  10309	   2845	mm/vmscan.o.old
barrios@barrios-desktop:~/linux-mmotm$ size mm/vmscan.o
   text	   data	    bss	    dec	    hex	filename
  10287	     30	      8	  10325	   2855	mm/vmscan.o

-- 
Kind regards,
Minchan Kim

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

Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep

[Shaohua Li]

On Fri, Jul 29, 2011 at 05:39:29PM +0800, Minchan Kim wrote:
> On Thu, Jul 28, 2011 at 04:13:09PM +0800, Shaohua Li wrote:
> > cleanup kswapd_try_to_sleep() a little bit. Sometimes kswapd doesn't
> > really sleep. In such case, don't call prepare_to_wait/finish_wait.
> > It just wastes CPU.
> > 
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> 
> And it increases just code size a little bit even without big benefit of CPU.
> You said it's cleanup but I doubt how it helps readability.
> So code itself dosn't have a problem but I don't like it.
> 
> barrios@barrios-desktop:~/linux-mmotm$ size mm/vmscan.o.old
>    text	   data	    bss	    dec	    hex	filename
>   10271	     30	      8	  10309	   2845	mm/vmscan.o.old
> barrios@barrios-desktop:~/linux-mmotm$ size mm/vmscan.o
>    text	   data	    bss	    dec	    hex	filename
>   10287	     30	      8	  10325	   2855	mm/vmscan.o
I'm curious why the size is increased, the patch doesn't add new code.
Maybe gcc has different optimization.
This hasn't big benefit for sure, but both prepare_to_wait/finish_wait
use spinlock, it's expensive operation even without contention. From
this point view, it has benefit because we don't blindly call them.
But anyway, it's a trival patch. I'm fine if it's rejected.

Thanks,
Shaohua

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

Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep

[KOSAKI Motohiro]

(2011/07/28 17:13), Shaohua Li wrote:
> cleanup kswapd_try_to_sleep() a little bit. Sometimes kswapd doesn't
> really sleep. In such case, don't call prepare_to_wait/finish_wait.
> It just wastes CPU.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> ---
>  mm/vmscan.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> Index: linux/mm/vmscan.c
> ===================================================================
> --- linux.orig/mm/vmscan.c	2011-07-28 15:52:35.000000000 +0800
> +++ linux/mm/vmscan.c	2011-07-28 15:55:56.000000000 +0800
> @@ -2709,13 +2709,11 @@ static void kswapd_try_to_sleep(pg_data_
>  	if (freezing(current) || kthread_should_stop())
>  		return;
>  
> -	prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
> -
>  	/* Try to sleep for a short interval */
>  	if (!sleeping_prematurely(pgdat, order, remaining, classzone_idx)) {
> +		prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
>  		remaining = schedule_timeout(HZ/10);
>  		finish_wait(&pgdat->kswapd_wait, &wait);
> -		prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
>  	}
>  
>  	/*
> @@ -2734,7 +2732,9 @@ static void kswapd_try_to_sleep(pg_data_
>  		 * them before going back to sleep.
>  		 */
>  		set_pgdat_percpu_threshold(pgdat, calculate_normal_threshold);
> +		prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
>  		schedule();
> +		finish_wait(&pgdat->kswapd_wait, &wait);
>  		set_pgdat_percpu_threshold(pgdat, calculate_pressure_threshold);
>  	} else {
>  		if (remaining)
> @@ -2742,7 +2742,6 @@ static void kswapd_try_to_sleep(pg_data_
>  		else
>  			count_vm_event(KSWAPD_HIGH_WMARK_HIT_QUICKLY);
>  	}
> -	finish_wait(&pgdat->kswapd_wait, &wait);
>  }
>  
>  /*

Prepare_to_wait/finish_wait basic usage is below. Briefly,
1) prepare_to_wait() is needed to call every sleeping
2) finish_wait is only need to exit sleeping loop

So, 1) moving prepare_to_wait looks pretty good to me. but I doubt the worth
of moving the finish_wait of function last.

-------------------------------------------------------------------------
#define __wait_event(wq, condition)                                     \
do {                                                                    \
        DEFINE_WAIT(__wait);                                            \
                                                                        \
        for (;;) {                                                      \
                prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE);    \
                if (condition)                                          \
                        break;                                          \
                schedule();                                             \
        }                                                               \
        finish_wait(&wq, &__wait);                                      \
} while (0)

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

Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep

[Shaohua Li]

On Tue, 2011-08-02 at 08:35 +0800, KOSAKI Motohiro wrote:
> (2011/07/28 17:13), Shaohua Li wrote:
> > cleanup kswapd_try_to_sleep() a little bit. Sometimes kswapd doesn't
> > really sleep. In such case, don't call prepare_to_wait/finish_wait.
> > It just wastes CPU.
> > 
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > ---
> >  mm/vmscan.c |    7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > Index: linux/mm/vmscan.c
> > ===================================================================
> > --- linux.orig/mm/vmscan.c	2011-07-28 15:52:35.000000000 +0800
> > +++ linux/mm/vmscan.c	2011-07-28 15:55:56.000000000 +0800
> > @@ -2709,13 +2709,11 @@ static void kswapd_try_to_sleep(pg_data_
> >  	if (freezing(current) || kthread_should_stop())
> >  		return;
> >  
> > -	prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
> > -
> >  	/* Try to sleep for a short interval */
> >  	if (!sleeping_prematurely(pgdat, order, remaining, classzone_idx)) {
> > +		prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
> >  		remaining = schedule_timeout(HZ/10);
> >  		finish_wait(&pgdat->kswapd_wait, &wait);
> > -		prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
> >  	}
> >  
> >  	/*
> > @@ -2734,7 +2732,9 @@ static void kswapd_try_to_sleep(pg_data_
> >  		 * them before going back to sleep.
> >  		 */
> >  		set_pgdat_percpu_threshold(pgdat, calculate_normal_threshold);
> > +		prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
> >  		schedule();
> > +		finish_wait(&pgdat->kswapd_wait, &wait);
> >  		set_pgdat_percpu_threshold(pgdat, calculate_pressure_threshold);
> >  	} else {
> >  		if (remaining)
> > @@ -2742,7 +2742,6 @@ static void kswapd_try_to_sleep(pg_data_
> >  		else
> >  			count_vm_event(KSWAPD_HIGH_WMARK_HIT_QUICKLY);
> >  	}
> > -	finish_wait(&pgdat->kswapd_wait, &wait);
> >  }
> >  
> >  /*
> 
> Prepare_to_wait/finish_wait basic usage is below. Briefly,
> 1) prepare_to_wait() is needed to call every sleeping
yes

> 2) finish_wait is only need to exit sleeping loop
> 
> So, 1) moving prepare_to_wait looks pretty good to me. but I doubt the worth
> of moving the finish_wait of function last.
so you are talking about leave the last finish_wait at the end of the
function, and delete other finish_wait, right? that is ok, but I'm
afraid it's not readable. a pair of prepare_to_wait/schedule/finish_wait
is more readable.

Thanks,
Shaohua

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

Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep

[KOSAKI Motohiro]

(2011/08/02 10:03), Shaohua Li wrote:
> On Tue, 2011-08-02 at 08:35 +0800, KOSAKI Motohiro wrote:
>> (2011/07/28 17:13), Shaohua Li wrote:
>>> cleanup kswapd_try_to_sleep() a little bit. Sometimes kswapd doesn't
>>> really sleep. In such case, don't call prepare_to_wait/finish_wait.
>>> It just wastes CPU.
>>>
>>> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
>>> ---
>>>  mm/vmscan.c |    7 +++----
>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> Index: linux/mm/vmscan.c
>>> ===================================================================
>>> --- linux.orig/mm/vmscan.c	2011-07-28 15:52:35.000000000 +0800
>>> +++ linux/mm/vmscan.c	2011-07-28 15:55:56.000000000 +0800
>>> @@ -2709,13 +2709,11 @@ static void kswapd_try_to_sleep(pg_data_
>>>  	if (freezing(current) || kthread_should_stop())
>>>  		return;
>>>  
>>> -	prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
>>> -
>>>  	/* Try to sleep for a short interval */
>>>  	if (!sleeping_prematurely(pgdat, order, remaining, classzone_idx)) {
>>> +		prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
>>>  		remaining = schedule_timeout(HZ/10);
>>>  		finish_wait(&pgdat->kswapd_wait, &wait);
>>> -		prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
>>>  	}
>>>  
>>>  	/*
>>> @@ -2734,7 +2732,9 @@ static void kswapd_try_to_sleep(pg_data_
>>>  		 * them before going back to sleep.
>>>  		 */
>>>  		set_pgdat_percpu_threshold(pgdat, calculate_normal_threshold);
>>> +		prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
>>>  		schedule();
>>> +		finish_wait(&pgdat->kswapd_wait, &wait);
>>>  		set_pgdat_percpu_threshold(pgdat, calculate_pressure_threshold);
>>>  	} else {
>>>  		if (remaining)
>>> @@ -2742,7 +2742,6 @@ static void kswapd_try_to_sleep(pg_data_
>>>  		else
>>>  			count_vm_event(KSWAPD_HIGH_WMARK_HIT_QUICKLY);
>>>  	}
>>> -	finish_wait(&pgdat->kswapd_wait, &wait);
>>>  }
>>>  
>>>  /*
>>
>> Prepare_to_wait/finish_wait basic usage is below. Briefly,
>> 1) prepare_to_wait() is needed to call every sleeping
> yes
> 
>> 2) finish_wait is only need to exit sleeping loop
>>
>> So, 1) moving prepare_to_wait looks pretty good to me. but I doubt the worth
>> of moving the finish_wait of function last.
> so you are talking about leave the last finish_wait at the end of the
> function, and delete other finish_wait, right? 

exactly. :)


> that is ok, but I'm
> afraid it's not readable. a pair of prepare_to_wait/schedule/finish_wait
> is more readable.

hm. I personally think keeping generic usage convention doesn't decrease
a readability. but I have no strong option. so, I'd like to hear other
developers opinion. if they agree with you, I'll not stick my opinion.

Thanks.




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

Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep

[Andrew Morton]

On Thu, 28 Jul 2011 16:13:09 +0800
Shaohua Li <shaohua.li@intel.com> wrote:

> cleanup kswapd_try_to_sleep() a little bit. Sometimes kswapd doesn't
> really sleep. In such case, don't call prepare_to_wait/finish_wait.
> It just wastes CPU.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> ---
>  mm/vmscan.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> Index: linux/mm/vmscan.c
> ===================================================================
> --- linux.orig/mm/vmscan.c	2011-07-28 15:52:35.000000000 +0800
> +++ linux/mm/vmscan.c	2011-07-28 15:55:56.000000000 +0800
> @@ -2709,13 +2709,11 @@ static void kswapd_try_to_sleep(pg_data_
>  	if (freezing(current) || kthread_should_stop())
>  		return;
>  
> -	prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
> -
>  	/* Try to sleep for a short interval */
>  	if (!sleeping_prematurely(pgdat, order, remaining, classzone_idx)) {
> +		prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
>  		remaining = schedule_timeout(HZ/10);
>  		finish_wait(&pgdat->kswapd_wait, &wait);
> -		prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
>  	}
>  
>  	/*
> @@ -2734,7 +2732,9 @@ static void kswapd_try_to_sleep(pg_data_
>  		 * them before going back to sleep.
>  		 */
>  		set_pgdat_percpu_threshold(pgdat, calculate_normal_threshold);
> +		prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
>  		schedule();
> +		finish_wait(&pgdat->kswapd_wait, &wait);
>  		set_pgdat_percpu_threshold(pgdat, calculate_pressure_threshold);
>  	} else {
>  		if (remaining)
> @@ -2742,7 +2742,6 @@ static void kswapd_try_to_sleep(pg_data_
>  		else
>  			count_vm_event(KSWAPD_HIGH_WMARK_HIT_QUICKLY);
>  	}
> -	finish_wait(&pgdat->kswapd_wait, &wait);
>  }

Well.   Here's some correct waiting code:

	prepare_to_wait(...);
	if (condition)
		schedule();
	finish_wait();

And here's come incorrect waiting code:

	if (condition) {
				<-- if `condition' becomese false here we can
				    sleep incorrectly and even miss a wakeup.
		prepare_to_wait(...);
		schedule();
		finish_wait();
	}

Your patch converts balance_pgdat() from the correct pattern to the
incorrect pattern.  This may be OK given the overall sloppiness of the
vmscan synchronisation.  But I think we need to convince ourselves that
we aren't adding rarely-occurring bugs or inefficiencies, and that this
change won't cause us to accidentally introduce rarely-occurring bugs
or inefficiencies as the code eveolves.


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