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

[Ying Huang <ying.huang@intel.com>]

On Wed, 2022-06-08 at 20:07 +0530, Aneesh Kumar K.V wrote:
> Ying Huang <ying.huang@intel.com> writes:
> 
> ....
> 
> > > > 
> > > > > is this good (not tested)?
> > > > > /*
> > > > >    * build the allowed promotion mask. Promotion is allowed
> > > > >    * from higher memory tier to lower memory tier only if
> > > > >    * lower memory tier doesn't include compute. We want to
> > > > >    * skip promotion from a memory tier, if any node which is
> > > > >    * part of that memory tier have CPUs. Once we detect such
> > > > >    * a memory tier, we consider that tier as top tier from
> > > > >    * which promotion is not allowed.
> > > > >    */
> > > > > list_for_each_entry_reverse(memtier, &memory_tiers, list) {
> > > > > 	nodes_and(allowed, node_state[N_CPU], memtier->nodelist);
> > > > > 	if (nodes_empty(allowed))
> > > > > 		nodes_or(promotion_mask, promotion_mask, allowed);
> > > > > 	else
> > > > > 		break;
> > > > > }
> > > > > 
> > > > > and then
> > > > > 
> > > > > static inline bool node_is_toptier(int node)
> > > > > {
> > > > > 
> > > > > 	return !node_isset(node, promotion_mask);
> > > > > }
> > > > > 
> > > > 
> > > > This should work.  But it appears unnatural.  So, I don't think we
> > > > should avoid to add more and more node masks to mitigate the design
> > > > decision that we cannot access memory tier information directly.  All
> > > > these becomes simple and natural, if we can access memory tier
> > > > information directly.
> > > > 
> > > 
> > > how do you derive whether node is toptier details if we have memtier 
> > > details in pgdat?
> > 
> > pgdat -> memory tier -> rank
> > 
> > Then we can compare this rank with the fast memory rank.  The fast
> > memory rank can be calculated dynamically at appropriate places.
> 
> This is what I am testing now. We still need to closely audit that lock
> free access to the NODE_DATA()->memtier. For v6 I will keep this as a
> separate patch and once we all agree that it is safe, I will fold it
> back.

Thanks for doing this.  We finally have a way to access memory_tier in
hot path.

[snip]

> +/*
> + * 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();

Per my understanding, in your code, when we change pgdat->memtier, we
will call synchronize_rcu() twice.  IMHO, once should be OK.  That is,
something like below,

	rcu_assign_pointer(pgdat->memtier, NULL);
	node_clear(node, memtier->nodelist);
	synchronize_rcu();
	node_set(node, new_memtier->nodelist);
	rcu_assign_pointer(pgdat->memtier, new_memtier);

In this way, there will be 3 states,

1. prev

pgdat->memtier == old_memtier
node_isset(node, old_memtier->node_list)
!node_isset(node, new_memtier->node_list)

2. transitioning

pgdat->memtier == NULL
!node_isset(node, old_memtier->node_list)
!node_isset(node, new_memtier->node_list)

3. after

pgdat->memtier == new_memtier
!node_isset(node, old_memtier->node_list)
node_isset(node, new_memtier->node_list)

The real state may be one of 1, 2, 3, 1+2, or 2+3.  But it will not be
1+3.  I think that this satisfied our requirements.

[snip]

>  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);+	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;
> +}
> +
> 
>   	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;
>  	}
>  
> 

[snip]

>  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;

No required absoluately.  But IMHO it's more consistent to use
rcu_assign_pointer() here.

> +		node_set(node, memtier->nodelist);
> +	}
>  	migrate_on_reclaim_init();
>  
> > 	return 0;

Best Regareds,
Huang, Ying