[PATCH] kswapd: avoid unnecessary rebalance after an unsuccessful balancing

[PATCH] kswapd: avoid unnecessary rebalance after an unsuccessful balancing

[Alex Shi]

In commit 215ddd66, Mel Gorman said kswapd is better to sleep after a
unsuccessful balancing if there is tighter reclaim request pending in
the balancing. In this scenario, the 'order' and 'classzone_idx'
that are checked for tighter request judgment is incorrect, since they
aren't the one kswapd should read from new pgdat, but the last time pgdat
value for just now balancing. Then kswapd will skip try_to_sleep func
and rebalance the last pgdat request. It's not our expected behavior.

So, I added new variables to distinguish the returned order/classzone_idx
from last balancing, that can resolved above issue in that scenario.

I tested the patch on our LKP system with swap-cp/fio mmap randrw
benchmarks. The performance has no change.

Padraig Brady, would you like to test this patch for your scenario.

Signed-off-by: Alex Shi <alex.shi@intel.com>
Reviewed-by:  Tim Chen <tim.c.chen@linux.intel.com>
---
 mm/vmscan.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index eb7bcce..6380674 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2792,7 +2792,9 @@ static void kswapd_try_to_sleep(pg_data_t *pgdat, int order, int classzone_idx)
 static int kswapd(void *p)
 {
 	unsigned long order, new_order;
+	unsigned balanced_order;
 	int classzone_idx, new_classzone_idx;
+	int balanced_classzone_idx;
 	pg_data_t *pgdat = (pg_data_t*)p;
 	struct task_struct *tsk = current;
 
@@ -2823,7 +2825,9 @@ static int kswapd(void *p)
 	set_freezable();
 
 	order = new_order = 0;
+	balanced_order = 0;
 	classzone_idx = new_classzone_idx = pgdat->nr_zones - 1;
+	balanced_classzone_idx = classzone_idx;
 	for ( ; ; ) {
 		int ret;
 
@@ -2832,7 +2836,7 @@ static int kswapd(void *p)
 		 * new request of a similar or harder type will succeed soon
 		 * so consider going to sleep on the basis we reclaimed at
 		 */
-		if (classzone_idx >= new_classzone_idx && order == new_order) {
+		if (balanced_classzone_idx >= new_classzone_idx && balanced_order == new_order) {
 			new_order = pgdat->kswapd_max_order;
 			new_classzone_idx = pgdat->classzone_idx;
 			pgdat->kswapd_max_order =  0;
@@ -2847,7 +2851,7 @@ static int kswapd(void *p)
 			order = new_order;
 			classzone_idx = new_classzone_idx;
 		} else {
-			kswapd_try_to_sleep(pgdat, order, classzone_idx);
+			kswapd_try_to_sleep(pgdat, balanced_order, balanced_classzone_idx);
 			order = pgdat->kswapd_max_order;
 			classzone_idx = pgdat->classzone_idx;
 			new_order = order;
@@ -2866,7 +2870,8 @@ static int kswapd(void *p)
 		 */
 		if (!ret) {
 			trace_mm_vmscan_kswapd_wake(pgdat->node_id, order);
-			order = balance_pgdat(pgdat, order, &classzone_idx);
+			balanced_classzone_idx = classzone_idx;
+			balanced_order = balance_pgdat(pgdat, order, &balanced_classzone_idx);
 		}
 	}
 	return 0;
-- 
1.6.3.3

--
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] kswapd: avoid unnecessary rebalance after an unsuccessful balancing

[Mel Gorman]

On Fri, Jul 29, 2011 at 11:23:10PM +0800, Alex Shi wrote:
> In commit 215ddd66, Mel Gorman said kswapd is better to sleep after a
> unsuccessful balancing if there is tighter reclaim request pending in
> the balancing. In this scenario, the 'order' and 'classzone_idx'
> that are checked for tighter request judgment is incorrect, since they
> aren't the one kswapd should read from new pgdat, but the last time pgdat
> value for just now balancing. Then kswapd will skip try_to_sleep func
> and rebalance the last pgdat request. It's not our expected behavior.
> 
> So, I added new variables to distinguish the returned order/classzone_idx
> from last balancing, that can resolved above issue in that scenario.
> 

I'm afraid this changelog is very difficult to read and I do not see
what problem you are trying to solve and I do not see what this patch
might solve.

When balance_pgdat() returns with a lower classzone or order, the values
stored in pgdat are not re-read and instead it tries to go to sleep
based on the starting request. Something like;

1. Read pgdat request A (classzone_idx, order)
2. balance_pgdat()
3.	During pgdat, a new pgdat request B (classzone_idx, order) is placed
4. balance_pgdat() returns but failed so classzone_idx is lower
5. Try to sleep based on pgdat request A

i.e. pgdat request B is not read and there is a comment explaining
why pgdat request B is not read after balance_pgdat() fails.

This patch adds some variables that might improve the readability
for some people but otherwise I can't see what problem is being
fixed. What did I miss?

-- 
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] kswapd: avoid unnecessary rebalance after an unsuccessful balancing

[Alex,Shi]

On Fri, 2011-07-29 at 23:40 +0800, Mel Gorman wrote:
> On Fri, Jul 29, 2011 at 11:23:10PM +0800, Alex Shi wrote:
> > In commit 215ddd66, Mel Gorman said kswapd is better to sleep after a
> > unsuccessful balancing if there is tighter reclaim request pending in
> > the balancing. In this scenario, the 'order' and 'classzone_idx'
> > that are checked for tighter request judgment is incorrect, since they
> > aren't the one kswapd should read from new pgdat, but the last time pgdat
> > value for just now balancing. Then kswapd will skip try_to_sleep func
> > and rebalance the last pgdat request. It's not our expected behavior.
> > 
> > So, I added new variables to distinguish the returned order/classzone_idx
> > from last balancing, that can resolved above issue in that scenario.
> > 
> 
> I'm afraid this changelog is very difficult to read and I do not see
> what problem you are trying to solve and I do not see what this patch
> might solve.
> 
> When balance_pgdat() returns with a lower classzone or order, the values
> stored in pgdat are not re-read and instead it tries to go to sleep
> based on the starting request. Something like;

Thanks for your comments, I will use this comments style next time, list
request A, B etc. 

> 
> 1. Read pgdat request A (classzone_idx, order)

Assume the order of A > 0, like is 3.

> 2. balance_pgdat()
> 3.	During pgdat, a new pgdat request B (classzone_idx, order) is placed
> 4. balance_pgdat() returns but failed so classzone_idx is lower

Another balance_pgdat() failure indicate is returned order == 0, am I
right?  If so, the next step of kswapd is not trying to sleep, but do
request A balance again.  And I thought this behavior doesn't match the
comments in kswapd. 

> 5. Try to sleep based on pgdat request A 

> 
> i.e. pgdat request B is not read and there is a comment explaining
> why pgdat request B is not read after balance_pgdat() fails.
> 
> This patch adds some variables that might improve the readability
> for some people but otherwise I can't see what problem is being
> fixed. What did I miss?
> 


--
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] kswapd: avoid unnecessary rebalance after an unsuccessful balancing

