Re: [PATCH ndctl v2 10/10] daxctl/test: Add tests for dynamic dax regions

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Thu, 2020-12-10 at 15:01 +0000, Joao Martins wrote:
> On 7/21/20 5:49 PM, Joao Martins wrote:
> > On 7/13/20 5:08 PM, Joao Martins wrote:
> > > Add a couple tests which exercise the new sysfs based
> > > interface for Soft-Reserved regions (by EFI/HMAT, or
> > > efi_fake_mem).
> > > 
> > > The tests exercise the daxctl orchestration surrounding
> > > for creating/disabling/destroying/reconfiguring devices.
> > > Furthermore it exercises dax region space allocation
> > > code paths particularly:
> > > 
> > >  1) zeroing out and reconfiguring a dax device from
> > >  its current size to be max available and back to initial
> > >  size
> > > 
> > >  2) creates devices from holes in the beginning,
> > >  middle of the region.
> > > 
> > >  3) reconfigures devices in a interleaving fashion
> > > 
> > >  4) test adjust of the region towards beginning and end
> > > 
> > > The tests assume you pass a valid efi_fake_mem parameter
> > > marked as EFI_MEMORY_SP e.g.
> > > 
> > > 	efi_fake_mem=112G@16G:0x40000
> > > 
> > > Naturally it bails out from the test if hmem device driver
> > > isn't loaded or builtin. If hmem regions are found, only
> > > region 0 is used, and the others remain untouched.
> > > 
> > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> > 
> > Following your suggestion[0], I added a couple more validations
> > to this test suite, covering the mappings. So on top of this patch
> > I added the following snip below the scissors mark. Mainly, I check
> > that the size calculated by mappingNNNN is the same as advertised by
> > the sysfs size attribute, thus looping through all the mappings.
> > 
> > Perhaps it would be enough to have just such validation in setup_dev()
> > to catch the bug in [0]. But I went ahead and also validated the test
> > cases where a certain amount of mappings are meant to be created.
> > 
> > My only worry is the last piece in daxctl_test_adjust() where we might
> > be tying too much on how a kernel version picks space from the region;
> > should this logic change in an unforeseeable future (e.g. allowing space
> > at the beginning to be adjusted). Otherwise, if this no concern, let me
> > know and I can resend a v3 with the adjustment below.
> > 
> 
> Ping?

Hi Joao,

Thanks for the patience on these, I've gone through the patches in
preparation for the next release, and they all look mostly fine. I had a
few minor fixups - to the documentation and the test (fixup module name,
and shellcheck complaints). I've appended a diff below of all the fixups
I added.

I've also included the patch below for the mapping size validation. I
think the concern for future kernel layout changes is valid, but if/when
that happens, we can always come back and relax or adjust the test as
needed. So for now, I think having a pickier test should be better than
not having one.

> 
> > ----->8------
> > Subject: Validate @size versus mappingX sizes
> > 
> > [0]
> > https://lore.kernel.org/linux-nvdimm/CAPcyv4hFS7JS9s7cUY=2Ru2kUTRsesxwX1PGnnc_tudJjoDUGw@mail.gmail.com/
> > 
> > ---
> > 
> >  test/daxctl-create.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 63 insertions(+), 1 deletion(-)
> > 

My fixups:

---

Documentation/daxctl/daxctl-create-device.txt      | 18 +++-----------
 Documentation/daxctl/daxctl-destroy-device.txt     | 22 ++++--------------
 Documentation/daxctl/daxctl-disable-device.txt     | 22 ++++--------------
 Documentation/daxctl/daxctl-enable-device.txt      | 22 ++++--------------
 Documentation/daxctl/daxctl-reconfigure-device.txt | 19 ++++-----------
 Documentation/daxctl/human-option.txt              |  8 +++++++
 Documentation/daxctl/region-option.txt             |  8 +++++++
 Documentation/daxctl/verbose-option.txt            |  5 ++++
 util/filter.c                                      |  2 +-
 test/daxctl-create.sh                              | 76 ++++++++++++++++++++++++++++++------------------------------
 10 files changed, 82 insertions(+), 120 deletions(-)

---

