Re: [PATCH v5 9/9] mm/demotion: Update node_is_toptier to work with memory tiers

[Tim Chen <tim.c.chen@linux.intel.com>]

On Wed, 2022-06-08 at 20:07 +0530, Aneesh Kumar K.V wrote:
> 
> 
> This is what I am testing now. We still need to closely audit that lock
> free access to the NODE_DATA()->memtier. 

You're refering to this or something else?  This is a write so seems okay.

> +	for_each_node_state(node, N_MEMORY) {
> +		/*
> +		 * Should be safe to do this early in the boot.
> +		 */
> +		NODE_DATA(node)->memtier = memtier;
> +		node_set(node, memtier->nodelist);
> +	}
>  	migrate_on_reclaim_init();


> For v6 I will keep this as a
> separate patch and once we all agree that it is safe, I will fold it
> back.

Please update code that uses __node_get_memory_tier(node) to use
NODE_DATA(node)->memtier;

Otherwise the code looks okay at a first glance.

Tim

> 
> diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h
> index a388a806b61a..3e733de1a8a0 100644
> --- a/include/linux/memory-tiers.h
> +++ b/include/linux/memory-tiers.h
> @@ -17,7 +17,6 @@
>  #define MAX_MEMORY_TIERS  (MAX_STATIC_MEMORY_TIERS + 2)
>  
>  extern bool numa_demotion_enabled;
> -extern nodemask_t promotion_mask;
>  int node_create_and_set_memory_tier(int node, int tier);
>  int next_demotion_node(int node);
>  int node_set_memory_tier(int node, int tier);
> @@ -25,15 +24,7 @@ int node_get_memory_tier_id(int node);
>  int node_reset_memory_tier(int node, int tier);
>  void node_remove_from_memory_tier(int node);
>  void node_get_allowed_targets(int node, nodemask_t *targets);
> -
> -/*
> - * By default all nodes are top tiper. As we create new memory tiers
> - * we below top tiers we add them to NON_TOP_TIER state.
> - */
> -static inline bool node_is_toptier(int node)
> -{
> -	return !node_isset(node, promotion_mask);
> -}
> +bool node_is_toptier(int node);
>  
>  #else
>  #define numa_demotion_enabled	false
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index aab70355d64f..c4fcfd2b9980 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -928,6 +928,9 @@ typedef struct pglist_data {
>  	/* Per-node vmstats */
>  	struct per_cpu_nodestat __percpu *per_cpu_nodestats;
>  	atomic_long_t		vm_stat[NR_VM_NODE_STAT_ITEMS];
> +#ifdef CONFIG_TIERED_MEMORY
> +	struct memory_tier *memtier;
> +#endif
>  } pg_data_t;
>  
>  #define node_present_pages(nid)	(NODE_DATA(nid)->node_present_pages)
> diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c
> index 29a038bb38b0..31ef0fab5f19 100644
> --- a/mm/memory-tiers.c
> +++ b/mm/memory-tiers.c
> @@ -7,6 +7,7 @@
>  #include <linux/random.h>
>  #include <linux/memory.h>
>  #include <linux/idr.h>
> +#include <linux/rcupdate.h>
>  
>  #include "internal.h"
>  
> @@ -26,7 +27,7 @@ struct demotion_nodes {
>  static void establish_migration_targets(void);
>  static DEFINE_MUTEX(memory_tier_lock);
>  static LIST_HEAD(memory_tiers);
> -nodemask_t promotion_mask;
> +static int top_tier_rank;
>  /*
>   * node_demotion[] examples:
>   *
> @@ -135,7 +136,7 @@ static void memory_tier_device_release(struct device *dev)
>  	if (tier->dev.id >= MAX_STATIC_MEMORY_TIERS)
>  		ida_free(&memtier_dev_id, tier->dev.id);
>  
> -	kfree(tier);
> +	kfree_rcu(tier);
>  }
>  
>  /*
> @@ -233,6 +234,70 @@ static struct memory_tier *__get_memory_tier_from_id(int id)
>  	return NULL;
>  }
>  
> +/*
> + * Called with memory_tier_lock. Hence the device references cannot
> + * be dropped during this function.
> + */
> +static void memtier_node_clear(int node, struct memory_tier *memtier)
> +{
> +	pg_data_t *pgdat;
> +
> +	pgdat = NODE_DATA(node);
> +	if (!pgdat)
> +		return;
> +
> +	rcu_assign_pointer(pgdat->memtier, NULL);
> +	/*
> +	 * Make sure read side see the NULL value before we clear the node
> +	 * from the nodelist.
> +	 */
> +	synchronize_rcu();
> +	node_clear(node, memtier->nodelist);
> +}
> +
> +static void memtier_node_set(int node, struct memory_tier *memtier)
> +{
> +	pg_data_t *pgdat;
> +
> +	pgdat = NODE_DATA(node);
> +	if (!pgdat)
> +		return;
> +	/*
> +	 * Make sure we mark the memtier NULL before we assign the new memory tier
> +	 * to the NUMA node. This make sure that anybody looking at NODE_DATA
> +	 * finds a NULL memtier or the one which is still valid.
> +	 */
> +	rcu_assign_pointer(pgdat->memtier, NULL);
> +	synchronize_rcu();
> +	node_set(node, memtier->nodelist);
> +	rcu_assign_pointer(pgdat->memtier, memtier);
> +}
> +
> +bool node_is_toptier(int node)
> +{
> +	bool toptier;
> +	pg_data_t *pgdat;
> +	struct memory_tier *memtier;
> +
> +	pgdat = NODE_DATA(node);
> +	if (!pgdat)
> +		return false;
> +
> +	rcu_read_lock();
> +	memtier = rcu_dereference(pgdat->memtier);
> +	if (!memtier) {
> +		toptier = true;
> +		goto out;
> +	}
> +	if (memtier->rank >= top_tier_rank)
> +		toptier = true;
> +	else
> +		toptier = false;
> +out:
> +	rcu_read_unlock();
> +	return toptier;
> +}
> +
>  static int __node_create_and_set_memory_tier(int node, int tier)
>  {
>  	int ret = 0;
> @@ -253,7 +318,7 @@ static int __node_create_and_set_memory_tier(int node, int tier)
>  			goto out;
>  		}
>  	}
> -	node_set(node, memtier->nodelist);
> +	memtier_node_set(node, memtier);
>  out:
>  	return ret;
>  }
> @@ -275,12 +340,12 @@ int node_create_and_set_memory_tier(int node, int tier)
>  	if (current_tier->dev.id == tier)
>  		goto out;
>  
> -	node_clear(node, current_tier->nodelist);
> +	memtier_node_clear(node, current_tier);
>  
>  	ret = __node_create_and_set_memory_tier(node, tier);
>  	if (ret) {
>  		/* reset it back to older tier */
> -		node_set(node, current_tier->nodelist);
> +		memtier_node_set(node, current_tier);
>  		goto out;
>  	}
>  
> @@ -305,7 +370,7 @@ static int __node_set_memory_tier(int node, int tier)
>  		ret = -EINVAL;
>  		goto out;
>  	}
> -	node_set(node, memtier->nodelist);
> +	memtier_node_set(node, memtier);
>  out:
>  	return ret;
>  }
> @@ -374,12 +439,12 @@ int node_reset_memory_tier(int node, int tier)
>  	if (current_tier->dev.id == tier)
>  		goto out;
>  
> -	node_clear(node, current_tier->nodelist);
> +	memtier_node_clear(node, current_tier);
>  
>  	ret = __node_set_memory_tier(node, tier);
>  	if (ret) {
>  		/* reset it back to older tier */
> -		node_set(node, current_tier->nodelist);
> +		memtier_node_set(node, current_tier);
>  		goto out;
>  	}
>  
> @@ -407,7 +472,7 @@ void node_remove_from_memory_tier(int node)
>  	 * empty then unregister it to make it invisible
>  	 * in sysfs.
>  	 */
> -	node_clear(node, memtier->nodelist);
> +	memtier_node_clear(node, memtier);
>  	if (nodes_empty(memtier->nodelist))
>  		unregister_memory_tier(memtier);
>  
> @@ -570,15 +635,13 @@ static void establish_migration_targets(void)
>  	 * a memory tier, we consider that tier as top tiper from
>  	 * which promotion is not allowed.
>  	 */
> -	promotion_mask = NODE_MASK_NONE;
>  	list_for_each_entry_reverse(memtier, &memory_tiers, list) {
>  		nodes_and(allowed, node_states[N_CPU], memtier->nodelist);
> -		if (nodes_empty(allowed))
> -			nodes_or(promotion_mask, promotion_mask, memtier->nodelist);
> -		else
> +		if (!nodes_empty(allowed)) {
> +			top_tier_rank = memtier->rank;
>  			break;
> +		}
>  	}
> -
>  	pr_emerg("top tier rank is %d\n", top_tier_rank);
>  	allowed = NODE_MASK_NONE;
>  	/*
> @@ -748,7 +811,7 @@ static const struct attribute_group *memory_tier_attr_groups[] = {
>  
>  static int __init memory_tier_init(void)
>  {
> -	int ret;
> +	int ret, node;
>  	struct memory_tier *memtier;
>  
>  	ret = subsys_system_register(&memory_tier_subsys, memory_tier_attr_groups);
> @@ -766,7 +829,13 @@ static int __init memory_tier_init(void)
>  		panic("%s() failed to register memory tier: %d\n", __func__, ret);
>  
>  	/* CPU only nodes are not part of memory tiers. */
> -	memtier->nodelist = node_states[N_MEMORY];
> +	for_each_node_state(node, N_MEMORY) {
> +		/*
> +		 * Should be safe to do this early in the boot.
> +		 */
> +		NODE_DATA(node)->memtier = memtier;
> +		node_set(node, memtier->nodelist);
> +	}
>  	migrate_on_reclaim_init();
>  
>  	return 0;