On Thu, Sep 28, 2023 at 10:06 AM Thomas Monjalon <tho...@monjalon.net> wrote:
>
> 22/08/2023 23:00, Tyler Retzlaff:
> > --- a/lib/eal/include/generic/rte_rwlock.h
> > +++ b/lib/eal/include/generic/rte_rwlock.h
> > @@ -32,6 +32,7 @@
> > #include <rte_common.h>
> > #include <rte_lock_annotations.h>
> > #include <rte_pause.h>
> > +#include <rte_stdatomic.h>
>
> I'm not sure about adding the include in patch 1 if it is not used here.
Yes, this is something I had already fixed locally.
>
> > --- /dev/null
> > +++ b/lib/eal/include/rte_stdatomic.h
> > @@ -0,0 +1,198 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2023 Microsoft Corporation
> > + */
> > +
> > +#ifndef _RTE_STDATOMIC_H_
> > +#define _RTE_STDATOMIC_H_
> > +
> > +#include <assert.h>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +#ifdef RTE_ENABLE_STDATOMIC
> > +#ifdef __STDC_NO_ATOMICS__
> > +#error enable_stdatomics=true but atomics not supported by toolchain
> > +#endif
> > +
> > +#include <stdatomic.h>
> > +
> > +/* RTE_ATOMIC(type) is provided for use as a type specifier
> > + * permitting designation of an rte atomic type.
> > + */
> > +#define RTE_ATOMIC(type) _Atomic(type)
> > +
> > +/* __rte_atomic is provided for type qualification permitting
> > + * designation of an rte atomic qualified type-name.
>
> Sorry I don't understand this comment.
The difference between atomic qualifier and atomic specifier and the
need for exposing those two notions are not obvious to me.
One clue I have is with one use later in the series:
+rte_mcslock_lock(RTE_ATOMIC(rte_mcslock_t *) *msl, rte_mcslock_t *me)
...
+ prev = rte_atomic_exchange_explicit(msl, me, rte_memory_order_acq_rel);
So at least RTE_ATOMIC() seems necessary.
>
> > + */
> > +#define __rte_atomic _Atomic
> > +
> > +/* The memory order is an enumerated type in C11. */
> > +typedef memory_order rte_memory_order;
> > +
> > +#define rte_memory_order_relaxed memory_order_relaxed
> > +#ifdef __ATOMIC_RELAXED
> > +static_assert(rte_memory_order_relaxed == __ATOMIC_RELAXED,
> > + "rte_memory_order_relaxed == __ATOMIC_RELAXED");
>
> Not sure about using static_assert or RTE_BUILD_BUG_ON
Do you mean you want no check at all in a public facing header?
Or is it that we have RTE_BUILD_BUG_ON and we should keep using it
instead of static_assert?
I remember some problems with RTE_BUILD_BUG_ON where the compiler
would silently drop the whole expression and reported no problem as it
could not evaluate the expression.
At least, with static_assert (iirc, it is new to C11) the compiler
complains with a clear "error: expression in static assertion is not
constant".
We could fix RTE_BUILD_BUG_ON, but I guess the fix would be equivalent
to map it to static_assert(!condition).
Using language standard constructs seems a better choice.
--
David Marchand
