Re: [PATCH 02/11] mm: compaction: introduceisolate_{free,migrate}pages_range().
From: Michal Nazarewicz
Date: Tue Jan 10 2012 - 10:04:14 EST
On Tue, 10 Jan 2012 14:43:51 +0100, Mel Gorman <mel@xxxxxxxxx> wrote:
On Thu, Dec 29, 2011 at 01:39:03PM +0100, Marek Szyprowski wrote:
From: Michal Nazarewicz <mina86@xxxxxxxxxx>
+static unsigned long
+isolate_freepages_range(struct zone *zone,
+ unsigned long start_pfn, unsigned long end_pfn,
+ struct list_head *freelist)
{
- unsigned long zone_end_pfn, end_pfn;
- int nr_scanned = 0, total_isolated = 0;
- struct page *cursor;
-
- /* Get the last PFN we should scan for free pages at */
- zone_end_pfn = zone->zone_start_pfn + zone->spanned_pages;
- end_pfn = min(blockpfn + pageblock_nr_pages, zone_end_pfn);
+ unsigned long nr_scanned = 0, total_isolated = 0;
+ unsigned long pfn = start_pfn;
+ struct page *page;
- /* Find the first usable PFN in the block to initialse page cursor */
- for (; blockpfn < end_pfn; blockpfn++) {
- if (pfn_valid_within(blockpfn))
- break;
- }
- cursor = pfn_to_page(blockpfn);
+ VM_BUG_ON(!pfn_valid(pfn));
+ page = pfn_to_page(pfn);
The existing code is able to find the first usable PFN in a pageblock
with pfn_valid_within(). It's allowed to do that because it knows
the pageblock is valid so calling pfn_valid() is unnecessary.
It is curious to change this to something that can sometimes BUG_ON
!pfn_valid(pfn) instead of having a PFN walker that knows how to
handle pfn_valid().
Actually, this walker seem redundant since one is already present in
isolate_freepages(), ie. if !pfn_valid(pfn) then the loop in
isolate_freepages() will âcontinue;â and the function will never get
called.
+cleanup:
+ /*
+ * Undo what we have done so far, and return. We know all pages from
+ * [start_pfn, pfn) are free because we have just freed them. If one of
+ * the page in the range was not freed, we would end up here earlier.
+ */
+ for (; start_pfn < pfn; ++start_pfn)
+ __free_page(pfn_to_page(start_pfn));
+ return 0;
This returns 0 even though you could have isolated some pages.
The loop's intention is to âunisolateâ (ie. free) anything that got
isolated, so at the end number of isolated pages in in fact zero.
Overall, there would have been less contortion if you
implemented isolate_freepages_range() in terms of the existing
isolate_freepages_block. Something that looked kinda like this not
compiled untested illustration.
static unsigned long isolate_freepages_range(struct zone *zone,
unsigned long start_pfn, unsigned long end_pfn,
struct list_head *list)
{
unsigned long pfn = start_pfn;
unsigned long isolated;
for (pfn = start_pfn; pfn < end_pfn; pfn += pageblock_nr_pages) {
if (!pfn_valid(pfn))
continue;
isolated += isolate_freepages_block(zone, pfn, list);
}
return isolated;
}
I neglected to update isolate_freepages_block() to take the end_pfn
meaning it will in fact isolate too much but that would be easy to
address.
This is not a problem since my isolate_freepages_range() implementation
can go beyond end_pfn, anyway.
Of course, the function taking end_pfn is an optimisation since it does
not have to compute zone_end each time.
It would be up to yourself whether to shuffle the tracepoint
around because with this example it will be triggered once per
pageblock. You'd still need the cleanup code and freelist check in
isolate_freepages_block() of course.
The point is that it would minimise the disruption to the existing
code and easier to get details like pfn_valid() right without odd
contortions, more gotos than should be necessary and trying to keep
pfn and page straight.
Even though I'd personally go with my approach, I see merit in your point,
so will change.
}
/* Returns true if the page is within a block suitable for migration to */
@@ -365,17 +403,49 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
nr_isolated++;
/* Avoid isolating too much */
- if (cc->nr_migratepages == COMPACT_CLUSTER_MAX)
+ if (cc->nr_migratepages == COMPACT_CLUSTER_MAX) {
+ ++low_pfn;
break;
+ }
}
This minor optimisation is unrelated to the implementation of
isolate_migratepages_range() and should be in its own patch.
It's actually not a mere minor optimisation, but rather making the function work
according to the documentation added, ie. that it returns âPFN of the first page
that was not scannedâ.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, MichaÅ âmina86â Nazarewicz (o o)
ooo +----<email/xmpp: mpn@xxxxxxxxxx>--------------ooO--(_)--Ooo--
--
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/