Re: [PATCH] mm, compaction: fix 'limit' in fast_isolate_freepages
From: Mel Gorman <hidden>
Date: 2021-06-23 09:15:15
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.
quoted hunk ↗ jump to hunk
Fixes: 5a811889de10f ("mm, compaction: use free lists to quickly locate a migration target") Signed-off-by: Wonhyuk Yang <redacted> --- 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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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