Message-ID: <878ro7jtiz.fsf@yhuang6-desk2.ccr.corp.intel.com>
Date:   Tue, 02 Aug 2022 09:58:28 +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 v11 4/8] mm/demotion/dax/kmem: Set node's abstract
 distance to MEMTIER_ADISTANCE_PMEM

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

> On 8/1/22 12:43 PM, Huang, Ying wrote:
>> Aneesh Kumar K V <aneesh.kumar@...ux.ibm.com> writes:
>> 
>>> On 8/1/22 12:07 PM, Huang, Ying wrote:
>>>> Aneesh Kumar K V <aneesh.kumar@...ux.ibm.com> writes:
>>>>
>>>>> On 8/1/22 10:40 AM, Huang, Ying wrote:
>>>>>> Aneesh Kumar K V <aneesh.kumar@...ux.ibm.com> writes:
>>>>>>
>>>>>>> On 8/1/22 7:36 AM, Huang, Ying wrote:
>>>>>>>> "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:
>>>
>>> ....
>>>
>>>>>>>
>>>>>>> With the module unload, it is kind of force removing the usage of the specific memtype.
>>>>>>> Considering module unload will remove the usage of specific memtype from other parts
>>>>>>> of the kernel and we already do all the required reset in memory hot unplug, do we
>>>>>>> need to do the clear_node_memory_type above? 
>>>>>>
>>>>>> Per my understanding, we need to call clear_node_memory_type() in
>>>>>> dev_dax_kmem_remove().  After that, we have nothing to do in
>>>>>> dax_kmem_exit().
>>>>>>
>>>>>
>>>>> Ok, I guess you are suggesting to do the clear_node_memory_type even if we fail the memory remove. 
>>>>
>>>> Can we use node_memory_types[] to indicate whether a node is managed by
>>>> a driver?
>>>>
>>>> Regardless being succeeded or failed, dev_dax_kmem_remove() will set
>>>> node_memory_types[] = NULL.  But until node is offlined, we will still
>>>> keep the node in the memory_dev_type (dax_pmem_type).
>>>>
>>>> And we will prevent dax/kmem from unloading via try_module_get() and add
>>>> "struct module *" to struct memory_dev_type.
>>>>
>>>
>>> Current dax/kmem driver is not holding any module reference and allows the module to be unloaded
>>> anytime. Even if the memory onlined by the driver fails to be unplugged. Addition of memory_dev_type
>>> as suggested by you will be different than that. Page demotion can continue to work without the
>>> support of dax_pmem_type as long as we keep the older demotion order. Any new demotion order
>>> rebuild will remove the the memory node which was not hotunplugged  from the demotion order. Isn't that
>>> a much simpler implementation? 
>> 
>> Per my understanding, unbinding/binding the dax/kmem driver means
>> changing the memory type of a memory device.  For example, unbinding
>> dax/kmem driver may mean changing the memory type from dax_pmem_type to
>> default_memory_type (or default_dram_type).  That appears strange.  But
>> if we force the NUMA node to be offlined for unbinding, we can avoid to
>> change the memory type to default_memory_type.
>> 
>
> If we are able to unplug all the memory, we do remove the node from N_MEMORY.
> If we fail to unplug the memory, we have two options. 
>
> 1) Keep the same demotion order
> 2) Rebuild the demotion order which results in memory NUMA node not participating
>    in demotion. 
>
> I agree with you that we should not switch to default memory type. 
>
> The below code demonstrate how it can be done. If we want to keep
> the same demotion order, we can remove establish_demotion_target() from
> the below code. 
>
> void clear_node_memory_type(int node, struct memory_dev_type *memtype)
> {
> 	struct memory_tier *memtier;
> 	pg_data_t *pgdat = NODE_DATA(node);
>
> 	mutex_lock(&memory_tier_lock);
> 	/*
> 	 * Even if we fail to unplug memory, clear the association of
> 	 * this node to this specific memory type.
> 	 */
> 	if (node_isset(node, memtype->nodes) && node_memory_types[node] == memtype) {
>
> 		memtier = __node_get_memory_tier(node);
> 		if (memtier) {
> 			rcu_assign_pointer(pgdat->memtier, NULL);
> 			synchronize_rcu();
> 		}
> 		node_clear(node, memtype->nodes);
> 		if (nodes_empty(memtype->nodes)) {
> 			list_del(&memtype->tier_sibiling);
> 			memtype->memtier = NULL;
> 			if (memtier && list_empty(&memtier->memory_types))
> 				destroy_memory_tier(memtier);
>
> 		}
> 		establish_demotion_targets();
> 	}
> 	node_memory_types[node] = NULL;
> 	mutex_unlock(&memory_tier_lock);
> }
>
>
> If we agree that we want to keep the current behavior (that is to allow kmem driver unload
> even on memory unplug failure) we can go with the above change. If we are suggesting we
> should prevent a driver unload, then IMHO it should be independent of memory_dev_type
> (or this patch series). We should make sure we take a module reference on successful
> memory online and drop it only on successful offline. 

I suggest to keep a NUMA node in the memory_dev_type (dax_pmem_type)
until the node is offlined.

Yes.  The dax/kmem driver may be unbound to the dax device even if
memory offlining fails.  But we can still find someway to keep
the memory_dev_type of the NUMA node unchanged.

Solution 1 is to prevent dax/kmem driver from unloading via module
reference.  I think we do that in this series.

Solution 2 is to allocate dax_pmem_type dynamically, and manage it like
"kmem_name".  We may need some kind of reference counting to manage it.

Best Regards,
Huang, Ying

Powered by blists - more mailing lists