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

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>

> > > > Subject: Re: [PATCH v13 2/5] eal: add the APIs to wait until equal
> > > >
> > > > 08/11/2019 17:38, Ananyev, Konstantin:
> > > > > > From: Gavin Hu <gavin.hu@arm.com>
> > > > > > +static __rte_always_inline void
> > > > > > +rte_wait_until_equal_64(volatile uint64_t *addr, uint64_t
> expected,
> > > > > > +         int memorder)
> > > > > > +{
> > > > > > + uint64_t value;
> > > > > > +
> > > > > > + assert(memorder == __ATOMIC_ACQUIRE || memorder ==
> > __ATOMIC_RELAXED);
> > > > > > +
> > > > > > + /*
> > > > > > +  * Atomic exclusive load from addr, it returns the 64-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.
> > > > > > +  */
> > > > > > +#define __LOAD_EXC_64(src, dst, memorder) {              \
> > > > > > + if (memorder == __ATOMIC_RELAXED) {              \
> > > > > > +         asm volatile("ldxr %x[tmp], [%x[addr]]"  \
> > > > > > +                 : [tmp] "=&r" (dst)              \
> > > > > > +                 : [addr] "r"(src)                \
> > > > > > +                 : "memory");                     \
> > > > > > + } else {                                         \
> > > > > > +         asm volatile("ldaxr %x[tmp], [%x[addr]]" \
> > > > > > +                 : [tmp] "=&r" (dst)              \
> > > > > > +                 : [addr] "r"(src)                \
> > > > > > +                 : "memory");                     \
> > > > > > + } }
> > > > > > +
> > > > > > + __LOAD_EXC_64(addr, value, memorder)  if (value != expected)
> > > > > > +{
> > > > > > +         __SEVL()
> > > > > > +         do {
> > > > > > +                 __WFE()
> > > > > > +                 __LOAD_EXC_64(addr, value, memorder)
> > > > > > +         } while (value != expected);  } } #undef
> > > > > > +__LOAD_EXC_64
> > > > > > +
> > > > > > +#undef __SEVL
> > > > > > +#undef __WFE
> > > > >
> > > > > Personally I don't see how these define/undef are better then
> > > > > inline
> > functions...
> > > >
> > > > The benefit it to not pollute the API namespace with some
> > > > functions which are used only locally.
> > > > After using a macro, it disappears with #undef.
> > > >
> > > > > Again I suppose they might be re-used in future some other ARM
> > specific low-level primitvies.
> > > >
> > > > Do this low-level routines _LOAD_EXC_ need to be exposed in EAL
> > > > for re-
> > use?
> > >
> > > About load_exc don't know for sure...
> > > Though as I can see wfe/sevl are used here:
> > > http://patchwork.dpdk.org/patch/61779/
> > > Might be it is possible to reuse these functions/macros...
> > > But again, I am not arm expert, would be interested to know what arm
> > guys think.
> >
> >
> > Considering WFE has a requirement on using load_exec on ARM so IMO, it
> > makes sense to expose load_exec in conjunction with wfe/sevl to use it
> > properly.
> Agree, WFE should have more use cases can be developed, not limited to the
> WAIT_UNTIL_EQUAL_XX APIs. Marvel's patch is an example.
> Sorry I don't have more use case so far, but Arm is evolving WFE, so I think
> more use cases are emerging.
> /Gavin
I think the focus of this patch is 'rte_wait_until_equal_xxx' APIs. I would say, this patch serves that purpose as we all agree on the definition of the APIs.
The implementation of the API may be conservative, but I think it is alright as it does not bind us from changing it in the future. We can look at further use cases as and when they come.

> >
> >
> > >
> > > >
> > > > > My preference would be to keep them as inline function - much
> > cleaner code.
> > > > > But as I don't develop for/use, I wouldn't insist and let you
> > > > > and arm
> > guys to decide.
> > > >
> > > >
> > >