Re: [PATCH] mm, compaction: fix 'limit' in fast_isolate_freepages

[Mel Gorman <mgorman@techsingularity.net>]

On Sun, Jun 20, 2021 at 11:57:42PM +0900, Wonhyuk Yang wrote:
> Because of 'min(1, ...)', fast_isolate_freepages set 'limit'
> to 0 or 1. This takes away the opportunities of find candinate
> pages. Also, even if 'limit' reaches zero, it scan once. It is
> not consistent. So, modify the minimum value of 'limit' to 1.
> 

The changelog could do with a little polish.

In addition, what were the effects of this and what load did you use to
evaluate it? While your patch is mostly correct, it has the potential
side-effect of increasing system CPU usage in some cases and I'm curious
to hear what you observed. Minimally it is worth noting in the changelog
that there is a risk of increasing system CPU usage but that there are
advantages too. Describe them in the changelog in case a regression
bisects to your patch.

> Fixes: 5a811889de10f ("mm, compaction: use free lists to quickly locate a migration target")
> 
> Signed-off-by: Wonhyuk Yang <vvghjk1234@gmail.com>
> ---
>  mm/compaction.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 84fde270ae74..2e41e7ab1f55 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1380,7 +1380,7 @@ static int next_search_order(struct compact_control *cc, int order)
>  static unsigned long
>  fast_isolate_freepages(struct compact_control *cc)
>  {
> -	unsigned int limit = min(1U, freelist_scan_limit(cc) >> 1);
> +	unsigned int limit = max(1U, freelist_scan_limit(cc) >> 1);
>  	unsigned int nr_scanned = 0;
>  	unsigned long low_pfn, min_pfn, highest = 0;
>  	unsigned long nr_isolated = 0;

Ok.

> @@ -1456,7 +1456,7 @@ fast_isolate_freepages(struct compact_control *cc)
>  				high_pfn = pfn;
>  
>  				/* Shorten the scan if a candidate is found */
> -				limit >>= 1;
> +				limit = max(1U, limit >> 1);
>  			}
>  
>  			if (order_scanned >= limit)

This hunk should be dropped. Once a candidate free page has been
identified, it's ok to decay the limit to 0. This hunk introduces a risk
of increasing system CPU usage unnecessarily.

> @@ -1496,7 +1496,7 @@ fast_isolate_freepages(struct compact_control *cc)
>  		 * to freelist_scan_limit.
>  		 */
>  		if (order_scanned >= limit)
> -			limit = min(1U, limit >> 1);
> +			limit = max(1U, limit >> 1);
>  	}

The change is fine but I have a minor nitpick that you are free to
ignore. The comment above this block has a typo.

s/scan ig related/scan is related/

Ordinarily patches to fix spelling are ignored but you are altering this
area anyway and it's helpful to see the full comment when reviewing this
patch. I think it would be harmless to fix the spelling in the context
of this patch.

Thanks.

-- 
Mel Gorman
SUSE Labs