Re: [dpdk-dev] [EXT] [PATCH v3 1/5] eal: add the APIs to wait until equal
From: Jerin Jacob Kollanukkaran <hidden>
Date: 2019-07-24 11:53:01
quoted hunk ↗ jump to hunk
-----Original Message----- From: Gavin Hu <redacted> Sent: Tuesday, July 23, 2019 9:14 PM To: dev@dpdk.org Cc: nd@arm.com; thomas@monjalon.net; stephen@networkplumber.org; Jerin Jacob Kollanukkaran [off-list ref]; Pavan Nikhilesh Bhagavatula [off-list ref]; Honnappa.Nagarahalli@arm.com; gavin.hu@arm.com Subject: [EXT] [PATCH v3 1/5] eal: add the APIs to wait until equal The rte_wait_until_equalxx APIs abstract the functionality of 'polling for a memory location to become equal to a given value'. Signed-off-by: Gavin Hu <redacted> Reviewed-by: Ruifeng Wang <redacted> Reviewed-by: Steve Capper <redacted> Reviewed-by: Ola Liljedahl <redacted> Reviewed-by: Honnappa Nagarahalli <redacted> Acked-by: Pavan Nikhilesh <redacted> --- .../common/include/arch/arm/rte_atomic_64.h | 4 + .../common/include/arch/arm/rte_pause_64.h | 106 +++++++++++++++++++++ lib/librte_eal/common/include/generic/rte_pause.h | 39 +++++++- 3 files changed, 148 insertions(+), 1 deletion(-)diff --git a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.hb/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h index 97060e4..8d742c6 100644--- a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h +++ b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h@@ -15,8 +15,12 @@ extern "C" { #include "generic/rte_atomic.h" +#ifndef dsb #define dsb(opt) asm volatile("dsb " #opt : : : "memory") +#endif +#ifndef dmb #define dmb(opt) asm volatile("dmb " #opt : : : "memory") +#endif
Is this change required? Please fix the root cause.
I do see some build error too.
In file included from /home/jerin/dpdk.org/build/include/rte_pause_64.h:13,
from /home/jerin/dpdk.org/build/include/rte_pause.h:13,
from /home/jerin/dpdk.org/build/include/generic/rte_spinlock.h:25,
from /home/jerin/dpdk.org/build/include/rte_spinlock.h:17,
from /home/jerin/dpdk.org/drivers/bus/fslmc/mc/mc_sys.c:10:
/home/jerin/dpdk.org/build/include/generic/rte_pause.h: In function 'rte_wait_until_equal16':
/home/jerin/dpdk.org/build/include/generic/rte_pause.h:44:49: error: macro "dmb" passed 1 arguments, but takes just 0
44 | __rte_wait_until_equal(addr, expected, memorder);
Command to reproduce(gcc 9.1)
rm -rf build && unset RTE_KERNELDIR && make -j T=arm64-thunderx-linux-gcc CROSS=aarch64-linux-gnu- && sed -ri 's,(CONFIG_RTE_KNI_KMOD=)y,\1n,' build/.config && sed -ri 's,(CONFIG_RTE_LIBRTE_VHOST_NUMA=)y,\1n,' build/.config && sed -ri 's,(CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=)y,\1n,' build/.config && sed -ri 's,(CONFIG_RTE_EAL_IGB_UIO=)y,\1n,' build/.config && CC="ccache gcc" make -j test-build CROSS=aarch64-linux-gnu-
quoted hunk ↗ jump to hunk
#define rte_mb() dsb(sy)diff --git a/lib/librte_eal/common/include/arch/arm/rte_pause_64.hb/lib/librte_eal/common/include/arch/arm/rte_pause_64.h index 93895d3..1f7be0a 100644--- a/lib/librte_eal/common/include/arch/arm/rte_pause_64.h +++ b/lib/librte_eal/common/include/arch/arm/rte_pause_64.h@@ -17,6 +17,112 @@ static inline void rte_pause(void) asm volatile("yield" ::: "memory"); } +#ifdef RTE_USE_WFE
Do we need it to be under RTE_USE_WFE? If there is no harm, no need to add Conditional compilation to detect build errors, especially config is disabled by default.
+/* Wait for *addr to be updated with expected value */ static
+__rte_always_inline void rte_wait_until_equal16(volatile uint16_t
+*addr, uint16_t expected, int memorder) {
+ uint16_t tmp;
+ if (memorder == __ATOMIC_RELAXED)
+ asm volatile(
+ "ldxrh %w[tmp], %w[addr]\n"
+ "cmp %w[tmp], %w[expected]\n"
+ "b.eq 2f\n"
+ "sevl\n"
+ "1: wfe\n"
+ "ldxrh %w[tmp], %w[addr]\n"
+ "cmp %w[tmp], %w[expected]\n"
+ "bne 1b\n"
+ "2:\n"
+ : [tmp] "=&r" (tmp)
+ : [addr] "Q"(*addr), [expected] "r"(expected)
+ : "cc", "memory");
+ else
+ asm volatile(
+ "ldaxrh %w[tmp], %w[addr]\n"
+ "cmp %w[tmp], %w[expected]\n"
+ "b.eq 2f\n"
+ "sevl\n"
+ "1: wfe\n"
+ "ldaxrh %w[tmp], %w[addr]\n"
+ "cmp %w[tmp], %w[expected]\n"
+ "bne 1b\n"
+ "2:\n"
+ : [tmp] "=&r" (tmp)
+ : [addr] "Q"(*addr), [expected] "r"(expected)
+ : "cc", "memory");
+}
+
+static __rte_always_inline void
+rte_wait_until_equal32(volatile uint32_t *addr, uint32_t expected, int
+memorder) {
+ uint32_t tmp;
+ if (memorder == __ATOMIC_RELAXED)
+ asm volatile(
+ "ldxr %w[tmp], %w[addr]\n"
+ "cmp %w[tmp], %w[expected]\n"
+ "b.eq 2f\n"
+ "sevl\n"
+ "1: wfe\n"
+ "ldxr %w[tmp], %w[addr]\n"
+ "cmp %w[tmp], %w[expected]\n"
+ "bne 1b\n"
+ "2:\n"
+ : [tmp] "=&r" (tmp)
+ : [addr] "Q"(*addr), [expected] "r"(expected)
+ : "cc", "memory");
+ else
+ asm volatile(
+ "ldaxr %w[tmp], %w[addr]\n"
+ "cmp %w[tmp], %w[expected]\n"
+ "b.eq 2f\n"
+ "sevl\n"
+ "1: wfe\n"
+ "ldaxr %w[tmp], %w[addr]\n"
+ "cmp %w[tmp], %w[expected]\n"
+ "bne 1b\n"
+ "2:\n"
+ : [tmp] "=&r" (tmp)
+ : [addr] "Q"(*addr), [expected] "r"(expected)
+ : "cc", "memory");
+}
+
+static __rte_always_inline void
+rte_wait_until_equal64(volatile uint64_t *addr, uint64_t expected, int
+memorder) {
+ uint64_t tmp;
+ if (memorder == __ATOMIC_RELAXED)
+ asm volatile(
+ "ldxr %x[tmp], %x[addr]\n"
+ "cmp %x[tmp], %x[expected]\n"
+ "b.eq 2f\n"
+ "sevl\n"
+ "1: wfe\n"
+ "ldxr %x[tmp], %x[addr]\n"
+ "cmp %x[tmp], %x[expected]\n"
+ "bne 1b\n"
+ "2:\n"
+ : [tmp] "=&r" (tmp)
+ : [addr] "Q"(*addr), [expected] "r"(expected)
+ : "cc", "memory");
+ else
+ asm volatile(
+ "ldaxr %x[tmp], %x[addr]\n"
+ "cmp %x[tmp], %x[expected]\n"
+ "b.eq 2f\n"
+ "sevl\n"
+ "1: wfe\n"
+ "ldaxr %x[tmp], %x[addr]\n"
+ "cmp %x[tmp], %x[expected]\n"
+ "bne 1b\n"
+ "2:\n"
+ : [tmp] "=&r" (tmp)
+ : [addr] "Q"(*addr), [expected] "r"(expected)
+ : "cc", "memory");Duplication of code. Please introduce a macro for assembly Skelton. Something like http://patches.dpdk.org/patch/56949/
quoted hunk ↗ jump to hunk
+} + +#endif + #ifdef __cplusplus } #endifdiff --git a/lib/librte_eal/common/include/generic/rte_pause.hb/lib/librte_eal/common/include/generic/rte_pause.h index 52bd4db..8f5f025 100644--- a/lib/librte_eal/common/include/generic/rte_pause.h +++ b/lib/librte_eal/common/include/generic/rte_pause.h@@ -4,7 +4,6 @@ #ifndef _RTE_PAUSE_H_ #define _RTE_PAUSE_H_ - /** * @file *@@ -12,6 +11,10 @@ * */ +#include <stdint.h> +#include <rte_common.h> +#include <rte_atomic.h> + /** * Pause CPU execution for a short while *@@ -20,4 +23,38 @@ */ static inline void rte_pause(void); +#if !defined(RTE_USE_WFE) +#ifdef RTE_USE_C11_MEM_MODEL +#define __rte_wait_until_equal(addr, expected, memorder) do {\ + while (__atomic_load_n(addr, memorder) != expected) \ + rte_pause();\ +} while (0) +#else +#define __rte_wait_until_equal(addr, expected, memorder) do {\ + while (*addr != expected)\ + rte_pause();\ + if (memorder != __ATOMIC_RELAXED)\ + rte_smp_rmb();\
Is this correct wrt all memorder? If there is no specific gain on no C11 mem model, let have only C11 memmodel Aka remove RTE_USE_C11_MEM_MODEL
+} while (0) +#endif +
Spotted public API. Lets have prototype with very good documentation on the API details.
+static __rte_always_inline void
+rte_wait_until_equal16(volatile uint16_t *addr, uint16_t expected, int
+memorder) {
+ __rte_wait_until_equal(addr, expected, memorder); }
+
+static __rte_always_inline void
+rte_wait_until_equal32(volatile uint32_t *addr, uint32_t expected, int
+memorder) {
+ __rte_wait_until_equal(addr, expected, memorder); }
+
+static __rte_always_inline void
+rte_wait_until_equal64(volatile uint64_t *addr, uint64_t expected, int
+memorder) {
+ __rte_wait_until_equal(addr, expected, memorder); } #endif /*
+RTE_USE_WFE */
+
#endif /* _RTE_PAUSE_H_ */
--
2.7.4