[v4,2/6] eal: add the APIs to wait until equal

Message ID 1566454356-37277-3-git-send-email-gavin.hu@arm.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series use WFE for locks and ring on aarch64 |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Gavin Hu Aug. 22, 2019, 6:12 a.m. UTC
  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 <gavin.hu@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Steve Capper <steve.capper@arm.com>
Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Acked-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 .../common/include/arch/arm/rte_pause_64.h         | 30 ++++++++++++++++++++++
 lib/librte_eal/common/include/generic/rte_pause.h  | 26 ++++++++++++++++++-
 2 files changed, 55 insertions(+), 1 deletion(-)
  

Comments

Jerin Jacob Sept. 11, 2019, 12:26 p.m. UTC | #1
On Thu, Aug 22, 2019 at 11:43 AM Gavin Hu <gavin.hu@arm.com> wrote:
>
> 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 <gavin.hu@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Steve Capper <steve.capper@arm.com>
> Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Acked-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> ---
>  .../common/include/arch/arm/rte_pause_64.h         | 30 ++++++++++++++++++++++
>  lib/librte_eal/common/include/generic/rte_pause.h  | 26 ++++++++++++++++++-
>  2 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/common/include/arch/arm/rte_pause_64.h b/lib/librte_eal/common/include/arch/arm/rte_pause_64.h
> index 93895d3..dabde17 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
> @@ -1,5 +1,6 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
>   * Copyright(c) 2017 Cavium, Inc
> + * Copyright(c) 2019 Arm Limited
>   */
>
>  #ifndef _RTE_PAUSE_ARM64_H_
> @@ -17,6 +18,35 @@ static inline void rte_pause(void)
>         asm volatile("yield" ::: "memory");
>  }
>
> +#ifdef RTE_ARM_USE_WFE
> +#define __WAIT_UNTIL_EQUAL(name, asm_op, wide, type) \
> +static __rte_always_inline void \
> +rte_wait_until_equal_##name(volatile type * addr, type expected) \
> +{ \
> +       type tmp; \
> +       asm volatile( \
> +               #asm_op " %" #wide "[tmp], %[addr]\n" \
> +               "cmp    %" #wide "[tmp], %" #wide "[expected]\n" \
> +               "b.eq   2f\n" \
> +               "sevl\n" \
> +               "1:     wfe\n" \
> +               #asm_op " %" #wide "[tmp], %[addr]\n" \
> +               "cmp    %" #wide "[tmp], %" #wide "[expected]\n" \
> +               "bne    1b\n" \
> +               "2:\n" \
> +               : [tmp] "=&r" (tmp) \
> +               : [addr] "Q"(*addr), [expected] "r"(expected) \
> +               : "cc", "memory"); \
> +}
>
> +/* Wait for *addr to be updated with expected value */
> +__WAIT_UNTIL_EQUAL(relaxed_16, ldxrh, w, uint16_t)
> +__WAIT_UNTIL_EQUAL(acquire_16, ldaxrh, w, uint16_t)
> +__WAIT_UNTIL_EQUAL(relaxed_32, ldxr, w, uint32_t)
> +__WAIT_UNTIL_EQUAL(acquire_32, ldaxr, w, uint32_t)
> +__WAIT_UNTIL_EQUAL(relaxed_64, ldxr, x, uint64_t)
> +__WAIT_UNTIL_EQUAL(acquire_64, ldaxr, x, uint64_t)
>

This scheme doesn't allow to write Doxygen comments for the API
Please change to some scheme where you can Doxygen comments for each API
without code duplication. Something like

/**
 * Doxygen comment
 */
rte_wait_until_equal_relaxed_16(..)
{
        __WAIT_UNTIL_EQUAL(..)
}
  
