[Qemu-devel] [kvm-unit-tests PATCH v3 0/2] run_tests: support concurrent test execution

[Qemu-devel] [kvm-unit-tests PATCH v3 0/2] run_tests: support concurrent test execution

[Peter Xu]

v3:
- better handling for ctrl-c during run_tests.sh [Radim]

v2:
- patch 1: do per-test logging in all cases
- patch 2: throw away task.bash, instead, take Radim's suggestion to
  use jobs

run_tests.sh is getting slower. Maybe it's time to let it run faster.
An obvious issue is that, we were running the tests sequentially in
the past.

This series provides another new "-j" parameter. "-j 8" means we run
the tests on 8 task queues. That'll fasten the script a lot. A very
quick test of mine shows 3x speed boost with 8 task queues.

Please review, thanks.

Peter Xu (2):
  run_tests: put logs into per-test file
  run_tests: allow run tests in parallel

 Makefile               |  4 ++--
 run_tests.sh           | 26 ++++++++++++++++++--------
 scripts/functions.bash | 26 ++++++++++++++++++++++++--
 scripts/global.bash    | 13 +++++++++++++
 4 files changed, 57 insertions(+), 12 deletions(-)
 create mode 100644 scripts/global.bash

-- 
2.7.4

[Qemu-devel] [kvm-unit-tests PATCH v3 1/2] run_tests: put logs into per-test file

[Peter Xu]

We were using test.log before to keep all the test logs. This patch
creates one log file per test case under logs/ directory with name
"TESTNAME.log".

A new file global.bash is added to store global informations.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 Makefile               |  4 ++--
 run_tests.sh           | 14 ++++++++------
 scripts/functions.bash | 11 +++++++++--
 scripts/global.bash    |  2 ++
 4 files changed, 21 insertions(+), 10 deletions(-)
 create mode 100644 scripts/global.bash

diff --git a/Makefile b/Makefile
index a32333b..f632c6c 100644
--- a/Makefile
+++ b/Makefile
@@ -94,9 +94,9 @@ libfdt_clean:
 	$(LIBFDT_objdir)/.*.d
 
 distclean: clean libfdt_clean
-	$(RM) lib/asm config.mak $(TEST_DIR)-run test.log msr.out cscope.* \
+	$(RM) lib/asm config.mak $(TEST_DIR)-run msr.out cscope.* \
 	      build-head
-	$(RM) -r tests
+	$(RM) -r tests logs
 
 cscope: cscope_dirs = lib lib/libfdt lib/linux $(TEST_DIR) $(ARCH_LIBDIRS) lib/asm-generic
 cscope:
diff --git a/run_tests.sh b/run_tests.sh
index 254129d..e1bb3a6 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -7,6 +7,7 @@ if [ ! -f config.mak ]; then
     exit 1
 fi
 source config.mak
+source scripts/global.bash
 source scripts/functions.bash
 
 function usage()
@@ -46,17 +47,18 @@ while getopts "g:hv" opt; do
     esac
 done
 
-RUNTIME_log_stderr () { cat >> test.log; }
+# RUNTIME_log_file will be configured later
+RUNTIME_log_stderr () { cat >> $RUNTIME_log_file; }
 RUNTIME_log_stdout () {
     if [ "$PRETTY_PRINT_STACKS" = "yes" ]; then
-        ./scripts/pretty_print_stacks.py $1 >> test.log
+        ./scripts/pretty_print_stacks.py $1 >> $RUNTIME_log_file
     else
-        cat >> test.log
+        cat >> $RUNTIME_log_file
     fi
 }
 
-
 config=$TEST_DIR/unittests.cfg
-rm -f test.log
-printf "BUILD_HEAD=$(cat build-head)\n\n" > test.log
+rm -rf $ut_log_dir
+mkdir $ut_log_dir
+printf "BUILD_HEAD=$(cat build-head)\n\n" > $ut_log_summary
 for_each_unittest $config run
