Re: [PATCH 4/4] mm/compaction: stop the isolation when we isolate enough freepage

[Joonsoo Kim]

On Wed, Dec 10, 2014 at 04:19:13PM +0100, Vlastimil Babka wrote:
> On 12/10/2014 08:00 AM, Joonsoo Kim wrote:
> >On Mon, Dec 08, 2014 at 10:59:17AM +0100, Vlastimil Babka wrote:
> >>On 12/08/2014 08:16 AM, Joonsoo Kim wrote:
> >>>From: Joonsoo Kim <js1304@xxxxxxxxx>
> >>>
> >>>Currently, freepage isolation in one pageblock doesn't consider how many
> >>>freepages we isolate. When I traced flow of compaction, compaction
> >>>sometimes isolates more than 256 freepages to migrate just 32 pages.
> >>>
> >>>In this patch, freepage isolation is stopped at the point that we
> >>>have more isolated freepage than isolated page for migration. This
> >>>results in slowing down free page scanner and make compaction success
> >>>rate higher.
> >>>
> >>>stress-highalloc test in mmtests with non movable order 7 allocation shows
> >>>increase of compaction success rate and slight improvement of allocation
> >>>success rate.
> >>>
> >>>Allocation success rate on phase 1 (%)
> >>>62.70 : 64.00
> >>>
> >>>Compaction success rate (Compaction success * 100 / Compaction stalls, %)
> >>>35.13 : 41.50
> >>
> >>This is weird. I could maybe understand that isolating too many
> >
> >In fact, I also didn't fully understand why it results in this
> >result. :)
> >
> >>freepages and then returning them is a waste of time if compaction
> >>terminates immediately after the following migration (otherwise we
> >>would keep those free pages for the future migrations within same
> >>compaction run). And wasting time could reduce success rates for
> >>async compaction terminating prematurely due to cond_resched(), but
> >>that should be all the difference, unless there's another subtle
> >>bug, no?
> >
> >My guess is that there is bad effect when we release isolated
> >freepages. In asynchronous compaction, this happens quite easily.
> >In this case, freepages are returned to page allocator and, maybe,
> >they are on pcp list or front of buddy list so they would be used by
> >another user at first. This reduces freepages we can utilize so
> >compaction is finished earlier.
> 
> Hmm, some might even stay on the pcplists and we won't isolate them
> again. So we will leave them behind. I wouldn't expect such big
> difference here, but anyway...
> It might be interesting to evaluate if a pcplists drain after
> returning isolated freepages (unless the scanners have already met,
> that's pointless) would make any difference.

Yes, I will check it.

> 
> >>
> >>>pfn where both scanners meets on compaction complete
> >>>(separate test due to enormous tracepoint buffer)
> >>>(zone_start=4096, zone_end=1048576)
> >>>586034 : 654378
> >>
> >>The difference here suggests that there is indeed another subtle bug
> >>related to where free scanner restarts, and we must be leaving the
> >>excessively isolated (and then returned) freepages behind. Otherwise
> >>I think the scanners should meet at the same place regardless of
> >>your patch.
> >
> >I tried to find another subtle bug, but, can't find any critical one.
> >Hmm...
> >
> >Anyway, regardless of the reason of result, this patch seems reasonable,
> >because we don't need to waste time to isolate unneeded freepages.
> 
> Right.
> 
> >Thanks.
> >
> >>
> >>>Signed-off-by: Joonsoo Kim <js1304@xxxxxxxxx>
> >>>---
> >>>  mm/compaction.c |   17 ++++++++++-------
> >>>  1 file changed, 10 insertions(+), 7 deletions(-)
> >>>
> >>>diff --git a/mm/compaction.c b/mm/compaction.c
> >>>index 2fd5f79..12223b9 100644
> >>>--- a/mm/compaction.c
> >>>+++ b/mm/compaction.c
> >>>@@ -422,6 +422,13 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
> >>>
> >>>  		/* If a page was split, advance to the end of it */
> >>>  		if (isolated) {
> >>>+			cc->nr_freepages += isolated;
> >>>+			if (!strict &&
> >>>+				cc->nr_migratepages <= cc->nr_freepages) {
> >>>+				blockpfn += isolated;
> >>>+				break;
> >>>+			}
> >>>+
> >>>  			blockpfn += isolated - 1;
> >>>  			cursor += isolated - 1;
> >>>  			continue;
> >>>@@ -831,7 +838,6 @@ static void isolate_freepages(struct compact_control *cc)
> >>>  	unsigned long isolate_start_pfn; /* exact pfn we start at */
> >>>  	unsigned long block_end_pfn;	/* end of current pageblock */
> >>>  	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
> >>>-	int nr_freepages = cc->nr_freepages;
> >>>  	struct list_head *freelist = &cc->freepages;
> >>>
> >>>  	/*
> >>>@@ -856,11 +862,11 @@ static void isolate_freepages(struct compact_control *cc)
> >>>  	 * pages on cc->migratepages. We stop searching if the migrate
> >>>  	 * and free page scanners meet or enough free pages are isolated.
> >>>  	 */
> >>>-	for (; block_start_pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
> >>>+	for (; block_start_pfn >= low_pfn &&
> >>>+			cc->nr_migratepages > cc->nr_freepages;
> >>>  				block_end_pfn = block_start_pfn,
> >>>  				block_start_pfn -= pageblock_nr_pages,
> >>>  				isolate_start_pfn = block_start_pfn) {
> >>>-		unsigned long isolated;
> >>>
> >>>  		/*
> >>>  		 * This can iterate a massively long zone without finding any
> >>>@@ -885,9 +891,8 @@ static void isolate_freepages(struct compact_control *cc)
> >>>  			continue;
> >>>
> >>>  		/* Found a block suitable for isolating free pages from. */
> >>>-		isolated = isolate_freepages_block(cc, &isolate_start_pfn,
> >>>+		isolate_freepages_block(cc, &isolate_start_pfn,
> >>>  					block_end_pfn, freelist, false);
> >>>-		nr_freepages += isolated;
> >>>
> >>>  		/*
> >>>  		 * Remember where the free scanner should restart next time,
> >>>@@ -919,8 +924,6 @@ static void isolate_freepages(struct compact_control *cc)
> >>>  	 */
> >>>  	if (block_start_pfn < low_pfn)
> >>>  		cc->free_pfn = cc->migrate_pfn;
> >>>-
> >>>-	cc->nr_freepages = nr_freepages;
> >>>  }
> >>>
> >>>  /*
> >>>
> >>
> >>--
> >>To unsubscribe, send a message with 'unsubscribe linux-mm' in
> >>the body to majordomo@xxxxxxxxxx  For more info on Linux MM,
> >>see: http://www.linux-mm.org/ .
> >>Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxxx  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/