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

Paolo Bonzini posted 3 patches 4 years, 8 months ago
[PATCH 2/3] ci: do not use #processors+1 jobs, #processors is enough
Posted by Paolo Bonzini 4 years, 8 months ago
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
Posted by Philippe Mathieu-Daudé 4 years, 8 months ago
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
Posted by Thomas Huth 4 years, 8 months ago
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
Posted by Paolo Bonzini 4 years, 8 months ago
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.