[PATCH 0/5] mm: compaction: cleanups & simplifications

[PATCH 0/5] mm: compaction: cleanups & simplifications

[Johannes Weiner]

These compaction cleanups are split out from the huge page allocator
series[1], as requested by reviewer feedback.

Against current mm-stable.

[1] https://lore.kernel.org/linux-mm/20230418191313.268131-1-hannes@cmpxchg.org/

 include/linux/compaction.h     | 96 ++----------------------------------------------------------------------------------------------
 include/trace/events/mmflags.h |  4 ++--
 mm/compaction.c                | 62 ++++++++++++++++++++++++++++++++++----------------------------
 mm/page_alloc.c                | 57 ++++++++++++++++++++-------------------------------------
 mm/vmscan.c                    | 27 ++++++++++++++-------------
 5 files changed, 72 insertions(+), 174 deletions(-)

[PATCH 1/5] mm: compaction: remove compaction result helpers

[Johannes Weiner]

The compaction result helpers encode quirks that are specific to the
allocator's retry logic. E.g. COMPACT_SUCCESS and COMPACT_COMPLETE
actually represent failures that should be retried upon, and so on. I
frequently found myself pulling up the helper implementation in order
to understand and work on the retry logic. They're not quite clean
abstractions; rather they split the retry logic into two locations.

Remove the helpers and inline the checks. Then comment on the result
interpretations directly where the decision making happens.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 include/linux/compaction.h     | 92 ----------------------------------
 include/trace/events/mmflags.h |  4 +-
 mm/page_alloc.c                | 30 ++++++-----
 3 files changed, 19 insertions(+), 107 deletions(-)

diff --git a/include/linux/compaction.h b/include/linux/compaction.h
index a6e512cfb670..1f0328a2ba48 100644
--- a/include/linux/compaction.h
+++ b/include/linux/compaction.h
@@ -95,78 +95,6 @@ extern enum compact_result compaction_suitable(struct zone *zone, int order,
 extern void compaction_defer_reset(struct zone *zone, int order,
 				bool alloc_success);
 
-/* Compaction has made some progress and retrying makes sense */
-static inline bool compaction_made_progress(enum compact_result result)
-{
-	/*
-	 * Even though this might sound confusing this in fact tells us
-	 * that the compaction successfully isolated and migrated some
-	 * pageblocks.
-	 */
-	if (result == COMPACT_SUCCESS)
-		return true;
-
-	return false;
-}
-
-/* Compaction has failed and it doesn't make much sense to keep retrying. */
-static inline bool compaction_failed(enum compact_result result)
-{
-	/* All zones were scanned completely and still not result. */
-	if (result == COMPACT_COMPLETE)
-		return true;
-
-	return false;
-}
-
-/* Compaction needs reclaim to be performed first, so it can continue. */
-static inline bool compaction_needs_reclaim(enum compact_result result)
-{
-	/*
-	 * Compaction backed off due to watermark checks for order-0
-	 * so the regular reclaim has to try harder and reclaim something.
-	 */
-	if (result == COMPACT_SKIPPED)
-		return true;
-
-	return false;
-}
-
-/*
- * Compaction has backed off for some reason after doing some work or none
- * at all. It might be throttling or lock contention. Retrying might be still
- * worthwhile, but with a higher priority if allowed.
- */
-static inline bool compaction_withdrawn(enum compact_result result)
-{
-	/*
-	 * If compaction is deferred for high-order allocations, it is
-	 * because sync compaction recently failed. If this is the case
-	 * and the caller requested a THP allocation, we do not want
-	 * to heavily disrupt the system, so we fail the allocation
-	 * instead of entering direct reclaim.
-	 */
-	if (result == COMPACT_DEFERRED)
-		return true;
-
-	/*
-	 * If compaction in async mode encounters contention or blocks higher
-	 * priority task we back off early rather than cause stalls.
-	 */
-	if (result == COMPACT_CONTENDED)
-		return true;
-
-	/*
-	 * Page scanners have met but we haven't scanned full zones so this
-	 * is a back off in fact.
-	 */
-	if (result == COMPACT_PARTIAL_SKIPPED)
-		return true;
-
-	return false;
-}
-
-
 bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
 					int alloc_flags);
 
@@ -185,26 +113,6 @@ static inline enum compact_result compaction_suitable(struct zone *zone, int ord
 	return COMPACT_SKIPPED;
 }
 
-static inline bool compaction_made_progress(enum compact_result result)
-{
-	return false;
-}
-
-static inline bool compaction_failed(enum compact_result result)
-{
-	return false;
-}
-
-static inline bool compaction_needs_reclaim(enum compact_result result)
-{
-	return false;
-}
-
-static inline bool compaction_withdrawn(enum compact_result result)
-{
-	return true;
-}
-
 static inline void kcompactd_run(int nid)
 {
 }
diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index b63e7c0fbbe5..1478b9dd05fa 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -223,8 +223,8 @@ IF_HAVE_VM_SOFTDIRTY(VM_SOFTDIRTY,	"softdirty"	)		\
 #define compact_result_to_feedback(result)	\
 ({						\
 	enum compact_result __result = result;	\
-	(compaction_failed(__result)) ? COMPACTION_FAILED : \
-		(compaction_withdrawn(__result)) ? COMPACTION_WITHDRAWN : COMPACTION_PROGRESS; \
+	(__result == COMPACT_COMPLETE) ? COMPACTION_FAILED : \
+		(__result == COMPACT_SUCCESS) ? COMPACTION_PROGRESS : COMPACTION_WITHDRAWN; \
 })
 
 #define COMPACTION_FEEDBACK		\
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 47421bedc12b..5a84a0bebc37 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3768,35 +3768,39 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 	if (fatal_signal_pending(current))
 		return false;
 
-	if (compaction_made_progress(compact_result))
+	/*
+	 * Compaction managed to coalesce some page blocks, but the
+	 * allocation failed presumably due to a race. Retry some.
+	 */
+	if (compact_result == COMPACT_SUCCESS)
 		(*compaction_retries)++;
 
 	/*
-	 * compaction considers all the zone as desperately out of memory
-	 * so it doesn't really make much sense to retry except when the
+	 * All zones were scanned completely and still no result. It
+	 * doesn't really make much sense to retry except when the
 	 * failure could be caused by insufficient priority
 	 */
