Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1581153 > unrolled thread

[PATCH 1/3] mm, vmscan: fix zone balance check in prepare_kswapd_sleep

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/3] mm, vmscan: fix zone balance check in prepare_kswapd_sleep Mel Gorman <mgorman@techsingularity.net> - 2017-02-15 10:30 +0100
    Re: [PATCH 1/3] mm, vmscan: fix zone balance check in prepare_kswapd_sleep "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2017-02-16 04:00 +0100
    Re: [PATCH 1/3] mm, vmscan: fix zone balance check in  prepare_kswapd_sleep Minchan Kim <minchan@kernel.org> - 2017-02-22 08:10 +0100
      Re: [PATCH 1/3] mm, vmscan: fix zone balance check in  prepare_kswapd_sleep Mel Gorman <mgorman@techsingularity.net> - 2017-02-23 16:10 +0100
        Re: [PATCH 1/3] mm, vmscan: fix zone balance check in  prepare_kswapd_sleep Minchan Kim <minchan@kernel.org> - 2017-02-24 02:20 +0100
          Re: [PATCH 1/3] mm, vmscan: fix zone balance check in  prepare_kswapd_sleep Mel Gorman <mgorman@techsingularity.net> - 2017-02-24 10:20 +0100
            Re: [PATCH 1/3] mm, vmscan: fix zone balance check in  prepare_kswapd_sleep Minchan Kim <minchan@kernel.org> - 2017-02-27 07:20 +0100

#1581153 — [PATCH 1/3] mm, vmscan: fix zone balance check in prepare_kswapd_sleep

From: Shantanu Goel <sgoel01@yahoo.com>

The check in prepare_kswapd_sleep needs to match the one in balance_pgdat
since the latter will return as soon as any one of the zones in the
classzone is above the watermark.  This is specially important for higher
order allocations since balance_pgdat will typically reset the order to
zero relying on compaction to create the higher order pages.  Without this
patch, prepare_kswapd_sleep fails to wake up kcompactd since the zone
balance check fails.

On 4.9.7 kswapd is failing to wake up kcompactd due to a mismatch in the
zone balance check between balance_pgdat() and prepare_kswapd_sleep().
balance_pgdat() returns as soon as a single zone satisfies the allocation
but prepare_kswapd_sleep() requires all zones to do +the same.  This causes
prepare_kswapd_sleep() to never succeed except in the order == 0 case and
consequently, wakeup_kcompactd() is never called.  On my machine prior to
apply this patch, the state of compaction from /proc/vmstat looked this
way after a day and a half +of uptime:

compact_migrate_scanned 240496
compact_free_scanned 76238632
compact_isolated 123472
compact_stall 1791
compact_fail 29
compact_success 1762
compact_daemon_wake 0

After applying the patch and about 10 hours of uptime the state looks
like this:

compact_migrate_scanned 59927299
compact_free_scanned 2021075136
compact_isolated 640926
compact_stall 4
compact_fail 2
compact_success 2
compact_daemon_wake 5160

Further notes from Mel that motivated him to pick this patch up and
resend it;

It was observed for the simoop workload (pressures the VM similar to HADOOP)
that kswapd was failing to keep ahead of direct reclaim. The investigation
noted that there was a need to rationalise kswapd decisions to reclaim
with kswapd decisions to sleep. With this patch on a 2-socket box, there
was a 43% reduction in direct reclaim scanning.

However, the impact otherwise is extremely negative. Kswapd reclaim
efficiency dropped from 98% to 76%. simoop has three latency-related
metrics for read, write and allocation (an anonymous mmap and fault).

                                         4.10.0-rc7            4.10.0-rc7
                                     mmots-20170209           fixcheck-v1
