Re: [patch 2/2] mm, compaction: persistently skip hugetlbfs pageblocks

[Vlastimil Babka <vbabka@suse.cz>]

On 08/16/2017 01:39 AM, David Rientjes wrote:
> It is pointless to migrate hugetlb memory as part of memory compaction if
> the hugetlb size is equal to the pageblock order.  No defragmentation is
> occurring in this condition.
> 
> It is also pointless to for the freeing scanner to scan a pageblock where
> a hugetlb page is pinned.  Unconditionally skip these pageblocks, and do
> so peristently so that they are not rescanned until it is observed that
> these hugepages are no longer pinned.
> 
> It would also be possible to do this by involving the hugetlb subsystem
> in marking pageblocks to no longer be skipped when they hugetlb pages are
> freed.  This is a simple solution that doesn't involve any additional
> subsystems in pageblock skip manipulation.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  mm/compaction.c | 48 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -217,6 +217,20 @@ static void reset_cached_positions(struct zone *zone)
>  				pageblock_start_pfn(zone_end_pfn(zone) - 1);
>  }
>  
> +/*
> + * Hugetlbfs pages should consistenly be skipped until updated by the hugetlb
> + * subsystem.  It is always pointless to compact pages of pageblock_order and
> + * the free scanner can reconsider when no longer huge.
> + */
> +static bool pageblock_skip_persistent(struct page *page, unsigned int order)
> +{
> +	if (!PageHuge(page))
> +		return false;
> +	if (order != pageblock_order)
> +		return false;
> +	return true;

Why just HugeTLBfs? There's also no point in migrating/finding free
pages in THPs. Actually, any compound page of pageblock order?

> +}
> +
>  /*
>   * This function is called to clear all cached information on pageblocks that
>   * should be skipped for page isolation when the migrate and free page scanner
> @@ -241,6 +255,8 @@ static void __reset_isolation_suitable(struct zone *zone)
>  			continue;
>  		if (zone != page_zone(page))
>  			continue;
> +		if (pageblock_skip_persistent(page, compound_order(page)))
> +			continue;

I like the idea of how persistency is achieved by rechecking in the reset.

>  
>  		clear_pageblock_skip(page);
>  	}
> @@ -448,13 +464,15 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>  		 * and the only danger is skipping too much.
>  		 */
>  		if (PageCompound(page)) {
> -			unsigned int comp_order = compound_order(page);
> -
> -			if (likely(comp_order < MAX_ORDER)) {
> -				blockpfn += (1UL << comp_order) - 1;
> -				cursor += (1UL << comp_order) - 1;
> +			const unsigned int order = compound_order(page);
> +
> +			if (pageblock_skip_persistent(page, order)) {
> +				set_pageblock_skip(page);
> +				blockpfn = end_pfn;
> +			} else if (likely(order < MAX_ORDER)) {
> +				blockpfn += (1UL << order) - 1;
> +				cursor += (1UL << order) - 1;
>  			}

Is this new code (and below) really necessary? The existing code should
already lead to skip bit being set via update_pageblock_skip()?

Thanks,
Vlastimil

> -
>  			goto isolate_fail;
>  		}
>  
> @@ -771,11 +789,13 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  		 * danger is skipping too much.
>  		 */
>  		if (PageCompound(page)) {
> -			unsigned int comp_order = compound_order(page);
> -
> -			if (likely(comp_order < MAX_ORDER))
> -				low_pfn += (1UL << comp_order) - 1;
> +			const unsigned int order = compound_order(page);
>  
> +			if (pageblock_skip_persistent(page, order)) {
> +				set_pageblock_skip(page);
> +				low_pfn = end_pfn;
> +			} else if (likely(order < MAX_ORDER))
> +				low_pfn += (1UL << order) - 1;
>  			goto isolate_fail;
>  		}
>  
> @@ -837,7 +857,13 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  			 * is safe to read and it's 0 for tail pages.
>  			 */
>  			if (unlikely(PageCompound(page))) {
> -				low_pfn += (1UL << compound_order(page)) - 1;
> +				const unsigned int order = compound_order(page);
> +
> +				if (pageblock_skip_persistent(page, order)) {
> +					set_pageblock_skip(page);
> +					low_pfn = end_pfn;
> +				} else
> +					low_pfn += (1UL << order) - 1;
>  				goto isolate_fail;
>  			}
>  		}
>