[dpdk-dev] [PATCH v7 3/7] spinlock: use wfe to reduce contention on aarch64

[Gavin Hu (Arm Technology China)]

Hi David,

> -----Original Message-----
> From: David Marchand <david.marchand at redhat.com>
> Sent: Friday, October 18, 2019 2:28 AM
> To: Gavin Hu (Arm Technology China) <Gavin.Hu at arm.com>
> Cc: dev <dev at dpdk.org>; nd <nd at arm.com>; thomas at monjalon.net;
> Stephen Hemminger <stephen at networkplumber.org>;
> hemant.agrawal at nxp.com; jerinj at marvell.com; Pavan Nikhilesh
> <pbhagavatula at marvell.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli at arm.com>; Ruifeng Wang (Arm Technology China)
> <Ruifeng.Wang at arm.com>; Phil Yang (Arm Technology China)
> <Phil.Yang at arm.com>; Steve Capper <Steve.Capper at arm.com>
> Subject: Re: [dpdk-dev] [PATCH v7 3/7] spinlock: use wfe to reduce
> contention on aarch64
> 
> On Fri, Sep 27, 2019 at 7:43 AM Gavin Hu <gavin.hu at arm.com> wrote:
> >
> > In acquiring a spinlock, cores repeatedly poll the lock variable.
> > This is replaced by rte_wait_until_equal API.
> >
> > Running the micro benchmarking and the testpmd and l3fwd traffic tests
> > on ThunderX2, Ampere eMAG80 and Arm N1SDP, everything went well
> and no
> > notable performance gain nor degradation was measured.
> >
> > Signed-off-by: Gavin Hu <gavin.hu at arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang at arm.com>
> > Reviewed-by: Phil Yang <phil.yang at arm.com>
> > Reviewed-by: Steve Capper <steve.capper at arm.com>
> > Reviewed-by: Ola Liljedahl <ola.liljedahl at arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli at arm.com>
> > Tested-by: Pavan Nikhilesh <pbhagavatula at marvell.com>
> > ---
> >  .../common/include/arch/arm/rte_spinlock.h         | 26
> ++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/lib/librte_eal/common/include/arch/arm/rte_spinlock.h
> b/lib/librte_eal/common/include/arch/arm/rte_spinlock.h
> > index 1a6916b..b61c055 100644
> > --- a/lib/librte_eal/common/include/arch/arm/rte_spinlock.h
> > +++ b/lib/librte_eal/common/include/arch/arm/rte_spinlock.h
> > @@ -16,6 +16,32 @@ extern "C" {
> >  #include <rte_common.h>
> >  #include "generic/rte_spinlock.h"
> >
> > +/* armv7a does support WFE, but an explicit wake-up signal using SEV
> is
> > + * required (must be preceded by DSB to drain the store buffer) and
> > + * this is less performant, so keep armv7a implementation unchanged.
> > + */
> > +#ifndef RTE_FORCE_INTRINSICS
> 
> Earlier, in the same file, I can see:
> https://git.dpdk.org/dpdk/tree/lib/librte_eal/common/include/arch/arm/r
> te_spinlock.h?h=v19.08#n8
> 
> #ifndef RTE_FORCE_INTRINSICS
> #  error Platform must be built with CONFIG_RTE_FORCE_INTRINSICS
> #endif
> 
> IIUC, this is dead code.

This is not dead code, RTE_FORCE_INTRINSICS is still mandatory for aarch64 ad RTE_ARM_USE_WFE is optional currently.
Yes, as Jerin pointed out, we may make it optional also, like x86, but now it is still too earlier before WFE is mandatory, anyway it is in our plan. 
I will tweak a little bit for the two macros to reflect this logic in v8.
/Gavin
> 
> > +static inline void
> > +rte_spinlock_lock(rte_spinlock_t *sl)
> > +{
> > +       unsigned int tmp;
> > +       /* http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.
> > +        * faqs/ka16809.html
> > +        */
> > +       asm volatile(
> > +               "1:     ldaxr %w[tmp], %w[locked]\n"
> > +               "cbnz   %w[tmp], 2f\n"
> > +               "stxr   %w[tmp], %w[one], %w[locked]\n"
> > +               "cbnz   %w[tmp], 1b\n"
> > +               "ret\n"
> > +               "2:     sevl\n"
> > +               "wfe\n"
> > +               "jmp    1b\n"
> > +               : [tmp] "=&r" (tmp), [locked] "+Q"(sl->locked)
> > +               : [one] "r" (1)
> > +}
> > +#endif
> > +
> >  static inline int rte_tm_supported(void)
> >  {
> >         return 0;
> > --
> > 2.7.4
> >
> 
> 
> --
> David Marchand