[PATCH v2 0/2] meson: ensure we enable CMPXCHG128 on x86_64

[PATCH v2 0/2] meson: ensure we enable CMPXCHG128 on x86_64

[Paolo Bonzini]

Alex discovered that CMPXCHG128 was not enabled when building for
x86_64, resulting in slow execution for wide atomic instructions,
creating a huge contention when combined with a high number of cpus
(found while booting android aarch64 guest on x86_64 host).

The problem is that even though we enable -mcx16 option for x86_64, this
is not used when testing for CMPXCHG128. Thus, we silently turn it off.

x86_64 is the only architecture adding machine flags for now, so the
problem is limited to this host architecture.  However, the problem
is generic, so define a new variable for all the -m options, so that
they can be used for other such tests in the future.

Based-on: <20241004223715.1275428-1-pierrick.bouvier@linaro.org>
Supersedes: <20241004220123.978938-1-pierrick.bouvier@linaro.org>

Paolo Bonzini (2):
  meson: define qemu_isa_flags
  meson: ensure -mcx16 is passed when detecting ATOMIC128

 meson.build | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

-- 
2.46.1

[PATCH 1/2] meson: define qemu_isa_flags

[Paolo Bonzini]

Create a separate variable for compiler flags that enable
specific instruction set extensions, so that they can be used with
cc.compiles/cc.links.

Note that -mfpmath=sse is a code generation option but it does not
enable new instructions, therefore I did not make it part of
qemu_isa_flags.

Suggested-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/meson.build b/meson.build
index 76335bb2a51..bbb0f6d9bcf 100644
--- a/meson.build
+++ b/meson.build
@@ -335,6 +335,10 @@ elif host_os == 'windows'
   endif
 endif
 
+# Choose instruction set (currently x86-only)
+
+qemu_isa_flags = []
+
 # __sync_fetch_and_and requires at least -march=i486. Many toolchains
 # use i686 as default anyway, but for those that don't, an explicit
 # specification is necessary
@@ -351,7 +355,7 @@ if host_arch == 'i386' and not cc.links('''
     sfaa(&val);
     return val;
   }''')
-  qemu_common_flags = ['-march=i486'] + qemu_common_flags
+  qemu_isa_flags = ['-march=i486']
 endif
 
 # Pick x86-64 baseline version
@@ -367,29 +371,31 @@ if host_arch in ['i386', 'x86_64']
     else
       # present on basically all processors but technically not part of
       # x86-64-v1, so only include -mneeded for x86-64 version 2 and above
-      qemu_common_flags = ['-mcx16'] + qemu_common_flags
+      qemu_isa_flags = ['-mcx16'] + qemu_isa_flags
     endif
   endif
   if get_option('x86_version') >= '2'
-    qemu_common_flags = ['-mpopcnt'] + qemu_common_flags
-    qemu_common_flags = cc.get_supported_arguments('-mneeded') + qemu_common_flags
+    qemu_isa_flags = ['-mpopcnt'] + qemu_isa_flags
+    qemu_isa_flags = cc.get_supported_arguments('-mneeded') + qemu_isa_flags
   endif
   if get_option('x86_version') >= '3'
-    qemu_common_flags = ['-mmovbe', '-mabm', '-mbmi', '-mbmi2', '-mfma', '-mf16c'] + qemu_common_flags
+    qemu_isa_flags = ['-mmovbe', '-mabm', '-mbmi', '-mbmi2', '-mfma', '-mf16c'] + qemu_isa_flags
   endif
 
   # add required vector instruction set (each level implies those below)
   if get_option('x86_version') == '1'
-    qemu_common_flags = ['-msse2'] + qemu_common_flags
+    qemu_isa_flags = ['-msse2'] + qemu_isa_flags
   elif get_option('x86_version') == '2'
-    qemu_common_flags = ['-msse4.2'] + qemu_common_flags
+    qemu_isa_flags = ['-msse4.2'] + qemu_isa_flags
   elif get_option('x86_version') == '3'
-    qemu_common_flags = ['-mavx2'] + qemu_common_flags
+    qemu_isa_flags = ['-mavx2'] + qemu_isa_flags
   elif get_option('x86_version') == '4'
