From mboxrd@z Thu Jan  1 00:00:00 1970
From: Peter Xu <peterx@redhat.com>
Subject: Re: [kvm-unit-tests PATCH 2/2] run_tests: allow run tests in parallel
Date: Tue, 3 Jan 2017 10:45:01 +0800
Message-ID: <20170103024501.GA22664@pxdev.xzpeter.org>
References: <1483266886-25050-1-git-send-email-peterx@redhat.com>
 <1483266886-25050-3-git-send-email-peterx@redhat.com>
 <20170102201824.GA2892@potion>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Andrew Jones <drjones@redhat.com>
To: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:56046 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S933716AbdACCpF (ORCPT <rfc822;kvm@vger.kernel.org>);
        Mon, 2 Jan 2017 21:45:05 -0500
Content-Disposition: inline
In-Reply-To: <20170102201824.GA2892@potion>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Mon, Jan 02, 2017 at 09:18:24PM +0100, Radim Krčmář wrote:
> 2017-01-01 18:34+0800, Peter Xu:
> > run_task.sh is getting slow. This patch is trying to make it faster by
> > running the tests concurrently.
> > 
> > First of all, 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.
> > When "-j" is not provided, we'll keep the old behavior.
> > 
> > When the tests are running concurrently, we will use seperate log file
> > for each test case (currently located in logs/ dir, with name
> > test.TESTNAME.log), to avoid test logs messing up with each other.
> > 
> > A quick test on my laptop (x86 with 4 cores and 2 threads, so 8
> > processors) shows 3x improvement on overall test time:
> > 
> >    |-----------------+-----------|
> >    | command         | time used |
> >    |-----------------+-----------|
> >    | run_test.sh     | 75s       |
> >    | run_test.sh -j8 | 27s       |
> >    |-----------------+-----------|
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > diff --git a/scripts/functions.bash b/scripts/functions.bash
> > @@ -1,7 +1,18 @@
> > +source scripts/global.bash
> > +source scripts/task.bash
> > +
> >  function run_task()
> >  {
> > -	RUNTIME_log_file=$ut_default_log_file
> > -	"$@"
> > +	local testname="$2"
> > +
> > +	if ut_in_parallel; then
> > +		RUNTIME_log_file="${ut_log_dir}/test.${testname}.log"
> 
> No need for the "test." prefix.
> 
> I would do this change regardless of ut_in_parallel.  Having output of
> all tests in one file just wasted time when most usecases were to find a
> specific failed test.
> 
> > +		# run in background
> > +		task_enqueue "$@"
> 
> Couldn't the queue be much simpler ...
> 
> > +	else
> > +		RUNTIME_log_file=$ut_default_log_file
> > +		"$@"
> > +	fi
> >  }
> >  
> >  function for_each_unittest()
> > @@ -51,5 +62,10 @@ function for_each_unittest()
> >  		fi
> >  	done
> 
> ... like this:
> 
>   while [ "`jobs | wc -l`" -gt $ut_run_queues ]; do
>     wait

I suppose you mean "wait -n" here? And also a "if" should suffice
here, though a "while" won't hurt as well.

>   done
>   run_task "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" &

I think this might work, however it has assumption that these $cmd
tasks are the only jobs that is running in the background.

I didn't notice the "-n" parameter for "wait", otherwise I won't
bother using SIGUSR1 at all. :)

Thanks,

-- peterx