Re: [RFC 07/13] mm, compaction: introduce direct compaction priority

[Michal Hocko]

On Tue 10-05-16 09:35:57, Vlastimil Babka wrote:
> In the context of direct compaction, for some types of allocations we would
> like the compaction to either succeed or definitely fail while trying as hard
> as possible. Current async/sync_light migration mode is insufficient, as there
> are heuristics such as caching scanner positions, marking pageblocks as
> unsuitable or deferring compaction for a zone. At least the final compaction
> attempt should be able to override these heuristics.
> 
> To communicate how hard compaction should try, we replace migration mode with
> a new enum compact_priority and change the relevant function signatures. In
> compact_zone_order() where struct compact_control is constructed, the priority
> is mapped to suitable control flags. This patch itself has no functional
> change, as the current priority levels are mapped back to the same migration
> modes as before. Expanding them will be done next.
> 
> Note that !CONFIG_COMPACTION variant of try_to_compact_pages() is removed, as
> the only caller exists under CONFIG_COMPACTION.

Your s-o-b is missing

Anyway I like the idea. The migration_mode felt really weird. It exposes
an internal detail of the compaction code which should have no business
in the allocator path.

Acked-by: Michal Hocko <mhocko@xxxxxxxx>

> ---
>  include/linux/compaction.h | 18 +++++++++---------
>  mm/compaction.c            | 14 ++++++++------
>  mm/page_alloc.c            | 27 +++++++++++++--------------
>  3 files changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> index 4ba90e74969c..900d181ff1b0 100644
> --- a/include/linux/compaction.h
> +++ b/include/linux/compaction.h
> @@ -1,6 +1,14 @@
>  #ifndef _LINUX_COMPACTION_H
>  #define _LINUX_COMPACTION_H
>  
> +// TODO: lower value means higher priority to match reclaim, makes sense?

Yes this makes sense to me.

> +enum compact_priority {

enums might be tricky but I guess it should work ok here. I would just
add

	COMPACT_MIN_PRIO,
> +	COMPACT_PRIO_SYNC_LIGHT = COMPACT_MIN_PRIO,
> +	DEF_COMPACT_PRIORITY = COMPACT_PRIO_SYNC_LIGHT,
> +	COMPACT_PRIO_ASYNC,
> +	INIT_COMPACT_PRIORITY = COMPACT_PRIO_ASYNC
> +};
> +

to make an implementation independent lowest priority.

[...]

> @@ -3269,11 +3269,11 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>  	/*
>  	 * compaction considers all the zone as desperately out of memory
>  	 * so it doesn't really make much sense to retry except when the
> -	 * failure could be caused by weak migration mode.
> +	 * failure could be caused by insufficient priority
>  	 */
>  	if (compaction_failed(compact_result)) {
> -		if (*migrate_mode == MIGRATE_ASYNC) {
> -			*migrate_mode = MIGRATE_SYNC_LIGHT;
> +		if (*compact_priority > 0) {

		if (*compact_priority > COMPACT_MIN_PRIO)

> +			(*compact_priority)--;
>  			return true;
>  		}
>  		return false;
-- 
Michal Hocko
SUSE Labs