Re: [patch v2 3/4] mm, compaction: add per-zone migration pfn cache for async compaction

Vlastimil Babka
2014-05-06T11:52:09Z

On 05/06/2014 02:29 AM, David Rientjes wrote:
> On Mon, 5 May 2014, Vlastimil Babka wrote:
>
>> I see, although I would still welcome some numbers to back such change.
>
> It's pretty difficult to capture numbers for this in real-world scenarios
> since it happens rarely (and when it happens, it's very significant
> latency) and without an instrumented kernel that will determine how many
> pageblocks have been skipped. I could create a synthetic example of it in
> the kernel and get numbers for a worst-case scenario with a 64GB zone if
> you'd like, I'm not sure how representative it will be.
>
>> What I still don't like is the removal of the intent of commit 50b5b094e6. You
>> now again call set_pageblock_skip() unconditionally, thus also on pageblocks
>> that async compaction skipped due to being non-MOVABLE. The sync compaction
>> will thus ignore them.
>>
>
> I'm not following you, with this patch there are two cached pfns for the
> migration scanner: one is used for sync and one is used for async. When
> cc->sync == true, both cached pfns are updated (async is not going to
> succeed for a pageblock when sync failed for that pageblock); when
> cc->sync == false, the async cached pfn is updated only and we pick up
> again where we left off for subsequent async compactions. Sync compaction
> will still begin where it last left off and consider these non-MOVABLE
> pageblocks.
>

Yeah I understand, the cached pfn's are not the problem. The problem is
that with your patch, set_pageblock_skip() will be called through
update_pageblock_skip() in async compaction, since you removed the
skipped_async_unsuitable variable. So in sync compaction, such pageblock
will be skipped thanks to the isolation_suitable() check which uses
get_pageblock_skip() to read the bit set by the async compaction.