Re: [PATCH v2 19/20] dax: Assign RAM regions to memory-hotplug by default

[Dave Jiang <dave.jiang@xxxxxxxxx>]

On 2/10/23 2:07 AM, Dan Williams wrote:
> The default mode for device-dax instances is backwards for RAM-regions
> as evidenced by the fact that it tends to catch end users by surprise.
> "Where is my memory?". Recall that platforms are increasingly shipping
> with performance-differentiated memory pools beyond typical DRAM and
> NUMA effects. This includes HBM (high-bandwidth-memory) and CXL (dynamic
> interleave, varied media types, and future fabric attached
> possibilities).
>
> For this reason the EFI_MEMORY_SP (EFI Special Purpose Memory => Linux
> 'Soft Reserved') attribute is expected to be applied to all memory-pools
> that are not the general purpose pool. This designation gives an
> Operating System a chance to defer usage of a memory pool until later in
> the boot process where its performance properties can be interrogated
> and administrator policy can be applied.
>
> 'Soft Reserved' memory can be anything from too limited and precious to
> be part of the general purpose pool (HBM), too slow to host hot kernel
> data structures (some PMEM media), or anything in between. However, in
> the absence of an explicit policy, the memory should at least be made
> usable by default. The current device-dax default hides all
> non-general-purpose memory behind a device interface.
>
> The expectation is that the distribution of users that want the memory
> online by default vs device-dedicated-access by default follows the
> Pareto principle. A small number of enlightened users may want to do
> userspace memory management through a device, but general users just
> want the kernel to make the memory available with an option to get more
> advanced later.
>
> Arrange for all device-dax instances not backed by PMEM to default to
> attaching to the dax_kmem driver. From there the baseline memory hotplug
> policy (CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE / memhp_default_state=)
> gates whether the memory comes online or stays offline. Where, if it
> stays offline, it can be reliably converted back to device-mode where it
> can be partitioned, or fronted by a userspace allocator.
>
> So, if someone wants device-dax instances for their 'Soft Reserved'
> memory:
>
> 1/ Build a kernel with CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE=n or boot
>     with memhp_default_state=offline, or roll the dice and hope that the
>     kernel has not pinned a page in that memory before step 2.
>
> 2/ Write a udev rule to convert the target dax device(s) from
>     'system-ram' mode to 'devdax' mode:
>
>     daxctl reconfigure-device $dax -m devdax -f
>
> Cc: Michal Hocko <mhocko@xxxxxxxx>
> Cc: David Hildenbrand <david@xxxxxxxxxx>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> Reviewed-by: Gregory Price <gregory.price@xxxxxxxxxxxx>
> Tested-by: Fan Ni <fan.ni@xxxxxxxxxxx>
> Link: https://lore.kernel.org/r/167564544513.847146.4645646177864365755.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>

Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>

