[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

[PATCH v3 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.

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

[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 v3 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 | 27 +++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 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
diff --git a/lib/stack/rte_stack_lf_c11.h b/lib/stack/rte_stack_lf_c11.h
index 60d46e963b..898d43edb2 100644
--- a/lib/stack/rte_stack_lf_c11.h
+++ b/lib/stack/rte_stack_lf_c11.h
@@ -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)
 {
-- 
2.47.2.vfs.0.1

Re: [PATCH v3 2/2] stack: enable build with MSVC

[Bruce Richardson]

On Mon, Feb 03, 2025 at 05:58:38PM -0800, Andre Muezerie wrote:
> 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 | 27 +++++++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 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
> diff --git a/lib/stack/rte_stack_lf_c11.h b/lib/stack/rte_stack_lf_c11.h
> index 60d46e963b..898d43edb2 100644
> --- a/lib/stack/rte_stack_lf_c11.h
> +++ b/lib/stack/rte_stack_lf_c11.h
> @@ -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 */
> +

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

[Andre Muezerie]

On Tue, Feb 04, 2025 at 10:30:03AM +0000, Bruce Richardson wrote:
> On Mon, Feb 03, 2025 at 05:58:38PM -0800, Andre Muezerie wrote:
> > 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 | 27 +++++++++++++++++++++++++++
> >  2 files changed, 27 insertions(+), 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
> > diff --git a/lib/stack/rte_stack_lf_c11.h b/lib/stack/rte_stack_lf_c11.h
> > index 60d46e963b..898d43edb2 100644
> > --- a/lib/stack/rte_stack_lf_c11.h
> > +++ b/lib/stack/rte_stack_lf_c11.h
> > @@ -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 */
> > +
> 
> Is there a particular reason for having this only in the stack library,
> more than more generically available?
> 
> /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

[Patrick Robb]

[-- Attachment #1: Type: text/plain, Size: 101 bytes --]

Recheck-request: iol-intel-Performance

Triggering a retest in CI for the failed performance result.

[-- Attachment #2: Type: text/html, Size: 508 bytes --]

Re: [PATCH v3 0/2] enable build of lib/stack when using MSVC

[David Marchand]

On Tue, Feb 4, 2025 at 2:58 AM 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 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(-)

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.


-- 
David Marchand