lkml.org 
[lkml]   [2014]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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>



\
 
 \ /
  Last update: 2014-12-10 17:01    [W:0.130 / U:0.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site