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 <[email protected]>
>>
>>
>>> 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 <[email protected]>
>>> ---
>>> 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
>>>
>>>
