[PATCH 0/3] Small CI improvements

[PATCH 0/3] Small CI improvements

[Paolo Bonzini]

Patch 1 adjusts cirrus-ci to also test installation, and thus
entitlement application on macOS.

Patch 2 and 3 tweak the number of jobs during "make".

Paolo Bonzini (3):
  cirrus-ci: test installation
  ci: do not use #processors+1 jobs, #processors is enough
  ci: add -j to all "make" jobs

 .cirrus.yml    | 22 ++++++++++++----------
 .gitlab-ci.yml | 10 +++++-----
 .travis.yml    | 10 +++++-----
 3 files changed, 22 insertions(+), 20 deletions(-)

-- 
2.31.1

[PATCH 1/3] cirrus-ci: test installation

[Paolo Bonzini]

Entitlements are applied via an install script that runs at installation
time.  Test it in CI.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 .cirrus.yml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index f4bf49b704..340fd395c0 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -37,6 +37,7 @@ macos_task:
     - gmake check-qapi-schema V=1
     - gmake check-softfloat V=1
     - gmake check-qtest-x86_64 V=1
+    - gmake install DESTDIR=$PWD/destdir
 
 macos_xcode_task:
   osx_instance:
@@ -47,7 +48,7 @@ macos_xcode_task:
   script:
     - mkdir build
     - cd build
-    - ../configure --extra-cflags='-Wno-error=deprecated-declarations' --enable-modules
+    - ../configure --extra-cflags='-Wno-error=deprecated-declarations' --enable-modules --disable-strip
                    --enable-werror --cc=clang || { cat config.log meson-logs/meson-log.txt; exit 1; }
     - gmake -j$(sysctl -n hw.ncpu)
     - gmake check-unit V=1
@@ -55,6 +56,7 @@ macos_xcode_task:
     - gmake check-qapi-schema V=1
     - gmake check-softfloat V=1
     - gmake check-qtest-x86_64 V=1
+    - gmake install DESTDIR=$PWD/destdir
 
 windows_msys2_task:
   timeout_in: 90m
-- 
2.31.1

Re: [PATCH 1/3] cirrus-ci: test installation

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Entitlements are applied via an install script that runs at installation
> time.  Test it in CI.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  .cirrus.yml | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/.cirrus.yml b/.cirrus.yml
> index f4bf49b704..340fd395c0 100644
> --- a/.cirrus.yml
> +++ b/.cirrus.yml
> @@ -37,6 +37,7 @@ macos_task:
>      - gmake check-qapi-schema V=1
>      - gmake check-softfloat V=1
>      - gmake check-qtest-x86_64 V=1
> +    - gmake install DESTDIR=$PWD/destdir
>  
>  macos_xcode_task:
>    osx_instance:
> @@ -47,7 +48,7 @@ macos_xcode_task:
>    script:
>      - mkdir build
>      - cd build
> -    - ../configure --extra-cflags='-Wno-error=deprecated-declarations' --enable-modules
> +    - ../configure --extra-cflags='-Wno-error=deprecated-declarations' --enable-modules --disable-strip

So what's the --disable-strip about? Surely we don't rely on the
installer packaging to do that for us?

>                     --enable-werror --cc=clang || { cat config.log meson-logs/meson-log.txt; exit 1; }
>      - gmake -j$(sysctl -n hw.ncpu)
>      - gmake check-unit V=1
> @@ -55,6 +56,7 @@ macos_xcode_task:
>      - gmake check-qapi-schema V=1
>      - gmake check-softfloat V=1
>      - gmake check-qtest-x86_64 V=1
> +    - gmake install DESTDIR=$PWD/destdir
>  
>  windows_msys2_task:
>    timeout_in: 90m


-- 
Alex Bennée

Re: [PATCH 1/3] cirrus-ci: test installation

[Paolo Bonzini]

On 18/05/21 15:50, Alex Bennée wrote:
>>   macos_xcode_task:
>>     osx_instance:
>> @@ -47,7 +48,7 @@ macos_xcode_task:
>>     script:
>>       - mkdir build
>>       - cd build
>> -    - ../configure --extra-cflags='-Wno-error=deprecated-declarations' --enable-modules
>> +    - ../configure --extra-cflags='-Wno-error=deprecated-declarations' --enable-modules --disable-strip
> So what's the --disable-strip about? Surely we don't rely on the
> installer packaging to do that for us?

It was simply testing both with and without strip, to make sure that 
there wasn't anything macOS-specific.

Paolo

[PATCH 2/3] ci: do not use #processors+1 jobs, #processors is enough

[Paolo Bonzini]