diff --git a/scripts/functions.bash b/scripts/functions.bash
index ee9143c..d1d2e1c 100644
--- a/scripts/functions.bash
+++ b/scripts/functions.bash
@@ -1,3 +1,10 @@
+function run_task()
+{
+	local testname="$2"
+
+	RUNTIME_log_file="${ut_log_dir}/${testname}.log"
+	"$@"
+}
 
 function for_each_unittest()
 {
@@ -17,7 +24,7 @@ function for_each_unittest()
 
 	while read -u $fd line; do
 		if [[ "$line" =~ ^\[(.*)\]$ ]]; then
-			"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+			run_task "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
 			testname=${BASH_REMATCH[1]}
 			smp=1
 			kernel=""
@@ -45,6 +52,6 @@ function for_each_unittest()
 			timeout=${BASH_REMATCH[1]}
 		fi
 	done
-	"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+	run_task "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
 	exec {fd}<&-
 }
diff --git a/scripts/global.bash b/scripts/global.bash
new file mode 100644
index 0000000..77b0b29
--- /dev/null
+++ b/scripts/global.bash
@@ -0,0 +1,2 @@
+: ${ut_log_dir:=logs}
+: ${ut_log_summary:=${ut_log_dir}/SUMMARY}
-- 
2.7.4

Re: [Qemu-devel] [kvm-unit-tests PATCH v3 1/2] run_tests: put logs into per-test file

[Andrew Jones]

On Fri, Jan 06, 2017 at 11:33:00AM +0800, Peter Xu wrote:
> We were using test.log before to keep all the test logs. This patch
> creates one log file per test case under logs/ directory with name
> "TESTNAME.log".
> 
> A new file global.bash is added to store global informations.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  Makefile               |  4 ++--
>  run_tests.sh           | 14 ++++++++------
>  scripts/functions.bash | 11 +++++++++--
>  scripts/global.bash    |  2 ++
>  4 files changed, 21 insertions(+), 10 deletions(-)
>  create mode 100644 scripts/global.bash
> 
> diff --git a/Makefile b/Makefile
> index a32333b..f632c6c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -94,9 +94,9 @@ libfdt_clean:
>  	$(LIBFDT_objdir)/.*.d
>  
>  distclean: clean libfdt_clean
> -	$(RM) lib/asm config.mak $(TEST_DIR)-run test.log msr.out cscope.* \
> +	$(RM) lib/asm config.mak $(TEST_DIR)-run msr.out cscope.* \
>  	      build-head
> -	$(RM) -r tests
> +	$(RM) -r tests logs

We need .gitignore changes for this.

>  
>  cscope: cscope_dirs = lib lib/libfdt lib/linux $(TEST_DIR) $(ARCH_LIBDIRS) lib/asm-generic
>  cscope:
> diff --git a/run_tests.sh b/run_tests.sh
> index 254129d..e1bb3a6 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -7,6 +7,7 @@ if [ ! -f config.mak ]; then
>      exit 1
>  fi
>  source config.mak
> +source scripts/global.bash
>  source scripts/functions.bash

Rather than add a new file, can't we just rename functions.bash to
something less function specific (common.bash?) and then add the
globals to that?

>  
>  function usage()
> @@ -46,17 +47,18 @@ while getopts "g:hv" opt; do
>      esac
>  done
>  
> -RUNTIME_log_stderr () { cat >> test.log; }
> +# RUNTIME_log_file will be configured later
> +RUNTIME_log_stderr () { cat >> $RUNTIME_log_file; }
>  RUNTIME_log_stdout () {
>      if [ "$PRETTY_PRINT_STACKS" = "yes" ]; then
> -        ./scripts/pretty_print_stacks.py $1 >> test.log
> +        ./scripts/pretty_print_stacks.py $1 >> $RUNTIME_log_file
>      else
> -        cat >> test.log
> +        cat >> $RUNTIME_log_file
>      fi
>  }
>  
> -
>  config=$TEST_DIR/unittests.cfg
> -rm -f test.log
> -printf "BUILD_HEAD=$(cat build-head)\n\n" > test.log
> +rm -rf $ut_log_dir

