enable build of lib/stack when using MSVC
cover enable build of lib/stack when using MSVC
MSVC does not support inline assembly, which is used by the implementation of rte_atomic128_cmp_exchange and is needed by lib/stack.
An implementation for rte_atomic128_cmp_exchange compatible with MSVC using an intrinsic function is added. For other compilers the existing implementation continues to be used.
A basic test is added to provide coverage for this special rte_atomic128_cmp_exchange implementation for MSVC. This same test can be run when using other compilers as well, in which case the old implementation for rte_atomic128_cmp_exchange is used.
Andre Muezerie (3):
lib/eal: add rte_atomic128_cmp_exchange compatible with MSVC app/test: add basic test for rte_atomic128_cmp_exchange lib/stack: enable build with MSVC
app/test/test_atomic.c | 59 +++++++++++++++++++++++++++++ lib/eal/x86/include/rte_atomic.h | 4 +- lib/eal/x86/include/rte_atomic_64.h | 18 +++++++++ lib/stack/meson.build | 6 --- 4 files changed, 79 insertions(+), 8 deletions(-)
--
2.47.0.vfs.0.3
PATCH 1/3 lib/eal: add rte_atomic128_cmp_exchange compatible with MSVC
MSVC does not support inline assembly, which is used by the implementation of rte_atomic128_cmp_exchange and is needed by lib/stack.
Error printed by MSVC:
stack_rte_stack_lf.c.obj : error LNK2019:
unresolved external symbol rte_atomic128_cmp_exchange referenced in function __rte_stack_lf_push_elems
Fix is to provide an implementation for rte_atomic128_cmp_exchange which uses an intrinsic function, which is used when compiling with MSVC. For other compilers the existing implementation continues to be used.
--- lib/eal/x86/include/rte_atomic.h | 4 ++-- lib/eal/x86/include/rte_atomic_64.h | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-)
lib/eal/x86/include/rte_atomic.h +2 -2
@@ -288,12 +288,12 @@ static inline int rte_atomic32_dec_and_test(rte_atomic32_t *v)#endif +#endif /* RTE_TOOLCHAIN_MSVC */ +#ifdef RTE_ARCH_I686 #include "rte_atomic_32.h" #else #include "rte_atomic_64.h" #endif -#endif -#endif /* _RTE_ATOMIC_X86_H_ */
lib/eal/x86/include/rte_atomic_64.h +18 -0
@@ -182,6 +182,23 @@ static inline void rte_atomic64_clear(rte_atomic64_t *v)/*------------------------ 128 bit atomic operations -------------------------*/ +#ifdef RTE_TOOLCHAIN_MSVC +static inline int +rte_atomic128_cmp_exchange (rte_int128_t *dst, +rte_int128_t *exp, +const rte_int128_t *src, +unsigned int weak, +int success, +int failure) +{ +return (int )_InterlockedCompareExchange128 ( + (int64_t volatile *) dst, + src->val[1 ],/* exchange high */ + src->val[0 ],/* exchange low */ + (int64_t *) exp/* comparand result */ + ); +} +#else static inline int rte_atomic128_cmp_exchange(rte_int128_t *dst, rte_int128_t *exp,
@@ -212,5 +229,6 @@ rte_atomic128_cmp_exchange(rte_int128_t *dst,return res; } +#endif /* RTE_TOOLCHAIN_MSVC */ #endif /* _RTE_ATOMIC_X86_64_H_ */
Signature · 1 line
2.47.0.vfs.0.3
Re: [PATCH 1/3] lib/eal: add rte_atomic128_cmp_exchange compatible with MSVC
Quoted 30 lines · Andre Muezerie
MSVC does not support inline assembly, which is used by the implementation of
rte_atomic128_cmp_exchangeand is needed by lib/stack.Error printed by MSVC:
stack_rte_stack_lf.c.obj : error LNK2019:
unresolved external symbolrte_atomic128_cmp_exchangereferenced in function__rte_stack_lf_push_elemsFix is to provide an implementation for
rte_atomic128_cmp_exchangewhich uses an intrinsic function, which is used when compiling with MSVC. For other compilers the existing implementation continues to be used.Signed-off-by Andre Muezerie <[email protected]>--- lib/eal/x86/include/rte_atomic.h | 4 ++-- lib/eal/x86/include/rte_atomic_64.h | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-)lib/eal/x86/include/rte_atomic.h +0
-0@@ -288,12 +288,12 @@ static inline int rte_atomic32_dec_and_test(rte_atomic32_t *v)#endif
lib/eal/x86/include/rte_atomic.h static inline int rte_atomic32_dec_and_test(rte_atomic32_t *v) · line 298
Quoted 10 lines
lib/eal/x86/include/rte_atomic.h +2
-2+#endif /* RTE_TOOLCHAIN_MSVC */ +#ifdef RTE_ARCH_I686 #include "rte_atomic_32.h" #else #include "rte_atomic_64.h" #endif -#endif -
This partially reverts 27da6a123414 ("eal: hide legacy atomics API for MSVC"). It would be better to implement an equivalent to rte_atomic128_cmp_exchange in the DPDK "new" stdatomic API (rte_stdatomic.h).
Signature · 1 line
David Marchand
Re: [PATCH 1/3] lib/eal: add rte_atomic128_cmp_exchange compatible with MSVC
Quoted 40 lines · David Marchand
Quoted 38 lines · Andre Muezerie
MSVC does not support inline assembly, which is used by the implementation of
rte_atomic128_cmp_exchangeand is needed by lib/stack.Error printed by MSVC:
stack_rte_stack_lf.c.obj : error LNK2019:
unresolved external symbolrte_atomic128_cmp_exchangereferenced in function__rte_stack_lf_push_elemsFix is to provide an implementation for
rte_atomic128_cmp_exchangewhich uses an intrinsic function, which is used when compiling with MSVC. For other compilers the existing implementation continues to be used.Signed-off-by Andre Muezerie <[email protected]>--- lib/eal/x86/include/rte_atomic.h | 4 ++-- lib/eal/x86/include/rte_atomic_64.h | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-)lib/eal/x86/include/rte_atomic.h +2
-0@@ -288,12 +288,12 @@ static inline int rte_atomic32_dec_and_test(rte_atomic32_t *v)#endif +#endif /* RTE_TOOLCHAIN_MSVC */ +#ifdef RTE_ARCH_I686 #include "rte_atomic_32.h" #else #include "rte_atomic_64.h" #endif
Quoted 11 lines
lib/eal/x86/include/rte_atomic.h static inline int rte_atomic32_dec_and_test(rte_atomic32_t *v) · line 298
Quoted 2 lines
lib/eal/x86/include/rte_atomic.h +0
-2-#endif -This partially reverts
27da6a123414("eal: hide legacy atomics API for MSVC"). It would be better to implement an equivalent torte_atomic128_cmp_exchangein the DPDK "new" stdatomic API (rte_stdatomic.h).Signature · 1 line
David Marchand
Thanks for calling that out. After looking at the past commits I got a better understanding of the reasons the atomic-related code is laid out the way it is, and I agree that we should attempt to follow the same guidelines.
For that reason I changed the approach taken in the v2 I sent out today.
Let me know your thoughts.
--
Andre Muezerie
PATCH 2/3 app/test: add basic test for rte_atomic128_cmp_exchange
In reply to Andre MuezerieA basic test for rte_atomic128_cmp_exchange that can also be compiled with MSVC and run on Windows is being added.
This is relevant as rte_atomic128_cmp_exchange uses a different implementation when compiled with MSVC and the existing tests for this function are not compatible with MSVC.
--- app/test/test_atomic.c | 59 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)
app/test/test_atomic.c +59 -0
@@ -20,6 +20,7 @@#include "test.h" +#ifndef RTE_TOOLCHAIN_MSVC /* * Atomic Variables * ================
@@ -441,9 +442,41 @@ test_atomic_exchange(__rte_unused void *arg) return 0; } +#endif /* RTE_TOOLCHAIN_MSVC */ + +#if defined(RTE_ARCH_X86_64 ) || defined(RTE_ARCH_ARM64 ) +static rte_int128_t big_int; + +/* +* This function atomically performs: +* big_int.val[1] += big_int.val[0] +* big_int.val[0] += 1 +*/ +static void +test_atomic_128_bit_compare_and_swap_basic_test (void ) +{ +rte_int128_t comparand = big_int; + +rte_int128_t src; + src.val[0 ] = big_int.val[0 ] +1 ; + src.val[1 ] = big_int.val[0 ] + big_int.val[1 ]; + +do { + ;/* nothing */ + }while (rte_atomic128_cmp_exchange (&big_int, + &comparand, + &src, +1 , +0 , +0 + )); +} +#endif +static int test_atomic (void ) { +#ifndef RTE_TOOLCHAIN_MSVC rte_atomic16_init (&a16);rte_atomic32_init (&a32);rte_atomic64_init (&a64);
@@ -628,6 +661,32 @@ test_atomic(void)printf ("Atomic exchange test failed\n" ); return -1; } +#endif /* RTE_TOOLCHAIN_MSVC */ + +#if defined(RTE_ARCH_X86_64 ) || defined(RTE_ARCH_ARM64 ) +/* +* This is a basic test case for rte_atomic128_cmp_exchange. +* On MSVC this test provides the confirmation that +* rte_atomic128_cmp_exchange passes the parameters correctly +* to the underlying intrinsic function responsible for the +* operation. +* +* The test atomically performs: +* big_int.val[1] += big_int.val[0] +* big_int.val[0] += 1 +*/ +printf ("128-bit compare and swap basic test\n" ); + + big_int.val[1 ] =23 ;/* should become 34 */ + big_int.val[0 ] =11 ;/* should become 12 */ + +test_atomic_128_bit_compare_and_swap_basic_test (); + +if (big_int.val[1 ] !=34 || big_int.val[0 ] !=12 ) { +printf ("128-bit compare and swap basic test failed\n" ); +return -1 ; + } +#endif return 0 ; }
Signature · 1 line
2.47.0.vfs.0.3
PATCH 3/3 lib/stack: enable build with MSVC
In reply to Andre MuezerieNow that the issues preventing the code needed to build lib/stack have been addressed, it can be enabled so that it also gets built when using the MSVC compiler.
--- lib/stack/meson.build | 6 ------ 1 file changed, 6 deletions(-)
lib/stack/meson.build +0 -6
@@ -1,12 +1,6 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2019 Intel Corporation -if is_ms_compiler - build = false - reason = 'not supported building with Visual Studio Toolset' - subdir_done() -endif - sources = files('rte_stack.c', 'rte_stack_std.c', 'rte_stack_lf.c') headers = files('rte_stack.h') # subheaders, not for direct inclusion by apps
Signature · 1 line
2.47.0.vfs.0.3
v2 cover enable build of lib/stack when using MSVC
In reply to Andre MuezerieMSVC does not support inline assembly, which is used by the implementation of rte_atomic128_cmp_exchange and is needed by the C11 flavor of lib/stack.
A special implementation of rte_atomic128_cmp_exchange compatible with MSVC is added to rte_stack_lf_c11.h. It uses an intrinsic function when using MSVC, and inline assembly when other compilers are used.
Existing atomic tests (which are not C11 compatible) are now skipped when using MSVC.
v2:
* Added MSVC compatible implementation of rte_atomic128_cmp_exchange
* Skipped non-C11 atomic tests when using MSVC
Andre Muezerie (2):
test: disable non-C11 atomic tests for MSVC
stack: enable build with MSVC
app/test/test_atomic.c | 2 ++ lib/stack/meson.build | 6 ---- lib/stack/rte_stack_lf_c11.h | 62 ++++++++++++++++++++++++++++++++++-- 3 files changed, 62 insertions(+), 8 deletions(-)
--
2.47.2.vfs.0.1
PATCH v2 1/2 test: disable non-C11 atomic tests for MSVC
In general, non-C11 atomics are not to be used with MSVC. This patch skips the non-C11 atomic tests when using MSVC.
--- app/test/test_atomic.c | 2 ++ 1 file changed, 2 insertions(+)
app/test/test_atomic.c +2 -0
@@ -20,6 +20,7 @@#include "test.h" +#ifndef RTE_TOOLCHAIN_MSVC /* * Atomic Variables * ================
@@ -632,3 +633,4 @@ test_atomic(void) return 0; }REGISTER_FAST_TEST (atomic_autotest, false, true, test_atomic); +#endif /* RTE_TOOLCHAIN_MSVC */
Signature · 1 line
2.47.2.vfs.0.1
PATCH v2 2/2 stack: enable build with MSVC
In reply to Andre MuezerieAn implementation compatible with MSVC is provided for atomic128_cmp_exchange in rte_stack_lf_c11.h.
Now that the issues preventing the code needed to build lib/stack have been addressed, it can be enabled so that it also gets built when using the MSVC compiler.
--- lib/stack/meson.build | 6 ---- lib/stack/rte_stack_lf_c11.h | 62 ++++++++++++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 8 deletions(-)
lib/stack/meson.build +0 -6
@@ -1,12 +1,6 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2019 Intel Corporation -if is_ms_compiler - build = false - reason = 'not supported building with Visual Studio Toolset' - subdir_done() -endif - sources = files('rte_stack.c', 'rte_stack_std.c', 'rte_stack_lf.c') headers = files('rte_stack.h') # subheaders, not for direct inclusion by apps
lib/stack/rte_stack_lf_c11.h +60 -2
80 lines not on this page · raw message
Signature · 1 line
2.47.2.vfs.0.1
v3 cover enable build of lib/stack when using MSVC
In reply to Andre MuezerieMSVC does not support inline assembly, which is used by the implementation of rte_atomic128_cmp_exchange and is needed by the C11 flavor of lib/stack.
A special implementation of rte_atomic128_cmp_exchange compatible with MSVC is added to rte_stack_lf_c11.h. It uses an intrinsic function when using MSVC, and inline assembly when other compilers are used.
Existing atomic tests (which are not C11 compatible) are now skipped when using MSVC.
v3:
* Isolated the additional implementation of rte_atomic128_cmp_exchange to MSVC only.
v2:
* Added MSVC compatible implementation of rte_atomic128_cmp_exchange
* Skipped non-C11 atomic tests when using MSVC
Andre Muezerie (2):
test: disable non-C11 atomic tests for MSVC
stack: enable build with MSVC
app/test/test_atomic.c | 2 ++ lib/stack/meson.build | 6 ------ lib/stack/rte_stack_lf_c11.h | 27 +++++++++++++++++++++++++++ 3 files changed, 29 insertions(+), 6 deletions(-)
--
2.47.2.vfs.0.1
PATCH v3 1/2 test: disable non-C11 atomic tests for MSVC
In general, non-C11 atomics are not to be used with MSVC. This patch skips the non-C11 atomic tests when using MSVC.
--- app/test/test_atomic.c | 2 ++ 1 file changed, 2 insertions(+)
app/test/test_atomic.c +2 -0
@@ -20,6 +20,7 @@#include "test.h" +#ifndef RTE_TOOLCHAIN_MSVC /* * Atomic Variables * ================
@@ -632,3 +633,4 @@ test_atomic(void) return 0; }REGISTER_FAST_TEST (atomic_autotest, false, true, test_atomic); +#endif /* RTE_TOOLCHAIN_MSVC */
Signature · 1 line
2.47.2.vfs.0.1
PATCH v3 2/2 stack: enable build with MSVC
In reply to Andre MuezerieAn implementation compatible with MSVC is provided for atomic128_cmp_exchange in rte_stack_lf_c11.h.
Now that the issues preventing the code needed to build lib/stack have been addressed, it can be enabled so that it also gets built when using the MSVC compiler.
--- lib/stack/meson.build | 6 ------ lib/stack/rte_stack_lf_c11.h | 27 +++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 6 deletions(-)
lib/stack/meson.build +0 -6
@@ -1,12 +1,6 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2019 Intel Corporation -if is_ms_compiler - build = false - reason = 'not supported building with Visual Studio Toolset' - subdir_done() -endif - sources = files('rte_stack.c', 'rte_stack_std.c', 'rte_stack_lf.c') headers = files('rte_stack.h') # subheaders, not for direct inclusion by apps
lib/stack/rte_stack_lf_c11.h +27 -0
@@ -8,6 +8,33 @@#include <rte_branch_prediction.h> #include <rte_prefetch.h> +#ifdef RTE_TOOLCHAIN_MSVC +/** +* The maximum lock-free data size that can be manipulated atomically using C11 +* standard is limited to 8 bytes. +* +* This implementation for rte_atomic128_cmp_exchange operates on 16-byte +* data types and is made available here so that it can be used without the +* need to unnecessarily expose other non-C11 atomics present in +* rte_atomic_64.h when using MSVC. +*/ +static inline int +rte_atomic128_cmp_exchange (rte_int128_t *dst, +rte_int128_t *exp, +const rte_int128_t *src, +unsigned int weak, +int success, +int failure) +{ +return (int )_InterlockedCompareExchange128 ( + (int64_t volatile *) dst, + src->val[1 ],/* exchange high */ + src->val[0 ],/* exchange low */ + (int64_t *) exp/* comparand result */ + ); +} +#endif /* RTE_TOOLCHAIN_MSVC */ + static __rte_always_inline unsigned int__rte_stack_lf_count (struct rte_stack *s) {
Signature · 1 line
2.47.2.vfs.0.1
Re: [PATCH v3 2/2] stack: enable build with MSVC
Quoted 55 lines · Andre Muezerie
An implementation compatible with MSVC is provided for
atomic128_cmp_exchangeinrte_stack_lf_c11.h.Now that the issues preventing the code needed to build lib/stack have been addressed, it can be enabled so that it also gets built when using the MSVC compiler.
Signed-off-by Andre Muezerie <[email protected]>--- lib/stack/meson.build | 6 ------ lib/stack/rte_stack_lf_c11.h | 27 +++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 6 deletions(-)lib/stack/meson.build +0
-6@@ -1,12 +1,6 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2019 Intel Corporation -if is_ms_compiler - build = false - reason = 'not supported building with Visual Studio Toolset' - subdir_done() -endif - sources = files('rte_stack.c', 'rte_stack_std.c', 'rte_stack_lf.c') headers = files('rte_stack.h') # subheaders, not for direct inclusion by appslib/stack/rte_stack_lf_c11.h +17
-0@@ -8,6 +8,33 @@#include <rte_branch_prediction.h> #include <rte_prefetch.h> +#ifdef RTE_TOOLCHAIN_MSVC +/** +* The maximum lock-free data size that can be manipulated atomically using C11 +* standard is limited to 8 bytes. +* +* This implementation for rte_atomic128_cmp_exchange operates on 16-byte +* data types and is made available here so that it can be used without the +* need to unnecessarily expose other non-C11 atomics present in +* rte_atomic_64.h when using MSVC. +*/ +static inline int +rte_atomic128_cmp_exchange (rte_int128_t *dst, +rte_int128_t *exp, +const rte_int128_t *src, +unsigned int weak, +int success, +int failure)
lib/stack/rte_stack_lf_c11.h line 37
Quoted 10 lines
lib/stack/rte_stack_lf_c11.h +10
-0+{ +return (int )_InterlockedCompareExchange128 ( + (int64_t volatile *) dst, + src->val[1 ],/* exchange high */ + src->val[0 ],/* exchange low */ + (int64_t *) exp/* comparand result */ + ); +} +#endif /* RTE_TOOLCHAIN_MSVC */ +
Is there a particular reason for having this only in the stack library, more than more generically available?
/Bruce
Re: [PATCH v3 2/2] stack: enable build with MSVC
Quoted 70 lines · Bruce Richardson
Quoted 55 lines · Andre Muezerie
An implementation compatible with MSVC is provided for
atomic128_cmp_exchangeinrte_stack_lf_c11.h.Now that the issues preventing the code needed to build lib/stack have been addressed, it can be enabled so that it also gets built when using the MSVC compiler.
Signed-off-by Andre Muezerie <[email protected]>--- lib/stack/meson.build | 6 ------ lib/stack/rte_stack_lf_c11.h | 27 +++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 6 deletions(-)lib/stack/meson.build +0
-6@@ -1,12 +1,6 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2019 Intel Corporation -if is_ms_compiler - build = false - reason = 'not supported building with Visual Studio Toolset' - subdir_done() -endif - sources = files('rte_stack.c', 'rte_stack_std.c', 'rte_stack_lf.c') headers = files('rte_stack.h') # subheaders, not for direct inclusion by appslib/stack/rte_stack_lf_c11.h +17
-0@@ -8,6 +8,33 @@#include <rte_branch_prediction.h> #include <rte_prefetch.h> +#ifdef RTE_TOOLCHAIN_MSVC +/** +* The maximum lock-free data size that can be manipulated atomically using C11 +* standard is limited to 8 bytes. +* +* This implementation for rte_atomic128_cmp_exchange operates on 16-byte +* data types and is made available here so that it can be used without the +* need to unnecessarily expose other non-C11 atomics present in +* rte_atomic_64.h when using MSVC. +*/ +static inline int +rte_atomic128_cmp_exchange (rte_int128_t *dst, +rte_int128_t *exp, +const rte_int128_t *src, +unsigned int weak, +int success, +int failure)lib/stack/rte_stack_lf_c11.h line 37
Quoted 10 lines
lib/stack/rte_stack_lf_c11.h +10
-0+{ +return (int )_InterlockedCompareExchange128 ( + (int64_t volatile *) dst, + src->val[1 ],/* exchange high */ + src->val[0 ],/* exchange low */ + (int64_t *) exp/* comparand result */ + ); +} +#endif /* RTE_TOOLCHAIN_MSVC */ +Is there a particular reason for having this only in the stack library, more than more generically available?
Quoted 1 line
/Bruce
The PR history shows that the intention with MSVC (which is a new toolchain/platform) was to block visibility of the rte_atomic APIs from day 1.
More details can be seen here:
https://github.com/JoverZhang/dpdk/pull/21/commits/27da6a1234148fef43687d324ab2221df0631b5c
If there's a strong need we could revert that decision, but for now it seems it would be better to follow the established direction.
--
Andre Muezerie
Re: [PATCH v3 2/2] stack: enable build with MSVC
In reply to Andre MuezerieRecheck-request: iol-intel-Performance
Triggering a retest in CI for the failed performance result.
Quoted 21 lines · Andre Muezerie
MSVC does not support inline assembly, which is used by the implementation of
rte_atomic128_cmp_exchangeand is needed by the C11 flavor of lib/stack.A special implementation of
rte_atomic128_cmp_exchangecompatible with MSVC is added torte_stack_lf_c11.h. It uses an intrinsic function when using MSVC, and inline assembly when other compilers are used.Existing atomic tests (which are not C11 compatible) are now skipped when using MSVC.
v3:
* Isolated the additional implementation ofrte_atomic128_cmp_exchangeto MSVC only.v2:
* Added MSVC compatible implementation ofrte_atomic128_cmp_exchange
* Skipped non-C11 atomic tests when using MSVC
Quoted 8 lines
Andre Muezerie (2):
test: disable non-C11 atomic tests for MSVC
stack: enable build with MSVCapp/test/test_atomic.c | 2 ++ lib/stack/meson.build | 6 ------ lib/stack/rte_stack_lf_c11.h | 27 +++++++++++++++++++++++++++ 3 files changed, 29 insertions(+), 6 deletions(-)
On the principle, the stack library should be reworked not to rely on the legacy API.
But on the other hand, the stack library is the only consumer of this legacy API, and the change is small.
So I decided to take this series for now.
We can revisit when there are more users of 128 bits atomics.
Series applied, thanks.
Signature · 1 line
David Marchand