I could not reconstruct the origin of the $(($(nproc) + 1)) idiom,
but I suspect it was there only to have a sensible result when nproc
or getconf do not exist.  This can be achieved also with an "||".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 .gitlab-ci.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 4bd1a91aa8..3f0d86cf0a 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -17,7 +17,7 @@ include:
   stage: build
   image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:latest
   before_script:
-    - JOBS=$(expr $(nproc) + 1)
+    - JOBS=$(nproc || echo 1)
   script:
     - if test -n "$LD_JOBS";
       then
-- 
2.31.1

Re: [PATCH 2/3] ci: do not use #processors+1 jobs, #processors is enough

[Philippe Mathieu-Daudé]

On 5/18/21 10:41 AM, Paolo Bonzini wrote:
> I could not reconstruct the origin of the $(($(nproc) + 1)) idiom,

I guess it is the historical way make would aggressively use
the most compute power it could? Then later this bad habit impact
was reduced by the -l option to keep make under some system load
value.

> but I suspect it was there only to have a sensible result when nproc
> or getconf do not exist.  This can be achieved also with an "||".
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  .gitlab-ci.yml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

BTW more candidates:

.gitlab-ci.d/edk2.yml:49: - export JOBS=$(($(getconf _NPROCESSORS_ONLN)
+ 1))
.gitlab-ci.d/opensbi.yml:52: - export JOBS=$(($(getconf
_NPROCESSORS_ONLN) + 1))
.travis.yml:93:  - export JOBS=3

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 2/3] ci: do not use #processors+1 jobs, #processors is enough

[Thomas Huth]

On 18/05/2021 10.41, Paolo Bonzini wrote:
> I could not reconstruct the origin of the $(($(nproc) + 1)) idiom,
> but I suspect it was there only to have a sensible result when nproc
> or getconf do not exist.  This can be achieved also with an "||".
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   .gitlab-ci.yml | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 4bd1a91aa8..3f0d86cf0a 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -17,7 +17,7 @@ include:
>     stage: build
>     image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:latest
>     before_script:
> -    - JOBS=$(expr $(nproc) + 1)
> +    - JOBS=$(nproc || echo 1)

The basic idea of the "+ 1" was to make sure that there is always a thread 
that runs on a CPU while maybe another one is waiting for I/O to complete. 
This is suggested by various sites on the web, e.g.:

 
https://unix.stackexchange.com/questions/519092/what-is-the-logic-of-using-nproc-1-in-make-command

So not sure whether this patch here make sense ... I'd rather drop it.

  Thomas

Re: [PATCH 2/3] ci: do not use #processors+1 jobs, #processors is enough

[Paolo Bonzini]

On 18/05/21 12:49, Thomas Huth wrote:
>>
>> -    - JOBS=$(expr $(nproc) + 1)
>> +    - JOBS=$(nproc || echo 1)
> 
> The basic idea of the "+ 1" was to make sure that there is always a 
> thread that runs on a CPU while maybe another one is waiting for I/O to 
> complete.

Ah, I see.  It doesn't make much sense for "make check" jobs however, 
which is where I wanted to get with the next patch.

I'm not sure it's even true anymore with current build machines (which 
tend to have a large buffer cache for headers) and optimizing compilers 
that compilation is I/O bound, so I'll time the two and see if there is 
an actual difference.

Paolo

  This is suggested by various sites on the web, e.g.:
> 
> https://unix.stackexchange.com/questions/519092/what-is-the-logic-of-using-nproc-1-in-make-command 
> 
> So not sure whether this patch here make sense ... I'd rather drop it.

Re: [PATCH 2/3] ci: do not use #processors+1 jobs, #processors is enough

[Daniel P. Berrangé]

On Tue, May 18, 2021 at 02:30:04PM +0200, Paolo Bonzini wrote:
> On 18/05/21 12:49, Thomas Huth wrote:
> > > 
> > > -    - JOBS=$(expr $(nproc) + 1)
> > > +    - JOBS=$(nproc || echo 1)
> > 
> > The basic idea of the "+ 1" was to make sure that there is always a
> > thread that runs on a CPU while maybe another one is waiting for I/O to
> > complete.
> 
> Ah, I see.  It doesn't make much sense for "make check" jobs however, which
> is where I wanted to get with the next patch.
> 
> I'm not sure it's even true anymore with current build machines (which tend
> to have a large buffer cache for headers) and optimizing compilers that
> compilation is I/O bound, so I'll time the two and see if there is an actual
> difference.

