vmscan: fix zone shrinking exit when scan work is done

Hi,

I think this should fix the problem of processes getting stuck in reclaim that has been reported several times. Kent actually single-stepped through this code and noted that it was never exiting shrink_zone(), which really narrowed it down a lot, considering the tons of nested loops from the allocator down to the list shrinking.

Hannes

---
From: Johannes Weiner<[email protected]>
Subject: vmscan: fix zone shrinking exit when scan work is done

'3e7d344 mm: vmscan: reclaim order-0 and use compaction instead of lumpy reclaim' introduced an indefinite loop in shrink_zone().

It meant to break out of this loop when no pages had been reclaimed and not a single page was even scanned. The way it would detect the latter is by taking a snapshot of sc->nr_scanned at the beginning of the function and comparing it against the new sc->nr_scanned after the scan loop. But it would re-iterate without updating that snapshot, looping forever if sc->nr_scanned changed at least once since shrink_zone() was invoked.

This is not the sole condition that would exit that loop, but it requires other processes to change the zone state, as the reclaimer that is stuck obviously can not anymore.

This is only happening for higher-order allocations, where reclaim is run back to back with compaction.

Reported-by Michal Hocko<[email protected]>

Reported-by Kent Overstreet<[email protected]>

Signed-off-by Johannes Weiner<[email protected]>

---
  mm/vmscan.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

mm/vmscan.c +0 -1

index 148c6e6..17497d0 100644
@@ -1882,12 +1882,12 @@ static void shrink_zone(int priority, struct zone *zone,
  	unsigned long nr[NR_LRU_LISTS];
  	unsigned long nr_to_scan;
  	enum lru_list l;
-	unsigned long nr_reclaimed;

Hi,

I think this should fix the problem of processes getting stuck in reclaim that has been reported several times.

Kent actually
single-stepped through this code and noted that it was never exiting shrink_zone(), which really narrowed it down a lot, considering the tons of nested loops from the allocator down to the list shrinking.

Hannes

---
From: Johannes Weiner <[email protected]>
Subject: vmscan: fix zone shrinking exit when scan work is done

'3e7d344 mm: vmscan: reclaim order-0 and use compaction instead of lumpy reclaim' introduced an indefinite loop in shrink_zone().

It meant to break out of this loop when no pages had been reclaimed and not a single page was even scanned. The way it would detect the latter is by taking a snapshot of sc->nr_scanned at the beginning of the function and comparing it against the new sc->nr_scanned after the scan loop. But it would re-iterate without updating that snapshot, looping forever if sc->nr_scanned changed at least once since shrink_zone() was invoked.

This is not the sole condition that would exit that loop, but it requires other processes to change the zone state, as the reclaimer that is stuck obviously can not anymore.

This is only happening for higher-order allocations, where reclaim is run back to back with compaction.

Reported-by Michal Hocko <[email protected]>

Reported-by Kent Overstreet <[email protected]>

Signed-off-by Johannes Weiner <[email protected]>

Quoted 4 lines · Johannes Weiner

Hi,

I think this should fix the problem of processes getting stuck in reclaim that has been reported several times.

I don't think it's the only source but I'm basing this on seeing constant looping in balance_pgdat() and calling congestion_wait() a few weeks ago that I haven't rechecked since. However, this looks like a real fix for a real problem.

Quoted 10 lines · Mel Gorman

Quoted 4 lines · Johannes Weiner

Hi,

I think this should fix the problem of processes getting stuck in reclaim that has been reported several times.

I don't think it's the only source but I'm basing this on seeing constant looping in balance_pgdat() and calling congestion_wait() a few weeks ago that I haven't rechecked since. However, this looks like a real fix for a real problem.

Agreed. Just yesterday I spent some time on the lumpy compaction changes after wondering about Michal's khugepaged 100% report, and I expected some fix was needed in this area (as I couldn't find any bug in khugepaged yet, so the lumpy compaction looked the next candidate for bugs).

I've also been wondering about the !nr_scanned check in should_continue_reclaim too but I didn't look too much into the caller (I was tempted to remove it all together). I don't see how checking nr_scanned can be safe even after we fix the caller to avoid passing non-zero values if "goto restart".

nr_scanned is incremented even for !page_evictable... so it's not really useful to insist, just because we scanned something, in my view. It looks bogus... So my proposal would be below.

