Messages in this thread | | | Date | Tue, 29 Jul 2014 11:31:51 +0200 | From | Vlastimil Babka <> | Subject | Re: [PATCH v5 06/14] mm, compaction: reduce zone checking frequency in the migration scanner |
| |
On 07/29/2014 02:44 AM, David Rientjes wrote: >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> >> Cc: Minchan Kim <minchan@kernel.org> >> Acked-by: Mel Gorman <mgorman@suse.de> >> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> >> Cc: Michal Nazarewicz <mina86@mina86.com> >> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> >> Cc: Christoph Lameter <cl@linux.com> >> Cc: Rik van Riel <riel@redhat.com> >> Cc: David Rientjes <rientjes@google.com> > > Acked-by: David Rientjes <rientjes@google.com> > > Minor comments below.
Thanks,
>> +/* >> + * 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 :)
| |