Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
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!
Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
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
Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
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/
Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
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
Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
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
> Signed-off-by: Ira Weiny
> ---
> 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 ..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
>
>
Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
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
> Signed-off-by: Ira Weiny
Reviewed-by: Dave Jiang
> ---
> 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 ..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()
>