====
Subject: mm: stop checking nr_scanned in should_continue_reclaim

From: Andrea Arcangeli <[email protected]>

nr_scanned is incremented even for !page_evictable... so it's not really useful to insist, just because we scanned something.

Quoted 10 lines · Mel Gorman

Quoted 4 lines · Johannes Weiner

Hi,

I think this should fix the problem of processes getting stuck in reclaim that has been reported several times.

I don't think it's the only source but I'm basing this on seeing constant looping in balance_pgdat() and calling congestion_wait() a few weeks ago that I haven't rechecked since. However, this looks like a real fix for a real problem.

Agreed. Just yesterday I spent some time on the lumpy compaction changes after wondering about Michal's khugepaged 100% report, and I expected some fix was needed in this area (as I couldn't find any bug in khugepaged yet, so the lumpy compaction looked the next candidate for bugs).

I've also been wondering about the !nr_scanned check in should_continue_reclaim too but I didn't look too much into the caller (I was tempted to remove it all together). I don't see how checking nr_scanned can be safe even after we fix the caller to avoid passing non-zero values if "goto restart".

nr_scanned is incremented even for !page_evictable... so it's not really useful to insist, just because we scanned something, in my view. It looks bogus... So my proposal would be below.

====
Subject: mm: stop checking nr_scanned in should_continue_reclaim

From: Andrea Arcangeli <[email protected]>

nr_scanned is incremented even for !page_evictable... so it's not really useful to insist, just because we scanned something.

Signed-off-by Andrea Arcangeli <[email protected]>

---

mm/vmscan.c +3 -13

