Re: [PATCH 02/10] mm/numa: automatically generate node migration order

[Dave Hansen <dave.hansen@intel.com>]

On 4/8/21 1:26 AM, Oscar Salvador wrote:
> On Thu, Apr 01, 2021 at 11:32:19AM -0700, Dave Hansen wrote:
>> The protocol for node_demotion[] access and writing is not
>> standard.  It has no specific locking and is intended to be read
>> locklessly.  Readers must take care to avoid observing changes
>> that appear incoherent.  This was done so that node_demotion[]
> 
> It might be just me being dense here, but that reads odd.
> 
> "Readers must take care to avoid observing changes that appear
> incoherent" - I am not sure what is that supposed to mean.
> 
> I guess you mean readers of next_demotion_node()?
> And if so, how do they have to take care? And what would apply for
> "incoherent" terminology here?

I've fleshed out the description a bit.  I hope this helps?

> Readers of node_demotion[] (such as next_demotion_node() callers)
> must take care to avoid observing changes that appear incoherent.
> For instance, even though no demotion cycles are allowed, it's
> possible that a cycle could be observed.
> 
> Let's say that there are three nodes, A, B and C.  node_demotion[]
> is set up to have this path:
>         
>         A -> B -> C
> 
> Suppose it was modified to instead represent this path:
> 
>         A -> C -> B
> 
> There is nothing to stop a reader from seeing B->C and then a
> moment later seeting C->B.  That *appears* to be a cycle.  This
> can be avoided with RCU and will be implemented in a later patch.

...
>> +again:
>> +	this_pass = next_pass;
>> +	next_pass = NODE_MASK_NONE;
>> +	/*
>> +	 * To avoid cycles in the migration "graph", ensure
>> +	 * that migration sources are not future targets by
>> +	 * setting them in 'used_targets'.  Do this only
>> +	 * once per pass so that multiple source nodes can
>> +	 * share a target node.
>> +	 *
>> +	 * 'used_targets' will become unavailable in future
>> +	 * passes.  This limits some opportunities for
>> +	 * multiple source nodes to share a destination.
>> +	 */
>> +	nodes_or(used_targets, used_targets, this_pass);
>> +	for_each_node_mask(node, this_pass) {
>> +		int target_node = establish_migrate_target(node, &used_targets);
>> +
>> +		if (target_node == NUMA_NO_NODE)
>> +			continue;
>> +
>> +		/* Visit targets from this pass in the next pass: */
>> +		node_set(target_node, next_pass);
>> +	}
>> +	/* Is another pass necessary? */
>> +	if (!nodes_empty(next_pass))
> 
> When I read this I was about puzzled and it took me a while to figure
> out how the passes were made.
> I think this could benefit from a better explanation on how the passes
> are being performed e.g: why next_pass should be empty before leaving.
> 
> Other than that looks good to me.
I've tried to flesh out those comments to elaborate on what is going on:

>                 /*
>                  * Visit targets from this pass in the next pass.
>                  * Eventually, every node will have been part of
>                  * a pass, and will become set in 'used_targets'.
>                  */
>                 node_set(target_node, next_pass);
>         }
>         /*
>          * 'next_pass' contains nodes which became migration
>          * targets in this pass.  Make additional passes until
>          * no more migrations targets are available.
>          */
>         if (!nodes_empty(next_pass))
>                 goto again;
> }