On 02/14/2017 07:10 PM, Johannes Weiner wrote:
>
> 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.
I'll incorporate this for the next posting, thanks for the feedback!
