Re: [dpdk-dev] Use WFE for spinlock and ring

[Ruifeng Wang <Ruifeng.Wang@arm.com>]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Friday, April 30, 2021 10:19 PM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Cc: thomas@monjalon.net; Ruifeng Wang <Ruifeng.Wang@arm.com>; David
> Marchand <david.marchand@redhat.com>; dev <dev@dpdk.org>;
> jerinj@marvell.com; nd <nd@arm.com>
> Subject: Re: Use WFE for spinlock and ring
> 
> On Fri, Apr 30, 2021 at 01:41:22PM +0000, Honnappa Nagarahalli wrote:
> > <snip>
> >
> > > > > > > > >
> > > > > > > > > The rte_wait_until_equal_xxx APIs abstract the
> > > > > > > > > functionality of 'polling for a memory location to
> > > > > > > > > become equal to a given
> > > value'[1].
> > > > > > > > >
> > > > > > > > > Use the API for the rte spinlock and ring implementations.
> > > > > > > > > With the wait until equal APIs being stable, changes
> > > > > > > > > will not impact
> > > ABI.
> > > > > > > >
> > > > > > > > Afaics, there is no ARM target with WFE enabled and we
> > > > > > > > lost ability to enable WFE support with removal of the
> > > > > > > > make build
> > > system.
> > > > > > >
> > > > > > > WFE can be enabled with direct meson file change.
> > > > > > > WFE is not intended to be enabled by default. It can be
> > > > > > > enabled based on benchmarking result on hardware.
> > > > > > > >
> > > > > > > > $ git grep RTE_ARM_USE_WFE
> > > > > > > > config/arm/meson.build:        ['RTE_ARM_USE_WFE', false],
> > > > > > > > lib/eal/arm/include/rte_pause_64.h:#ifdef RTE_ARM_USE_WFE
> > > > > > > >
> > > > > > > > How did you enable WFE to test this series?
> > > > > > >
> > > > > > > I modified meson file to test.
> > > > > > > Tests were also done with WFE disabled to make sure no
> > > > > > > degradation with
> > > > > > generic implementation.
> > > > > >
> > > > > > I don't understand the usage.
> > > > > > Which platform should use it?
> > > > >
> > > > > Platforms that implement WFE semantic (e.g. N1) can use.
> > > > > The user can enable this feature for power efficiency purpose.
> > > > > But there is something to note as described in commit message
> > > > > 1be7855d77
> > > when the API was introduced.
> > > > >
> > > > > > Should it be a compile-time option?
> > > > >
> > > > > Yes, it should be a compile-time option.
> > > > > It can be configured via c_args meson option?
> > > >
> > > > +Cc Bruce for discussing how to enable such feature.
> > > >
> > > > The problem with c_args is that the application has no way to know.
> > > >
> > > Agree about c_args not being a great choice. Why does this need to
> > > be a compile-time option? Can runtime support not be detected in
> > > some manner?
> > The problem is inconsistency in performance on different Arm platforms.
> We had decided that each platform needs to enable it after some testing.
> 
> Then it sounds like it does indeed need to be a build option. Does it need to
> be added to the meson_options.txt, or can it just be specified in cross-files
> and optionally via c_args?

Add it to the meson_options.txt is good as the option will be clearly exposed.
My concern is more options (e.g. RTE_ARCH_ARM64_MEMCPY) need to be added in.
Will the options bloat meson_options.txt?