-	if (compaction_failed(compact_result))
+	if (compact_result == COMPACT_COMPLETE)
 		goto check_priority;
 
 	/*
-	 * compaction was skipped because there are not enough order-0 pages
-	 * to work with, so we retry only if it looks like reclaim can help.
+	 * Compaction was skipped due to a lack of free order-0
+	 * migration targets. Continue if reclaim can help.
 	 */
-	if (compaction_needs_reclaim(compact_result)) {
+	if (compact_result == COMPACT_SKIPPED) {
 		ret = compaction_zonelist_suitable(ac, order, alloc_flags);
 		goto out;
 	}
 
 	/*
-	 * make sure the compaction wasn't deferred or didn't bail out early
-	 * due to locks contention before we declare that we should give up.
-	 * But the next retry should use a higher priority if allowed, so
-	 * we don't just keep bailing out endlessly.
+	 * If compaction backed due to being deferred, due to
+	 * contended locks in async mode, or due to scanners meeting
+	 * after a partial scan, retry with increased priority.
 	 */
-	if (compaction_withdrawn(compact_result)) {
+	if (compact_result == COMPACT_DEFERRED ||
+	    compact_result == COMPACT_CONTENDED ||
+	    compact_result == COMPACT_PARTIAL_SKIPPED)
 		goto check_priority;
-	}
 
 	/*
 	 * !costly requests are much more important than __GFP_RETRY_MAYFAIL
-- 
2.40.0

Re: [PATCH 1/5] mm: compaction: remove compaction result helpers

[Vlastimil Babka]

On 5/19/23 14:39, Johannes Weiner wrote:
> The compaction result helpers encode quirks that are specific to the
> allocator's retry logic. E.g. COMPACT_SUCCESS and COMPACT_COMPLETE
> actually represent failures that should be retried upon, and so on. I
> frequently found myself pulling up the helper implementation in order
> to understand and work on the retry logic. They're not quite clean
> abstractions; rather they split the retry logic into two locations.
> 
> Remove the helpers and inline the checks. Then comment on the result
> interpretations directly where the decision making happens.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Since the usage of helpers never proliferated outside of
should_compact_retry() with the exception of tracepoint, I guess it makes
sense to remove them.

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  include/linux/compaction.h     | 92 ----------------------------------
>  include/trace/events/mmflags.h |  4 +-
>  mm/page_alloc.c                | 30 ++++++-----
>  3 files changed, 19 insertions(+), 107 deletions(-)
> 
> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> index a6e512cfb670..1f0328a2ba48 100644
> --- a/include/linux/compaction.h
> +++ b/include/linux/compaction.h
> @@ -95,78 +95,6 @@ extern enum compact_result compaction_suitable(struct zone *zone, int order,
>  extern void compaction_defer_reset(struct zone *zone, int order,
>  				bool alloc_success);
>  
> -/* Compaction has made some progress and retrying makes sense */
> -static inline bool compaction_made_progress(enum compact_result result)
> -{
> -	/*
> -	 * Even though this might sound confusing this in fact tells us
> -	 * that the compaction successfully isolated and migrated some
> -	 * pageblocks.
> -	 */
> -	if (result == COMPACT_SUCCESS)
> -		return true;
> -
> -	return false;
> -}
> -
> -/* Compaction has failed and it doesn't make much sense to keep retrying. */
> -static inline bool compaction_failed(enum compact_result result)
> -{
> -	/* All zones were scanned completely and still not result. */
> -	if (result == COMPACT_COMPLETE)
> -		return true;
> -
> -	return false;
> -}
> -
> -/* Compaction needs reclaim to be performed first, so it can continue. */
> -static inline bool compaction_needs_reclaim(enum compact_result result)
> -{
> -	/*
> -	 * Compaction backed off due to watermark checks for order-0
> -	 * so the regular reclaim has to try harder and reclaim something.
> -	 */
> -	if (result == COMPACT_SKIPPED)
> -		return true;
> -
> -	return false;
> -}
> -
> -/*
> - * Compaction has backed off for some reason after doing some work or none
> - * at all. It might be throttling or lock contention. Retrying might be still
> - * worthwhile, but with a higher priority if allowed.
> - */
> -static inline bool compaction_withdrawn(enum compact_result result)
> -{
> -	/*
> -	 * If compaction is deferred for high-order allocations, it is
> -	 * because sync compaction recently failed. If this is the case
> -	 * and the caller requested a THP allocation, we do not want
> -	 * to heavily disrupt the system, so we fail the allocation
> -	 * instead of entering direct reclaim.
> -	 */
> -	if (result == COMPACT_DEFERRED)
> -		return true;
> -
> -	/*
> -	 * If compaction in async mode encounters contention or blocks higher
> -	 * priority task we back off early rather than cause stalls.
> -	 */
> -	if (result == COMPACT_CONTENDED)
> -		return true;
> -
> -	/*
> -	 * Page scanners have met but we haven't scanned full zones so this
> -	 * is a back off in fact.
> -	 */
> -	if (result == COMPACT_PARTIAL_SKIPPED)
> -		return true;
> -
> -	return false;
> -}
> -
> -
>  bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
>  					int alloc_flags);
>  
> @@ -185,26 +113,6 @@ static inline enum compact_result compaction_suitable(struct zone *zone, int ord
>  	return COMPACT_SKIPPED;
>  }
>  
> -static inline bool compaction_made_progress(enum compact_result result)
> -{
> -	return false;
> -}
> -
> -static inline bool compaction_failed(enum compact_result result)
> -{
> -	return false;
> -}
> -
> -static inline bool compaction_needs_reclaim(enum compact_result result)
> -{
> -	return false;
> -}
> -
> -static inline bool compaction_withdrawn(enum compact_result result)
> -{
> -	return true;
> -}
> -
>  static inline void kcompactd_run(int nid)
>  {
>  }
> diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
> index b63e7c0fbbe5..1478b9dd05fa 100644
> --- a/include/trace/events/mmflags.h
> +++ b/include/trace/events/mmflags.h
> @@ -223,8 +223,8 @@ IF_HAVE_VM_SOFTDIRTY(VM_SOFTDIRTY,	"softdirty"	)		\
>  #define compact_result_to_feedback(result)	\
>  ({						\
>  	enum compact_result __result = result;	\
> -	(compaction_failed(__result)) ? COMPACTION_FAILED : \
> -		(compaction_withdrawn(__result)) ? COMPACTION_WITHDRAWN : COMPACTION_PROGRESS; \
> +	(__result == COMPACT_COMPLETE) ? COMPACTION_FAILED : \
> +		(__result == COMPACT_SUCCESS) ? COMPACTION_PROGRESS : COMPACTION_WITHDRAWN; \
>  })
>  
>  #define COMPACTION_FEEDBACK		\
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 47421bedc12b..5a84a0bebc37 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3768,35 +3768,39 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>  	if (fatal_signal_pending(current))
>  		return false;
>  
> -	if (compaction_made_progress(compact_result))
> +	/*
> +	 * Compaction managed to coalesce some page blocks, but the
> +	 * allocation failed presumably due to a race. Retry some.
> +	 */
> +	if (compact_result == COMPACT_SUCCESS)
>  		(*compaction_retries)++;
>  
>  	/*
> -	 * compaction considers all the zone as desperately out of memory
> -	 * so it doesn't really make much sense to retry except when the
> +	 * All zones were scanned completely and still no result. It
> +	 * doesn't really make much sense to retry except when the
>  	 * failure could be caused by insufficient priority
>  	 */
> -	if (compaction_failed(compact_result))
> +	if (compact_result == COMPACT_COMPLETE)
>  		goto check_priority;
>  
>  	/*
> -	 * compaction was skipped because there are not enough order-0 pages
> -	 * to work with, so we retry only if it looks like reclaim can help.
> +	 * Compaction was skipped due to a lack of free order-0
> +	 * migration targets. Continue if reclaim can help.
>  	 */
> -	if (compaction_needs_reclaim(compact_result)) {
> +	if (compact_result == COMPACT_SKIPPED) {
>  		ret = compaction_zonelist_suitable(ac, order, alloc_flags);
>  		goto out;
>  	}
>  
>  	/*
> -	 * make sure the compaction wasn't deferred or didn't bail out early
> -	 * due to locks contention before we declare that we should give up.
> -	 * But the next retry should use a higher priority if allowed, so
> -	 * we don't just keep bailing out endlessly.
> +	 * If compaction backed due to being deferred, due to
> +	 * contended locks in async mode, or due to scanners meeting
> +	 * after a partial scan, retry with increased priority.
>  	 */
> -	if (compaction_withdrawn(compact_result)) {
> +	if (compact_result == COMPACT_DEFERRED ||
> +	    compact_result == COMPACT_CONTENDED ||
> +	    compact_result == COMPACT_PARTIAL_SKIPPED)
>  		goto check_priority;
> -	}
>  
>  	/*
>  	 * !costly requests are much more important than __GFP_RETRY_MAYFAIL

[PATCH 2/5] mm: compaction: simplify should_compact_retry()

[Johannes Weiner]

The different branches for retry are unnecessarily complicated. There
are really only three outcomes: progress (retry n times), skipped
(retry if reclaim can help), failed (retry with higher priority).

Rearrange the branches and the retry counter to make it simpler.

v2:
- fix trace point build (Mel)
- fix max_retries logic for costly allocs (Huang)

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/page_alloc.c | 53 +++++++++++++++----------------------------------
 1 file changed, 16 insertions(+), 37 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 5a84a0bebc37..72660e924b95 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3772,16 +3772,22 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 	 * Compaction managed to coalesce some page blocks, but the
 	 * allocation failed presumably due to a race. Retry some.
 	 */
-	if (compact_result == COMPACT_SUCCESS)
-		(*compaction_retries)++;
+	if (compact_result == COMPACT_SUCCESS) {
+		/*
+		 * !costly requests are much more important than
+		 * __GFP_RETRY_MAYFAIL costly ones because they are de
+		 * facto nofail and invoke OOM killer to move on while
+		 * costly can fail and users are ready to cope with
+		 * that. 1/4 retries is rather arbitrary but we would
+		 * need much more detailed feedback from compaction to
+		 * make a better decision.
+		 */
+		if (order > PAGE_ALLOC_COSTLY_ORDER)
+			max_retries /= 4;
 
-	/*
-	 * All zones were scanned completely and still no result. It
-	 * doesn't really make much sense to retry except when the
-	 * failure could be caused by insufficient priority
-	 */
-	if (compact_result == COMPACT_COMPLETE)
-		goto check_priority;
+		ret = ++(*compaction_retries) <= max_retries;
+		goto out;
+	}
 
 	/*
 	 * Compaction was skipped due to a lack of free order-0
@@ -3793,35 +3799,8 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 	}
 
 	/*
-	 * If compaction backed due to being deferred, due to
-	 * contended locks in async mode, or due to scanners meeting
-	 * after a partial scan, retry with increased priority.
-	 */
-	if (compact_result == COMPACT_DEFERRED ||
-	    compact_result == COMPACT_CONTENDED ||
-	    compact_result == COMPACT_PARTIAL_SKIPPED)
-		goto check_priority;
-
-	/*
-	 * !costly requests are much more important than __GFP_RETRY_MAYFAIL
-	 * costly ones because they are de facto nofail and invoke OOM
-	 * killer to move on while costly can fail and users are ready
-	 * to cope with that. 1/4 retries is rather arbitrary but we
-	 * would need much more detailed feedback from compaction to
-	 * make a better decision.
-	 */
-	if (order > PAGE_ALLOC_COSTLY_ORDER)
-		max_retries /= 4;
-	if (*compaction_retries <= max_retries) {
-		ret = true;
-		goto out;
-	}
-
-	/*
-	 * Make sure there are attempts at the highest priority if we exhausted
-	 * all retries or failed at the lower priorities.
+	 * Compaction failed. Retry with increasing priority.
 	 */
-check_priority:
 	min_priority = (order > PAGE_ALLOC_COSTLY_ORDER) ?
 			MIN_COMPACT_COSTLY_PRIORITY : MIN_COMPACT_PRIORITY;
 
-- 
2.40.0

Re: [PATCH 2/5] mm: compaction: simplify should_compact_retry()

[Vlastimil Babka]

On 5/19/23 14:39, Johannes Weiner wrote:
> The different branches for retry are unnecessarily complicated. There
> are really only three outcomes: progress (retry n times), skipped
> (retry if reclaim can help), failed (retry with higher priority).
> 
> Rearrange the branches and the retry counter to make it simpler.
> 
> v2:
> - fix trace point build (Mel)
> - fix max_retries logic for costly allocs (Huang)
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  mm/page_alloc.c | 53 +++++++++++++++----------------------------------
>  1 file changed, 16 insertions(+), 37 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 5a84a0bebc37..72660e924b95 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3772,16 +3772,22 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>  	 * Compaction managed to coalesce some page blocks, but the
>  	 * allocation failed presumably due to a race. Retry some.
>  	 */
> -	if (compact_result == COMPACT_SUCCESS)
> -		(*compaction_retries)++;
> +	if (compact_result == COMPACT_SUCCESS) {
> +		/*
> +		 * !costly requests are much more important than
> +		 * __GFP_RETRY_MAYFAIL costly ones because they are de
> +		 * facto nofail and invoke OOM killer to move on while
> +		 * costly can fail and users are ready to cope with
> +		 * that. 1/4 retries is rather arbitrary but we would
> +		 * need much more detailed feedback from compaction to
> +		 * make a better decision.
> +		 */
> +		if (order > PAGE_ALLOC_COSTLY_ORDER)
> +			max_retries /= 4;
>  
> -	/*
> -	 * All zones were scanned completely and still no result. It
> -	 * doesn't really make much sense to retry except when the
> -	 * failure could be caused by insufficient priority
> -	 */
> -	if (compact_result == COMPACT_COMPLETE)
> -		goto check_priority;
> +		ret = ++(*compaction_retries) <= max_retries;
> +		goto out;

I think you simplified this part too much, so now once it runs out of
retries, it will return false, while previously it would increase the priority.

> +	}
>  
>  	/*
>  	 * Compaction was skipped due to a lack of free order-0
> @@ -3793,35 +3799,8 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>  	}
>  
>  	/*
> -	 * If compaction backed due to being deferred, due to
> -	 * contended locks in async mode, or due to scanners meeting
> -	 * after a partial scan, retry with increased priority.
> -	 */
> -	if (compact_result == COMPACT_DEFERRED ||
> -	    compact_result == COMPACT_CONTENDED ||
> -	    compact_result == COMPACT_PARTIAL_SKIPPED)
> -		goto check_priority;
> -
> -	/*
> -	 * !costly requests are much more important than __GFP_RETRY_MAYFAIL
> -	 * costly ones because they are de facto nofail and invoke OOM
> -	 * killer to move on while costly can fail and users are ready
> -	 * to cope with that. 1/4 retries is rather arbitrary but we
> -	 * would need much more detailed feedback from compaction to
> -	 * make a better decision.
> -	 */
> -	if (order > PAGE_ALLOC_COSTLY_ORDER)
> -		max_retries /= 4;
> -	if (*compaction_retries <= max_retries) {
> -		ret = true;
> -		goto out;
> -	}
> -
> -	/*
> -	 * Make sure there are attempts at the highest priority if we exhausted
> -	 * all retries or failed at the lower priorities.
> +	 * Compaction failed. Retry with increasing priority.
>  	 */
> -check_priority:
>  	min_priority = (order > PAGE_ALLOC_COSTLY_ORDER) ?
>  			MIN_COMPACT_COSTLY_PRIORITY : MIN_COMPACT_PRIORITY;
>  

Re: [PATCH 2/5] mm: compaction: simplify should_compact_retry()

[Johannes Weiner]

On Mon, May 29, 2023 at 03:03:52PM +0200, Vlastimil Babka wrote:
> On 5/19/23 14:39, Johannes Weiner wrote:
> > The different branches for retry are unnecessarily complicated. There
> > are really only three outcomes: progress (retry n times), skipped
> > (retry if reclaim can help), failed (retry with higher priority).
> > 
> > Rearrange the branches and the retry counter to make it simpler.
> > 
> > v2:
> > - fix trace point build (Mel)
> > - fix max_retries logic for costly allocs (Huang)
> > 
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> > ---
> >  mm/page_alloc.c | 53 +++++++++++++++----------------------------------
> >  1 file changed, 16 insertions(+), 37 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 5a84a0bebc37..72660e924b95 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -3772,16 +3772,22 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
> >  	 * Compaction managed to coalesce some page blocks, but the
> >  	 * allocation failed presumably due to a race. Retry some.
> >  	 */
> > -	if (compact_result == COMPACT_SUCCESS)
> > -		(*compaction_retries)++;
> > +	if (compact_result == COMPACT_SUCCESS) {
> > +		/*
> > +		 * !costly requests are much more important than
> > +		 * __GFP_RETRY_MAYFAIL costly ones because they are de
> > +		 * facto nofail and invoke OOM killer to move on while
> > +		 * costly can fail and users are ready to cope with
> > +		 * that. 1/4 retries is rather arbitrary but we would
> > +		 * need much more detailed feedback from compaction to
> > +		 * make a better decision.
> > +		 */
> > +		if (order > PAGE_ALLOC_COSTLY_ORDER)
> > +			max_retries /= 4;
> >  
> > -	/*
> > -	 * All zones were scanned completely and still no result. It
> > -	 * doesn't really make much sense to retry except when the
> > -	 * failure could be caused by insufficient priority
> > -	 */
> > -	if (compact_result == COMPACT_COMPLETE)
> > -		goto check_priority;
> > +		ret = ++(*compaction_retries) <= max_retries;
> > +		goto out;
> 
> I think you simplified this part too much, so now once it runs out of
> retries, it will return false, while previously it would increase the priority.

Oops, I'll send a delta fix to Andrew tomorrow. Thanks!

[PATCH 3/5] mm: compaction: refactor __compaction_suitable()

[Johannes Weiner]

__compaction_suitable() is supposed to check for available migration
targets. However, it also checks whether the operation was requested
via /proc/sys/vm/compact_memory, and whether the original allocation
request can already succeed. These don't apply to all callsites.

Move the checks out to the callers, so that later patches can deal
with them one by one. No functional change intended.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 include/linux/compaction.h |  4 +-
 mm/compaction.c            | 78 ++++++++++++++++++++++++--------------
 mm/vmscan.c                | 35 ++++++++++-------
 3 files changed, 73 insertions(+), 44 deletions(-)

diff --git a/include/linux/compaction.h b/include/linux/compaction.h
index 1f0328a2ba48..9f7cf3e1bf89 100644
--- a/include/linux/compaction.h
+++ b/include/linux/compaction.h
@@ -90,7 +90,7 @@ extern enum compact_result try_to_compact_pages(gfp_t gfp_mask,
 		struct page **page);
 extern void reset_isolation_suitable(pg_data_t *pgdat);
 extern enum compact_result compaction_suitable(struct zone *zone, int order,
-		unsigned int alloc_flags, int highest_zoneidx);
+					       int highest_zoneidx);
 
 extern void compaction_defer_reset(struct zone *zone, int order,
 				bool alloc_success);
@@ -108,7 +108,7 @@ static inline void reset_isolation_suitable(pg_data_t *pgdat)
 }
 
 static inline enum compact_result compaction_suitable(struct zone *zone, int order,
-					int alloc_flags, int highest_zoneidx)
+						      int highest_zoneidx)
 {
 	return COMPACT_SKIPPED;
 }
