* Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep
2011-07-28 8:13 [patch 3/3]vmscan: cleanup kswapd_try_to_sleep Shaohua Li
@ 2011-07-28 11:03 ` Mel Gorman
2011-07-29 0:17 ` Shaohua Li
2011-07-29 9:39 ` Minchan Kim
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Mel Gorman @ 2011-07-28 11:03 UTC (permalink / raw)
To: Shaohua Li; +Cc: Andrew Morton, linux-mm, 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>
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>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep
2011-07-28 11:03 ` Mel Gorman
@ 2011-07-29 0:17 ` Shaohua Li
0 siblings, 0 replies; 9+ messages in thread
From: Shaohua Li @ 2011-07-29 0:17 UTC (permalink / raw)
To: Mel Gorman; +Cc: Andrew Morton, linux-mm, Minchan Kim
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep
2011-07-28 8:13 [patch 3/3]vmscan: cleanup kswapd_try_to_sleep Shaohua Li
2011-07-28 11:03 ` Mel Gorman
@ 2011-07-29 9:39 ` Minchan Kim
2011-07-29 10:46 ` Shaohua Li
2011-08-02 0:35 ` KOSAKI Motohiro
2011-08-19 23:56 ` Andrew Morton
3 siblings, 1 reply; 9+ messages in thread
From: Minchan Kim @ 2011-07-29 9:39 UTC (permalink / raw)
To: Shaohua Li; +Cc: Andrew Morton, linux-mm, mgorman
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep
2011-07-29 9:39 ` Minchan Kim
@ 2011-07-29 10:46 ` Shaohua Li
0 siblings, 0 replies; 9+ messages in thread
From: Shaohua Li @ 2011-07-29 10:46 UTC (permalink / raw)
To: Minchan Kim; +Cc: Andrew Morton, linux-mm, mgorman@suse.de
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep
2011-07-28 8:13 [patch 3/3]vmscan: cleanup kswapd_try_to_sleep Shaohua Li
2011-07-28 11:03 ` Mel Gorman
2011-07-29 9:39 ` Minchan Kim
@ 2011-08-02 0:35 ` KOSAKI Motohiro
2011-08-02 1:03 ` Shaohua Li
2011-08-19 23:56 ` Andrew Morton
3 siblings, 1 reply; 9+ messages in thread
From: KOSAKI Motohiro @ 2011-08-02 0:35 UTC (permalink / raw)
To: shaohua.li; +Cc: akpm, linux-mm, mgorman, minchan.kim
(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>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep
2011-08-02 0:35 ` KOSAKI Motohiro
@ 2011-08-02 1:03 ` Shaohua Li
2011-08-02 1:48 ` KOSAKI Motohiro
0 siblings, 1 reply; 9+ messages in thread
From: Shaohua Li @ 2011-08-02 1:03 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: akpm@linux-foundation.org, linux-mm@kvack.org, mgorman@suse.de,
minchan.kim@gmail.com
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>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep
2011-08-02 1:03 ` Shaohua Li
@ 2011-08-02 1:48 ` KOSAKI Motohiro
0 siblings, 0 replies; 9+ messages in thread
From: KOSAKI Motohiro @ 2011-08-02 1:48 UTC (permalink / raw)
To: shaohua.li; +Cc: akpm, linux-mm, mgorman, minchan.kim
(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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 3/3]vmscan: cleanup kswapd_try_to_sleep
2011-07-28 8:13 [patch 3/3]vmscan: cleanup kswapd_try_to_sleep Shaohua Li
` (2 preceding siblings ...)
2011-08-02 0:35 ` KOSAKI Motohiro
@ 2011-08-19 23:56 ` Andrew Morton
3 siblings, 0 replies; 9+ messages in thread
From: Andrew Morton @ 2011-08-19 23:56 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-mm, mgorman, Minchan Kim
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>
^ permalink raw reply [flat|nested] 9+ messages in thread