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@xxxxxxxxx>
> ---
> 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
