Re: [RFC PATCH v4 7/7] mm/demotion: Demote pages according to allocation fallback order

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 27 May 2022 17:55:28 +0530
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> wrote:

> From: Jagdish Gediya <jvgediya@linux.ibm.com>
> 
> currently, a higher tier node can only be demoted to selected
> nodes on the next lower tier as defined by the demotion path,
> not any other node from any lower tier.  This strict, hard-coded
> demotion order does not work in all use cases (e.g. some use cases
> may want to allow cross-socket demotion to another node in the same
> demotion tier as a fallback when the preferred demotion node is out
> of space). This demotion order is also inconsistent with the page
> allocation fallback order when all the nodes in a higher tier are
> out of space: The page allocation can fall back to any node from any
> lower tier, whereas the demotion order doesn't allow that currently.
> 
> This patch adds support to get all the allowed demotion targets mask
> for node, also demote_page_list() function is modified to utilize this
> allowed node mask by filling it in migration_target_control structure
> before passing it to migrate_pages().
> 
> Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Ah ok. So this deals with any tier with higher rank case.

Painful though it is I would suggest the series needs recreating
as a single set of steps to reach the end goal rather than introducing
one approach then modifying it.  What you have now might work but as
a series it's very hard to review.

If their is a good reason to maintain this 'path to the answer' then
it can be done but it's going to make this harder to get review
+ merge.

Superficially this looks like it addresses my earlier comments.

Jonathan

