Alistair Popple <apopple@xxxxxxxxxx> writes:
> Huang Ying <ying.huang@xxxxxxxxx> writes:
>
>> Previously, a fixed abstract distance MEMTIER_DEFAULT_DAX_ADISTANCE is
>> used for slow memory type in kmem driver. This limits the usage of
>> kmem driver, for example, it cannot be used for HBM (high bandwidth
>> memory).
>>
>> So, we use the general abstract distance calculation mechanism in kmem
>> drivers to get more accurate abstract distance on systems with proper
>> support. The original MEMTIER_DEFAULT_DAX_ADISTANCE is used as
>> fallback only.
>>
>> Now, multiple memory types may be managed by kmem. These memory types
>> are put into the "kmem_memory_types" list and protected by
>> kmem_memory_type_lock.
>
> See below but I wonder if kmem_memory_types could be a common helper
> rather than kdax specific?
>
>> Signed-off-by: "Huang, Ying" <ying.huang@xxxxxxxxx>
>> Cc: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxx>
>> Cc: Wei Xu <weixugc@xxxxxxxxxx>
>> Cc: Alistair Popple <apopple@xxxxxxxxxx>
>> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
>> Cc: Dave Hansen <dave.hansen@xxxxxxxxx>
>> Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx>
>> Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
>> Cc: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>> Cc: Michal Hocko <mhocko@xxxxxxxxxx>
>> Cc: Yang Shi <shy828301@xxxxxxxxx>
>> Cc: Rafael J Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> ---
>> drivers/dax/kmem.c | 54 +++++++++++++++++++++++++++---------
>> include/linux/memory-tiers.h | 2 ++
>> mm/memory-tiers.c | 2 +-
>> 3 files changed, 44 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
>> index 898ca9505754..837165037231 100644
>> --- a/drivers/dax/kmem.c
>> +++ b/drivers/dax/kmem.c
>> @@ -49,14 +49,40 @@ struct dax_kmem_data {
>> struct resource *res[];
>> };
>>
>> -static struct memory_dev_type *dax_slowmem_type;
>> +static DEFINE_MUTEX(kmem_memory_type_lock);
>> +static LIST_HEAD(kmem_memory_types);
>> +
>> +static struct memory_dev_type *kmem_find_alloc_memorty_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);
>> + }
>> + mutex_unlock(&kmem_memory_type_lock);
>> +
>> + return mtype;
>> +}
>> +
>> static int dev_dax_kmem_probe(struct dev_dax *dev_dax)
>> {
>> struct device *dev = &dev_dax->dev;
>> unsigned long total_len = 0;
>> struct dax_kmem_data *data;
>> + struct memory_dev_type *mtype;
>> int i, rc, mapped = 0;
>> int numa_node;
>> + int adist = MEMTIER_DEFAULT_DAX_ADISTANCE;
>>
>> /*
>> * Ensure good NUMA information for the persistent memory.
>> @@ -71,6 +97,11 @@ static int dev_dax_kmem_probe(struct dev_dax *dev_dax)
>> return -EINVAL;
>> }
>>
>> + mt_calc_adistance(numa_node, &adist);
>> + mtype = kmem_find_alloc_memorty_type(adist);
>> + if (IS_ERR(mtype))
>> + return PTR_ERR(mtype);
>> +
>
> I wrote my own quick and dirty module to test this and wrote basically
> the same code sequence.
>
> I notice your using a list of memory types here though. I think it would
> be nice to have a common helper that other users could call to do the
> mt_calc_adistance() / kmem_find_alloc_memory_type() /
> init_node_memory_type() sequence and cleanup as my naive approach would
> result in a new memory_dev_type per device even though adist might be
> the same. A common helper would make it easy to de-dup those.
If it's useful, we can move kmem_find_alloc_memory_type() to
memory-tier.c after some revision. But I tend to move it after we have
the second user. What do you think about that?
--
Best Regards,
Huang, Ying
>> for (i = 0; i < dev_dax->nr_range; i++) {
>> struct range range;
>>
>> @@ -88,7 +119,7 @@ static int dev_dax_kmem_probe(struct dev_dax *dev_dax)
>> return -EINVAL;
>> }
>>
>> - init_node_memory_type(numa_node, dax_slowmem_type);
>> + init_node_memory_type(numa_node, mtype);
>>
>> rc = -ENOMEM;
>> data = kzalloc(struct_size(data, res, dev_dax->nr_range), GFP_KERNEL);
>> @@ -167,7 +198,7 @@ static int dev_dax_kmem_probe(struct dev_dax *dev_dax)
>> err_res_name:
>> kfree(data);
>> err_dax_kmem_data:
>> - clear_node_memory_type(numa_node, dax_slowmem_type);
>> + clear_node_memory_type(numa_node, mtype);
>> return rc;
>> }
>>
>> @@ -219,7 +250,7 @@ static void dev_dax_kmem_remove(struct dev_dax *dev_dax)
>> * for that. This implies this reference will be around
>> * till next reboot.
>> */
>> - clear_node_memory_type(node, dax_slowmem_type);
>> + clear_node_memory_type(node, NULL);
>> }
>> }
>> #else
>> @@ -251,12 +282,6 @@ static int __init dax_kmem_init(void)
>> if (!kmem_name)
>> return -ENOMEM;
>>
>> - dax_slowmem_type = alloc_memory_type(MEMTIER_DEFAULT_DAX_ADISTANCE);
>> - if (IS_ERR(dax_slowmem_type)) {
>> - rc = PTR_ERR(dax_slowmem_type);
>> - goto err_dax_slowmem_type;
>> - }
>> -
>> rc = dax_driver_register(&device_dax_kmem_driver);
>> if (rc)
>> goto error_dax_driver;
>> @@ -264,18 +289,21 @@ static int __init dax_kmem_init(void)
>> return rc;
>>
>> error_dax_driver:
>> - destroy_memory_type(dax_slowmem_type);
>> -err_dax_slowmem_type:
>> kfree_const(kmem_name);
>> return rc;
>> }
>>
>> static void __exit dax_kmem_exit(void)
>> {
>> + struct memory_dev_type *mtype, *mtn;
>> +
>> dax_driver_unregister(&device_dax_kmem_driver);
>> if (!any_hotremove_failed)
>> kfree_const(kmem_name);
>> - destroy_memory_type(dax_slowmem_type);
>> + list_for_each_entry_safe(mtype, mtn, &kmem_memory_types, list) {
>> + list_del(&mtype->list);
>> + destroy_memory_type(mtype);
>> + }
>> }
>>
>> MODULE_AUTHOR("Intel Corporation");
>> diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h
>> index 9377239c8d34..aca22220cb5c 100644
>> --- a/include/linux/memory-tiers.h
>> +++ b/include/linux/memory-tiers.h
>> @@ -24,6 +24,8 @@ struct memory_tier;
>> struct memory_dev_type {
>> /* list of memory types that are part of same tier as this type */
>> struct list_head tier_sibiling;
>> + /* list of memory types that are managed by one driver */
>> + struct list_head list;
>> /* abstract distance for this specific memory type */
>> int adistance;
>> /* Nodes of same abstract distance */
>> diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c
>> index 9a734ef2edfb..38005c60fa2d 100644
>> --- a/mm/memory-tiers.c
>> +++ b/mm/memory-tiers.c
>> @@ -581,7 +581,7 @@ EXPORT_SYMBOL_GPL(init_node_memory_type);
>> void clear_node_memory_type(int node, struct memory_dev_type *memtype)
>> {
>> mutex_lock(&memory_tier_lock);
>> - if (node_memory_types[node].memtype == memtype)
>> + if (node_memory_types[node].memtype == memtype || !memtype)
>> node_memory_types[node].map_count--;
>> /*
>> * If we umapped all the attached devices to this node,