Amean    p50-Read             22325202.49 (  0.00%) 20026926.55 ( 10.29%)
Amean    p95-Read             26102988.80 (  0.00%) 27023360.00 ( -3.53%)
Amean    p99-Read             30935176.53 (  0.00%) 30994432.00 ( -0.19%)
Amean    p50-Write                 976.44 (  0.00%)     1905.28 (-95.12%)
Amean    p95-Write               15471.29 (  0.00%)    36210.09 (-134.05%)
Amean    p99-Write               35108.62 (  0.00%)   479494.96 (-1265.75%)
Amean    p50-Allocation          76382.61 (  0.00%)    87603.20 (-14.69%)
Amean    p95-Allocation         127777.39 (  0.00%)   244491.38 (-91.34%)
Amean    p99-Allocation         187937.39 (  0.00%)  1745237.33 (-828.63%)

There are also more allocation stalls. One of the largest impacts was due
to pages written back from kswapd context rising from 0 pages to 4516642
pages during the hour the workload ran for. By and large, the patch has very
bad behaviour but easily missed as the impact on a UMA machine is negligible.

This patch is included with the data in case a bisection leads to this area.
This patch is also a pre-requisite for the rest of the series.

Signed-off-by: Shantanu Goel <sgoel01@yahoo.com>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/vmscan.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 26c3b405ef34..92fc66bd52bc 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -3140,11 +3140,11 @@ static bool prepare_kswapd_sleep(pg_data_t *pgdat, int order, int classzone_idx)
 		if (!managed_zone(zone))
 			continue;
 
-		if (!zone_balanced(zone, order, classzone_idx))
-			return false;
+		if (zone_balanced(zone, order, classzone_idx))
+			return true;
 	}
 
