* [PATCH ndctl RESEND 0/2] ndctl: Fix ups for region destroy
@ 2023-12-01 4:06 Ira Weiny
2023-12-01 4:06 ` [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test Ira Weiny
2023-12-01 4:06 ` [PATCH ndctl RESEND 2/2] cxl/region: Fix memory device teardown in disable-region Ira Weiny
0 siblings, 2 replies; 10+ messages in thread
From: Ira Weiny @ 2023-12-01 4:06 UTC (permalink / raw
To: Vishal Verma; +Cc: Dave Jiang, Dan Williams, nvdimm, linux-cxl, Ira Weiny
The patch to force device tear down on region disable caused a
regression for regions without system ram.
Add a test for such a case and a fix to the patch.
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
[djiang: RESEND with proper mailing lists.]
---
Ira Weiny (2):
ndctl/test: Add destroy region test
cxl/region: Fix memory device teardown in disable-region
cxl/region.c | 3 ++
daxctl/lib/libdaxctl.c | 4 +--
daxctl/lib/libdaxctl.sym | 5 +++
daxctl/libdaxctl.h | 1 +
test/cxl-destroy-region.sh | 76 ++++++++++++++++++++++++++++++++++++++++++++++
test/meson.build | 2 ++
6 files changed, 89 insertions(+), 2 deletions(-)
---
base-commit: cbf049039482a56c2b66ede3e10d5e9c652890b7
change-id: 20231130-fix-region-destroy-a85ad1fde87b
Best regards,
--
Ira Weiny <ira.weiny@intel.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
2023-12-01 4:06 [PATCH ndctl RESEND 0/2] ndctl: Fix ups for region destroy Ira Weiny
@ 2023-12-01 4:06 ` Ira Weiny
2023-12-01 17:40 ` Dave Jiang
2023-12-01 19:39 ` Alison Schofield
2023-12-01 4:06 ` [PATCH ndctl RESEND 2/2] cxl/region: Fix memory device teardown in disable-region Ira Weiny
1 sibling, 2 replies; 10+ messages in thread
From: Ira Weiny @ 2023-12-01 4:06 UTC (permalink / raw
To: Vishal Verma; +Cc: Dave Jiang, Dan Williams, nvdimm, linux-cxl, Ira Weiny
Commit 9399aa667ab0 ("cxl/region: Add -f option for disable-region")
introduced a regression when destroying a region.
Add a tests for destroying a region.
Cc: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
test/cxl-destroy-region.sh | 76 ++++++++++++++++++++++++++++++++++++++++++++++
test/meson.build | 2 ++
2 files changed, 78 insertions(+)
diff --git a/test/cxl-destroy-region.sh b/test/cxl-destroy-region.sh
new file mode 100644
index 000000000000..251720a98688
--- /dev/null
+++ b/test/cxl-destroy-region.sh
@@ -0,0 +1,76 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2023 Intel Corporation. All rights reserved.
+
+. $(dirname $0)/common
+
+rc=77
+
+set -ex
+
+trap 'err $LINENO' ERR
+
+check_prereq "jq"
+
+modprobe -r cxl_test
+modprobe cxl_test
+rc=1
+
+check_destroy_ram()
+{
+ mem=$1
+ decoder=$2
+
+ region=$($CXL create-region -d "$decoder" -m "$mem" | jq -r ".region")
+ if [ "$region" == "null" ]; then
+ err "$LINENO"
+ fi
+ $CXL enable-region "$region"
+
+ # default is memory is system-ram offline
+ $CXL disable-region $region
+ $CXL destroy-region $region
+}
+
+check_destroy_devdax()
+{
+ mem=$1
+ decoder=$2
+
+ region=$($CXL create-region -d "$decoder" -m "$mem" | jq -r ".region")
+ if [ "$region" == "null" ]; then
+ err "$LINENO"
+ fi
+ $CXL enable-region "$region"
+
+ dax=$($CXL list -X -r "$region" | jq -r ".[].daxregion.devices" | jq -r '.[].chardev')
+
+ $DAXCTL reconfigure-device -m devdax "$dax"
+
+ $CXL disable-region $region
+ $CXL destroy-region $region
+}
+
+# Find a memory device to create regions on to test the destroy
+readarray -t mems < <("$CXL" list -b cxl_test -M | jq -r '.[].memdev')
+for mem in ${mems[@]}; do
+ ramsize=$($CXL list -m $mem | jq -r '.[].ram_size')
+ if [ "$ramsize" == "null" ]; then
+ continue
+ fi
+ decoder=$($CXL list -b cxl_test -D -d root -m "$mem" |
+ jq -r ".[] |
+ select(.volatile_capable == true) |
+ select(.nr_targets == 1) |
+ select(.size >= ${ramsize}) |
+ .decoder")
+ if [[ $decoder ]]; then
+ check_destroy_ram $mem $decoder
+ check_destroy_devdax $mem $decoder
+ break
+ fi
+done
+
+check_dmesg "$LINENO"
+
+modprobe -r cxl_test
diff --git a/test/meson.build b/test/meson.build
index 2706fa5d633c..126d663dfce2 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -158,6 +158,7 @@ cxl_xor_region = find_program('cxl-xor-region.sh')
cxl_update_firmware = find_program('cxl-update-firmware.sh')
cxl_events = find_program('cxl-events.sh')
cxl_poison = find_program('cxl-poison.sh')
+cxl_destroy_region = find_program('cxl-destroy-region.sh')
tests = [
[ 'libndctl', libndctl, 'ndctl' ],
@@ -188,6 +189,7 @@ tests = [
[ 'cxl-xor-region.sh', cxl_xor_region, 'cxl' ],
[ 'cxl-events.sh', cxl_events, 'cxl' ],
[ 'cxl-poison.sh', cxl_poison, 'cxl' ],
+ [ 'cxl-destroy-region.sh', cxl_destroy_region, 'cxl' ],
]
if get_option('destructive').enabled()
--
2.42.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH ndctl RESEND 2/2] cxl/region: Fix memory device teardown in disable-region
2023-12-01 4:06 [PATCH ndctl RESEND 0/2] ndctl: Fix ups for region destroy Ira Weiny
2023-12-01 4:06 ` [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test Ira Weiny
@ 2023-12-01 4:06 ` Ira Weiny
2023-12-01 17:39 ` Dave Jiang
1 sibling, 1 reply; 10+ messages in thread
From: Ira Weiny @ 2023-12-01 4:06 UTC (permalink / raw
To: Vishal Verma; +Cc: Dave Jiang, Dan Williams, nvdimm, linux-cxl, Ira Weiny
When a region is requested to be disabled, memory devices are normally
automatically torn down. Commit 9399aa667ab0 prevents tear down if
memory is online without a force flag. However, daxctl_dev_get_memory()
may return NULL if the memory device in question is not system-ram
capable as is the case for a region with only devdax devices. Such
devices do not need to be off-lined explicitly.
Skip non-system-ram devices rather than error the operation.
Fixes: 9399aa667ab0 ("cxl/region: Add -f option for disable-region")
Cc: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
cxl/region.c | 3 +++
daxctl/lib/libdaxctl.c | 4 ++--
daxctl/lib/libdaxctl.sym | 5 +++++
daxctl/libdaxctl.h | 1 +
4 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/cxl/region.c b/cxl/region.c
index 5cbbf2749e2d..44ac76b001e9 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -805,6 +805,9 @@ static int disable_region(struct cxl_region *region)
goto out;
daxctl_dev_foreach(dax_region, dev) {
+ if (!daxctl_dev_is_system_ram_capable(dev))
+ continue;
+
mem = daxctl_dev_get_memory(dev);
if (!mem)
return -ENXIO;
diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index 4f9aba0b09f2..9fbefe2e8329 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -385,7 +385,7 @@ static bool device_model_is_dax_bus(struct daxctl_dev *dev)
return false;
}
-static int dev_is_system_ram_capable(struct daxctl_dev *dev)
+DAXCTL_EXPORT int daxctl_dev_is_system_ram_capable(struct daxctl_dev *dev)
{
const char *devname = daxctl_dev_get_devname(dev);
struct daxctl_ctx *ctx = daxctl_dev_get_ctx(dev);
@@ -432,7 +432,7 @@ static struct daxctl_memory *daxctl_dev_alloc_mem(struct daxctl_dev *dev)
char buf[SYSFS_ATTR_SIZE];
int node_num;
- if (!dev_is_system_ram_capable(dev))
+ if (!daxctl_dev_is_system_ram_capable(dev))
return NULL;
mem = calloc(1, sizeof(*mem));
diff --git a/daxctl/lib/libdaxctl.sym b/daxctl/lib/libdaxctl.sym
index fe68fd0a9cde..309881196c86 100644
--- a/daxctl/lib/libdaxctl.sym
+++ b/daxctl/lib/libdaxctl.sym
@@ -99,3 +99,8 @@ global:
daxctl_set_config_path;
daxctl_get_config_path;
} LIBDAXCTL_8;
+
+LIBDAXCTL_10 {
+global:
+ daxctl_dev_is_system_ram_capable;
+} LIBDAXCTL_9;
diff --git a/daxctl/libdaxctl.h b/daxctl/libdaxctl.h
index 6876037a9427..53c6bbdae5c3 100644
--- a/daxctl/libdaxctl.h
+++ b/daxctl/libdaxctl.h
@@ -77,6 +77,7 @@ int daxctl_dev_will_auto_online_memory(struct daxctl_dev *dev);
int daxctl_dev_has_online_memory(struct daxctl_dev *dev);
struct daxctl_memory;
+int daxctl_dev_is_system_ram_capable(struct daxctl_dev *dev);
struct daxctl_memory *daxctl_dev_get_memory(struct daxctl_dev *dev);
struct daxctl_dev *daxctl_memory_get_dev(struct daxctl_memory *mem);
const char *daxctl_memory_get_node_path(struct daxctl_memory *mem);
--
2.42.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH ndctl RESEND 2/2] cxl/region: Fix memory device teardown in disable-region
2023-12-01 4:06 ` [PATCH ndctl RESEND 2/2] cxl/region: Fix memory device teardown in disable-region Ira Weiny
@ 2023-12-01 17:39 ` Dave Jiang
0 siblings, 0 replies; 10+ messages in thread
From: Dave Jiang @ 2023-12-01 17:39 UTC (permalink / raw
To: Ira Weiny, Vishal Verma; +Cc: Dan Williams, nvdimm, linux-cxl
On 11/30/23 21:06, Ira Weiny wrote:
> When a region is requested to be disabled, memory devices are normally
> automatically torn down. Commit 9399aa667ab0 prevents tear down if
> memory is online without a force flag. However, daxctl_dev_get_memory()
> may return NULL if the memory device in question is not system-ram
> capable as is the case for a region with only devdax devices. Such
> devices do not need to be off-lined explicitly.
>
> Skip non-system-ram devices rather than error the operation.
>
> Fixes: 9399aa667ab0 ("cxl/region: Add -f option for disable-region")
> Cc: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> cxl/region.c | 3 +++
> daxctl/lib/libdaxctl.c | 4 ++--
> daxctl/lib/libdaxctl.sym | 5 +++++
> daxctl/libdaxctl.h | 1 +
> 4 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/cxl/region.c b/cxl/region.c
> index 5cbbf2749e2d..44ac76b001e9 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -805,6 +805,9 @@ static int disable_region(struct cxl_region *region)
> goto out;
>
> daxctl_dev_foreach(dax_region, dev) {
> + if (!daxctl_dev_is_system_ram_capable(dev))
> + continue;
> +
> mem = daxctl_dev_get_memory(dev);
> if (!mem)
> return -ENXIO;
> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
> index 4f9aba0b09f2..9fbefe2e8329 100644
> --- a/daxctl/lib/libdaxctl.c
> +++ b/daxctl/lib/libdaxctl.c
> @@ -385,7 +385,7 @@ static bool device_model_is_dax_bus(struct daxctl_dev *dev)
> return false;
> }
>
> -static int dev_is_system_ram_capable(struct daxctl_dev *dev)
> +DAXCTL_EXPORT int daxctl_dev_is_system_ram_capable(struct daxctl_dev *dev)
> {
> const char *devname = daxctl_dev_get_devname(dev);
> struct daxctl_ctx *ctx = daxctl_dev_get_ctx(dev);
> @@ -432,7 +432,7 @@ static struct daxctl_memory *daxctl_dev_alloc_mem(struct daxctl_dev *dev)
> char buf[SYSFS_ATTR_SIZE];
> int node_num;
>
> - if (!dev_is_system_ram_capable(dev))
> + if (!daxctl_dev_is_system_ram_capable(dev))
> return NULL;
>
> mem = calloc(1, sizeof(*mem));
> diff --git a/daxctl/lib/libdaxctl.sym b/daxctl/lib/libdaxctl.sym
> index fe68fd0a9cde..309881196c86 100644
> --- a/daxctl/lib/libdaxctl.sym
> +++ b/daxctl/lib/libdaxctl.sym
> @@ -99,3 +99,8 @@ global:
> daxctl_set_config_path;
> daxctl_get_config_path;
> } LIBDAXCTL_8;
> +
> +LIBDAXCTL_10 {
> +global:
> + daxctl_dev_is_system_ram_capable;
> +} LIBDAXCTL_9;
> diff --git a/daxctl/libdaxctl.h b/daxctl/libdaxctl.h
> index 6876037a9427..53c6bbdae5c3 100644
> --- a/daxctl/libdaxctl.h
> +++ b/daxctl/libdaxctl.h
> @@ -77,6 +77,7 @@ int daxctl_dev_will_auto_online_memory(struct daxctl_dev *dev);
> int daxctl_dev_has_online_memory(struct daxctl_dev *dev);
>
> struct daxctl_memory;
> +int daxctl_dev_is_system_ram_capable(struct daxctl_dev *dev);
> struct daxctl_memory *daxctl_dev_get_memory(struct daxctl_dev *dev);
> struct daxctl_dev *daxctl_memory_get_dev(struct daxctl_memory *mem);
> const char *daxctl_memory_get_node_path(struct daxctl_memory *mem);
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
2023-12-01 4:06 ` [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test Ira Weiny
@ 2023-12-01 17:40 ` Dave Jiang
2023-12-01 19:39 ` Alison Schofield
1 sibling, 0 replies; 10+ messages in thread
From: Dave Jiang @ 2023-12-01 17:40 UTC (permalink / raw
To: Ira Weiny, Vishal Verma; +Cc: Dan Williams, nvdimm, linux-cxl
On 11/30/23 21:06, Ira Weiny wrote:
> Commit 9399aa667ab0 ("cxl/region: Add -f option for disable-region")
> introduced a regression when destroying a region.
>
> Add a tests for destroying a region.
>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> test/cxl-destroy-region.sh | 76 ++++++++++++++++++++++++++++++++++++++++++++++
> test/meson.build | 2 ++
> 2 files changed, 78 insertions(+)
>
> diff --git a/test/cxl-destroy-region.sh b/test/cxl-destroy-region.sh
> new file mode 100644
> index 000000000000..251720a98688
> --- /dev/null
> +++ b/test/cxl-destroy-region.sh
> @@ -0,0 +1,76 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2023 Intel Corporation. All rights reserved.
> +
> +. $(dirname $0)/common
> +
> +rc=77
> +
> +set -ex
> +
> +trap 'err $LINENO' ERR
> +
> +check_prereq "jq"
> +
> +modprobe -r cxl_test
> +modprobe cxl_test
> +rc=1
> +
> +check_destroy_ram()
> +{
> + mem=$1
> + decoder=$2
> +
> + region=$($CXL create-region -d "$decoder" -m "$mem" | jq -r ".region")
> + if [ "$region" == "null" ]; then
> + err "$LINENO"
> + fi
> + $CXL enable-region "$region"
> +
> + # default is memory is system-ram offline
> + $CXL disable-region $region
> + $CXL destroy-region $region
> +}
> +
> +check_destroy_devdax()
> +{
> + mem=$1
> + decoder=$2
> +
> + region=$($CXL create-region -d "$decoder" -m "$mem" | jq -r ".region")
> + if [ "$region" == "null" ]; then
> + err "$LINENO"
> + fi
> + $CXL enable-region "$region"
> +
> + dax=$($CXL list -X -r "$region" | jq -r ".[].daxregion.devices" | jq -r '.[].chardev')
> +
> + $DAXCTL reconfigure-device -m devdax "$dax"
> +
> + $CXL disable-region $region
> + $CXL destroy-region $region
> +}
> +
> +# Find a memory device to create regions on to test the destroy
> +readarray -t mems < <("$CXL" list -b cxl_test -M | jq -r '.[].memdev')
> +for mem in ${mems[@]}; do
> + ramsize=$($CXL list -m $mem | jq -r '.[].ram_size')
> + if [ "$ramsize" == "null" ]; then
> + continue
> + fi
> + decoder=$($CXL list -b cxl_test -D -d root -m "$mem" |
> + jq -r ".[] |
> + select(.volatile_capable == true) |
> + select(.nr_targets == 1) |
> + select(.size >= ${ramsize}) |
> + .decoder")
> + if [[ $decoder ]]; then
> + check_destroy_ram $mem $decoder
> + check_destroy_devdax $mem $decoder
> + break
> + fi
> +done
> +
> +check_dmesg "$LINENO"
> +
> +modprobe -r cxl_test
> diff --git a/test/meson.build b/test/meson.build
> index 2706fa5d633c..126d663dfce2 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -158,6 +158,7 @@ cxl_xor_region = find_program('cxl-xor-region.sh')
> cxl_update_firmware = find_program('cxl-update-firmware.sh')
> cxl_events = find_program('cxl-events.sh')
> cxl_poison = find_program('cxl-poison.sh')
> +cxl_destroy_region = find_program('cxl-destroy-region.sh')
>
> tests = [
> [ 'libndctl', libndctl, 'ndctl' ],
> @@ -188,6 +189,7 @@ tests = [
> [ 'cxl-xor-region.sh', cxl_xor_region, 'cxl' ],
> [ 'cxl-events.sh', cxl_events, 'cxl' ],
> [ 'cxl-poison.sh', cxl_poison, 'cxl' ],
> + [ 'cxl-destroy-region.sh', cxl_destroy_region, 'cxl' ],
> ]
>
> if get_option('destructive').enabled()
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
2023-12-01 4:06 ` [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test Ira Weiny
2023-12-01 17:40 ` Dave Jiang
@ 2023-12-01 19:39 ` Alison Schofield
2023-12-04 18:05 ` Ira Weiny
1 sibling, 1 reply; 10+ messages in thread
From: Alison Schofield @ 2023-12-01 19:39 UTC (permalink / raw
To: Ira Weiny; +Cc: Vishal Verma, Dave Jiang, Dan Williams, nvdimm, linux-cxl
On Thu, Nov 30, 2023 at 08:06:13PM -0800, Ira Weiny wrote:
> Commit 9399aa667ab0 ("cxl/region: Add -f option for disable-region")
> introduced a regression when destroying a region.
>
> Add a tests for destroying a region.
>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
> test/cxl-destroy-region.sh | 76 ++++++++++++++++++++++++++++++++++++++++++++++
> test/meson.build | 2 ++
> 2 files changed, 78 insertions(+)
>
> diff --git a/test/cxl-destroy-region.sh b/test/cxl-destroy-region.sh
> new file mode 100644
> index 000000000000..251720a98688
> --- /dev/null
> +++ b/test/cxl-destroy-region.sh
> @@ -0,0 +1,76 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2023 Intel Corporation. All rights reserved.
> +
> +. $(dirname $0)/common
> +
> +rc=77
> +
> +set -ex
> +
> +trap 'err $LINENO' ERR
> +
> +check_prereq "jq"
> +
> +modprobe -r cxl_test
> +modprobe cxl_test
> +rc=1
> +
> +check_destroy_ram()
> +{
> + mem=$1
> + decoder=$2
> +
> + region=$($CXL create-region -d "$decoder" -m "$mem" | jq -r ".region")
> + if [ "$region" == "null" ]; then
> + err "$LINENO"
> + fi
> + $CXL enable-region "$region"
> +
> + # default is memory is system-ram offline
> + $CXL disable-region $region
> + $CXL destroy-region $region
> +}
> +
> +check_destroy_devdax()
> +{
> + mem=$1
> + decoder=$2
> +
> + region=$($CXL create-region -d "$decoder" -m "$mem" | jq -r ".region")
> + if [ "$region" == "null" ]; then
> + err "$LINENO"
> + fi
> + $CXL enable-region "$region"
> +
> + dax=$($CXL list -X -r "$region" | jq -r ".[].daxregion.devices" | jq -r '.[].chardev')
> +
> + $DAXCTL reconfigure-device -m devdax "$dax"
> +
> + $CXL disable-region $region
> + $CXL destroy-region $region
> +}
> +
> +# Find a memory device to create regions on to test the destroy
> +readarray -t mems < <("$CXL" list -b cxl_test -M | jq -r '.[].memdev')
> +for mem in ${mems[@]}; do
> + ramsize=$($CXL list -m $mem | jq -r '.[].ram_size')
> + if [ "$ramsize" == "null" ]; then
> + continue
> + fi
> + decoder=$($CXL list -b cxl_test -D -d root -m "$mem" |
> + jq -r ".[] |
> + select(.volatile_capable == true) |
> + select(.nr_targets == 1) |
> + select(.size >= ${ramsize}) |
> + .decoder")
> + if [[ $decoder ]]; then
> + check_destroy_ram $mem $decoder
> + check_destroy_devdax $mem $decoder
> + break
> + fi
> +done
Does this need to check results of the region disable & destroy?
Did the regression this is after leave a trace in the dmesg log,
so checking that is all that's needed?
> +
> +check_dmesg "$LINENO"
> +
> +modprobe -r cxl_test
> diff --git a/test/meson.build b/test/meson.build
> index 2706fa5d633c..126d663dfce2 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -158,6 +158,7 @@ cxl_xor_region = find_program('cxl-xor-region.sh')
> cxl_update_firmware = find_program('cxl-update-firmware.sh')
> cxl_events = find_program('cxl-events.sh')
> cxl_poison = find_program('cxl-poison.sh')
> +cxl_destroy_region = find_program('cxl-destroy-region.sh')
>
> tests = [
> [ 'libndctl', libndctl, 'ndctl' ],
> @@ -188,6 +189,7 @@ tests = [
> [ 'cxl-xor-region.sh', cxl_xor_region, 'cxl' ],
> [ 'cxl-events.sh', cxl_events, 'cxl' ],
> [ 'cxl-poison.sh', cxl_poison, 'cxl' ],
> + [ 'cxl-destroy-region.sh', cxl_destroy_region, 'cxl' ],
> ]
>
> if get_option('destructive').enabled()
>
> --
> 2.42.0
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
2023-12-01 19:39 ` Alison Schofield
@ 2023-12-04 18:05 ` Ira Weiny
2023-12-04 20:42 ` Verma, Vishal L
0 siblings, 1 reply; 10+ messages in thread
From: Ira Weiny @ 2023-12-04 18:05 UTC (permalink / raw
To: Alison Schofield, Ira Weiny
Cc: Vishal Verma, Dave Jiang, Dan Williams, nvdimm, linux-cxl
Alison Schofield wrote:
> On Thu, Nov 30, 2023 at 08:06:13PM -0800, Ira Weiny wrote:
[snip]
> > +
> > +check_destroy_devdax()
> > +{
> > + mem=$1
> > + decoder=$2
> > +
> > + region=$($CXL create-region -d "$decoder" -m "$mem" | jq -r ".region")
> > + if [ "$region" == "null" ]; then
> > + err "$LINENO"
> > + fi
> > + $CXL enable-region "$region"
> > +
> > + dax=$($CXL list -X -r "$region" | jq -r ".[].daxregion.devices" | jq -r '.[].chardev')
> > +
> > + $DAXCTL reconfigure-device -m devdax "$dax"
> > +
> > + $CXL disable-region $region
> > + $CXL destroy-region $region
> > +}
> > +
> > +# Find a memory device to create regions on to test the destroy
> > +readarray -t mems < <("$CXL" list -b cxl_test -M | jq -r '.[].memdev')
> > +for mem in ${mems[@]}; do
> > + ramsize=$($CXL list -m $mem | jq -r '.[].ram_size')
> > + if [ "$ramsize" == "null" ]; then
> > + continue
> > + fi
> > + decoder=$($CXL list -b cxl_test -D -d root -m "$mem" |
> > + jq -r ".[] |
> > + select(.volatile_capable == true) |
> > + select(.nr_targets == 1) |
> > + select(.size >= ${ramsize}) |
> > + .decoder")
> > + if [[ $decoder ]]; then
> > + check_destroy_ram $mem $decoder
> > + check_destroy_devdax $mem $decoder
> > + break
> > + fi
> > +done
>
> Does this need to check results of the region disable & destroy?
>
> Did the regression this is after leave a trace in the dmesg log,
> so checking that is all that's needed?
>
The regression causes
check_destroy_devdax()
$CXL disable-region $region
to fail. That command failure will exit with an error which causes the
test script to exit with that error as well.
At least that is what happened when I used this to test the fix. I'll
defer to Vishal if there is a more explicit or better way to check for
that cxl-cli command to fail.
Ira
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
2023-12-04 18:05 ` Ira Weiny
@ 2023-12-04 20:42 ` Verma, Vishal L
2023-12-05 21:22 ` Ira Weiny
0 siblings, 1 reply; 10+ messages in thread
From: Verma, Vishal L @ 2023-12-04 20:42 UTC (permalink / raw
To: Schofield, Alison, Weiny, Ira
Cc: Williams, Dan J, Jiang, Dave, linux-cxl@vger.kernel.org,
nvdimm@lists.linux.dev
On Mon, 2023-12-04 at 10:05 -0800, Ira Weiny wrote:
> Alison Schofield wrote:
> > On Thu, Nov 30, 2023 at 08:06:13PM -0800, Ira Weiny wrote:
>
> [snip]
>
> > > +
> > > +check_destroy_devdax()
> > > +{
> > > + mem=$1
> > > + decoder=$2
> > > +
> > > + region=$($CXL create-region -d "$decoder" -m "$mem" | jq -r ".region")
> > > + if [ "$region" == "null" ]; then
> > > + err "$LINENO"
> > > + fi
> > > + $CXL enable-region "$region"
> > > +
> > > + dax=$($CXL list -X -r "$region" | jq -r ".[].daxregion.devices" | jq -r '.[].chardev')
> > > +
> > > + $DAXCTL reconfigure-device -m devdax "$dax"
> > > +
> > > + $CXL disable-region $region
> > > + $CXL destroy-region $region
> > > +}
> > > +
> > > +# Find a memory device to create regions on to test the destroy
> > > +readarray -t mems < <("$CXL" list -b cxl_test -M | jq -r '.[].memdev')
> > > +for mem in ${mems[@]}; do
> > > + ramsize=$($CXL list -m $mem | jq -r '.[].ram_size')
> > > + if [ "$ramsize" == "null" ]; then
> > > + continue
> > > + fi
> > > + decoder=$($CXL list -b cxl_test -D -d root -m "$mem" |
> > > + jq -r ".[] |
> > > + select(.volatile_capable == true) |
> > > + select(.nr_targets == 1) |
> > > + select(.size >= ${ramsize}) |
> > > + .decoder")
> > > + if [[ $decoder ]]; then
> > > + check_destroy_ram $mem $decoder
> > > + check_destroy_devdax $mem $decoder
> > > + break
> > > + fi
> > > +done
> >
> > Does this need to check results of the region disable & destroy?
> >
> > Did the regression this is after leave a trace in the dmesg log,
> > so checking that is all that's needed?
> >
>
> The regression causes
>
> check_destroy_devdax()
> $CXL disable-region $region
>
> to fail. That command failure will exit with an error which causes the
> test script to exit with that error as well.
>
> At least that is what happened when I used this to test the fix. I'll
> defer to Vishal if there is a more explicit or better way to check for
> that cxl-cli command to fail.
>
Correct, the set -e will cause the script to abort with an error exit
code whenever a command fails.
I do wonder if we need this new test - with Dave's patch here[1],
destroy-region and disable-region both use the same helper that
performs the libdaxctl checks.
cxl-create-region.sh already has flows that create a region and then
destroy it. Those should now cover this case as well yeah?
[1]: https://lore.kernel.org/all/170112921107.2687457.2741231995154639197.stgit@djiang5-mobl3/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
2023-12-04 20:42 ` Verma, Vishal L
@ 2023-12-05 21:22 ` Ira Weiny
2023-12-06 3:00 ` Verma, Vishal L
0 siblings, 1 reply; 10+ messages in thread
From: Ira Weiny @ 2023-12-05 21:22 UTC (permalink / raw
To: Verma, Vishal L, Schofield, Alison, Weiny, Ira
Cc: Williams, Dan J, Jiang, Dave, linux-cxl@vger.kernel.org,
nvdimm@lists.linux.dev
Verma, Vishal L wrote:
[snip]
> > > > +# Find a memory device to create regions on to test the destroy
> > > > +readarray -t mems < <("$CXL" list -b cxl_test -M | jq -r '.[].memdev')
> > > > +for mem in ${mems[@]}; do
> > > > + ramsize=$($CXL list -m $mem | jq -r '.[].ram_size')
> > > > + if [ "$ramsize" == "null" ]; then
> > > > + continue
> > > > + fi
> > > > + decoder=$($CXL list -b cxl_test -D -d root -m "$mem" |
> > > > + jq -r ".[] |
> > > > + select(.volatile_capable == true) |
> > > > + select(.nr_targets == 1) |
> > > > + select(.size >= ${ramsize}) |
> > > > + .decoder")
> > > > + if [[ $decoder ]]; then
> > > > + check_destroy_ram $mem $decoder
> > > > + check_destroy_devdax $mem $decoder
> > > > + break
> > > > + fi
> > > > +done
> > >
> > > Does this need to check results of the region disable & destroy?
> > >
> > > Did the regression this is after leave a trace in the dmesg log,
> > > so checking that is all that's needed?
> > >
> >
> > The regression causes
> >
> > check_destroy_devdax()
> > $CXL disable-region $region
> >
> > to fail. That command failure will exit with an error which causes the
> > test script to exit with that error as well.
> >
> > At least that is what happened when I used this to test the fix. I'll
> > defer to Vishal if there is a more explicit or better way to check for
> > that cxl-cli command to fail.
> >
> Correct, the set -e will cause the script to abort with an error exit
> code whenever a command fails.
>
> I do wonder if we need this new test - with Dave's patch here[1],
I'm not sure.
> destroy-region and disable-region both use the same helper that
> performs the libdaxctl checks.
>
> cxl-create-region.sh already has flows that create a region and then
> destroy it. Those should now cover this case as well yeah?
I thought it would have but I don't think it covers the case where the dax
device is not system ram (the default when creating a region).
Ira
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
2023-12-05 21:22 ` Ira Weiny
@ 2023-12-06 3:00 ` Verma, Vishal L
0 siblings, 0 replies; 10+ messages in thread
From: Verma, Vishal L @ 2023-12-06 3:00 UTC (permalink / raw
To: Schofield, Alison, Weiny, Ira
Cc: Williams, Dan J, Jiang, Dave, linux-cxl@vger.kernel.org,
nvdimm@lists.linux.dev
On Tue, 2023-12-05 at 13:22 -0800, Ira Weiny wrote:
> Verma, Vishal L wrote:
>
> [snip]
>
> > > >
> > >
> > Correct, the set -e will cause the script to abort with an error exit
> > code whenever a command fails.
> >
> > I do wonder if we need this new test - with Dave's patch here[1],
>
> I'm not sure.
>
> > destroy-region and disable-region both use the same helper that
> > performs the libdaxctl checks.
> >
> > cxl-create-region.sh already has flows that create a region and then
> > destroy it. Those should now cover this case as well yeah?
>
> I thought it would have but I don't think it covers the case where the dax
> device is not system ram (the default when creating a region).
Oh, you're right, the devdax case isn't covered by the other test. I'll
keep this then, thanks!
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-12-06 3:00 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-01 4:06 [PATCH ndctl RESEND 0/2] ndctl: Fix ups for region destroy Ira Weiny
2023-12-01 4:06 ` [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test Ira Weiny
2023-12-01 17:40 ` Dave Jiang
2023-12-01 19:39 ` Alison Schofield
2023-12-04 18:05 ` Ira Weiny
2023-12-04 20:42 ` Verma, Vishal L
2023-12-05 21:22 ` Ira Weiny
2023-12-06 3:00 ` Verma, Vishal L
2023-12-01 4:06 ` [PATCH ndctl RESEND 2/2] cxl/region: Fix memory device teardown in disable-region Ira Weiny
2023-12-01 17:39 ` 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).