diff --git a/mm/compaction.c b/mm/compaction.c
index c9a4b6dffcf2..8f61cfa87c49 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2206,24 +2206,10 @@ static enum compact_result compact_finished(struct compact_control *cc)
 }
 
 static enum compact_result __compaction_suitable(struct zone *zone, int order,
-					unsigned int alloc_flags,
 					int highest_zoneidx,
 					unsigned long wmark_target)
 {
 	unsigned long watermark;
-
-	if (is_via_compact_memory(order))
-		return COMPACT_CONTINUE;
-
-	watermark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK);
-	/*
-	 * If watermarks for high-order allocation are already met, there
-	 * should be no need for compaction at all.
-	 */
-	if (zone_watermark_ok(zone, order, watermark, highest_zoneidx,
-								alloc_flags))
-		return COMPACT_SUCCESS;
-
 	/*
 	 * Watermarks for order-0 must be met for compaction to be able to
 	 * isolate free pages for migration targets. This means that the
@@ -2256,13 +2242,12 @@ static enum compact_result __compaction_suitable(struct zone *zone, int order,
  *   COMPACT_CONTINUE - If compaction should run now
  */
 enum compact_result compaction_suitable(struct zone *zone, int order,
-					unsigned int alloc_flags,
 					int highest_zoneidx)
 {
 	enum compact_result ret;
 	int fragindex;
 
-	ret = __compaction_suitable(zone, order, alloc_flags, highest_zoneidx,
+	ret = __compaction_suitable(zone, order, highest_zoneidx,
 				    zone_page_state(zone, NR_FREE_PAGES));
 	/*
 	 * fragmentation index determines if allocation failures are due to
@@ -2306,7 +2291,16 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
 	for_each_zone_zonelist_nodemask(zone, z, ac->zonelist,
 				ac->highest_zoneidx, ac->nodemask) {
 		unsigned long available;
-		enum compact_result compact_result;
+		unsigned long watermark;
+
+		if (is_via_compact_memory(order))
+			return true;
+
+		/* Allocation can already succeed, nothing to do */
+		watermark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK);
+		if (zone_watermark_ok(zone, order, watermark,
+				      ac->highest_zoneidx, alloc_flags))
+			continue;
 
 		/*
 		 * Do not consider all the reclaimable memory because we do not
@@ -2316,9 +2310,8 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
 		 */
 		available = zone_reclaimable_pages(zone) / order;
 		available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
-		compact_result = __compaction_suitable(zone, order, alloc_flags,
-				ac->highest_zoneidx, available);
-		if (compact_result == COMPACT_CONTINUE)
+		if (__compaction_suitable(zone, order, ac->highest_zoneidx,
+					  available) == COMPACT_CONTINUE)
 			return true;
 	}
 
