Re: [PATCH v4 2/4] mm,compaction: Let isolate_migratepages_{range,block} return error codes

[Vlastimil Babka]

On 3/10/21 4:08 PM, Oscar Salvador wrote:
> Currently, isolate_migratepages_{range,block} and their callers use
> a pfn == 0 vs pfn != 0 scheme to let the caller know whether there was
> any error during isolation.
> This does not work as soon as we need to start reporting different error
> codes and make sure we pass them down the chain, so they are properly
> interpreted by functions like e.g: alloc_contig_range.
> 
> Let us rework isolate_migratepages_{range,block} so we can report error
> codes.
> Since isolate_migratepages_block will stop returning the next pfn to be
> scanned, we reuse the cc->migrate_pfn field to keep track of that.
> 
> Signed-off-by: Oscar Salvador <osalvador@xxxxxxx>

Acked-by: Vlastimil Babka <vbabka@xxxxxxx>

> ---
>  mm/compaction.c | 48 ++++++++++++++++++++++++------------------------
>  mm/internal.h   |  2 +-
>  mm/page_alloc.c |  7 +++----
>  3 files changed, 28 insertions(+), 29 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index e04f4476e68e..5769753a8f60 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -787,15 +787,16 @@ static bool too_many_isolated(pg_data_t *pgdat)
>   *
>   * Isolate all pages that can be migrated from the range specified by
>   * [low_pfn, end_pfn). The range is expected to be within same pageblock.
> - * Returns zero if there is a fatal signal pending, otherwise PFN of the
> - * first page that was not scanned (which may be both less, equal to or more
> - * than end_pfn).
> + * Returns -EINTR in case we need to abort when we have too many isolated pages
> + * due to e.g: signal pending, async mode or having still pages to migrate, or 0.
> + * cc->migrate_pfn will contain the next pfn to scan (which may be both less,
> + * equal to or more that end_pfn).
>   *
>   * The pages are isolated on cc->migratepages list (not required to be empty),
>   * and cc->nr_migratepages is updated accordingly. The cc->migrate_pfn field
>   * is neither read nor updated.
>   */
> -static unsigned long
> +static int
>  isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  			unsigned long end_pfn, isolate_mode_t isolate_mode)
>  {
> @@ -810,6 +811,8 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  	unsigned long next_skip_pfn = 0;
>  	bool skip_updated = false;
>  
> +	cc->migrate_pfn = low_pfn;
> +
>  	/*
>  	 * Ensure that there are not too many pages isolated from the LRU
>  	 * list by either parallel reclaimers or compaction. If there are,
> @@ -818,16 +821,16 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  	while (unlikely(too_many_isolated(pgdat))) {
>  		/* stop isolation if there are still pages not migrated */
>  		if (cc->nr_migratepages)
> -			return 0;
> +			return -EINTR;
>  
>  		/* async migration should just abort */
>  		if (cc->mode == MIGRATE_ASYNC)
> -			return 0;
> +			return -EINTR;
>  
>  		congestion_wait(BLK_RW_ASYNC, HZ/10);
>  
>  		if (fatal_signal_pending(current))
> -			return 0;
> +			return -EINTR;
>  	}
>  
>  	cond_resched();
> @@ -1130,7 +1133,9 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  	if (nr_isolated)
>  		count_compact_events(COMPACTISOLATED, nr_isolated);
>  
> -	return low_pfn;
> +	cc->migrate_pfn = low_pfn;
> +
> +	return 0;
>  }
>  
>  /**
> @@ -1139,15 +1144,15 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>   * @start_pfn: The first PFN to start isolating.
>   * @end_pfn:   The one-past-last PFN.
>   *
> - * Returns zero if isolation fails fatally due to e.g. pending signal.
> - * Otherwise, function returns one-past-the-last PFN of isolated page
> - * (which may be greater than end_pfn if end fell in a middle of a THP page).
> + * Returns -EINTR in case isolation fails fatally due to e.g. pending signal,
> + * or 0.
>   */
> -unsigned long
> +int
>  isolate_migratepages_range(struct compact_control *cc, unsigned long start_pfn,
>  							unsigned long end_pfn)
>  {
>  	unsigned long pfn, block_start_pfn, block_end_pfn;
> +	int ret = 0;
>  
>  	/* Scan block by block. First and last block may be incomplete */
>  	pfn = start_pfn;
> @@ -1166,17 +1171,17 @@ isolate_migratepages_range(struct compact_control *cc, unsigned long start_pfn,
>  					block_end_pfn, cc->zone))
>  			continue;
>  
> -		pfn = isolate_migratepages_block(cc, pfn, block_end_pfn,
> -							ISOLATE_UNEVICTABLE);
> +		ret = isolate_migratepages_block(cc, pfn, block_end_pfn,
> +						 ISOLATE_UNEVICTABLE);
>  
> -		if (!pfn)
> +		if (ret)
>  			break;
>  
>  		if (cc->nr_migratepages >= COMPACT_CLUSTER_MAX)
>  			break;
>  	}
>  
> -	return pfn;
> +	return ret;
>  }
>  
>  #endif /* CONFIG_COMPACTION || CONFIG_CMA */
> @@ -1847,7 +1852,7 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
>  	 */
>  	for (; block_end_pfn <= cc->free_pfn;
>  			fast_find_block = false,
> -			low_pfn = block_end_pfn,
> +			cc->migrate_pfn = low_pfn = block_end_pfn,
>  			block_start_pfn = block_end_pfn,
>  			block_end_pfn += pageblock_nr_pages) {
>  
> @@ -1889,10 +1894,8 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
>  		}
>  
>  		/* Perform the isolation */
> -		low_pfn = isolate_migratepages_block(cc, low_pfn,
> -						block_end_pfn, isolate_mode);
> -
> -		if (!low_pfn)
> +		if (isolate_migratepages_block(cc, low_pfn, block_end_pfn,
> +						isolate_mode))
>  			return ISOLATE_ABORT;
>  
>  		/*
> @@ -1903,9 +1906,6 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
>  		break;
>  	}
>  
> -	/* Record where migration scanner will be restarted. */
> -	cc->migrate_pfn = low_pfn;
> -
>  	return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;
>  }
>  
> diff --git a/mm/internal.h b/mm/internal.h
> index 9902648f2206..e670abb1154c 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -261,7 +261,7 @@ struct capture_control {
>  unsigned long
>  isolate_freepages_range(struct compact_control *cc,
>  			unsigned long start_pfn, unsigned long end_pfn);
> -unsigned long
> +int
>  isolate_migratepages_range(struct compact_control *cc,
>  			   unsigned long low_pfn, unsigned long end_pfn);
>  int find_suitable_fallback(struct free_area *area, unsigned int order,
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 94467f1b85ff..2184e3ef4116 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8477,11 +8477,10 @@ static int __alloc_contig_migrate_range(struct compact_control *cc,
>  
>  		if (list_empty(&cc->migratepages)) {
>  			cc->nr_migratepages = 0;
> -			pfn = isolate_migratepages_range(cc, pfn, end);
> -			if (!pfn) {
> -				ret = -EINTR;
> +			ret = isolate_migratepages_range(cc, pfn, end);
> +			if (ret)
>  				break;
> -			}
> +			pfn = cc->migrate_pfn;
>  			tries = 0;
>  		} else if (++tries == 5) {
>  			ret = -EBUSY;
>