Instead of the 'rm', let's do
 'mv $ut_log_dir $ut_log_dir.old || { echo [...]; exit 2; }'
as I never liked that test.log was silently overwritten.

> +mkdir $ut_log_dir
> +printf "BUILD_HEAD=$(cat build-head)\n\n" > $ut_log_summary

I'm not sure we need the ut_ prefix on these variables. If we
do think we need to start prefixing variables, then I'd prefer
not to abbreviate, i.e. 'unittest_'

>  for_each_unittest $config run
> diff --git a/scripts/functions.bash b/scripts/functions.bash
> index ee9143c..d1d2e1c 100644
> --- a/scripts/functions.bash
> +++ b/scripts/functions.bash
> @@ -1,3 +1,10 @@
> +function run_task()
> +{
> +	local testname="$2"
> +
> +	RUNTIME_log_file="${ut_log_dir}/${testname}.log"
> +	"$@"
> +}
>  
>  function for_each_unittest()
>  {
> @@ -17,7 +24,7 @@ function for_each_unittest()
>  
>  	while read -u $fd line; do
>  		if [[ "$line" =~ ^\[(.*)\]$ ]]; then
> -			"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> +			run_task "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>  			testname=${BASH_REMATCH[1]}
>  			smp=1
>  			kernel=""
> @@ -45,6 +52,6 @@ function for_each_unittest()
>  			timeout=${BASH_REMATCH[1]}
>  		fi
>  	done
> -	"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> +	run_task "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>  	exec {fd}<&-
>  }
> diff --git a/scripts/global.bash b/scripts/global.bash
> new file mode 100644
> index 0000000..77b0b29
> --- /dev/null
> +++ b/scripts/global.bash
> @@ -0,0 +1,2 @@
> +: ${ut_log_dir:=logs}
> +: ${ut_log_summary:=${ut_log_dir}/SUMMARY}

Do we even need these variables? When/why would someone override these
defaults?

Thanks,
drew

Re: [Qemu-devel] [kvm-unit-tests PATCH v3 1/2] run_tests: put logs into per-test file

[Peter Xu]

On Fri, Jan 06, 2017 at 02:51:58PM +0100, Andrew Jones wrote:
> On Fri, Jan 06, 2017 at 11:33:00AM +0800, Peter Xu wrote:
> > We were using test.log before to keep all the test logs. This patch
> > creates one log file per test case under logs/ directory with name
> > "TESTNAME.log".
> > 
> > A new file global.bash is added to store global informations.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  Makefile               |  4 ++--
> >  run_tests.sh           | 14 ++++++++------
> >  scripts/functions.bash | 11 +++++++++--
> >  scripts/global.bash    |  2 ++
> >  4 files changed, 21 insertions(+), 10 deletions(-)
> >  create mode 100644 scripts/global.bash
> > 
> > diff --git a/Makefile b/Makefile
> > index a32333b..f632c6c 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -94,9 +94,9 @@ libfdt_clean:
> >  	$(LIBFDT_objdir)/.*.d
> >  
> >  distclean: clean libfdt_clean
> > -	$(RM) lib/asm config.mak $(TEST_DIR)-run test.log msr.out cscope.* \
> > +	$(RM) lib/asm config.mak $(TEST_DIR)-run msr.out cscope.* \
> >  	      build-head
> > -	$(RM) -r tests
> > +	$(RM) -r tests logs
> 
> We need .gitignore changes for this.

Yep.

