Messages in this thread Patch in this message | | | Date | Wed, 10 Dec 2014 16:19:13 +0100 | From | Vlastimil Babka <> | Subject | Re: [PATCH 4/4] mm/compaction: stop the isolation when we isolate enough freepage |
| |
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@gmail.com> >>> >>> 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.
>> >>> 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@gmail.com> >>> --- >>> 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@kvack.org. For more info on Linux MM, >> see: http://www.linux-mm.org/ . >> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
| |