* [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).