> 
> >  
> >  cscope: cscope_dirs = lib lib/libfdt lib/linux $(TEST_DIR) $(ARCH_LIBDIRS) lib/asm-generic
> >  cscope:
> > diff --git a/run_tests.sh b/run_tests.sh
> > index 254129d..e1bb3a6 100755
> > --- a/run_tests.sh
> > +++ b/run_tests.sh
> > @@ -7,6 +7,7 @@ if [ ! -f config.mak ]; then
> >      exit 1
> >  fi
> >  source config.mak
> > +source scripts/global.bash
> >  source scripts/functions.bash
> 
> Rather than add a new file, can't we just rename functions.bash to
> something less function specific (common.bash?) and then add the
> globals to that?

Sure, common.bash is a good one, will use that.

> 
> >  
> >  function usage()
> > @@ -46,17 +47,18 @@ while getopts "g:hv" opt; do
> >      esac
> >  done
> >  
> > -RUNTIME_log_stderr () { cat >> test.log; }
> > +# RUNTIME_log_file will be configured later
> > +RUNTIME_log_stderr () { cat >> $RUNTIME_log_file; }
> >  RUNTIME_log_stdout () {
> >      if [ "$PRETTY_PRINT_STACKS" = "yes" ]; then
> > -        ./scripts/pretty_print_stacks.py $1 >> test.log
> > +        ./scripts/pretty_print_stacks.py $1 >> $RUNTIME_log_file
> >      else
> > -        cat >> test.log
> > +        cat >> $RUNTIME_log_file
> >      fi
> >  }
> >  
> > -
> >  config=$TEST_DIR/unittests.cfg
> > -rm -f test.log
> > -printf "BUILD_HEAD=$(cat build-head)\n\n" > test.log
> > +rm -rf $ut_log_dir
> 
> Instead of the 'rm', let's do
>  'mv $ut_log_dir $ut_log_dir.old || { echo [...]; exit 2; }'
> as I never liked that test.log was silently overwritten.

"Move" is indeed a much better way than "remove", especially "rm -rf"
(I think I should avoid using "rm -rf" in the future scripts as
well...). Will fix it.

> 
> > +mkdir $ut_log_dir
> > +printf "BUILD_HEAD=$(cat build-head)\n\n" > $ut_log_summary
> 
> I'm not sure we need the ut_ prefix on these variables. If we
> do think we need to start prefixing variables, then I'd prefer
> not to abbreviate, i.e. 'unittest_'

Since I would prefer a prefix, I'll use "unittest_" then.

[...]

> > diff --git a/scripts/global.bash b/scripts/global.bash
> > new file mode 100644
> > index 0000000..77b0b29
> > --- /dev/null
> > +++ b/scripts/global.bash
> > @@ -0,0 +1,2 @@
> > +: ${ut_log_dir:=logs}
> > +: ${ut_log_summary:=${ut_log_dir}/SUMMARY}
> 
> Do we even need these variables? When/why would someone override these
> defaults?

I use variables when I write constant more than once. This suites the
case for log_dir. I can remove the latter log_summary, but I'll still
prefer keep the log_dir if you don't mind.

Thanks!

-- peterx

[Qemu-devel] [kvm-unit-tests PATCH v3 2/2] run_tests: allow run tests in parallel

[Peter Xu]

run_task.sh is getting slow. This patch is trying to make it faster by
running the tests concurrently.

We provide a new parameter "-j" for the run_tests.sh, which can be used
to specify how many run queues we want for the tests. Default queue
length is 1, which is the old behavior.

Quick test on my laptop (4 cores, 2 threads each) shows 3x speed boost:

   |-----------------+-----------|
   | command         | time used |
   |-----------------+-----------|
   | run_test.sh     | 75s       |
   | run_test.sh -j8 | 27s       |
   |-----------------+-----------|

Suggested-by: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 run_tests.sh           | 12 ++++++++++--
 scripts/functions.bash | 17 ++++++++++++++++-
 scripts/global.bash    | 11 +++++++++++
 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/run_tests.sh b/run_tests.sh
