Re: [dpdk-dev] [PATCH v5 01/15] eal/arm: atomic operations for ARM

[Jan Viktorin <viktorin@rehivetech.com>]

On Mon, 2 Nov 2015 11:23:05 +0530
Jerin Jacob <jerin.jacob@caviumnetworks.com> wrote:

--snip--
> > +/*------------------------- 16 bit atomic operations -------------------------*/
> > +
> > +#ifndef RTE_FORCE_INTRINSICS
> > +static inline int
> > +rte_atomic16_cmpset(volatile uint16_t *dst, uint16_t exp, uint16_t src)
> > +{
> > +	return __atomic_compare_exchange(dst, &exp, &src, 0, __ATOMIC_ACQUIRE,
> > +		__ATOMIC_ACQUIRE) ? 1 : 0;
> > +}  
> 
> IMO, it should be __ATOMIC_SEQ_CST be instead of __ATOMIC_ACQUIRE.
> __ATOMIC_ACQUIRE works in conjunction with __ATOMIC_RELEASE.
> AFAIK, DPDK atomic api expects full barrier. C11 memory model not yet
> used.

Seems to be reasonable, thanks.

> So why can't we use RTE_FORCE_INTRINSICS based generic
> implementation. Same holds true for spinlock implementation too(i.e using
> RTE_FORCE_INTRINSICS). Am I missing something here ?

True. This was done with the intention to rewrite as a platform-specific
assembly. But it's never been done yet... If you mean to set
RTE_FORCE_INTRINSICS=y in the defconfig and remove this code entirely
(at least for ARMv7), I would agree.

> 
> 
> 
> > +
> > +static inline int rte_atomic16_test_and_set(rte_atomic16_t *v)
> > +{
> > +	return rte_atomic16_cmpset((volatile uint16_t *)&v->cnt, 0, 1);
> > +}
--snip--

-- 
   Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic