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@xxxxxxxxxxx>
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@xxxxxxx>
> ---
> 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
