diff mbox series

[blktests,2/2] Add a test that triggers blk_mq_update_nr_hw_queues()

Message ID 20191021225719.211651-3-bvanassche@acm.org (mailing list archive)
State New, archived
Headers show
Series Add a test that triggers blk_mq_update_nr_hw_queues() | expand

Commit Message

Bart Van Assche Oct. 21, 2019, 10:57 p.m. UTC 
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 tests/block/029     | 63 +++++++++++++++++++++++++++++++++++++++++++++
 tests/block/029.out |  1 +
 2 files changed, 64 insertions(+)
 create mode 100755 tests/block/029
 create mode 100644 tests/block/029.out

Comments

Chaitanya Kulkarni Oct. 22, 2019, 5:59 p.m. UTC | #1 
Look good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 10/21/2019 03:57 PM, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   tests/block/029     | 63 +++++++++++++++++++++++++++++++++++++++++++++
>   tests/block/029.out |  1 +
>   2 files changed, 64 insertions(+)
>   create mode 100755 tests/block/029
>   create mode 100644 tests/block/029.out
>
> diff --git a/tests/block/029 b/tests/block/029
> new file mode 100755
> index 000000000000..1999168603c1
> --- /dev/null
> +++ b/tests/block/029
> @@ -0,0 +1,63 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2019 Google Inc.
> +#
> +# Trigger blk_mq_update_nr_hw_queues().
> +
> +. tests/block/rc
> +. common/null_blk
> +
> +DESCRIPTION="trigger blk_mq_update_nr_hw_queues()"
> +QUICK=1
> +
> +requires() {
> +	_have_null_blk
> +}
> +
> +# Configure one null_blk instance.
> +configure_null_blk() {
> +	(
> +		cd /sys/kernel/config/nullb || return $?
> +		(
> +			mkdir -p nullb0 &&
> +				cd nullb0 &&
> +				echo 0 > completion_nsec &&
> +				echo 512 > blocksize &&
> +				echo 16 > size &&
> +				echo 1 > memory_backed &&
> +				echo 1 > power
> +		)
> +	) &&
> +	ls -l /dev/nullb* &>>"$FULL"
> +}
> +
> +modify_nr_hw_queues() {
> +	local deadline num_cpus
> +
> +	deadline=$(($(_uptime_s) + TIMEOUT))
> +	num_cpus=$(find /sys/devices/system/cpu -maxdepth 1 -name 'cpu[0-9]*' |
> +			   wc -l)
> +	while [ "$(_uptime_s)" -lt "$deadline" ]; do
> +		sleep .1
> +		echo 1 > /sys/kernel/config/nullb/nullb0/submit_queues
> +		sleep .1
> +		echo "$num_cpus" > /sys/kernel/config/nullb/nullb0/submit_queues
> +	done
> +}
> +
> +test() {
> +	: "${TIMEOUT:=30}"
> +	_init_null_blk nr_devices=0 queue_mode=2 &&
> +	configure_null_blk
> +	modify_nr_hw_queues &
> +	fio --rw=randwrite --bs=4K --loops=$((10**6)) \
> +		--iodepth=64 --group_reporting --sync=1 --direct=1 \
> +		--ioengine=libaio --filename="/dev/nullb0" \
> +		--runtime="${TIMEOUT}" --name=nullb0 \
> +		--output="${RESULTS_DIR}/block/fio-output-029.txt" \
> +		>>"$FULL"
> +	wait
> +	rmdir /sys/kernel/config/nullb/nullb0
> +	_exit_null_blk
> +	echo Passed
> +}
> diff --git a/tests/block/029.out b/tests/block/029.out
> new file mode 100644
> index 000000000000..863339fb8ced
> --- /dev/null
> +++ b/tests/block/029.out
> @@ -0,0 +1 @@
> +Passed
>
Omar Sandoval Oct. 24, 2019, 5:42 p.m. UTC | #2 
On Mon, Oct 21, 2019 at 03:57:19PM -0700, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  tests/block/029     | 63 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/block/029.out |  1 +
>  2 files changed, 64 insertions(+)
>  create mode 100755 tests/block/029
>  create mode 100644 tests/block/029.out
> 
> diff --git a/tests/block/029 b/tests/block/029
> new file mode 100755
> index 000000000000..1999168603c1
> --- /dev/null
> +++ b/tests/block/029
> @@ -0,0 +1,63 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2019 Google Inc.
> +#
> +# Trigger blk_mq_update_nr_hw_queues().
> +
> +. tests/block/rc
> +. common/null_blk
> +
> +DESCRIPTION="trigger blk_mq_update_nr_hw_queues()"
> +QUICK=1
> +
> +requires() {
> +	_have_null_blk
> +}
> +
> +# Configure one null_blk instance.
> +configure_null_blk() {
> +	(
> +		cd /sys/kernel/config/nullb || return $?
> +		(
> +			mkdir -p nullb0 &&
> +				cd nullb0 &&
> +				echo 0 > completion_nsec &&
> +				echo 512 > blocksize &&
> +				echo 16 > size &&
> +				echo 1 > memory_backed &&
> +				echo 1 > power
> +		)
> +	) &&
> +	ls -l /dev/nullb* &>>"$FULL"

What's the point of these nested subshells? Can't this just be:

configure_null_blk() {
	cd /sys/kernel/config/nullb &&
	mkdir -p nullb0 &&
	cd nullb0 &&
	echo 0 > completion_nsec &&
	echo 512 > blocksize &&
	echo 16 > size &&
	echo 1 > memory_backed &&
	echo 1 > power &&
	ls -l /dev/nullb* &>>"$FULL"
}

> +}
> +
> +modify_nr_hw_queues() {
> +	local deadline num_cpus
> +
> +	deadline=$(($(_uptime_s) + TIMEOUT))
> +	num_cpus=$(find /sys/devices/system/cpu -maxdepth 1 -name 'cpu[0-9]*' |
> +			   wc -l)

Please just use nproc. Or even better, can you just read the original
value of /sys/kernel/config/nullb/nullb0/submit_queues? Or does that
start at 1?

> +	while [ "$(_uptime_s)" -lt "$deadline" ]; do
> +		sleep .1
> +		echo 1 > /sys/kernel/config/nullb/nullb0/submit_queues
> +		sleep .1
> +		echo "$num_cpus" > /sys/kernel/config/nullb/nullb0/submit_queues
> +	done
> +}
> +
> +test() {
> +	: "${TIMEOUT:=30}"
> +	_init_null_blk nr_devices=0 queue_mode=2 &&
> +	configure_null_blk
> +	modify_nr_hw_queues &
> +	fio --rw=randwrite --bs=4K --loops=$((10**6)) \
> +		--iodepth=64 --group_reporting --sync=1 --direct=1 \
> +		--ioengine=libaio --filename="/dev/nullb0" \
> +		--runtime="${TIMEOUT}" --name=nullb0 \
> +		--output="${RESULTS_DIR}/block/fio-output-029.txt" \
> +		>>"$FULL"
> +	wait
> +	rmdir /sys/kernel/config/nullb/nullb0
> +	_exit_null_blk
> +	echo Passed
> +}
> diff --git a/tests/block/029.out b/tests/block/029.out
> new file mode 100644
> index 000000000000..863339fb8ced
> --- /dev/null
> +++ b/tests/block/029.out
> @@ -0,0 +1 @@
> +Passed
> -- 
> 2.23.0.866.gb869b98d4c-goog
>
Bart Van Assche Oct. 24, 2019, 5:55 p.m. UTC | #3 
On 10/24/19 10:42 AM, Omar Sandoval wrote:
> On Mon, Oct 21, 2019 at 03:57:19PM -0700, Bart Van Assche wrote:
>> +# Configure one null_blk instance.
>> +configure_null_blk() {
>> +	(
>> +		cd /sys/kernel/config/nullb || return $?
>> +		(
>> +			mkdir -p nullb0 &&
>> +				cd nullb0 &&
>> +				echo 0 > completion_nsec &&
>> +				echo 512 > blocksize &&
>> +				echo 16 > size &&
>> +				echo 1 > memory_backed &&
>> +				echo 1 > power
>> +		)
>> +	) &&
>> +	ls -l /dev/nullb* &>>"$FULL"
> 
> What's the point of these nested subshells? Can't this just be:
> 
> configure_null_blk() {
> 	cd /sys/kernel/config/nullb &&
> 	mkdir -p nullb0 &&
> 	cd nullb0 &&
> 	echo 0 > completion_nsec &&
> 	echo 512 > blocksize &&
> 	echo 16 > size &&
> 	echo 1 > memory_backed &&
> 	echo 1 > power &&
> 	ls -l /dev/nullb* &>>"$FULL"
> }

The above code is not equivalent to the original code because it does 
not restore the original working directory.

When using 'cd' inside a subshell, the working directory that was 
effective before the 'cd' command is restored automatically when exiting 
from the subshell. I prefer using 'cd' in a subshell instead of using 
pushd / popd because with the former approach the old working directory 
is restored no matter how the exit from the subshell happens.

>> +modify_nr_hw_queues() {
>> +	local deadline num_cpus
>> +
>> +	deadline=$(($(_uptime_s) + TIMEOUT))
>> +	num_cpus=$(find /sys/devices/system/cpu -maxdepth 1 -name 'cpu[0-9]*' |
>> +			   wc -l)
> 
> Please just use nproc. Or even better, can you just read the original
> value of /sys/kernel/config/nullb/nullb0/submit_queues? Or does that
> start at 1?

I will have a closer look and see which alternative works best.

Bart.
Omar Sandoval Oct. 24, 2019, 5:58 p.m. UTC | #4 
On Thu, Oct 24, 2019 at 10:55:06AM -0700, Bart Van Assche wrote:
> On 10/24/19 10:42 AM, Omar Sandoval wrote:
> > On Mon, Oct 21, 2019 at 03:57:19PM -0700, Bart Van Assche wrote:
> > > +# Configure one null_blk instance.
> > > +configure_null_blk() {
> > > +	(
> > > +		cd /sys/kernel/config/nullb || return $?
> > > +		(
> > > +			mkdir -p nullb0 &&
> > > +				cd nullb0 &&
> > > +				echo 0 > completion_nsec &&
> > > +				echo 512 > blocksize &&
> > > +				echo 16 > size &&
> > > +				echo 1 > memory_backed &&
> > > +				echo 1 > power
> > > +		)
> > > +	) &&
> > > +	ls -l /dev/nullb* &>>"$FULL"
> > 
> > What's the point of these nested subshells? Can't this just be:
> > 
> > configure_null_blk() {
> > 	cd /sys/kernel/config/nullb &&
> > 	mkdir -p nullb0 &&
> > 	cd nullb0 &&
> > 	echo 0 > completion_nsec &&
> > 	echo 512 > blocksize &&
> > 	echo 16 > size &&
> > 	echo 1 > memory_backed &&
> > 	echo 1 > power &&
> > 	ls -l /dev/nullb* &>>"$FULL"
> > }
> 
> The above code is not equivalent to the original code because it does not
> restore the original working directory.
> 
> When using 'cd' inside a subshell, the working directory that was effective
> before the 'cd' command is restored automatically when exiting from the
> subshell. I prefer using 'cd' in a subshell instead of using pushd / popd
> because with the former approach the old working directory is restored no
> matter how the exit from the subshell happens.

Ah, right, I didn't pay enough attention to the cd. In that case, I'd
prefer not doing the cd at all. Something like:

configure_null_blk() {
	local nullb0="/sys/kernel/config/nullb/nullb0"
	mkdir -p "$nullb0" &&
	echo 0 > "$nullb0/completion_nsec" &&
	echo 512 > "$nullb0/blocksize" &&
	echo 16 > "$nullb0/size" &&
	echo 1 > "$nullb0/memory_backed" &&
	echo 1 > "$nullb0/power" &&
	ls -l /dev/nullb* &>>"$FULL"
}

> > > +modify_nr_hw_queues() {
> > > +	local deadline num_cpus
> > > +
> > > +	deadline=$(($(_uptime_s) + TIMEOUT))
> > > +	num_cpus=$(find /sys/devices/system/cpu -maxdepth 1 -name 'cpu[0-9]*' |
> > > +			   wc -l)
> > 
> > Please just use nproc. Or even better, can you just read the original
> > value of /sys/kernel/config/nullb/nullb0/submit_queues? Or does that
> > start at 1?
> 
> I will have a closer look and see which alternative works best.

Thanks!
diff mbox series

Patch

diff --git a/tests/block/029 b/tests/block/029
new file mode 100755
index 000000000000..1999168603c1
--- /dev/null
+++ b/tests/block/029
@@ -0,0 +1,63 @@ 
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2019 Google Inc.
+#
+# Trigger blk_mq_update_nr_hw_queues().
+
+. tests/block/rc
+. common/null_blk
+
+DESCRIPTION="trigger blk_mq_update_nr_hw_queues()"
+QUICK=1
+
+requires() {
+	_have_null_blk
+}
+
+# Configure one null_blk instance.
+configure_null_blk() {
+	(
+		cd /sys/kernel/config/nullb || return $?
+		(
+			mkdir -p nullb0 &&
+				cd nullb0 &&
+				echo 0 > completion_nsec &&
+				echo 512 > blocksize &&
+				echo 16 > size &&
+				echo 1 > memory_backed &&
+				echo 1 > power
+		)
+	) &&
+	ls -l /dev/nullb* &>>"$FULL"
+}
+
+modify_nr_hw_queues() {
+	local deadline num_cpus
+
+	deadline=$(($(_uptime_s) + TIMEOUT))
+	num_cpus=$(find /sys/devices/system/cpu -maxdepth 1 -name 'cpu[0-9]*' |
+			   wc -l)
+	while [ "$(_uptime_s)" -lt "$deadline" ]; do
+		sleep .1
+		echo 1 > /sys/kernel/config/nullb/nullb0/submit_queues
+		sleep .1
+		echo "$num_cpus" > /sys/kernel/config/nullb/nullb0/submit_queues
+	done
+}
+
+test() {
+	: "${TIMEOUT:=30}"
+	_init_null_blk nr_devices=0 queue_mode=2 &&
+	configure_null_blk
+	modify_nr_hw_queues &
+	fio --rw=randwrite --bs=4K --loops=$((10**6)) \
+		--iodepth=64 --group_reporting --sync=1 --direct=1 \
+		--ioengine=libaio --filename="/dev/nullb0" \
+		--runtime="${TIMEOUT}" --name=nullb0 \
+		--output="${RESULTS_DIR}/block/fio-output-029.txt" \
+		>>"$FULL"
+	wait
+	rmdir /sys/kernel/config/nullb/nullb0
+	_exit_null_blk
+	echo Passed
+}
diff --git a/tests/block/029.out b/tests/block/029.out
new file mode 100644
index 000000000000..863339fb8ced
--- /dev/null
+++ b/tests/block/029.out
@@ -0,0 +1 @@ 
+Passed