index 148c6e6..9741884 100644
@@ -1831,7 +1831,6 @@ out:
  */
 static inline bool should_continue_reclaim(struct zone *zone,
 					unsigned long nr_reclaimed,
-					unsigned long nr_scanned,
 					struct scan_control *sc)
 {
 	unsigned long pages_for_compaction;

@@ -1841,15 +1840,8 @@ static inline bool should_continue_reclaim(struct zone *zone,
 	if (!(sc->reclaim_mode & RECLAIM_MODE_COMPACTION))
 		return false;
 
-	/*
-	 * If we failed to reclaim and have scanned the full list, stop.
-	 * NOTE: Checking just nr_reclaimed would exit reclaim/compaction far
-	 *       faster but obviously would be less likely to succeed
-	 *       allocation. If this is desirable, use GFP_REPEAT to decide
-	 *       if both reclaimed and scanned should be checked or just
-	 *       reclaimed
-	 */
-	if (!nr_reclaimed && !nr_scanned)
+	/* If we failed to reclaim stop. */
+	if (!nr_reclaimed)
 		return false;
 
 	/*

@@ -1884,7 +1876,6 @@ static void shrink_zone(int priority, struct zone *zone,
 	enum lru_list l;
 	unsigned long nr_reclaimed;
 	unsigned long nr_to_reclaim = sc->nr_to_reclaim;
-	unsigned long nr_scanned = sc->nr_scanned;
 
 restart:
 	nr_reclaimed = 0;

@@ -1923,8 +1914,7 @@ restart:
 		shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
 
 	/* reclaim/compaction might need reclaim to continue */
-	if (should_continue_reclaim(zone, nr_reclaimed,
-					sc->nr_scanned - nr_scanned, sc))
+	if (should_continue_reclaim(zone, nr_reclaimed, sc))
 		goto restart;
 
 	throttle_vm_writeout(sc->gfp_mask);

Quoted 12 lines · Andrea Arcangeli

Quoted 10 lines · Mel Gorman

Quoted 4 lines · Johannes Weiner

Hi,

I think this should fix the problem of processes getting stuck in reclaim that has been reported several times.

I don't think it's the only source but I'm basing this on seeing constant looping in balance_pgdat() and calling congestion_wait() a few weeks ago that I haven't rechecked since. However, this looks like a real fix for a real problem.

Quoted 6 lines

Agreed. Just yesterday I spent some time on the lumpy compaction changes after wondering about Michal's khugepaged 100% report, and I expected some fix was needed in this area (as I couldn't find any bug in khugepaged yet, so the lumpy compaction looked the next candidate for bugs).

Michal did report that disabling defrag did not help but the stack trace also showed that it was stuck in shrink_zone() which is what Johannes' patch targets. It's not unreasonable to test if Johannes' patch solves Michal's problem. Michal, I know that your workload is a bit random and may not be reproducible but do you think it'd be possible to determine if Johannes' patch helps?

We should not be ending up in a situation with the LRU list of only page_evictable pages and that situation persisting causing excessive (or infinite) looping. As unevictable pages are encountered on the LRU list, they should be moved to the unevictable lists by putback_lru_page(). Are you aware of a situation where this becomes broken?

I recognise that SWAP_CLUSTER_MAX pages could all be unevictable and they are all get moved. In this case, nr_scanned is positive and we continue to scan but this is expected and desirable: Reclaim/compaction needs more pages to be freed before it starts compaction. If it stops scanning early, then it would just fail the allocation later. This is what the "NOTE" is about.

I prefer Johannes' fix for the observed problem.

Quoted 12 lines · Mel Gorman

We should not be ending up in a situation with the LRU list of only page_evictable pages and that situation persisting causing excessive (or infinite) looping. As unevictable pages are encountered on the LRU list, they should be moved to the unevictable lists by putback_lru_page(). Are you aware of a situation where this becomes broken?

I recognise that SWAP_CLUSTER_MAX pages could all be unevictable and they are all get moved. In this case, nr_scanned is positive and we continue to scan but this is expected and desirable: Reclaim/compaction needs more pages to be freed before it starts compaction. If it stops scanning early, then it would just fail the allocation later. This is what the "NOTE" is about.

Quoted 1 line

I prefer Johannes' fix for the observed problem.

should_continue_reclaim is only needed for compaction. It tries to free enough pages so that compaction can succeed in its defrag attempt.

So breaking the loop faster isn't going to cause failures for 0 order pages.

My worry is that we loop too much in shrink_zone just
for compaction even when we don't do any progress. shrink_zone would never scan more than SWAP_CLUSTER_MAX pages, before this change.

Now
it can loop over the whole lru as long as we're scanning stuff.

Ok to
overboost shrink_zone if we're making progress to allow compaction at the next round, but if we don't visibly make progress, I'm concerned that it may be too aggressive to scan the whole list. The performance benefit of having an hugepage isn't as huge as scanning all pages in the lru when before we would have broken the loop and declared failure after only SWAP_CLUSTER_MAX pages, and then we would have fallen back in a order 0 allocation.

The fix may help of course, maybe it's enough
for his case I don't know, but I don't see it making a whole lot of difference, except now it will stop when the lru is practically empty which clearly is an improvement. I think we shouldn't be so worried about succeeding compaction, the important thing is we don't waste time in compaction if there's not enough free memory but compaction_suitable used by both logics should be enough for that.

I'd rather prefer that if hugetlbfs has special needs it uses a __GFP_

flag or similar that increases how compaction is strict in succeeding, up to scanning the whole lru in one go in order to make some free memory for compaction to succeed.

Going ahead with the scan until compaction_suitable is true instead makes sense when there's absence of memory pressure and nr_reclaimed is never zero.

Maybe we should try a bit more times than just nr_reclaim but going over the whole lru, sounds a bit extreme.

The issue isn't just for unevictable pages, that will be refiled during the scan but it will also happen in presence of lots of referenced pages. For example if we don't apply my fix, the current code can take down all young bits in all ptes in one go in the whole system before returning from shrink_zone, that is too much in my view, and losing all that information in one go (not even to tell the cost associated with losing it) can hardly be offseted by the improvement given by 1 more hugepage.

But please let me know if I've misread something...

Also true, I commented on this in the "Note" your patch deletes and a suggestion on how an alternative would be to break early unless GFP_REPEAT.

Sortof. Lumpy reclaim would have scanned more than SWAP_CLUSTER_MAX so scanning was still pretty high. The other costs of lumpy reclaim would hide it of course.

What about other cases such as order-1 allocations for stack or order-3 allocations for those network cards using jumbo frames without scatter/gather?

Don't get me wrong, I see your point but I'm wondering if there really are cases where we routinely scan an entire LRU list of unevictable pages that are somehow not being migrated properly to the unevictable lists. If this is happening, we are also in trouble for reclaiming for order-0 pages, right?

It uses GFP_REPEAT. That is why I specifically mentioned it in the "NOTE" as an alternative to how we could break early while still being agressive when required. The only reason it's not that way now is because a) I didn't consider an LRU mostly full of unevictable pages to be the normal case and b) for allocations such as order-3 that are preferable not to fail.

