diff mbox series

[NDCTL,v2] cxl/region: Add -f option for disable-region

Message ID 169525064907.3085225.2583864429793298106.stgit@djiang5-mobl3
State Superseded
Commit c2b28b8ae4256f9ed076c65c18c34d05b275c07c
Headers show
Series [NDCTL,v2] cxl/region: Add -f option for disable-region | expand

Commit Message

Dave Jiang Sept. 20, 2023, 10:57 p.m. UTC
The current operation for disable_region does not check if the memory
covered by a region is online before attempting to disable the cxl region.
Provide a -f option for the region to force offlining of currently online
memory before disabling the region. Also add a check to fail the operation
entirely if the memory is non-movable.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>

---
v2:
- Update documentation and help output. (Vishal)
---
 Documentation/cxl/cxl-disable-region.txt |    7 ++++
 cxl/region.c                             |   49 +++++++++++++++++++++++++++++-
 2 files changed, 55 insertions(+), 1 deletion(-)

Comments

Zhijian Li (Fujitsu) Sept. 21, 2023, 2:58 a.m. UTC | #1
Dave,

Forgive me for not having a new thread, I'd ask a possible relevant questions about disable-memdev
We noticed that only -f is implemented for disable-memdev, and it left a
"TODO: actually detect rather than assume active" in cxl/memdev.c.

My questions are:
1. Does the *active* here mean the region(the memdev belongs to) is active ?
2. Is the without force method under developing ?

