memory tier: consolidate the initialization of memory tiers

PATCH Superseded Ho-Ren (Jack) Chuang 3 participants 9 messages Jun 28, 2024 – Jul 4, 2024

Subsystems (3)

Memory management ACPI MM NUMA placement

lore

Share & tools

lore

Ho-Ren (Jack) Chuang

author

2024-06-28 06:09 UTC #0 lore raw

v2 cover memory tier: consolidate the initialization of memory tiers

The current memory tier initialization process is distributed across two different functions, memory_tier_init() and memory_tier_late_init(). This design is hard to maintain. Thus, this patch is proposed to reduce the possible code paths by consolidating different initialization patches into one.

The earlier discussion with Jonathan and Ying is listed here:
https://lore.kernel.org/lkml/[email protected]/

If we want to put these two initializations together, they must be placed together in the later function. Because only at that time, the HMAT information will be ready, adist between nodes can be calculated, and memory tiering can be established based on the adist. So we position the initialization at memory_tier_init() to the memory_tier_late_init() call.

Moreover, it's natural to keep memory_tier initialization in drivers at device_initcall() level.

- v2:
Thanks to Huang, Ying's and Andrew's comments
* Add cover letter
* Add Suggested-by: Jonathan Cameron <[email protected]>
* Add get/put_online_mems() protection in memory_tier_late_init()
* If memtype is set, skip initializing its node
* Remove redundant code/comments or rewrite code in a cleaner manner
- v1:
* https://lore.kernel.org/all/[email protected]/

This patchset is based on commits cf93be18fa1b and a72a30af550c:
[0/2] https://lkml.kernel.org/r/[email protected]
[1/2] https://lkml.kernel.org/r/[email protected]
[1/2] https://lkml.kernel.org/r/[email protected]

Ho-Ren (Jack) Chuang (1):
memory tier: consolidate the initialization of memory tiers

 drivers/acpi/numa/hmat.c     |  5 +--
 include/linux/memory-tiers.h |  2 ++
 mm/memory-tiers.c            | 59 +++++++++++++++---------------------
 3 files changed, 28 insertions(+), 38 deletions(-)

Signature · 1 line

Ho-Ren (Jack) Chuang

Ho-Ren (Jack) Chuang

author

2024-06-28 06:09 UTC #1 lore raw

PATCH v2 1/1 memory tier: consolidate the initialization of memory tiers

If we simply move the set_node_memory_tier() from memory_tier_init() to late_initcall(), it will result in HMAT not registering the mt_adistance_algorithm callback function, because set_node_memory_tier() is not performed during the memory tiering initialization phase, leading to a lack of correct default_dram information.

Therefore, we introduced a nodemask to pass the information of the default DRAM nodes. The reason for not choosing to reuse default_dram_type->nodes is that it is not clean enough. So in the end, we use a __initdata variable, which is a variable that is released once initialization is complete, including both CPU and memory nodes for HMAT to iterate through.

Besides, since default_dram_type may be checked/used during the initialization process of HMAT and drivers, it is better to keep the allocation of default_dram_type in memory_tier_init().

Signed-off-by Ho-Ren (Jack) Chuang <[email protected]>

---
 drivers/acpi/numa/hmat.c     |  5 +--
 include/linux/memory-tiers.h |  2 ++
 mm/memory-tiers.c            | 59 +++++++++++++++---------------------
 3 files changed, 28 insertions(+), 38 deletions(-)

drivers/acpi/numa/hmat.c +1 -4

index 2c8ccc91ebe6..a2f9e7a4b479 100644
@@ -940,10 +940,7 @@ static int hmat_set_default_dram_perf(void)
 	struct memory_target *target;
 	struct access_coordinate *attrs;
 