index e1bb3a6..a4fc895 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -14,10 +14,11 @@ function usage()
 {
 cat <<EOF
 
-Usage: $0 [-g group] [-h] [-v]
+Usage: $0 [-g group] [-h] [-v] [-j N]
 
     -g: Only execute tests in the given group
     -h: Output this help text
+    -j: Execute tests in parallel
     -v: Enables verbose mode
 
 Set the environment variable QEMU=/path/to/qemu-system-ARCH to
@@ -29,7 +30,7 @@ EOF
 RUNTIME_arch_run="./$TEST_DIR/run"
 source scripts/runtime.bash
 
-while getopts "g:hv" opt; do
+while getopts "g:hj:v" opt; do
     case $opt in
         g)
             only_group=$OPTARG
@@ -38,6 +39,13 @@ while getopts "g:hv" opt; do
             usage
             exit
             ;;
+        j)
+            ut_run_queues=$OPTARG
+            if ! is_number "$ut_run_queues"; then
+                echo "Invalid -j option: $ut_run_queues"
+                exit 1
+            fi
+            ;;
         v)
             verbose="yes"
             ;;
diff --git a/scripts/functions.bash b/scripts/functions.bash
index d1d2e1c..c6281f4 100644
--- a/scripts/functions.bash
+++ b/scripts/functions.bash
@@ -1,9 +1,18 @@
+source scripts/global.bash
+
 function run_task()
 {
 	local testname="$2"
 
+	while [[ "$(jobs | wc -l)" == $ut_run_queues ]]; do
+		# wait for any background test to finish
+		wait -n
+	done
+
 	RUNTIME_log_file="${ut_log_dir}/${testname}.log"
-	"$@"
+
+	# start the testcase in the background
+	"$@" &
 }
 
 function for_each_unittest()
@@ -20,6 +29,8 @@ function for_each_unittest()
 	local accel
 	local timeout
 
+	trap "wait; exit 130" SIGINT
+
 	exec {fd}<"$unittests"
 
 	while read -u $fd line; do
@@ -53,5 +64,9 @@ function for_each_unittest()
 		fi
 	done
 	run_task "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+
+	# wait all task finish
+	wait
+
 	exec {fd}<&-
 }
diff --git a/scripts/global.bash b/scripts/global.bash
index 77b0b29..52095bd 100644
--- a/scripts/global.bash
+++ b/scripts/global.bash
@@ -1,2 +1,13 @@
 : ${ut_log_dir:=logs}
 : ${ut_log_summary:=${ut_log_dir}/SUMMARY}
+: ${ut_run_queues:=1}
+
+function ut_in_parallel()
+{
+    [[ $ut_run_queues != 1 ]]
+}
+
+function is_number()
+{
+    [[ "$1" =~ ^[0-9]+$ ]]
+}
-- 
2.7.4

Re: [Qemu-devel] [kvm-unit-tests PATCH v3 2/2] run_tests: allow run tests in parallel

[Andrew Jones]

On Fri, Jan 06, 2017 at 11:33:01AM +0800, Peter Xu wrote:
> run_task.sh is getting slow. This patch is trying to make it faster by
> running the tests concurrently.
> 
> We provide a new parameter "-j" for the run_tests.sh, which can be used
> to specify how many run queues we want for the tests. Default queue
> length is 1, which is the old behavior.
> 
> Quick test on my laptop (4 cores, 2 threads each) shows 3x speed boost:
> 
>    |-----------------+-----------|
>    | command         | time used |
>    |-----------------+-----------|
>    | run_test.sh     | 75s       |
>    | run_test.sh -j8 | 27s       |
>    |-----------------+-----------|
> 
> Suggested-by: Radim Krčmář <rkrcmar@redhat.com>

