Re: [PATCH v5 06/14] mm, compaction: reduce zone checking frequency in the migration scanner

From: Vlastimil Babka
Date: Tue Jul 29 2014 - 05:31:56 EST

On 07/29/2014 02:44 AM, David Rientjes wrote:
Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
Cc: Minchan Kim <minchan@xxxxxxxxxx>
Acked-by: Mel Gorman <mgorman@xxxxxxx>
Cc: Joonsoo Kim <>
Cc: Michal Nazarewicz <mina86@xxxxxxxxxx>
Cc: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
Cc: Christoph Lameter <cl@xxxxxxxxx>
Cc: Rik van Riel <riel@xxxxxxxxxx>
Cc: David Rientjes <rientjes@xxxxxxxxxx>

Acked-by: David Rientjes <rientjes@xxxxxxxxxx>

Minor comments below.


+ * Check that the whole (or subset of) a pageblock given by the interval of
+ * [start_pfn, end_pfn) is valid and within the same zone, before scanning it
+ * with the migration of free compaction scanner. The scanners then need to
+ * use only pfn_valid_within() check for arches that allow holes within
+ * pageblocks.
+ *
+ * Return struct page pointer of start_pfn, or NULL if checks were not passed.
+ *
+ * It's possible on some configurations to have a setup like node0 node1 node0
+ * i.e. it's possible that all pages within a zones range of pages do not
+ * belong to a single zone. We assume that a border between node0 and node1
+ * can occur within a single pageblock, but not a node0 node1 node0
+ * interleaving within a single pageblock. It is therefore sufficient to check
+ * the first and last page of a pageblock and avoid checking each individual
+ * page in a pageblock.
+ */
+static struct page *pageblock_within_zone(unsigned long start_pfn,
+ unsigned long end_pfn, struct zone *zone)

The name of this function is quite strange, it's returning a pointer to
the actual start page but the name implies it would be a boolean.

Yeah but I couldn't think of a better name that wouldn't be long and ugly :(

+ struct page *start_page;
+ struct page *end_page;
+ /* end_pfn is one past the range we are checking */
+ end_pfn--;

With the given implementation, yes, but I'm not sure if that should be
assumed for any class of callers. It seems better to call with
end_pfn - 1.

Well, I think the rest of compaction functions assume one-past-end parameters so this would make it an exception. Better hide the exception in the implementation and not expose it to callers?

+ if (!pfn_valid(start_pfn) || !pfn_valid(end_pfn))
+ return NULL;

Ok, so even with this check, we still need to check pfn_valid_within() for
all pfns between start_pfn and end_pfn if there are memory holes. I
checked that both the migration and freeing scanners do that before
reading your comment above the function, looks good.

Yeah, and thankfully pfn_valid_within() is a no-op on many archs :)

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at