| Message ID | 20240312061729.1997111-2-horenchuang@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improved Memory Tier Creation for CPUless NUMA Nodes | expand |
"Ho-Ren (Jack) Chuang" <horenchuang@bytedance.com> writes: > The current implementation treats emulated memory devices, such as > CXL1.1 type3 memory, as normal DRAM when they are emulated as normal memory > (E820_TYPE_RAM). However, these emulated devices have different > characteristics than traditional DRAM, making it important to > distinguish them. Thus, we modify the tiered memory initialization process > to introduce a delay specifically for CPUless NUMA nodes. This delay > ensures that the memory tier initialization for these nodes is deferred > until HMAT information is obtained during the boot process. Finally, > demotion tables are recalculated at the end. > > * Abstract common functions into `find_alloc_memory_type()` We should move kmem_put_memory_types() (renamed to mt_put_memory_types()?) too. This can be put in a separate patch. > Since different memory devices require finding or allocating a memory type, > these common steps are abstracted into a single function, > `find_alloc_memory_type()`, enhancing code scalability and conciseness. > > * Handle cases where there is no HMAT when creating memory tiers > There is a scenario where a CPUless node does not provide HMAT information. > If no HMAT is specified, it falls back to using the default DRAM tier. > > * Change adist calculation code to use another new lock, mt_perf_lock. > In the current implementation, iterating through CPUlist nodes requires > holding the `memory_tier_lock`. However, `mt_calc_adistance()` will end up > trying to acquire the same lock, leading to a potential deadlock. > Therefore, we propose introducing a standalone `mt_perf_lock` to protect > `default_dram_perf`. This approach not only avoids deadlock but also > prevents holding a large lock simultaneously. > > Signed-off-by: Ho-Ren (Jack) Chuang <horenchuang@bytedance.com> > Signed-off-by: Hao Xiang <hao.xiang@bytedance.com> > --- > drivers/acpi/numa/hmat.c | 11 ++++++ > drivers/dax/kmem.c | 13 +------ > include/linux/acpi.h | 6 ++++ > include/linux/memory-tiers.h | 8 +++++ > mm/memory-tiers.c | 70 +++++++++++++++++++++++++++++++++--- > 5 files changed, 92 insertions(+), 16 deletions(-) > > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c > index d6b85f0f6082..28812ec2c793 100644 > --- a/drivers/acpi/numa/hmat.c > +++ b/drivers/acpi/numa/hmat.c > @@ -38,6 +38,8 @@ static LIST_HEAD(targets); > static LIST_HEAD(initiators); > static LIST_HEAD(localities); > > +static LIST_HEAD(hmat_memory_types); > + HMAT isn't a device driver for some memory devices. So I don't think we should manage memory types in HMAT. Instead, if the memory_type of a node isn't set by the driver, we should manage it in memory-tier.c as fallback. > static DEFINE_MUTEX(target_lock); > > /* > @@ -149,6 +151,12 @@ int acpi_get_genport_coordinates(u32 uid, > } > EXPORT_SYMBOL_NS_GPL(acpi_get_genport_coordinates, CXL); > > +struct memory_dev_type *hmat_find_alloc_memory_type(int adist) > +{ > + return find_alloc_memory_type(adist, &hmat_memory_types); > +} > +EXPORT_SYMBOL_GPL(hmat_find_alloc_memory_type); > + > static __init void alloc_memory_initiator(unsigned int cpu_pxm) > { > struct memory_initiator *initiator; > @@ -1038,6 +1046,9 @@ static __init int hmat_init(void) > if (!hmat_set_default_dram_perf()) > register_mt_adistance_algorithm(&hmat_adist_nb); > > + /* Post-create CPUless memory tiers after getting HMAT info */ > + memory_tier_late_init(); > + This should be called in memory-tier.c via late_initcall(memory_tier_late_init); Then, we don't need hmat to call it. > return 0; > out_put: > hmat_free_structures(); > diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c > index 42ee360cf4e3..aee17ab59f4f 100644 > --- a/drivers/dax/kmem.c > +++ b/drivers/dax/kmem.c > @@ -55,21 +55,10 @@ static LIST_HEAD(kmem_memory_types); > > static struct memory_dev_type *kmem_find_alloc_memory_type(int adist) > { > - bool found = false; > struct memory_dev_type *mtype; > > mutex_lock(&kmem_memory_type_lock); > - list_for_each_entry(mtype, &kmem_memory_types, list) { > - if (mtype->adistance == adist) { > - found = true; > - break; > - } > - } > - if (!found) { > - mtype = alloc_memory_type(adist); > - if (!IS_ERR(mtype)) > - list_add(&mtype->list, &kmem_memory_types); > - } > + mtype = find_alloc_memory_type(adist, &kmem_memory_types); > mutex_unlock(&kmem_memory_type_lock); > > return mtype; > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index b7165e52b3c6..3f927ff01f02 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -434,12 +434,18 @@ int thermal_acpi_critical_trip_temp(struct acpi_device *adev, int *ret_temp); > > #ifdef CONFIG_ACPI_HMAT > int acpi_get_genport_coordinates(u32 uid, struct access_coordinate *coord); > +struct memory_dev_type *hmat_find_alloc_memory_type(int adist); > #else > static inline int acpi_get_genport_coordinates(u32 uid, > struct access_coordinate *coord) > { > return -EOPNOTSUPP; > } > + > +static inline struct memory_dev_type *hmat_find_alloc_memory_type(int adist) > +{ > + return NULL; > +} > #endif > > #ifdef CONFIG_ACPI_NUMA > diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h > index 69e781900082..4bc2596c5774 100644 > --- a/include/linux/memory-tiers.h > +++ b/include/linux/memory-tiers.h > @@ -48,6 +48,9 @@ int mt_calc_adistance(int node, int *adist); > int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, > const char *source); > int mt_perf_to_adistance(struct access_coordinate *perf, int *adist); > +struct memory_dev_type *find_alloc_memory_type(int adist, > + struct list_head *memory_types); > +void memory_tier_late_init(void); > #ifdef CONFIG_MIGRATION > int next_demotion_node(int node); > void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets); > @@ -136,5 +139,10 @@ static inline int mt_perf_to_adistance(struct access_coordinate *perf, int *adis > { > return -EIO; > } > + > +static inline void memory_tier_late_init(void) > +{ > + > +} > #endif /* CONFIG_NUMA */ > #endif /* _LINUX_MEMORY_TIERS_H */ > diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c > index 0537664620e5..79f748d60e6f 100644 > --- a/mm/memory-tiers.c > +++ b/mm/memory-tiers.c > @@ -6,6 +6,7 @@ > #include <linux/memory.h> > #include <linux/memory-tiers.h> > #include <linux/notifier.h> > +#include <linux/acpi.h> > > #include "internal.h" > > @@ -35,6 +36,7 @@ struct node_memory_type_map { > }; > > static DEFINE_MUTEX(memory_tier_lock); > +static DEFINE_MUTEX(mt_perf_lock); Please add comments about what it protects. And put it near the data structure it protects. > static LIST_HEAD(memory_tiers); > static struct node_memory_type_map node_memory_types[MAX_NUMNODES]; > struct memory_dev_type *default_dram_type; > @@ -623,6 +625,58 @@ void clear_node_memory_type(int node, struct memory_dev_type *memtype) > } > EXPORT_SYMBOL_GPL(clear_node_memory_type); > > +struct memory_dev_type *find_alloc_memory_type(int adist, struct list_head *memory_types) > +{ > + bool found = false; > + struct memory_dev_type *mtype; > + > + list_for_each_entry(mtype, memory_types, list) { > + if (mtype->adistance == adist) { > + found = true; > + break; > + } > + } > + if (!found) { > + mtype = alloc_memory_type(adist); > + if (!IS_ERR(mtype)) > + list_add(&mtype->list, memory_types); > + } > + > + return mtype; > +} > +EXPORT_SYMBOL_GPL(find_alloc_memory_type); > + > +static void memory_tier_late_create(int node) > +{ > + struct memory_dev_type *mtype = NULL; > + int adist = MEMTIER_ADISTANCE_DRAM; > + > + mt_calc_adistance(node, &adist); > + if (adist != MEMTIER_ADISTANCE_DRAM) { We can manage default_dram_type() via find_alloc_memory_type() too. And, if "node_memory_types[node].memtype == NULL", we can call mt_calc_adistance(node, &adist) and find_alloc_memory_type() in set_node_memory_tier(). Then, we can cover hotpluged memory node too. > + mtype = hmat_find_alloc_memory_type(adist); > + if (!IS_ERR(mtype)) > + __init_node_memory_type(node, mtype); > + else > + pr_err("Failed to allocate a memory type at %s()\n", __func__); > + } > + > + set_node_memory_tier(node); > +} > + > +void memory_tier_late_init(void) > +{ > + int nid; > + > + mutex_lock(&memory_tier_lock); > + for_each_node_state(nid, N_MEMORY) > + if (!node_state(nid, N_CPU)) We should exclude "node_memory_types[nid].memtype != NULL". Some memory nodes may be onlined by some device drivers and setup memory tiers already. > + memory_tier_late_create(nid); > + > + establish_demotion_targets(); > + mutex_unlock(&memory_tier_lock); > +} > +EXPORT_SYMBOL_GPL(memory_tier_late_init); > + > static void dump_hmem_attrs(struct access_coordinate *coord, const char *prefix) > { > pr_info( > @@ -636,7 +690,7 @@ int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, > { > int rc = 0; > > - mutex_lock(&memory_tier_lock); > + mutex_lock(&mt_perf_lock); > if (default_dram_perf_error) { > rc = -EIO; > goto out; > @@ -684,7 +738,7 @@ int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, > } > > out: > - mutex_unlock(&memory_tier_lock); > + mutex_unlock(&mt_perf_lock); > return rc; > } > > @@ -700,7 +754,7 @@ int mt_perf_to_adistance(struct access_coordinate *perf, int *adist) > perf->read_bandwidth + perf->write_bandwidth == 0) > return -EINVAL; > > - mutex_lock(&memory_tier_lock); > + mutex_lock(&mt_perf_lock); > /* > * The abstract distance of a memory node is in direct proportion to > * its memory latency (read + write) and inversely proportional to its > @@ -713,7 +767,7 @@ int mt_perf_to_adistance(struct access_coordinate *perf, int *adist) > (default_dram_perf.read_latency + default_dram_perf.write_latency) * > (default_dram_perf.read_bandwidth + default_dram_perf.write_bandwidth) / > (perf->read_bandwidth + perf->write_bandwidth); > - mutex_unlock(&memory_tier_lock); > + mutex_unlock(&mt_perf_lock); > > return 0; > } > @@ -836,6 +890,14 @@ static int __init memory_tier_init(void) > * 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 when HMAT information is HMAT is platform specific, we should avoid to mention it in general code if possible. > + * available. > + */ > + continue; > + > memtier = set_node_memory_tier(node); > if (IS_ERR(memtier)) > /* -- Best Regards, Huang, Ying
On Tue, Mar 12, 2024 at 2:21 AM Huang, Ying <ying.huang@intel.com> wrote: > > "Ho-Ren (Jack) Chuang" <horenchuang@bytedance.com> writes: > > > The current implementation treats emulated memory devices, such as > > CXL1.1 type3 memory, as normal DRAM when they are emulated as normal memory > > (E820_TYPE_RAM). However, these emulated devices have different > > characteristics than traditional DRAM, making it important to > > distinguish them. Thus, we modify the tiered memory initialization process > > to introduce a delay specifically for CPUless NUMA nodes. This delay > > ensures that the memory tier initialization for these nodes is deferred > > until HMAT information is obtained during the boot process. Finally, > > demotion tables are recalculated at the end. > > > > * Abstract common functions into `find_alloc_memory_type()` > > We should move kmem_put_memory_types() (renamed to > mt_put_memory_types()?) too. This can be put in a separate patch. > Will do! Thanks, > > > Since different memory devices require finding or allocating a memory type, > > these common steps are abstracted into a single function, > > `find_alloc_memory_type()`, enhancing code scalability and conciseness. > > > > * Handle cases where there is no HMAT when creating memory tiers > > There is a scenario where a CPUless node does not provide HMAT information. > > If no HMAT is specified, it falls back to using the default DRAM tier. > > > > * Change adist calculation code to use another new lock, mt_perf_lock. > > In the current implementation, iterating through CPUlist nodes requires > > holding the `memory_tier_lock`. However, `mt_calc_adistance()` will end up > > trying to acquire the same lock, leading to a potential deadlock. > > Therefore, we propose introducing a standalone `mt_perf_lock` to protect > > `default_dram_perf`. This approach not only avoids deadlock but also > > prevents holding a large lock simultaneously. > > > > Signed-off-by: Ho-Ren (Jack) Chuang <horenchuang@bytedance.com> > > Signed-off-by: Hao Xiang <hao.xiang@bytedance.com> > > --- > > drivers/acpi/numa/hmat.c | 11 ++++++ > > drivers/dax/kmem.c | 13 +------ > > include/linux/acpi.h | 6 ++++ > > include/linux/memory-tiers.h | 8 +++++ > > mm/memory-tiers.c | 70 +++++++++++++++++++++++++++++++++--- > > 5 files changed, 92 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c > > index d6b85f0f6082..28812ec2c793 100644 > > --- a/drivers/acpi/numa/hmat.c > > +++ b/drivers/acpi/numa/hmat.c > > @@ -38,6 +38,8 @@ static LIST_HEAD(targets); > > static LIST_HEAD(initiators); > > static LIST_HEAD(localities); > > > > +static LIST_HEAD(hmat_memory_types); > > + > > HMAT isn't a device driver for some memory devices. So I don't think we > should manage memory types in HMAT. I can put it back in memory-tier.c. How about the list? Do we still need to keep a separate list for storing late_inited memory nodes? And how about the list name if we need to remove the prefix "hmat_"? > Instead, if the memory_type of a > node isn't set by the driver, we should manage it in memory-tier.c as > fallback. > Do you mean some device drivers may init memory tiers between memory_tier_init() and late_initcall(memory_tier_late_init);? And this is the reason why you mention to exclude "node_memory_types[nid].memtype != NULL" in memory_tier_late_init(). Is my understanding correct? > > static DEFINE_MUTEX(target_lock); > > > > /* > > @@ -149,6 +151,12 @@ int acpi_get_genport_coordinates(u32 uid, > > } > > EXPORT_SYMBOL_NS_GPL(acpi_get_genport_coordinates, CXL); > > > > +struct memory_dev_type *hmat_find_alloc_memory_type(int adist) > > +{ > > + return find_alloc_memory_type(adist, &hmat_memory_types); > > +} > > +EXPORT_SYMBOL_GPL(hmat_find_alloc_memory_type); > > + > > static __init void alloc_memory_initiator(unsigned int cpu_pxm) > > { > > struct memory_initiator *initiator; > > @@ -1038,6 +1046,9 @@ static __init int hmat_init(void) > > if (!hmat_set_default_dram_perf()) > > register_mt_adistance_algorithm(&hmat_adist_nb); > > > > + /* Post-create CPUless memory tiers after getting HMAT info */ > > + memory_tier_late_init(); > > + > > This should be called in memory-tier.c via > > late_initcall(memory_tier_late_init); > > Then, we don't need hmat to call it. > Thanks. Learned! > > return 0; > > out_put: > > hmat_free_structures(); > > diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c > > index 42ee360cf4e3..aee17ab59f4f 100644 > > --- a/drivers/dax/kmem.c > > +++ b/drivers/dax/kmem.c > > @@ -55,21 +55,10 @@ static LIST_HEAD(kmem_memory_types); > > > > static struct memory_dev_type *kmem_find_alloc_memory_type(int adist) > > { > > - bool found = false; > > struct memory_dev_type *mtype; > > > > mutex_lock(&kmem_memory_type_lock); > > - list_for_each_entry(mtype, &kmem_memory_types, list) { > > - if (mtype->adistance == adist) { > > - found = true; > > - break; > > - } > > - } > > - if (!found) { > > - mtype = alloc_memory_type(adist); > > - if (!IS_ERR(mtype)) > > - list_add(&mtype->list, &kmem_memory_types); > > - } > > + mtype = find_alloc_memory_type(adist, &kmem_memory_types); > > mutex_unlock(&kmem_memory_type_lock); > > > > return mtype; > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > > index b7165e52b3c6..3f927ff01f02 100644 > > --- a/include/linux/acpi.h > > +++ b/include/linux/acpi.h > > @@ -434,12 +434,18 @@ int thermal_acpi_critical_trip_temp(struct acpi_device *adev, int *ret_temp); > > > > #ifdef CONFIG_ACPI_HMAT > > int acpi_get_genport_coordinates(u32 uid, struct access_coordinate *coord); > > +struct memory_dev_type *hmat_find_alloc_memory_type(int adist); > > #else > > static inline int acpi_get_genport_coordinates(u32 uid, > > struct access_coordinate *coord) > > { > > return -EOPNOTSUPP; > > } > > + > > +static inline struct memory_dev_type *hmat_find_alloc_memory_type(int adist) > > +{ > > + return NULL; > > +} > > #endif > > > > #ifdef CONFIG_ACPI_NUMA > > diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h > > index 69e781900082..4bc2596c5774 100644 > > --- a/include/linux/memory-tiers.h > > +++ b/include/linux/memory-tiers.h > > @@ -48,6 +48,9 @@ int mt_calc_adistance(int node, int *adist); > > int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, > > const char *source); > > int mt_perf_to_adistance(struct access_coordinate *perf, int *adist); > > +struct memory_dev_type *find_alloc_memory_type(int adist, > > + struct list_head *memory_types); > > +void memory_tier_late_init(void); > > #ifdef CONFIG_MIGRATION > > int next_demotion_node(int node); > > void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets); > > @@ -136,5 +139,10 @@ static inline int mt_perf_to_adistance(struct access_coordinate *perf, int *adis > > { > > return -EIO; > > } > > + > > +static inline void memory_tier_late_init(void) > > +{ > > + > > +} > > #endif /* CONFIG_NUMA */ > > #endif /* _LINUX_MEMORY_TIERS_H */ > > diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c > > index 0537664620e5..79f748d60e6f 100644 > > --- a/mm/memory-tiers.c > > +++ b/mm/memory-tiers.c > > @@ -6,6 +6,7 @@ > > #include <linux/memory.h> > > #include <linux/memory-tiers.h> > > #include <linux/notifier.h> > > +#include <linux/acpi.h> > > > > #include "internal.h" > > > > @@ -35,6 +36,7 @@ struct node_memory_type_map { > > }; > > > > static DEFINE_MUTEX(memory_tier_lock); > > +static DEFINE_MUTEX(mt_perf_lock); > > Please add comments about what it protects. And put it near the data > structure it protects. > Where is better for me to add comments for this? Code? Patch description? Will put it closer to the protected data. Thanks. > > static LIST_HEAD(memory_tiers); > > static struct node_memory_type_map node_memory_types[MAX_NUMNODES]; > > struct memory_dev_type *default_dram_type; > > @@ -623,6 +625,58 @@ void clear_node_memory_type(int node, struct memory_dev_type *memtype) > > } > > EXPORT_SYMBOL_GPL(clear_node_memory_type); > > > > +struct memory_dev_type *find_alloc_memory_type(int adist, struct list_head *memory_types) > > +{ > > + bool found = false; > > + struct memory_dev_type *mtype; > > + > > + list_for_each_entry(mtype, memory_types, list) { > > + if (mtype->adistance == adist) { > > + found = true; > > + break; > > + } > > + } > > + if (!found) { > > + mtype = alloc_memory_type(adist); > > + if (!IS_ERR(mtype)) > > + list_add(&mtype->list, memory_types); > > + } > > + > > + return mtype; > > +} > > +EXPORT_SYMBOL_GPL(find_alloc_memory_type); > > + > > +static void memory_tier_late_create(int node) > > +{ > > + struct memory_dev_type *mtype = NULL; > > + int adist = MEMTIER_ADISTANCE_DRAM; > > + > > + mt_calc_adistance(node, &adist); > > + if (adist != MEMTIER_ADISTANCE_DRAM) { > > We can manage default_dram_type() via find_alloc_memory_type() > too. > > And, if "node_memory_types[node].memtype == NULL", we can call > mt_calc_adistance(node, &adist) and find_alloc_memory_type() in > set_node_memory_tier(). Then, we can cover hotpluged memory node too. > > > + mtype = hmat_find_alloc_memory_type(adist); > > + if (!IS_ERR(mtype)) > > + __init_node_memory_type(node, mtype); > > + else > > + pr_err("Failed to allocate a memory type at %s()\n", __func__); > > + } > > + > > + set_node_memory_tier(node); > > +} > > + > > +void memory_tier_late_init(void) > > +{ > > + int nid; > > + > > + mutex_lock(&memory_tier_lock); > > + for_each_node_state(nid, N_MEMORY) > > + if (!node_state(nid, N_CPU)) > > We should exclude "node_memory_types[nid].memtype != NULL". Some memory > nodes may be onlined by some device drivers and setup memory tiers > already. > > > + memory_tier_late_create(nid); > > + > > + establish_demotion_targets(); > > + mutex_unlock(&memory_tier_lock); > > +} > > +EXPORT_SYMBOL_GPL(memory_tier_late_init); > > + > > static void dump_hmem_attrs(struct access_coordinate *coord, const char *prefix) > > { > > pr_info( > > @@ -636,7 +690,7 @@ int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, > > { > > int rc = 0; > > > > - mutex_lock(&memory_tier_lock); > > + mutex_lock(&mt_perf_lock); > > if (default_dram_perf_error) { > > rc = -EIO; > > goto out; > > @@ -684,7 +738,7 @@ int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, > > } > > > > out: > > - mutex_unlock(&memory_tier_lock); > > + mutex_unlock(&mt_perf_lock); > > return rc; > > } > > > > @@ -700,7 +754,7 @@ int mt_perf_to_adistance(struct access_coordinate *perf, int *adist) > > perf->read_bandwidth + perf->write_bandwidth == 0) > > return -EINVAL; > > > > - mutex_lock(&memory_tier_lock); > > + mutex_lock(&mt_perf_lock); > > /* > > * The abstract distance of a memory node is in direct proportion to > > * its memory latency (read + write) and inversely proportional to its > > @@ -713,7 +767,7 @@ int mt_perf_to_adistance(struct access_coordinate *perf, int *adist) > > (default_dram_perf.read_latency + default_dram_perf.write_latency) * > > (default_dram_perf.read_bandwidth + default_dram_perf.write_bandwidth) / > > (perf->read_bandwidth + perf->write_bandwidth); > > - mutex_unlock(&memory_tier_lock); > > + mutex_unlock(&mt_perf_lock); > > > > return 0; > > } > > @@ -836,6 +890,14 @@ static int __init memory_tier_init(void) > > * 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 when HMAT information is > > HMAT is platform specific, we should avoid to mention it in general code > if possible. > Will fix! Thanks, > > + * available. > > + */ > > + continue; > > + > > memtier = set_node_memory_tier(node); > > if (IS_ERR(memtier)) > > /* > > -- > Best Regards, > Huang, Ying
"Ho-Ren (Jack) Chuang" <horenchuang@bytedance.com> writes: > On Tue, Mar 12, 2024 at 2:21 AM Huang, Ying <ying.huang@intel.com> wrote: >> >> "Ho-Ren (Jack) Chuang" <horenchuang@bytedance.com> writes: >> >> > The current implementation treats emulated memory devices, such as >> > CXL1.1 type3 memory, as normal DRAM when they are emulated as normal memory >> > (E820_TYPE_RAM). However, these emulated devices have different >> > characteristics than traditional DRAM, making it important to >> > distinguish them. Thus, we modify the tiered memory initialization process >> > to introduce a delay specifically for CPUless NUMA nodes. This delay >> > ensures that the memory tier initialization for these nodes is deferred >> > until HMAT information is obtained during the boot process. Finally, >> > demotion tables are recalculated at the end. >> > >> > * Abstract common functions into `find_alloc_memory_type()` >> >> We should move kmem_put_memory_types() (renamed to >> mt_put_memory_types()?) too. This can be put in a separate patch. >> > > Will do! Thanks, > > >> >> > Since different memory devices require finding or allocating a memory type, >> > these common steps are abstracted into a single function, >> > `find_alloc_memory_type()`, enhancing code scalability and conciseness. >> > >> > * Handle cases where there is no HMAT when creating memory tiers >> > There is a scenario where a CPUless node does not provide HMAT information. >> > If no HMAT is specified, it falls back to using the default DRAM tier. >> > >> > * Change adist calculation code to use another new lock, mt_perf_lock. >> > In the current implementation, iterating through CPUlist nodes requires >> > holding the `memory_tier_lock`. However, `mt_calc_adistance()` will end up >> > trying to acquire the same lock, leading to a potential deadlock. >> > Therefore, we propose introducing a standalone `mt_perf_lock` to protect >> > `default_dram_perf`. This approach not only avoids deadlock but also >> > prevents holding a large lock simultaneously. >> > >> > Signed-off-by: Ho-Ren (Jack) Chuang <horenchuang@bytedance.com> >> > Signed-off-by: Hao Xiang <hao.xiang@bytedance.com> >> > --- >> > drivers/acpi/numa/hmat.c | 11 ++++++ >> > drivers/dax/kmem.c | 13 +------ >> > include/linux/acpi.h | 6 ++++ >> > include/linux/memory-tiers.h | 8 +++++ >> > mm/memory-tiers.c | 70 +++++++++++++++++++++++++++++++++--- >> > 5 files changed, 92 insertions(+), 16 deletions(-) >> > >> > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c >> > index d6b85f0f6082..28812ec2c793 100644 >> > --- a/drivers/acpi/numa/hmat.c >> > +++ b/drivers/acpi/numa/hmat.c >> > @@ -38,6 +38,8 @@ static LIST_HEAD(targets); >> > static LIST_HEAD(initiators); >> > static LIST_HEAD(localities); >> > >> > +static LIST_HEAD(hmat_memory_types); >> > + >> >> HMAT isn't a device driver for some memory devices. So I don't think we >> should manage memory types in HMAT. > > I can put it back in memory-tier.c. How about the list? Do we still > need to keep a separate list for storing late_inited memory nodes? > And how about the list name if we need to remove the prefix "hmat_"? I don't think we need a separate list for memory-less nodes. Just iterate all memory-less nodes. > >> Instead, if the memory_type of a >> node isn't set by the driver, we should manage it in memory-tier.c as >> fallback. >> > > Do you mean some device drivers may init memory tiers between > memory_tier_init() and late_initcall(memory_tier_late_init);? > And this is the reason why you mention to exclude > "node_memory_types[nid].memtype != NULL" in memory_tier_late_init(). > Is my understanding correct? Yes. >> > static DEFINE_MUTEX(target_lock); >> > >> > /* >> > @@ -149,6 +151,12 @@ int acpi_get_genport_coordinates(u32 uid, >> > } >> > EXPORT_SYMBOL_NS_GPL(acpi_get_genport_coordinates, CXL); >> > >> > +struct memory_dev_type *hmat_find_alloc_memory_type(int adist) >> > +{ >> > + return find_alloc_memory_type(adist, &hmat_memory_types); >> > +} >> > +EXPORT_SYMBOL_GPL(hmat_find_alloc_memory_type); >> > + >> > static __init void alloc_memory_initiator(unsigned int cpu_pxm) >> > { >> > struct memory_initiator *initiator; >> > @@ -1038,6 +1046,9 @@ static __init int hmat_init(void) >> > if (!hmat_set_default_dram_perf()) >> > register_mt_adistance_algorithm(&hmat_adist_nb); >> > >> > + /* Post-create CPUless memory tiers after getting HMAT info */ >> > + memory_tier_late_init(); >> > + >> >> This should be called in memory-tier.c via >> >> late_initcall(memory_tier_late_init); >> >> Then, we don't need hmat to call it. >> > > Thanks. Learned! > > >> > return 0; >> > out_put: >> > hmat_free_structures(); >> > diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c >> > index 42ee360cf4e3..aee17ab59f4f 100644 >> > --- a/drivers/dax/kmem.c >> > +++ b/drivers/dax/kmem.c >> > @@ -55,21 +55,10 @@ static LIST_HEAD(kmem_memory_types); >> > >> > static struct memory_dev_type *kmem_find_alloc_memory_type(int adist) >> > { >> > - bool found = false; >> > struct memory_dev_type *mtype; >> > >> > mutex_lock(&kmem_memory_type_lock); >> > - list_for_each_entry(mtype, &kmem_memory_types, list) { >> > - if (mtype->adistance == adist) { >> > - found = true; >> > - break; >> > - } >> > - } >> > - if (!found) { >> > - mtype = alloc_memory_type(adist); >> > - if (!IS_ERR(mtype)) >> > - list_add(&mtype->list, &kmem_memory_types); >> > - } >> > + mtype = find_alloc_memory_type(adist, &kmem_memory_types); >> > mutex_unlock(&kmem_memory_type_lock); >> > >> > return mtype; >> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h >> > index b7165e52b3c6..3f927ff01f02 100644 >> > --- a/include/linux/acpi.h >> > +++ b/include/linux/acpi.h >> > @@ -434,12 +434,18 @@ int thermal_acpi_critical_trip_temp(struct acpi_device *adev, int *ret_temp); >> > >> > #ifdef CONFIG_ACPI_HMAT >> > int acpi_get_genport_coordinates(u32 uid, struct access_coordinate *coord); >> > +struct memory_dev_type *hmat_find_alloc_memory_type(int adist); >> > #else >> > static inline int acpi_get_genport_coordinates(u32 uid, >> > struct access_coordinate *coord) >> > { >> > return -EOPNOTSUPP; >> > } >> > + >> > +static inline struct memory_dev_type *hmat_find_alloc_memory_type(int adist) >> > +{ >> > + return NULL; >> > +} >> > #endif >> > >> > #ifdef CONFIG_ACPI_NUMA >> > diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h >> > index 69e781900082..4bc2596c5774 100644 >> > --- a/include/linux/memory-tiers.h >> > +++ b/include/linux/memory-tiers.h >> > @@ -48,6 +48,9 @@ int mt_calc_adistance(int node, int *adist); >> > int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, >> > const char *source); >> > int mt_perf_to_adistance(struct access_coordinate *perf, int *adist); >> > +struct memory_dev_type *find_alloc_memory_type(int adist, >> > + struct list_head *memory_types); >> > +void memory_tier_late_init(void); >> > #ifdef CONFIG_MIGRATION >> > int next_demotion_node(int node); >> > void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets); >> > @@ -136,5 +139,10 @@ static inline int mt_perf_to_adistance(struct access_coordinate *perf, int *adis >> > { >> > return -EIO; >> > } >> > + >> > +static inline void memory_tier_late_init(void) >> > +{ >> > + >> > +} >> > #endif /* CONFIG_NUMA */ >> > #endif /* _LINUX_MEMORY_TIERS_H */ >> > diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c >> > index 0537664620e5..79f748d60e6f 100644 >> > --- a/mm/memory-tiers.c >> > +++ b/mm/memory-tiers.c >> > @@ -6,6 +6,7 @@ >> > #include <linux/memory.h> >> > #include <linux/memory-tiers.h> >> > #include <linux/notifier.h> >> > +#include <linux/acpi.h> >> > >> > #include "internal.h" >> > >> > @@ -35,6 +36,7 @@ struct node_memory_type_map { >> > }; >> > >> > static DEFINE_MUTEX(memory_tier_lock); >> > +static DEFINE_MUTEX(mt_perf_lock); >> >> Please add comments about what it protects. And put it near the data >> structure it protects. >> > > Where is better for me to add comments for this? Code? Patch description? > Will put it closer to the protected data. Thanks. Just put the comments at the above of the lock in the source code. > > >> > static LIST_HEAD(memory_tiers); >> > static struct node_memory_type_map node_memory_types[MAX_NUMNODES]; >> > struct memory_dev_type *default_dram_type; >> > @@ -623,6 +625,58 @@ void clear_node_memory_type(int node, struct memory_dev_type *memtype) >> > } >> > EXPORT_SYMBOL_GPL(clear_node_memory_type); >> > >> > +struct memory_dev_type *find_alloc_memory_type(int adist, struct list_head *memory_types) >> > +{ >> > + bool found = false; >> > + struct memory_dev_type *mtype; >> > + >> > + list_for_each_entry(mtype, memory_types, list) { >> > + if (mtype->adistance == adist) { >> > + found = true; >> > + break; >> > + } >> > + } >> > + if (!found) { >> > + mtype = alloc_memory_type(adist); >> > + if (!IS_ERR(mtype)) >> > + list_add(&mtype->list, memory_types); >> > + } >> > + >> > + return mtype; >> > +} >> > +EXPORT_SYMBOL_GPL(find_alloc_memory_type); >> > + >> > +static void memory_tier_late_create(int node) >> > +{ >> > + struct memory_dev_type *mtype = NULL; >> > + int adist = MEMTIER_ADISTANCE_DRAM; >> > + >> > + mt_calc_adistance(node, &adist); >> > + if (adist != MEMTIER_ADISTANCE_DRAM) { >> >> We can manage default_dram_type() via find_alloc_memory_type() >> too. >> >> And, if "node_memory_types[node].memtype == NULL", we can call >> mt_calc_adistance(node, &adist) and find_alloc_memory_type() in >> set_node_memory_tier(). Then, we can cover hotpluged memory node too. >> >> > + mtype = hmat_find_alloc_memory_type(adist); >> > + if (!IS_ERR(mtype)) >> > + __init_node_memory_type(node, mtype); >> > + else >> > + pr_err("Failed to allocate a memory type at %s()\n", __func__); >> > + } >> > + >> > + set_node_memory_tier(node); >> > +} >> > + >> > +void memory_tier_late_init(void) >> > +{ >> > + int nid; >> > + >> > + mutex_lock(&memory_tier_lock); >> > + for_each_node_state(nid, N_MEMORY) >> > + if (!node_state(nid, N_CPU)) >> >> We should exclude "node_memory_types[nid].memtype != NULL". Some memory >> nodes may be onlined by some device drivers and setup memory tiers >> already. >> >> > + memory_tier_late_create(nid); >> > + >> > + establish_demotion_targets(); >> > + mutex_unlock(&memory_tier_lock); >> > +} >> > +EXPORT_SYMBOL_GPL(memory_tier_late_init); >> > + >> > static void dump_hmem_attrs(struct access_coordinate *coord, const char *prefix) >> > { >> > pr_info( >> > @@ -636,7 +690,7 @@ int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, >> > { >> > int rc = 0; >> > >> > - mutex_lock(&memory_tier_lock); >> > + mutex_lock(&mt_perf_lock); >> > if (default_dram_perf_error) { >> > rc = -EIO; >> > goto out; >> > @@ -684,7 +738,7 @@ int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, >> > } >> > >> > out: >> > - mutex_unlock(&memory_tier_lock); >> > + mutex_unlock(&mt_perf_lock); >> > return rc; >> > } >> > >> > @@ -700,7 +754,7 @@ int mt_perf_to_adistance(struct access_coordinate *perf, int *adist) >> > perf->read_bandwidth + perf->write_bandwidth == 0) >> > return -EINVAL; >> > >> > - mutex_lock(&memory_tier_lock); >> > + mutex_lock(&mt_perf_lock); >> > /* >> > * The abstract distance of a memory node is in direct proportion to >> > * its memory latency (read + write) and inversely proportional to its >> > @@ -713,7 +767,7 @@ int mt_perf_to_adistance(struct access_coordinate *perf, int *adist) >> > (default_dram_perf.read_latency + default_dram_perf.write_latency) * >> > (default_dram_perf.read_bandwidth + default_dram_perf.write_bandwidth) / >> > (perf->read_bandwidth + perf->write_bandwidth); >> > - mutex_unlock(&memory_tier_lock); >> > + mutex_unlock(&mt_perf_lock); >> > >> > return 0; >> > } >> > @@ -836,6 +890,14 @@ static int __init memory_tier_init(void) >> > * 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 when HMAT information is >> >> HMAT is platform specific, we should avoid to mention it in general code >> if possible. >> > > Will fix! Thanks, > > >> > + * available. >> > + */ >> > + continue; >> > + >> > memtier = set_node_memory_tier(node); >> > if (IS_ERR(memtier)) >> > /* >> -- Best Regards, Huang, Ying
I'm working on V3. Thanks for Ying's feedback. cc: sthanneeru@micron.com On Thu, Mar 14, 2024 at 12:54 AM Huang, Ying <ying.huang@intel.com> wrote: > > "Ho-Ren (Jack) Chuang" <horenchuang@bytedance.com> writes: > > > On Tue, Mar 12, 2024 at 2:21 AM Huang, Ying <ying.huang@intel.com> wrote: > >> > >> "Ho-Ren (Jack) Chuang" <horenchuang@bytedance.com> writes: > >> > >> > The current implementation treats emulated memory devices, such as > >> > CXL1.1 type3 memory, as normal DRAM when they are emulated as normal memory > >> > (E820_TYPE_RAM). However, these emulated devices have different > >> > characteristics than traditional DRAM, making it important to > >> > distinguish them. Thus, we modify the tiered memory initialization process > >> > to introduce a delay specifically for CPUless NUMA nodes. This delay > >> > ensures that the memory tier initialization for these nodes is deferred > >> > until HMAT information is obtained during the boot process. Finally, > >> > demotion tables are recalculated at the end. > >> > > >> > * Abstract common functions into `find_alloc_memory_type()` > >> > >> We should move kmem_put_memory_types() (renamed to > >> mt_put_memory_types()?) too. This can be put in a separate patch. > >> > > > > Will do! Thanks, > > > > > >> > >> > Since different memory devices require finding or allocating a memory type, > >> > these common steps are abstracted into a single function, > >> > `find_alloc_memory_type()`, enhancing code scalability and conciseness. > >> > > >> > * Handle cases where there is no HMAT when creating memory tiers > >> > There is a scenario where a CPUless node does not provide HMAT information. > >> > If no HMAT is specified, it falls back to using the default DRAM tier. > >> > > >> > * Change adist calculation code to use another new lock, mt_perf_lock. > >> > In the current implementation, iterating through CPUlist nodes requires > >> > holding the `memory_tier_lock`. However, `mt_calc_adistance()` will end up > >> > trying to acquire the same lock, leading to a potential deadlock. > >> > Therefore, we propose introducing a standalone `mt_perf_lock` to protect > >> > `default_dram_perf`. This approach not only avoids deadlock but also > >> > prevents holding a large lock simultaneously. > >> > > >> > Signed-off-by: Ho-Ren (Jack) Chuang <horenchuang@bytedance.com> > >> > Signed-off-by: Hao Xiang <hao.xiang@bytedance.com> > >> > --- > >> > drivers/acpi/numa/hmat.c | 11 ++++++ > >> > drivers/dax/kmem.c | 13 +------ > >> > include/linux/acpi.h | 6 ++++ > >> > include/linux/memory-tiers.h | 8 +++++ > >> > mm/memory-tiers.c | 70 +++++++++++++++++++++++++++++++++--- > >> > 5 files changed, 92 insertions(+), 16 deletions(-) > >> > > >> > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c > >> > index d6b85f0f6082..28812ec2c793 100644 > >> > --- a/drivers/acpi/numa/hmat.c > >> > +++ b/drivers/acpi/numa/hmat.c > >> > @@ -38,6 +38,8 @@ static LIST_HEAD(targets); > >> > static LIST_HEAD(initiators); > >> > static LIST_HEAD(localities); > >> > > >> > +static LIST_HEAD(hmat_memory_types); > >> > + > >> > >> HMAT isn't a device driver for some memory devices. So I don't think we > >> should manage memory types in HMAT. > > > > I can put it back in memory-tier.c. How about the list? Do we still > > need to keep a separate list for storing late_inited memory nodes? > > And how about the list name if we need to remove the prefix "hmat_"? > > I don't think we need a separate list for memory-less nodes. Just > iterate all memory-less nodes. > Ok. There is no need to keep a list of memory-less nodes. I will only keep a memory_type list to manage created memory types. > > > >> Instead, if the memory_type of a > >> node isn't set by the driver, we should manage it in memory-tier.c as > >> fallback. > >> > > > > Do you mean some device drivers may init memory tiers between > > memory_tier_init() and late_initcall(memory_tier_late_init);? > > And this is the reason why you mention to exclude > > "node_memory_types[nid].memtype != NULL" in memory_tier_late_init(). > > Is my understanding correct? > > Yes. > Thanks. > >> > static DEFINE_MUTEX(target_lock); > >> > > >> > /* > >> > @@ -149,6 +151,12 @@ int acpi_get_genport_coordinates(u32 uid, > >> > } > >> > EXPORT_SYMBOL_NS_GPL(acpi_get_genport_coordinates, CXL); > >> > > >> > +struct memory_dev_type *hmat_find_alloc_memory_type(int adist) > >> > +{ > >> > + return find_alloc_memory_type(adist, &hmat_memory_types); > >> > +} > >> > +EXPORT_SYMBOL_GPL(hmat_find_alloc_memory_type); > >> > + > >> > static __init void alloc_memory_initiator(unsigned int cpu_pxm) > >> > { > >> > struct memory_initiator *initiator; > >> > @@ -1038,6 +1046,9 @@ static __init int hmat_init(void) > >> > if (!hmat_set_default_dram_perf()) > >> > register_mt_adistance_algorithm(&hmat_adist_nb); > >> > > >> > + /* Post-create CPUless memory tiers after getting HMAT info */ > >> > + memory_tier_late_init(); > >> > + > >> > >> This should be called in memory-tier.c via > >> > >> late_initcall(memory_tier_late_init); > >> > >> Then, we don't need hmat to call it. > >> > > > > Thanks. Learned! > > > > > >> > return 0; > >> > out_put: > >> > hmat_free_structures(); > >> > diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c > >> > index 42ee360cf4e3..aee17ab59f4f 100644 > >> > --- a/drivers/dax/kmem.c > >> > +++ b/drivers/dax/kmem.c > >> > @@ -55,21 +55,10 @@ static LIST_HEAD(kmem_memory_types); > >> > > >> > static struct memory_dev_type *kmem_find_alloc_memory_type(int adist) > >> > { > >> > - bool found = false; > >> > struct memory_dev_type *mtype; > >> > > >> > mutex_lock(&kmem_memory_type_lock); > >> > - list_for_each_entry(mtype, &kmem_memory_types, list) { > >> > - if (mtype->adistance == adist) { > >> > - found = true; > >> > - break; > >> > - } > >> > - } > >> > - if (!found) { > >> > - mtype = alloc_memory_type(adist); > >> > - if (!IS_ERR(mtype)) > >> > - list_add(&mtype->list, &kmem_memory_types); > >> > - } > >> > + mtype = find_alloc_memory_type(adist, &kmem_memory_types); > >> > mutex_unlock(&kmem_memory_type_lock); > >> > > >> > return mtype; > >> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > >> > index b7165e52b3c6..3f927ff01f02 100644 > >> > --- a/include/linux/acpi.h > >> > +++ b/include/linux/acpi.h > >> > @@ -434,12 +434,18 @@ int thermal_acpi_critical_trip_temp(struct acpi_device *adev, int *ret_temp); > >> > > >> > #ifdef CONFIG_ACPI_HMAT > >> > int acpi_get_genport_coordinates(u32 uid, struct access_coordinate *coord); > >> > +struct memory_dev_type *hmat_find_alloc_memory_type(int adist); > >> > #else > >> > static inline int acpi_get_genport_coordinates(u32 uid, > >> > struct access_coordinate *coord) > >> > { > >> > return -EOPNOTSUPP; > >> > } > >> > + > >> > +static inline struct memory_dev_type *hmat_find_alloc_memory_type(int adist) > >> > +{ > >> > + return NULL; > >> > +} > >> > #endif > >> > > >> > #ifdef CONFIG_ACPI_NUMA > >> > diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h > >> > index 69e781900082..4bc2596c5774 100644 > >> > --- a/include/linux/memory-tiers.h > >> > +++ b/include/linux/memory-tiers.h > >> > @@ -48,6 +48,9 @@ int mt_calc_adistance(int node, int *adist); > >> > int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, > >> > const char *source); > >> > int mt_perf_to_adistance(struct access_coordinate *perf, int *adist); > >> > +struct memory_dev_type *find_alloc_memory_type(int adist, > >> > + struct list_head *memory_types); > >> > +void memory_tier_late_init(void); > >> > #ifdef CONFIG_MIGRATION > >> > int next_demotion_node(int node); > >> > void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets); > >> > @@ -136,5 +139,10 @@ static inline int mt_perf_to_adistance(struct access_coordinate *perf, int *adis > >> > { > >> > return -EIO; > >> > } > >> > + > >> > +static inline void memory_tier_late_init(void) > >> > +{ > >> > + > >> > +} > >> > #endif /* CONFIG_NUMA */ > >> > #endif /* _LINUX_MEMORY_TIERS_H */ > >> > diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c > >> > index 0537664620e5..79f748d60e6f 100644 > >> > --- a/mm/memory-tiers.c > >> > +++ b/mm/memory-tiers.c > >> > @@ -6,6 +6,7 @@ > >> > #include <linux/memory.h> > >> > #include <linux/memory-tiers.h> > >> > #include <linux/notifier.h> > >> > +#include <linux/acpi.h> > >> > > >> > #include "internal.h" > >> > > >> > @@ -35,6 +36,7 @@ struct node_memory_type_map { > >> > }; > >> > > >> > static DEFINE_MUTEX(memory_tier_lock); > >> > +static DEFINE_MUTEX(mt_perf_lock); > >> > >> Please add comments about what it protects. And put it near the data > >> structure it protects. > >> > > > > Where is better for me to add comments for this? Code? Patch description? > > Will put it closer to the protected data. Thanks. > > Just put the comments at the above of the lock in the source code. > Got it. Thanks! > > > > > > >> > static LIST_HEAD(memory_tiers); > >> > static struct node_memory_type_map node_memory_types[MAX_NUMNODES]; > >> > struct memory_dev_type *default_dram_type; > >> > @@ -623,6 +625,58 @@ void clear_node_memory_type(int node, struct memory_dev_type *memtype) > >> > } > >> > EXPORT_SYMBOL_GPL(clear_node_memory_type); > >> > > >> > +struct memory_dev_type *find_alloc_memory_type(int adist, struct list_head *memory_types) > >> > +{ > >> > + bool found = false; > >> > + struct memory_dev_type *mtype; > >> > + > >> > + list_for_each_entry(mtype, memory_types, list) { > >> > + if (mtype->adistance == adist) { > >> > + found = true; > >> > + break; > >> > + } > >> > + } > >> > + if (!found) { > >> > + mtype = alloc_memory_type(adist); > >> > + if (!IS_ERR(mtype)) > >> > + list_add(&mtype->list, memory_types); > >> > + } > >> > + > >> > + return mtype; > >> > +} > >> > +EXPORT_SYMBOL_GPL(find_alloc_memory_type); > >> > + > >> > +static void memory_tier_late_create(int node) > >> > +{ > >> > + struct memory_dev_type *mtype = NULL; > >> > + int adist = MEMTIER_ADISTANCE_DRAM; > >> > + > >> > + mt_calc_adistance(node, &adist); > >> > + if (adist != MEMTIER_ADISTANCE_DRAM) { > >> > >> We can manage default_dram_type() via find_alloc_memory_type() > >> too. > >> > >> And, if "node_memory_types[node].memtype == NULL", we can call > >> mt_calc_adistance(node, &adist) and find_alloc_memory_type() in > >> set_node_memory_tier(). Then, we can cover hotpluged memory node too. > >> > >> > + mtype = hmat_find_alloc_memory_type(adist); > >> > + if (!IS_ERR(mtype)) > >> > + __init_node_memory_type(node, mtype); > >> > + else > >> > + pr_err("Failed to allocate a memory type at %s()\n", __func__); > >> > + } > >> > + > >> > + set_node_memory_tier(node); > >> > +} > >> > + > >> > +void memory_tier_late_init(void) > >> > +{ > >> > + int nid; > >> > + > >> > + mutex_lock(&memory_tier_lock); > >> > + for_each_node_state(nid, N_MEMORY) > >> > + if (!node_state(nid, N_CPU)) > >> > >> We should exclude "node_memory_types[nid].memtype != NULL". Some memory > >> nodes may be onlined by some device drivers and setup memory tiers > >> already. > >> > >> > + memory_tier_late_create(nid); > >> > + > >> > + establish_demotion_targets(); > >> > + mutex_unlock(&memory_tier_lock); > >> > +} > >> > +EXPORT_SYMBOL_GPL(memory_tier_late_init); > >> > + > >> > static void dump_hmem_attrs(struct access_coordinate *coord, const char *prefix) > >> > { > >> > pr_info( > >> > @@ -636,7 +690,7 @@ int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, > >> > { > >> > int rc = 0; > >> > > >> > - mutex_lock(&memory_tier_lock); > >> > + mutex_lock(&mt_perf_lock); > >> > if (default_dram_perf_error) { > >> > rc = -EIO; > >> > goto out; > >> > @@ -684,7 +738,7 @@ int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, > >> > } > >> > > >> > out: > >> > - mutex_unlock(&memory_tier_lock); > >> > + mutex_unlock(&mt_perf_lock); > >> > return rc; > >> > } > >> > > >> > @@ -700,7 +754,7 @@ int mt_perf_to_adistance(struct access_coordinate *perf, int *adist) > >> > perf->read_bandwidth + perf->write_bandwidth == 0) > >> > return -EINVAL; > >> > > >> > - mutex_lock(&memory_tier_lock); > >> > + mutex_lock(&mt_perf_lock); > >> > /* > >> > * The abstract distance of a memory node is in direct proportion to > >> > * its memory latency (read + write) and inversely proportional to its > >> > @@ -713,7 +767,7 @@ int mt_perf_to_adistance(struct access_coordinate *perf, int *adist) > >> > (default_dram_perf.read_latency + default_dram_perf.write_latency) * > >> > (default_dram_perf.read_bandwidth + default_dram_perf.write_bandwidth) / > >> > (perf->read_bandwidth + perf->write_bandwidth); > >> > - mutex_unlock(&memory_tier_lock); > >> > + mutex_unlock(&mt_perf_lock); > >> > > >> > return 0; > >> > } > >> > @@ -836,6 +890,14 @@ static int __init memory_tier_init(void) > >> > * 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 when HMAT information is > >> > >> HMAT is platform specific, we should avoid to mention it in general code > >> if possible. > >> > > > > Will fix! Thanks, > > > > > >> > + * available. > >> > + */ > >> > + continue; > >> > + > >> > memtier = set_node_memory_tier(node); > >> > if (IS_ERR(memtier)) > >> > /* > >> > > -- > Best Regards, > Huang, Ying -- Best regards, Ho-Ren (Jack) Chuang 莊賀任
diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c index d6b85f0f6082..28812ec2c793 100644 --- a/drivers/acpi/numa/hmat.c +++ b/drivers/acpi/numa/hmat.c @@ -38,6 +38,8 @@ static LIST_HEAD(targets); static LIST_HEAD(initiators); static LIST_HEAD(localities); +static LIST_HEAD(hmat_memory_types); + static DEFINE_MUTEX(target_lock); /* @@ -149,6 +151,12 @@ int acpi_get_genport_coordinates(u32 uid, } EXPORT_SYMBOL_NS_GPL(acpi_get_genport_coordinates, CXL); +struct memory_dev_type *hmat_find_alloc_memory_type(int adist) +{ + return find_alloc_memory_type(adist, &hmat_memory_types); +} +EXPORT_SYMBOL_GPL(hmat_find_alloc_memory_type); + static __init void alloc_memory_initiator(unsigned int cpu_pxm) { struct memory_initiator *initiator; @@ -1038,6 +1046,9 @@ static __init int hmat_init(void) if (!hmat_set_default_dram_perf()) register_mt_adistance_algorithm(&hmat_adist_nb); + /* Post-create CPUless memory tiers after getting HMAT info */ + memory_tier_late_init(); + return 0; out_put: hmat_free_structures(); diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c index 42ee360cf4e3..aee17ab59f4f 100644 --- a/drivers/dax/kmem.c +++ b/drivers/dax/kmem.c @@ -55,21 +55,10 @@ static LIST_HEAD(kmem_memory_types); static struct memory_dev_type *kmem_find_alloc_memory_type(int adist) { - bool found = false; struct memory_dev_type *mtype; mutex_lock(&kmem_memory_type_lock); - list_for_each_entry(mtype, &kmem_memory_types, list) { - if (mtype->adistance == adist) { - found = true; - break; - } - } - if (!found) { - mtype = alloc_memory_type(adist); - if (!IS_ERR(mtype)) - list_add(&mtype->list, &kmem_memory_types); - } + mtype = find_alloc_memory_type(adist, &kmem_memory_types); mutex_unlock(&kmem_memory_type_lock); return mtype; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index b7165e52b3c6..3f927ff01f02 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -434,12 +434,18 @@ int thermal_acpi_critical_trip_temp(struct acpi_device *adev, int *ret_temp); #ifdef CONFIG_ACPI_HMAT int acpi_get_genport_coordinates(u32 uid, struct access_coordinate *coord); +struct memory_dev_type *hmat_find_alloc_memory_type(int adist); #else static inline int acpi_get_genport_coordinates(u32 uid, struct access_coordinate *coord) { return -EOPNOTSUPP; } + +static inline struct memory_dev_type *hmat_find_alloc_memory_type(int adist) +{ + return NULL; +} #endif #ifdef CONFIG_ACPI_NUMA diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h index 69e781900082..4bc2596c5774 100644 --- a/include/linux/memory-tiers.h +++ b/include/linux/memory-tiers.h @@ -48,6 +48,9 @@ int mt_calc_adistance(int node, int *adist); int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, const char *source); int mt_perf_to_adistance(struct access_coordinate *perf, int *adist); +struct memory_dev_type *find_alloc_memory_type(int adist, + struct list_head *memory_types); +void memory_tier_late_init(void); #ifdef CONFIG_MIGRATION int next_demotion_node(int node); void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets); @@ -136,5 +139,10 @@ static inline int mt_perf_to_adistance(struct access_coordinate *perf, int *adis { return -EIO; } + +static inline void memory_tier_late_init(void) +{ + +} #endif /* CONFIG_NUMA */ #endif /* _LINUX_MEMORY_TIERS_H */ diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c index 0537664620e5..79f748d60e6f 100644 --- a/mm/memory-tiers.c +++ b/mm/memory-tiers.c @@ -6,6 +6,7 @@ #include <linux/memory.h> #include <linux/memory-tiers.h> #include <linux/notifier.h> +#include <linux/acpi.h> #include "internal.h" @@ -35,6 +36,7 @@ struct node_memory_type_map { }; static DEFINE_MUTEX(memory_tier_lock); +static DEFINE_MUTEX(mt_perf_lock); static LIST_HEAD(memory_tiers); static struct node_memory_type_map node_memory_types[MAX_NUMNODES]; struct memory_dev_type *default_dram_type; @@ -623,6 +625,58 @@ void clear_node_memory_type(int node, struct memory_dev_type *memtype) } EXPORT_SYMBOL_GPL(clear_node_memory_type); +struct memory_dev_type *find_alloc_memory_type(int adist, struct list_head *memory_types) +{ + bool found = false; + struct memory_dev_type *mtype; + + list_for_each_entry(mtype, memory_types, list) { + if (mtype->adistance == adist) { + found = true; + break; + } + } + if (!found) { + mtype = alloc_memory_type(adist); + if (!IS_ERR(mtype)) + list_add(&mtype->list, memory_types); + } + + return mtype; +} +EXPORT_SYMBOL_GPL(find_alloc_memory_type); + +static void memory_tier_late_create(int node) +{ + struct memory_dev_type *mtype = NULL; + int adist = MEMTIER_ADISTANCE_DRAM; + + mt_calc_adistance(node, &adist); + if (adist != MEMTIER_ADISTANCE_DRAM) { + mtype = hmat_find_alloc_memory_type(adist); + if (!IS_ERR(mtype)) + __init_node_memory_type(node, mtype); + else + pr_err("Failed to allocate a memory type at %s()\n", __func__); + } + + set_node_memory_tier(node); +} + +void memory_tier_late_init(void) +{ + int nid; + + mutex_lock(&memory_tier_lock); + for_each_node_state(nid, N_MEMORY) + if (!node_state(nid, N_CPU)) + memory_tier_late_create(nid); + + establish_demotion_targets(); + mutex_unlock(&memory_tier_lock); +} +EXPORT_SYMBOL_GPL(memory_tier_late_init); + static void dump_hmem_attrs(struct access_coordinate *coord, const char *prefix) { pr_info( @@ -636,7 +690,7 @@ int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, { int rc = 0; - mutex_lock(&memory_tier_lock); + mutex_lock(&mt_perf_lock); if (default_dram_perf_error) { rc = -EIO; goto out; @@ -684,7 +738,7 @@ int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, } out: - mutex_unlock(&memory_tier_lock); + mutex_unlock(&mt_perf_lock); return rc; } @@ -700,7 +754,7 @@ int mt_perf_to_adistance(struct access_coordinate *perf, int *adist) perf->read_bandwidth + perf->write_bandwidth == 0) return -EINVAL; - mutex_lock(&memory_tier_lock); + mutex_lock(&mt_perf_lock); /* * The abstract distance of a memory node is in direct proportion to * its memory latency (read + write) and inversely proportional to its @@ -713,7 +767,7 @@ int mt_perf_to_adistance(struct access_coordinate *perf, int *adist) (default_dram_perf.read_latency + default_dram_perf.write_latency) * (default_dram_perf.read_bandwidth + default_dram_perf.write_bandwidth) / (perf->read_bandwidth + perf->write_bandwidth); - mutex_unlock(&memory_tier_lock); + mutex_unlock(&mt_perf_lock); return 0; } @@ -836,6 +890,14 @@ static int __init memory_tier_init(void) * 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 when HMAT information is + * available. + */ + continue; + memtier = set_node_memory_tier(node); if (IS_ERR(memtier)) /*