I'd be surprised if you can measure any statistically reliable difference
at all wrt public CI. I've tried measuring CI performance for small changes
and found it impossible in short time frames, as the deviation between runs
is way too large. GitLab CI speeds tend to slow down as the day goes on and
US wakes up, so by time you run QEMU CI a second time in the day, it will
be slower. They clearly overcommit resources on the cloud host so you're
at the mercy of whatever else is running.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/3] ci: do not use #processors+1 jobs, #processors is enough

[Paolo Bonzini]

On 18/05/21 14:43, Daniel P. Berrangé wrote:
> I'd be surprised if you can measure any statistically reliable difference
> at all wrt public CI. I've tried measuring CI performance for small changes
> and found it impossible in short time frames, as the deviation between runs
> is way too large. GitLab CI speeds tend to slow down as the day goes on and
> US wakes up, so by time you run QEMU CI a second time in the day, it will
> be slower. They clearly overcommit resources on the cloud host so you're
> at the mercy of whatever else is running.

Yeah, I was going to test it locally (using CPU offlining and hugetlbfs 
to simulate a 4-CPU machine with not that much memory).

Paolo

[PATCH 3/3] ci: add -j to all "make" jobs

[Paolo Bonzini]

Run the tests in parallel, as that can greatly speed up the jobs.
"meson test" is able to report failures to the terminal in a way
that is readable enough even when tests are run in parallel.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 .cirrus.yml    | 18 +++++++++---------
 .gitlab-ci.yml |  8 ++++----
 .travis.yml    | 10 +++++-----
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index 340fd395c0..fcf0f34a4c 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -32,11 +32,11 @@ macos_task:
                    --extra-cflags='-Wno-error=deprecated-declarations'
                    || { cat config.log meson-logs/meson-log.txt; exit 1; }
     - gmake -j$(sysctl -n hw.ncpu)
-    - gmake check-unit V=1
+    - gmake -j$(sysctl -n hw.ncpu) check-unit V=1
     - gmake check-block V=1
-    - gmake check-qapi-schema V=1
-    - gmake check-softfloat V=1
-    - gmake check-qtest-x86_64 V=1
+    - gmake -j$(sysctl -n hw.ncpu) check-qapi-schema V=1
+    - gmake -j$(sysctl -n hw.ncpu) check-softfloat V=1
+    - gmake -j$(sysctl -n hw.ncpu) check-qtest-x86_64 V=1
     - gmake install DESTDIR=$PWD/destdir
 
 macos_xcode_task:
@@ -51,11 +51,11 @@ macos_xcode_task:
     - ../configure --extra-cflags='-Wno-error=deprecated-declarations' --enable-modules --disable-strip
                    --enable-werror --cc=clang || { cat config.log meson-logs/meson-log.txt; exit 1; }
     - gmake -j$(sysctl -n hw.ncpu)
-    - gmake check-unit V=1
+    - gmake -j$(sysctl -n hw.ncpu) check-unit V=1
     - gmake check-block V=1
-    - gmake check-qapi-schema V=1
-    - gmake check-softfloat V=1
-    - gmake check-qtest-x86_64 V=1
+    - gmake -j$(sysctl -n hw.ncpu) check-qapi-schema V=1
+    - gmake -j$(sysctl -n hw.ncpu) check-softfloat V=1
+    - gmake -j$(sysctl -n hw.ncpu) check-qtest-x86_64 V=1
     - gmake install DESTDIR=$PWD/destdir
 
 windows_msys2_task:
@@ -163,5 +163,5 @@ windows_msys2_task:
     - C:\tools\msys64\usr\bin\bash.exe -lc "cd build && make -j8"
     - exit $LastExitCode
   test_script:
-    - C:\tools\msys64\usr\bin\bash.exe -lc "cd build && make V=1 check"
+    - C:\tools\msys64\usr\bin\bash.exe -lc "cd build && make -j4 V=1 check"
     - exit $LastExitCode
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 3f0d86cf0a..4c0cc47c25 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -50,7 +50,7 @@ include:
     - cd build
     - find . -type f -exec touch {} +
     # Avoid recompiling by hiding ninja with NINJA=":"
-    - make NINJA=":" $MAKE_CHECK_ARGS
+    - make -j"$JOBS" NINJA=":" $MAKE_CHECK_ARGS
 
 .meson_test_template: &meson_test_definition
   artifacts:
@@ -402,8 +402,8 @@ build-tcg-disabled:
     - ../configure --disable-tcg --audio-drv-list="" --with-coroutine=ucontext
       || { cat config.log meson-logs/meson-log.txt && exit 1; }
     - make -j"$JOBS"
-    - make check-unit
-    - make check-qapi-schema
+    - make check-unit -j"$JOBS"
+    - make check-qapi-schema -j"$JOBS"
     - cd tests/qemu-iotests/
     - ./check -raw 001 002 003 004 005 008 009 010 011 012 021 025 032 033 048
             052 063 077 086 101 104 106 113 148 150 151 152 157 159 160 163
