Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when stealing from pageblock

[Johannes Weiner]

On Fri, Feb 10, 2017 at 06:23:37PM +0100, Vlastimil Babka wrote:
> When stealing pages from pageblock of a different migratetype, we count how
> many free pages were stolen, and change the pageblock's migratetype if more
> than half of the pageblock was free. This might be too conservative, as there
> might be other pages that are not free, but were allocated with the same
> migratetype as our allocation requested.
> 
> While we cannot determine the migratetype of allocated pages precisely (at
> least without the page_owner functionality enabled), we can count pages that
> compaction would try to isolate for migration - those are either on LRU or
> __PageMovable(). The rest can be assumed to be MIGRATE_RECLAIMABLE or
> MIGRATE_UNMOVABLE, which we cannot easily distinguish. This counting can be
> done as part of free page stealing with little additional overhead.
> 
> The page stealing code is changed so that it considers free pages plus pages
> of the "good" migratetype for the decision whether to change pageblock's
> migratetype.
> 
> The result should be more accurate migratetype of pageblocks wrt the actual
> pages in the pageblocks, when stealing from semi-occupied pageblocks. This
> should help the efficiency of page grouping by mobility.
> 
> Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>

That makes sense to me. I have just one nit about the patch:

> @@ -1981,10 +1994,29 @@ static void steal_suitable_fallback(struct zone *zone, struct page *page,
>  		return;
>  	}
>  
> -	pages = move_freepages_block(zone, page, start_type);
> +	free_pages = move_freepages_block(zone, page, start_type,
> +						&good_pages);
> +	/*
> +	 * good_pages is now the number of movable pages, but if we
> +	 * want UNMOVABLE or RECLAIMABLE allocation, it's more tricky
> +	 */
> +	if (start_type != MIGRATE_MOVABLE) {
> +		/*
> +		 * If we are falling back to MIGRATE_MOVABLE pageblock,
> +		 * treat all non-movable pages as good. If it's UNMOVABLE
> +		 * falling back to RECLAIMABLE or vice versa, be conservative
> +		 * as we can't distinguish the exact migratetype.
> +		 */
> +		old_block_type = get_pageblock_migratetype(page);
> +		if (old_block_type == MIGRATE_MOVABLE)
> +			good_pages = pageblock_nr_pages
> +						- free_pages - good_pages;

This line had me scratch my head for a while, and I think it's mostly
because of the variable naming and the way the comments are phrased.

Could you use a variable called movable_pages to pass to and be filled
in by move_freepages_block?

And instead of good_pages something like starttype_pages or
alike_pages or st_pages or mt_pages or something, to indicate the
number of pages that are comparable to the allocation's migratetype?

> -	/* Claim the whole block if over half of it is free */
> -	if (pages >= (1 << (pageblock_order-1)) ||
> +	/* Claim the whole block if over half of it is free or good type */
> +	if (free_pages + good_pages >= (1 << (pageblock_order-1)) ||
>  			page_group_by_mobility_disabled)
>  		set_pageblock_migratetype(page, start_type);

This would then read

	if (free_pages + alike_pages ...)

which I think would be more descriptive.

The comment leading the entire section following move_freepages_block
could then say something like "If a sufficient number of pages in the
block are either free or of comparable migratability as our
allocation, claim the whole block." Followed by the caveats of how we
determine this migratibility.

Or maybe even the function. The comment above the function seems out
of date after this patch.