Where should be draw the line? We could come up with ratio of the lists depending on priority but it'd be hard to measure the gain or loss without having a profile of a problem case to look at.

I don't think you have misread anything but if we're going to weaken this logic, I'd at least like to see the GFP_REPEAT option tried - i.e.

preserve being aggressive if set. I'm also not convinced we routinely get into a situation where the LRU consists of almost all unevictable pages and if we are in this situation, that is a serious problem on its own. It would also be preferable if we could get latency figures on alloc_pages for hugepage-sized allocations and a count of how many are succeeding or failing to measure the impact (if any).

Quoted 2 lines · Mel Gorman

Also true, I commented on this in the "Note" your patch deletes and a suggestion on how an alternative would be to break early unless GFP_REPEAT.

Yep noticed that ;), doing that with __GFP_REPEAT sounds just fine to me.

Quoted 3 lines

Sortof. Lumpy reclaim would have scanned more than SWAP_CLUSTER_MAX so scanning was still pretty high. The other costs of lumpy reclaim would hide it of course.

Ok but we know lumpy reclaim was not ok to start with.

Quoted 3 lines

What about other cases such as order-1 allocations for stack or order-3 allocations for those network cards using jumbo frames without scatter/gather?

stack order 1 is one of the few cases that come to mind where failing an allocation becomes fatal. Maybe we should use __GFP_REPEAT there too.

But we probably need a way to discriminate callers that can gracefully fallback. I'd be extremely surprised if the cost of looping all over the lru taking down all young bits could ever be offseted by the jumbo frame. In fact the jumbo frame I'm afraid might be better off without using compaction at all because it's probably very latency sensitive.

We need a 'lowlatency' version of compaction for these users where the improvement of having a compound page instead of a regular page isn't very significant.

On a separated topic, I'm currently trying to use the new async compaction code upstream with jumbo frames. I'm also wondering if I'll have to set sync=0 by default unless __GFP_REPEAT is set. It seems adding compaction to jumbo frames is increasing latency to certain workloads in a measurable way.

Things were fine when compaction was
only used by THP and not for all order allocations (but I didn't test the async mode yet plus the other optimizations for compaction you did recently, I hope they're enough to jumbo frames).

Quoted 5 lines

Don't get me wrong, I see your point but I'm wondering if there really are cases where we routinely scan an entire LRU list of unevictable pages that are somehow not being migrated properly to the unevictable lists. If this is happening, we are also in trouble for reclaiming for order-0 pages, right?

Well unevictable pages are just an example and like you said they last only one round of the loop at most. But other caching bits like the referenced bits and all young bits will get all taken down during all later loops too. We definitely don't want to swap just to allow compaction to succeed! I think this argument explains it pretty well, if you takedown all young bits in a constant loop, then system may end up swapping. That's definitely something we don't want.

Things may be different if this is a stack allocation without fallback, or if it's hugetlbfs again without kernel fallback (only userland fallback).

Quoted 5 lines

It uses GFP_REPEAT. That is why I specifically mentioned it in the "NOTE" as an alternative to how we could break early while still being agressive when required. The only reason it's not that way now is because a) I didn't consider an LRU mostly full of unevictable pages to be the normal case and b) for allocations such as order-3 that are preferable not to fail.

Ok.

Quoted 3 lines

Where should be draw the line? We could come up with ratio of the lists depending on priority but it'd be hard to measure the gain or loss without having a profile of a problem case to look at.

I would just stick to !nr_reclaimed to break the loop, and ignore the nr_scanned unless __GFP_REPEAT is set, in which case you're welcome to scan everything.

Then we've to decide if to add __GFP_REPEAT to the stack allocation...

Quoted 2 lines

I don't think you have misread anything but if we're going to weaken this logic, I'd at least like to see the GFP_REPEAT option tried - i.e.

I see the point of __GFP_REPEAT, that sounds the best, I should have just followed your comment but I felt scanning everything was too heavyweight regardless, but ok I see you want as much accuracy as possible in that case, even if that end up in a swap storm.

Quoted 6 lines

