[PATCH] kswapd: assign new_order and new_classzone_idx after wakeup in sleeping

[PATCH] kswapd: assign new_order and new_classzone_idx after wakeup in sleeping

[Alex Shi]

There 2 place to read pgdat in kswapd. One is return from a successful
balance, another is waked up from sleeping. But the new_order and
new_classzone_idx are not assigned after kswapd_try_to_sleep(), that
will cause a bug in the following scenario.

After the last time successful balance, kswapd goes to sleep. So the
new_order and new_classzone_idx were assigned to 0 and MAX-1 since there
is no new wakeup during last time balancing. Now, a new wakeup came and
finish balancing successful with order > 0. But since new_order is still
0, this time successful balancing were judged as a failed balance. so,
if there is another new wakeup coming during balancing, kswapd cann't
read this and still want to try to sleep. And if the new wakeup is a
tighter request, kswapd may goes to sleep, not to do balancing. That is
incorrect.

So, to avoid above problem, the new_order and new_classzone_idx need to
be assigned for later successful comparison.

Signed-off-by: Alex Shi <alex.shi@intel.com>
---
 mm/vmscan.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 7ef6912..eb7bcce 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2850,6 +2850,8 @@ static int kswapd(void *p)
 			kswapd_try_to_sleep(pgdat, order, classzone_idx);
 			order = pgdat->kswapd_max_order;
 			classzone_idx = pgdat->classzone_idx;
+			new_order = order;
+			new_classzone_idx = classzone_idx;
 			pgdat->kswapd_max_order = 0;
 			pgdat->classzone_idx = pgdat->nr_zones - 1;
 		}
-- 
1.6.3.3

Re: [PATCH] kswapd: assign new_order and new_classzone_idx after wakeup in sleeping

[Mel Gorman]

On Thu, Jul 28, 2011 at 04:11:28PM +0800, Alex Shi wrote:
> There 2 place to read pgdat in kswapd. One is return from a successful
> balance, another is waked up from sleeping. But the new_order and
> new_classzone_idx are not assigned after kswapd_try_to_sleep(), that
> will cause a bug in the following scenario.
> 
> After the last time successful balance, kswapd goes to sleep. So the
> new_order and new_classzone_idx were assigned to 0 and MAX-1 since there
> is no new wakeup during last time balancing. Now, a new wakeup came and
> finish balancing successful with order > 0. But since new_order is still
> 0, this time successful balancing were judged as a failed balance. so,
> if there is another new wakeup coming during balancing, kswapd cann't
> read this and still want to try to sleep. And if the new wakeup is a
> tighter request, kswapd may goes to sleep, not to do balancing. That is
> incorrect.
> 
> So, to avoid above problem, the new_order and new_classzone_idx need to
> be assigned for later successful comparison.
> 

Other than a different changelog, this is identical to
https://lkml.org/lkml/2011/6/30/157 so

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

It won't be merged to -stable until it goes to mainline though so
minimally you need to post this to linux-mm.

For -stable, you should explain why it is a candidate. I didn't push
the patch at the time because user problems were already resolved
and I wanted the merged for 3.0 before revisiting it. What problem
did you observe without this patch? With the lack of reference to
the other thread or the previous patch, I'm assuming you found and
solved the problem independently and I'd like to add a test case.

Thanks.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] kswapd: assign new_order and new_classzone_idx after wakeup in sleeping

[Alex,Shi]

On Thu, 2011-07-28 at 18:19 +0800, Mel Gorman wrote:
> On Thu, Jul 28, 2011 at 04:11:28PM +0800, Alex Shi wrote:
> > There 2 place to read pgdat in kswapd. One is return from a successful
> > balance, another is waked up from sleeping. But the new_order and
> > new_classzone_idx are not assigned after kswapd_try_to_sleep(), that
> > will cause a bug in the following scenario.
> > 
> > After the last time successful balance, kswapd goes to sleep. So the
> > new_order and new_classzone_idx were assigned to 0 and MAX-1 since there
> > is no new wakeup during last time balancing. Now, a new wakeup came and
> > finish balancing successful with order > 0. But since new_order is still
> > 0, this time successful balancing were judged as a failed balance. so,
> > if there is another new wakeup coming during balancing, kswapd cann't
> > read this and still want to try to sleep. And if the new wakeup is a
> > tighter request, kswapd may goes to sleep, not to do balancing. That is
> > incorrect.
> > 
> > So, to avoid above problem, the new_order and new_classzone_idx need to
> > be assigned for later successful comparison.
> > 
> 
> Other than a different changelog, this is identical to
> https://lkml.org/lkml/2011/6/30/157 so

