[PATCH 0/3] enable build of lib/stack when using MSVC

[PATCH 0/3] enable build of lib/stack when using MSVC

[Andre Muezerie]

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

[Andre Muezerie]

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.

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
---
 lib/eal/x86/include/rte_atomic.h    |  4 ++--
 lib/eal/x86/include/rte_atomic_64.h | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/lib/eal/x86/include/rte_atomic.h b/lib/eal/x86/include/rte_atomic.h
index c72c47c83e..e8e0e4c33c 100644
--- a/lib/eal/x86/include/rte_atomic.h
+++ b/lib/eal/x86/include/rte_atomic.h
@@ -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_ */
diff --git a/lib/eal/x86/include/rte_atomic_64.h b/lib/eal/x86/include/rte_atomic_64.h
index 0a7a2131e0..26c87a2da6 100644
--- a/lib/eal/x86/include/rte_atomic_64.h
+++ b/lib/eal/x86/include/rte_atomic_64.h
@@ -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_ */
-- 
2.47.0.vfs.0.3

Re: [PATCH 1/3] lib/eal: add rte_atomic128_cmp_exchange compatible with MSVC

[David Marchand]

On Tue, Dec 10, 2024 at 5:33 PM Andre Muezerie
<andremue@linux.microsoft.com> wrote:
>
> 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.
>
> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> ---
>  lib/eal/x86/include/rte_atomic.h    |  4 ++--
>  lib/eal/x86/include/rte_atomic_64.h | 18 ++++++++++++++++++
>  2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/lib/eal/x86/include/rte_atomic.h b/lib/eal/x86/include/rte_atomic.h
> index c72c47c83e..e8e0e4c33c 100644
> --- a/lib/eal/x86/include/rte_atomic.h
> +++ b/lib/eal/x86/include/rte_atomic.h
> @@ -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
> -

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).


-- 
David Marchand

Re: [PATCH 1/3] lib/eal: add rte_atomic128_cmp_exchange compatible with MSVC

[Andre Muezerie]

On Fri, Jan 24, 2025 at 03:27:06PM +0100, David Marchand wrote:
> On Tue, Dec 10, 2024 at 5:33 PM Andre Muezerie
> <andremue@linux.microsoft.com> wrote:
> >
> > 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.
> >
> > Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> > ---
> >  lib/eal/x86/include/rte_atomic.h    |  4 ++--
> >  lib/eal/x86/include/rte_atomic_64.h | 18 ++++++++++++++++++
> >  2 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/eal/x86/include/rte_atomic.h b/lib/eal/x86/include/rte_atomic.h
> > index c72c47c83e..e8e0e4c33c 100644
> > --- a/lib/eal/x86/include/rte_atomic.h
> > +++ b/lib/eal/x86/include/rte_atomic.h
> > @@ -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
> > -
> 
> 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).
> 
> 
> -- 
> 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

[Andre Muezerie]

A 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.

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
---
 app/test/test_atomic.c | 59 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/app/test/test_atomic.c b/app/test/test_atomic.c
index db07159e81..d3d9de0a41 100644
--- a/app/test/test_atomic.c
+++ b/app/test/test_atomic.c
@@ -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;
 }
-- 
2.47.0.vfs.0.3

[PATCH 3/3] lib/stack: enable build with MSVC

[Andre Muezerie]

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 <andremue@linux.microsoft.com>
---
 lib/stack/meson.build | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/lib/stack/meson.build b/lib/stack/meson.build
index 7631a14784..18177a742f 100644
--- a/lib/stack/meson.build
+++ b/lib/stack/meson.build
@@ -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
-- 
2.47.0.vfs.0.3

[PATCH v2 0/2] enable build of lib/stack when using MSVC

[Andre Muezerie]

MSVC 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

[Andre Muezerie]

In general, non-C11 atomics are not to be used with MSVC.
This patch skips the non-C11 atomic tests when using MSVC.

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
---
 app/test/test_atomic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/test/test_atomic.c b/app/test/test_atomic.c
index db07159e81..3f26ce88d9 100644
--- a/app/test/test_atomic.c
+++ b/app/test/test_atomic.c
@@ -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 */
-- 
2.47.2.vfs.0.1

[PATCH v2 2/2] stack: enable build with MSVC

[Andre Muezerie]

An 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.

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
---
 lib/stack/meson.build        |  6 ----
 lib/stack/rte_stack_lf_c11.h | 62 ++++++++++++++++++++++++++++++++++--
 2 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/lib/stack/meson.build b/lib/stack/meson.build
index 7631a14784..18177a742f 100644
--- a/lib/stack/meson.build
+++ b/lib/stack/meson.build
@@ -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
diff --git a/lib/stack/rte_stack_lf_c11.h b/lib/stack/rte_stack_lf_c11.h
index 60d46e963b..79d40486f9 100644
--- a/lib/stack/rte_stack_lf_c11.h
+++ b/lib/stack/rte_stack_lf_c11.h
@@ -8,6 +8,64 @@
 #include <rte_branch_prediction.h>
 #include <rte_prefetch.h>
 
+/**
+ * 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.
+ */
+#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,
+			     const rte_int128_t *src,
+			     unsigned int weak,
+			     int success,
+			     int failure)
+{
+	RTE_SET_USED(weak);
+	RTE_SET_USED(success);
+	RTE_SET_USED(failure);
+	uint8_t res;
+
+	asm volatile (
+		      MPLOCKED
+		      "cmpxchg16b %[dst];"
+		      " sete %[res]"
+		      : [dst] "=m" (dst->val[0]),
+			"=a" (exp->val[0]),
+			"=d" (exp->val[1]),
+			[res] "=r" (res)
+		      : "b" (src->val[0]),
+			"c" (src->val[1]),
+			"a" (exp->val[0]),
+			"d" (exp->val[1]),
+			"m" (dst->val[0])
+		      : "memory");
+
+	return res;
+}
+#endif /* RTE_TOOLCHAIN_MSVC */
+
 static __rte_always_inline unsigned int
 __rte_stack_lf_count(struct rte_stack *s)
 {
@@ -55,7 +113,7 @@ __rte_stack_lf_push_elems(struct rte_stack_lf_list *list,
 		/* Use the release memmodel to ensure the writes to the LF LIFO
 		 * elements are visible before the head pointer write.
 		 */
-		success = rte_atomic128_cmp_exchange(
+		success = __rte_atomic128_cmp_exchange(
 				(rte_int128_t *)&list->head,
 				(rte_int128_t *)&old_head,
 				(rte_int128_t *)&new_head,
@@ -155,7 +213,7 @@ __rte_stack_lf_pop_elems(struct rte_stack_lf_list *list,
 		 * length is visible before the head update, but
 		 * acquire semantics on the length update is enough.
 		 */
-		success = rte_atomic128_cmp_exchange(
+		success = __rte_atomic128_cmp_exchange(
 				(rte_int128_t *)&list->head,
 				(rte_int128_t *)&old_head,
 				(rte_int128_t *)&new_head,
-- 
2.47.2.vfs.0.1