preserve being aggressive if set. I'm also not convinced we routinely get into a situation where the LRU consists of almost all unevictable pages and if we are in this situation, that is a serious problem on its own. It would also be preferable if we could get latency figures on alloc_pages for hugepage-sized allocations and a count of how many are succeeding or failing to measure the impact (if any).

I think I made not the best example talking about unevictable pages. I said that because the code is like below, but to me all the goto something following the !page_evictable check were also relevant for this shrink_zone loop. The real life issue is avoid swap storm (or expensive loop flooding the whole system of ipis to takedown all young bits in all ptes), to allocate an hugepage or jumboframe that has a graceful fallback that performs not hugely slower than the hugepage/jumboframe.

     	  sc->nr_scanned++;

          if (unlikely(!page_evictable(page, NULL)))
	      goto cull_mlocked;

I think making the !nr_scanned check conditional to __GFP_REPEAT as the comment suggested, for now is the best way to go.

The mean allocation times for THP allocations are also slightly reduced. The maximum latency was slightly increased as predicted by the comments due to reclaim/compaction breaking early. However, workloads care more about the latency of lower-order allocations than THP so it's an acceptable trade-off. Please consider merging for 2.6.38.

Quoted 5 lines · Mel Gorman

The mean allocation times for THP allocations are also slightly reduced. The maximum latency was slightly increased as predicted by the comments due to reclaim/compaction breaking early. However, workloads care more about the latency of lower-order allocations than THP so it's an acceptable trade-off. Please consider merging for 2.6.38.

Full agreement. I'm currently dealing with latency issues (nothing major! but still not nice to see a reproducible regression, even if a small one only visible in the benchmark) with compaction and jumbo frames.

This won't be enough to close them completely though because I didn't backport the change to vmscan.c and should_continue_reclaim (I backported all the other compaction improvements though, so this practically is the only missing bit).

I also suspected the e1000
driver, that sets the NAPI latency to bulk_latency when it uses jumbo frames, so I wonder if it could be that with compaction we get more jumbo frames and the latency then gets reduced by the driver as side effect. Not sure yet.

I like the above because it's less likely to give us compaction issues with jumbo frames when I add should_continue_reclaim on top. It seems anonymous memory allocation are orders of magnitude more long lived than jumbo frames could ever be, so at this point I'm not even entirely certain it's ok to enable compaction at all for jumbo frames. But I still like the above regardless of my current issue (just because of the young bits going nuked in one go the lumpy hammer way, even if it actually increases latency a bit for THP allocations).

One issue with compaction for jumbo frames, is the potentially very long loop, for the scan in isolated_migratepages.

I added a counter to
break the loop after 1024 pages scanned. This is extreme but this is a debug patch for now, I also did if (retval == bulk_latency) reval = low_latency in the e1000* drivers to see if it makes a difference. If

any of the two will help I will track down how much each change contributes to lowering the network latency to pre-compaction levels. It may very well be only a compaction issue, or only a driver issue, I don't know yet (the latter less likely because this very compaction loop spikes at the top of oprofile output, but maybe that only affects throughput and the driver is to blame for the latency reduction... this is what I'm going to find pretty soon). Also this isolate_migratepages loop I think needs a cond_resched()

(I didn't add
that yet ;). 1024 pages scanned is too few, I just want to see how it behaves with an extremely permissive setting. I'll let you know when I come to some more reliable conclusion.

Out of curiousity, what are you measuring the latency of and how? I used a combination of the function_graph ftrace analyser and the mm_page_alloc tracepoint myself to avoid any additional patching and it was easier than cobbling together something with kprobes. A perl script configures ftrace and then parses the contents of trace_pipe - crude but does the job without patching the kernel.

How big are the discrepancies?

No idea.

Can I have your ack on the patch then? Even if it doesn't resolve the

jumbo frame problems, it's in the right direction. Measuring how it currently behaves and what direction should be taken may be something still worth discussing at LSF/MM.

Quoted 2 lines

One issue with compaction for jumbo frames, is the potentially very long loop, for the scan in isolated_migratepages.

Yes, the scanner is poor. The scanner for free pages is potentially just as bad. I prototyped some designs that should have been faster but they didn't make any significant difference so they got discarded.

This surprises me. In my own tests at least, the compaction stuff was way down in the profile and I wouldn't have expected scanning to take so long as to require a cond_resched. I was depending on the cond_resched() in migrate_pages() to yield the processor as necessary.

