From: EXT Ola Liljedahl [mailto:[email protected]] Sent: Wednesday, November 11, 2015 6:50 PM To: Savolainen, Petri (Nokia - FI/Espoo) Cc: LNG ODP Mailman List Subject: Re: [lng-odp] [API-NEXT PATCH v2] api: atomic: added atomic_lock_free_u64
On 11 November 2015 at 15:24, Petri Savolainen <[email protected]<mailto:[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. This is topic of another patch (set). This patch provides the type, other patches can make use of it. It’s a separate discussion if this type should be used in general or platform specific init parameters. Signed-off-by: Petri Savolainen <[email protected]<mailto:[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. Comment here is merely the name of the operation. Anyway, changed it in v3. + 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. It’s not here, because it’s not currently in the API. + uint32_t cas : 1; /**< Atomic compare and swap */ + uint32_t _reserved : 18; /**< Reserved - do not use */ Is this field actually needed? Padding can be actually defined also anonymously, but checkpatch didn’t understand that (mixed it with labels). So, I removed padding. + } 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? Thought that all.ops would be useful to access all operation flags, but ended up removing that because C spec leaves it implementation defined how bit fields are mapped (14x 1bit vs 1x 14bits may not map the same way). + +/** + * 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. It is all.ops = 0x3fff. This was the idea of all.ops. Now there’s only all_bits and you can just do all_bits = 0xffffffff. -Petri + } + + return 2; +#endif +} -- 2.6.2 _______________________________________________ lng-odp mailing list [email protected]<mailto:[email protected]> https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