> ---
>   drivers/dax/Kconfig     |    2 +-
>   drivers/dax/bus.c       |   53 ++++++++++++++++++++---------------------------
>   drivers/dax/bus.h       |   12 +++++++++--
>   drivers/dax/device.c    |    3 +--
>   drivers/dax/hmem/hmem.c |   12 ++++++++++-
>   drivers/dax/kmem.c      |    1 +
>   6 files changed, 46 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/dax/Kconfig b/drivers/dax/Kconfig
> index d13c889c2a64..1163eb62e5f6 100644
> --- a/drivers/dax/Kconfig
> +++ b/drivers/dax/Kconfig
> @@ -50,7 +50,7 @@ config DEV_DAX_HMEM_DEVICES
>   	def_bool y
>   
>   config DEV_DAX_KMEM
> -	tristate "KMEM DAX: volatile-use of persistent memory"
> +	tristate "KMEM DAX: map dax-devices as System-RAM"
>   	default DEV_DAX
>   	depends on DEV_DAX
>   	depends on MEMORY_HOTPLUG # for add_memory() and friends
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index 1dad813ee4a6..012d576004e9 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -56,6 +56,25 @@ static int dax_match_id(struct dax_device_driver *dax_drv, struct device *dev)
>   	return match;
>   }
>   
> +static int dax_match_type(struct dax_device_driver *dax_drv, struct device *dev)
> +{
> +	enum dax_driver_type type = DAXDRV_DEVICE_TYPE;
> +	struct dev_dax *dev_dax = to_dev_dax(dev);
> +
> +	if (dev_dax->region->res.flags & IORESOURCE_DAX_KMEM)
> +		type = DAXDRV_KMEM_TYPE;
> +
> +	if (dax_drv->type == type)
> +		return 1;
> +
> +	/* default to device mode if dax_kmem is disabled */
> +	if (dax_drv->type == DAXDRV_DEVICE_TYPE &&
> +	    !IS_ENABLED(CONFIG_DEV_DAX_KMEM))
> +		return 1;
> +
> +	return 0;
> +}
> +
>   enum id_action {
>   	ID_REMOVE,
>   	ID_ADD,
> @@ -216,14 +235,9 @@ static int dax_bus_match(struct device *dev, struct device_driver *drv)
>   {
>   	struct dax_device_driver *dax_drv = to_dax_drv(drv);
>   
> -	/*
> -	 * All but the 'device-dax' driver, which has 'match_always'
> -	 * set, requires an exact id match.
> -	 */
> -	if (dax_drv->match_always)
> +	if (dax_match_id(dax_drv, dev))
>   		return 1;
> -
> -	return dax_match_id(dax_drv, dev);
> +	return dax_match_type(dax_drv, dev);
>   }
>   
>   /*
> @@ -1413,13 +1427,10 @@ struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data)
>   }
>   EXPORT_SYMBOL_GPL(devm_create_dev_dax);
>   
> -static int match_always_count;
> -
>   int __dax_driver_register(struct dax_device_driver *dax_drv,
>   		struct module *module, const char *mod_name)
>   {
>   	struct device_driver *drv = &dax_drv->drv;
> -	int rc = 0;
>   
>   	/*
>   	 * dax_bus_probe() calls dax_drv->probe() unconditionally.
> @@ -1434,26 +1445,7 @@ int __dax_driver_register(struct dax_device_driver *dax_drv,
>   	drv->mod_name = mod_name;
>   	drv->bus = &dax_bus_type;
>   
> -	/* there can only be one default driver */
> -	mutex_lock(&dax_bus_lock);
> -	match_always_count += dax_drv->match_always;
> -	if (match_always_count > 1) {
> -		match_always_count--;
> -		WARN_ON(1);
> -		rc = -EINVAL;
> -	}
> -	mutex_unlock(&dax_bus_lock);
> -	if (rc)
> -		return rc;
> -
> -	rc = driver_register(drv);
> -	if (rc && dax_drv->match_always) {
> -		mutex_lock(&dax_bus_lock);
> -		match_always_count -= dax_drv->match_always;
> -		mutex_unlock(&dax_bus_lock);
> -	}
> -
> -	return rc;
> +	return driver_register(drv);
>   }
>   EXPORT_SYMBOL_GPL(__dax_driver_register);
>   
> @@ -1463,7 +1455,6 @@ void dax_driver_unregister(struct dax_device_driver *dax_drv)
>   	struct dax_id *dax_id, *_id;
>   
>   	mutex_lock(&dax_bus_lock);
> -	match_always_count -= dax_drv->match_always;
>   	list_for_each_entry_safe(dax_id, _id, &dax_drv->ids, list) {
>   		list_del(&dax_id->list);
>   		kfree(dax_id);
> diff --git a/drivers/dax/bus.h b/drivers/dax/bus.h
> index fbb940293d6d..8cd79ab34292 100644
> --- a/drivers/dax/bus.h
> +++ b/drivers/dax/bus.h
> @@ -11,7 +11,10 @@ struct dax_device;
>   struct dax_region;
>   void dax_region_put(struct dax_region *dax_region);
>   
> -#define IORESOURCE_DAX_STATIC (1UL << 0)
> +/* dax bus specific ioresource flags */
> +#define IORESOURCE_DAX_STATIC BIT(0)
> +#define IORESOURCE_DAX_KMEM BIT(1)
> +
>   struct dax_region *alloc_dax_region(struct device *parent, int region_id,
>   		struct range *range, int target_node, unsigned int align,
>   		unsigned long flags);
> @@ -25,10 +28,15 @@ struct dev_dax_data {
>   
>   struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data);
>   
> +enum dax_driver_type {
> +	DAXDRV_KMEM_TYPE,
> +	DAXDRV_DEVICE_TYPE,
> +};
> +
>   struct dax_device_driver {
>   	struct device_driver drv;
>   	struct list_head ids;
> -	int match_always;
> +	enum dax_driver_type type;
>   	int (*probe)(struct dev_dax *dev);
>   	void (*remove)(struct dev_dax *dev);
>   };
> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> index 5494d745ced5..ecdff79e31f2 100644
> --- a/drivers/dax/device.c
> +++ b/drivers/dax/device.c
> @@ -475,8 +475,7 @@ EXPORT_SYMBOL_GPL(dev_dax_probe);
>   
>   static struct dax_device_driver device_dax_driver = {
>   	.probe = dev_dax_probe,
> -	/* all probe actions are unwound by devm, so .remove isn't necessary */
> -	.match_always = 1,
> +	.type = DAXDRV_DEVICE_TYPE,
>   };
>   
>   static int __init dax_init(void)
> diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
> index e7bdff3132fa..5ec08f9f8a57 100644
> --- a/drivers/dax/hmem/hmem.c
> +++ b/drivers/dax/hmem/hmem.c
> @@ -11,15 +11,25 @@ module_param_named(region_idle, region_idle, bool, 0644);
>   
>   static int dax_hmem_probe(struct platform_device *pdev)
>   {
> +	unsigned long flags = IORESOURCE_DAX_KMEM;
>   	struct device *dev = &pdev->dev;
>   	struct dax_region *dax_region;
>   	struct memregion_info *mri;
>   	struct dev_dax_data data;
>   	struct dev_dax *dev_dax;
>   
> +	/*
> +	 * @region_idle == true indicates that an administrative agent
> +	 * wants to manipulate the range partitioning before the devices
> +	 * are created, so do not send them to the dax_kmem driver by
> +	 * default.
> +	 */
> +	if (region_idle)
> +		flags = 0;
> +
>   	mri = dev->platform_data;
>   	dax_region = alloc_dax_region(dev, pdev->id, &mri->range,
> -				      mri->target_node, PMD_SIZE, 0);
> +				      mri->target_node, PMD_SIZE, flags);
>   	if (!dax_region)
>   		return -ENOMEM;
>   
> diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
> index 4852a2dbdb27..918d01d3fbaa 100644
> --- a/drivers/dax/kmem.c
> +++ b/drivers/dax/kmem.c
> @@ -239,6 +239,7 @@ static void dev_dax_kmem_remove(struct dev_dax *dev_dax)
>   static struct dax_device_driver device_dax_kmem_driver = {
>   	.probe = dev_dax_kmem_probe,
>   	.remove = dev_dax_kmem_remove,
> +	.type = DAXDRV_KMEM_TYPE,
>   };
>   
>   static int __init dax_kmem_init(void)