diff --git a/Documentation/daxctl/daxctl-create-device.txt b/Documentation/daxctl/daxctl-create-device.txt
index 648d254..70029ab 100644
--- a/Documentation/daxctl/daxctl-create-device.txt
+++ b/Documentation/daxctl/daxctl-create-device.txt
@@ -71,12 +71,7 @@ EFI memory map with EFI_MEMORY_SP. The resultant ranges mean that it's
 
 OPTIONS
 -------
--r::
---region=::
-	Restrict the operation to devices belonging to the specified region(s).
-	A device-dax region is a contiguous range of memory that hosts one or
-	more /dev/daxX.Y devices, where X is the region id and Y is the device
-	instance id.
+include::region-option.txt[]
 
 -s::
 --size=::
@@ -87,16 +82,9 @@ OPTIONS
 
 	The size must be a multiple of the region alignment.
 
--u::
---human::
-	By default the command will output machine-friendly raw-integer
-	data. Instead, with this flag, numbers representing storage size
-	will be formatted as human readable strings with units, other
-	fields are converted to hexadecimal strings.
+include::human-option.txt[]
 
--v::
---verbose::
-	Emit more debug messages
+include::verbose-option.txt[]
 
 include::../copyright.txt[]
 
diff --git a/Documentation/daxctl/daxctl-destroy-device.txt b/Documentation/daxctl/daxctl-destroy-device.txt
index 1c91cb2..a63ab0c 100644
--- a/Documentation/daxctl/daxctl-destroy-device.txt
+++ b/Documentation/daxctl/daxctl-destroy-device.txt
@@ -38,23 +38,11 @@ Destroys a dax device in 'devdax' mode.
 
 OPTIONS
 -------
--r::
---region=::
-	Restrict the operation to devices belonging to the specified region(s).
-	A device-dax region is a contiguous range of memory that hosts one or
-	more /dev/daxX.Y devices, where X is the region id and Y is the device
-	instance id.
-
--u::
---human::
-	By default the command will output machine-friendly raw-integer
-	data. Instead, with this flag, numbers representing storage size
-	will be formatted as human readable strings with units, other
-	fields are converted to hexadecimal strings.
-
--v::
---verbose::
-	Emit more debug messages
+include::region-option.txt[]
+
+include::human-option.txt[]
+
+include::verbose-option.txt[]
 
 include::../copyright.txt[]
 
diff --git a/Documentation/daxctl/daxctl-disable-device.txt b/Documentation/daxctl/daxctl-disable-device.txt
index 383aeeb..ee9f6e8 100644
--- a/Documentation/daxctl/daxctl-disable-device.txt
+++ b/Documentation/daxctl/daxctl-disable-device.txt
@@ -33,23 +33,11 @@ Disables a dax device in 'devdax' mode.
 
 OPTIONS
 -------
--r::
---region=::
-	Restrict the operation to devices belonging to the specified region(s).
-	A device-dax region is a contiguous range of memory that hosts one or
-	more /dev/daxX.Y devices, where X is the region id and Y is the device
-	instance id.
-
--u::
---human::
-	By default the command will output machine-friendly raw-integer
-	data. Instead, with this flag, numbers representing storage size
-	will be formatted as human readable strings with units, other
-	fields are converted to hexadecimal strings.
-
--v::
---verbose::
-	Emit more debug messages
+include::region-option.txt[]
+
+include::human-option.txt[]
+
+include::verbose-option.txt[]
 
 include::../copyright.txt[]
 
diff --git a/Documentation/daxctl/daxctl-enable-device.txt b/Documentation/daxctl/daxctl-enable-device.txt
index 6410d92..24cdcf3 100644
--- a/Documentation/daxctl/daxctl-enable-device.txt
+++ b/Documentation/daxctl/daxctl-enable-device.txt
@@ -34,23 +34,11 @@ Enables a dax device in 'devdax' mode.
 
 OPTIONS
 -------
--r::
---region=::
-	Restrict the operation to devices belonging to the specified region(s).
-	A device-dax region is a contiguous range of memory that hosts one or
-	more /dev/daxX.Y devices, where X is the region id and Y is the device
-	instance id.
-
--u::
---human::
-	By default the command will output machine-friendly raw-integer
-	data. Instead, with this flag, numbers representing storage size
-	will be formatted as human readable strings with units, other
-	fields are converted to hexadecimal strings.
-
--v::
---verbose::
-	Emit more debug messages
+include::region-option.txt[]
+
+include::human-option.txt[]
+
+include::verbose-option.txt[]
 
 include::../copyright.txt[]
 
