On 11 November 2015 at 15:24, Petri Savolainen <[email protected]> wrote:
> Platforms may support some uint64 operations lock-free and > others not. For example, inc_64 can be natively supported but > cas_64 (or max_64/min_64) not. User may be able to switch to > 32 bit variables when a 64 bit operation uses locks. The same > atomic operation struture could be used for platform init to guide > structure? > lock vs. lock-free implementation (e.g. if cas_64 is never > called, inc_64 can be lock-free). > I think this also should be added to the patch. It feels incomplete now. > Signed-off-by: Petri Savolainen <[email protected]> > --- > include/odp/api/atomic.h | 51 > +++++++++++++++++++++++++++++++++++++ > platform/linux-generic/Makefile.am | 1 + > platform/linux-generic/odp_atomic.c | 26 +++++++++++++++++++ > 3 files changed, 78 insertions(+) > create mode 100644 platform/linux-generic/odp_atomic.c > > diff --git a/include/odp/api/atomic.h b/include/odp/api/atomic.h > index 316f13a..17a38fb 100644 > --- a/include/odp/api/atomic.h > +++ b/include/odp/api/atomic.h > @@ -388,6 +388,57 @@ void odp_atomic_add_rls_u32(odp_atomic_u32_t *atom, > uint32_t val); > void odp_atomic_sub_rls_u32(odp_atomic_u32_t *atom, uint32_t val); > > /** > + * Atomic operations > + * > + * Atomic operations listed in a bit field structure. > + */ > +typedef union odp_atomic_op_t { > + /** Operation flags */ > + struct { > + uint32_t init : 1; /**< Atomic init */ > Init doesn't have to be atomic. No other thread is supposed to operate on an atomic/shared variable before being signalled that this is OK (and initialisation has been done). The signalling will include the necessary ordering. + uint32_t load : 1; /**< Atomic load */ > + uint32_t store : 1; /**< Atomic store */ > + uint32_t fetch_add : 1; /**< Atomic fetch and add */ > + uint32_t add : 1; /**< Atomic add */ > + uint32_t fetch_sub : 1; /**< Atomic fetch and substract */ > + uint32_t sub : 1; /**< Atomic substract */ > + uint32_t fetch_inc : 1; /**< Atomic fetch and increment */ > + uint32_t inc : 1; /**< Atomic increment */ > + uint32_t fetch_dec : 1; /**< Atomic fetch and decrement */ > + uint32_t dec : 1; /**< Atomic decrement */ > + uint32_t min : 1; /**< Atomic minimum */ > + uint32_t max : 1; /**< Atomic maximum */ > I am missing atomic exchange here. > + uint32_t cas : 1; /**< Atomic compare and swap */ > + uint32_t _reserved : 18; /**< Reserved - do not use */ > Is this field actually needed? > + } op; > + > + /** All flags */ > + union { > + uint32_t ops : 14; /**< All operations*/ > + uint32_t _reserved : 18; /**< Reserved - do not use */ > You are missing a struct here? > + > + uint32_t bits; /**< All bits */ > + } all; > +} odp_atomic_op_t; > Feels a little bit too complicated. Wouldn't it be enough with just a union of the op bitfield struct and the uint32_t bits? + > +/** > + * Query which atomic uint64 operations are lock-free > + * > + * Lock-free implementations have higher performance and scale better than > + * implementations using locks. User can decide to use e.g. uint32 atomic > + * variables instead of uint64 to optimize performance on platforms that > + * implement a performance critical operation using locks. > + * > + * @param atomic_op Pointer to atomic operation structure for storing > + * operation flags. All bits are initialized to zero > during > + * the operation. The parameter is ignored when NULL. > + * @retval 0 None of the operations are lock-free > + * @retval 1 Some of the operations are lock-free > + * @retval 2 All operations are lock-free > + */ > +int odp_atomic_lock_free_u64(odp_atomic_op_t *atomic_op); > + > +/** > * @} > */ > > diff --git a/platform/linux-generic/Makefile.am > b/platform/linux-generic/Makefile.am > index a6b6029..0b7234e 100644 > --- a/platform/linux-generic/Makefile.am > +++ b/platform/linux-generic/Makefile.am > @@ -151,6 +151,7 @@ noinst_HEADERS = \ > ${srcdir}/Makefile.inc > > __LIB__libodp_la_SOURCES = \ > + odp_atomic.c \ > odp_barrier.c \ > odp_buffer.c \ > odp_classification.c \ > diff --git a/platform/linux-generic/odp_atomic.c > b/platform/linux-generic/odp_atomic.c > new file mode 100644 > index 0000000..8955b41 > --- /dev/null > +++ b/platform/linux-generic/odp_atomic.c > @@ -0,0 +1,26 @@ > +/* Copyright (c) 2015, Linaro Limited > + * All rights reserved. > + * > + * SPDX-License-Identifier: BSD-3-Clause > + */ > + > +#include <odp/atomic.h> > + > +int odp_atomic_lock_free_u64(odp_atomic_op_t *atomic_op) > +{ > +#if __GCC_ATOMIC_LLONG_LOCK_FREE < 2 > + /* All operations have locks */ > + if (atomic_op) > + atomic_op->all.bits = 0; > + > + return 0; > +#else > + /* All operations are lock-free */ > + if (atomic_op) { > + atomic_op->all.bits = 0; > + atomic_op->all.ops = 0x3fff; > How do we know the right bits are set? Endian and compiler differences. Perhaps atomic_op->ops = 0x3fff is better. > + } > + > + return 2; > +#endif > +} > -- > 2.6.2 > > _______________________________________________ > lng-odp mailing list > [email protected] > https://lists.linaro.org/mailman/listinfo/lng-odp >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
