Re: [PATCH 2/6] cxl: add sysram_region memory controller

[<dan.j.williams@intel.com>]

Gregory Price wrote:
> Add a sysram memctrl that directly hotplugs memory without needing to
> route through DAX.  This simplifies the sysram usecase considerably.
> 
> The sysram memctl adds new sysfs controls when registered:
> 	region/memctrl/[hotplug, hotunplug, state]
> 
> hotplug:   controller attempts to hotplug the memory region
> hotunplug: controller attempts to offline and hotunplug the memory region
> state:     [online,online_normal,offline]
>    online       : controller onlines blocks in ZONE_MOVABLE
>    online_normal: controller onlines blocks in ZONE_NORMAL
>    offline      : controller attempts to offline the memory blocks
> 
> Hotplug note - by default the controller will hotplug the blocks, but
> leave them offline (unless MHP auto-online in Kconfig is enabled).
> 
> Setting state to "online_normal" may prevent future hot-unplug of sysram
> regions, and unbinding a memory region with memory online in ZONE_NORMAL
> may result in the device being removed but the memory remaining online.
> 
> This can result in future management functions failing (such as adding a
> new region).  This is why "online_normal" is explicit, and the default
> online zone is ZONE_MOVABLE.

David's early feedback aligns with my own with respect to not creating
new "online_*" ABI terms, but I want to go a step further.

Part of the proposal here solves a fundamental problem with the way
dax_kmem operates in terms of fixing the complication of dax_kmem
depending on fine grained / multi-step online control via memblock
sysfs.

If we are going to introduce a new omnibus way to online entire regions
at a time then that goodness should first come to dax_kmem and then
potentially be refactored into a library that CXL can use to skip the
device_dax indirection.

I.e. the end result would be this "hotplug" mechanism that fixes a long
standing dax_kmem problem and then go further to drop the indirection
through device_dax and have a "hotplug" mechanism directly at the
cxl_region level.

> +int devm_cxl_add_sysram_region(struct cxl_region *cxlr)
> +{
[..]
> +err_add_group:
> +	dev_set_drvdata(dev, NULL);
> +	/* if this fails, memory cannot be removed from the system until reboot */
> +	remove_memory(range.start, range_len(&range));
> +err_add_memory:
> +	remove_resource(res);
> +	kfree(res);
> +err_request_mem:
> +	memory_group_unregister(data->mgid);
> +err_reg_mgid:
> +	kfree(data->res_name);
> +err_res_name:
> +	kfree(data);
> +err_data:
> +	clear_node_memory_type(numa_node, mtype);
> +	return rc;

...btw, this feels like too many new gotos in the age of
scope-based-cleanup. It also feels like a bunch of duplicated code that
CXL and fixed up dax_kmem can share.