Signed-off-by Mel Gorman <[email protected]>

---
 mm/vmscan.c |   32 ++++++++++++++++++++++----------
 1 files changed, 22 insertions(+), 10 deletions(-)

This patch will stop reclaim/compaction if no pages were reclaimed in the last SWAP_CLUSTER_MAX pages that were considered. For allocations such as hugetlbfs that use GFP_REPEAT and have fewer fallback options, the full LRU list may still be scanned.

Signed-off-by Mel Gorman<[email protected]>

should_continue_reclaim() for reclaim/compaction allows scanning to continue even if pages are not being reclaimed until the full list is scanned. In terms of allocation success, this makes sense but potentially it introduces unwanted latency for high-order allocations such as transparent hugepages and network jumbo frames that would prefer to fail the allocation attempt and fallback to order-0 pages. Worse, there is a potential that the full LRU scan will clear all the young bits, distort page aging information and potentially push pages into swap that would have otherwise remained resident.

This patch will stop reclaim/compaction if no pages were reclaimed in the last SWAP_CLUSTER_MAX pages that were considered. For allocations such as hugetlbfs that use GFP_REPEAT and have fewer fallback options, the full LRU list may still be scanned.

To test this, a tool was developed based on ftrace that tracked the latency of high-order allocations while transparent hugepage support was enabled and three benchmarks were run. The "fix-infinite" figures are 2.6.38-rc4 with Johannes's patch "vmscan: fix zone shrinking exit when scan work is done" applied.

STREAM Highorder Allocation Latency Statistics
fix-infinite break-early
1 :: Count 10298 10229

1 :: Min             0.4560          0.4640
1 :: Mean            1.0589          1.0183
1 :: Max            14.5990         11.7510
1 :: Stddev          0.5208          0.4719
2 :: Count                2               1
2 :: Min             1.8610          3.7240
2 :: Mean            3.4325          3.7240
2 :: Max             5.0040          3.7240
2 :: Stddev          1.5715          0.0000
9 :: Count           111696          111694
9 :: Min             0.5230          0.4110
9 :: Mean           10.5831         10.5718
9 :: Max            38.4480         43.2900

9 :: Stddev 1.1147 1.1325

Mean time for order-1 allocations is reduced. order-2 looks increased but with so few allocations, it's not particularly significant. THP mean allocation latency is also reduced. That said, allocation time varies so significantly that the reductions are within noise.

Max allocation time is reduced by a significant amount for low-order allocations but reduced for THP allocations which presumably are now breaking before reclaim has done enough work.

SysBench Highorder Allocation Latency Statistics
fix-infinite break-early
1 :: Count 15745 15677

1 :: Min             0.4250          0.4550
1 :: Mean            1.1023          1.0810
1 :: Max            14.4590         10.8220
1 :: Stddev          0.5117          0.5100
2 :: Count                1               1
2 :: Min             3.0040          2.1530
2 :: Mean            3.0040          2.1530
2 :: Max             3.0040          2.1530
2 :: Stddev          0.0000          0.0000
9 :: Count             2017            1931
9 :: Min             0.4980          0.7480
9 :: Mean           10.4717         10.3840
9 :: Max            24.9460         26.2500

9 :: Stddev 1.1726 1.1966

Again, mean time for order-1 allocations is reduced while order-2 allocations are too few to draw conclusions from. The mean time for THP allocations is also slightly reduced albeit the reductions are within varianes.

Once again, our maximum allocation time is significantly reduced for low-order allocations and slightly increased for THP allocations.

Anon stream mmap reference Highorder Allocation Latency Statistics
1 :: Count 1376 1790

1 :: Min             0.4940          0.5010
1 :: Mean            1.0289          0.9732
1 :: Max             6.2670          4.2540
1 :: Stddev          0.4142          0.2785
2 :: Count                1               -
2 :: Min             1.9060               -
2 :: Mean            1.9060               -
2 :: Max             1.9060               -
2 :: Stddev          0.0000               -

9 :: Count 11266 11257
9 :: Min 0.4990 0.4940
9 :: Mean 27250.4669 24256.1919
9 :: Max 11439211.0000 6008885.0000
9 :: Stddev 226427.4624 186298.1430

This benchmark creates one thread per CPU which references an amount of anonymous memory 1.5 times the size of physical RAM. This pounds swap quite heavily and is intended to exercise THP a bit.

