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

[Vlastimil Babka]

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 <iamjoonsoo.kim@xxxxxxx>
> > 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.

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 :)


--
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/