Oops, I didn't aware this, otherwise it will save me several hours to
explain what the problem in current code to Shaohua and others. :) 
In fact, I still hold another patch of kswapd and some idea of how to
kswapd working that want to talk with you.

> 
> Acked-by: Mel Gorman <mgorman@suse.de>
> 
> It won't be merged to -stable until it goes to mainline though so
> minimally you need to post this to linux-mm.
> 
> For -stable, you should explain why it is a candidate. I didn't push
> the patch at the time because user problems were already resolved
> and I wanted the merged for 3.0 before revisiting it. What problem
> did you observe without this patch? With the lack of reference to
> the other thread or the previous patch, I'm assuming you found and
> solved the problem independently and I'd like to add a test case.

Actually, our LKP testing didn't find this problem on this point. Even
with the patch, performance has no change on our machines. I just find
this by my eyes. 

BTW, I have tracked our benchmarks for their hot path in kernel. The
most exercised benchmark on kswapd is no more than 5% of system load.
that is fio mmap rand write or randrw. 

Do you have some benchmark can use kswapd much? 

BTW, our project http://kernel-perf.sourceforge.net/ 
> 
> Thanks.
> 

Re: [PATCH] kswapd: assign new_order and new_classzone_idx after wakeup in sleeping

[Mel Gorman]

On Thu, Jul 28, 2011 at 06:47:09PM +0800, Alex,Shi wrote:
> On Thu, 2011-07-28 at 18:19 +0800, Mel Gorman wrote:
> > On Thu, Jul 28, 2011 at 04:11:28PM +0800, Alex Shi wrote:
> > > There 2 place to read pgdat in kswapd. One is return from a successful
> > > balance, another is waked up from sleeping. But the new_order and
> > > new_classzone_idx are not assigned after kswapd_try_to_sleep(), that
> > > will cause a bug in the following scenario.
> > > 
> > > After the last time successful balance, kswapd goes to sleep. So the
> > > new_order and new_classzone_idx were assigned to 0 and MAX-1 since there
> > > is no new wakeup during last time balancing. Now, a new wakeup came and
> > > finish balancing successful with order > 0. But since new_order is still
> > > 0, this time successful balancing were judged as a failed balance. so,
> > > if there is another new wakeup coming during balancing, kswapd cann't
> > > read this and still want to try to sleep. And if the new wakeup is a
> > > tighter request, kswapd may goes to sleep, not to do balancing. That is
> > > incorrect.
> > > 
> > > So, to avoid above problem, the new_order and new_classzone_idx need to
> > > be assigned for later successful comparison.
> > > 
> > 
> > Other than a different changelog, this is identical to
> > https://lkml.org/lkml/2011/6/30/157 so
> 
> Oops, I didn't aware this, otherwise it will save me several hours to
> explain what the problem in current code to Shaohua and others. :) 

Sorry for wasting your time.

> In fact, I still hold another patch of kswapd and some idea of how to
> kswapd working that want to talk with you.
> 

Post to linux-mm, cc me.

> > Acked-by: Mel Gorman <mgorman@suse.de>
> > 
> > It won't be merged to -stable until it goes to mainline though so
> > minimally you need to post this to linux-mm.
> > 
> > For -stable, you should explain why it is a candidate. I didn't push
> > the patch at the time because user problems were already resolved
> > and I wanted the merged for 3.0 before revisiting it. What problem
> > did you observe without this patch? With the lack of reference to
> > the other thread or the previous patch, I'm assuming you found and
> > solved the problem independently and I'd like to add a test case.
> 
> Actually, our LKP testing didn't find this problem on this point. Even
> with the patch, performance has no change on our machines. I just find
> this by my eyes. 
> 

Dang. I figured all right that it was unlikely the patch would
actually fix any problem but it looks correct and shouldnt' cause a
regression. You should resend the patch to Andrew cc'ing the people
in the old thread and linux-mm and ask Padraig Brady to test the
patch to confirm his problem does not reappear. When it gets into
mainline, try for -stable but I think there is very little motivation
for merging it there.

