"Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxx> writes:
> "Huang, Ying" <ying.huang@xxxxxxxxx> writes:
>
>> Aneesh Kumar K V <aneesh.kumar@xxxxxxxxxxxxx> writes:
>>
>>> On 7/26/22 1:14 PM, Huang, Ying wrote:
>>>> "Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxx> 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
