Re: [RFC PATCH 2/3] mm, compaction: use MIN_COMPACT_COSTLY_PRIORITY everywhere for costly orders

[Vlastimil Babka <vbabka@suse.cz>]

On 7/24/19 7:50 PM, Mike Kravetz wrote:
> For PAGE_ALLOC_COSTLY_ORDER allocations, MIN_COMPACT_COSTLY_PRIORITY is
> minimum (highest priority).  Other places in the compaction code key off
> of MIN_COMPACT_PRIORITY.  Costly order allocations will never get to
> MIN_COMPACT_PRIORITY.  Therefore, some conditions will never be met for
> costly order allocations.
> 
> This was observed when hugetlb allocations could stall for minutes or
> hours when should_compact_retry() would return true more often then it
> should.  Specifically, this was in the case where compact_result was
> COMPACT_DEFERRED and COMPACT_PARTIAL_SKIPPED and no progress was being
> made.

Hmm, the point of MIN_COMPACT_COSTLY_PRIORITY was that costly
allocations will not reach the priority where compaction becomes too
expensive. With your patch, they still don't reach that priority value,
but are allowed to be thorough anyway, even sooner. That just seems like
a wrong way to fix the problem. If should_compact_retry() returns
misleading results for costly allocations, then that should be fixed
instead?

Alternatively, you might want to say that hugetlb allocations are not
like other random costly allocations, because the admin setting
nr_hugepages is prepared to take the cost (I thought that was indicated
by the __GFP_RETRY_MAYFAIL flag, but seeing all the other users of it,
I'm not sure anymore). In that case should_compact_retry() could take
__GFP_RETRY_MAYFAIL into account and allow MIN_COMPACT_PRIORITY even for
costly allocations.

> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>  mm/compaction.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 952dc2fb24e5..325b746068d1 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -2294,9 +2294,15 @@ static enum compact_result compact_zone_order(struct zone *zone, int order,
>  		.alloc_flags = alloc_flags,
>  		.classzone_idx = classzone_idx,
>  		.direct_compaction = true,
> -		.whole_zone = (prio == MIN_COMPACT_PRIORITY),
> -		.ignore_skip_hint = (prio == MIN_COMPACT_PRIORITY),
> -		.ignore_block_suitable = (prio == MIN_COMPACT_PRIORITY)
> +		.whole_zone = ((order > PAGE_ALLOC_COSTLY_ORDER) ?
> +				(prio == MIN_COMPACT_COSTLY_PRIORITY) :
> +				(prio == MIN_COMPACT_PRIORITY)),
> +		.ignore_skip_hint = ((order > PAGE_ALLOC_COSTLY_ORDER) ?
> +				(prio == MIN_COMPACT_COSTLY_PRIORITY) :
> +				(prio == MIN_COMPACT_PRIORITY)),
> +		.ignore_block_suitable = ((order > PAGE_ALLOC_COSTLY_ORDER) ?
> +				(prio == MIN_COMPACT_COSTLY_PRIORITY) :
> +				(prio == MIN_COMPACT_PRIORITY))
>  	};
>  	struct capture_control capc = {
>  		.cc = &cc,
> @@ -2338,6 +2344,7 @@ enum compact_result try_to_compact_pages(gfp_t gfp_mask, unsigned int order,
>  	int may_perform_io = gfp_mask & __GFP_IO;
>  	struct zoneref *z;
>  	struct zone *zone;
> +	int min_priority;
>  	enum compact_result rc = COMPACT_SKIPPED;
>  
>  	/*
> @@ -2350,12 +2357,13 @@ enum compact_result try_to_compact_pages(gfp_t gfp_mask, unsigned int order,
>  	trace_mm_compaction_try_to_compact_pages(order, gfp_mask, prio);
>  
>  	/* Compact each zone in the list */
> +	min_priority = (order > PAGE_ALLOC_COSTLY_ORDER) ?
> +			MIN_COMPACT_COSTLY_PRIORITY : MIN_COMPACT_PRIORITY;
>  	for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
>  								ac->nodemask) {
>  		enum compact_result status;
>  
> -		if (prio > MIN_COMPACT_PRIORITY
> -					&& compaction_deferred(zone, order)) {
> +		if (prio > min_priority && compaction_deferred(zone, order)) {
>  			rc = max_t(enum compact_result, COMPACT_DEFERRED, rc);
>  			continue;
>  		}
>