-	return true;
+	return false;
 }
 
 /*
-- 
2.11.0

[toc] | [next] | [standalone]

#1582289

On February 15, 2017 5:23 PM Mel Gorman wrote: 
> 
> From: Shantanu Goel <sgoel01@yahoo.com>
> 
> The check in prepare_kswapd_sleep needs to match the one in balance_pgdat
> since the latter will return as soon as any one of the zones in the
> classzone is above the watermark.  This is specially important for higher
> order allocations since balance_pgdat will typically reset the order to
> zero relying on compaction to create the higher order pages.  Without this
> patch, prepare_kswapd_sleep fails to wake up kcompactd since the zone
> balance check fails.
> 
> On 4.9.7 kswapd is failing to wake up kcompactd due to a mismatch in the
> zone balance check between balance_pgdat() and prepare_kswapd_sleep().
> balance_pgdat() returns as soon as a single zone satisfies the allocation
> but prepare_kswapd_sleep() requires all zones to do +the same.  This causes
> prepare_kswapd_sleep() to never succeed except in the order == 0 case and
> consequently, wakeup_kcompactd() is never called.  On my machine prior to
> apply this patch, the state of compaction from /proc/vmstat looked this
> way after a day and a half +of uptime:
> 
> compact_migrate_scanned 240496
> compact_free_scanned 76238632
> compact_isolated 123472
> compact_stall 1791
> compact_fail 29
> compact_success 1762
> compact_daemon_wake 0
> 
> After applying the patch and about 10 hours of uptime the state looks
> like this:
> 
> compact_migrate_scanned 59927299
> compact_free_scanned 2021075136
> compact_isolated 640926
> compact_stall 4
> compact_fail 2
> compact_success 2
> compact_daemon_wake 5160
> 
> Further notes from Mel that motivated him to pick this patch up and
> resend it;
> 
> It was observed for the simoop workload (pressures the VM similar to HADOOP)
> that kswapd was failing to keep ahead of direct reclaim. The investigation
> noted that there was a need to rationalise kswapd decisions to reclaim
> with kswapd decisions to sleep. With this patch on a 2-socket box, there
> was a 43% reduction in direct reclaim scanning.
> 
> However, the impact otherwise is extremely negative. Kswapd reclaim
> efficiency dropped from 98% to 76%. simoop has three latency-related
> metrics for read, write and allocation (an anonymous mmap and fault).
> 
>                                          4.10.0-rc7            4.10.0-rc7
>                                      mmots-20170209           fixcheck-v1
> Amean    p50-Read             22325202.49 (  0.00%) 20026926.55 ( 10.29%)
> Amean    p95-Read             26102988.80 (  0.00%) 27023360.00 ( -3.53%)
> Amean    p99-Read             30935176.53 (  0.00%) 30994432.00 ( -0.19%)
> Amean    p50-Write                 976.44 (  0.00%)     1905.28 (-95.12%)
> Amean    p95-Write               15471.29 (  0.00%)    36210.09 (-134.05%)
> Amean    p99-Write               35108.62 (  0.00%)   479494.96 (-1265.75%)
> Amean    p50-Allocation          76382.61 (  0.00%)    87603.20 (-14.69%)
> Amean    p95-Allocation         127777.39 (  0.00%)   244491.38 (-91.34%)
> Amean    p99-Allocation         187937.39 (  0.00%)  1745237.33 (-828.63%)
> 
> There are also more allocation stalls. One of the largest impacts was due
> to pages written back from kswapd context rising from 0 pages to 4516642
> pages during the hour the workload ran for. By and large, the patch has very
> bad behaviour but easily missed as the impact on a UMA machine is negligible.
> 
> This patch is included with the data in case a bisection leads to this area.
> This patch is also a pre-requisite for the rest of the series.
> 
> Signed-off-by: Shantanu Goel <sgoel01@yahoo.com>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> ---
Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>

>  mm/vmscan.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 26c3b405ef34..92fc66bd52bc 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -3140,11 +3140,11 @@ static bool prepare_kswapd_sleep(pg_data_t *pgdat, int order, int classzone_idx)
>  		if (!managed_zone(zone))
>  			continue;
> 
> -		if (!zone_balanced(zone, order, classzone_idx))
> -			return false;
> +		if (zone_balanced(zone, order, classzone_idx))
> +			return true;
>  	}
> 
> -	return true;
> +	return false;
>  }
> 
>  /*
> --
> 2.11.0

[toc] | [prev] | [next] | [standalone]

#1585957 — Re: [PATCH 1/3] mm, vmscan: fix zone balance check in prepare_kswapd_sleep

Hi,

On Wed, Feb 15, 2017 at 09:22:45AM +0000, Mel Gorman wrote:
> From: Shantanu Goel <sgoel01@yahoo.com>
> 
> The check in prepare_kswapd_sleep needs to match the one in balance_pgdat
> since the latter will return as soon as any one of the zones in the
> classzone is above the watermark.  This is specially important for higher
> order allocations since balance_pgdat will typically reset the order to
> zero relying on compaction to create the higher order pages.  Without this
> patch, prepare_kswapd_sleep fails to wake up kcompactd since the zone
> balance check fails.
> 
> On 4.9.7 kswapd is failing to wake up kcompactd due to a mismatch in the
> zone balance check between balance_pgdat() and prepare_kswapd_sleep().
> balance_pgdat() returns as soon as a single zone satisfies the allocation
> but prepare_kswapd_sleep() requires all zones to do +the same.  This causes
> prepare_kswapd_sleep() to never succeed except in the order == 0 case and
> consequently, wakeup_kcompactd() is never called.  On my machine prior to
> apply this patch, the state of compaction from /proc/vmstat looked this
> way after a day and a half +of uptime:
> 
> compact_migrate_scanned 240496
> compact_free_scanned 76238632
> compact_isolated 123472
> compact_stall 1791
> compact_fail 29
> compact_success 1762
> compact_daemon_wake 0
> 
> After applying the patch and about 10 hours of uptime the state looks
> like this:
> 
> compact_migrate_scanned 59927299
> compact_free_scanned 2021075136
> compact_isolated 640926
> compact_stall 4
> compact_fail 2
> compact_success 2
> compact_daemon_wake 5160
> 
> Further notes from Mel that motivated him to pick this patch up and
> resend it;
> 
> It was observed for the simoop workload (pressures the VM similar to HADOOP)
> that kswapd was failing to keep ahead of direct reclaim. The investigation
> noted that there was a need to rationalise kswapd decisions to reclaim
> with kswapd decisions to sleep. With this patch on a 2-socket box, there
> was a 43% reduction in direct reclaim scanning.
> 
> However, the impact otherwise is extremely negative. Kswapd reclaim
> efficiency dropped from 98% to 76%. simoop has three latency-related
> metrics for read, write and allocation (an anonymous mmap and fault).
> 
>                                          4.10.0-rc7            4.10.0-rc7
>                                      mmots-20170209           fixcheck-v1
> Amean    p50-Read             22325202.49 (  0.00%) 20026926.55 ( 10.29%)
> Amean    p95-Read             26102988.80 (  0.00%) 27023360.00 ( -3.53%)
> Amean    p99-Read             30935176.53 (  0.00%) 30994432.00 ( -0.19%)
> Amean    p50-Write                 976.44 (  0.00%)     1905.28 (-95.12%)
> Amean    p95-Write               15471.29 (  0.00%)    36210.09 (-134.05%)
> Amean    p99-Write               35108.62 (  0.00%)   479494.96 (-1265.75%)
> Amean    p50-Allocation          76382.61 (  0.00%)    87603.20 (-14.69%)
> Amean    p95-Allocation         127777.39 (  0.00%)   244491.38 (-91.34%)
> Amean    p99-Allocation         187937.39 (  0.00%)  1745237.33 (-828.63%)
> 
> There are also more allocation stalls. One of the largest impacts was due
> to pages written back from kswapd context rising from 0 pages to 4516642
> pages during the hour the workload ran for. By and large, the patch has very
> bad behaviour but easily missed as the impact on a UMA machine is negligible.
> 
> This patch is included with the data in case a bisection leads to this area.
> This patch is also a pre-requisite for the rest of the series.
> 
> Signed-off-by: Shantanu Goel <sgoel01@yahoo.com>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>

Hmm, I don't understand why we should bind wakeup_kcompactd to kswapd's
short sleep point where every eligible zones are balanced.
What's the correlation between them?

Can't we wake up kcompactd once we found a zone has enough free pages
above high watermark like this?

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 26c3b405ef34..f4f0ad0e9ede 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -3346,13 +3346,6 @@ static void kswapd_try_to_sleep(pg_data_t *pgdat, int alloc_order, int reclaim_o
 		 * that pages and compaction may succeed so reset the cache.
 		 */
 		reset_isolation_suitable(pgdat);