[Mel Gorman]

On Mon, Aug 01, 2011 at 08:45:17AM +0800, Alex,Shi wrote:
> On Fri, 2011-07-29 at 23:40 +0800, Mel Gorman wrote:
> > On Fri, Jul 29, 2011 at 11:23:10PM +0800, Alex Shi wrote:
> > > In commit 215ddd66, Mel Gorman said kswapd is better to sleep after a
> > > unsuccessful balancing if there is tighter reclaim request pending in
> > > the balancing. In this scenario, the 'order' and 'classzone_idx'
> > > that are checked for tighter request judgment is incorrect, since they
> > > aren't the one kswapd should read from new pgdat, but the last time pgdat
> > > value for just now balancing. Then kswapd will skip try_to_sleep func
> > > and rebalance the last pgdat request. It's not our expected behavior.
> > > 
> > > So, I added new variables to distinguish the returned order/classzone_idx
> > > from last balancing, that can resolved above issue in that scenario.
> > > 
> > 
> > I'm afraid this changelog is very difficult to read and I do not see
> > what problem you are trying to solve and I do not see what this patch
> > might solve.
> > 
> > When balance_pgdat() returns with a lower classzone or order, the values
> > stored in pgdat are not re-read and instead it tries to go to sleep
> > based on the starting request. Something like;
> 
> Thanks for your comments, I will use this comments style next time, list
> request A, B etc. 
> 
> > 
> > 1. Read pgdat request A (classzone_idx, order)
> 
> Assume the order of A > 0, like is 3.
> 
> > 2. balance_pgdat()
> > 3.During pgdat, a new pgdat request B (classzone_idx, order) is placed
> > 4. balance_pgdat() returns but failed so classzone_idx is lower
> 
> Another balance_pgdat() failure indicate is returned order == 0, am I
> right? 

Yes.

> If so, the next step of kswapd is not trying to sleep, but do
> request A balance again.  And I thought this behavior doesn't match the
> comments in kswapd. 
> 

You're right. I was thinking only of classzone_idx. I see the point now.

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] kswapd: avoid unnecessary rebalance after an unsuccessful balancing