-    qemu_common_flags = ['-mavx512f', '-mavx512bw', '-mavx512cd', '-mavx512dq', '-mavx512vl'] + qemu_common_flags
+    qemu_isa_flags = ['-mavx512f', '-mavx512bw', '-mavx512cd', '-mavx512dq', '-mavx512vl'] + qemu_isa_flags
   endif
 endif
 
+qemu_common_flags = qemu_isa_flags + qemu_common_flags
+
 if get_option('prefer_static')
   qemu_ldflags += get_option('b_pie') ? '-static-pie' : '-static'
 endif
-- 
2.46.1

[PATCH 2/2] meson: ensure -mcx16 is passed when detecting ATOMIC128

[Paolo Bonzini]

Moving -mcx16 out of CPU_CFLAGS caused the detection of ATOMIC128 to
fail, because flags have to be specified by hand in cc.compiles and
cc.links invocations (why oh why??).

Ensure that these tests enable all the instruction set extensions that
will be used to build the emulators.

Fixes: c2bf2ccb266 ("configure: move -mcx16 flag out of CPU_CFLAGS", 2024-05-24)
Reported-by: Alex Bennée <alex.bennee@linaro.org>
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/meson.build b/meson.build
index bbb0f6d9bcf..4ceae98ee87 100644
--- a/meson.build
+++ b/meson.build
@@ -2831,7 +2831,7 @@ config_host_data.set('CONFIG_ATOMIC64', cc.links('''
     __atomic_exchange_n(&x, y, __ATOMIC_RELAXED);
     __atomic_fetch_add(&x, y, __ATOMIC_RELAXED);
     return 0;
-  }'''))
+  }''', args: qemu_isa_flags))
 
 has_int128_type = cc.compiles('''
   __int128_t a;
