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 <[email protected]> > > > > 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_tudjjod...@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", ®ion_id) == 1 - || sscanf(ident, "region%d", ®ion_id) == 1) + || sscanf(ident, "region%d", ®ion_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 -- [email protected] To unsubscribe send an email to [email protected]
