| Message ID | 20200713160837.13774-11-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | daxctl: Support for sub-dividing soft-reserved regions | expand |
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. ----->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(-) diff --git a/test/daxctl-create.sh b/test/daxctl-create.sh index 0d35112b4119..8dbc00fc574f 100755 --- a/test/daxctl-create.sh +++ b/test/daxctl-create.sh @@ -46,8 +46,16 @@ find_testdev() setup_dev() { + local rc=1 + local nmaps=0 test -n "$testdev" + nmaps=$(daxctl_get_nr_mappings "$testdev") + if [[ $nmaps == 0 ]]; then + printf "Device is idle" + exit "$rc" + fi + "$DAXCTL" disable-device "$testdev" "$DAXCTL" reconfigure-device -s 0 "$testdev" available=$("$DAXCTL" list -r 0 | jq -er '.[0].available_size | .//""') @@ -110,6 +118,47 @@ daxctl_get_mode() "$DAXCTL" list -d "$1" | jq -er '.[].mode' } +daxctl_get_size_by_mapping() +{ + local size=0 + local _start=0 + local _end=0 + + _start=$(cat $1/start) + _end=$(cat $1/end) + ((size=size + _end - _start + 1)) + echo $size +} + +daxctl_get_nr_mappings() +{ + local i=0 + local _size=0 + local devsize=0 + local path="" + + path=$(readlink -f /sys/bus/dax/devices/"$1"/) + until ! [ -d $path/mapping$i ] + do + _size=$(daxctl_get_size_by_mapping "$path/mapping$i") + if [[ $msize == 0 ]]; then + i=0 + break + fi + ((devsize=devsize + _size)) + ((i=i + 1)) + done + + # 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 + echo 0 + else + echo $i + fi +} + daxctl_test_multi() { local daxdev @@ -139,6 +188,7 @@ daxctl_test_multi() 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 fail_if_available @@ -173,6 +223,9 @@ daxctl_test_multi_reconfig() "$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)) + fail_if_available "$DAXCTL" disable-device "$daxdev_1" && "$DAXCTL" destroy-device "$daxdev_1" @@ -191,7 +244,8 @@ daxctl_test_adjust() start=$(expr $size + $size) for i in $(seq 1 1 $ncfgs) do - daxdev=$("$DAXCTL" create-device -r 0 -s $size) + 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") @@ -202,10 +256,17 @@ daxctl_test_adjust() daxdev=$(daxctl_get_dev "dax0.2") "$DAXCTL" disable-device "$daxdev" "$DAXCTL" reconfigure-device -s $(expr $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 daxdev=$(daxctl_get_dev "dax0.3") "$DAXCTL" disable-device "$daxdev" "$DAXCTL" reconfigure-device -s $(expr $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 fail_if_available @@ -232,6 +293,7 @@ daxctl_test1() daxdev=$("$DAXCTL" create-device -r 0 | jq -er '.[].chardev') test -n "$daxdev" + test $(daxctl_get_nr_mappings $daxdev) -eq 1 fail_if_available "$DAXCTL" disable-device "$daxdev" && "$DAXCTL" destroy-device "$daxdev"
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? > ----->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(-) > > diff --git a/test/daxctl-create.sh b/test/daxctl-create.sh > index 0d35112b4119..8dbc00fc574f 100755 > --- a/test/daxctl-create.sh > +++ b/test/daxctl-create.sh > @@ -46,8 +46,16 @@ find_testdev() > > setup_dev() > { > + local rc=1 > + local nmaps=0 > test -n "$testdev" > > + nmaps=$(daxctl_get_nr_mappings "$testdev") > + if [[ $nmaps == 0 ]]; then > + printf "Device is idle" > + exit "$rc" > + fi > + > "$DAXCTL" disable-device "$testdev" > "$DAXCTL" reconfigure-device -s 0 "$testdev" > available=$("$DAXCTL" list -r 0 | jq -er '.[0].available_size | .//""') > @@ -110,6 +118,47 @@ daxctl_get_mode() > "$DAXCTL" list -d "$1" | jq -er '.[].mode' > } > > +daxctl_get_size_by_mapping() > +{ > + local size=0 > + local _start=0 > + local _end=0 > + > + _start=$(cat $1/start) > + _end=$(cat $1/end) > + ((size=size + _end - _start + 1)) > + echo $size > +} > + > +daxctl_get_nr_mappings() > +{ > + local i=0 > + local _size=0 > + local devsize=0 > + local path="" > + > + path=$(readlink -f /sys/bus/dax/devices/"$1"/) > + until ! [ -d $path/mapping$i ] > + do > + _size=$(daxctl_get_size_by_mapping "$path/mapping$i") > + if [[ $msize == 0 ]]; then > + i=0 > + break > + fi > + ((devsize=devsize + _size)) > + ((i=i + 1)) > + done > + > + # 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 > + echo 0 > + else > + echo $i > + fi > +} > + > daxctl_test_multi() > { > local daxdev > @@ -139,6 +188,7 @@ daxctl_test_multi() > 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 > > fail_if_available > > @@ -173,6 +223,9 @@ daxctl_test_multi_reconfig() > "$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)) > + > fail_if_available > > "$DAXCTL" disable-device "$daxdev_1" && "$DAXCTL" destroy-device "$daxdev_1" > @@ -191,7 +244,8 @@ daxctl_test_adjust() > start=$(expr $size + $size) > for i in $(seq 1 1 $ncfgs) > do > - daxdev=$("$DAXCTL" create-device -r 0 -s $size) > + 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") > @@ -202,10 +256,17 @@ daxctl_test_adjust() > daxdev=$(daxctl_get_dev "dax0.2") > "$DAXCTL" disable-device "$daxdev" > "$DAXCTL" reconfigure-device -s $(expr $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 > > daxdev=$(daxctl_get_dev "dax0.3") > "$DAXCTL" disable-device "$daxdev" > "$DAXCTL" reconfigure-device -s $(expr $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 > > fail_if_available > > @@ -232,6 +293,7 @@ daxctl_test1() > daxdev=$("$DAXCTL" create-device -r 0 | jq -er '.[].chardev') > > test -n "$daxdev" > + 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 >
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", ®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"
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", ®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"
On 12/16/20 10:25 AM, Verma, Vishal L wrote: > 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. > The adjustments are looking good AFAICT. > 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. > Yeah, makes sense. Thanks! Joao
diff --git a/test/Makefile.am b/test/Makefile.am index 1d24a65ded8b..6b7c82f9a4e2 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -58,6 +58,7 @@ TESTS +=\ device-dax \ device-dax-fio.sh \ daxctl-devices.sh \ + daxctl-create.sh \ dm.sh \ mmap.sh diff --git a/test/daxctl-create.sh b/test/daxctl-create.sh new file mode 100755 index 000000000000..0d35112b4119 --- /dev/null +++ b/test/daxctl-create.sh @@ -0,0 +1,294 @@ +#!/bin/bash -Ex +# SPDX-License-Identifier: GPL-2.0 +# Copyright(c) 2020 Oracle Corporation. + +rc=77 +. $(dirname $0)/common + +trap 'cleanup $LINENO' ERR + +cleanup() +{ + printf "Error at line %d\n" "$1" + [[ $testdev ]] && reset + exit $rc +} + +find_testdev() +{ + local rc=77 + + # 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 [ ! -d "/sys/module/device_hmem" ]; then + printf "Unable to find hmem module\n" + exit $rc + fi + fi + + # find a victim region provided by dax_hmem + testpath=$("$DAXCTL" list -r 0 | jq -er '.[0].path | .//""') + if [[ ! "$testpath" == *"hmem"* ]]; then + printf "Unable to find a victim region\n" + exit "$rc" + fi + + # find a victim device + testdev=$("$DAXCTL" list -D -r 0 | jq -er '.[0].chardev | .//""') + if [[ ! $testdev ]]; then + printf "Unable to find a victim device\n" + exit "$rc" + fi + printf "Found victim dev: %s on region id 0\n" "$testdev" +} + +setup_dev() +{ + test -n "$testdev" + + "$DAXCTL" disable-device "$testdev" + "$DAXCTL" reconfigure-device -s 0 "$testdev" + available=$("$DAXCTL" list -r 0 | jq -er '.[0].available_size | .//""') +} + +reset_dev() +{ + test -n "$testdev" + + "$DAXCTL" disable-device "$testdev" + "$DAXCTL" reconfigure-device -s $available "$testdev" + "$DAXCTL" enable-device "$testdev" +} + +reset() +{ + test -n "$testdev" + + "$DAXCTL" disable-device -r 0 all + "$DAXCTL" destroy-device -r 0 all + "$DAXCTL" reconfigure-device -s $available "$testdev" +} + +clear_dev() +{ + "$DAXCTL" disable-device "$testdev" + "$DAXCTL" reconfigure-device -s 0 "$testdev" +} + +test_pass() +{ + local rc=1 + + # 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" + exit "$rc" + fi +} + +fail_if_available() +{ + local rc=1 + + _size=$("$DAXCTL" list -r 0 | jq -er '.[0].available_size | .//""') + if [[ $_size ]]; then + printf "Unexpected available size $_size\n" + exit "$rc" + fi +} + +daxctl_get_dev() +{ + "$DAXCTL" list -d "$1" | jq -er '.[].chardev' +} + +daxctl_get_mode() +{ + "$DAXCTL" list -d "$1" | jq -er '.[].mode' +} + +daxctl_test_multi() +{ + local daxdev + + size=$(expr $available / 4) + + if [[ $2 ]]; then + "$DAXCTL" disable-device "$testdev" + "$DAXCTL" reconfigure-device -s $size "$testdev" + fi + + daxdev_1=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev') + test -n $daxdev_1 + + daxdev_2=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev') + test -n $daxdev_2 + + if [[ ! $2 ]]; then + daxdev_3=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev') + test -n $daxdev_3 + fi + + # Hole + "$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 + + fail_if_available + + "$DAXCTL" disable-device -r 0 all + "$DAXCTL" destroy-device -r 0 all +} + +daxctl_test_multi_reconfig() +{ + local ncfgs=$1 + local daxdev + + size=$(expr $available / $ncfgs) + + test -n "$testdev" + + "$DAXCTL" disable-device "$testdev" + "$DAXCTL" reconfigure-device -s $size "$testdev" + "$DAXCTL" disable-device "$testdev" + + 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) + for i in $(seq $start $size $max) + do + "$DAXCTL" disable-device "$testdev" + "$DAXCTL" reconfigure-device -s $i "$testdev" + + "$DAXCTL" disable-device "$daxdev_1" + "$DAXCTL" reconfigure-device -s $i "$daxdev_1" + done + + fail_if_available + + "$DAXCTL" disable-device "$daxdev_1" && "$DAXCTL" destroy-device "$daxdev_1" +} + +daxctl_test_adjust() +{ + local rc=1 + local ncfgs=4 + local daxdev + + size=$(expr $available / $ncfgs) + + test -n "$testdev" + + start=$(expr $size + $size) + for i in $(seq 1 1 $ncfgs) + do + daxdev=$("$DAXCTL" create-device -r 0 -s $size) + done + + daxdev=$(daxctl_get_dev "dax0.1") + "$DAXCTL" disable-device "$daxdev" && "$DAXCTL" destroy-device "$daxdev" + daxdev=$(daxctl_get_dev "dax0.4") + "$DAXCTL" disable-device "$daxdev" && "$DAXCTL" destroy-device "$daxdev" + + daxdev=$(daxctl_get_dev "dax0.2") + "$DAXCTL" disable-device "$daxdev" + "$DAXCTL" reconfigure-device -s $(expr $size \* 2) "$daxdev" + + daxdev=$(daxctl_get_dev "dax0.3") + "$DAXCTL" disable-device "$daxdev" + "$DAXCTL" reconfigure-device -s $(expr $size \* 2) "$daxdev" + + fail_if_available + + daxdev=$(daxctl_get_dev "dax0.3") + "$DAXCTL" disable-device "$daxdev" && "$DAXCTL" destroy-device "$daxdev" + daxdev=$(daxctl_get_dev "dax0.2") + "$DAXCTL" disable-device "$daxdev" && "$DAXCTL" destroy-device "$daxdev" +} + +# Test 0: +# Sucessfully zero out the region device and allocate the whole space again. +daxctl_test0() +{ + clear_dev + test_pass +} + +# Test 1: +# Sucessfully creates and destroys a device with the whole available space +daxctl_test1() +{ + local daxdev + + daxdev=$("$DAXCTL" create-device -r 0 | jq -er '.[].chardev') + + test -n "$daxdev" + fail_if_available + + "$DAXCTL" disable-device "$daxdev" && "$DAXCTL" destroy-device "$daxdev" + + clear_dev + test_pass +} + +# Test 2: space at the middle and at the end +# Successfully pick space in the middle and space at the end, by +# having the region device reconfigured with some of the memory. +daxctl_test2() +{ + daxctl_test_multi "dax0.1" 1 + clear_dev + test_pass +} + +# Test 3: space at the beginning and at the end +# Successfully pick space in the beginning and space at the end, by +# having the region device emptied (so region beginning starts with dax0.1). +daxctl_test3() +{ + daxctl_test_multi "dax0.1" + clear_dev + test_pass +} + +# Test 4: space at the end +# Successfully reconfigure two devices in increasingly bigger allocations. +# The difference is that it reuses an existing resource, and only needs to +# pick at the end of the region +daxctl_test4() +{ + daxctl_test_multi_reconfig 8 + clear_dev + test_pass +} + +# Test 5: space adjust +# Successfully adjusts two resources to fill the whole region +# First adjusts towards the beginning of region, the second towards the end. +daxctl_test5() +{ + daxctl_test_adjust + clear_dev + test_pass +} + +find_testdev +rc=1 +setup_dev +daxctl_test0 +daxctl_test1 +daxctl_test2 +daxctl_test3 +daxctl_test4 +daxctl_test5 +reset_dev +exit 0
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> --- test/Makefile.am | 1 + test/daxctl-create.sh | 294 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 295 insertions(+) create mode 100755 test/daxctl-create.sh