Re: [ndctl PATCH 1/1] daxctl: Output more information if memblock is unremovable

[Li Ming <ming.li@zohomail.com>]

On 1/9/2025 12:46 AM, Dave Jiang wrote:
>
> On 12/5/24 6:10 PM, Alison Schofield wrote:
>> On Thu, Dec 05, 2024 at 12:14:56AM +0800, Li Ming wrote:
>>> If CONFIG_MEMORY_HOTREMOVE is disabled by kernel, memblocks will not be
>>> removed, so 'dax offline-memory all' will output below error logs:
>>>
>>>   libdaxctl: offline_one_memblock: dax0.0: Failed to offline /sys/devices/system/node/node6/memory371/state: Invalid argument
>>>   dax0.0: failed to offline memory: Invalid argument
>>>   error offlining memory: Invalid argument
>>>   offlined memory for 0 devices
>>>
>>> The log does not clearly show why the command failed. So checking if the
>>> target memblock is removable before offlining it by querying
>>> '/sys/devices/system/node/nodeX/memoryY/removable', then output specific
>>> logs if the memblock is unremovable, output will be:
>>>
>>>   libdaxctl: offline_one_memblock: dax0.0: memory371 is unremovable
>>>   dax0.0: failed to offline memory: Operation not supported
>>>   error offlining memory: Operation not supported
>>>   offlined memory for 0 devices
>>>
>> Hi Ming,
>>
>> This led me to catch up on movable and removable in DAX context.
>> Not all 'Movable' DAX memory is 'Removable' right?
>>
>> Would it be useful to add 'removable' to the daxctl list json:
>>
>> # daxctl list
>> [
>>   {
>>     "chardev":"dax0.0",
>>     "size":536870912,
>>     "target_node":0,
>>     "align":2097152,
>>     "mode":"system-ram",
>>     "online_memblocks":4,
>>     "total_memblocks":4,
>>     "movable":true
>>     "removable":false  <----
> Maybe adding some documentation and explaining the two fields? Otherwise it may get confusing.
>
> DJ

Hi Dave,


Thanks for your review, As my latest comment,

if no "movable" in daxctl list, that means the kernel not supported MEMORY_HOTREMOVE, the meanning is the same as "removable: false".

if a "movable" in daxctl list, that means the kernel supporting MEMORY_HOTREMOVE, and the value of "movable" decides whether the memory block can be removed.

My feeling is that "movable" is enough, may I know if it still is worth to add a new "removable"?


Ming


>
>>   }
>> ]
>>
>> You've already added the helper to discover removable.
>>
>> Otherwise, LGTM,
>> Reviewed-by: Alison Schofield <alison.schofield@intel.com>
>>
>>
>>> Besides, delay to set up string 'path' for offlining memblock operation,
>>> because string 'path' is stored in 'mem->mem_buf' which is a shared
>>> buffer, it will be used in memblock_is_removable().
>>>
>>> Signed-off-by: Li Ming <ming.li@zohomail.com>
>>> ---
>>>  daxctl/lib/libdaxctl.c | 52 ++++++++++++++++++++++++++++++++++++------
>>>  1 file changed, 45 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
>>> index 9fbefe2e8329..b7fa0de0b73d 100644
>>> --- a/daxctl/lib/libdaxctl.c
>>> +++ b/daxctl/lib/libdaxctl.c
>>> @@ -1310,6 +1310,37 @@ static int memblock_is_online(struct daxctl_memory *mem, char *memblock)
>>>  	return 0;
>>>  }
>>>  
>>> +static int memblock_is_removable(struct daxctl_memory *mem, char *memblock)
>>> +{
>>> +	struct daxctl_dev *dev = daxctl_memory_get_dev(mem);
>>> +	const char *devname = daxctl_dev_get_devname(dev);
>>> +	struct daxctl_ctx *ctx = daxctl_dev_get_ctx(dev);
>>> +	int len = mem->buf_len, rc;
>>> +	char buf[SYSFS_ATTR_SIZE];
>>> +	char *path = mem->mem_buf;
>>> +	const char *node_path;
>>> +
>>> +	node_path = daxctl_memory_get_node_path(mem);
>>> +	if (!node_path)
>>> +		return -ENXIO;
>>> +
>>> +	rc = snprintf(path, len, "%s/%s/removable", node_path, memblock);
>>> +	if (rc < 0)
>>> +		return -ENOMEM;
>>> +
>>> +	rc = sysfs_read_attr(ctx, path, buf);
>>> +	if (rc) {
>>> +		err(ctx, "%s: Failed to read %s: %s\n",
>>> +			devname, path, strerror(-rc));
>>> +		return rc;
>>> +	}
>>> +
>>> +	if (strtoul(buf, NULL, 0) == 0)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static int online_one_memblock(struct daxctl_memory *mem, char *memblock,
>>>  		enum memory_zones zone, int *status)
>>>  {
>>> @@ -1362,6 +1393,20 @@ static int offline_one_memblock(struct daxctl_memory *mem, char *memblock)
>>>  	char *path = mem->mem_buf;
>>>  	const char *node_path;
>>>  
>>> +	/* if already offline, there is nothing to do */
>>> +	rc = memblock_is_online(mem, memblock);
>>> +	if (rc < 0)
>>> +		return rc;
>>> +	if (!rc)
>>> +		return 1;
>>> +
>>> +	rc = memblock_is_removable(mem, memblock);
>>> +	if (rc) {
>>> +		if (rc == -EOPNOTSUPP)
>>> +			err(ctx, "%s: %s is unremovable\n", devname, memblock);
>>> +		return rc;
>>> +	}
>>> +
>>>  	node_path = daxctl_memory_get_node_path(mem);
>>>  	if (!node_path)
>>>  		return -ENXIO;
>>> @@ -1370,13 +1415,6 @@ static int offline_one_memblock(struct daxctl_memory *mem, char *memblock)
>>>  	if (rc < 0)
>>>  		return -ENOMEM;
>>>  
>>> -	/* if already offline, there is nothing to do */
>>> -	rc = memblock_is_online(mem, memblock);
>>> -	if (rc < 0)
>>> -		return rc;
>>> -	if (!rc)
>>> -		return 1;
>>> -
>>>  	rc = sysfs_write_attr_quiet(ctx, path, mode);
>>>  	if (rc) {
>>>  		/* check if something raced us to offline (unlikely) */
>>> -- 
>>> 2.34.1
>>>
>>>