> BTW, I have tracked our benchmarks for their hot path in kernel. The
> most exercised benchmark on kswapd is no more than 5% of system load.
> that is fio mmap rand write or randrw. 
> 
> Do you have some benchmark can use kswapd much? 
> 

Not excessively. Except in cases where kswapd is buggy, I don't remember
many cases where it gets very far over 9% . I'll dig through old results
later today or tomorrow and double check.

> BTW, our project http://kernel-perf.sourceforge.net/ 

Thanks.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] kswapd: assign new_order and new_classzone_idx after wakeup in sleeping

[Alex,Shi]

> Post to linux-mm, cc me.
> 
> > > Acked-by: Mel Gorman <mgorman@suse.de>
> > > 
> > > It won't be merged to -stable until it goes to mainline though so
> > > minimally you need to post this to linux-mm.
> > > 
> > > For -stable, you should explain why it is a candidate. I didn't push
> > > the patch at the time because user problems were already resolved
> > > and I wanted the merged for 3.0 before revisiting it. What problem
> > > did you observe without this patch? With the lack of reference to
> > > the other thread or the previous patch, I'm assuming you found and
> > > solved the problem independently and I'd like to add a test case.
> > 
> > Actually, our LKP testing didn't find this problem on this point. Even
> > with the patch, performance has no change on our machines. I just find
> > this by my eyes. 
> > 
> 
> Dang. I figured all right that it was unlikely the patch would
> actually fix any problem but it looks correct and shouldnt' cause a
> regression. You should resend the patch to Andrew cc'ing the people
> in the old thread and linux-mm and ask Padraig Brady to test the
> patch to confirm his problem does not reappear. When it gets into
> mainline, try for -stable but I think there is very little motivation
> for merging it there.

Thanks a lot for the suggestions. I will do them. 

[PATCH] kswapd: assign new_order and new_classzone_idx after wakeup in sleeping

[Alex Shi]

There 2 place to read pgdat in kswapd. One is return from a successful
balance, another is waked up from sleeping. But the new_order and
new_classzone_idx are not assigned after kswapd_try_to_sleep(), that
will cause a bug in the following scenario.

After the last time successful balance, kswapd goes to sleep. So the
new_order and new_classzone_idx were assigned to 0 and MAX-1 since there
is no new wakeup during last time balancing. Now, a new wakeup came and
finish balancing successful with order > 0. But since new_order is still
0, this time successful balancing were judged as a failed balance. so,
if there is another new wakeup coming during balancing, kswapd cann't
read this and still want to try to sleep. And if the new wakeup is a
tighter request, kswapd may goes to sleep, not to do balancing. That is
incorrect.

So, to avoid above problem, the new_order and new_classzone_idx need to
be assigned for later successful comparison.

Paidrag Brady, Could like do a retry for your problem on this patch?

Signed-off-by: Alex Shi <alex.shi@intel.com>
Acked-by: Mel Gorman <mgorman@suse.de>
---
 mm/vmscan.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 7ef6912..eb7bcce 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2850,6 +2850,8 @@ static int kswapd(void *p)
 			kswapd_try_to_sleep(pgdat, order, classzone_idx);
 			order = pgdat->kswapd_max_order;
 			classzone_idx = pgdat->classzone_idx;
+			new_order = order;
+			new_classzone_idx = classzone_idx;
 			pgdat->kswapd_max_order = 0;
 			pgdat->classzone_idx = pgdat->nr_zones - 1;
 		}
-- 
1.6.3.3

Re: [PATCH] kswapd: assign new_order and new_classzone_idx after wakeup in sleeping

[Minchan Kim]

On Fri, Jul 29, 2011 at 09:34:42AM +0800, Alex Shi wrote:
> There 2 place to read pgdat in kswapd. One is return from a successful
> balance, another is waked up from sleeping. But the new_order and
> new_classzone_idx are not assigned after kswapd_try_to_sleep(), that
> will cause a bug in the following scenario.
> 
> After the last time successful balance, kswapd goes to sleep. So the
> new_order and new_classzone_idx were assigned to 0 and MAX-1 since there
> is no new wakeup during last time balancing. Now, a new wakeup came and
> finish balancing successful with order > 0. But since new_order is still
> 0, this time successful balancing were judged as a failed balance. so,
> if there is another new wakeup coming during balancing, kswapd cann't
> read this and still want to try to sleep. And if the new wakeup is a
> tighter request, kswapd may goes to sleep, not to do balancing. That is
> incorrect.
> 
> So, to avoid above problem, the new_order and new_classzone_idx need to
> be assigned for later successful comparison.
> 
> Paidrag Brady, Could like do a retry for your problem on this patch?
> 
> Signed-off-by: Alex Shi <alex.shi@intel.com>
> Acked-by: Mel Gorman <mgorman@suse.de>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>

