Re: [PATCH v10 5/8] mm/demotion: Build demotion targets based on explicit memory tiers

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

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:

 > "Huang, Ying" <ying.huang@intel.com> writes:
>
>> Aneesh Kumar K V <aneesh.kumar@linux.ibm.com> writes:
>>
>>> On 7/26/22 1:14 PM, Huang, Ying wrote:
>>>> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>
> ....
>
>>>> +
>>>>>  static void init_node_memory_tier(int node)
>>>>>  {
>>>>>  	int perf_level;
>>>>> @@ -84,11 +285,19 @@ static void init_node_memory_tier(int node)
>>>>>  	mutex_lock(&memory_tier_lock);
>>>>>  
>>>>>  	memtier = __node_get_memory_tier(node);
>>>>> +	/*
>>>>> +	 * if node is already part of the tier proceed with the
>>>>> +	 * current tier value, because we might want to establish
>>>>> +	 * new migration paths now. The node might be added to a tier
>>>>> +	 * before it was made part of N_MEMORY, hence estabilish_migration_targets
>>>>> +	 * will have skipped this node.
>>>>> +	 */
>>>>>  	if (!memtier) {
>>>>>  		perf_level = node_devices[node]->perf_level;
>>>>>  		memtier = find_create_memory_tier(perf_level);
>>>>>  		node_set(node, memtier->nodelist);
>>>>>  	}
>>>>> +	establish_migration_targets();
>>>> 
>>>> Why combines memory tiers establishing with demotion targets building?
>>>> I think that it's better to separate them.   For example, if we move a
>>>> set of NUMA node from one memory tier to another memory tier, we only
>>>> need to run establish_migration_targets() once after moving all nodes.
>>>> 
>>>
>>> Yes agree. I am not sure I followed your comment here. 
>>>
>>> Demotion target rebuilding is a separate helper. Any update to memory tiers needs rebuilding
>>> of demotion targets. Also any change in node mask of memory tier needs
>>> demotion target rebuild. Can you clarify the code change you are suggesting here?
>>
>> I think we should call establish_migration_targets() in
>> migrate_on_reclaim_callback() directly.  As the example I mentioned
>> above, sometimes, we don't need to call establish_migration_targets()
>> for each node changing.
>>
>
> We need to hold memory_tier_lock while updating node's memory tier and
> rebuilding demotion targets. All of that is done in the same function
> here. An update node memory tier that allow for updating multiple node's
> memory tier together would do what you mentioned above under
> memory_tier_lock ie, update all the nodes memory tier and then call
> establish_migration_targets() once.

I don't think it's good to duplicate code unnecessarily.  Managing
memory tiers and estabilishing demotion target are two separate stuff.
We shouldn't combined them.  If memory_tier_lock needs to be held, just
enclosing estabilish_migration_targets() with it.

Best Regards,
Huang, Ying