-
-		/*
-		 * We have freed the memory, now we should compact it to make
-		 * allocation of the requested order possible.
-		 */
-		wakeup_kcompactd(pgdat, alloc_order, classzone_idx);
-
 		remaining = schedule_timeout(HZ/10);
 
 		/*
@@ -3451,6 +3444,14 @@ static int kswapd(void *p)
 		bool ret;
 
 kswapd_try_sleep:
+		/*
+		 * We have freed the memory, now we should compact it to make
+		 * allocation of the requested order possible.
+		 */
+		if (alloc_order > 0 && zone_balanced(zone, reclaim_order,
+							classzone_idx))
+			wakeup_kcompactd(pgdat, alloc_order, classzone_idx);
+
 		kswapd_try_to_sleep(pgdat, alloc_order, reclaim_order,
 					classzone_idx);
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1586943 — Re: [PATCH 1/3] mm, vmscan: fix zone balance check in prepare_kswapd_sleep

On Wed, Feb 22, 2017 at 04:00:36PM +0900, Minchan Kim wrote:
> > There are also more allocation stalls. One of the largest impacts was due
> > to pages written back from kswapd context rising from 0 pages to 4516642
> > pages during the hour the workload ran for. By and large, the patch has very
> > bad behaviour but easily missed as the impact on a UMA machine is negligible.
> > 
> > This patch is included with the data in case a bisection leads to this area.
> > This patch is also a pre-requisite for the rest of the series.
> > 
> > Signed-off-by: Shantanu Goel <sgoel01@yahoo.com>
> > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> 
> Hmm, I don't understand why we should bind wakeup_kcompactd to kswapd's
> short sleep point where every eligible zones are balanced.
> What's the correlation between them?
> 

