On 12 November 2015 at 09:40, Savolainen, Petri (Nokia - FI/Espoo) <[email protected]> wrote: > > > > > 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]> > 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. OK
> > > > > > > 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. > > > > Comment here is merely the name of the operation. Anyway, changed it in v3. What I mean is that since init does not have to be atomic, it doesn't have to be included in this list of (supported) atomic operations. It is not relevant whether init is atomic or not. It does not belong here. > > > > > > + 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. Please add it. I see it as a basic atomic operation. In some situations, you don't want to have to use load and CAS when exchange would be sufficient. Especially not if there is a "far" implementation of atomic exchange, load would defeat the purpose of such a "far" implementation. > > > > > > + 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. Good. > > > > > > + } 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). OK > > > > > > + > +/** > + * 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. As long as you have GCC on your side... (and LLVM). > > > > -Petri > > > > + } > + > + 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