Gavin Hu Sept. 12, 2019, 8:25 a.m. UTC | #2
Hi Jerin,

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, September 11, 2019 8:27 PM
> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> Cc: dev@dpdk.org; nd <nd@arm.com>; thomas@monjalon.net;
> stephen@networkplumber.org; jerinj@marvell.com; Pavan Nikhilesh
> <pbhagavatula@marvell.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v4 2/6] eal: add the APIs to wait until equal
> 
> On Thu, Aug 22, 2019 at 11:43 AM Gavin Hu <gavin.hu@arm.com> wrote:
> >
> > 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 <gavin.hu@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > Reviewed-by: Steve Capper <steve.capper@arm.com>
> > Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > Acked-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > ---
> >  .../common/include/arch/arm/rte_pause_64.h         | 30
> ++++++++++++++++++++++
> >  lib/librte_eal/common/include/generic/rte_pause.h  | 26
> ++++++++++++++++++-
> >  2 files changed, 55 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_eal/common/include/arch/arm/rte_pause_64.h
> b/lib/librte_eal/common/include/arch/arm/rte_pause_64.h
> > index 93895d3..dabde17 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
> > @@ -1,5 +1,6 @@
> >  /* SPDX-License-Identifier: BSD-3-Clause
> >   * Copyright(c) 2017 Cavium, Inc
> > + * Copyright(c) 2019 Arm Limited
> >   */
> >
> >  #ifndef _RTE_PAUSE_ARM64_H_
> > @@ -17,6 +18,35 @@ static inline void rte_pause(void)
> >         asm volatile("yield" ::: "memory");
> >  }
> >
> > +#ifdef RTE_ARM_USE_WFE
> > +#define __WAIT_UNTIL_EQUAL(name, asm_op, wide, type) \
> > +static __rte_always_inline void \
> > +rte_wait_until_equal_##name(volatile type * addr, type expected) \
> > +{ \
> > +       type tmp; \
> > +       asm volatile( \
> > +               #asm_op " %" #wide "[tmp], %[addr]\n" \
> > +               "cmp    %" #wide "[tmp], %" #wide "[expected]\n" \
> > +               "b.eq   2f\n" \
> > +               "sevl\n" \
> > +               "1:     wfe\n" \
> > +               #asm_op " %" #wide "[tmp], %[addr]\n" \
> > +               "cmp    %" #wide "[tmp], %" #wide "[expected]\n" \
> > +               "bne    1b\n" \
> > +               "2:\n" \
> > +               : [tmp] "=&r" (tmp) \
> > +               : [addr] "Q"(*addr), [expected] "r"(expected) \
> > +               : "cc", "memory"); \
> > +}
> >
> > +/* Wait for *addr to be updated with expected value */
> > +__WAIT_UNTIL_EQUAL(relaxed_16, ldxrh, w, uint16_t)
> > +__WAIT_UNTIL_EQUAL(acquire_16, ldaxrh, w, uint16_t)
> > +__WAIT_UNTIL_EQUAL(relaxed_32, ldxr, w, uint32_t)
> > +__WAIT_UNTIL_EQUAL(acquire_32, ldaxr, w, uint32_t)
> > +__WAIT_UNTIL_EQUAL(relaxed_64, ldxr, x, uint64_t)
> > +__WAIT_UNTIL_EQUAL(acquire_64, ldaxr, x, uint64_t)
> >
> 
> This scheme doesn't allow to write Doxygen comments for the API
> Please change to some scheme where you can Doxygen comments for each
> API
> without code duplication. Something like
Thanks for pointing out this, I will fix this in next version. 
> 
> /**
>  * Doxygen comment
>  */
> rte_wait_until_equal_relaxed_16(..)
> {
>         __WAIT_UNTIL_EQUAL(..)
> }
Following the other examples, just add some declarations of the APIs in the beginning of the file, with the Doxygen comments above can fix this problem.
The implementations of the APIs do not need to change, please help review the v5 version.
  

Patch

diff --git a/lib/librte_eal/common/include/arch/arm/rte_pause_64.h b/lib/librte_eal/common/include/arch/arm/rte_pause_64.h
index 93895d3..dabde17 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
@@ -1,5 +1,6 @@ 
 /* SPDX-License-Identifier: BSD-3-Clause
  * Copyright(c) 2017 Cavium, Inc
+ * Copyright(c) 2019 Arm Limited
  */
 
 #ifndef _RTE_PAUSE_ARM64_H_
