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

[Dave Jiang <dave.jiang@intel.com>]

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:
>>