diff --git a/Documentation/daxctl/daxctl-reconfigure-device.txt b/Documentation/daxctl/daxctl-reconfigure-device.txt
index 8caae43..9a11ff5 100644
--- a/Documentation/daxctl/daxctl-reconfigure-device.txt
+++ b/Documentation/daxctl/daxctl-reconfigure-device.txt
@@ -121,12 +121,7 @@ refrain from then onlining it.
 
 OPTIONS
 -------
--r::
---region=::
-	Restrict the operation to devices belonging to the specified region(s).
-	A device-dax region is a contiguous range of memory that hosts one or
-	more /dev/daxX.Y devices, where X is the region id and Y is the device
-	instance id.
+include::region-option.txt[]
 
 -s::
 --size=::
@@ -161,16 +156,10 @@ include::movable-options.txt[]
 	to offline the memory on the NUMA node associated with the dax device
 	before converting it back to "devdax" mode.
 
--u::
---human::
-	By default the command will output machine-friendly raw-integer
-	data. Instead, with this flag, numbers representing storage size
-	will be formatted as human readable strings with units, other
-	fields are converted to hexadecimal strings.
 
--v::
---verbose::
-	Emit more debug messages
+include::human-option.txt[]
+
+include::verbose-option.txt[]
 
 include::../copyright.txt[]
 
diff --git a/Documentation/daxctl/human-option.txt b/Documentation/daxctl/human-option.txt
new file mode 100644
index 0000000..2f4de7a
--- /dev/null
+++ b/Documentation/daxctl/human-option.txt
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+
+-u::
+--human::
+	By default the command will output machine-friendly raw-integer
+	data. Instead, with this flag, numbers representing storage size
+	will be formatted as human readable strings with units, other
+	fields are converted to hexadecimal strings.
diff --git a/Documentation/daxctl/region-option.txt b/Documentation/daxctl/region-option.txt
new file mode 100644
index 0000000..a824e22
--- /dev/null
+++ b/Documentation/daxctl/region-option.txt
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+
+-r::
+--region=::
+	Restrict the operation to devices belonging to the specified region(s).
+	A device-dax region is a contiguous range of memory that hosts one or
+	more /dev/daxX.Y devices, where X is the region id and Y is the device
+	instance id.
diff --git a/Documentation/daxctl/verbose-option.txt b/Documentation/daxctl/verbose-option.txt
new file mode 100644
index 0000000..cb62c8e
--- /dev/null
+++ b/Documentation/daxctl/verbose-option.txt
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0
+
+-v::
+--verbose::
+	Emit more debug messages
diff --git a/util/filter.c b/util/filter.c
index 7c8debb..8c78f32 100644
--- a/util/filter.c
+++ b/util/filter.c
@@ -342,7 +342,7 @@ struct daxctl_region *util_daxctl_region_filter(struct daxctl_region *region,
 		return region;
 
 	if ((sscanf(ident, "%d", &region_id) == 1
-				|| sscanf(ident, "region%d", &region_id) == 1)
+			|| sscanf(ident, "region%d", &region_id) == 1)
 			&& daxctl_region_get_id(region) == region_id)
 		return region;
 
diff --git a/test/daxctl-create.sh b/test/daxctl-create.sh
index 8dbc00f..a4fbe06 100755
--- a/test/daxctl-create.sh
+++ b/test/daxctl-create.sh
@@ -20,8 +20,8 @@ find_testdev()
 
 	# The hmem driver is needed to change the device mode, only
 	# kernels >= v5.6 might have it available. Skip if not.
-	if ! modinfo hmem; then
-		# check if hmem is builtin
+	if ! modinfo dax_hmem; then
+		# check if dax_hmem is builtin
 		if [ ! -d "/sys/module/device_hmem" ]; then
 			printf "Unable to find hmem module\n"
 			exit $rc
@@ -66,7 +66,7 @@ reset_dev()
 	test -n "$testdev"
 
 	"$DAXCTL" disable-device "$testdev"
