linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH blktests 0/2] Add a test that triggers blk_mq_update_nr_hw_queues()
@ 2019-10-21 22:57 Bart Van Assche
  2019-10-21 22:57 ` [PATCH blktests 1/2] Move and rename uptime_s() Bart Van Assche
  2019-10-21 22:57 ` [PATCH blktests 2/2] Add a test that triggers blk_mq_update_nr_hw_queues() Bart Van Assche
  0 siblings, 2 replies; 11+ messages in thread
From: Bart Van Assche @ 2019-10-21 22:57 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: linux-block, Johannes Thumshirn, Bart Van Assche

Hi Omar,

This patch series includes the test that I used to verify my recently posted
blk_mq_update_nr_hw_queues() patches. Please consider these patches for
inclusion in the blktests repository.

Thanks,

Bart.

Bart Van Assche (2):
  Move and rename uptime_s()
  Add a test that triggers blk_mq_update_nr_hw_queues()

 common/multipath-over-rdma |  9 +-----
 common/rc                  |  9 ++++++
 tests/block/029            | 63 ++++++++++++++++++++++++++++++++++++++
 tests/block/029.out        |  1 +
 tests/nvmeof-mp/rc         |  2 +-
 tests/srp/014              |  2 +-
 tests/srp/rc               |  2 +-
 7 files changed, 77 insertions(+), 11 deletions(-)
 create mode 100755 tests/block/029
 create mode 100644 tests/block/029.out

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH blktests 1/2] Move and rename uptime_s()
  2019-10-21 22:57 [PATCH blktests 0/2] Add a test that triggers blk_mq_update_nr_hw_queues() Bart Van Assche
@ 2019-10-21 22:57 ` Bart Van Assche
  2019-10-22 17:55   ` Chaitanya Kulkarni
  2019-10-24 17:27   ` Omar Sandoval
  2019-10-21 22:57 ` [PATCH blktests 2/2] Add a test that triggers blk_mq_update_nr_hw_queues() Bart Van Assche
  1 sibling, 2 replies; 11+ messages in thread
From: Bart Van Assche @ 2019-10-21 22:57 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: linux-block, Johannes Thumshirn, Bart Van Assche

Make it easy to use the uptime_s() function from block tests.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 common/multipath-over-rdma | 9 +--------
 common/rc                  | 9 +++++++++
 tests/nvmeof-mp/rc         | 2 +-
 tests/srp/014              | 2 +-
 tests/srp/rc               | 2 +-
 5 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/common/multipath-over-rdma b/common/multipath-over-rdma
index 65ebb7b7f5f7..545a81e8c18e 100644
--- a/common/multipath-over-rdma
+++ b/common/multipath-over-rdma
@@ -129,19 +129,12 @@ held_by() {
 	done
 }
 
