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

From: Joao Martins <joao.m.martins@oracle.com>
To: Dan Williams <dan.j.williams@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>
Cc: linux-nvdimm@lists.01.org
Subject: Re: [PATCH ndctl v2 10/10] daxctl/test: Add tests for dynamic dax regions
Date: Thu, 10 Dec 2020 15:01:30 +0000	[thread overview]
Message-ID: <42e0711f-b26b-65af-4f12-efb28b07a096@oracle.com> (raw)
In-Reply-To: <5cf76b3d-21db-daf9-dc1d-d38714a9a7c2@oracle.com>

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
> 
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org