-	if (!default_dram_type)
-		return -EIO;
-
-	for_each_node_mask(nid, default_dram_type->nodes) {
+	for_each_node_mask(nid, default_dram_nodes) {
 		pxm = node_to_pxm(nid);
 		target = find_mem_target(pxm);
 		if (!target)

include/linux/memory-tiers.h +2 -0

index 0d70788558f4..fa61ad9c4d75 100644
@@ -38,6 +38,7 @@ struct access_coordinate;
 #ifdef CONFIG_NUMA
 extern bool numa_demotion_enabled;
 extern struct memory_dev_type *default_dram_type;
+extern nodemask_t default_dram_nodes __initdata;
 struct memory_dev_type *alloc_memory_type(int adistance);
 void put_memory_type(struct memory_dev_type *memtype);
 void init_node_memory_type(int node, struct memory_dev_type *default_type);

@@ -76,6 +77,7 @@ static inline bool node_is_toptier(int node)
 
 #define numa_demotion_enabled	false
 #define default_dram_type	NULL
+#define default_dram_nodes NODE_MASK_NONE
 /*
  * CONFIG_NUMA implementation returns non NULL error.
  */

mm/memory-tiers.c +25 ~~-34~~

103 lines not on this page · raw message

Signature · 1 line

Ho-Ren (Jack) Chuang

Huang, Ying

2024-07-01 05:15 UTC #2 lore raw

Hi, Jack,

"Ho-Ren (Jack) Chuang" <[email protected]> writes:

I suggest you to merge the [0/1] with the change log here. [0/1] describes why do we need the patch. The below text describes some details. Just don't use "---" to separate them. We need both parts in the final commit message.

Quoted 14 lines

If we simply move the set_node_memory_tier() from memory_tier_init() to late_initcall(), it will result in HMAT not registering the mt_adistance_algorithm callback function, because set_node_memory_tier() is not performed during the memory tiering initialization phase, leading to a lack of correct default_dram information.
Therefore, we introduced a nodemask to pass the information of the default DRAM nodes. The reason for not choosing to reuse default_dram_type->nodes is that it is not clean enough. So in the end, we use a __initdata variable, which is a variable that is released once initialization is complete, including both CPU and memory nodes for HMAT to iterate through.

Quoted 3 lines

Besides, since default_dram_type may be checked/used during the initialization process of HMAT and drivers, it is better to keep the allocation of default_dram_type in memory_tier_init().

Why do we need it? IIRC, we have deleted its usage in hmat.c.

Quoted 29 lines

Signed-off-by Ho-Ren (Jack) Chuang <[email protected]>

---
 drivers/acpi/numa/hmat.c     |  5 +--
 include/linux/memory-tiers.h |  2 ++
 mm/memory-tiers.c            | 59 +++++++++++++++---------------------
 3 files changed, 28 insertions(+), 38 deletions(-)

drivers/acpi/numa/hmat.c +1 -4

index 2c8ccc91ebe6..a2f9e7a4b479 100644
@@ -940,10 +940,7 @@ static int hmat_set_default_dram_perf(void)
 	struct memory_target *target;
 	struct access_coordinate *attrs;
 
-	if (!default_dram_type)
-		return -EIO;
-
-	for_each_node_mask(nid, default_dram_type->nodes) {
+	for_each_node_mask(nid, default_dram_nodes) {
 		pxm = node_to_pxm(nid);
 		target = find_mem_target(pxm);
 		if (!target)

include/linux/memory-tiers.h +0 -0

index 0d70788558f4..fa61ad9c4d75 100644

include/linux/memory-tiers.h struct access_coordinate; · line 40

Quoted 3 lines

include/linux/memory-tiers.h +0 -0

@@ -38,6 +38,7 @@ struct access_coordinate;
 #ifdef CONFIG_NUMA
 extern bool numa_demotion_enabled;
 extern struct memory_dev_type *default_dram_type;

Can we remove the above line?

include/linux/memory-tiers.h struct access_coordinate; · line 41

Quoted 1 line

include/linux/memory-tiers.h +1 -0
+extern nodemask_t default_dram_nodes __initdata;

We don't need to use __initdata in variable declaration.

include/linux/memory-tiers.h static inline bool node_is_toptier(int node) · line 80

Quoted 8 lines

include/linux/memory-tiers.h +1 -0

 struct memory_dev_type *alloc_memory_type(int adistance);
 void put_memory_type(struct memory_dev_type *memtype);
 void init_node_memory_type(int node, struct memory_dev_type *default_type);

@@ -76,6 +77,7 @@ static inline bool node_is_toptier(int node)
 
 #define numa_demotion_enabled	false
 #define default_dram_type	NULL
+#define default_dram_nodes NODE_MASK_NONE

Should we use <tab> after "default_dram_nodes"?

Quoted 26 lines

include/linux/memory-tiers.h +0 -0

 /*
  * CONFIG_NUMA implementation returns non NULL error.
  */

mm/memory-tiers.c +3 -2

index 6632102bd5c9..a19a90c3ad36 100644
@@ -43,6 +43,7 @@ static LIST_HEAD(memory_tiers);
 static LIST_HEAD(default_memory_types);
 static struct node_memory_type_map node_memory_types[MAX_NUMNODES];
 struct memory_dev_type *default_dram_type;
+nodemask_t default_dram_nodes __initdata = NODE_MASK_NONE;
 
 static const struct bus_type memory_tier_subsys = {
 	.name = "memory_tiering",

@@ -671,28 +672,38 @@ EXPORT_SYMBOL_GPL(mt_put_memory_types);
 
 /*
  * This is invoked via `late_initcall()` to initialize memory tiers for
- * CPU-less memory nodes after driver initialization, which is
- * expected to provide `adistance` algorithms.
+ * memory nodes, both with and without CPUs. After the initialization of
+ * firmware and devices, adistance algorithms are expected to be provided.
  */
 static int __init memory_tier_late_init(void)
 {

mm/memory-tiers.c EXPORT_SYMBOL_GPL(mt_put_memory_types); · line 689

Quoted 10 lines

mm/memory-tiers.c +7 -0

 	int nid;
+	struct memory_tier *memtier;
 
+	get_online_mems();
 	guard(mutex)(&memory_tier_lock);
+	/*
+	 * Look at all the existing and uninitialized N_MEMORY nodes and
+	 * add them to default memory tier or to a tier if we already have
+	 * memory types assigned.
+	 */

If the memory type of the node has been assigned, we will skip it in the following code. So, I think that we need to revise the comments.

Quoted 69 lines

mm/memory-tiers.c +12 ~~-28~~

 	for_each_node_state(nid, N_MEMORY) {
 		/*
-		 * Some device drivers may have initialized memory tiers
-		 * between `memory_tier_init()` and `memory_tier_late_init()`,
-		 * potentially bringing online memory nodes and
-		 * configuring memory tiers. Exclude them here.
+		 * Some device drivers may have initialized
+		 * memory tiers, potentially bringing memory nodes
+		 * online and configuring memory tiers.
+		 * Exclude them here.
 		 */
 		if (node_memory_types[nid].memtype)
 			continue;
 
-		set_node_memory_tier(nid);
+		memtier = set_node_memory_tier(nid);
+		if (IS_ERR(memtier))
+			/* Continue with memtiers we are able to setup. */
+			break;
 	}
-
 	establish_demotion_targets();
+	put_online_mems();
 
 	return 0;
 }

@@ -875,8 +886,7 @@ static int __meminit memtier_hotplug_callback(struct notifier_block *self,
 
 static int __init memory_tier_init(void)
 {
-	int ret, node;
-	struct memory_tier *memtier;
+	int ret;
 
 	ret = subsys_virtual_register(&memory_tier_subsys, NULL);
 	if (ret)

@@ -887,7 +897,8 @@ static int __init memory_tier_init(void)
 				GFP_KERNEL);
 	WARN_ON(!node_demotion);
 #endif
-	mutex_lock(&memory_tier_lock);
+
+	guard(mutex)(&memory_tier_lock);
 	/*
 	 * For now we can have 4 faster memory tiers with smaller adistance
 	 * than default DRAM tier.

@@ -897,29 +908,9 @@ static int __init memory_tier_init(void)
 	if (IS_ERR(default_dram_type))
 		panic("%s() failed to allocate default DRAM tier\n", __func__);
 
-	/*
-	 * Look at all the existing N_MEMORY nodes and add them to
-	 * default memory tier or to a tier if we already have memory
-	 * types assigned.
-	 */
-	for_each_node_state(node, N_MEMORY) {
-		if (!node_state(node, N_CPU))
-			/*
-			 * Defer memory tier initialization on
-			 * CPUless numa nodes. These will be initialized
-			 * after firmware and devices are initialized.
-			 */
-			continue;
-
-		memtier = set_node_memory_tier(node);
-		if (IS_ERR(memtier))
-			/*
-			 * Continue with memtiers we are able to setup
-			 */

mm/memory-tiers.c static int __init memory_tier_init(void) · line 916

Quoted 10 lines

mm/memory-tiers.c +3 -4

-			break;
-	}
-	establish_demotion_targets();
-	mutex_unlock(&memory_tier_lock);
+	/* Record nodes with memory and CPU to set default DRAM performance. */
+	nodes_and(default_dram_nodes, node_states[N_MEMORY],
+		  node_states[N_CPU]);
 
 	hotplug_memory_notifier(memtier_hotplug_callback, MEMTIER_HOTPLUG_PRI);
 	return 0;

--
Best Regards,
Huang, Ying

Ho-Ren (Jack) Chuang

author

2024-07-02 05:37 UTC #3 lore raw

Hi Huang, Ying,

Thanks for your feedback and helpful suggestions. Replies inlined.

Quoted 5 lines · Ying

Hi, Jack,
"Ho-Ren (Jack) Chuang" <[email protected]> writes:

Quoted 8 lines

I suggest you to merge the [0/1] with the change log here. [0/1]
describes why do we need the patch. The below text describes some
details. Just don't use "---" to separate them. We need both parts in
the final commit message.

Sounds good! I will merge them into 1 patch in the v3.

Quoted 25 lines

Quoted 25 lines
If we simply move the set_node_memory_tier() from memory_tier_init()
to late_initcall(), it will result in HMAT not registering
the mt_adistance_algorithm callback function, because
set_node_memory_tier() is not performed during the memory tiering
initialization phase, leading to a lack of correct default_dram
information.
Therefore, we introduced a nodemask to pass the information of the
default DRAM nodes. The reason for not choosing to reuse
default_dram_type->nodes is that it is not clean enough. So in the end,
we use a __initdata variable, which is a variable that is released once
initialization is complete, including both CPU and memory nodes for HMAT
to iterate through.

Quoted 9 lines

Quoted 6 lines
Besides, since default_dram_type may be checked/used during the
initialization process of HMAT and drivers, it is better to keep the
allocation of default_dram_type in memory_tier_init().
Why do we need it? IIRC, we have deleted its usage in hmat.c.

Although default_dram_type is still used in set_node_memory_tier(), I can totally remove this description to remove confusion.

Quoted 57 lines

Quoted 57 lines
Signed-off-by Ho-Ren (Jack) Chuang <[email protected]>
Suggested-by Jonathan Cameron <[email protected]>
---
 drivers/acpi/numa/hmat.c | 5 +--
 include/linux/memory-tiers.h | 2 ++
 mm/memory-tiers.c | 59 +++++++++++++++---------------------
 3 files changed, 28 insertions(+), 38 deletions(-)
diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index 2c8ccc91ebe6..a2f9e7a4b479 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -940,10 +940,7 @@ static int hmat_set_default_dram_perf(void)
struct memory_target *target;
struct access_coordinate *attrs;
- if (!default_dram_type)
- return -EIO;
-
- for_each_node_mask(nid, default_dram_type->nodes) {
+ for_each_node_mask(nid, default_dram_nodes) {
pxm = node_to_pxm(nid);
target = find_mem_target(pxm);
if (!target)
diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h
index 0d70788558f4..fa61ad9c4d75 100644
--- a/include/linux/memory-tiers.h
+++ b/include/linux/memory-tiers.h
@@ -38,6 +38,7 @@ struct access_coordinate;

Quoted 9 lines

Quoted 6 lines
#ifdef CONFIG_NUMA
extern bool numa_demotion_enabled;
extern struct memory_dev_type *default_dram_type;
Can we remove the above line?

Yes, you are right. Good catch, thanks! Will remove it in the v3.

Quoted 6 lines

Quoted 3 lines
+extern nodemask_t default_dram_nodes __initdata;
We don't need to use __initdata in variable declaration.

Thank you for your guidance! Will remove __initdata it in the v3.

Quoted 11 lines

Quoted 11 lines
struct memory_dev_type *alloc_memory_type(int adistance);
void put_memory_type(struct memory_dev_type *memtype);
void init_node_memory_type(int node, struct memory_dev_type *default_type);
@@ -76,6 +77,7 @@ static inline bool node_is_toptier(int node)

Quoted 9 lines

Quoted 6 lines
#define numa_demotion_enabled false
#define default_dram_type NULL
+#define default_dram_nodes NODE_MASK_NONE
Should we use <tab> after "default_dram_nodes"?

Yes, thanks for the reminder. Will fix it in the v3.

Quoted 69 lines

Quoted 69 lines
/*
* CONFIG_NUMA implementation returns non NULL error.
*/
diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c
index 6632102bd5c9..a19a90c3ad36 100644
--- a/mm/memory-tiers.c
+++ b/mm/memory-tiers.c
@@ -43,6 +43,7 @@ static LIST_HEAD(memory_tiers);
static LIST_HEAD(default_memory_types);
static struct node_memory_type_map node_memory_types[MAX_NUMNODES];
struct memory_dev_type *default_dram_type;
+nodemask_t default_dram_nodes __initdata = NODE_MASK_NONE;
static const struct bus_type memory_tier_subsys = {
.name = "memory_tiering",
@@ -671,28 +672,38 @@ EXPORT_SYMBOL_GPL(mt_put_memory_types);
/*
* This is invoked via late_initcall() to initialize memory tiers for
- * CPU-less memory nodes after driver initialization, which is
- * expected to provide adistance algorithms.
+ * memory nodes, both with and without CPUs. After the initialization of
+ * firmware and devices, adistance algorithms are expected to be provided.
*/
static int __init memory_tier_late_init(void)
{
int nid;
+ struct memory_tier *memtier;
+ get_online_mems();
guard(mutex)(&memory_tier_lock);
+ /*
+ * Look at all the existing and uninitialized N_MEMORY nodes and
+ * add them to default memory tier or to a tier if we already have

Quoted 9 lines

Quoted 4 lines
+ * memory types assigned.
+ */
If the memory type of the node has been assigned, we will skip it in the
following code. So, I think that we need to revise the comments.

You are right, the new version in the v3 will be:
/* Assign each uninitialized N_MEMORY node to a memory tier. */

Quoted 157 lines

Quoted 157 lines
for_each_node_state(nid, N_MEMORY) {
/*
- * Some device drivers may have initialized memory tiers
- * between memory_tier_init() and memory_tier_late_init(),
- * potentially bringing online memory nodes and
- * configuring memory tiers. Exclude them here.
+ * Some device drivers may have initialized
+ * memory tiers, potentially bringing memory nodes
+ * online and configuring memory tiers.
+ * Exclude them here.
*/
if (node_memory_types[nid].memtype)
continue;
- set_node_memory_tier(nid);
+ memtier = set_node_memory_tier(nid);
+ if (IS_ERR(memtier))
+ /* Continue with memtiers we are able to setup. */
+ break;
}
-
establish_demotion_targets();
+ put_online_mems();
return 0;
}
@@ -875,8 +886,7 @@ static int __meminit memtier_hotplug_callback(struct notifier_block *self,
static int __init memory_tier_init(void)
{
- int ret, node;
- struct memory_tier *memtier;
+ int ret;
ret = subsys_virtual_register(&memory_tier_subsys, NULL);
if (ret)
@@ -887,7 +897,8 @@ static int __init memory_tier_init(void)
GFP_KERNEL);
WARN_ON(!node_demotion);
#endif
- mutex_lock(&memory_tier_lock);
+
+ guard(mutex)(&memory_tier_lock);
/*
* For now we can have 4 faster memory tiers with smaller adistance
* than default DRAM tier.
@@ -897,29 +908,9 @@ static int __init memory_tier_init(void)
if (IS_ERR(default_dram_type))
panic("%s() failed to allocate default DRAM tier\n", __func__);
- /*
- * Look at all the existing N_MEMORY nodes and add them to
- * default memory tier or to a tier if we already have memory
- * types assigned.
- */
- for_each_node_state(node, N_MEMORY) {
- if (!node_state(node, N_CPU))
- /*
- * Defer memory tier initialization on
- * CPUless numa nodes. These will be initialized
- * after firmware and devices are initialized.
- */
- continue;
-
- memtier = set_node_memory_tier(node);
- if (IS_ERR(memtier))
- /*
- * Continue with memtiers we are able to setup
- */
- break;
- }
- establish_demotion_targets();
- mutex_unlock(&memory_tier_lock);
+ /* Record nodes with memory and CPU to set default DRAM performance. */
+ nodes_and(default_dram_nodes, node_states[N_MEMORY],
+ node_states[N_CPU]);
hotplug_memory_notifier(memtier_hotplug_callback, MEMTIER_HOTPLUG_PRI);

Quoted 9 lines

Quoted 2 lines
return 0;
--
Best Regards,
Huang, Ying

--
Best Regards,
Ho-Ren (Jack) Chuang

Jonathan Cameron

2024-07-02 13:25 UTC #4 lore raw

In reply to Ho-Ren (Jack) Chuang

Quoted 31 lines · On Fri, 28 Jun 2024 06:09:23 +0000 "Ho-Ren (Jack) Chuang"…

If we simply move the set_node_memory_tier() from memory_tier_init() to late_initcall(), it will result in HMAT not registering the mt_adistance_algorithm callback function, because set_node_memory_tier() is not performed during the memory tiering initialization phase, leading to a lack of correct default_dram information.
Therefore, we introduced a nodemask to pass the information of the default DRAM nodes. The reason for not choosing to reuse default_dram_type->nodes is that it is not clean enough. So in the end, we use a __initdata variable, which is a variable that is released once initialization is complete, including both CPU and memory nodes for HMAT to iterate through.
Besides, since default_dram_type may be checked/used during the initialization process of HMAT and drivers, it is better to keep the allocation of default_dram_type in memory_tier_init().
Signed-off-by Ho-Ren (Jack) Chuang <[email protected]>
Suggested-by Jonathan Cameron <[email protected]>
---
 drivers/acpi/numa/hmat.c     |  5 +--
 include/linux/memory-tiers.h |  2 ++
 mm/memory-tiers.c            | 59 +++++++++++++++---------------------
 3 files changed, 28 insertions(+), 38 deletions(-)
drivers/acpi/numa/hmat.c +0 -0
index 2c8ccc91ebe6..a2f9e7a4b479 100644

drivers/acpi/numa/hmat.c static int hmat_set_default_dram_perf(void) · line 943

Quoted 8 lines

drivers/acpi/numa/hmat.c +1 -4

@@ -940,10 +940,7 @@ static int hmat_set_default_dram_perf(void)
 	struct memory_target *target;
 	struct access_coordinate *attrs;
 
-	if (!default_dram_type)
-		return -EIO;
-
-	for_each_node_mask(nid, default_dram_type->nodes) {
+	for_each_node_mask(nid, default_dram_nodes) {

As below. Do we care if the combination of RAM + CPU wasn't true
earlier and is true by this point? If not, why not just
compute the node mask in here and not store it.

Quoted 64 lines

drivers/acpi/numa/hmat.c +0 -0

 		pxm = node_to_pxm(nid);
 		target = find_mem_target(pxm);
 		if (!target)

include/linux/memory-tiers.h +2 -0

index 0d70788558f4..fa61ad9c4d75 100644
@@ -38,6 +38,7 @@ struct access_coordinate;
 #ifdef CONFIG_NUMA
 extern bool numa_demotion_enabled;
 extern struct memory_dev_type *default_dram_type;
+extern nodemask_t default_dram_nodes __initdata;
 struct memory_dev_type *alloc_memory_type(int adistance);
 void put_memory_type(struct memory_dev_type *memtype);
 void init_node_memory_type(int node, struct memory_dev_type *default_type);

@@ -76,6 +77,7 @@ static inline bool node_is_toptier(int node)
 
 #define numa_demotion_enabled	false
 #define default_dram_type	NULL
+#define default_dram_nodes NODE_MASK_NONE
 /*
  * CONFIG_NUMA implementation returns non NULL error.
  */

mm/memory-tiers.c +12 -6

index 6632102bd5c9..a19a90c3ad36 100644
@@ -43,6 +43,7 @@ static LIST_HEAD(memory_tiers);
 static LIST_HEAD(default_memory_types);
 static struct node_memory_type_map node_memory_types[MAX_NUMNODES];
 struct memory_dev_type *default_dram_type;
+nodemask_t default_dram_nodes __initdata = NODE_MASK_NONE;
 
 static const struct bus_type memory_tier_subsys = {
 	.name = "memory_tiering",

@@ -671,28 +672,38 @@ EXPORT_SYMBOL_GPL(mt_put_memory_types);
 
 /*
  * This is invoked via `late_initcall()` to initialize memory tiers for
- * CPU-less memory nodes after driver initialization, which is
- * expected to provide `adistance` algorithms.
+ * memory nodes, both with and without CPUs. After the initialization of
+ * firmware and devices, adistance algorithms are expected to be provided.
  */
 static int __init memory_tier_late_init(void)
 {
 	int nid;
+	struct memory_tier *memtier;
 
+	get_online_mems();
 	guard(mutex)(&memory_tier_lock);
+	/*
+	 * Look at all the existing and uninitialized N_MEMORY nodes and
+	 * add them to default memory tier or to a tier if we already have
+	 * memory types assigned.
+	 */
 	for_each_node_state(nid, N_MEMORY) {
 		/*
-		 * Some device drivers may have initialized memory tiers
-		 * between `memory_tier_init()` and `memory_tier_late_init()`,
-		 * potentially bringing online memory nodes and
-		 * configuring memory tiers. Exclude them here.
+		 * Some device drivers may have initialized
+		 * memory tiers, potentially bringing memory nodes

mm/memory-tiers.c EXPORT_SYMBOL_GPL(mt_put_memory_types); · line 702

Quoted 10 lines

mm/memory-tiers.c +5 -1

+		 * online and configuring memory tiers.
+		 * Exclude them here.
 		 */
 		if (node_memory_types[nid].memtype)
 			continue;
 
-		set_node_memory_tier(nid);
+		memtier = set_node_memory_tier(nid);
+		if (IS_ERR(memtier))
+			/* Continue with memtiers we are able to setup. */

Might later ones be possible if we just continued this loop?

mm/memory-tiers.c EXPORT_SYMBOL_GPL(mt_put_memory_types); · line 694

Quoted 3 lines

mm/memory-tiers.c +1 -1
+			break;
 	}
-

White space was harmless - I'd leave it there rather than adding noise to this diff.

Quoted 16 lines

mm/memory-tiers.c +2 -2

 	establish_demotion_targets();
+	put_online_mems();
 
 	return 0;
 }

@@ -875,8 +886,7 @@ static int __meminit memtier_hotplug_callback(struct notifier_block *self,
 
 static int __init memory_tier_init(void)
 {
-	int ret, node;
-	struct memory_tier *memtier;
+	int ret;
 
 	ret = subsys_virtual_register(&memory_tier_subsys, NULL);
 	if (ret)

mm/memory-tiers.c static int __init memory_tier_init(void) · line 901

Quoted 6 lines

mm/memory-tiers.c +2 -1

@@ -887,7 +897,8 @@ static int __init memory_tier_init(void)
 				GFP_KERNEL);
 	WARN_ON(!node_demotion);
 #endif
-	mutex_lock(&memory_tier_lock);
+
+	guard(mutex)(&memory_tier_lock);

If this was safe to do without the rest of the change (I think so) then better to pull that out as a trivial precursor so less noise in here.

Quoted 23 lines

mm/memory-tiers.c +0 ~~-16~~

 	/*
 	 * For now we can have 4 faster memory tiers with smaller adistance
 	 * than default DRAM tier.

@@ -897,29 +908,9 @@ static int __init memory_tier_init(void)
 	if (IS_ERR(default_dram_type))
 		panic("%s() failed to allocate default DRAM tier\n", __func__);
 
-	/*
-	 * Look at all the existing N_MEMORY nodes and add them to
-	 * default memory tier or to a tier if we already have memory
-	 * types assigned.
-	 */
-	for_each_node_state(node, N_MEMORY) {
-		if (!node_state(node, N_CPU))
-			/*
-			 * Defer memory tier initialization on
-			 * CPUless numa nodes. These will be initialized
-			 * after firmware and devices are initialized.
-			 */
-			continue;
-
-		memtier = set_node_memory_tier(node);
-		if (IS_ERR(memtier))

mm/memory-tiers.c static int __init memory_tier_init(void) · line 913

Quoted 10 lines

mm/memory-tiers.c +3 -7

-			/*
-			 * Continue with memtiers we are able to setup
-			 */
-			break;
-	}
-	establish_demotion_targets();
-	mutex_unlock(&memory_tier_lock);
+	/* Record nodes with memory and CPU to set default DRAM performance. */
+	nodes_and(default_dram_nodes, node_states[N_MEMORY],
+		  node_states[N_CPU]);

There are systems where (for various esoteric reasons, such as describing an association with some other memory that isn't DRAM where the granularity doesn't match) the CPU nodes contain no DRAM but rather it's one node away. Handling that can be a job for another day though.

Why does this need to be computed here? Why not do it in hmat_set_default_dram_perf? Doesn't seem to be used anywhere else.

Quoted 3 lines

mm/memory-tiers.c +0 -0

 	hotplug_memory_notifier(memtier_hotplug_callback, MEMTIER_HOTPLUG_PRI);
 	return 0;

Ho-Ren (Jack) Chuang

author

2024-07-03 08:33 UTC #5 lore raw

Hi Jonathan,

I appreciate your feedback and valuable suggestions. Replies inlined.

Quoted 82 lines · Jonathan Cameron

On Fri, 28 Jun 2024 06:09:23 +0000
Quoted 77 lines · Ho-Ren (Jack) Chuang
If we simply move the set_node_memory_tier() from memory_tier_init()
to late_initcall(), it will result in HMAT not registering
the mt_adistance_algorithm callback function, because
set_node_memory_tier() is not performed during the memory tiering
initialization phase, leading to a lack of correct default_dram
information.
Therefore, we introduced a nodemask to pass the information of the
default DRAM nodes. The reason for not choosing to reuse
default_dram_type->nodes is that it is not clean enough. So in the end,
we use a __initdata variable, which is a variable that is released once
initialization is complete, including both CPU and memory nodes for HMAT
to iterate through.
Besides, since default_dram_type may be checked/used during the
initialization process of HMAT and drivers, it is better to keep the
allocation of default_dram_type in memory_tier_init().
Signed-off-by Ho-Ren (Jack) Chuang <[email protected]>
Suggested-by Jonathan Cameron <[email protected]>
---
 drivers/acpi/numa/hmat.c | 5 +--
 include/linux/memory-tiers.h | 2 ++
 mm/memory-tiers.c | 59 +++++++++++++++---------------------
 3 files changed, 28 insertions(+), 38 deletions(-)
diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index 2c8ccc91ebe6..a2f9e7a4b479 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -940,10 +940,7 @@ static int hmat_set_default_dram_perf(void)
struct memory_target *target;
struct access_coordinate *attrs;
- if (!default_dram_type)
- return -EIO;
-
- for_each_node_mask(nid, default_dram_type->nodes) {

Quoted 9 lines

Quoted 2 lines
+ for_each_node_mask(nid, default_dram_nodes) {
As below. Do we care if the combination of RAM + CPU wasn't true
earlier and is true by this point? If not, why not just
compute the node mask in here and not store it.

It makes sense to me. I think we can move the computation to here and remove the global node mask.

Quoted 143 lines

Quoted 143 lines
pxm = node_to_pxm(nid);
target = find_mem_target(pxm);
if (!target)
diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h
index 0d70788558f4..fa61ad9c4d75 100644
--- a/include/linux/memory-tiers.h
+++ b/include/linux/memory-tiers.h
@@ -38,6 +38,7 @@ struct access_coordinate;
#ifdef CONFIG_NUMA
extern bool numa_demotion_enabled;
extern struct memory_dev_type *default_dram_type;
+extern nodemask_t default_dram_nodes __initdata;
struct memory_dev_type *alloc_memory_type(int adistance);
void put_memory_type(struct memory_dev_type *memtype);
void init_node_memory_type(int node, struct memory_dev_type *default_type);
@@ -76,6 +77,7 @@ static inline bool node_is_toptier(int node)
#define numa_demotion_enabled false
#define default_dram_type NULL
+#define default_dram_nodes NODE_MASK_NONE
/*
* CONFIG_NUMA implementation returns non NULL error.
*/
diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c
index 6632102bd5c9..a19a90c3ad36 100644
--- a/mm/memory-tiers.c
+++ b/mm/memory-tiers.c
@@ -43,6 +43,7 @@ static LIST_HEAD(memory_tiers);
static LIST_HEAD(default_memory_types);
static struct node_memory_type_map node_memory_types[MAX_NUMNODES];
struct memory_dev_type *default_dram_type;
+nodemask_t default_dram_nodes __initdata = NODE_MASK_NONE;
static const struct bus_type memory_tier_subsys = {
.name = "memory_tiering",
@@ -671,28 +672,38 @@ EXPORT_SYMBOL_GPL(mt_put_memory_types);
/*
* This is invoked via late_initcall() to initialize memory tiers for
- * CPU-less memory nodes after driver initialization, which is
- * expected to provide adistance algorithms.
+ * memory nodes, both with and without CPUs. After the initialization of
+ * firmware and devices, adistance algorithms are expected to be provided.
*/
static int __init memory_tier_late_init(void)
{
int nid;
+ struct memory_tier *memtier;
+ get_online_mems();
guard(mutex)(&memory_tier_lock);
+ /*
+ * Look at all the existing and uninitialized N_MEMORY nodes and
+ * add them to default memory tier or to a tier if we already have
+ * memory types assigned.
+ */
for_each_node_state(nid, N_MEMORY) {
/*
- * Some device drivers may have initialized memory tiers
- * between memory_tier_init() and memory_tier_late_init(),
- * potentially bringing online memory nodes and
- * configuring memory tiers. Exclude them here.
+ * Some device drivers may have initialized
+ * memory tiers, potentially bringing memory nodes
+ * online and configuring memory tiers.
+ * Exclude them here.
*/
if (node_memory_types[nid].memtype)
continue;
- set_node_memory_tier(nid);

Quoted 9 lines

Quoted 6 lines
+ memtier = set_node_memory_tier(nid);
+ if (IS_ERR(memtier))
+ /* Continue with memtiers we are able to setup. */
Might later ones be possible if we just continued this loop?

I agree with you that theoretically, it’s possible for later attempts to succeed. I also agree that there is no harm in iterating through all possibilities. Therefore, we can do a continue here.

Since it's legacy code.
I would also like to hear Huang, Ying’s thoughts about this.

Quoted 10 lines

Quoted 7 lines
+ break;
}
-
White space was harmless - I'd leave it there rather than adding noise to this diff.

Thanks! Got it. I will roll it back in the v3.

Quoted 43 lines

Quoted 43 lines
establish_demotion_targets();
+ put_online_mems();
return 0;
}
@@ -875,8 +886,7 @@ static int __meminit memtier_hotplug_callback(struct notifier_block *self,
static int __init memory_tier_init(void)
{
- int ret, node;
- struct memory_tier *memtier;
+ int ret;
ret = subsys_virtual_register(&memory_tier_subsys, NULL);
if (ret)
@@ -887,7 +897,8 @@ static int __init memory_tier_init(void)
GFP_KERNEL);
WARN_ON(!node_demotion);
#endif
- mutex_lock(&memory_tier_lock);
+

Quoted 9 lines

Quoted 2 lines
+ guard(mutex)(&memory_tier_lock);
If this was safe to do without the rest of the change (I think so)
then better to pull that out as a trivial precursor so less noise
in here.

Do you mean instead of using guard(mutex)(),
use mutex_lock() as it was? or?

Quoted 68 lines

Quoted 59 lines
/*
* For now we can have 4 faster memory tiers with smaller adistance
* than default DRAM tier.
@@ -897,29 +908,9 @@ static int __init memory_tier_init(void)
if (IS_ERR(default_dram_type))
panic("%s() failed to allocate default DRAM tier\n", __func__);
- /*
- * Look at all the existing N_MEMORY nodes and add them to
- * default memory tier or to a tier if we already have memory
- * types assigned.
- */
- for_each_node_state(node, N_MEMORY) {
- if (!node_state(node, N_CPU))
- /*
- * Defer memory tier initialization on
- * CPUless numa nodes. These will be initialized
- * after firmware and devices are initialized.
- */
- continue;
-
- memtier = set_node_memory_tier(node);
- if (IS_ERR(memtier))
- /*
- * Continue with memtiers we are able to setup
- */
- break;
- }
- establish_demotion_targets();
Quoted 8 lines
- mutex_unlock(&memory_tier_lock);
+ /* Record nodes with memory and CPU to set default DRAM performance. */
+ nodes_and(default_dram_nodes, node_states[N_MEMORY],
+ node_states[N_CPU]);

Quoted 8 lines

There are systems where (for various esoteric reasons, such as describing an
association with some other memory that isn't DRAM where the granularity
doesn't match) the CPU nodes contain no DRAM but rather it's one node away.
Handling that can be a job for another day though.

Thank you for informing me of this situation.
Sounds like handling that also requires a mapping table between the CPU and the corresponding DRAM.

Quoted 4 lines

Why does this need to be computed here? Why not do it in
hmat_set_default_dram_perf? Doesn't seem to be used anywhere else.

Replied above.

Quoted 6 lines

Quoted 5 lines
hotplug_memory_notifier(memtier_hotplug_callback, MEMTIER_HOTPLUG_PRI);
return 0;

--
Best Regards,
Ho-Ren (Jack) Chuang

Jonathan Cameron

2024-07-04 17:08 UTC #6 lore raw

Hi,

Quoted 34 lines

Quoted 34 lines
Quoted 23 lines
static int __init memory_tier_init(void)
{
- int ret, node;
- struct memory_tier *memtier;
+ int ret;
ret = subsys_virtual_register(&memory_tier_subsys, NULL);
if (ret)
@@ -887,7 +897,8 @@ static int __init memory_tier_init(void)
GFP_KERNEL);
WARN_ON(!node_demotion);
Quoted 8 lines
#endif
- mutex_lock(&memory_tier_lock);
+
+ guard(mutex)(&memory_tier_lock);
If this was safe to do without the rest of the change (I think so)

Quoted 8 lines

Quoted 4 lines
then better to pull that out as a trivial precursor so less noise
in here.
Do you mean instead of using guard(mutex)(),
use mutex_lock() as it was? or?

Code as here, but possibly pull the guard(mutex) part out as a patch 1 as it's an unrelated improvement to the rest of the set which would be in patch 2.

Not particularly important though as you've sent a v3 in the meantime and it's fine to have it in one patch.

Quoted 72 lines

Quoted 72 lines
Quoted 59 lines
/*
* For now we can have 4 faster memory tiers with smaller adistance
* than default DRAM tier.
@@ -897,29 +908,9 @@ static int __init memory_tier_init(void)
if (IS_ERR(default_dram_type))
panic("%s() failed to allocate default DRAM tier\n", __func__);
- /*
- * Look at all the existing N_MEMORY nodes and add them to
- * default memory tier or to a tier if we already have memory
- * types assigned.
- */
- for_each_node_state(node, N_MEMORY) {
- if (!node_state(node, N_CPU))
- /*
- * Defer memory tier initialization on
- * CPUless numa nodes. These will be initialized
- * after firmware and devices are initialized.
- */
- continue;
-
- memtier = set_node_memory_tier(node);
- if (IS_ERR(memtier))
- /*
- * Continue with memtiers we are able to setup
- */
- break;
- }
- establish_demotion_targets();
Quoted 8 lines
- mutex_unlock(&memory_tier_lock);
+ /* Record nodes with memory and CPU to set default DRAM performance. */
+ nodes_and(default_dram_nodes, node_states[N_MEMORY],
+ node_states[N_CPU]);
There are systems where (for various esoteric reasons, such as describing an
association with some other memory that isn't DRAM where the granularity

Quoted 8 lines

Quoted 4 lines
doesn't match) the CPU nodes contain no DRAM but rather it's one node away.
Handling that can be a job for another day though.
Thank you for informing me of this situation.
Sounds like handling that also requires a mapping table between the CPU and the corresponding DRAM.

I've not yet looked at how it interacts with this, but from an ACPI point of view it's just 'near' in SLIT and HMAT. The nearest thing to a description is Memory Proximity Domain Attributes structures in HMAT. That allows you to describe the location of the memory controller, but in this type of system there may be a many to 1 mapping (interleaving across memory controllers in some CPU only nodes) for example.

Anyhow, guess I need to spin up some emulated machines and see what breaks :)

Jonathan

Huang, Ying

2024-07-03 08:53 UTC #7 lore raw

In reply to Jonathan Cameron

Quoted 2 lines · Jonathan Cameron

On Fri, 28 Jun 2024 06:09:23 +0000
"Ho-Ren (Jack) Chuang" <[email protected]> wrote:

[snip]

Quoted 61 lines

Quoted 11 lines

mm/memory-tiers.c +1 -2

@@ -875,8 +886,7 @@ static int __meminit memtier_hotplug_callback(struct notifier_block *self,
 
 static int __init memory_tier_init(void)
 {
-	int ret, node;
-	struct memory_tier *memtier;
+	int ret;
 
 	ret = subsys_virtual_register(&memory_tier_subsys, NULL);
 	if (ret)

mm/memory-tiers.c static int __init memory_tier_init(void) · line 901

Quoted 6 lines

mm/memory-tiers.c +2 -1

@@ -887,7 +897,8 @@ static int __init memory_tier_init(void)
 				GFP_KERNEL);
 	WARN_ON(!node_demotion);
 #endif
-	mutex_lock(&memory_tier_lock);
+
+	guard(mutex)(&memory_tier_lock);

If this was safe to do without the rest of the change (I think so) then better to pull that out as a trivial precursor so less noise in here.

Quoted 23 lines

mm/memory-tiers.c +0 ~~-16~~

 	/*
 	 * For now we can have 4 faster memory tiers with smaller adistance
 	 * than default DRAM tier.

@@ -897,29 +908,9 @@ static int __init memory_tier_init(void)
 	if (IS_ERR(default_dram_type))
 		panic("%s() failed to allocate default DRAM tier\n", __func__);
 
-	/*
-	 * Look at all the existing N_MEMORY nodes and add them to
-	 * default memory tier or to a tier if we already have memory
-	 * types assigned.
-	 */
-	for_each_node_state(node, N_MEMORY) {
-		if (!node_state(node, N_CPU))
-			/*
-			 * Defer memory tier initialization on
-			 * CPUless numa nodes. These will be initialized
-			 * after firmware and devices are initialized.
-			 */
-			continue;
-
-		memtier = set_node_memory_tier(node);
-		if (IS_ERR(memtier))

mm/memory-tiers.c static int __init memory_tier_init(void) · line 913

Quoted 10 lines

mm/memory-tiers.c +3 -7

-			/*
-			 * Continue with memtiers we are able to setup
-			 */
-			break;
-	}
-	establish_demotion_targets();
-	mutex_unlock(&memory_tier_lock);
+	/* Record nodes with memory and CPU to set default DRAM performance. */
+	nodes_and(default_dram_nodes, node_states[N_MEMORY],
+		  node_states[N_CPU]);

Quoted 2 lines

Why does this need to be computed here? Why not do it in hmat_set_default_dram_perf? Doesn't seem to be used anywhere else.

IMO, which node is default dram node is a general concept instead of HMAT specific. So, I think that it's better to decide that in the general code (memory-tiers.c).

Quoted 3 lines

mm/memory-tiers.c static int __init memory_tier_init(void) · line 916
Quoted 3 lines
mm/memory-tiers.c +0 -0
 	hotplug_memory_notifier(memtier_hotplug_callback, MEMTIER_HOTPLUG_PRI);
 	return 0;

--
Best Regards,
Huang, Ying

Jonathan Cameron

2024-07-04 17:09 UTC #8 lore raw

Quoted 68 lines · Ying

Quoted 2 lines · Jonathan Cameron
On Fri, 28 Jun 2024 06:09:23 +0000
"Ho-Ren (Jack) Chuang" <[email protected]> wrote:
[snip]
Quoted 61 lines
Quoted 11 lines
mm/memory-tiers.c +1 -2
@@ -875,8 +886,7 @@ static int __meminit memtier_hotplug_callback(struct notifier_block *self,
 
 static int __init memory_tier_init(void)
 {
-	int ret, node;
-	struct memory_tier *memtier;
+	int ret;
 
 	ret = subsys_virtual_register(&memory_tier_subsys, NULL);
 	if (ret)
mm/memory-tiers.c static int __init memory_tier_init(void) · line 901
Quoted 6 lines
mm/memory-tiers.c +2 -1
@@ -887,7 +897,8 @@ static int __init memory_tier_init(void)
 				GFP_KERNEL);
 	WARN_ON(!node_demotion);
 #endif
-	mutex_lock(&memory_tier_lock);
+
+	guard(mutex)(&memory_tier_lock);  
If this was safe to do without the rest of the change (I think so) then better to pull that out as a trivial precursor so less noise in here.
Quoted 23 lines
mm/memory-tiers.c +0 ~~-16~~
 	/*
 	 * For now we can have 4 faster memory tiers with smaller adistance
 	 * than default DRAM tier.
@@ -897,29 +908,9 @@ static int __init memory_tier_init(void)
 	if (IS_ERR(default_dram_type))
 		panic("%s() failed to allocate default DRAM tier\n", __func__);
 
-	/*
-	 * Look at all the existing N_MEMORY nodes and add them to
-	 * default memory tier or to a tier if we already have memory
-	 * types assigned.
-	 */
-	for_each_node_state(node, N_MEMORY) {
-		if (!node_state(node, N_CPU))
-			/*
-			 * Defer memory tier initialization on
-			 * CPUless numa nodes. These will be initialized
-			 * after firmware and devices are initialized.
-			 */
-			continue;
-
-		memtier = set_node_memory_tier(node);
-		if (IS_ERR(memtier))
mm/memory-tiers.c static int __init memory_tier_init(void) · line 913
Quoted 10 lines
mm/memory-tiers.c +3 -7
-			/*
-			 * Continue with memtiers we are able to setup
-			 */
-			break;
-	}
-	establish_demotion_targets();
-	mutex_unlock(&memory_tier_lock);
+	/* Record nodes with memory and CPU to set default DRAM performance. */
+	nodes_and(default_dram_nodes, node_states[N_MEMORY],
+		  node_states[N_CPU]);  
There are systems where (for various esoteric reasons, such as describing an association with some other memory that isn't DRAM where the granularity doesn't match) the CPU nodes contain no DRAM but rather it's one node away. Handling that can be a job for another day though.

Quoted 6 lines

Quoted 2 lines
Why does this need to be computed here? Why not do it in hmat_set_default_dram_perf? Doesn't seem to be used anywhere else.
IMO, which node is default dram node is a general concept instead of HMAT specific. So, I think that it's better to decide that in the general code (memory-tiers.c).

That makes sense given I'd imagine this will spread to other firmware types in time.

Thanks,

Jonathan

Quoted 9 lines

Quoted 4 lines
mm/memory-tiers.c static int __init memory_tier_init(void) · line 916
Quoted 4 lines
mm/memory-tiers.c +0 -0
 	hotplug_memory_notifier(memtier_hotplug_callback, MEMTIER_HOTPLUG_PRI);
 	return 0;  
--
Best Regards,
Huang, Ying