Re: [PATCH v9 1/8] mm/demotion: Add support for explicit memory tiers

[Aneesh Kumar K V <aneesh.kumar@linux.ibm.com>]

On 7/18/22 11:28 AM, Alistair Popple wrote:
> 
> "Huang, Ying" <ying.huang@intel.com> writes:
> 
>> Wei Xu <weixugc@google.com> writes:
>>
>>> On Fri, Jul 15, 2022 at 12:53 AM Huang, Ying <ying.huang@intel.com> wrote:
>>>>
>>>> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>>>>
>>>>> In the current kernel, memory tiers are defined implicitly via a
>>>>> demotion path relationship between NUMA nodes, which is created
>>>>> during the kernel initialization and updated when a NUMA node is
>>>>> hot-added or hot-removed.  The current implementation puts all
>>>>> nodes with CPU into the top tier, and builds the tier hierarchy
>>>>> tier-by-tier by establishing the per-node demotion targets based
>>>>> on the distances between nodes.
>>>>>
>>>>> This current memory tier kernel interface needs to be improved for
>>>>> several important use cases,
>>>>>
>>>>> The current tier initialization code always initializes
>>>>> each memory-only NUMA node into a lower tier.  But a memory-only
>>>>> NUMA node may have a high performance memory device (e.g. a DRAM
>>>>> device attached via CXL.mem or a DRAM-backed memory-only node on
>>>>> a virtual machine) and should be put into a higher tier.
>>>>>
>>>>> The current tier hierarchy always puts CPU nodes into the top
>>>>> tier. But on a system with HBM or GPU devices, the
>>>>> memory-only NUMA nodes mapping these devices should be in the
>>>>> top tier, and DRAM nodes with CPUs are better to be placed into the
>>>>> next lower tier.
>>>>>
>>>>> With current kernel higher tier node can only be demoted to selected nodes on the
>>>>> next lower tier as defined by the demotion path, not any other
>>>>> node from any lower tier.  This strict, hard-coded demotion order
>>>>> does not work in all use cases (e.g. some use cases may want to
>>>>> allow cross-socket demotion to another node in the same demotion
>>>>> tier as a fallback when the preferred demotion node is out of
>>>>> space), This demotion order is also inconsistent with the page
>>>>> allocation fallback order when all the nodes in a higher tier are
>>>>> out of space: The page allocation can fall back to any node from
>>>>> any lower tier, whereas the demotion order doesn't allow that.
>>>>>
>>>>> The current kernel also don't provide any interfaces for the
>>>>> userspace to learn about the memory tier hierarchy in order to
>>>>> optimize its memory allocations.
>>>>>
>>>>> This patch series address the above by defining memory tiers explicitly.
>>>>>
>>>>> This patch introduce explicity memory tiers. The tier ID value
>>>>> of a memory tier is used to derive the demotion order between
>>>>> NUMA nodes.
>>>>>
>>>>> For example, if we have 3 memtiers: memtier100, memtier200, memiter300
>>>>> then the memory tier order is: memtier300 -> memtier200 -> memtier100
>>>>> where memtier300 is the highest tier and memtier100 is the lowest tier.
>>>>>
>>>>> While reclaim we migrate pages from fast(higher) tiers to slow(lower)
>>>>> tiers when the fast(higher) tier is under memory pressure.
>>>>>
>>>>> This patchset introduce 3 memory tiers (memtier100, memtier200 and memtier300)
>>>>> which are created by different kernel subsystems. The default memory
>>>>> tier created by the kernel is memtier200. A kernel parameter is provided
>>>>> to override the default memory tier.
>>>>>
>>>>> Link: https://lore.kernel.org/linux-mm/CAAPL-u9Wv+nH1VOZTj=9p9S70Y3Qz3+63EkqncRDdHfubsrjfw@mail.gmail.com
>>>>> Link: https://lore.kernel.org/linux-mm/7b72ccf4-f4ae-cb4e-f411-74d055482026@linux.ibm.com
>>>>>
>>>>> Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>>> ---
>>>>>  include/linux/memory-tiers.h | 15 +++++++
>>>>>  mm/Makefile                  |  1 +
>>>>>  mm/memory-tiers.c            | 78 ++++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 94 insertions(+)
>>>>>  create mode 100644 include/linux/memory-tiers.h
>>>>>  create mode 100644 mm/memory-tiers.c
>>>>>
>>>>> diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h
>>>>> new file mode 100644
>>>>> index 000000000000..a81dbc20e0d1
>>>>> --- /dev/null
>>>>> +++ b/include/linux/memory-tiers.h
>>>>> @@ -0,0 +1,15 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>> +#ifndef _LINUX_MEMORY_TIERS_H
>>>>> +#define _LINUX_MEMORY_TIERS_H
>>>>> +
>>>>> +#ifdef CONFIG_NUMA
>>>>> +
>>>>> +#define MEMORY_TIER_HBM_GPU  300
>>>>> +#define MEMORY_TIER_DRAM     200
>>>>> +#define MEMORY_TIER_PMEM     100
>>>>> +
>>>>> +#define DEFAULT_MEMORY_TIER  MEMORY_TIER_DRAM
>>>>> +#define MAX_MEMORY_TIER_ID   400
>>>>> +
>>>>> +#endif       /* CONFIG_NUMA */
>>>>> +#endif  /* _LINUX_MEMORY_TIERS_H */
>>>>> diff --git a/mm/Makefile b/mm/Makefile
>>>>> index 6f9ffa968a1a..d30acebc2164 100644
>>>>> --- a/mm/Makefile
>>>>> +++ b/mm/Makefile
>>>>> @@ -92,6 +92,7 @@ obj-$(CONFIG_KFENCE) += kfence/
>>>>>  obj-$(CONFIG_FAILSLAB) += failslab.o
>>>>>  obj-$(CONFIG_MEMTEST)                += memtest.o
>>>>>  obj-$(CONFIG_MIGRATION) += migrate.o
>>>>> +obj-$(CONFIG_NUMA) += memory-tiers.o
>>>>>  obj-$(CONFIG_DEVICE_MIGRATION) += migrate_device.o
>>>>>  obj-$(CONFIG_TRANSPARENT_HUGEPAGE) += huge_memory.o khugepaged.o
>>>>>  obj-$(CONFIG_PAGE_COUNTER) += page_counter.o
>>>>> diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c
>>>>> new file mode 100644
>>>>> index 000000000000..011877b6dbb9
>>>>> --- /dev/null
>>>>> +++ b/mm/memory-tiers.c
>>>>> @@ -0,0 +1,78 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +#include <linux/types.h>
>>>>> +#include <linux/nodemask.h>
>>>>> +#include <linux/slab.h>
>>>>> +#include <linux/lockdep.h>
>>>>> +#include <linux/moduleparam.h>
>>>>> +#include <linux/memory-tiers.h>
>>>>> +
>>>>> +struct memory_tier {
>>>>> +     struct list_head list;
>>>>> +     int id;
>>>>> +     nodemask_t nodelist;
>>>>> +};
>>>>> +
>>>>> +static DEFINE_MUTEX(memory_tier_lock);
>>>>> +static LIST_HEAD(memory_tiers);
>>>>> +
>>>>> +static void insert_memory_tier(struct memory_tier *memtier)
>>>>> +{
>>>>> +     struct list_head *ent;
>>>>> +     struct memory_tier *tmp_memtier;
>>>>> +
>>>>> +     lockdep_assert_held_once(&memory_tier_lock);
>>>>> +
>>>>> +     list_for_each(ent, &memory_tiers) {
>>>>> +             tmp_memtier = list_entry(ent, struct memory_tier, list);
>>>>> +             if (tmp_memtier->id < memtier->id) {
>>>>> +                     list_add_tail(&memtier->list, ent);
>>>>> +                     return;
>>>>> +             }
>>>>> +     }
>>>>> +     list_add_tail(&memtier->list, &memory_tiers);
>>>>> +}
>>>>> +
>>>>> +static struct memory_tier *register_memory_tier(unsigned int tier)
>>>>> +{
>>>>> +     struct memory_tier *memtier;
>>>>> +
>>>>> +     if (tier > MAX_MEMORY_TIER_ID)
>>>>> +             return ERR_PTR(-EINVAL);
>>>>> +
>>>>> +     memtier = kzalloc(sizeof(struct memory_tier), GFP_KERNEL);
>>>>> +     if (!memtier)
>>>>> +             return ERR_PTR(-ENOMEM);
>>>>> +
>>>>> +     memtier->id   = tier;
>>>>> +
>>>>> +     insert_memory_tier(memtier);
>>>>> +
>>>>> +     return memtier;
>>>>> +}
>>>>> +
>>>>> +static unsigned int default_memtier = DEFAULT_MEMORY_TIER;
>>>>> +core_param(default_memory_tier, default_memtier, uint, 0644);
>>>>> +
>>>>> +static int __init memory_tier_init(void)
>>>>> +{
>>>>> +     struct memory_tier *memtier;
>>>>> +
>>>>> +     /*
>>>>> +      * Register only default memory tier to hide all empty
>>>>> +      * memory tier from sysfs. Since this is early during
>>>>> +      * boot, we could avoid holding memtory_tier_lock. But
>>>>> +      * keep it simple by holding locks. So we can add lock
>>>>> +      * held debug checks in other functions.
>>>>> +      */
>>>>> +     mutex_lock(&memory_tier_lock);
>>>>> +     memtier = register_memory_tier(default_memtier);
>>>>> +     if (IS_ERR(memtier))
>>>>> +             panic("%s() failed to register memory tier: %ld\n",
>>>>> +                   __func__, PTR_ERR(memtier));
>>>>> +
>>>>> +     /* CPU only nodes are not part of memory tiers. */
>>>>> +     memtier->nodelist = node_states[N_MEMORY];
>>>>> +     mutex_unlock(&memory_tier_lock);
>>>>> +     return 0;
>>>>> +}
>>>>> +subsys_initcall(memory_tier_init);
>>>>
>>>> You dropped the original sysfs interface patches from the series, but
>>>> the kernel internal implementation is still for the original sysfs
>>>> interface.  For example, memory tier ID is for the original sysfs
>>>> interface, not for the new proposed sysfs interface.  So I suggest you
>>>> to implement with the new interface in mind.  What do you think about
>>>> the following design?
>>>>
>>>> - Each NUMA node belongs to a memory type, and each memory type
>>>>   corresponds to a "abstract distance", so each NUMA node corresonds to
>>>>   a "distance".  For simplicity, we can start with static distances, for
>>>>   example, DRAM (default): 150, PMEM: 250.
>>>
>>> I agree with this design, though I'd prefer the new attribute to not
>>> be named as "distance".  This is to both avoid the confusion with the
>>> SLIT distance and to avoid the misconception that only the latency
>>> matters, but the bandwidth doesn't.
>>>
>>> How about we call it "performance level" (perf_level) or something
>>> similar instead?
>>
>> I have no strong opinion on this.  Both "distance" or "perf_level" looks
>> OK to me.
>>
>>>> The distance of each NUMA
>>>>   node can be recorded in a global array,
>>>>
>>>>     int node_distances[MAX_NUMNODES];
>>>>
>>>>   or, just
>>>>
>>>>     pgdat->distance
>>>
>>> I think node_devices[] is a better place to record this new attribute.
>>> The HMAT performance data is also listed there.
>>
>> Firstly, we all agree that we need a place to record this information,
>> per node or per memory type.  Personally, I prefer to separate the data
>> and its interface (such as sysfs).
>>
>>>> - Each memory tier corresponds to a range of distance, for example,
>>>>   0-100, 100-200, 200-300, >300, we can start with static ranges too.
>>>>
>>>> - The core API of memory tier could be
>>>>
>>>>     struct memory_tier *find_create_memory_tier(int distance);
>>>>
>>>>   it will find the memory tier which covers "distance" in the memory
>>>>   tier list, or create a new memory tier if not found.
>>>>
>>>> - kmem_dax driver will setup distance for PMEM NUMA nodes before online
>>>>   them.
>>>
>>> This attribute should be a property of the NUMA node based on the
>>> device hardware.
>>
>> Yes.  Or a property of a memory type.
>>
>>> For PMEM, it is better to handle at the ACPI level.
>>> For example, we can consider initializing this attribute for a PMEM
>>> node in acpi_numa_memory_affinity_init() when the node is
>>> non-volatile.
>>
>> The abstract_distance/perf_level may be determined from multiple
>> information sources, e.g., ACPI SLIT/SRAT/HMAT, etc.  It should be the
>> responsibility of device drivers (e.g., kmem_dax) to determine the final
>> value of abstract_distance/perf_level based on the information
>> availability/priority and some specific knowledge of the hardware.  Yes,
>> ACPI SRAT is valuable to determine the abstract_distance/perf_level.
>> And, it's better for kmem_dax to use it to determine the final value of
>> abstract_distance/perf_level.
>>
>> To make the first version as simple as possible, I think we can just use
>> some static abstract_distance/perf_level in kmem_dax driver for the NUMA
>> nodes onlined by it.  Because we use the driver for PMEM only now.  We
>> can enhance the implementation later.
> 
> I agree. Ideally I think all this should be derived from ACPI tables,
> etc. However I think it will take a while for both FW and SW to make
> that information available and correct. Letting drivers initialise that
> for now at least should aid development in determining how performance
> levels should be set from multiple information sources, especially if
> there is no way of overriding it from userspace.
>