Radim suggested how to implement this, but not the idea of implementing
it. The suggested-by tag is for ideas, not the code, and the idea (which
is a good one) was yours, not Radim's. So the above tag should be removed.
It's hard to credit Radim for his help here. Adding a signed-off-by to
indicate co-authorship is probably the best you can do. Of course he's
the maintainer and will add that when he merges anyway though...

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  run_tests.sh           | 12 ++++++++++--
>  scripts/functions.bash | 17 ++++++++++++++++-
>  scripts/global.bash    | 11 +++++++++++
>  3 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/run_tests.sh b/run_tests.sh
> index e1bb3a6..a4fc895 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -14,10 +14,11 @@ function usage()
>  {
>  cat <<EOF
>  
> -Usage: $0 [-g group] [-h] [-v]
> +Usage: $0 [-g group] [-h] [-v] [-j N]

s/N/num_run_queues/

>  
>      -g: Only execute tests in the given group
>      -h: Output this help text
> +    -j: Execute tests in parallel
>      -v: Enables verbose mode
>  
>  Set the environment variable QEMU=/path/to/qemu-system-ARCH to
> @@ -29,7 +30,7 @@ EOF
>  RUNTIME_arch_run="./$TEST_DIR/run"
>  source scripts/runtime.bash
>  
> -while getopts "g:hv" opt; do
> +while getopts "g:hj:v" opt; do
>      case $opt in
>          g)
>              only_group=$OPTARG
> @@ -38,6 +39,13 @@ while getopts "g:hv" opt; do
>              usage
>              exit
>              ;;
> +        j)
> +            ut_run_queues=$OPTARG
> +            if ! is_number "$ut_run_queues"; then
> +                echo "Invalid -j option: $ut_run_queues"
> +                exit 1
> +            fi

Instead of adding is_number() and just checking for numeric
input, I'd check the input is > 0. A string input resolves
as zero when treated as a number, so it'll fail too.

> +            ;;
>          v)
>              verbose="yes"
>              ;;
> diff --git a/scripts/functions.bash b/scripts/functions.bash
> index d1d2e1c..c6281f4 100644
> --- a/scripts/functions.bash
> +++ b/scripts/functions.bash
> @@ -1,9 +1,18 @@
> +source scripts/global.bash
> +
>  function run_task()
>  {
>  	local testname="$2"
>  
> +	while [[ "$(jobs | wc -l)" == $ut_run_queues ]]; do

Bash arithmetic expressions should use (( ... )) instead
of [[ ... ]]

> +		# wait for any background test to finish
> +		wait -n
> +	done
> +
>  	RUNTIME_log_file="${ut_log_dir}/${testname}.log"
> -	"$@"
> +
> +	# start the testcase in the background
> +	"$@" &
>  }
>  
>  function for_each_unittest()
> @@ -20,6 +29,8 @@ function for_each_unittest()
>  	local accel
>  	local timeout
>  
> +	trap "wait; exit 130" SIGINT
> +
>  	exec {fd}<"$unittests"
>  
>  	while read -u $fd line; do
> @@ -53,5 +64,9 @@ function for_each_unittest()
>  		fi
>  	done
>  	run_task "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> +
> +	# wait all task finish
> +	wait
> +
>  	exec {fd}<&-
>  }
> diff --git a/scripts/global.bash b/scripts/global.bash
> index 77b0b29..52095bd 100644
> --- a/scripts/global.bash
> +++ b/scripts/global.bash
> @@ -1,2 +1,13 @@
>  : ${ut_log_dir:=logs}
>  : ${ut_log_summary:=${ut_log_dir}/SUMMARY}
> +: ${ut_run_queues:=1}
> +
> +function ut_in_parallel()
> +{
> +    [[ $ut_run_queues != 1 ]]
> +}

I don't see any users of ut_in_parallel. Anyway I'd drop it
and open code the condition, with ((...)), when needed.

> +
> +function is_number()
> +{
> +    [[ "$1" =~ ^[0-9]+$ ]]
> +}
> -- 
> 2.7.4
> 
> 

Thanks,
drew

Re: [Qemu-devel] [kvm-unit-tests PATCH v3 2/2] run_tests: allow run tests in parallel

[Peter Xu]

