On Mon, Aug 14, 2023 at 10:00:49AM +0200, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com]
> > Sent: Friday, 11 August 2023 19.32
> >
> > Adapt the EAL public headers to use rte optional atomics API instead of
> > directly using and exposing toolchain specific atomic builtin intrinsics.
> >
> > Signed-off-by: Tyler Retzlaff <roret...@linux.microsoft.com>
> > ---
>
> [...]
>
will fix the comments identified.
>
> [...]
>
> > --- a/lib/eal/include/generic/rte_spinlock.h
> > +++ b/lib/eal/include/generic/rte_spinlock.h
> > @@ -29,7 +29,7 @@
> > * The rte_spinlock_t type.
> > */
> > typedef struct __rte_lockable {
> > - volatile int locked; /**< lock status 0 = unlocked, 1 = locked */
> > + volatile int __rte_atomic locked; /**< lock status 0 = unlocked, 1 =
> > locked */
>
> I think __rte_atomic should be before the type:
> volatile __rte_atomic int locked; /**< lock status [...]
> Alternatively (just mentioning it, I know we don't use this form):
> volatile __rte_atomic(int) locked; /**< lock status [...]
>
> Thinking of where you would put "const" might help.
>
> Maybe your order is also correct, so it is a matter of preference.
so for me what you suggest is the canonical convention for c and i did
initially try to make the change with this convention but ran into
trouble when using the keyword in a context used as a type specifier
and the type was incomplete.
the rte_mcslock is a good example for illustration.
// original struct
typedef struct rte_mcslock {
struct rte_mcslock *next;
...
};
it simply doesn't work / won't compile (at least with clang) which is
what drove me to use the less-often used syntax.
typedef struct rte_mcslock {
_Atomic struct rte_mcslock *next;
...
};
In file included from ../app/test/test_mcslock.c:19:
..\lib\eal\include\rte_mcslock.h:36:2: error: _Atomic cannot be applied
to incomplete type 'struct rte_mcslock'
_Atomic struct rte_mcslock *next;
^
..\lib\eal\include\rte_mcslock.h:35:16: note: definition of 'struct
rte_mcslock' is not complete until the closing '}'
typedef struct rte_mcslock {
^
1 error generated.
so i ended up choosing to use a single syntax by convention consistently
rather than using one for the exceptional case and one everywhere else.
i think (based on our other thread of discussion) i would recommend we
use adopt and require the use of the _Atomic(T) macro to disambiguate it
also has the advantage of not being churned later when we can do c++23.
// using macro
typedef struct rte_mcslock {
_Atomic(struct rte_mcslock *) next;
...
};
this is much easier at a glance to know when the specified type is the T
or the T * similarly in parameter lists it becomes more clear too.
e.g.
void foo(int *v)
that it is either void foo(_Atomic(int) *v) or void foo(_Atomic(int *) v)
becomes
much clearer without having to do mental gymnastics.
so i propose we retain
#define __rte_atomic _Atomic
allow it to be used in contexts where we need a type-qualifier.
note:
most of the cases where _Atomic is used as a type-qualifier it
is a red flag that we are sensitive to an implementation detail
of the compiler. in time i hope most of these will go away as we
remove deprecated rte_atomic_xx apis.
but also introduce the following macro
#define RTE_ATOMIC(type) _Atomic(type)
require it be used in the contexts that we are using it as a type-specifier.
if folks agree with this please reply back positively and i'll update
the series. feel free to propose alternate names or whatever, but sooner
than later so i don't have to churn things too much :)
thanks!
>
> The DPDK coding style guidelines doesn't mention where to place "const", but
> looking at the code, it seems to use "const unsigned int" and "const char *".
we probably should document it as a convention and most likely we should
adopt what is already in use more commonly.
>
> > } rte_spinlock_t;
> >
> > /**
>
> [...]
>
> > --- a/lib/eal/include/rte_mcslock.h
> > +++ b/lib/eal/include/rte_mcslock.h
> > @@ -33,8 +33,8 @@
> > * The rte_mcslock_t type.
> > */
> > typedef struct rte_mcslock {
> > - struct rte_mcslock *next;
> > - int locked; /* 1 if the queue locked, 0 otherwise */
> > + struct rte_mcslock * __rte_atomic next;
>
> Correct, the pointer is atomic, not the struct.
>
> > + int __rte_atomic locked; /* 1 if the queue locked, 0 otherwise */
>
> Again, I think __rte_atomic should be before the type:
> __rte_atomic int locked; /* 1 if the queue locked, 0 otherwise */
>
> > } rte_mcslock_t;
> >
>
> [...]
>
> > @@ -101,34 +101,34 @@
> > * A pointer to the node of MCS lock passed in rte_mcslock_lock.
> > */
> > static inline void
> > -rte_mcslock_unlock(rte_mcslock_t **msl, rte_mcslock_t *me)
> > +rte_mcslock_unlock(rte_mcslock_t * __rte_atomic *msl, rte_mcslock_t *
> > __rte_atomic me)
> > {
> > /* Check if there are more nodes in the queue. */
> > - if (likely(__atomic_load_n(&me->next, __ATOMIC_RELAXED) == NULL)) {
> > + if (likely(rte_atomic_load_explicit(&me->next, rte_memory_order_relaxed)
> > == NULL)) {
> > /* No, last member in the queue. */
> > - rte_mcslock_t *save_me = __atomic_load_n(&me, __ATOMIC_RELAXED);
> > + rte_mcslock_t *save_me = rte_atomic_load_explicit(&me,
> > rte_memory_order_relaxed);
> >
> > /* Release the lock by setting it to NULL */
> > - if (likely(__atomic_compare_exchange_n(msl, &save_me, NULL, 0,
> > - __ATOMIC_RELEASE, __ATOMIC_RELAXED)))
> > + if (likely(rte_atomic_compare_exchange_strong_explicit(msl,
> > &save_me, NULL,
> > + rte_memory_order_release,
> > rte_memory_order_relaxed)))
> > return;
> >
> > /* Speculative execution would be allowed to read in the
> > * while-loop first. This has the potential to cause a
> > * deadlock. Need a load barrier.
> > */
> > - __atomic_thread_fence(__ATOMIC_ACQUIRE);
> > + __rte_atomic_thread_fence(rte_memory_order_acquire);
> > /* More nodes added to the queue by other CPUs.
> > * Wait until the next pointer is set.
> > */
> > - uintptr_t *next;
> > - next = (uintptr_t *)&me->next;
> > + uintptr_t __rte_atomic *next;
> > + next = (uintptr_t __rte_atomic *)&me->next;
>
> This way around, I think:
> __rte_atomic uintptr_t *next;
> next = (__rte_atomic uintptr_t *)&me->next;
>
> [...]
>
> > --- a/lib/eal/include/rte_pflock.h
> > +++ b/lib/eal/include/rte_pflock.h
> > @@ -41,8 +41,8 @@
> > */
> > struct rte_pflock {
> > struct {
> > - uint16_t in;
> > - uint16_t out;
> > + uint16_t __rte_atomic in;
> > + uint16_t __rte_atomic out;
>
> Again, I think __rte_atomic should be before the type:
> __rte_atomic uint16_t in;
> __rte_atomic uint16_t out;
>
> > } rd, wr;
> > };
>
> [...]
>
> > --- a/lib/eal/include/rte_seqcount.h
> > +++ b/lib/eal/include/rte_seqcount.h
> > @@ -32,7 +32,7 @@
> > * The RTE seqcount type.
> > */
> > typedef struct {
> > - uint32_t sn; /**< A sequence number for the protected data. */
> > + uint32_t __rte_atomic sn; /**< A sequence number for the protected data.
> > */
>
> Again, I think __rte_atomic should be before the type:
> __rte_atomic uint32_t sn; /**< A sequence [...]
>
> > } rte_seqcount_t;
>
> [...]
>
> > --- a/lib/eal/include/rte_ticketlock.h
> > +++ b/lib/eal/include/rte_ticketlock.h
> > @@ -30,10 +30,10 @@
> > * The rte_ticketlock_t type.
> > */
> > typedef union {
> > - uint32_t tickets;
> > + uint32_t __rte_atomic tickets;
> > struct {
> > - uint16_t current;
> > - uint16_t next;
> > + uint16_t __rte_atomic current;
> > + uint16_t __rte_atomic next;
>
> Again, I think __rte_atomic should be before the type:
> __rte_atomic uint16_t current;
> __rte_atomic uint16_t next;
>
> > } s;
> > } rte_ticketlock_t;
>
>
>
> > @@ -127,7 +129,7 @@
> >
> > typedef struct {
> > rte_ticketlock_t tl; /**< the actual ticketlock */
> > - int user; /**< core id using lock, TICKET_LOCK_INVALID_ID for unused */
> > + int __rte_atomic user; /**< core id using lock, TICKET_LOCK_INVALID_ID
> > for unused */
>
> Again, I think __rte_atomic should be before the type:
> __rte_atomic int user; /**< core id [...]
>
> > unsigned int count; /**< count of time this lock has been called */
> > } rte_ticketlock_recursive_t;
>
> [...]
>
> > --- a/lib/eal/include/rte_trace_point.h
> > +++ b/lib/eal/include/rte_trace_point.h
> > @@ -33,7 +33,7 @@
> > #include <rte_stdatomic.h>
> >
> > /** The tracepoint object. */
> > -typedef uint64_t rte_trace_point_t;
> > +typedef uint64_t __rte_atomic rte_trace_point_t;
>
> Again, I think __rte_atomic should be before the type:
> typedef __rte_atomic uint64_t rte_trace_point_t;
>
> [...]
>
> At the risk of having gone "speed blind" by all the search-replaces along the
> way...
>
> Reviewed-by: Morten Brørup <m...@smartsharesystems.com>
>
