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ář <address@hidden>

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 <address@hidden>
> ---
>  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