On Fri, Jan 06, 2017 at 03:40:41PM +0100, Andrew Jones wrote:
> On Fri, Jan 06, 2017 at 11:33:01AM +0800, Peter Xu wrote:
> > run_task.sh is getting slow. This patch is trying to make it faster by
> > running the tests concurrently.
> > 
> > We provide a new parameter "-j" for the run_tests.sh, which can be used
> > to specify how many run queues we want for the tests. Default queue
> > length is 1, which is the old behavior.
> > 
> > Quick test on my laptop (4 cores, 2 threads each) shows 3x speed boost:
> > 
> >    |-----------------+-----------|
> >    | command         | time used |
> >    |-----------------+-----------|
> >    | run_test.sh     | 75s       |
> >    | run_test.sh -j8 | 27s       |
> >    |-----------------+-----------|
> > 
> > Suggested-by: Radim Krčmář <rkrcmar@redhat.com>
> 
> Radim suggested how to implement this, but not the idea of implementing
> it. The suggested-by tag is for ideas, not the code, and the idea (which
> is a good one) was yours, not Radim's. So the above tag should be removed.
> It's hard to credit Radim for his help here. Adding a signed-off-by to
> indicate co-authorship is probably the best you can do. Of course he's
> the maintainer and will add that when he merges anyway though...

I see, thanks for the clarification. Then let me remove this line.

> 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  run_tests.sh           | 12 ++++++++++--
> >  scripts/functions.bash | 17 ++++++++++++++++-
> >  scripts/global.bash    | 11 +++++++++++
> >  3 files changed, 37 insertions(+), 3 deletions(-)
> > 
> > diff --git a/run_tests.sh b/run_tests.sh
> > index e1bb3a6..a4fc895 100755
> > --- a/run_tests.sh
> > +++ b/run_tests.sh
> > @@ -14,10 +14,11 @@ function usage()
> >  {
> >  cat <<EOF
> >  
> > -Usage: $0 [-g group] [-h] [-v]
> > +Usage: $0 [-g group] [-h] [-v] [-j N]
> 
> s/N/num_run_queues/

Sure.

[...]

> > @@ -38,6 +39,13 @@ while getopts "g:hv" opt; do
> >              usage
> >              exit
> >              ;;
> > +        j)
> > +            ut_run_queues=$OPTARG
> > +            if ! is_number "$ut_run_queues"; then
> > +                echo "Invalid -j option: $ut_run_queues"
> > +                exit 1
> > +            fi
> 
> Instead of adding is_number() and just checking for numeric
> input, I'd check the input is > 0. A string input resolves
> as zero when treated as a number, so it'll fail too.

Wasn't familiar with shell arithmetic before. Your suggestion is much
simpler, thanks.

> 
> > +            ;;
> >          v)
> >              verbose="yes"
> >              ;;
> > diff --git a/scripts/functions.bash b/scripts/functions.bash
> > index d1d2e1c..c6281f4 100644
> > --- a/scripts/functions.bash
> > +++ b/scripts/functions.bash
> > @@ -1,9 +1,18 @@
> > +source scripts/global.bash
> > +
> >  function run_task()
> >  {
> >  	local testname="$2"
> >  
> > +	while [[ "$(jobs | wc -l)" == $ut_run_queues ]]; do
> 
> Bash arithmetic expressions should use (( ... )) instead
> of [[ ... ]]

Will do.

[...]

> > diff --git a/scripts/global.bash b/scripts/global.bash
> > index 77b0b29..52095bd 100644
> > --- a/scripts/global.bash
> > +++ b/scripts/global.bash
> > @@ -1,2 +1,13 @@
> >  : ${ut_log_dir:=logs}
> >  : ${ut_log_summary:=${ut_log_dir}/SUMMARY}
> > +: ${ut_run_queues:=1}
> > +
> > +function ut_in_parallel()
> > +{
> > +    [[ $ut_run_queues != 1 ]]
> > +}
> 
> I don't see any users of ut_in_parallel. Anyway I'd drop it
> and open code the condition, with ((...)), when needed.

Yes, it should be removed.

Thanks!

-- peterx