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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87mtckiqsr.fsf@yhuang6-desk2.ccr.corp.intel.com>
Date:   Thu, 04 Aug 2022 12:19:32 +0800
From:   "Huang, Ying" <ying.huang@...el.com>
To:     "Aneesh Kumar K.V" <aneesh.kumar@...ux.ibm.com>
Cc:     linux-mm@...ck.org, akpm@...ux-foundation.org,
        Wei Xu <weixugc@...gle.com>, Yang Shi <shy828301@...il.com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Tim C Chen <tim.c.chen@...el.com>,
        Michal Hocko <mhocko@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Hesham Almatary <hesham.almatary@...wei.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Alistair Popple <apopple@...dia.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Johannes Weiner <hannes@...xchg.org>, jvgediya.oss@...il.com
Subject: Re: [PATCH v10 5/8] mm/demotion: Build demotion targets based on
 explicit memory tiers

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

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