-- 
Kind regards,
Minchan Kim

RE: [PATCH] kswapd: assign new_order and new_classzone_idx after wakeup in sleeping

[Shi, Alex]

> -----Original Message-----
> From: Minchan Kim [mailto:minchan.kim@gmail.com]
> Sent: Friday, July 29, 2011 4:57 PM
> To: Shi, Alex
> Cc: majordomo@kvack.org; P@draigBrady.com; mgorman@suse.de;
> linux-kernel@vger.kernel.org; andrea@cpushare.com; Chen, Tim C; Li,
> Shaohua; akpm@linux-foundation.org; riel@redhat.com; luto@mit.edu
> Subject: Re: [PATCH] kswapd: assign new_order and new_classzone_idx after
> wakeup in sleeping
> 
> On Fri, Jul 29, 2011 at 09:34:42AM +0800, Alex Shi wrote:
> > There 2 place to read pgdat in kswapd. One is return from a successful
> > balance, another is waked up from sleeping. But the new_order and
> > new_classzone_idx are not assigned after kswapd_try_to_sleep(), that
> > will cause a bug in the following scenario.
> >
> > After the last time successful balance, kswapd goes to sleep. So the
> > new_order and new_classzone_idx were assigned to 0 and MAX-1 since there
> > is no new wakeup during last time balancing. Now, a new wakeup came and
> > finish balancing successful with order > 0. But since new_order is still
> > 0, this time successful balancing were judged as a failed balance. so,
> > if there is another new wakeup coming during balancing, kswapd cann't
> > read this and still want to try to sleep. And if the new wakeup is a
> > tighter request, kswapd may goes to sleep, not to do balancing. That is
> > incorrect.
> >
> > So, to avoid above problem, the new_order and new_classzone_idx need to
> > be assigned for later successful comparison.
> >
> > Paidrag Brady, Could like do a retry for your problem on this patch?
> >
> > Signed-off-by: Alex Shi <alex.shi@intel.com>
> > Acked-by: Mel Gorman <mgorman@suse.de>
> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>

Thanks for review. 
BTW, I remove the incorrect email address of linux-mm. sorry for this! 

[PATCH] kswapd: assign new_order and new_classzone_idx after wakeup in sleeping

[Alex Shi]

There 2 place to read pgdat in kswapd. One is return from a successful
balance, another is waked up from sleeping. But the new_order and
new_classzone_idx are not assigned after kswapd_try_to_sleep(), that
will cause a bug in the following scenario.

After the last time successful balance, kswapd goes to sleep. So the
new_order and new_classzone_idx were assigned to 0 and MAX-1 since there
is no new wakeup during last time balancing. Now, a new wakeup came and
finish balancing successful with order > 0. But since new_order is still
0, this time successful balancing were judged as a failed balance. so,
if there is another new wakeup coming during balancing, kswapd cann't
read this and still want to try to sleep. And if the new wakeup is a
tighter request, kswapd may goes to sleep, not to do balancing. That is
incorrect.

So, to avoid above problem, the new_order and new_classzone_idx need to
be assigned for later successful comparison.

Paidrag Brady, Could like do a retry for your problem on this patch?

Signed-off-by: Alex Shi <alex.shi@intel.com>
Acked-by: Mel Gorman <mgorman@suse.de>
---
 mm/vmscan.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 7ef6912..eb7bcce 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2850,6 +2850,8 @@ static int kswapd(void *p)
 			kswapd_try_to_sleep(pgdat, order, classzone_idx);
 			order = pgdat->kswapd_max_order;
 			classzone_idx = pgdat->classzone_idx;
+			new_order = order;
+			new_classzone_idx = classzone_idx;
 			pgdat->kswapd_max_order = 0;
 			pgdat->classzone_idx = pgdat->nr_zones - 1;
 		}
-- 
1.6.3.3