On 11/04/2014 01:37 AM, Joonsoo Kim wrote:
On Mon, Nov 03, 2014 at 09:12:33AM +0100, Vlastimil Babka wrote:
On 10/27/2014 08:41 AM, Joonsoo Kim wrote:
On Tue, Oct 07, 2014 at 05:33:39PM +0200, Vlastimil Babka wrote:
And, I wonder why last_migrated_pfn is set after isolate_migratepages().
Not sure I understand your question. With the mistake above, it
cannot currently be set at the point isolate_migratepages() is
called, so you might question the goto check_drain in the
ISOLATE_NONE case, if that's what you are wondering about.
When I correct that, it might be set when COMPACT_CLUSTER_MAX pages
are isolated and migrated the middle of a pageblock, and then the
rest of the pageblock contains no pages that could be isolated, so
the last isolate_migratepages() attempt in the pageblock returns
with ISOLATE_NONE. Still there were some migrations that produced
free pages that should be drained at that point.
To clarify my question, I attach psuedo code that I thought correct.
Sorry for the late reply.
static int compact_zone()
{
unsigned long last_migrated_pfn = 0;
...
compaction_suitable();
...
while (compact_finished()) {
if (!last_migrated_pfn)
last_migrated_pfn = cc->migrate_pfn - 1;
isolate_migratepages();
switch case
migrate_pages();
...
check_drain: (at the end of loop)
do flush and reset last_migrated_pfn if needed
}
}
We should record last_migrated_pfn before isolate_migratepages() and
then compare it with cc->migrate_pfn after isolate_migratepages() to
know if we moved away from the previous cc->order aligned block.
Am I missing something?
What about this scenario, with pageblock order:
- record cc->migrate_pfn pointing to pageblock X
- isolate_migratepages() skips the pageblock due to e.g. skip bit, or
the pageblock being a THP already...
- loop to pageblock X+1, last_migrated_pfn is still set to pfn of
pageblock X (more precisely the pfn is (X << pageblock_order) - 1 per
your code, but doesn't matter)
- isolate_migratepages isolates something, but ends up somewhere in the
middle of pageblock due to COMPACT_CLUSTER_MAX
- cc->migrate_pfn points to pageblock X+1 (plus some pages it scanned)
- so it will decide that it has fully migrated pageblock X and it's time
to drain. But the drain is most likely useless - we didn't migrate
anything in pageblock X, we skipped it. And in X+1 we didn't migrate
everything yet, so we should drain only after finishing the other part
of the pageblock.
In short, "last_migrated_pfn" is not "last position of migrate scanner"
but "last block where we *actually* migrated".
I think if you would try to fix the scenario above, you would end up
with something like my patch :)
Vlastimil
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