When we parse the firmware tables, node_devices is mostly not allocated.
That get allocated in register_one_node. We can do a hotplug
callback like below. This should also allow us to update perf_level based
ACPI tables.

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index ae5f4acf2675..89b010e0461e 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -15,6 +15,7 @@
 #include <linux/sort.h>
 #include <linux/io.h>
 #include <linux/nd.h>
+#include <linux/memory.h>
 #include <asm/cacheflush.h>
 #include <acpi/nfit.h>
 #include "intel.h"
@@ -3470,6 +3471,45 @@ static struct acpi_driver acpi_nfit_driver = {
 	},
 };
 
+static int nfit_callback(struct notifier_block *self,
+			 unsigned long action, void *arg)
+{
+	bool found = false;
+	struct memory_notify *mnb = arg;
+	int nid = mnb->status_change_nid;
+	struct nfit_spa *nfit_spa;
+	struct acpi_nfit_desc *acpi_desc;
+
+	if (nid == NUMA_NO_NODE || action != MEM_ONLINE)
+		return NOTIFY_OK;
+
+	mutex_lock(&acpi_desc_lock);
+	list_for_each_entry(acpi_desc, &acpi_descs, list) {
+		mutex_lock(&acpi_desc->init_mutex);
+		list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
+			struct acpi_nfit_system_address *spa = nfit_spa->spa;
+			int target_node = pxm_to_node(spa->proximity_domain);
+
+			if (target_node == nid) {
+				node_devices[nid]->perf_level = 1;
+				found = true;
+				break;
+			}
+		}
+		mutex_unlock(&acpi_desc->init_mutex);
+		if (found)
+			break;
+	}
+	mutex_unlock(&acpi_desc_lock);
+	return NOTIFY_OK;
+}
+
+static struct notifier_block nfit_callback_nb = {
+	.notifier_call = nfit_callback,
+	.priority = 2,
+};
+
+
 static __init int nfit_init(void)
 {
 	int ret;
@@ -3509,7 +3549,11 @@ static __init int nfit_init(void)
 		nfit_mce_unregister();
 		destroy_workqueue(nfit_wq);
 	}
-
+	/*
+	 * register a memory hotplug notifier at prio 2 so that we
+	 * can update the perf level for the node.
+	 */
+	register_hotmemory_notifier(&nfit_callback_nb);
 	return ret;
 
 }