@@ -2865,7 +2865,7 @@ if has_int128_type
       __atomic_compare_exchange_n(&p[4], &p[5], p[6], 0, __ATOMIC_RELAXED, __ATOMIC_RELAXED);
       return 0;
     }'''
-  has_atomic128 = cc.links(atomic_test_128)
+  has_atomic128 = cc.links(atomic_test_128, args: qemu_isa_flags)
 
   config_host_data.set('CONFIG_ATOMIC128', has_atomic128)
 
@@ -2874,7 +2874,8 @@ if has_int128_type
     # without optimization enabled.  Try again with optimizations locally
     # enabled for the function.  See
     #   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107389
-    has_atomic128_opt = cc.links('__attribute__((optimize("O1")))' + atomic_test_128)
+    has_atomic128_opt = cc.links('__attribute__((optimize("O1")))' + atomic_test_128,
+                                 args: qemu_isa_flags)
     config_host_data.set('CONFIG_ATOMIC128_OPT', has_atomic128_opt)
 
     if not has_atomic128_opt
@@ -2885,7 +2886,7 @@ if has_int128_type
           __sync_val_compare_and_swap_16(&x, y, x);
           return 0;
         }
-      '''))
+      ''', args: qemu_isa_flags))
     endif
   endif
 endif
-- 
2.46.1

Re: [PATCH v2 0/2] meson: ensure we enable CMPXCHG128 on x86_64

[Michael Tokarev]

07.10.2024 11:43, Paolo Bonzini wrote:
> 
> Alex discovered that CMPXCHG128 was not enabled when building for
> x86_64, resulting in slow execution for wide atomic instructions,
> creating a huge contention when combined with a high number of cpus
> (found while booting android aarch64 guest on x86_64 host).
> 
> The problem is that even though we enable -mcx16 option for x86_64, this
> is not used when testing for CMPXCHG128. Thus, we silently turn it off.
> 
> x86_64 is the only architecture adding machine flags for now, so the
> problem is limited to this host architecture.  However, the problem
> is generic, so define a new variable for all the -m options, so that
> they can be used for other such tests in the future.
> 
> Based-on: <20241004223715.1275428-1-pierrick.bouvier@linaro.org>
> Supersedes: <20241004220123.978938-1-pierrick.bouvier@linaro.org>
> 
> Paolo Bonzini (2):
>    meson: define qemu_isa_flags
>    meson: ensure -mcx16 is passed when detecting ATOMIC128
> 
>   meson.build | 33 ++++++++++++++++++++-------------
>   1 file changed, 20 insertions(+), 13 deletions(-)

Series:
Reviewed-By: Michael Tokarev <mjt@tls.msk.ru>

Yeah, this is exactly what I thought about when replying to email
by Pierrick Bouvier.  Well, with less details ofc :)

Thanks,

/mjt

Re: [PATCH v2 0/2] meson: ensure we enable CMPXCHG128 on x86_64

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Alex discovered that CMPXCHG128 was not enabled when building for
> x86_64, resulting in slow execution for wide atomic instructions,
> creating a huge contention when combined with a high number of cpus
> (found while booting android aarch64 guest on x86_64 host).
>
> The problem is that even though we enable -mcx16 option for x86_64, this
> is not used when testing for CMPXCHG128. Thus, we silently turn it off.
>
> x86_64 is the only architecture adding machine flags for now, so the
> problem is limited to this host architecture.  However, the problem
> is generic, so define a new variable for all the -m options, so that
> they can be used for other such tests in the future.
>
> Based-on: <20241004223715.1275428-1-pierrick.bouvier@linaro.org>
> Supersedes: <20241004220123.978938-1-pierrick.bouvier@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 1/2] meson: define qemu_isa_flags

[Richard Henderson]

On 10/7/24 01:43, Paolo Bonzini wrote:
> Create a separate variable for compiler flags that enable
> specific instruction set extensions, so that they can be used with
> cc.compiles/cc.links.
> 
> Note that -mfpmath=sse is a code generation option but it does not
> enable new instructions, therefore I did not make it part of
> qemu_isa_flags.
> 
> Suggested-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   meson.build | 24 +++++++++++++++---------
>   1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index 76335bb2a51..bbb0f6d9bcf 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -335,6 +335,10 @@ elif host_os == 'windows'
>     endif
>   endif
>   
> +# Choose instruction set (currently x86-only)
> +
> +qemu_isa_flags = []
> +
>   # __sync_fetch_and_and requires at least -march=i486. Many toolchains
>   # use i686 as default anyway, but for those that don't, an explicit
>   # specification is necessary
> @@ -351,7 +355,7 @@ if host_arch == 'i386' and not cc.links('''
>       sfaa(&val);
>       return val;
>     }''')
> -  qemu_common_flags = ['-march=i486'] + qemu_common_flags
> +  qemu_isa_flags = ['-march=i486']

Use += ?

> -      qemu_common_flags = ['-mcx16'] + qemu_common_flags
> +      qemu_isa_flags = ['-mcx16'] + qemu_isa_flags

Likewise, why verbosely prepend, rather than append with += ?
The same for all others, including

> +qemu_common_flags = qemu_isa_flags + qemu_common_flags


r~

Re: [PATCH 2/2] meson: ensure -mcx16 is passed when detecting ATOMIC128

[Richard Henderson]

On 10/7/24 01:43, Paolo Bonzini wrote:
> Moving -mcx16 out of CPU_CFLAGS caused the detection of ATOMIC128 to
> fail, because flags have to be specified by hand in cc.compiles and
> cc.links invocations (why oh why??).
> 
> Ensure that these tests enable all the instruction set extensions that
> will be used to build the emulators.
> 
> Fixes: c2bf2ccb266 ("configure: move -mcx16 flag out of CPU_CFLAGS", 2024-05-24)
> Reported-by: Alex Bennée<alex.bennee@linaro.org>
> Cc:qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   meson.build | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 1/2] meson: define qemu_isa_flags

[Paolo Bonzini]

On Mon, Oct 7, 2024 at 4:46 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
> > -  qemu_common_flags = ['-march=i486'] + qemu_common_flags
> > +  qemu_isa_flags = ['-march=i486']
>
> Use += ?

Here the qemu_isa_flags are known to be empty.

> > -      qemu_common_flags = ['-mcx16'] + qemu_common_flags
> > +      qemu_isa_flags = ['-mcx16'] + qemu_isa_flags
>
> Likewise, why verbosely prepend, rather than append with += ?
> The same for all others, including
>
> > +qemu_common_flags = qemu_isa_flags + qemu_common_flags

The prepending behavior came from CPU_CFLAGS originally being before
all other compiler flags.

I just didn't want to change behavior here (especially since this
patch was already about subtle changes in behavior). But you're right
that at least within qemu_isa_flags the order should not matter.

Paolo

Re: [PATCH 1/2] meson: define qemu_isa_flags

[Richard Henderson]

On 10/7/24 07:48, Paolo Bonzini wrote:
> On Mon, Oct 7, 2024 at 4:46 PM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>> -  qemu_common_flags = ['-march=i486'] + qemu_common_flags
>>> +  qemu_isa_flags = ['-march=i486']
>>
>> Use += ?
> 
> Here the qemu_isa_flags are known to be empty.

... until we add something else beforehand and fail to update.
Surely it's easier to assume that it's not empty.

> I just didn't want to change behavior here (especially since this
> patch was already about subtle changes in behavior). But you're right
> that at least within qemu_isa_flags the order should not matter.

Fair enough.


r~

Re: [PATCH 1/2] meson: define qemu_isa_flags

[Michael Tokarev]

07.10.2024 17:46, Richard Henderson пишет:
> On 10/7/24 01:43, Paolo Bonzini wrote:
>> Create a separate variable for compiler flags that enable
>> specific instruction set extensions, so that they can be used with
>> cc.compiles/cc.links.
>>
>> Note that -mfpmath=sse is a code generation option but it does not
>> enable new instructions, therefore I did not make it part of
>> qemu_isa_flags.
>>
>> Suggested-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   meson.build | 24 +++++++++++++++---------
>>   1 file changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/meson.build b/meson.build
>> index 76335bb2a51..bbb0f6d9bcf 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -335,6 +335,10 @@ elif host_os == 'windows'
>>     endif
>>   endif
>> +# Choose instruction set (currently x86-only)
>> +
>> +qemu_isa_flags = []
>> +
>>   # __sync_fetch_and_and requires at least -march=i486. Many toolchains
>>   # use i686 as default anyway, but for those that don't, an explicit
>>   # specification is necessary
>> @@ -351,7 +355,7 @@ if host_arch == 'i386' and not cc.links('''
>>       sfaa(&val);
>>       return val;
>>     }''')
>> -  qemu_common_flags = ['-march=i486'] + qemu_common_flags
>> +  qemu_isa_flags = ['-march=i486']
> 
> Use += ?

Yes, it is better to use += here - when reviewing the patch this is
obvious qemu_isa_flags is empty at this point, but it wont be as
obvious in the actual code when initial assignment is quite far away
from this prepending.


>> -      qemu_common_flags = ['-mcx16'] + qemu_common_flags
>> +      qemu_isa_flags = ['-mcx16'] + qemu_isa_flags
> 
> Likewise, why verbosely prepend, rather than append with += ?
> The same for all others, including
> 
>> +qemu_common_flags = qemu_isa_flags + qemu_common_flags

I'd keep prepending here (but not in qemu_isa_flags above) because this
way it is possible to override some flags by the user.

/mjt

Re: [PATCH v2 0/2] meson: ensure we enable CMPXCHG128 on x86_64

[Pierrick Bouvier]

On 10/7/24 01:43, Paolo Bonzini wrote:
> 
> Alex discovered that CMPXCHG128 was not enabled when building for
> x86_64, resulting in slow execution for wide atomic instructions,
> creating a huge contention when combined with a high number of cpus
> (found while booting android aarch64 guest on x86_64 host).
> 
> The problem is that even though we enable -mcx16 option for x86_64, this
> is not used when testing for CMPXCHG128. Thus, we silently turn it off.
> 
> x86_64 is the only architecture adding machine flags for now, so the
> problem is limited to this host architecture.  However, the problem
> is generic, so define a new variable for all the -m options, so that
> they can be used for other such tests in the future.
> 
> Based-on: <20241004223715.1275428-1-pierrick.bouvier@linaro.org>
> Supersedes: <20241004220123.978938-1-pierrick.bouvier@linaro.org>
> 
> Paolo Bonzini (2):
>    meson: define qemu_isa_flags
>    meson: ensure -mcx16 is passed when detecting ATOMIC128
> 
>   meson.build | 33 ++++++++++++++++++++-------------
>   1 file changed, 20 insertions(+), 13 deletions(-)
> 

Hi Paolo,

Thanks for fixing this.

Series:
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

I was wondering why we have a specific flags path for x64.
People who are interested in getting machine specific binaries can 
always play with CFLAGS, so what was the original intent here?

Thanks,
Pierrick

[PATCH 1/2] meson: define qemu_isa_flags

[Paolo Bonzini]

Create a separate variable for compiler flags that enable
specific instruction set extensions, so that they can be used with
cc.compiles/cc.links.

Note that -mfpmath=sse is a code generation option but it does not
enable new instructions, therefore I did not make it part of
qemu_isa_flags.

Suggested-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/meson.build b/meson.build
index 76335bb2a51..65ea45cef03 100644
--- a/meson.build
+++ b/meson.build
@@ -335,6 +335,10 @@ elif host_os == 'windows'
   endif
 endif
 
+# Choose instruction set (currently x86-only)
+
+qemu_isa_flags = []
+
 # __sync_fetch_and_and requires at least -march=i486. Many toolchains
 # use i686 as default anyway, but for those that don't, an explicit
 # specification is necessary
@@ -351,7 +355,7 @@ if host_arch == 'i386' and not cc.links('''
     sfaa(&val);
     return val;
   }''')
-  qemu_common_flags = ['-march=i486'] + qemu_common_flags
+  qemu_isa_flags += ['-march=i486']
 endif
 
 # Pick x86-64 baseline version
@@ -367,29 +371,31 @@ if host_arch in ['i386', 'x86_64']
     else
       # present on basically all processors but technically not part of
       # x86-64-v1, so only include -mneeded for x86-64 version 2 and above
-      qemu_common_flags = ['-mcx16'] + qemu_common_flags
+      qemu_isa_flags += ['-mcx16']
     endif
   endif
   if get_option('x86_version') >= '2'
-    qemu_common_flags = ['-mpopcnt'] + qemu_common_flags
-    qemu_common_flags = cc.get_supported_arguments('-mneeded') + qemu_common_flags
+    qemu_isa_flags += ['-mpopcnt']
+    qemu_isa_flags += cc.get_supported_arguments('-mneeded')
   endif
   if get_option('x86_version') >= '3'
-    qemu_common_flags = ['-mmovbe', '-mabm', '-mbmi', '-mbmi2', '-mfma', '-mf16c'] + qemu_common_flags
+    qemu_isa_flags += ['-mmovbe', '-mabm', '-mbmi', '-mbmi2', '-mfma', '-mf16c']
   endif
 
   # add required vector instruction set (each level implies those below)
   if get_option('x86_version') == '1'
-    qemu_common_flags = ['-msse2'] + qemu_common_flags
+    qemu_isa_flags += ['-msse2']
   elif get_option('x86_version') == '2'
-    qemu_common_flags = ['-msse4.2'] + qemu_common_flags
+    qemu_isa_flags += ['-msse4.2']
   elif get_option('x86_version') == '3'
-    qemu_common_flags = ['-mavx2'] + qemu_common_flags
+    qemu_isa_flags += ['-mavx2']
   elif get_option('x86_version') == '4'
-    qemu_common_flags = ['-mavx512f', '-mavx512bw', '-mavx512cd', '-mavx512dq', '-mavx512vl'] + qemu_common_flags
+    qemu_isa_flags += ['-mavx512f', '-mavx512bw', '-mavx512cd', '-mavx512dq', '-mavx512vl']
   endif
 endif
 
+qemu_common_flags = qemu_isa_flags + qemu_common_flags
+
 if get_option('prefer_static')
   qemu_ldflags += get_option('b_pie') ? '-static-pie' : '-static'
 endif
-- 
2.46.1