@@ -2348,11 +2341,23 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
 	INIT_LIST_HEAD(&cc->migratepages);
 
 	cc->migratetype = gfp_migratetype(cc->gfp_mask);
-	ret = compaction_suitable(cc->zone, cc->order, cc->alloc_flags,
-							cc->highest_zoneidx);
-	/* Compaction is likely to fail */
-	if (ret == COMPACT_SUCCESS || ret == COMPACT_SKIPPED)
-		return ret;
+
+	if (!is_via_compact_memory(cc->order)) {
+		unsigned long watermark;
+
+		/* Allocation can already succeed, nothing to do */
+		watermark = wmark_pages(cc->zone,
+					cc->alloc_flags & ALLOC_WMARK_MASK);
+		if (zone_watermark_ok(cc->zone, cc->order, watermark,
+				      cc->highest_zoneidx, cc->alloc_flags))
+			return COMPACT_SUCCESS;
+
+		ret = compaction_suitable(cc->zone, cc->order,
+					  cc->highest_zoneidx);
+		/* Compaction is likely to fail */
+		if (ret == COMPACT_SKIPPED)
+			return ret;
+	}
 
 	/*
 	 * Clear pageblock skip if there were failures recently and compaction
@@ -2845,7 +2850,16 @@ static bool kcompactd_node_suitable(pg_data_t *pgdat)
 		if (!populated_zone(zone))
 			continue;
 
-		if (compaction_suitable(zone, pgdat->kcompactd_max_order, 0,
+		if (is_via_compact_memory(pgdat->kcompactd_max_order))
+			return true;
+
+		/* Allocation can already succeed, check other zones */
+		if (zone_watermark_ok(zone, pgdat->kcompactd_max_order,
+				      min_wmark_pages(zone),
+				      highest_zoneidx, 0))
+			continue;
+
+		if (compaction_suitable(zone, pgdat->kcompactd_max_order,
 					highest_zoneidx) == COMPACT_CONTINUE)
 			return true;
 	}