@@ -689,7 +689,7 @@ build-oss-fuzz:
         "${fuzzer}" -runs=1 -seed=1 || exit 1 ;
       done
     # Unrelated to fuzzer: run some tests with -fsanitize=address
-    - cd build-oss-fuzz && make check-qtest-i386 check-unit MTESTARGS="-t 2"
+    - cd build-oss-fuzz && make check-qtest-i386 check-unit -j"$JOBS" MTESTARGS="-t 2"
 
 build-tci:
   extends: .native_build_job_template
diff --git a/.travis.yml b/.travis.yml
index 4609240b5a..42e8f0b348 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -73,7 +73,7 @@ env:
     - BUILD_DIR="build"
     - BASE_CONFIG="--disable-docs --disable-tools"
     - TEST_BUILD_CMD=""
-    - TEST_CMD="make check V=1"
+    - TEST_CMD="make check -j${JOBS} V=1"
     # This is broadly a list of "mainline" softmmu targets which have support across the major distros
     - MAIN_SOFTMMU_TARGETS="aarch64-softmmu,mips64-softmmu,ppc64-softmmu,riscv64-softmmu,s390x-softmmu,x86_64-softmmu"
     - CCACHE_SLOPPINESS="include_file_ctime,include_file_mtime"
@@ -151,7 +151,7 @@ jobs:
           # Tests dependencies
           - genisoimage
       env:
-        - TEST_CMD="make check check-tcg V=1"
+        - TEST_CMD="make check check-tcg -j${JOBS} V=1"
         - CONFIG="--disable-containers --target-list=${MAIN_SOFTMMU_TARGETS} --cxx=/bin/false"
         - UNRELIABLE=true
 
@@ -185,7 +185,7 @@ jobs:
           # Tests dependencies
           - genisoimage
       env:
-        - TEST_CMD="make check check-tcg V=1"
+        - TEST_CMD="make check check-tcg -j${JOBS} V=1"
         - CONFIG="--disable-containers --target-list=ppc64-softmmu,ppc64le-linux-user"
 
     - name: "[s390x] GCC check-tcg"
@@ -218,7 +218,7 @@ jobs:
           # Tests dependencies
           - genisoimage
       env:
-        - TEST_CMD="make check check-tcg V=1"
+        - TEST_CMD="make check check-tcg -j${JOBS} V=1"
         - CONFIG="--disable-containers --target-list=${MAIN_SOFTMMU_TARGETS},s390x-linux-user"
         - UNRELIABLE=true
       script:
@@ -301,7 +301,7 @@ jobs:
           - libvte-2.91-dev
           - ninja-build
       env:
-        - TEST_CMD="make check-unit"
+        - TEST_CMD="make check-unit -j${JOBS}"
         - CONFIG="--disable-containers --disable-tcg --enable-kvm
                   --disable-tools --host-cc=clang --cxx=clang++"
         - UNRELIABLE=true
-- 
2.31.1

Re: [PATCH 3/3] ci: add -j to all "make" jobs

[Philippe Mathieu-Daudé]

On 5/18/21 10:41 AM, Paolo Bonzini wrote:
> Run the tests in parallel, as that can greatly speed up the jobs.
> "meson test" is able to report failures to the terminal in a way
> that is readable enough even when tests are run in parallel.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  .cirrus.yml    | 18 +++++++++---------
>  .gitlab-ci.yml |  8 ++++----
>  .travis.yml    | 10 +++++-----
>  3 files changed, 18 insertions(+), 18 deletions(-)

Hmmm maybe that can be merged with "Run GNU make via the $MAKE variable"
https://www.mail-archive.com/qemu-devel@nongnu.org/msg805900.html

Re: [PATCH 0/3] Small CI improvements

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Patch 1 adjusts cirrus-ci to also test installation, and thus
> entitlement application on macOS.
>
> Patch 2 and 3 tweak the number of jobs during "make".

Hmm this seemed to time out a load and fail with some exit codes:

  https://gitlab.com/stsquad/qemu/-/pipelines/304979329/failures

>
> Paolo Bonzini (3):
>   cirrus-ci: test installation
>   ci: do not use #processors+1 jobs, #processors is enough
>   ci: add -j to all "make" jobs
>
>  .cirrus.yml    | 22 ++++++++++++----------
>  .gitlab-ci.yml | 10 +++++-----
>  .travis.yml    | 10 +++++-----
>  3 files changed, 22 insertions(+), 20 deletions(-)


-- 
Alex Bennée