Re: [PATCH 1/2] eal: fix constraints on stdatomic API

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

On Fri, Dec 15, 2023 at 10:47:36AM +0800, Jie Hai wrote:
> On 2023/12/12 2:53, Tyler Retzlaff wrote:
> >On Mon, Dec 11, 2023 at 03:39:03PM +0800, Jie Hai wrote:
> >>The first parameter of rte_atomic_exchange_explicit() must be a
> >>pointer to _Atomic type. If run command "meson setup --werror
> >>-Denable_stdatomic=true build && ninja -C build", error will occur.
> >>Thia patch fixes it.
> >>
> >>Fixes: 1ec6a845b5cb ("eal: use stdatomic API in public headers")
> >>Cc: stable@dpdk.org
> >>
> >>Signed-off-by: Jie Hai <haijie1@huawei.com>
> >>---
> >>  app/test/test_atomic.c               |  6 +++---
> >>  lib/eal/include/generic/rte_atomic.h | 12 ++++++------
> >>  2 files changed, 9 insertions(+), 9 deletions(-)
> >>
> >>diff --git a/app/test/test_atomic.c b/app/test/test_atomic.c
> >>index db07159e81ab..c3cb3ae0ea57 100644
> >>--- a/app/test/test_atomic.c
> >>+++ b/app/test/test_atomic.c
> >>@@ -347,9 +347,9 @@ typedef union {
> >>  const uint8_t CRC8_POLY = 0x91;
> >>  uint8_t crc8_table[256];
> >>-volatile uint16_t token16;
> >>-volatile uint32_t token32;
> >>-volatile uint64_t token64;
> >>+volatile RTE_ATOMIC(uint16_t) token16;
> >>+volatile RTE_ATOMIC(uint32_t) token32;
> >>+volatile RTE_ATOMIC(uint64_t) token64;
> >
> >subject to my comment below, volatile qualification can be removed.
> >
> >>  static void
> >>  build_crc8_table(void)
> >>diff --git a/lib/eal/include/generic/rte_atomic.h b/lib/eal/include/generic/rte_atomic.h
> >>index 0e639dad76a4..38c3b41f9c68 100644
> >>--- a/lib/eal/include/generic/rte_atomic.h
> >>+++ b/lib/eal/include/generic/rte_atomic.h
> >>@@ -207,11 +207,11 @@ rte_atomic16_cmpset(volatile uint16_t *dst, uint16_t exp, uint16_t src)
> >>   *   The original value at that location
> >>   */
> >>  static inline uint16_t
> >>-rte_atomic16_exchange(volatile uint16_t *dst, uint16_t val);
> >>+rte_atomic16_exchange(volatile RTE_ATOMIC(uint16_t) *dst, uint16_t val);
> >
> >the existing rte_atomicNN (the old non-standard ones) are deprecated and will
> >be eventually removed so there isn't a lot of value in churning their
> >signatures to wrap the rte_stdatomic macros.
> >
> >the right thing to do here to just change the calling code to use the generic
> >rte_stdatomic macros directly so we can eventually remove
> >rte_atomicNN_xxx.
> >
> >ty
> >
> Hi, Tyler Retzlaff,
> 
> Thank you for your review.
> 
> As I understand it, this code is used to test the API
> rte_atomXXX_change, and the call here should not be modified.
> 
> Since the current problem affects compilation, I think it can be
> solved first.

okay, i understand the motivation now and see what you mean.

first, sorry for the trouble i did not expect anyone to start using this
option until i had completed full conversion of the tree.  drivers and
tests are still on my todo list.

for now would it be reasonable to just stop building this test when
enable_stdatomic=true? the api are still going to be tested by the ci
and builds that do not enable the option.

as for changing the signatures of the existing api i don't strictly
object since the RTE_ATOMIC() macro expands empty for the non stdatomic
builds so isn't technically an api or abi change. but there may still be
some resistance to merging regardless.

wonder if anyone else has any suggestions here?

ty

> 
> What do you think?
> 
> Thanks,
> Jie Hai
> >.