Re: [RFC v2 5/6] eal: add atomic bit operations

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

On Fri, Apr 26, 2024 at 11:39:17AM +0200, Mattias Rönnblom wrote:

[ ... ]

> >
> >>
> >>The only reason for _Atomic being as it is, as far as I can see, is to
> >>accommodate for ISAs which does not have the appropriate atomic machine
> >>instructions, and thus require a lock or some other data associated with
> >>the actual user-data-carrying bits. Neither GCC nor DPDK supports any
> >>such ISAs, to my knowledge. I suspect neither never will. So the cast
> >>will continue to work.
> >
> >I tend to agree with you on this.
> >
> >We should officially decide that DPDK treats RTE_ATOMIC types as a union of _Atomic and non-atomic, i.e. operations on RTE_ATOMIC types can be both atomic and non-atomic.
> >
> 
> I think this is a subject which needs to be further explored.
> 
> Objects that can be accessed both atomically and non-atomically
> should be without _Atomic. With my current understanding of this
> issue, that seems like the best option.

i've been distracted by other work and while not in the scope of this
series i want to say +1 to having this discussion. utilizing a union for
this atomic vs non-atomic access that appears in practice is a good idea.

> 
> You could turn it around as well, and have such marked _Atomic and
> have explicit casts to their non-_Atomic cousins when operated upon
> by non-atomic functions. Not sure how realistic that is, since
> non-atomicity is the norm. All generic selection-based "functions"
> must take this into account.

the problem with casts is they are actually different types and may have
different size and/or alignment relative to their non-atomic types.
for current non-locking atomics the implementations happen to be the
same (presumably because it was practical) but the union is definitely a
cleaner approach.

> 
> >>
> >>>>+				      unsigned int nr, int memory_order) \
> >>>>+	{								\
> >>>>+		RTE_ASSERT(nr < size);					\
> >>>>+									\
> >>>>+		const RTE_ATOMIC(uint ## size ## _t) *a_addr =		\
> >>>>+			(const RTE_ATOMIC(uint ## size ## _t) *)addr;	\
> >>>>+		uint ## size ## _t mask = (uint ## size ## _t)1 << nr;	\
> >>>>+		return rte_atomic_load_explicit(a_addr, memory_order) &
> >>mask; \
> >>>>+	}
> >>>
> >>>
> >>>Similar considerations regarding volatile qualifier for the "once"
> >>operations.
> >>>