* [NDCTL PATCH v2] cxl/region: Add -f option for disable-region
@ 2023-09-20 22:57 Dave Jiang
2023-09-21 2:58 ` Zhijian Li (Fujitsu)
2023-10-09 10:52 ` Xiao Yang
0 siblings, 2 replies; 10+ messages in thread
From: Dave Jiang @ 2023-09-20 22:57 UTC (permalink / raw
To: vishal.l.verma; +Cc: linux-cxl, nvdimm
The current operation for disable_region does not check if the memory
covered by a region is online before attempting to disable the cxl region.
Provide a -f option for the region to force offlining of currently online
memory before disabling the region. Also add a check to fail the operation
entirely if the memory is non-movable.
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v2:
- Update documentation and help output. (Vishal)
---
Documentation/cxl/cxl-disable-region.txt | 7 ++++
cxl/region.c | 49 +++++++++++++++++++++++++++++-
2 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/Documentation/cxl/cxl-disable-region.txt b/Documentation/cxl/cxl-disable-region.txt
index 6a39aee6ea69..9b98d4d8745a 100644
--- a/Documentation/cxl/cxl-disable-region.txt
+++ b/Documentation/cxl/cxl-disable-region.txt
@@ -25,6 +25,13 @@ OPTIONS
-------
include::bus-option.txt[]
+-f::
+--force::
+ Attempt to offline any memory that has been hot-added into the system
+ via the CXL region before disabling the region. This won't be attempted
+ if the memory was not added as 'movable', and may still fail even if it
+ was movable.
+
include::decoder-option.txt[]
include::debug-option.txt[]
diff --git a/cxl/region.c b/cxl/region.c
index bcd703956207..f8303869727a 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -14,6 +14,7 @@
#include <util/parse-options.h>
#include <ccan/minmax/minmax.h>
#include <ccan/short_types/short_types.h>
+#include <daxctl/libdaxctl.h>
#include "filter.h"
#include "json.h"
@@ -95,6 +96,8 @@ static const struct option enable_options[] = {
static const struct option disable_options[] = {
BASE_OPTIONS(),
+ OPT_BOOLEAN('f', "force", ¶m.force,
+ "attempt to offline memory before disabling the region"),
OPT_END(),
};
@@ -789,13 +792,57 @@ static int destroy_region(struct cxl_region *region)
return cxl_region_delete(region);
}
+static int disable_region(struct cxl_region *region)
+{
+ const char *devname = cxl_region_get_devname(region);
+ struct daxctl_region *dax_region;
+ struct daxctl_memory *mem;
+ struct daxctl_dev *dev;
+ int rc;
+
+ dax_region = cxl_region_get_daxctl_region(region);
+ if (!dax_region)
+ goto out;
+
+ daxctl_dev_foreach(dax_region, dev) {
+ mem = daxctl_dev_get_memory(dev);
+ if (!mem)
+ return -ENXIO;
+
+ if (daxctl_memory_online_no_movable(mem)) {
+ log_err(&rl, "%s: memory unmovable for %s\n",
+ devname,
+ daxctl_dev_get_devname(dev));
+ return -EPERM;
+ }
+
+ /*
+ * If memory is still online and user wants to force it, attempt
+ * to offline it.
+ */
+ if (daxctl_memory_is_online(mem) && param.force) {
+ rc = daxctl_memory_offline(mem);
+ if (rc) {
+ log_err(&rl, "%s: unable to offline %s: %s\n",
+ devname,
+ daxctl_dev_get_devname(dev),
+ strerror(abs(rc)));
+ return rc;
+ }
+ }
+ }
+
+out:
+ return cxl_region_disable(region);
+}
+
static int do_region_xable(struct cxl_region *region, enum region_actions action)
{
switch (action) {
case ACTION_ENABLE:
return cxl_region_enable(region);
case ACTION_DISABLE:
- return cxl_region_disable(region);
+ return disable_region(region);
case ACTION_DESTROY:
return destroy_region(region);
default:
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [NDCTL PATCH v2] cxl/region: Add -f option for disable-region
2023-09-20 22:57 [NDCTL PATCH v2] cxl/region: Add -f option for disable-region Dave Jiang
@ 2023-09-21 2:58 ` Zhijian Li (Fujitsu)
2023-09-21 23:19 ` Dave Jiang
2023-10-09 10:52 ` Xiao Yang
1 sibling, 1 reply; 10+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-09-21 2:58 UTC (permalink / raw
To: Dave Jiang, vishal.l.verma@intel.com
Cc: linux-cxl@vger.kernel.org, nvdimm@lists.linux.dev,
Xiao Yang (Fujitsu), Quanquan Cao (Fujitsu)
Dave，
Forgive me for not having a new thread, I'd ask a possible relevant questions about disable-memdev
We noticed that only -f is implemented for disable-memdev, and it left a
"TODO: actually detect rather than assume active" in cxl/memdev.c.
My questions are:
1. Does the *active* here mean the region(the memdev belongs to) is active ?
2. Is the without force method under developing ?
My colleagues(in CC's) are investigating how to gracefully disable-memdev
Thanks
Zhijian
On 21/09/2023 06:57, Dave Jiang wrote:
> The current operation for disable_region does not check if the memory
> covered by a region is online before attempting to disable the cxl region.
> Provide a -f option for the region to force offlining of currently online
> memory before disabling the region. Also add a check to fail the operation
> entirely if the memory is non-movable.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>
> ---
> v2:
> - Update documentation and help output. (Vishal)
> ---
> Documentation/cxl/cxl-disable-region.txt | 7 ++++
> cxl/region.c | 49 +++++++++++++++++++++++++++++-
> 2 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/cxl/cxl-disable-region.txt b/Documentation/cxl/cxl-disable-region.txt
> index 6a39aee6ea69..9b98d4d8745a 100644
> --- a/Documentation/cxl/cxl-disable-region.txt
> +++ b/Documentation/cxl/cxl-disable-region.txt
> @@ -25,6 +25,13 @@ OPTIONS
> -------
> include::bus-option.txt[]
>
> +-f::
> +--force::
> + Attempt to offline any memory that has been hot-added into the system
> + via the CXL region before disabling the region. This won't be attempted
> + if the memory was not added as 'movable', and may still fail even if it
> + was movable.
> +
> include::decoder-option.txt[]
>
> include::debug-option.txt[]
> diff --git a/cxl/region.c b/cxl/region.c
> index bcd703956207..f8303869727a 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -14,6 +14,7 @@
> #include <util/parse-options.h>
> #include <ccan/minmax/minmax.h>
> #include <ccan/short_types/short_types.h>
> +#include <daxctl/libdaxctl.h>
>
> #include "filter.h"
> #include "json.h"
> @@ -95,6 +96,8 @@ static const struct option enable_options[] = {
>
> static const struct option disable_options[] = {
> BASE_OPTIONS(),
> + OPT_BOOLEAN('f', "force", ¶m.force,
> + "attempt to offline memory before disabling the region"),
> OPT_END(),
> };
>
> @@ -789,13 +792,57 @@ static int destroy_region(struct cxl_region *region)
> return cxl_region_delete(region);
> }
>
> +static int disable_region(struct cxl_region *region)
> +{
> + const char *devname = cxl_region_get_devname(region);
> + struct daxctl_region *dax_region;
> + struct daxctl_memory *mem;
> + struct daxctl_dev *dev;
> + int rc;
> +
> + dax_region = cxl_region_get_daxctl_region(region);
> + if (!dax_region)
> + goto out;
> +
> + daxctl_dev_foreach(dax_region, dev) {
> + mem = daxctl_dev_get_memory(dev);
> + if (!mem)
> + return -ENXIO;
> +
> + if (daxctl_memory_online_no_movable(mem)) {
> + log_err(&rl, "%s: memory unmovable for %s\n",
> + devname,
> + daxctl_dev_get_devname(dev));
> + return -EPERM;
> + }
> +
> + /*
> + * If memory is still online and user wants to force it, attempt
> + * to offline it.
> + */
> + if (daxctl_memory_is_online(mem) && param.force) {
> + rc = daxctl_memory_offline(mem);
> + if (rc) {
> + log_err(&rl, "%s: unable to offline %s: %s\n",
> + devname,
> + daxctl_dev_get_devname(dev),
> + strerror(abs(rc)));
> + return rc;
> + }
> + }
> + }
> +
> +out:
> + return cxl_region_disable(region);
> +}
> +
> static int do_region_xable(struct cxl_region *region, enum region_actions action)
> {
> switch (action) {
> case ACTION_ENABLE:
> return cxl_region_enable(region);
> case ACTION_DISABLE:
> - return cxl_region_disable(region);
> + return disable_region(region);
> case ACTION_DESTROY:
> return destroy_region(region);
> default:
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [NDCTL PATCH v2] cxl/region: Add -f option for disable-region
2023-09-21 2:58 ` Zhijian Li (Fujitsu)
@ 2023-09-21 23:19 ` Dave Jiang
2023-09-22 1:26 ` Zhijian Li (Fujitsu)
0 siblings, 1 reply; 10+ messages in thread
From: Dave Jiang @ 2023-09-21 23:19 UTC (permalink / raw
To: Zhijian Li (Fujitsu), vishal.l.verma@intel.com
Cc: linux-cxl@vger.kernel.org, nvdimm@lists.linux.dev,
Xiao Yang (Fujitsu), Quanquan Cao (Fujitsu)
On 9/20/23 19:58, Zhijian Li (Fujitsu) wrote:
> Dave，
>
> Forgive me for not having a new thread, I'd ask a possible relevant questions about disable-memdev
> We noticed that only -f is implemented for disable-memdev, and it left a
> "TODO: actually detect rather than assume active" in cxl/memdev.c.
>
> My questions are:
> 1. Does the *active* here mean the region(the memdev belongs to) is active ?
> 2. Is the without force method under developing ?
>
> My colleagues(in CC's) are investigating how to gracefully disable-memdev
Zhijian,
So this was there before the region enumeration showed up according to Dan. Now an update to check if the memdev is part of any active region should be added. Either you guys can send a patch or I can go added it. Let me know. Thanks!
>
> Thanks
> Zhijian
>
> On 21/09/2023 06:57, Dave Jiang wrote:
>> The current operation for disable_region does not check if the memory
>> covered by a region is online before attempting to disable the cxl region.
>> Provide a -f option for the region to force offlining of currently online
>> memory before disabling the region. Also add a check to fail the operation
>> entirely if the memory is non-movable.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>
>> ---
>> v2:
>> - Update documentation and help output. (Vishal)
>> ---
>> Documentation/cxl/cxl-disable-region.txt | 7 ++++
>> cxl/region.c | 49 +++++++++++++++++++++++++++++-
>> 2 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/cxl/cxl-disable-region.txt b/Documentation/cxl/cxl-disable-region.txt
>> index 6a39aee6ea69..9b98d4d8745a 100644
>> --- a/Documentation/cxl/cxl-disable-region.txt
>> +++ b/Documentation/cxl/cxl-disable-region.txt
>> @@ -25,6 +25,13 @@ OPTIONS
>> -------
>> include::bus-option.txt[]
>>
>> +-f::
>> +--force::
>> + Attempt to offline any memory that has been hot-added into the system
>> + via the CXL region before disabling the region. This won't be attempted
>> + if the memory was not added as 'movable', and may still fail even if it
>> + was movable.
>> +
>> include::decoder-option.txt[]
>>
>> include::debug-option.txt[]
>> diff --git a/cxl/region.c b/cxl/region.c
>> index bcd703956207..f8303869727a 100644
>> --- a/cxl/region.c
>> +++ b/cxl/region.c
>> @@ -14,6 +14,7 @@
>> #include <util/parse-options.h>
>> #include <ccan/minmax/minmax.h>
>> #include <ccan/short_types/short_types.h>
>> +#include <daxctl/libdaxctl.h>
>>
>> #include "filter.h"
>> #include "json.h"
>> @@ -95,6 +96,8 @@ static const struct option enable_options[] = {
>>
>> static const struct option disable_options[] = {
>> BASE_OPTIONS(),
>> + OPT_BOOLEAN('f', "force", ¶m.force,
>> + "attempt to offline memory before disabling the region"),
>> OPT_END(),
>> };
>>
>> @@ -789,13 +792,57 @@ static int destroy_region(struct cxl_region *region)
>> return cxl_region_delete(region);
>> }
>>
>> +static int disable_region(struct cxl_region *region)
>> +{
>> + const char *devname = cxl_region_get_devname(region);
>> + struct daxctl_region *dax_region;
>> + struct daxctl_memory *mem;
>> + struct daxctl_dev *dev;
>> + int rc;
>> +
>> + dax_region = cxl_region_get_daxctl_region(region);
>> + if (!dax_region)
>> + goto out;
>> +
>> + daxctl_dev_foreach(dax_region, dev) {
>> + mem = daxctl_dev_get_memory(dev);
>> + if (!mem)
>> + return -ENXIO;
>> +
>> + if (daxctl_memory_online_no_movable(mem)) {
>> + log_err(&rl, "%s: memory unmovable for %s\n",
>> + devname,
>> + daxctl_dev_get_devname(dev));
>> + return -EPERM;
>> + }
>> +
>> + /*
>> + * If memory is still online and user wants to force it, attempt
>> + * to offline it.
>> + */
>> + if (daxctl_memory_is_online(mem) && param.force) {
>> + rc = daxctl_memory_offline(mem);
>> + if (rc) {
>> + log_err(&rl, "%s: unable to offline %s: %s\n",
>> + devname,
>> + daxctl_dev_get_devname(dev),
>> + strerror(abs(rc)));
>> + return rc;
>> + }
>> + }
>> + }
>> +
>> +out:
>> + return cxl_region_disable(region);
>> +}
>> +
>> static int do_region_xable(struct cxl_region *region, enum region_actions action)
>> {
>> switch (action) {
>> case ACTION_ENABLE:
>> return cxl_region_enable(region);
>> case ACTION_DISABLE:
>> - return cxl_region_disable(region);
>> + return disable_region(region);
>> case ACTION_DESTROY:
>> return destroy_region(region);
>> default:
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [NDCTL PATCH v2] cxl/region: Add -f option for disable-region
2023-09-21 23:19 ` Dave Jiang
@ 2023-09-22 1:26 ` Zhijian Li (Fujitsu)
0 siblings, 0 replies; 10+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-09-22 1:26 UTC (permalink / raw
To: Dave Jiang, vishal.l.verma@intel.com
Cc: linux-cxl@vger.kernel.org, nvdimm@lists.linux.dev,
Xiao Yang (Fujitsu), Quanquan Cao (Fujitsu)
On 22/09/2023 07:19, Dave Jiang wrote:
>
>
> On 9/20/23 19:58, Zhijian Li (Fujitsu) wrote:
>> Dave，
>>
>> Forgive me for not having a new thread, I'd ask a possible relevant questions about disable-memdev
>> We noticed that only -f is implemented for disable-memdev, and it left a
>> "TODO: actually detect rather than assume active" in cxl/memdev.c.
>>
>> My questions are:
>> 1. Does the *active* here mean the region(the memdev belongs to) is active ?
>> 2. Is the without force method under developing ?
>>
>> My colleagues(in CC's) are investigating how to gracefully disable-memdev
>
> Zhijian,
> So this was there before the region enumeration showed up according to Dan. Now an update to check if the memdev is part of any active region should be added. Either you guys can send a patch or I can go added it. Let me know. Thanks!
Understood, thanks for your information, please go ahead :)
I believe our guys are willing to test it.
Thanks
Zhijian
>
>
>>
>> Thanks
>> Zhijian
>>
>> On 21/09/2023 06:57, Dave Jiang wrote:
>>> The current operation for disable_region does not check if the memory
>>> covered by a region is online before attempting to disable the cxl region.
>>> Provide a -f option for the region to force offlining of currently online
>>> memory before disabling the region. Also add a check to fail the operation
>>> entirely if the memory is non-movable.
>>>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>
>>> ---
>>> v2:
>>> - Update documentation and help output. (Vishal)
>>> ---
>>> Documentation/cxl/cxl-disable-region.txt | 7 ++++
>>> cxl/region.c | 49 +++++++++++++++++++++++++++++-
>>> 2 files changed, 55 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/cxl/cxl-disable-region.txt b/Documentation/cxl/cxl-disable-region.txt
>>> index 6a39aee6ea69..9b98d4d8745a 100644
>>> --- a/Documentation/cxl/cxl-disable-region.txt
>>> +++ b/Documentation/cxl/cxl-disable-region.txt
>>> @@ -25,6 +25,13 @@ OPTIONS
>>> -------
>>> include::bus-option.txt[]
>>>
>>> +-f::
>>> +--force::
>>> + Attempt to offline any memory that has been hot-added into the system
>>> + via the CXL region before disabling the region. This won't be attempted
>>> + if the memory was not added as 'movable', and may still fail even if it
>>> + was movable.
>>> +
>>> include::decoder-option.txt[]
>>>
>>> include::debug-option.txt[]
>>> diff --git a/cxl/region.c b/cxl/region.c
>>> index bcd703956207..f8303869727a 100644
>>> --- a/cxl/region.c
>>> +++ b/cxl/region.c
>>> @@ -14,6 +14,7 @@
>>> #include <util/parse-options.h>
>>> #include <ccan/minmax/minmax.h>
>>> #include <ccan/short_types/short_types.h>
>>> +#include <daxctl/libdaxctl.h>
>>>
>>> #include "filter.h"
>>> #include "json.h"
>>> @@ -95,6 +96,8 @@ static const struct option enable_options[] = {
>>>
>>> static const struct option disable_options[] = {
>>> BASE_OPTIONS(),
>>> + OPT_BOOLEAN('f', "force", ¶m.force,
>>> + "attempt to offline memory before disabling the region"),
>>> OPT_END(),
>>> };
>>>
>>> @@ -789,13 +792,57 @@ static int destroy_region(struct cxl_region *region)
>>> return cxl_region_delete(region);
>>> }
>>>
>>> +static int disable_region(struct cxl_region *region)
>>> +{
>>> + const char *devname = cxl_region_get_devname(region);
>>> + struct daxctl_region *dax_region;
>>> + struct daxctl_memory *mem;
>>> + struct daxctl_dev *dev;
>>> + int rc;
>>> +
>>> + dax_region = cxl_region_get_daxctl_region(region);
>>> + if (!dax_region)
>>> + goto out;
>>> +
>>> + daxctl_dev_foreach(dax_region, dev) {
>>> + mem = daxctl_dev_get_memory(dev);
>>> + if (!mem)
>>> + return -ENXIO;
>>> +
>>> + if (daxctl_memory_online_no_movable(mem)) {
>>> + log_err(&rl, "%s: memory unmovable for %s\n",
>>> + devname,
>>> + daxctl_dev_get_devname(dev));
>>> + return -EPERM;
>>> + }
>>> +
>>> + /*
>>> + * If memory is still online and user wants to force it, attempt
>>> + * to offline it.
>>> + */
>>> + if (daxctl_memory_is_online(mem) && param.force) {
>>> + rc = daxctl_memory_offline(mem);
>>> + if (rc) {
>>> + log_err(&rl, "%s: unable to offline %s: %s\n",
>>> + devname,
>>> + daxctl_dev_get_devname(dev),
>>> + strerror(abs(rc)));
>>> + return rc;
>>> + }
>>> + }
>>> + }
>>> +
>>> +out:
>>> + return cxl_region_disable(region);
>>> +}
>>> +
>>> static int do_region_xable(struct cxl_region *region, enum region_actions action)
>>> {
>>> switch (action) {
>>> case ACTION_ENABLE:
>>> return cxl_region_enable(region);
>>> case ACTION_DISABLE:
>>> - return cxl_region_disable(region);
>>> + return disable_region(region);
>>> case ACTION_DESTROY:
>>> return destroy_region(region);
>>> default:
>>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [NDCTL PATCH v2] cxl/region: Add -f option for disable-region
2023-09-20 22:57 [NDCTL PATCH v2] cxl/region: Add -f option for disable-region Dave Jiang
2023-09-21 2:58 ` Zhijian Li (Fujitsu)
@ 2023-10-09 10:52 ` Xiao Yang
2023-10-13 22:38 ` Dave Jiang
1 sibling, 1 reply; 10+ messages in thread
From: Xiao Yang @ 2023-10-09 10:52 UTC (permalink / raw
To: Dave Jiang, vishal.l.verma
Cc: linux-cxl, nvdimm, lizhijian@fujitsu.com, caoqq
On 2023/9/21 6:57, Dave Jiang wrote:
> + if (daxctl_memory_online_no_movable(mem)) {
> + log_err(&rl, "%s: memory unmovable for %s\n",
> + devname,
> + daxctl_dev_get_devname(dev));
> + return -EPERM;
> + }
Hi Dave,
It seems wrong to check if memory is unmovable by the return number of
daxctl_memory_online_no_movable(mem) here. IIRC, the return number of
daxctl_memory_online_no_movable(mem)/daxctl_memory_op(MEM_GET_ZONE)
indicates how many memory blocks have the same memory zone. So I think
you should check mem->zone and MEM_ZONE_NORMAL as
daxctl_memory_is_movable() did.
Besides, I send a patch to improve the implementation of
daxctl_memory_online_with_zone().
https://lore.kernel.org/nvdimm/20231009103521.1463-1-yangx.jy@fujitsu.com/T/#u
Best Regards,
Xiao Yang
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [NDCTL PATCH v2] cxl/region: Add -f option for disable-region
2023-10-09 10:52 ` Xiao Yang
@ 2023-10-13 22:38 ` Dave Jiang
2023-10-30 4:33 ` Xiao Yang
0 siblings, 1 reply; 10+ messages in thread
From: Dave Jiang @ 2023-10-13 22:38 UTC (permalink / raw
To: Xiao Yang, vishal.l.verma; +Cc: linux-cxl, nvdimm, lizhijian@fujitsu.com, caoqq
On 10/9/23 03:52, Xiao Yang wrote:
> On 2023/9/21 6:57, Dave Jiang wrote:
>> + if (daxctl_memory_online_no_movable(mem)) {
>> + log_err(&rl, "%s: memory unmovable for %s\n",
>> + devname,
>> + daxctl_dev_get_devname(dev));
>> + return -EPERM;
>> + }
> Hi Dave,
>
> It seems wrong to check if memory is unmovable by the return number of daxctl_memory_online_no_movable(mem) here. IIRC, the return number of daxctl_memory_online_no_movable(mem)/daxctl_memory_op(MEM_GET_ZONE) indicates how many memory blocks have the same memory zone. So I think you should check mem->zone and MEM_ZONE_NORMAL as daxctl_memory_is_movable() did.
Do you mean:
rc = daxctl_memory_online_no_movable(mem);
if (rc < 0)
return rc;
if (rc > 0) {
log_err(&rl, "%s memory unmovable for %s\n' ...);
return -EPERM;
}
>
> Besides, I send a patch to improve the implementation of daxctl_memory_online_with_zone().
> https://lore.kernel.org/nvdimm/20231009103521.1463-1-yangx.jy@fujitsu.com/T/#u
Thanks. I added my review tag.
>
> Best Regards,
> Xiao Yang
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [NDCTL PATCH v2] cxl/region: Add -f option for disable-region
2023-10-13 22:38 ` Dave Jiang
@ 2023-10-30 4:33 ` Xiao Yang
2023-10-30 16:24 ` Dave Jiang
0 siblings, 1 reply; 10+ messages in thread
From: Xiao Yang @ 2023-10-30 4:33 UTC (permalink / raw
To: Dave Jiang, vishal.l.verma
Cc: linux-cxl, nvdimm, lizhijian@fujitsu.com, caoqq
On 2023/10/14 6:38, Dave Jiang wrote:
>
> On 10/9/23 03:52, Xiao Yang wrote:
>> On 2023/9/21 6:57, Dave Jiang wrote:
>>> + if (daxctl_memory_online_no_movable(mem)) {
>>> + log_err(&rl, "%s: memory unmovable for %s\n",
>>> + devname,
>>> + daxctl_dev_get_devname(dev));
>>> + return -EPERM;
>>> + }
>> Hi Dave,
>>
>> It seems wrong to check if memory is unmovable by the return number of daxctl_memory_online_no_movable(mem) here. IIRC, the return number of daxctl_memory_online_no_movable(mem)/daxctl_memory_op(MEM_GET_ZONE) indicates how many memory blocks have the same memory zone. So I think you should check mem->zone and MEM_ZONE_NORMAL as daxctl_memory_is_movable() did.
> Do you mean:
> rc = daxctl_memory_online_no_movable(mem);
> if (rc < 0)
> return rc;
> if (rc > 0) {
> log_err(&rl, "%s memory unmovable for %s\n' ...);
> return -EPERM;
> }
>
Hi Dave,
Sorry for the late reply.
Is it necessary to try to online the memory region to the
MEM_ZONE_NORMAL by daxctl_memory_online_no_movable(mem)? If you just
want to check if the onlined memory region is in the MEM_ZONE_NORMAL,
the following code seems better:
mem->zone = 0;
rc = daxctl_memory_op(mem, MEM_GET_ZONE);
if (rc < 0)
return rc;
if (mem->zone == MEM_ZONE_NORMAL) {
log_err(&rl, "%s memory unmovable for %s\n' ...);
return -EPERM;
}
Best Regards,
Xiao Yang
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [NDCTL PATCH v2] cxl/region: Add -f option for disable-region
2023-10-30 4:33 ` Xiao Yang
@ 2023-10-30 16:24 ` Dave Jiang
2023-10-30 18:31 ` Dan Williams
0 siblings, 1 reply; 10+ messages in thread
From: Dave Jiang @ 2023-10-30 16:24 UTC (permalink / raw
To: Xiao Yang, vishal.l.verma; +Cc: linux-cxl, nvdimm, lizhijian@fujitsu.com, caoqq
On 10/29/23 21:33, Xiao Yang wrote:
> On 2023/10/14 6:38, Dave Jiang wrote:
>>
>> On 10/9/23 03:52, Xiao Yang wrote:
>>> On 2023/9/21 6:57, Dave Jiang wrote:
>>>> + if (daxctl_memory_online_no_movable(mem)) {
>>>> + log_err(&rl, "%s: memory unmovable for %s\n",
>>>> + devname,
>>>> + daxctl_dev_get_devname(dev));
>>>> + return -EPERM;
>>>> + }
>>> Hi Dave,
>>>
>>> It seems wrong to check if memory is unmovable by the return number of daxctl_memory_online_no_movable(mem) here. IIRC, the return number of daxctl_memory_online_no_movable(mem)/daxctl_memory_op(MEM_GET_ZONE) indicates how many memory blocks have the same memory zone. So I think you should check mem->zone and MEM_ZONE_NORMAL as daxctl_memory_is_movable() did.
>> Do you mean:
>> rc = daxctl_memory_online_no_movable(mem);
>> if (rc < 0)
>> return rc;
>> if (rc > 0) {
>> log_err(&rl, "%s memory unmovable for %s\n' ...);
>> return -EPERM;
>> }
>>
> Hi Dave,
>
> Sorry for the late reply.
>
> Is it necessary to try to online the memory region to the MEM_ZONE_NORMAL by daxctl_memory_online_no_movable(mem)? If you just want to check if the onlined memory region is in the MEM_ZONE_NORMAL, the following code seems better:
> mem->zone = 0;
> rc = daxctl_memory_op(mem, MEM_GET_ZONE);
> if (rc < 0)
> return rc;
> if (mem->zone == MEM_ZONE_NORMAL) {
> log_err(&rl, "%s memory unmovable for %s\n' ...);
> return -EPERM;
> }
>
Ah that was a mistake. I meant to call the query function and not the online op function. Do you have any objections to
if (!daxctl_memory_is_movable(mem))
> Best Regards,
> Xiao Yang
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [NDCTL PATCH v2] cxl/region: Add -f option for disable-region
2023-10-30 16:24 ` Dave Jiang
@ 2023-10-30 18:31 ` Dan Williams
2023-10-30 21:30 ` Dave Jiang
0 siblings, 1 reply; 10+ messages in thread
From: Dan Williams @ 2023-10-30 18:31 UTC (permalink / raw
To: Dave Jiang, Xiao Yang, vishal.l.verma
Cc: linux-cxl, nvdimm, lizhijian@fujitsu.com, caoqq
Dave Jiang wrote:
[..]
> Ah that was a mistake. I meant to call the query function and not the
> online op function. Do you have any objections to
>
> if (!daxctl_memory_is_movable(mem))
Wait, why check for movable? ZONE_NORMAL can be removed if you are
lucky, and ZONE_MOVABLE may still not be able to be removed if you are
unlucky. So I would expect that disable-region attempts to offline
all-memory blocks, and if that fails then fail the disable-region. That
would of course need to come with documentation that disable-region may
leave the memory in a partially offline state. Then the force can just
rip the device away with the warning message that physical address space
has now been permanently leaked and can not be recovered until a reboot.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [NDCTL PATCH v2] cxl/region: Add -f option for disable-region
2023-10-30 18:31 ` Dan Williams
@ 2023-10-30 21:30 ` Dave Jiang
0 siblings, 0 replies; 10+ messages in thread
From: Dave Jiang @ 2023-10-30 21:30 UTC (permalink / raw
To: Dan Williams, Xiao Yang, vishal.l.verma
Cc: linux-cxl, nvdimm, lizhijian@fujitsu.com, caoqq
On 10/30/23 11:31, Dan Williams wrote:
> Dave Jiang wrote:
> [..]
>> Ah that was a mistake. I meant to call the query function and not the
>> online op function. Do you have any objections to
>>
>> if (!daxctl_memory_is_movable(mem))
>
> Wait, why check for movable? ZONE_NORMAL can be removed if you are
> lucky, and ZONE_MOVABLE may still not be able to be removed if you are
> unlucky. So I would expect that disable-region attempts to offline
> all-memory blocks, and if that fails then fail the disable-region. That
> would of course need to come with documentation that disable-region may
> leave the memory in a partially offline state. Then the force can just
> rip the device away with the warning message that physical address space
> has now been permanently leaked and can not be recovered until a reboot.
I'll update as you outlined above.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-10-30 21:30 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-20 22:57 [NDCTL PATCH v2] cxl/region: Add -f option for disable-region Dave Jiang
2023-09-21 2:58 ` Zhijian Li (Fujitsu)
2023-09-21 23:19 ` Dave Jiang
2023-09-22 1:26 ` Zhijian Li (Fujitsu)
2023-10-09 10:52 ` Xiao Yang
2023-10-13 22:38 ` Dave Jiang
2023-10-30 4:33 ` Xiao Yang
2023-10-30 16:24 ` Dave Jiang
2023-10-30 18:31 ` Dan Williams
2023-10-30 21:30 ` Dave Jiang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).