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
> }
> ]
>
> You've already added the helper to discover removable.
>
> Otherwise, LGTM,
> Reviewed-by: Alison Schofield <alison.schofi...@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...@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
>>
>>
>
