[dpdk-dev] [PATCH v11 2/5] eal: add the APIs to wait until equal

[David Marchand]

On Sun, Oct 27, 2019 at 1:53 PM Gavin Hu <gavin.hu at arm.com> wrote:

[snip]

> 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..1680d7a 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

[snip]

> @@ -17,6 +23,188 @@ static inline void rte_pause(void)
>         asm volatile("yield" ::: "memory");
>  }
>
> +/**
> + * Send an event to quit WFE.
> + */
> +static inline void rte_sevl(void);
> +
> +/**
> + * Put processor into low power WFE(Wait For Event) state
> + */
> +static inline void rte_wfe(void);
> +
> +#ifdef RTE_ARM_USE_WFE
> +static inline void rte_sevl(void)
> +{
> +       asm volatile("sevl" : : : "memory");
> +}
> +
> +static inline void rte_wfe(void)
> +{
> +       asm volatile("wfe" : : : "memory");
> +}
> +#else
> +static inline void rte_sevl(void)
> +{
> +}
> +static inline void rte_wfe(void)
> +{
> +       rte_pause();
> +}
> +#endif
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice

experimental?
Just complaining on the principle, you missed the __rte_experimental
in such a case.
But this API is a no go for me, see below.


> + *
> + * Atomic exclusive load from addr, it returns the 16-bit content of *addr
> + * while making it 'monitored',when it is written by someone else, the
> + * 'monitored' state is cleared and a event is generated implicitly to exit
> + * WFE.
> + *
> + * @param addr
> + *  A pointer to the memory location.
> + * @param memorder
> + *  The valid memory order variants are __ATOMIC_ACQUIRE and __ATOMIC_RELAXED.
> + *  These map to C++11 memory orders with the same names, see the C++11 standard
> + *  the GCC wiki on atomic synchronization for detailed definitions.
> + */
> +static __rte_always_inline uint16_t
> +rte_atomic_load_ex_16(volatile uint16_t *addr, int memorder);

This API does not make sense for anything but arm, so this prefix is not good.

On arm, when RTE_ARM_USE_WFE is undefined, why would you need it?
A non exclusive load is enough since you don't want to use wfe.

[snip]

> +
> +static __rte_always_inline uint16_t
> +rte_atomic_load_ex_16(volatile uint16_t *addr, int memorder)
> +{
> +       uint16_t tmp;
> +       assert((memorder == __ATOMIC_ACQUIRE)
> +                       || (memorder == __ATOMIC_RELAXED));
> +       if (memorder == __ATOMIC_ACQUIRE)
> +               asm volatile("ldaxrh %w[tmp], [%x[addr]]"
> +                       : [tmp] "=&r" (tmp)
> +                       : [addr] "r"(addr)
> +                       : "memory");
> +       else if (memorder == __ATOMIC_RELAXED)
> +               asm volatile("ldxrh %w[tmp], [%x[addr]]"
> +                       : [tmp] "=&r" (tmp)
> +                       : [addr] "r"(addr)
> +                       : "memory");
> +       return tmp;
> +}
> +
> +static __rte_always_inline uint32_t
> +rte_atomic_load_ex_32(volatile uint32_t *addr, int memorder)
> +{
> +       uint32_t tmp;
> +       assert((memorder == __ATOMIC_ACQUIRE)
> +                       || (memorder == __ATOMIC_RELAXED));
> +       if (memorder == __ATOMIC_ACQUIRE)
> +               asm volatile("ldaxr %w[tmp], [%x[addr]]"
> +                       : [tmp] "=&r" (tmp)
> +                       : [addr] "r"(addr)
> +                       : "memory");
> +       else if (memorder == __ATOMIC_RELAXED)
> +               asm volatile("ldxr %w[tmp], [%x[addr]]"
> +                       : [tmp] "=&r" (tmp)
> +                       : [addr] "r"(addr)
> +                       : "memory");
> +       return tmp;
> +}
> +
> +static __rte_always_inline uint64_t
> +rte_atomic_load_ex_64(volatile uint64_t *addr, int memorder)
> +{
> +       uint64_t tmp;
> +       assert((memorder == __ATOMIC_ACQUIRE)
> +                       || (memorder == __ATOMIC_RELAXED));
> +       if (memorder == __ATOMIC_ACQUIRE)
> +               asm volatile("ldaxr %x[tmp], [%x[addr]]"
> +                       : [tmp] "=&r" (tmp)
> +                       : [addr] "r"(addr)
> +                       : "memory");
> +       else if (memorder == __ATOMIC_RELAXED)
> +               asm volatile("ldxr %x[tmp], [%x[addr]]"
> +                       : [tmp] "=&r" (tmp)
> +                       : [addr] "r"(addr)
> +                       : "memory");
> +       return tmp;
> +}
> +
> +#ifdef RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED
> +static __rte_always_inline void
> +rte_wait_until_equal_16(volatile uint16_t *addr, uint16_t expected,
> +int memorder)
> +{
> +       if (__atomic_load_n(addr, memorder) != expected) {
> +               rte_sevl();
> +               do {
> +                       rte_wfe();


We are in the RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED case.
rte_wfe() is always asm volatile("wfe" : : : "memory");


> +               } while (rte_atomic_load_ex_16(addr, memorder) != expected);
> +       }
> +}
> +
> +static __rte_always_inline void
> +rte_wait_until_equal_32(volatile uint32_t *addr, uint32_t expected,
> +int memorder)
> +{
> +       if (__atomic_load_n(addr, memorder) != expected) {
> +               rte_sevl();
> +               do {
> +                       rte_wfe();
> +               } while (__atomic_load_n(addr, memorder) != expected);
> +       }
> +}

The while() should be with an exclusive load.


I will submit a v12 with those comments addressed so that we move
forward for rc2.
But it won't make it in rc1, sorry.


-- 
David Marchand