@@ -17,6 +18,35 @@  static inline void rte_pause(void)
 	asm volatile("yield" ::: "memory");
 }
 
+#ifdef RTE_ARM_USE_WFE
+#define __WAIT_UNTIL_EQUAL(name, asm_op, wide, type) \
+static __rte_always_inline void \
+rte_wait_until_equal_##name(volatile type * addr, type expected) \
+{ \
+	type tmp; \
+	asm volatile( \
+		#asm_op " %" #wide "[tmp], %[addr]\n" \
+		"cmp	%" #wide "[tmp], %" #wide "[expected]\n" \
+		"b.eq	2f\n" \
+		"sevl\n" \
+		"1:	wfe\n" \
+		#asm_op " %" #wide "[tmp], %[addr]\n" \
+		"cmp	%" #wide "[tmp], %" #wide "[expected]\n" \
+		"bne	1b\n" \
+		"2:\n" \
+		: [tmp] "=&r" (tmp) \
+		: [addr] "Q"(*addr), [expected] "r"(expected) \
+		: "cc", "memory"); \
+}
+/* Wait for *addr to be updated with expected value */
+__WAIT_UNTIL_EQUAL(relaxed_16, ldxrh, w, uint16_t)
+__WAIT_UNTIL_EQUAL(acquire_16, ldaxrh, w, uint16_t)
+__WAIT_UNTIL_EQUAL(relaxed_32, ldxr, w, uint32_t)
+__WAIT_UNTIL_EQUAL(acquire_32, ldaxr, w, uint32_t)
+__WAIT_UNTIL_EQUAL(relaxed_64, ldxr, x, uint64_t)
+__WAIT_UNTIL_EQUAL(acquire_64, ldaxr, x, uint64_t)
+#endif
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/common/include/generic/rte_pause.h b/lib/librte_eal/common/include/generic/rte_pause.h
index 52bd4db..4741f8a 100644
--- a/lib/librte_eal/common/include/generic/rte_pause.h
+++ b/lib/librte_eal/common/include/generic/rte_pause.h
@@ -1,10 +1,10 @@ 
 /* SPDX-License-Identifier: BSD-3-Clause
  * Copyright(c) 2017 Cavium, Inc
+ * Copyright(c) 2019 Arm Limited
  */
 
 #ifndef _RTE_PAUSE_H_
 #define _RTE_PAUSE_H_
-
 /**
  * @file
  *
@@ -12,6 +12,10 @@ 
  *
  */
 
+#include <stdint.h>
+#include <rte_common.h>
+#include <rte_atomic.h>
+
 /**
  * Pause CPU execution for a short while
  *
@@ -20,4 +24,24 @@ 
  */
 static inline void rte_pause(void);
 
+#if !defined(RTE_ARM_USE_WFE)
+#define __WAIT_UNTIL_EQUAL(op_name, size, type, memorder) \
+__rte_always_inline \
+static void	\
+rte_wait_until_equal_##op_name##_##size(volatile type *addr, \
+	type expected) \
+{ \
+	while (__atomic_load_n(addr, memorder) != expected) \
+		rte_pause(); \
+}
+
+/* Wait for *addr to be updated with expected value */
+__WAIT_UNTIL_EQUAL(relaxed, 16, uint16_t, __ATOMIC_RELAXED)
+__WAIT_UNTIL_EQUAL(acquire, 16, uint16_t, __ATOMIC_ACQUIRE)
+__WAIT_UNTIL_EQUAL(relaxed, 32, uint32_t, __ATOMIC_RELAXED)
+__WAIT_UNTIL_EQUAL(acquire, 32, uint32_t, __ATOMIC_ACQUIRE)
+__WAIT_UNTIL_EQUAL(relaxed, 64, uint64_t, __ATOMIC_RELAXED)
+__WAIT_UNTIL_EQUAL(acquire, 64, uint64_t, __ATOMIC_ACQUIRE)
+#endif /* RTE_ARM_USE_WFE */
+
 #endif /* _RTE_PAUSE_H_ */