[Alex,Shi]

> You're right. I was thinking only of classzone_idx. I see the point now.
> 
> Acked-by: Mel Gorman <mgorman@suse.de>

Thanks for your 'Ack', I want to follow your commit comments and correct
a coding style issue, so I rewrite the patch here.

--------------

In commit 215ddd66, Mel Gorman said kswapd is better to sleep after a
unsuccessful balancing if there is tighter reclaim request pending in
the balancing. But in the following scenario, kswapd do something that
is not matched our expectation. The patch fixes this issue.

1, Read pgdat request A (classzone_idx, order = 3)
2, balance_pgdat()
3,    During pgdat, a new pgdat request B (classzone_idx, order = 5) is
placed
4, balance_pgdat() returns but failed since returned order = 0
5, pgdat of request A assigned to balance_pgdat(), and do balancing
again. While the expectation behavior of kswapd should try to sleep.

Signed-off-by: Alex Shi <alex.shi@intel.com>
Reviewed-by: Tim Chen <tim.c.chen@linux.intel.com>
Acked-by: Mel Gorman <mgorman@suse.de>
---
 mm/vmscan.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index eb7bcce..ed8c84c 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2792,7 +2792,9 @@ static void kswapd_try_to_sleep(pg_data_t *pgdat, int order, int classzone_idx)
 static int kswapd(void *p)
 {
 	unsigned long order, new_order;
+	unsigned balanced_order;
 	int classzone_idx, new_classzone_idx;
+	int balanced_classzone_idx;
 	pg_data_t *pgdat = (pg_data_t*)p;
 	struct task_struct *tsk = current;
 
@@ -2823,7 +2825,9 @@ static int kswapd(void *p)
 	set_freezable();
 
 	order = new_order = 0;
+	balanced_order = 0;
 	classzone_idx = new_classzone_idx = pgdat->nr_zones - 1;
+	balanced_classzone_idx = classzone_idx;
 	for ( ; ; ) {
 		int ret;
 
@@ -2832,7 +2836,8 @@ static int kswapd(void *p)
 		 * new request of a similar or harder type will succeed soon
 		 * so consider going to sleep on the basis we reclaimed at
 		 */
-		if (classzone_idx >= new_classzone_idx && order == new_order) {
+		if (balanced_classzone_idx >= new_classzone_idx &&
+					balanced_order == new_order) {
 			new_order = pgdat->kswapd_max_order;
 			new_classzone_idx = pgdat->classzone_idx;
 			pgdat->kswapd_max_order =  0;
@@ -2847,7 +2852,8 @@ static int kswapd(void *p)
 			order = new_order;
 			classzone_idx = new_classzone_idx;
 		} else {
-			kswapd_try_to_sleep(pgdat, order, classzone_idx);
+			kswapd_try_to_sleep(pgdat, balanced_order,
+						balanced_classzone_idx);
 			order = pgdat->kswapd_max_order;
 			classzone_idx = pgdat->classzone_idx;
 			new_order = order;
@@ -2866,7 +2872,9 @@ static int kswapd(void *p)
 		 */
 		if (!ret) {
 			trace_mm_vmscan_kswapd_wake(pgdat->node_id, order);
-			order = balance_pgdat(pgdat, order, &classzone_idx);
+			balanced_classzone_idx = classzone_idx;
+			balanced_order = balance_pgdat(pgdat, order,
+						&balanced_classzone_idx);
 		}
 	}
 	return 0;
-- 
1.6.3.3



--
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] kswapd: avoid unnecessary rebalance after an unsuccessful balancing

[Pádraig Brady]

On 07/29/2011 04:23 PM, Alex Shi wrote:
> In commit 215ddd66, Mel Gorman said kswapd is better to sleep after a
> unsuccessful balancing if there is tighter reclaim request pending in
> the balancing. In this scenario, the 'order' and 'classzone_idx'
> that are checked for tighter request judgment is incorrect, since they
> aren't the one kswapd should read from new pgdat, but the last time pgdat
> value for just now balancing. Then kswapd will skip try_to_sleep func
> and rebalance the last pgdat request. It's not our expected behavior.
> 
> So, I added new variables to distinguish the returned order/classzone_idx
> from last balancing, that can resolved above issue in that scenario.
> 
> I tested the patch on our LKP system with swap-cp/fio mmap randrw
> benchmarks. The performance has no change.
> 
> Padraig Brady, would you like to test this patch for your scenario.

This
+ your previous 2 line patch
+ Mel's 3 patches
+ 2.6.38.4

still works fine for me.

cheers,
Padraig.

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