@@ -2883,10 +2897,18 @@ static void kcompactd_do_work(pg_data_t *pgdat)
 		if (compaction_deferred(zone, cc.order))
 			continue;
 
-		if (compaction_suitable(zone, cc.order, 0, zoneid) !=
-							COMPACT_CONTINUE)
+		if (is_via_compact_memory(cc.order))
+			goto compact;
+
+		/* Allocation can already succeed, nothing to do */
+		if (zone_watermark_ok(zone, cc.order,
+				      min_wmark_pages(zone), zoneid, 0))
 			continue;
 
+		if (compaction_suitable(zone, cc.order,
+					zoneid) != COMPACT_CONTINUE)
+			continue;
+compact:
 		if (kthread_should_stop())
 			return;
 
diff --git a/mm/vmscan.c b/mm/vmscan.c
index d257916f39e5..c9c0f3e081f5 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -6397,14 +6397,17 @@ static inline bool should_continue_reclaim(struct pglist_data *pgdat,
 		if (!managed_zone(zone))
 			continue;
 
-		switch (compaction_suitable(zone, sc->order, 0, sc->reclaim_idx)) {
-		case COMPACT_SUCCESS:
-		case COMPACT_CONTINUE:
+		if (sc->order == -1) /* is_via_compact_memory() */
+			return false;
+
+		/* Allocation can already succeed, nothing to do */
+		if (zone_watermark_ok(zone, sc->order, min_wmark_pages(zone),
+				      sc->reclaim_idx, 0))
+			return false;
+
+		if (compaction_suitable(zone, sc->order,
+					sc->reclaim_idx) == COMPACT_CONTINUE)
 			return false;
-		default:
-			/* check next zone */
-			;
-		}
 	}
 
 	/*
@@ -6592,16 +6595,20 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc)
 static inline bool compaction_ready(struct zone *zone, struct scan_control *sc)
 {
 	unsigned long watermark;
-	enum compact_result suitable;
 
-	suitable = compaction_suitable(zone, sc->order, 0, sc->reclaim_idx);
-	if (suitable == COMPACT_SUCCESS)
-		/* Allocation should succeed already. Don't reclaim. */
+	if (sc->order == -1) /* is_via_compact_memory() */
+		goto suitable;
+
+	/* Allocation can already succeed, nothing to do */
+	if (zone_watermark_ok(zone, sc->order, min_wmark_pages(zone),
+			      sc->reclaim_idx, 0))
 		return true;