-	"$DAXCTL" reconfigure-device -s $available "$testdev"
+	"$DAXCTL" reconfigure-device -s "$available" "$testdev"
 	"$DAXCTL" enable-device "$testdev"
 }
 
@@ -76,7 +76,7 @@ reset()
 
 	"$DAXCTL" disable-device -r 0 all
 	"$DAXCTL" destroy-device -r 0 all
-	"$DAXCTL" reconfigure-device -s $available "$testdev"
+	"$DAXCTL" reconfigure-device -s "$available" "$testdev"
 }
 
 clear_dev()
@@ -91,8 +91,8 @@ test_pass()
 
 	# Available size
 	_available_size=$("$DAXCTL" list -r 0 | jq -er '.[0].available_size | .//""')
-	if [[ ! $_available_size == $available ]]; then
-		printf "Unexpected available size $_available_size != $available\n"
+	if [[ ! $_available_size == "$available" ]]; then
+		echo "Unexpected available size $_available_size != $available"
 		exit "$rc"
 	fi
 }
@@ -103,7 +103,7 @@ fail_if_available()
 
 	_size=$("$DAXCTL" list -r 0 | jq -er '.[0].available_size | .//""')
 	if [[ $_size ]]; then
-		printf "Unexpected available size $_size\n"
+		echo "Unexpected available size $_size"
 		exit "$rc"
 	fi
 }
@@ -124,8 +124,8 @@ daxctl_get_size_by_mapping()
 	local _start=0
 	local _end=0
 
-	_start=$(cat $1/start)
-	_end=$(cat $1/end)
+	_start=$(cat "$1"/start)
+	_end=$(cat "$1"/end)
 	((size=size + _end - _start + 1))
 	echo $size
 }
@@ -138,10 +138,10 @@ daxctl_get_nr_mappings()
 	local path=""
 
 	path=$(readlink -f /sys/bus/dax/devices/"$1"/)
-	until ! [ -d $path/mapping$i ]
+	until ! [ -d "$path/mapping$i" ]
 	do
 		_size=$(daxctl_get_size_by_mapping "$path/mapping$i")
-		if [[ $msize == 0 ]]; then
+		if [[ $_size == 0 ]]; then
 			i=0
 			break
 		fi
@@ -151,8 +151,8 @@ daxctl_get_nr_mappings()
 
 	# Return number of mappings if the sizes between size field
 	# and the one computed by mappingNNN are the same
-	_size=$("$DAXCTL" list -d $1 | jq -er '.[0].size | .//""')
-	if [[ ! $_size == $devsize ]]; then
+	_size=$("$DAXCTL" list -d "$1" | jq -er '.[0].size | .//""')
+	if [[ ! $_size == "$devsize" ]]; then
 		echo 0
 	else
 		echo $i
