On 10/5/24 09:16, Michael Tokarev wrote:
05.10.2024 01:01, Pierrick Bouvier 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.
Meson compiler tests are supposed to be independent of environment flags
(https://mesonbuild.com/Reference-manual_returned_compiler.html#returned-by).
However, CFLAGS are used anyway, thus masking the problem when using
something like CFLAGS='-march=native'. This is a meson bug and was reported:
https://github.com/mesonbuild/meson/issues/13757
Signed-off-by: Pierrick Bouvier <pierrick.bouv...@linaro.org>
---
meson.build | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index b18c2a54ab5..af2ce595dcc 100644
--- a/meson.build
+++ b/meson.build
@@ -2867,6 +2867,13 @@ if has_int128_type
config_host_data.set('CONFIG_ATOMIC128_OPT', has_atomic128_opt)
if not has_atomic128_opt
+
+ host_flags = []
+ if host_arch == 'x86_64'
+ # for x86_64, x86_version must be >= 1, and we always enable cmpxchg16
+ # in this case.
+ host_flags += ['-mcx16']
+ endif
config_host_data.set('CONFIG_CMPXCHG128', cc.links('''
int main(void)
{
@@ -2874,7 +2881,8 @@ if has_int128_type
__sync_val_compare_and_swap_16(&x, y, x);
return 0;
}
- '''))
+ ''',
+ args: host_flags))
endif
endif
endif
This does not look right. We test with an extra compiler flag, we should
ensure it is actually enabled for the actual build. Here, we only test
if cmpxchg128 works with this flag appended, but do not enable if for
the build. It just happens we have it enabled elsewhere..
Maybe we should add this flags somewhere to $qemu_cflags in this place
if the test were successful. With the proposed variant it is confusing
and works just because it happens to match in a few places, not because
it is supposed to work :)
If you look upper in meson.build (x86_version), you'll see this flag
(-mcx16) is *always* enabled for x86_64, which is what the comment
states in this patch.
So it's already added to qemu_common_flags anyway. The problem is just
that when we call cc.links, environment we defined is not reused.
After discussing on IRC, we identified it was a really special case only
for x64 and this instruction, so I didn't find useful to make a more
complex solution than just using the same flag.
Maybe it would be more clear to define a qemu_machine_flags, and reuse
it here (or in other places where it might be needed), but I feel like
it's too complicated, and we can always add this later.
I'm open to implement something different for flags management, but I
feel like we are thinking too much ahead.
Besides, in the current situation where CONFIG_CMPXCHG128 is not defined
due to this bug, the final link fails due to generated calls to -latomic, -
which might mean we have something else wrong.
By default, __sync_val_compare_and_swap_16 is not available, except if
we use explicitely -mcx16, or if we use an -march that implies this.
Our default level for x86_version does not enable it.
$ cat ./build/meson-logs/meson-log.txt
Code:
int main(void)
{
__uint128_t x = 0, y = 0;
__sync_val_compare_and_swap_16(&x, y, x);
return 0;
}
-----------
Command line: `cc -m64
/home/user/.work/qemu/build/meson-private/tmpkoy6pfnd/testfile.c -o
/home/user/.work/qemu/build/meson-private/tmpkoy6pfnd/output.exe -O2 -g
-fno-omit-f
rame-pointer -D_FILE_OFFSET_BITS=64 -O0 -std=gnu11` -> 1
stderr:
/usr/bin/ld: /tmp/cccPzS5X.o: in function `main':
/home/user/.work/qemu/build/meson-private/tmpkoy6pfnd/testfile.c:5:
undefined reference to `__sync_val_compare_and_swap_16'
collect2: error: ld returned 1 exit status
-----------
FWIW.
/mjt
