diff mbox series

[ndctl,RESEND,1/2] ndctl/test: Add destroy region test

Message ID 20231130-fix-region-destroy-v1-1-7f916d2bd379@intel.com
State Accepted
Commit 182e894ea2df03e051c027a2a11a16b8af617938
Headers show
Series ndctl: Fix ups for region destroy | expand

Commit Message

Ira Weiny Dec. 1, 2023, 4:06 a.m. UTC
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(+)

Comments

Dave Jiang Dec. 1, 2023, 5:40 p.m. UTC | #1
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()
>
Alison Schofield Dec. 1, 2023, 7:39 p.m. UTC | #2
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
> 
>
Ira Weiny Dec. 4, 2023, 6:05 p.m. UTC | #3
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
Verma, Vishal L Dec. 4, 2023, 8:42 p.m. UTC | #4
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/
Ira Weiny Dec. 5, 2023, 9:22 p.m. UTC | #5
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
Verma, Vishal L Dec. 6, 2023, 3 a.m. UTC | #6
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!
diff mbox series

Patch

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