Mean allocation time for order-1 is reduced as before. It's also reduced for THP allocations but the variations here are pretty massive due to swap. As before, maximum allocation times are significantly reduced.

Overall, the patch reduces the mean and maximum allocation latencies for the smaller high-order allocations. This was with Slab configured so it would be expected to be more significant with Slub which uses these size allocations more aggressively.

The mean allocation times for THP allocations are also slightly reduced. The maximum latency was slightly increased as predicted by the comments due to reclaim/compaction breaking early. However, workloads care more about the latency of lower-order allocations than THP so it's an acceptable trade-off. Please consider merging for 2.6.38.

Signed-off-by Mel Gorman <[email protected]>

should_continue_reclaim() for reclaim/compaction allows scanning to continue even if pages are not being reclaimed until the full list is scanned. In terms of allocation success, this makes sense but potentially it introduces unwanted latency for high-order allocations such as transparent hugepages and network jumbo frames that would prefer to fail the allocation attempt and fallback to order-0 pages.  Worse, there is a potential that the full LRU scan will clear all the young bits, distort page aging information and potentially push pages into swap that would have otherwise remained resident.

This patch will stop reclaim/compaction if no pages were reclaimed in the last SWAP_CLUSTER_MAX pages that were considered. For allocations such as hugetlbfs that use GFP_REPEAT and have fewer fallback options, the full LRU list may still be scanned.

To test this, a tool was developed based on ftrace that tracked the latency of high-order allocations while transparent hugepage support was enabled and three benchmarks were run. The "fix-infinite" figures are 2.6.38-rc4 with Johannes's patch "vmscan: fix zone shrinking exit when scan work is done" applied.

STREAM Highorder Allocation Latency Statistics
              fix-infinite     break-early
1 :: Count            10298           10229
1 :: Min             0.4560          0.4640
1 :: Mean            1.0589          1.0183
1 :: Max            14.5990         11.7510
1 :: Stddev          0.5208          0.4719
2 :: Count                2               1
2 :: Min             1.8610          3.7240
2 :: Mean            3.4325          3.7240
2 :: Max             5.0040          3.7240
2 :: Stddev          1.5715          0.0000
9 :: Count           111696          111694
9 :: Min             0.5230          0.4110
9 :: Mean           10.5831         10.5718
9 :: Max            38.4480         43.2900
9 :: Stddev          1.1147          1.1325

Mean time for order-1 allocations is reduced. order-2 looks increased but with so few allocations, it's not particularly significant. THP mean allocation latency is also reduced. That said, allocation time varies so significantly that the reductions are within noise.

Max allocation time is reduced by a significant amount for low-order allocations but reduced for THP allocations which presumably are now breaking before reclaim has done enough work.

SysBench Highorder Allocation Latency Statistics
              fix-infinite     break-early
1 :: Count            15745           15677
1 :: Min             0.4250          0.4550
1 :: Mean            1.1023          1.0810
1 :: Max            14.4590         10.8220
1 :: Stddev          0.5117          0.5100
2 :: Count                1               1
2 :: Min             3.0040          2.1530
2 :: Mean            3.0040          2.1530
2 :: Max             3.0040          2.1530
2 :: Stddev          0.0000          0.0000
9 :: Count             2017            1931
9 :: Min             0.4980          0.7480
9 :: Mean           10.4717         10.3840
9 :: Max            24.9460         26.2500
9 :: Stddev          1.1726          1.1966

Again, mean time for order-1 allocations is reduced while order-2 allocations are too few to draw conclusions from. The mean time for THP allocations is also slightly reduced albeit the reductions are within varianes.

Once again, our maximum allocation time is significantly reduced for low-order allocations and slightly increased for THP allocations.

Anon stream mmap reference Highorder Allocation Latency Statistics 1 :: Count             1376            1790
1 :: Min             0.4940          0.5010
1 :: Mean            1.0289          0.9732
1 :: Max             6.2670          4.2540
1 :: Stddev          0.4142          0.2785
2 :: Count                1               -
2 :: Min             1.9060               -
2 :: Mean            1.9060               -
2 :: Max             1.9060               -
2 :: Stddev          0.0000               -
9 :: Count            11266           11257
9 :: Min             0.4990          0.4940
9 :: Mean        27250.4669      24256.1919
9 :: Max      11439211.0000    6008885.0000
9 :: Stddev     226427.4624     186298.1430