@@ -163,7 +163,7 @@ daxctl_test_multi()
 {
 	local daxdev
 
-	size=$(expr $available / 4)
+	size=$((available / 4))
 
 	if [[ $2 ]]; then
 		"$DAXCTL" disable-device "$testdev"
@@ -171,24 +171,24 @@ daxctl_test_multi()
 	fi
 
 	daxdev_1=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev')
-	test -n $daxdev_1
+	test -n "$daxdev_1"
 
 	daxdev_2=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev')
-	test -n $daxdev_2
+	test -n "$daxdev_2"
 
 	if [[ ! $2 ]]; then
 		daxdev_3=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev')
-		test -n $daxdev_3
+		test -n "$daxdev_3"
 	fi
 
 	# Hole
-	"$DAXCTL" disable-device  $1 &&	"$DAXCTL" destroy-device  $1
+	"$DAXCTL" disable-device  "$1" && "$DAXCTL" destroy-device "$1"
 
 	# Pick space in the created hole and at the end
-	new_size=$(expr $size \* 2)
-	daxdev_4=$("$DAXCTL" create-device -r 0 -s $new_size | jq -er '.[].chardev')
-	test -n $daxdev_4
-	test $(daxctl_get_nr_mappings $daxdev_4) -eq 2
+	new_size=$((size * 2))
+	daxdev_4=$("$DAXCTL" create-device -r 0 -s "$new_size" | jq -er '.[].chardev')
+	test -n "$daxdev_4"
+	test "$(daxctl_get_nr_mappings "$daxdev_4")" -eq 2
 
 	fail_if_available
 
@@ -201,7 +201,7 @@ daxctl_test_multi_reconfig()
 	local ncfgs=$1
 	local daxdev
 
-	size=$(expr $available / $ncfgs)
+	size=$((available / ncfgs))
 
 	test -n "$testdev"
 
@@ -212,19 +212,19 @@ daxctl_test_multi_reconfig()
 	daxdev_1=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev')
 	"$DAXCTL" disable-device "$daxdev_1"
 
-	start=$(expr $size + $size)
-	max=$(expr $ncfgs / 2 \* $size)
+	start=$((size + size))
+	max=$((size * ncfgs / 2))
 	for i in $(seq $start $size $max)
 	do
 		"$DAXCTL" disable-device "$testdev"
-		"$DAXCTL" reconfigure-device -s $i "$testdev"
+		"$DAXCTL" reconfigure-device -s "$i" "$testdev"
 
 		"$DAXCTL" disable-device "$daxdev_1"
-		"$DAXCTL" reconfigure-device -s $i "$daxdev_1"
+		"$DAXCTL" reconfigure-device -s "$i" "$daxdev_1"
 	done
 
-	test $(daxctl_get_nr_mappings $testdev) -eq $((ncfgs / 2))
-	test $(daxctl_get_nr_mappings $daxdev_1) -eq $((ncfgs / 2))
+	test "$(daxctl_get_nr_mappings "$testdev")" -eq $((ncfgs / 2))
+	test "$(daxctl_get_nr_mappings "$daxdev_1")" -eq $((ncfgs / 2))
 
 	fail_if_available
 
@@ -237,15 +237,15 @@ daxctl_test_adjust()
 	local ncfgs=4
 	local daxdev
 
-	size=$(expr $available / $ncfgs)
+	size=$((available / ncfgs))
 
 	test -n "$testdev"
 
-	start=$(expr $size + $size)
+	start=$((size + size))
 	for i in $(seq 1 1 $ncfgs)
 	do
-		daxdev=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev')
-		test $(daxctl_get_nr_mappings $daxdev) -eq 1
+		daxdev=$("$DAXCTL" create-device -r 0 -s "$size" | jq -er '.[].chardev')
+		test "$(daxctl_get_nr_mappings "$daxdev")" -eq 1
 	done
 
 	daxdev=$(daxctl_get_dev "dax0.1")
@@ -255,18 +255,18 @@ daxctl_test_adjust()
 
 	daxdev=$(daxctl_get_dev "dax0.2")
 	"$DAXCTL" disable-device "$daxdev"
-	"$DAXCTL" reconfigure-device -s $(expr $size \* 2) "$daxdev"
+	"$DAXCTL" reconfigure-device -s $((size * 2)) "$daxdev"
 	# Allocates space at the beginning: expect two mappings as
 	# as don't adjust the mappingX region. This is because we
 	# preserve the relative page_offset of existing allocations
-	test $(daxctl_get_nr_mappings $daxdev) -eq 2
+	test "$(daxctl_get_nr_mappings "$daxdev")" -eq 2
 
 	daxdev=$(daxctl_get_dev "dax0.3")
 	"$DAXCTL" disable-device "$daxdev"
-	"$DAXCTL" reconfigure-device -s $(expr $size \* 2) "$daxdev"
+	"$DAXCTL" reconfigure-device -s $((size * 2)) "$daxdev"
 	# Adjusts space at the end, expect one mapping because we are
 	# able to extend existing region range.
-	test $(daxctl_get_nr_mappings $daxdev) -eq 1
+	test "$(daxctl_get_nr_mappings "$daxdev")" -eq 1
 
 	fail_if_available
 
@@ -293,7 +293,7 @@ daxctl_test1()
 	daxdev=$("$DAXCTL" create-device -r 0 | jq -er '.[].chardev')
 
 	test -n "$daxdev"
-	test $(daxctl_get_nr_mappings $daxdev) -eq 1
+	test "$(daxctl_get_nr_mappings "$daxdev")" -eq 1
 	fail_if_available
 
 	"$DAXCTL" disable-device "$daxdev" && "$DAXCTL" destroy-device "$daxdev"

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org