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

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

On 1/8/25 5:58 PM, Li Ming wrote:
> 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"?

Yes "movable" is sufficient. No need to over complicate things. 

DJ

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