-# System uptime in seconds.
-uptime_s() {
-	local a b
-
-	echo "$(</proc/uptime)" | { read -r a b && echo "${a%%.*}"; }
-}
-
 # Sleep until either $1 seconds have elapsed or until the deadline $2 has been
 # reached. Return 1 if and only if the deadline has been met.
 sleep_until() {
 	local duration=$1 deadline=$2 u
 
-	u=$(uptime_s)
+	u=$(_uptime_s)
 	if [ $((u + duration)) -le "$deadline" ]; then
 		sleep "$duration"
 	else
diff --git a/common/rc b/common/rc
index 41aee3aaa735..c00f2fe1f463 100644
--- a/common/rc
+++ b/common/rc
@@ -246,3 +246,12 @@ _test_dev_is_partition() {
 _filter_xfs_io_error() {
 	sed -e 's/^\(.*\)64\(: .*$\)/\1\2/'
 }
+
+# System uptime in seconds.
+_uptime_s() {
+	local a b
+
+	echo "$(</proc/uptime)" | {
+		read -r a b && echo "$b" >/dev/null && echo "${a%%.*}";
+	}
+}
diff --git a/tests/nvmeof-mp/rc b/tests/nvmeof-mp/rc
index 2493fcee12de..278843a1270d 100755
--- a/tests/nvmeof-mp/rc
+++ b/tests/nvmeof-mp/rc
@@ -113,7 +113,7 @@ simulate_network_failure_loop() {
 
 	[ -e "$dev" ] || return $?
 	[ -n "$duration" ] || return $?
-	deadline=$(($(uptime_s) + duration))
+	deadline=$(($(_uptime_s) + duration))
 	while [ $rc = 0 ]; do
 		sleep_until 5 ${deadline} || break
 		for d in $(held_by "$dev"); do
diff --git a/tests/srp/014 b/tests/srp/014
index 8ecd8a439a82..7afde6284b83 100755
--- a/tests/srp/014
+++ b/tests/srp/014
@@ -69,7 +69,7 @@ sg_reset_loop() {
 	[ -e "$dev" ] || return $?
 	[ -n "$duration" ] || return $?
 	reset_type=(-d -b)
-	deadline=$(($(uptime_s) + duration))
+	deadline=$(($(_uptime_s) + duration))
 	while true; do
 		sleep_until 1 ${deadline} || break
 		cmd="sg_reset --no-esc ${reset_type[i++ % 2]} $dev"
diff --git a/tests/srp/rc b/tests/srp/rc
index 696d94e5fb97..a1bc09b496ec 100755
--- a/tests/srp/rc
+++ b/tests/srp/rc
@@ -247,7 +247,7 @@ simulate_network_failure_loop() {
 
 	[ -e "$dev" ] || return $?
 	[ -n "$duration" ] || return $?
-	deadline=$(($(uptime_s) + duration))
+	deadline=$(($(_uptime_s) + duration))
 	s=5
 	while [ $rc = 0 ]; do
 		sleep_until 5 ${deadline} || break
-- 
2.23.0.866.gb869b98d4c-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH blktests 2/2] Add a test that triggers blk_mq_update_nr_hw_queues()
  2019-10-21 22:57 [PATCH blktests 0/2] Add a test that triggers blk_mq_update_nr_hw_queues() Bart Van Assche
  2019-10-21 22:57 ` [PATCH blktests 1/2] Move and rename uptime_s() Bart Van Assche
@ 2019-10-21 22:57 ` Bart Van Assche
  2019-10-22 17:59   ` Chaitanya Kulkarni
  2019-10-24 17:42   ` Omar Sandoval
  1 sibling, 2 replies; 11+ messages in thread
From: Bart Van Assche @ 2019-10-21 22:57 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: linux-block, Johannes Thumshirn, Bart Van Assche

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
-- 
2.23.0.866.gb869b98d4c-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH blktests 1/2] Move and rename uptime_s()
  2019-10-21 22:57 ` [PATCH blktests 1/2] Move and rename uptime_s() Bart Van Assche
@ 2019-10-22 17:55   ` Chaitanya Kulkarni
  2019-10-24 17:27   ` Omar Sandoval
  1 sibling, 0 replies; 11+ messages in thread
From: Chaitanya Kulkarni @ 2019-10-22 17:55 UTC (permalink / raw)
  To: Bart Van Assche, Omar Sandoval; +Cc: linux-block, Johannes Thumshirn

Look good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
On 10/21/2019 03:57 PM, Bart Van Assche wrote:
> Make it easy to use the uptime_s() function from block tests.
>
> Signed-off-by: Bart Van Assche<bvanassche@acm.org>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH blktests 2/2] Add a test that triggers blk_mq_update_nr_hw_queues()
  2019-10-21 22:57 ` [PATCH blktests 2/2] Add a test that triggers blk_mq_update_nr_hw_queues() Bart Van Assche
@ 2019-10-22 17:59   ` Chaitanya Kulkarni
  2019-10-24 17:42   ` Omar Sandoval
  1 sibling, 0 replies; 11+ messages in thread
From: Chaitanya Kulkarni @ 2019-10-22 17:59 UTC (permalink / raw)
  To: Bart Van Assche, Omar Sandoval; +Cc: linux-block, Johannes Thumshirn

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
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH blktests 1/2] Move and rename uptime_s()
  2019-10-21 22:57 ` [PATCH blktests 1/2] Move and rename uptime_s() Bart Van Assche
  2019-10-22 17:55   ` Chaitanya Kulkarni
@ 2019-10-24 17:27   ` Omar Sandoval
  2019-10-24 17:41     ` Bart Van Assche
  1 sibling, 1 reply; 11+ messages in thread
From: Omar Sandoval @ 2019-10-24 17:27 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: Omar Sandoval, linux-block, Johannes Thumshirn

On Mon, Oct 21, 2019 at 03:57:18PM -0700, Bart Van Assche wrote:
> Make it easy to use the uptime_s() function from block tests.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  common/multipath-over-rdma | 9 +--------
>  common/rc                  | 9 +++++++++
>  tests/nvmeof-mp/rc         | 2 +-
>  tests/srp/014              | 2 +-
>  tests/srp/rc               | 2 +-
>  5 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/common/multipath-over-rdma b/common/multipath-over-rdma
> index 65ebb7b7f5f7..545a81e8c18e 100644
> --- a/common/multipath-over-rdma
> +++ b/common/multipath-over-rdma
> @@ -129,19 +129,12 @@ held_by() {
>  	done
>  }
>  
> -# System uptime in seconds.
> -uptime_s() {
> -	local a b
> -
> -	echo "$(</proc/uptime)" | { read -r a b && echo "${a%%.*}"; }
> -}
> -
>  # Sleep until either $1 seconds have elapsed or until the deadline $2 has been
>  # reached. Return 1 if and only if the deadline has been met.
>  sleep_until() {
>  	local duration=$1 deadline=$2 u
>  
> -	u=$(uptime_s)
> +	u=$(_uptime_s)
>  	if [ $((u + duration)) -le "$deadline" ]; then
>  		sleep "$duration"
>  	else
> diff --git a/common/rc b/common/rc
> index 41aee3aaa735..c00f2fe1f463 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -246,3 +246,12 @@ _test_dev_is_partition() {
>  _filter_xfs_io_error() {
>  	sed -e 's/^\(.*\)64\(: .*$\)/\1\2/'
>  }
> +
> +# System uptime in seconds.
> +_uptime_s() {
> +	local a b
> +
> +	echo "$(</proc/uptime)" | {

What's wrong with cat /proc/uptime? Or even better,

  { read ... } < /proc/uptime

> +		read -r a b && echo "$b" >/dev/null && echo "${a%%.*}";

What's the point of the echo "$b" here? Seems like this could all be
condensed to:

  { read -r s && echo "${s%%.*}" } < /proc/uptime

But that's more cryptic than it needs to be. Can we just do:

  awk '{ print int($1) }' /proc/uptime

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH blktests 1/2] Move and rename uptime_s()
  2019-10-24 17:27   ` Omar Sandoval
@ 2019-10-24 17:41     ` Bart Van Assche
  2019-10-24 17:44       ` Omar Sandoval
  0 siblings, 1 reply; 11+ messages in thread
From: Bart Van Assche @ 2019-10-24 17:41 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: Omar Sandoval, linux-block, Johannes Thumshirn

On 10/24/19 10:27 AM, Omar Sandoval wrote:
> On Mon, Oct 21, 2019 at 03:57:18PM -0700, Bart Van Assche wrote:
>> +# System uptime in seconds.
>> +_uptime_s() {
>> +	local a b
>> +
>> +	echo "$(</proc/uptime)" | {
> 
> What's wrong with cat /proc/uptime? Or even better,
> 
>    { read ... } < /proc/uptime

Hi Omar,

As you probably know 'cat' triggers a fork() system call but echo 
$(<...) not. This is a performance optimization. Input redirection would 
also work.

>> +		read -r a b && echo "$b" >/dev/null && echo "${a%%.*}";
> 
> What's the point of the echo "$b" here?

That echo "$b" statement suppresses a shellcheck warning about $b not 
being used.

> Seems like this could all be condensed to:
> 
>    { read -r s && echo "${s%%.*}" } < /proc/uptime
> 
> But that's more cryptic than it needs to be. Can we just do:
> 
>    awk '{ print int($1) }' /proc/uptime

That's a valid alternative, but an alternative that triggers a fork() 
system call. I don't have a strong opinion about which alternative to 
choose. Do you perhaps have a preference?

Thanks,

Bart.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH blktests 2/2] Add a test that triggers blk_mq_update_nr_hw_queues()
  2019-10-21 22:57 ` [PATCH blktests 2/2] Add a test that triggers blk_mq_update_nr_hw_queues() Bart Van Assche
  2019-10-22 17:59   ` Chaitanya Kulkarni
@ 2019-10-24 17:42   ` Omar Sandoval
  2019-10-24 17:55     ` Bart Van Assche
  1 sibling, 1 reply; 11+ messages in thread
From: Omar Sandoval @ 2019-10-24 17:42 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: Omar Sandoval, linux-block, Johannes Thumshirn

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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH blktests 1/2] Move and rename uptime_s()
  2019-10-24 17:41     ` Bart Van Assche
@ 2019-10-24 17:44       ` Omar Sandoval
  0 siblings, 0 replies; 11+ messages in thread
From: Omar Sandoval @ 2019-10-24 17:44 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: Omar Sandoval, linux-block, Johannes Thumshirn

On Thu, Oct 24, 2019 at 10:41:49AM -0700, Bart Van Assche wrote:
> On 10/24/19 10:27 AM, Omar Sandoval wrote:
> > On Mon, Oct 21, 2019 at 03:57:18PM -0700, Bart Van Assche wrote:
> > > +# System uptime in seconds.
> > > +_uptime_s() {
> > > +	local a b
> > > +
> > > +	echo "$(</proc/uptime)" | {
> > 
> > What's wrong with cat /proc/uptime? Or even better,
> > 
> >    { read ... } < /proc/uptime
> 
> Hi Omar,
> 
> As you probably know 'cat' triggers a fork() system call but echo $(<...)
> not. This is a performance optimization. Input redirection would also work.
> 
> > > +		read -r a b && echo "$b" >/dev/null && echo "${a%%.*}";
> > 
> > What's the point of the echo "$b" here?
> 
> That echo "$b" statement suppresses a shellcheck warning about $b not being
> used.
> 
> > Seems like this could all be condensed to:
> > 
> >    { read -r s && echo "${s%%.*}" } < /proc/uptime
> > 
> > But that's more cryptic than it needs to be. Can we just do:
> > 
> >    awk '{ print int($1) }' /proc/uptime
> 
> That's a valid alternative, but an alternative that triggers a fork() system
> call. I don't have a strong opinion about which alternative to choose. Do
> you perhaps have a preference?

The awk option is more readable, and we're not trying to win any
performance awards in blktests. Let's just go with that.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH blktests 2/2] Add a test that triggers blk_mq_update_nr_hw_queues()
  2019-10-24 17:42   ` Omar Sandoval
@ 2019-10-24 17:55     ` Bart Van Assche
  2019-10-24 17:58       ` Omar Sandoval
  0 siblings, 1 reply; 11+ messages in thread
From: Bart Van Assche @ 2019-10-24 17:55 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: Omar Sandoval, linux-block, Johannes Thumshirn

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH blktests 2/2] Add a test that triggers blk_mq_update_nr_hw_queues()
  2019-10-24 17:55     ` Bart Van Assche
@ 2019-10-24 17:58       ` Omar Sandoval
  0 siblings, 0 replies; 11+ messages in thread
From: Omar Sandoval @ 2019-10-24 17:58 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: Omar Sandoval, linux-block, Johannes Thumshirn

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!

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-10-24 17:58 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-21 22:57 [PATCH blktests 0/2] Add a test that triggers blk_mq_update_nr_hw_queues() Bart Van Assche
2019-10-21 22:57 ` [PATCH blktests 1/2] Move and rename uptime_s() Bart Van Assche
2019-10-22 17:55   ` Chaitanya Kulkarni
2019-10-24 17:27   ` Omar Sandoval
2019-10-24 17:41     ` Bart Van Assche
2019-10-24 17:44       ` Omar Sandoval
2019-10-21 22:57 ` [PATCH blktests 2/2] Add a test that triggers blk_mq_update_nr_hw_queues() Bart Van Assche
2019-10-22 17:59   ` Chaitanya Kulkarni
2019-10-24 17:42   ` Omar Sandoval
2019-10-24 17:55     ` Bart Van Assche
2019-10-24 17:58       ` Omar Sandoval

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).