> ---
>  include/linux/migrate.h |  5 ++++
>  mm/migrate.c            | 52 +++++++++++++++++++++++++++++++++++++----
>  mm/vmscan.c             | 38 ++++++++++++++----------------
>  3 files changed, 71 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index 77c581f47953..1f3cbd5185ca 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -182,6 +182,7 @@ void node_remove_from_memory_tier(int node);
>  int node_get_memory_tier_id(int node);
>  int node_set_memory_tier_rank(int node, int tier);
>  int node_reset_memory_tier(int node, int tier);
> +void node_get_allowed_targets(int node, nodemask_t *targets);
>  #else
>  #define numa_demotion_enabled	false
>  static inline int next_demotion_node(int node)
> @@ -189,6 +190,10 @@ static inline int next_demotion_node(int node)
>  	return NUMA_NO_NODE;
>  }
>  
> +static inline void node_get_allowed_targets(int node, nodemask_t *targets)
> +{
> +	*targets = NODE_MASK_NONE;
> +}
>  #endif	/* CONFIG_TIERED_MEMORY */
>  
>  #endif /* _LINUX_MIGRATE_H */
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 114c7428b9f3..84fac477538c 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2129,6 +2129,7 @@ struct memory_tier {
>  
>  struct demotion_nodes {
>  	nodemask_t preferred;
> +	nodemask_t allowed;
>  };
>  
>  #define to_memory_tier(device) container_of(device, struct memory_tier, dev)
> @@ -2475,6 +2476,25 @@ int node_set_memory_tier_rank(int node, int rank)
>  }
>  EXPORT_SYMBOL_GPL(node_set_memory_tier_rank);
>  
> +void node_get_allowed_targets(int node, nodemask_t *targets)
> +{
> +	/*
> +	 * node_demotion[] is updated without excluding this
> +	 * function from running.
> +	 *
> +	 * If any node is moving to lower tiers then modifications
> +	 * in node_demotion[] are still valid for this node, if any
> +	 * node is moving to higher tier then moving node may be
> +	 * used once for demotion which should be ok so rcu should
> +	 * be enough here.
> +	 */
> +	rcu_read_lock();
> +
> +	*targets = node_demotion[node].allowed;
> +
> +	rcu_read_unlock();
> +}
> +
>  /**
>   * next_demotion_node() - Get the next node in the demotion path
>   * @node: The starting node to lookup the next node
> @@ -2534,8 +2554,10 @@ static void __disable_all_migrate_targets(void)
>  {
>  	int node;
>  
> -	for_each_node_mask(node, node_states[N_MEMORY])
> +	for_each_node_mask(node, node_states[N_MEMORY]) {
>  		node_demotion[node].preferred = NODE_MASK_NONE;
> +		node_demotion[node].allowed = NODE_MASK_NONE;
> +	}
>  }
>  
>  static void disable_all_migrate_targets(void)
> @@ -2558,12 +2580,11 @@ static void disable_all_migrate_targets(void)
>  */
>  static void establish_migration_targets(void)
>  {
> -	struct list_head *ent;
>  	struct memory_tier *memtier;
>  	struct demotion_nodes *nd;
> -	int tier, target = NUMA_NO_NODE, node;
> +	int target = NUMA_NO_NODE, node;
>  	int distance, best_distance;
> -	nodemask_t used;
> +	nodemask_t used, allowed = NODE_MASK_NONE;
>  
>  	if (!node_demotion)
>  		return;
> @@ -2603,6 +2624,29 @@ static void establish_migration_targets(void)
>  			}
>  		} while (1);
>  	}
> +	/*
> +	 * Now build the allowed mask for each node collecting node mask from
> +	 * all memory tier below it. This allows us to fallback demotion page
> +	 * allocation to a set of nodes that is closer the above selected
> +	 * perferred node.
> +	 */
> +	list_for_each_entry(memtier, &memory_tiers, list)
> +		nodes_or(allowed, allowed, memtier->nodelist);
> +	/*
> +	 * Removes nodes not yet in N_MEMORY.
> +	 */
> +	nodes_and(allowed, node_states[N_MEMORY], allowed);
> +
> +	list_for_each_entry(memtier, &memory_tiers, list) {
> +		/*
> +		 * Keep removing current tier from allowed nodes,
> +		 * This will remove all nodes in current and above
> +		 * memory tier from the allowed mask.
> +		 */
> +		nodes_andnot(allowed, allowed, memtier->nodelist);
> +		for_each_node_mask(node, memtier->nodelist)
> +			node_demotion[node].allowed = allowed;
> +	}
>  }
>  
>  /*
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 1678802e03e7..feb994589481 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1454,23 +1454,6 @@ static void folio_check_dirty_writeback(struct folio *folio,
>  		mapping->a_ops->is_dirty_writeback(&folio->page, dirty, writeback);
>  }
>  
> -static struct page *alloc_demote_page(struct page *page, unsigned long node)
> -{
> -	struct migration_target_control mtc = {
> -		/*
> -		 * Allocate from 'node', or fail quickly and quietly.
> -		 * When this happens, 'page' will likely just be discarded
> -		 * instead of migrated.
> -		 */
> -		.gfp_mask = (GFP_HIGHUSER_MOVABLE & ~__GFP_RECLAIM) |
> -			    __GFP_THISNODE  | __GFP_NOWARN |
> -			    __GFP_NOMEMALLOC | GFP_NOWAIT,
> -		.nid = node
> -	};
> -
> -	return alloc_migration_target(page, (unsigned long)&mtc);
> -}
> -
>  /*
>   * Take pages on @demote_list and attempt to demote them to
>   * another node.  Pages which are not demoted are left on
> @@ -1481,6 +1464,19 @@ static unsigned int demote_page_list(struct list_head *demote_pages,
>  {
>  	int target_nid = next_demotion_node(pgdat->node_id);
>  	unsigned int nr_succeeded;
> +	nodemask_t allowed_mask;
> +
> +	struct migration_target_control mtc = {
> +		/*
> +		 * Allocate from 'node', or fail quickly and quietly.
> +		 * When this happens, 'page' will likely just be discarded
> +		 * instead of migrated.
> +		 */
> +		.gfp_mask = (GFP_HIGHUSER_MOVABLE & ~__GFP_RECLAIM) | __GFP_NOWARN |
> +			__GFP_NOMEMALLOC | GFP_NOWAIT,
> +		.nid = target_nid,
> +		.nmask = &allowed_mask
> +	};
>  
>  	if (list_empty(demote_pages))
>  		return 0;
> @@ -1488,10 +1484,12 @@ static unsigned int demote_page_list(struct list_head *demote_pages,
>  	if (target_nid == NUMA_NO_NODE)
>  		return 0;
>  
> +	node_get_allowed_targets(pgdat->node_id, &allowed_mask);
> +
>  	/* Demotion ignores all cpuset and mempolicy settings */
> -	migrate_pages(demote_pages, alloc_demote_page, NULL,
> -			    target_nid, MIGRATE_ASYNC, MR_DEMOTION,
> -			    &nr_succeeded);
> +	migrate_pages(demote_pages, alloc_migration_target, NULL,
> +		      (unsigned long)&mtc, MIGRATE_ASYNC, MR_DEMOTION,
> +		      &nr_succeeded);
>  
>  	if (current_is_kswapd())
>  		__count_vm_events(PGDEMOTE_KSWAPD, nr_succeeded);