This benchmark creates one thread per CPU which references an amount of anonymous memory 1.5 times the size of physical RAM. This pounds swap quite heavily and is intended to exercise THP a bit.

Mean allocation time for order-1 is reduced as before. It's also reduced for THP allocations but the variations here are pretty massive due to swap. As before, maximum allocation times are significantly reduced.

Overall, the patch reduces the mean and maximum allocation latencies for the smaller high-order allocations. This was with Slab configured so it would be expected to be more significant with Slub which uses these size allocations more aggressively.

The mean allocation times for THP allocations are also slightly reduced. The maximum latency was slightly increased as predicted by the comments due to reclaim/compaction breaking early. However, workloads care more about the latency of lower-order allocations than THP so it's an acceptable trade-off. Please consider merging for 2.6.38.

Signed-off-by Mel Gorman <[email protected]>

---
 mm/vmscan.c |   32 ++++++++++++++++++++++----------

 1 files changed, 22 insertions(+), 10 deletions(-)

mm/vmscan.c +0 -0

index 148c6e6..591b907 100644

should_continue_reclaim() for reclaim/compaction allows scanning to continue even if pages are not being reclaimed until the full list is scanned. In terms of allocation success, this makes sense but potentially it introduces unwanted latency for high-order allocations such as transparent hugepages and network jumbo frames that would prefer to fail the allocation attempt and fallback to order-0 pages. Worse, there is a potential that the full LRU scan will clear all the young bits, distort page aging information and potentially push pages into swap that would have otherwise remained resident.

This patch will stop reclaim/compaction if no pages were reclaimed in the last SWAP_CLUSTER_MAX pages that were considered.

Quoted 8 lines · On Wed, 16 Feb 2011 09:50:49 +0000 Mel Gorman <[email protected]> wrote:

should_continue_reclaim() for reclaim/compaction allows scanning to continue even if pages are not being reclaimed until the full list is scanned. In terms of allocation success, this makes sense but potentially it introduces unwanted latency for high-order allocations such as transparent hugepages and network jumbo frames that would prefer to fail the allocation attempt and fallback to order-0 pages. Worse, there is a potential that the full LRU scan will clear all the young bits, distort page aging information and potentially push pages into swap that would have otherwise remained resident.

afaict the patch affects order-0 allocations as well. What are the implications of this?

Also, what might be the downsides of this change, and did you test for them?

Quoted 2 lines

This patch will stop reclaim/compaction if no pages were reclaimed in the last SWAP_CLUSTER_MAX pages that were considered.

a) Why SWAP_CLUSTER_MAX? Is (SWAP_CLUSTER_MAX+7) better or worse?

b) The sentence doesn't seem even vaguely accurate. shrink_zone() will scan vastly more than SWAP_CLUSTER_MAX pages before calling should_continue_reclaim(). Confused.

c) The patch doesn't "stop reclaim/compaction" fully. It stops it against one zone. reclaim will then advance on to any other eligible zones.

Hi,

I think this should fix the problem of processes getting stuck in reclaim that has been reported several times.  Kent actually single-stepped through this code and noted that it was never exiting shrink_zone(), which really narrowed it down a lot, considering the tons of nested loops from the allocator down to the list shrinking.

       Hannes

---
From: Johannes Weiner <[email protected]>
Subject: vmscan: fix zone shrinking exit when scan work is done

'3e7d344 mm: vmscan: reclaim order-0 and use compaction instead of lumpy reclaim' introduced an indefinite loop in shrink_zone().

It meant to break out of this loop when no pages had been reclaimed and not a single page was even scanned.  The way it would detect the latter is by taking a snapshot of sc->nr_scanned at the beginning of the function and comparing it against the new sc->nr_scanned after the scan loop.  But it would re-iterate without updating that snapshot, looping forever if sc->nr_scanned changed at least once since shrink_zone() was invoked.

This is not the sole condition that would exit that loop, but it requires other processes to change the zone state, as the reclaimer that is stuck obviously can not anymore.

This is only happening for higher-order allocations, where reclaim is run back to back with compaction.

Reported-by Michal Hocko <[email protected]>

Reported-by Kent Overstreet <[email protected]>

Signed-off-by Johannes Weiner <[email protected]>