My colleagues(in CC's) are investigating how to gracefully disable-memdev

Thanks
Zhijian

On 21/09/2023 06:57, Dave Jiang wrote:
> The current operation for disable_region does not check if the memory
> covered by a region is online before attempting to disable the cxl region.
> Provide a -f option for the region to force offlining of currently online
> memory before disabling the region. Also add a check to fail the operation
> entirely if the memory is non-movable.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> ---
> v2:
> - Update documentation and help output. (Vishal)
> ---
>   Documentation/cxl/cxl-disable-region.txt |    7 ++++
>   cxl/region.c                             |   49 +++++++++++++++++++++++++++++-
>   2 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/cxl/cxl-disable-region.txt b/Documentation/cxl/cxl-disable-region.txt
> index 6a39aee6ea69..9b98d4d8745a 100644
> --- a/Documentation/cxl/cxl-disable-region.txt
> +++ b/Documentation/cxl/cxl-disable-region.txt
> @@ -25,6 +25,13 @@ OPTIONS
>   -------
>   include::bus-option.txt[]
>   
> +-f::
> +--force::
> +	Attempt to offline any memory that has been hot-added into the system
> +	via the CXL region before disabling the region. This won't be attempted
> +	if the memory was not added as 'movable', and may still fail even if it
> +	was movable.
> +
>   include::decoder-option.txt[]
>   
>   include::debug-option.txt[]
> diff --git a/cxl/region.c b/cxl/region.c
> index bcd703956207..f8303869727a 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -14,6 +14,7 @@
>   #include <util/parse-options.h>
>   #include <ccan/minmax/minmax.h>
>   #include <ccan/short_types/short_types.h>
> +#include <daxctl/libdaxctl.h>
>   
>   #include "filter.h"
>   #include "json.h"
> @@ -95,6 +96,8 @@ static const struct option enable_options[] = {
>   
>   static const struct option disable_options[] = {
>   	BASE_OPTIONS(),
> +	OPT_BOOLEAN('f', "force", &param.force,
> +		    "attempt to offline memory before disabling the region"),
>   	OPT_END(),
>   };
>   
> @@ -789,13 +792,57 @@ static int destroy_region(struct cxl_region *region)
>   	return cxl_region_delete(region);
>   }
>   
> +static int disable_region(struct cxl_region *region)
> +{
> +	const char *devname = cxl_region_get_devname(region);
> +	struct daxctl_region *dax_region;
> +	struct daxctl_memory *mem;
> +	struct daxctl_dev *dev;
> +	int rc;
> +
> +	dax_region = cxl_region_get_daxctl_region(region);
> +	if (!dax_region)
> +		goto out;
> +
> +	daxctl_dev_foreach(dax_region, dev) {
> +		mem = daxctl_dev_get_memory(dev);
> +		if (!mem)
> +			return -ENXIO;
> +
> +		if (daxctl_memory_online_no_movable(mem)) {
> +			log_err(&rl, "%s: memory unmovable for %s\n",
> +					devname,
> +					daxctl_dev_get_devname(dev));
> +			return -EPERM;
> +		}
> +
> +		/*
> +		 * If memory is still online and user wants to force it, attempt
> +		 * to offline it.
> +		 */
> +		if (daxctl_memory_is_online(mem) && param.force) {
> +			rc = daxctl_memory_offline(mem);
> +			if (rc) {
> +				log_err(&rl, "%s: unable to offline %s: %s\n",
> +					devname,
> +					daxctl_dev_get_devname(dev),
> +					strerror(abs(rc)));
> +				return rc;
> +			}
> +		}
> +	}
> +
> +out:
> +	return cxl_region_disable(region);
> +}
> +
>   static int do_region_xable(struct cxl_region *region, enum region_actions action)
>   {
>   	switch (action) {
>   	case ACTION_ENABLE:
>   		return cxl_region_enable(region);
>   	case ACTION_DISABLE:
> -		return cxl_region_disable(region);
> +		return disable_region(region);
>   	case ACTION_DESTROY:
>   		return destroy_region(region);
>   	default:
> 
>
Dave Jiang Sept. 21, 2023, 11:19 p.m. UTC | #2
On 9/20/23 19:58, Zhijian Li (Fujitsu) wrote:
> Dave,
> 
> Forgive me for not having a new thread, I'd ask a possible relevant questions about disable-memdev
> We noticed that only -f is implemented for disable-memdev, and it left a
> "TODO: actually detect rather than assume active" in cxl/memdev.c.
> 
> My questions are:
> 1. Does the *active* here mean the region(the memdev belongs to) is active ?
> 2. Is the without force method under developing ?
> 
> My colleagues(in CC's) are investigating how to gracefully disable-memdev

Zhijian,
So this was there before the region enumeration showed up according to Dan. Now an update to check if the memdev is part of any active region should be added. Either you guys can send a patch or I can go added it. Let me know. Thanks!


> 
> Thanks
> Zhijian
> 
> On 21/09/2023 06:57, Dave Jiang wrote:
>> The current operation for disable_region does not check if the memory
>> covered by a region is online before attempting to disable the cxl region.
>> Provide a -f option for the region to force offlining of currently online
>> memory before disabling the region. Also add a check to fail the operation
>> entirely if the memory is non-movable.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>
>> ---
>> v2:
>> - Update documentation and help output. (Vishal)
>> ---
>>   Documentation/cxl/cxl-disable-region.txt |    7 ++++
>>   cxl/region.c                             |   49 +++++++++++++++++++++++++++++-
>>   2 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/cxl/cxl-disable-region.txt b/Documentation/cxl/cxl-disable-region.txt
>> index 6a39aee6ea69..9b98d4d8745a 100644
>> --- a/Documentation/cxl/cxl-disable-region.txt
>> +++ b/Documentation/cxl/cxl-disable-region.txt
>> @@ -25,6 +25,13 @@ OPTIONS
>>   -------
>>   include::bus-option.txt[]
>>   
>> +-f::
>> +--force::
>> +	Attempt to offline any memory that has been hot-added into the system
>> +	via the CXL region before disabling the region. This won't be attempted
>> +	if the memory was not added as 'movable', and may still fail even if it
>> +	was movable.
>> +
>>   include::decoder-option.txt[]
>>   
>>   include::debug-option.txt[]
>> diff --git a/cxl/region.c b/cxl/region.c
>> index bcd703956207..f8303869727a 100644
>> --- a/cxl/region.c
>> +++ b/cxl/region.c
>> @@ -14,6 +14,7 @@
>>   #include <util/parse-options.h>
>>   #include <ccan/minmax/minmax.h>
>>   #include <ccan/short_types/short_types.h>
>> +#include <daxctl/libdaxctl.h>
>>   
>>   #include "filter.h"
>>   #include "json.h"
>> @@ -95,6 +96,8 @@ static const struct option enable_options[] = {
>>   
>>   static const struct option disable_options[] = {
>>   	BASE_OPTIONS(),
>> +	OPT_BOOLEAN('f', "force", &param.force,
>> +		    "attempt to offline memory before disabling the region"),
>>   	OPT_END(),
>>   };
>>   
>> @@ -789,13 +792,57 @@ static int destroy_region(struct cxl_region *region)
>>   	return cxl_region_delete(region);
>>   }
>>   
>> +static int disable_region(struct cxl_region *region)
>> +{
>> +	const char *devname = cxl_region_get_devname(region);
>> +	struct daxctl_region *dax_region;
>> +	struct daxctl_memory *mem;
>> +	struct daxctl_dev *dev;
>> +	int rc;
>> +
>> +	dax_region = cxl_region_get_daxctl_region(region);
>> +	if (!dax_region)
>> +		goto out;
>> +
>> +	daxctl_dev_foreach(dax_region, dev) {
>> +		mem = daxctl_dev_get_memory(dev);
>> +		if (!mem)
>> +			return -ENXIO;
>> +
>> +		if (daxctl_memory_online_no_movable(mem)) {
>> +			log_err(&rl, "%s: memory unmovable for %s\n",
>> +					devname,
>> +					daxctl_dev_get_devname(dev));
>> +			return -EPERM;
>> +		}
>> +
>> +		/*
>> +		 * If memory is still online and user wants to force it, attempt
>> +		 * to offline it.
>> +		 */
>> +		if (daxctl_memory_is_online(mem) && param.force) {
>> +			rc = daxctl_memory_offline(mem);
>> +			if (rc) {
>> +				log_err(&rl, "%s: unable to offline %s: %s\n",
>> +					devname,
>> +					daxctl_dev_get_devname(dev),
>> +					strerror(abs(rc)));
>> +				return rc;
>> +			}
>> +		}
>> +	}
>> +
>> +out:
>> +	return cxl_region_disable(region);
>> +}
>> +
>>   static int do_region_xable(struct cxl_region *region, enum region_actions action)
>>   {
>>   	switch (action) {
>>   	case ACTION_ENABLE:
>>   		return cxl_region_enable(region);
>>   	case ACTION_DISABLE:
>> -		return cxl_region_disable(region);
>> +		return disable_region(region);
>>   	case ACTION_DESTROY:
>>   		return destroy_region(region);
>>   	default:
>>
Zhijian Li (Fujitsu) Sept. 22, 2023, 1:26 a.m. UTC | #3
On 22/09/2023 07:19, Dave Jiang wrote:
> 
> 
> On 9/20/23 19:58, Zhijian Li (Fujitsu) wrote:
>> Dave,
>>
>> Forgive me for not having a new thread, I'd ask a possible relevant questions about disable-memdev
>> We noticed that only -f is implemented for disable-memdev, and it left a
>> "TODO: actually detect rather than assume active" in cxl/memdev.c.
>>
>> My questions are:
>> 1. Does the *active* here mean the region(the memdev belongs to) is active ?
>> 2. Is the without force method under developing ?
>>
>> My colleagues(in CC's) are investigating how to gracefully disable-memdev
> 
> Zhijian,
> So this was there before the region enumeration showed up according to Dan. Now an update to check if the memdev is part of any active region should be added. Either you guys can send a patch or I can go added it. Let me know. Thanks!

Understood, thanks for your information, please go ahead :)
I believe our guys are willing to test it.

Thanks
Zhijian

> 
> 
>>
>> Thanks
>> Zhijian
>>
>> On 21/09/2023 06:57, Dave Jiang wrote:
>>> The current operation for disable_region does not check if the memory
>>> covered by a region is online before attempting to disable the cxl region.
>>> Provide a -f option for the region to force offlining of currently online
>>> memory before disabling the region. Also add a check to fail the operation
>>> entirely if the memory is non-movable.
>>>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>
>>> ---
>>> v2:
>>> - Update documentation and help output. (Vishal)
>>> ---
>>>    Documentation/cxl/cxl-disable-region.txt |    7 ++++
>>>    cxl/region.c                             |   49 +++++++++++++++++++++++++++++-
>>>    2 files changed, 55 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/cxl/cxl-disable-region.txt b/Documentation/cxl/cxl-disable-region.txt
>>> index 6a39aee6ea69..9b98d4d8745a 100644
>>> --- a/Documentation/cxl/cxl-disable-region.txt
>>> +++ b/Documentation/cxl/cxl-disable-region.txt
>>> @@ -25,6 +25,13 @@ OPTIONS
>>>    -------
>>>    include::bus-option.txt[]
>>>    
>>> +-f::
>>> +--force::
>>> +	Attempt to offline any memory that has been hot-added into the system
>>> +	via the CXL region before disabling the region. This won't be attempted
>>> +	if the memory was not added as 'movable', and may still fail even if it
>>> +	was movable.
>>> +
>>>    include::decoder-option.txt[]
>>>    
>>>    include::debug-option.txt[]
>>> diff --git a/cxl/region.c b/cxl/region.c
>>> index bcd703956207..f8303869727a 100644
>>> --- a/cxl/region.c
>>> +++ b/cxl/region.c
>>> @@ -14,6 +14,7 @@
>>>    #include <util/parse-options.h>
>>>    #include <ccan/minmax/minmax.h>
>>>    #include <ccan/short_types/short_types.h>
>>> +#include <daxctl/libdaxctl.h>
>>>    
>>>    #include "filter.h"
>>>    #include "json.h"
>>> @@ -95,6 +96,8 @@ static const struct option enable_options[] = {
>>>    
>>>    static const struct option disable_options[] = {
>>>    	BASE_OPTIONS(),
>>> +	OPT_BOOLEAN('f', "force", &param.force,
>>> +		    "attempt to offline memory before disabling the region"),
>>>    	OPT_END(),
>>>    };
>>>    
>>> @@ -789,13 +792,57 @@ static int destroy_region(struct cxl_region *region)
>>>    	return cxl_region_delete(region);
>>>    }
>>>    
>>> +static int disable_region(struct cxl_region *region)
>>> +{
>>> +	const char *devname = cxl_region_get_devname(region);
>>> +	struct daxctl_region *dax_region;
>>> +	struct daxctl_memory *mem;
>>> +	struct daxctl_dev *dev;
>>> +	int rc;
>>> +
>>> +	dax_region = cxl_region_get_daxctl_region(region);
>>> +	if (!dax_region)
>>> +		goto out;
>>> +
>>> +	daxctl_dev_foreach(dax_region, dev) {
>>> +		mem = daxctl_dev_get_memory(dev);
>>> +		if (!mem)
>>> +			return -ENXIO;
>>> +
>>> +		if (daxctl_memory_online_no_movable(mem)) {
>>> +			log_err(&rl, "%s: memory unmovable for %s\n",
>>> +					devname,
>>> +					daxctl_dev_get_devname(dev));
>>> +			return -EPERM;
>>> +		}
>>> +
>>> +		/*
>>> +		 * If memory is still online and user wants to force it, attempt
>>> +		 * to offline it.
>>> +		 */
>>> +		if (daxctl_memory_is_online(mem) && param.force) {
>>> +			rc = daxctl_memory_offline(mem);
>>> +			if (rc) {
>>> +				log_err(&rl, "%s: unable to offline %s: %s\n",
>>> +					devname,
>>> +					daxctl_dev_get_devname(dev),
>>> +					strerror(abs(rc)));
>>> +				return rc;
>>> +			}
>>> +		}
>>> +	}
>>> +
>>> +out:
>>> +	return cxl_region_disable(region);
>>> +}
>>> +
>>>    static int do_region_xable(struct cxl_region *region, enum region_actions action)
>>>    {
>>>    	switch (action) {
>>>    	case ACTION_ENABLE:
>>>    		return cxl_region_enable(region);
>>>    	case ACTION_DISABLE:
>>> -		return cxl_region_disable(region);
>>> +		return disable_region(region);
>>>    	case ACTION_DESTROY:
>>>    		return destroy_region(region);
>>>    	default:
>>>
Xiao Yang Oct. 9, 2023, 10:52 a.m. UTC | #4
On 2023/9/21 6:57, Dave Jiang wrote:
> +		if (daxctl_memory_online_no_movable(mem)) {
> +			log_err(&rl, "%s: memory unmovable for %s\n",
> +					devname,
> +					daxctl_dev_get_devname(dev));
> +			return -EPERM;
> +		}
Hi Dave,

It seems wrong to check if memory is unmovable by the return number of 
daxctl_memory_online_no_movable(mem) here. IIRC, the return number of 
daxctl_memory_online_no_movable(mem)/daxctl_memory_op(MEM_GET_ZONE) 
indicates how many memory blocks have the same memory zone. So I think 
you should check mem->zone and MEM_ZONE_NORMAL as 
daxctl_memory_is_movable() did.

Besides, I send a patch to improve the implementation of 
daxctl_memory_online_with_zone().
https://lore.kernel.org/nvdimm/20231009103521.1463-1-yangx.jy@fujitsu.com/T/#u

Best Regards,
Xiao Yang
Dave Jiang Oct. 13, 2023, 10:38 p.m. UTC | #5
On 10/9/23 03:52, Xiao Yang wrote:
> On 2023/9/21 6:57, Dave Jiang wrote:
>> +        if (daxctl_memory_online_no_movable(mem)) {
>> +            log_err(&rl, "%s: memory unmovable for %s\n",
>> +                    devname,
>> +                    daxctl_dev_get_devname(dev));
>> +            return -EPERM;
>> +        }
> Hi Dave,
> 
> It seems wrong to check if memory is unmovable by the return number of daxctl_memory_online_no_movable(mem) here. IIRC, the return number of daxctl_memory_online_no_movable(mem)/daxctl_memory_op(MEM_GET_ZONE) indicates how many memory blocks have the same memory zone. So I think you should check mem->zone and MEM_ZONE_NORMAL as daxctl_memory_is_movable() did.

Do you mean:
rc = daxctl_memory_online_no_movable(mem);
if (rc < 0)
	return rc;
if (rc > 0) {
	log_err(&rl, "%s memory unmovable for %s\n' ...);
	return -EPERM;
}

> 
> Besides, I send a patch to improve the implementation of daxctl_memory_online_with_zone().
> https://lore.kernel.org/nvdimm/20231009103521.1463-1-yangx.jy@fujitsu.com/T/#u

Thanks. I added my review tag.

> 
> Best Regards,
> Xiao Yang
Xiao Yang Oct. 30, 2023, 4:33 a.m. UTC | #6
On 2023/10/14 6:38, Dave Jiang wrote:
> 
> On 10/9/23 03:52, Xiao Yang wrote:
>> On 2023/9/21 6:57, Dave Jiang wrote:
>>> +        if (daxctl_memory_online_no_movable(mem)) {
>>> +            log_err(&rl, "%s: memory unmovable for %s\n",
>>> +                    devname,
>>> +                    daxctl_dev_get_devname(dev));
>>> +            return -EPERM;
>>> +        }
>> Hi Dave,
>>
>> It seems wrong to check if memory is unmovable by the return number of daxctl_memory_online_no_movable(mem) here. IIRC, the return number of daxctl_memory_online_no_movable(mem)/daxctl_memory_op(MEM_GET_ZONE) indicates how many memory blocks have the same memory zone. So I think you should check mem->zone and MEM_ZONE_NORMAL as daxctl_memory_is_movable() did.
> Do you mean:
> rc = daxctl_memory_online_no_movable(mem);
> if (rc < 0)
> 	return rc;
> if (rc > 0) {
> 	log_err(&rl, "%s memory unmovable for %s\n' ...);
> 	return -EPERM;
> }
> 
Hi Dave,

Sorry for the late reply.

Is it necessary to try to online the memory region to the 
MEM_ZONE_NORMAL by daxctl_memory_online_no_movable(mem)? If you just 
want to check if the onlined memory region is in the MEM_ZONE_NORMAL, 
the following code seems better:
     mem->zone = 0;
     rc = daxctl_memory_op(mem, MEM_GET_ZONE);
     if (rc < 0)
         return rc;
     if (mem->zone == MEM_ZONE_NORMAL) {
         log_err(&rl, "%s memory unmovable for %s\n' ...);
	return -EPERM;
     }

Best Regards,
Xiao Yang
Dave Jiang Oct. 30, 2023, 4:24 p.m. UTC | #7
On 10/29/23 21:33, Xiao Yang wrote:
> On 2023/10/14 6:38, Dave Jiang wrote:
>>
>> On 10/9/23 03:52, Xiao Yang wrote:
>>> On 2023/9/21 6:57, Dave Jiang wrote:
>>>> +        if (daxctl_memory_online_no_movable(mem)) {
>>>> +            log_err(&rl, "%s: memory unmovable for %s\n",
>>>> +                    devname,
>>>> +                    daxctl_dev_get_devname(dev));
>>>> +            return -EPERM;
>>>> +        }
>>> Hi Dave,
>>>
>>> It seems wrong to check if memory is unmovable by the return number of daxctl_memory_online_no_movable(mem) here. IIRC, the return number of daxctl_memory_online_no_movable(mem)/daxctl_memory_op(MEM_GET_ZONE) indicates how many memory blocks have the same memory zone. So I think you should check mem->zone and MEM_ZONE_NORMAL as daxctl_memory_is_movable() did.
>> Do you mean:
>> rc = daxctl_memory_online_no_movable(mem);
>> if (rc < 0)
>>     return rc;
>> if (rc > 0) {
>>     log_err(&rl, "%s memory unmovable for %s\n' ...);
>>     return -EPERM;
>> }
>>
> Hi Dave,
> 
> Sorry for the late reply.
> 
> Is it necessary to try to online the memory region to the MEM_ZONE_NORMAL by daxctl_memory_online_no_movable(mem)? If you just want to check if the onlined memory region is in the MEM_ZONE_NORMAL, the following code seems better:
>     mem->zone = 0;
>     rc = daxctl_memory_op(mem, MEM_GET_ZONE);
>     if (rc < 0)
>         return rc;
>     if (mem->zone == MEM_ZONE_NORMAL) {
>         log_err(&rl, "%s memory unmovable for %s\n' ...);
>     return -EPERM;
>     }
> 

Ah that was a mistake. I meant to call the query function and not the online op function. Do you have any objections to

if (!daxctl_memory_is_movable(mem))

> Best Regards,
> Xiao Yang
Dan Williams Oct. 30, 2023, 6:31 p.m. UTC | #8
Dave Jiang wrote:
[..]
> Ah that was a mistake. I meant to call the query function and not the
> online op function. Do you have any objections to
> 
> if (!daxctl_memory_is_movable(mem))

Wait, why check for movable? ZONE_NORMAL can be removed if you are
lucky, and ZONE_MOVABLE may still not be able to be removed if you are
unlucky. So I would expect that disable-region attempts to offline
all-memory blocks, and if that fails then fail the disable-region. That
would of course need to come with documentation that disable-region may
leave the memory in a partially offline state. Then the force can just
rip the device away with the warning message that physical address space
has now been permanently leaked and can not be recovered until a reboot.
Dave Jiang Oct. 30, 2023, 9:30 p.m. UTC | #9
On 10/30/23 11:31, Dan Williams wrote:
> Dave Jiang wrote:
> [..]
>> Ah that was a mistake. I meant to call the query function and not the
>> online op function. Do you have any objections to
>>
>> if (!daxctl_memory_is_movable(mem))
> 
> Wait, why check for movable? ZONE_NORMAL can be removed if you are
> lucky, and ZONE_MOVABLE may still not be able to be removed if you are
> unlucky. So I would expect that disable-region attempts to offline
> all-memory blocks, and if that fails then fail the disable-region. That
> would of course need to come with documentation that disable-region may
> leave the memory in a partially offline state. Then the force can just
> rip the device away with the warning message that physical address space
> has now been permanently leaked and can not be recovered until a reboot.


I'll update as you outlined above.
diff mbox series

Patch

diff --git a/Documentation/cxl/cxl-disable-region.txt b/Documentation/cxl/cxl-disable-region.txt
index 6a39aee6ea69..9b98d4d8745a 100644
--- a/Documentation/cxl/cxl-disable-region.txt
+++ b/Documentation/cxl/cxl-disable-region.txt
@@ -25,6 +25,13 @@  OPTIONS
 -------
 include::bus-option.txt[]
 
+-f::
+--force::
+	Attempt to offline any memory that has been hot-added into the system
+	via the CXL region before disabling the region. This won't be attempted
+	if the memory was not added as 'movable', and may still fail even if it
+	was movable.
+
 include::decoder-option.txt[]
 
 include::debug-option.txt[]
diff --git a/cxl/region.c b/cxl/region.c
index bcd703956207..f8303869727a 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -14,6 +14,7 @@ 
 #include <util/parse-options.h>
 #include <ccan/minmax/minmax.h>
 #include <ccan/short_types/short_types.h>
+#include <daxctl/libdaxctl.h>
 
 #include "filter.h"
 #include "json.h"
@@ -95,6 +96,8 @@  static const struct option enable_options[] = {
 
 static const struct option disable_options[] = {
 	BASE_OPTIONS(),
+	OPT_BOOLEAN('f', "force", &param.force,
+		    "attempt to offline memory before disabling the region"),
 	OPT_END(),
 };
 
@@ -789,13 +792,57 @@  static int destroy_region(struct cxl_region *region)
 	return cxl_region_delete(region);
 }
 
+static int disable_region(struct cxl_region *region)
+{
+	const char *devname = cxl_region_get_devname(region);
+	struct daxctl_region *dax_region;
+	struct daxctl_memory *mem;
+	struct daxctl_dev *dev;
+	int rc;
+
+	dax_region = cxl_region_get_daxctl_region(region);
+	if (!dax_region)
+		goto out;
+
+	daxctl_dev_foreach(dax_region, dev) {
+		mem = daxctl_dev_get_memory(dev);
+		if (!mem)
+			return -ENXIO;
+
+		if (daxctl_memory_online_no_movable(mem)) {
+			log_err(&rl, "%s: memory unmovable for %s\n",
+					devname,
+					daxctl_dev_get_devname(dev));
+			return -EPERM;
+		}
+
+		/*
+		 * If memory is still online and user wants to force it, attempt
+		 * to offline it.
+		 */
+		if (daxctl_memory_is_online(mem) && param.force) {
+			rc = daxctl_memory_offline(mem);
+			if (rc) {
+				log_err(&rl, "%s: unable to offline %s: %s\n",
+					devname,
+					daxctl_dev_get_devname(dev),
+					strerror(abs(rc)));
+				return rc;
+			}
+		}
+	}
+
+out:
+	return cxl_region_disable(region);
+}
+
 static int do_region_xable(struct cxl_region *region, enum region_actions action)
 {
 	switch (action) {
 	case ACTION_ENABLE:
 		return cxl_region_enable(region);
 	case ACTION_DISABLE:
-		return cxl_region_disable(region);
+		return disable_region(region);
 	case ACTION_DESTROY:
 		return destroy_region(region);
 	default: