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ář <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...

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

> 
> > 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/

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