-	if (suitable == COMPACT_SKIPPED)
-		/* Compaction cannot yet proceed. Do reclaim. */
-		return false;
 
+	/* Compaction cannot yet proceed. Do reclaim. */
+	if (compaction_suitable(zone, sc->order,
+				sc->reclaim_idx) == COMPACT_SKIPPED)
+		return false;
+suitable:
 	/*
 	 * Compaction is already possible, but it takes time to run and there
 	 * are potentially other callers using the pages just freed. So proceed
-- 
2.40.0

Re: [PATCH 3/5] mm: compaction: refactor __compaction_suitable()

[Vlastimil Babka]

On 5/19/23 14:39, Johannes Weiner wrote:
> __compaction_suitable() is supposed to check for available migration
> targets. However, it also checks whether the operation was requested
> via /proc/sys/vm/compact_memory, and whether the original allocation
> request can already succeed. These don't apply to all callsites.
> 
> Move the checks out to the callers, so that later patches can deal
> with them one by one. No functional change intended.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

Note comment on compaction_suitable() still mentions COMPACT_SUCCESS, that
is no longer possible, so delete that line?
Also on closer look, both compaction_suitable() and __compaction_suitable()
could now simply return bool. The callers use it that way anyway. There
would just be some extra fiddling internally aroud the tracepoint.

> ---
>  include/linux/compaction.h |  4 +-
>  mm/compaction.c            | 78 ++++++++++++++++++++++++--------------
>  mm/vmscan.c                | 35 ++++++++++-------
>  3 files changed, 73 insertions(+), 44 deletions(-)
> 
> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> index 1f0328a2ba48..9f7cf3e1bf89 100644
> --- a/include/linux/compaction.h
> +++ b/include/linux/compaction.h
> @@ -90,7 +90,7 @@ extern enum compact_result try_to_compact_pages(gfp_t gfp_mask,
>  		struct page **page);
>  extern void reset_isolation_suitable(pg_data_t *pgdat);
>  extern enum compact_result compaction_suitable(struct zone *zone, int order,
> -		unsigned int alloc_flags, int highest_zoneidx);
> +					       int highest_zoneidx);
>  
>  extern void compaction_defer_reset(struct zone *zone, int order,
>  				bool alloc_success);
> @@ -108,7 +108,7 @@ static inline void reset_isolation_suitable(pg_data_t *pgdat)
>  }
>  
>  static inline enum compact_result compaction_suitable(struct zone *zone, int order,
> -					int alloc_flags, int highest_zoneidx)
> +						      int highest_zoneidx)
>  {
>  	return COMPACT_SKIPPED;
>  }
> diff --git a/mm/compaction.c b/mm/compaction.c
> index c9a4b6dffcf2..8f61cfa87c49 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -2206,24 +2206,10 @@ static enum compact_result compact_finished(struct compact_control *cc)
>  }
>  
>  static enum compact_result __compaction_suitable(struct zone *zone, int order,
> -					unsigned int alloc_flags,
>  					int highest_zoneidx,
>  					unsigned long wmark_target)
>  {
>  	unsigned long watermark;
> -
> -	if (is_via_compact_memory(order))
> -		return COMPACT_CONTINUE;
> -
> -	watermark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK);
> -	/*
> -	 * If watermarks for high-order allocation are already met, there
> -	 * should be no need for compaction at all.
> -	 */
> -	if (zone_watermark_ok(zone, order, watermark, highest_zoneidx,
> -								alloc_flags))
> -		return COMPACT_SUCCESS;
> -
>  	/*
>  	 * Watermarks for order-0 must be met for compaction to be able to
>  	 * isolate free pages for migration targets. This means that the
> @@ -2256,13 +2242,12 @@ static enum compact_result __compaction_suitable(struct zone *zone, int order,
>   *   COMPACT_CONTINUE - If compaction should run now
>   */
>  enum compact_result compaction_suitable(struct zone *zone, int order,
> -					unsigned int alloc_flags,
>  					int highest_zoneidx)
>  {
>  	enum compact_result ret;
>  	int fragindex;
>  
> -	ret = __compaction_suitable(zone, order, alloc_flags, highest_zoneidx,
> +	ret = __compaction_suitable(zone, order, highest_zoneidx,
>  				    zone_page_state(zone, NR_FREE_PAGES));
>  	/*
>  	 * fragmentation index determines if allocation failures are due to
> @@ -2306,7 +2291,16 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
>  	for_each_zone_zonelist_nodemask(zone, z, ac->zonelist,
>  				ac->highest_zoneidx, ac->nodemask) {
>  		unsigned long available;
> -		enum compact_result compact_result;
> +		unsigned long watermark;
> +
> +		if (is_via_compact_memory(order))
> +			return true;
> +
> +		/* Allocation can already succeed, nothing to do */
> +		watermark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK);
> +		if (zone_watermark_ok(zone, order, watermark,
> +				      ac->highest_zoneidx, alloc_flags))
> +			continue;
>  
>  		/*
>  		 * Do not consider all the reclaimable memory because we do not
> @@ -2316,9 +2310,8 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
>  		 */
>  		available = zone_reclaimable_pages(zone) / order;
>  		available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
> -		compact_result = __compaction_suitable(zone, order, alloc_flags,
> -				ac->highest_zoneidx, available);
> -		if (compact_result == COMPACT_CONTINUE)
> +		if (__compaction_suitable(zone, order, ac->highest_zoneidx,
> +					  available) == COMPACT_CONTINUE)
>  			return true;
>  	}
>  
> @@ -2348,11 +2341,23 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
>  	INIT_LIST_HEAD(&cc->migratepages);
>  
>  	cc->migratetype = gfp_migratetype(cc->gfp_mask);
> -	ret = compaction_suitable(cc->zone, cc->order, cc->alloc_flags,
> -							cc->highest_zoneidx);
> -	/* Compaction is likely to fail */
> -	if (ret == COMPACT_SUCCESS || ret == COMPACT_SKIPPED)
> -		return ret;
> +
> +	if (!is_via_compact_memory(cc->order)) {
> +		unsigned long watermark;
> +
> +		/* Allocation can already succeed, nothing to do */
> +		watermark = wmark_pages(cc->zone,
> +					cc->alloc_flags & ALLOC_WMARK_MASK);
> +		if (zone_watermark_ok(cc->zone, cc->order, watermark,
> +				      cc->highest_zoneidx, cc->alloc_flags))
> +			return COMPACT_SUCCESS;
> +
> +		ret = compaction_suitable(cc->zone, cc->order,
> +					  cc->highest_zoneidx);
> +		/* Compaction is likely to fail */
> +		if (ret == COMPACT_SKIPPED)
> +			return ret;
> +	}
>  
>  	/*
>  	 * Clear pageblock skip if there were failures recently and compaction
> @@ -2845,7 +2850,16 @@ static bool kcompactd_node_suitable(pg_data_t *pgdat)
>  		if (!populated_zone(zone))
>  			continue;
>  
> -		if (compaction_suitable(zone, pgdat->kcompactd_max_order, 0,
> +		if (is_via_compact_memory(pgdat->kcompactd_max_order))
> +			return true;
> +
> +		/* Allocation can already succeed, check other zones */
> +		if (zone_watermark_ok(zone, pgdat->kcompactd_max_order,
> +				      min_wmark_pages(zone),
> +				      highest_zoneidx, 0))
> +			continue;
> +
> +		if (compaction_suitable(zone, pgdat->kcompactd_max_order,
>  					highest_zoneidx) == COMPACT_CONTINUE)
>  			return true;
>  	}
> @@ -2883,10 +2897,18 @@ static void kcompactd_do_work(pg_data_t *pgdat)
>  		if (compaction_deferred(zone, cc.order))
>  			continue;
>  
> -		if (compaction_suitable(zone, cc.order, 0, zoneid) !=
> -							COMPACT_CONTINUE)
> +		if (is_via_compact_memory(cc.order))
> +			goto compact;
> +
> +		/* Allocation can already succeed, nothing to do */
> +		if (zone_watermark_ok(zone, cc.order,
> +				      min_wmark_pages(zone), zoneid, 0))
>  			continue;
>  
> +		if (compaction_suitable(zone, cc.order,
> +					zoneid) != COMPACT_CONTINUE)
> +			continue;
> +compact:
>  		if (kthread_should_stop())
>  			return;
>  
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index d257916f39e5..c9c0f3e081f5 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -6397,14 +6397,17 @@ static inline bool should_continue_reclaim(struct pglist_data *pgdat,
>  		if (!managed_zone(zone))
>  			continue;
>  
> -		switch (compaction_suitable(zone, sc->order, 0, sc->reclaim_idx)) {
> -		case COMPACT_SUCCESS:
> -		case COMPACT_CONTINUE:
> +		if (sc->order == -1) /* is_via_compact_memory() */
> +			return false;
> +
> +		/* Allocation can already succeed, nothing to do */
> +		if (zone_watermark_ok(zone, sc->order, min_wmark_pages(zone),
> +				      sc->reclaim_idx, 0))
> +			return false;
> +
> +		if (compaction_suitable(zone, sc->order,
> +					sc->reclaim_idx) == COMPACT_CONTINUE)
>  			return false;
> -		default:
> -			/* check next zone */
> -			;
> -		}
>  	}
>  
>  	/*
> @@ -6592,16 +6595,20 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc)
>  static inline bool compaction_ready(struct zone *zone, struct scan_control *sc)
>  {
>  	unsigned long watermark;
> -	enum compact_result suitable;
>  
> -	suitable = compaction_suitable(zone, sc->order, 0, sc->reclaim_idx);
> -	if (suitable == COMPACT_SUCCESS)
> -		/* Allocation should succeed already. Don't reclaim. */
> +	if (sc->order == -1) /* is_via_compact_memory() */
> +		goto suitable;
> +
> +	/* Allocation can already succeed, nothing to do */
> +	if (zone_watermark_ok(zone, sc->order, min_wmark_pages(zone),
> +			      sc->reclaim_idx, 0))
>  		return true;
> -	if (suitable == COMPACT_SKIPPED)
> -		/* Compaction cannot yet proceed. Do reclaim. */
> -		return false;
>  
> +	/* Compaction cannot yet proceed. Do reclaim. */
> +	if (compaction_suitable(zone, sc->order,
> +				sc->reclaim_idx) == COMPACT_SKIPPED)
> +		return false;
> +suitable:
>  	/*
>  	 * Compaction is already possible, but it takes time to run and there
>  	 * are potentially other callers using the pages just freed. So proceed

[PATCH 4/5] mm: compaction: remove unnecessary is_via_compact_memory() checks

[Johannes Weiner]

Remove from all paths not reachable via /proc/sys/vm/compact_memory.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/compaction.c | 11 +----------
 mm/vmscan.c     |  8 +-------
 2 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 8f61cfa87c49..83ecbc829c62 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2293,9 +2293,6 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
 		unsigned long available;
 		unsigned long watermark;
 
-		if (is_via_compact_memory(order))
-			return true;
-
 		/* Allocation can already succeed, nothing to do */
 		watermark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK);
 		if (zone_watermark_ok(zone, order, watermark,
@@ -2850,9 +2847,6 @@ static bool kcompactd_node_suitable(pg_data_t *pgdat)
 		if (!populated_zone(zone))
 			continue;
 
-		if (is_via_compact_memory(pgdat->kcompactd_max_order))
-			return true;
-
 		/* Allocation can already succeed, check other zones */
 		if (zone_watermark_ok(zone, pgdat->kcompactd_max_order,
 				      min_wmark_pages(zone),
@@ -2897,9 +2891,6 @@ static void kcompactd_do_work(pg_data_t *pgdat)
 		if (compaction_deferred(zone, cc.order))
 			continue;
 
-		if (is_via_compact_memory(cc.order))
-			goto compact;
-
 		/* Allocation can already succeed, nothing to do */
 		if (zone_watermark_ok(zone, cc.order,
 				      min_wmark_pages(zone), zoneid, 0))
@@ -2908,7 +2899,7 @@ static void kcompactd_do_work(pg_data_t *pgdat)
 		if (compaction_suitable(zone, cc.order,
 					zoneid) != COMPACT_CONTINUE)
 			continue;
-compact:
+
 		if (kthread_should_stop())
 			return;
 
diff --git a/mm/vmscan.c b/mm/vmscan.c
index c9c0f3e081f5..c0cfa9b86b48 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -6397,9 +6397,6 @@ static inline bool should_continue_reclaim(struct pglist_data *pgdat,
 		if (!managed_zone(zone))
 			continue;
 
-		if (sc->order == -1) /* is_via_compact_memory() */
-			return false;
-
 		/* Allocation can already succeed, nothing to do */
 		if (zone_watermark_ok(zone, sc->order, min_wmark_pages(zone),
 				      sc->reclaim_idx, 0))
@@ -6596,9 +6593,6 @@ static inline bool compaction_ready(struct zone *zone, struct scan_control *sc)
 {
 	unsigned long watermark;
 
-	if (sc->order == -1) /* is_via_compact_memory() */
-		goto suitable;
-
 	/* Allocation can already succeed, nothing to do */
 	if (zone_watermark_ok(zone, sc->order, min_wmark_pages(zone),
 			      sc->reclaim_idx, 0))
@@ -6608,7 +6602,7 @@ static inline bool compaction_ready(struct zone *zone, struct scan_control *sc)
 	if (compaction_suitable(zone, sc->order,
 				sc->reclaim_idx) == COMPACT_SKIPPED)
 		return false;
-suitable:
+
 	/*
 	 * Compaction is already possible, but it takes time to run and there
 	 * are potentially other callers using the pages just freed. So proceed
-- 
2.40.0

Re: [PATCH 4/5] mm: compaction: remove unnecessary is_via_compact_memory() checks

[Vlastimil Babka]

On 5/19/23 14:39, Johannes Weiner wrote:
> Remove from all paths not reachable via /proc/sys/vm/compact_memory.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

[PATCH 5/5] mm: compaction: drop redundant watermark check in compaction_zonelist_suitable()

[Johannes Weiner]

The watermark check in compaction_zonelist_suitable(), called from
should_compact_retry(), is sandwiched between two watermark checks
already: before, there are freelist attempts as part of direct reclaim
and direct compaction; after, there is a last-minute freelist attempt
in __alloc_pages_may_oom().

The check in compaction_zonelist_suitable() isn't necessary. Kill it.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/compaction.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 83ecbc829c62..40ce4e6dd17e 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2291,13 +2291,6 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
 	for_each_zone_zonelist_nodemask(zone, z, ac->zonelist,
 				ac->highest_zoneidx, ac->nodemask) {
 		unsigned long available;
-		unsigned long watermark;
-
-		/* Allocation can already succeed, nothing to do */
-		watermark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK);
-		if (zone_watermark_ok(zone, order, watermark,
-				      ac->highest_zoneidx, alloc_flags))
-			continue;
 
 		/*
 		 * Do not consider all the reclaimable memory because we do not
-- 
2.40.0

Re: [PATCH 5/5] mm: compaction: drop redundant watermark check in compaction_zonelist_suitable()

[Vlastimil Babka]

On 5/19/23 14:39, Johannes Weiner wrote:
> The watermark check in compaction_zonelist_suitable(), called from
> should_compact_retry(), is sandwiched between two watermark checks
> already: before, there are freelist attempts as part of direct reclaim
> and direct compaction; after, there is a last-minute freelist attempt
> in __alloc_pages_may_oom().
> 
> The check in compaction_zonelist_suitable() isn't necessary. Kill it.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Acked-by: Vlastimil Babka <vbabka@suse.cz>