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

[Alison Schofield <alison.schofield@intel.com>]

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

Hi Ming,

I was looking to merge this today but realized that I only have
a test for it in the 'affirmative'. The ndctl:dax unit tests exercise
this path, but since nfit-test module requires CONFIG_MEMORY_HOTREMOVE
I could only prove that this works one way. I put in some printfs and
saw it in action:
        libdaxctl: memblock_is_removable: Ming: memblock_is_removable()
        libdaxctl: offline_one_memblock: Ming: return from memblock_is_removable()

Let's get another reviewer eyes on this before merging.
Also, if you can add your qemu (i'm guessing) demo that would be helpful.

Also, we were having discussions about exposing more info to user, but I
think that can be a follow on. You can propose that it another patch after
this one, if you think it is worthwhile.



snip