If kswapd is ready for a short sleep, eligible zones are balanced for
order-0 but not necessarily the originally requested order if kswapd
gave up reclaiming as compaction was ready to start. As kswapd is ready
to sleep for a short period, it's a suitable time for kcompactd to decide
if it should start working or not. There is no need for kswapd to be aware
of kcompactd's wakeup criteria.

> Can't we wake up kcompactd once we found a zone has enough free pages
> above high watermark like this?
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 26c3b405ef34..f4f0ad0e9ede 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -3346,13 +3346,6 @@ static void kswapd_try_to_sleep(pg_data_t *pgdat, int alloc_order, int reclaim_o
>  		 * that pages and compaction may succeed so reset the cache.
>  		 */
>  		reset_isolation_suitable(pgdat);
> -
> -		/*
> -		 * We have freed the memory, now we should compact it to make
> -		 * allocation of the requested order possible.
> -		 */
> -		wakeup_kcompactd(pgdat, alloc_order, classzone_idx);
> -
>  		remaining = schedule_timeout(HZ/10);
>  
>  		/*
> @@ -3451,6 +3444,14 @@ static int kswapd(void *p)
>  		bool ret;
>  
>  kswapd_try_sleep:
> +		/*
> +		 * We have freed the memory, now we should compact it to make
> +		 * allocation of the requested order possible.
> +		 */
> +		if (alloc_order > 0 && zone_balanced(zone, reclaim_order,
> +							classzone_idx))
> +			wakeup_kcompactd(pgdat, alloc_order, classzone_idx);
> +
>  		kswapd_try_to_sleep(pgdat, alloc_order, reclaim_order,
>  					classzone_idx);

That's functionally very similar to what happens already.  wakeup_kcompactd
checks the order and does not wake for order-0. It also makes its own
decisions that include zone_balanced on whether it is safe to wakeup.

I doubt there would be any measurable difference from a patch like this
and to my mind at least, it does not improve the readability or flow of
the code.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1587226 — Re: [PATCH 1/3] mm, vmscan: fix zone balance check in prepare_kswapd_sleep

Hi Mel,

On Thu, Feb 23, 2017 at 03:05:34PM +0000, Mel Gorman wrote:
> On Wed, Feb 22, 2017 at 04:00:36PM +0900, Minchan Kim wrote:
> > > There are also more allocation stalls. One of the largest impacts was due
> > > to pages written back from kswapd context rising from 0 pages to 4516642
> > > pages during the hour the workload ran for. By and large, the patch has very
> > > bad behaviour but easily missed as the impact on a UMA machine is negligible.
> > > 
> > > This patch is included with the data in case a bisection leads to this area.
> > > This patch is also a pre-requisite for the rest of the series.
> > > 
> > > Signed-off-by: Shantanu Goel <sgoel01@yahoo.com>
> > > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> > 
> > Hmm, I don't understand why we should bind wakeup_kcompactd to kswapd's
> > short sleep point where every eligible zones are balanced.
> > What's the correlation between them?
> > 
> 
> If kswapd is ready for a short sleep, eligible zones are balanced for
> order-0 but not necessarily the originally requested order if kswapd
> gave up reclaiming as compaction was ready to start. As kswapd is ready
> to sleep for a short period, it's a suitable time for kcompactd to decide
> if it should start working or not. There is no need for kswapd to be aware
> of kcompactd's wakeup criteria.

If all eligible zones are balanced for order-0, I agree it's good timing
because high-order alloc's ratio would be higher since kcompactd can compact
eligible zones, not that only classzone.
However, this patch breaks it as well as long time kswapd behavior which
continues to balance eligible zones for order-0.
Is it really okay now?

> 
> > Can't we wake up kcompactd once we found a zone has enough free pages
> > above high watermark like this?
> > 
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 26c3b405ef34..f4f0ad0e9ede 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -3346,13 +3346,6 @@ static void kswapd_try_to_sleep(pg_data_t *pgdat, int alloc_order, int reclaim_o
> >  		 * that pages and compaction may succeed so reset the cache.
> >  		 */
> >  		reset_isolation_suitable(pgdat);
> > -
> > -		/*
> > -		 * We have freed the memory, now we should compact it to make
> > -		 * allocation of the requested order possible.
> > -		 */
> > -		wakeup_kcompactd(pgdat, alloc_order, classzone_idx);
> > -
> >  		remaining = schedule_timeout(HZ/10);
> >  
> >  		/*
> > @@ -3451,6 +3444,14 @@ static int kswapd(void *p)
> >  		bool ret;
> >  
> >  kswapd_try_sleep:
> > +		/*
> > +		 * We have freed the memory, now we should compact it to make
> > +		 * allocation of the requested order possible.
> > +		 */
> > +		if (alloc_order > 0 && zone_balanced(zone, reclaim_order,
> > +							classzone_idx))
> > +			wakeup_kcompactd(pgdat, alloc_order, classzone_idx);
> > +
> >  		kswapd_try_to_sleep(pgdat, alloc_order, reclaim_order,
> >  					classzone_idx);
> 
> That's functionally very similar to what happens already.  wakeup_kcompactd
> checks the order and does not wake for order-0. It also makes its own
> decisions that include zone_balanced on whether it is safe to wakeup.

Agree.

> 
> I doubt there would be any measurable difference from a patch like this
> and to my mind at least, it does not improve the readability or flow of
> the code.

However, my concern is premature kswapd sleep for order-0 which has been
long time behavior so I hope it should be documented why it's okay now.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1587445 — Re: [PATCH 1/3] mm, vmscan: fix zone balance check in prepare_kswapd_sleep

On Fri, Feb 24, 2017 at 10:17:06AM +0900, Minchan Kim wrote:
> Hi Mel,
> 
> On Thu, Feb 23, 2017 at 03:05:34PM +0000, Mel Gorman wrote:
> > On Wed, Feb 22, 2017 at 04:00:36PM +0900, Minchan Kim wrote:
> > > > There are also more allocation stalls. One of the largest impacts was due
> > > > to pages written back from kswapd context rising from 0 pages to 4516642
> > > > pages during the hour the workload ran for. By and large, the patch has very
> > > > bad behaviour but easily missed as the impact on a UMA machine is negligible.
> > > > 
> > > > This patch is included with the data in case a bisection leads to this area.
> > > > This patch is also a pre-requisite for the rest of the series.
> > > > 
> > > > Signed-off-by: Shantanu Goel <sgoel01@yahoo.com>
> > > > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> > > 
> > > Hmm, I don't understand why we should bind wakeup_kcompactd to kswapd's
> > > short sleep point where every eligible zones are balanced.
> > > What's the correlation between them?
> > > 
> > 
> > If kswapd is ready for a short sleep, eligible zones are balanced for
> > order-0 but not necessarily the originally requested order if kswapd
> > gave up reclaiming as compaction was ready to start. As kswapd is ready
> > to sleep for a short period, it's a suitable time for kcompactd to decide
> > if it should start working or not. There is no need for kswapd to be aware
> > of kcompactd's wakeup criteria.
> 
> If all eligible zones are balanced for order-0, I agree it's good timing
> because high-order alloc's ratio would be higher since kcompactd can compact
> eligible zones, not that only classzone.
> However, this patch breaks it as well as long time kswapd behavior which
> continues to balance eligible zones for order-0.
> Is it really okay now?
> 

Reclaim stops in balance_pgdat() if any eligible zone for the requested
classzone is free. The initial sleep for kswapd is very different because
it'll sleep if all zones are balanced for order-0 which is a bad disconnect.
The way node balancing works means there is no guarantee at all that all
zones will be balanced even if there is little or no memory pressure and
one large zone in a node with multiple zones can be balanced quickly.

The short-sleep logic that kswapd uses to decide whether to go to sleep
is shortcut and it does not properly try the short sleep checking if the
high watermarks are quickly reached or not. Instead, it quickly fails the
first attempt at sleep, reenters balance_pgdat(), finds nothing to do and
rechecks sleeping based on order-0, classzone-0 which it can easily sleep
for but is *not* what kswapd was woken for in the first place.

For many allocation requests that initially woke kswapd, the impact is
marginal. kswapd sleeps early and is woken in the near future if there
is a continual stream of allocations with a risk that direct reclaim is
required. While the motivation for the patch was that kcompact is not woken
up, the existing behaviour is just wrong -- kswapd should be deciding to
sleep based on the classzone it was woken for and if possible, the order
it was woken for but the classzone is more important in the common case
for order-0 allocations.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1588520 — Re: [PATCH 1/3] mm, vmscan: fix zone balance check in prepare_kswapd_sleep

Hi Mel,

On Fri, Feb 24, 2017 at 09:11:28AM +0000, Mel Gorman wrote:
> On Fri, Feb 24, 2017 at 10:17:06AM +0900, Minchan Kim wrote:
> > Hi Mel,
> > 
> > On Thu, Feb 23, 2017 at 03:05:34PM +0000, Mel Gorman wrote:
> > > On Wed, Feb 22, 2017 at 04:00:36PM +0900, Minchan Kim wrote:
> > > > > There are also more allocation stalls. One of the largest impacts was due
> > > > > to pages written back from kswapd context rising from 0 pages to 4516642
> > > > > pages during the hour the workload ran for. By and large, the patch has very
> > > > > bad behaviour but easily missed as the impact on a UMA machine is negligible.
> > > > > 
> > > > > This patch is included with the data in case a bisection leads to this area.
> > > > > This patch is also a pre-requisite for the rest of the series.
> > > > > 
> > > > > Signed-off-by: Shantanu Goel <sgoel01@yahoo.com>
> > > > > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> > > > 
> > > > Hmm, I don't understand why we should bind wakeup_kcompactd to kswapd's
> > > > short sleep point where every eligible zones are balanced.
> > > > What's the correlation between them?
> > > > 
> > > 
> > > If kswapd is ready for a short sleep, eligible zones are balanced for
> > > order-0 but not necessarily the originally requested order if kswapd
> > > gave up reclaiming as compaction was ready to start. As kswapd is ready
> > > to sleep for a short period, it's a suitable time for kcompactd to decide
> > > if it should start working or not. There is no need for kswapd to be aware
> > > of kcompactd's wakeup criteria.
> > 
> > If all eligible zones are balanced for order-0, I agree it's good timing
> > because high-order alloc's ratio would be higher since kcompactd can compact
> > eligible zones, not that only classzone.
> > However, this patch breaks it as well as long time kswapd behavior which
> > continues to balance eligible zones for order-0.
> > Is it really okay now?
> > 
> 
> Reclaim stops in balance_pgdat() if any eligible zone for the requested
> classzone is free. The initial sleep for kswapd is very different because
> it'll sleep if all zones are balanced for order-0 which is a bad disconnect.
> The way node balancing works means there is no guarantee at all that all
> zones will be balanced even if there is little or no memory pressure and
> one large zone in a node with multiple zones can be balanced quickly.

Indeed but it would tip toward direct relcaim more so it could make more
failure for allocation relies on kswapd like atomic allocation
However, if VM balance all of zones for order-0, it would make excessive
reclaim with node-based LRU unlike zone-based, which is bad, too.

> 
> The short-sleep logic that kswapd uses to decide whether to go to sleep
> is shortcut and it does not properly try the short sleep checking if the
> high watermarks are quickly reached or not. Instead, it quickly fails the
> first attempt at sleep, reenters balance_pgdat(), finds nothing to do and
> rechecks sleeping based on order-0, classzone-0 which it can easily sleep
> for but is *not* what kswapd was woken for in the first place.
> 
> For many allocation requests that initially woke kswapd, the impact is
> marginal. kswapd sleeps early and is woken in the near future if there
> is a continual stream of allocations with a risk that direct reclaim is
> required. While the motivation for the patch was that kcompact is not woken
> up, the existing behaviour is just wrong -- kswapd should be deciding to
> sleep based on the classzone it was woken for and if possible, the order
> it was woken for but the classzone is more important in the common case
> for order-0 allocations.

I agree but I think it's rather risky to paper over order-0 zone-balancing
problem by kcompactd missing problem so at least, it should be documented.

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web