> -----Original Message-----
> From: Tyler Retzlaff <roret...@linux.microsoft.com>
> Sent: Thursday, October 26, 2023 6:38 AM
> To: Ruifeng Wang <ruifeng.w...@arm.com>
> Cc: dev@dpdk.org; Akhil Goyal <gak...@marvell.com>; Anatoly Burakov
> <anatoly.bura...@intel.com>; Andrew Rybchenko
> <andrew.rybche...@oktetlabs.ru>; Bruce
> Richardson <bruce.richard...@intel.com>; Chenbo Xia <chenbo....@intel.com>;
> Ciara Power
> <ciara.po...@intel.com>; David Christensen <d...@linux.vnet.ibm.com>; David
> Hunt
> <david.h...@intel.com>; Dmitry Kozlyuk <dmitry.kozl...@gmail.com>; Dmitry
> Malloy
> <dmit...@microsoft.com>; Elena Agostini <eagost...@nvidia.com>; Erik Gabriel
> Carrillo
> <erik.g.carri...@intel.com>; Fan Zhang <fanzhang....@gmail.com>; Ferruh Yigit
> <ferruh.yi...@amd.com>; Harman Kalra <hka...@marvell.com>; Harry van Haaren
> <harry.van.haa...@intel.com>; Honnappa Nagarahalli
> <honnappa.nagaraha...@arm.com>;
> jer...@marvell.com; Konstantin Ananyev <konstantin.v.anan...@yandex.ru>;
> Matan Azrad
> <ma...@nvidia.com>; Maxime Coquelin <maxime.coque...@redhat.com>; Narcisa Ana
> Maria Vasile
> <navas...@linux.microsoft.com>; Nicolas Chautru <nicolas.chau...@intel.com>;
> Olivier Matz
> <olivier.m...@6wind.com>; Ori Kam <or...@nvidia.com>; Pallavi Kadam
> <pallavi.ka...@intel.com>; Pavan Nikhilesh <pbhagavat...@marvell.com>; Reshma
> Pattan
> <reshma.pat...@intel.com>; Sameh Gobriel <sameh.gobr...@intel.com>; Shijith
> Thotton
> <sthot...@marvell.com>; Sivaprasad Tummala <sivaprasad.tumm...@amd.com>;
> Stephen Hemminger
> <step...@networkplumber.org>; Suanming Mou <suanmi...@nvidia.com>; Sunil
> Kumar Kori
> <sk...@marvell.com>; tho...@monjalon.net; Viacheslav Ovsiienko
> <viachesl...@nvidia.com>;
> Vladimir Medvedkin <vladimir.medved...@intel.com>; Yipeng Wang
> <yipeng1.w...@intel.com>;
> nd <n...@arm.com>
> Subject: Re: [PATCH v2 09/19] rcu: use rte optional stdatomic API
>
> On Wed, Oct 25, 2023 at 09:41:22AM +0000, Ruifeng Wang wrote:
> > > -----Original Message-----
> > > From: Tyler Retzlaff <roret...@linux.microsoft.com>
> > > Sent: Wednesday, October 18, 2023 4:31 AM
> > > To: dev@dpdk.org
> > > Cc: Akhil Goyal <gak...@marvell.com>; Anatoly Burakov
> > > <anatoly.bura...@intel.com>; Andrew Rybchenko
> > > <andrew.rybche...@oktetlabs.ru>; Bruce Richardson
> > > <bruce.richard...@intel.com>; Chenbo Xia <chenbo....@intel.com>;
> > > Ciara Power <ciara.po...@intel.com>; David Christensen
> > > <d...@linux.vnet.ibm.com>; David Hunt <david.h...@intel.com>; Dmitry
> > > Kozlyuk <dmitry.kozl...@gmail.com>; Dmitry Malloy
> > > <dmit...@microsoft.com>; Elena Agostini <eagost...@nvidia.com>; Erik
> > > Gabriel Carrillo <erik.g.carri...@intel.com>; Fan Zhang
> > > <fanzhang....@gmail.com>; Ferruh Yigit <ferruh.yi...@amd.com>;
> > > Harman Kalra <hka...@marvell.com>; Harry van Haaren
> > > <harry.van.haa...@intel.com>; Honnappa Nagarahalli
> > > <honnappa.nagaraha...@arm.com>; jer...@marvell.com; Konstantin
> > > Ananyev <konstantin.v.anan...@yandex.ru>; Matan Azrad
> > > <ma...@nvidia.com>; Maxime Coquelin <maxime.coque...@redhat.com>;
> > > Narcisa Ana Maria Vasile <navas...@linux.microsoft.com>; Nicolas
> > > Chautru <nicolas.chau...@intel.com>; Olivier Matz
> > > <olivier.m...@6wind.com>; Ori Kam <or...@nvidia.com>; Pallavi Kadam
> > > <pallavi.ka...@intel.com>; Pavan Nikhilesh
> > > <pbhagavat...@marvell.com>; Reshma Pattan <reshma.pat...@intel.com>;
> > > Sameh Gobriel <sameh.gobr...@intel.com>; Shijith Thotton
> > > <sthot...@marvell.com>; Sivaprasad Tummala
> > > <sivaprasad.tumm...@amd.com>; Stephen Hemminger
> > > <step...@networkplumber.org>; Suanming Mou <suanmi...@nvidia.com>;
> > > Sunil Kumar Kori <sk...@marvell.com>; tho...@monjalon.net;
> > > Viacheslav Ovsiienko <viachesl...@nvidia.com>; Vladimir Medvedkin
> > > <vladimir.medved...@intel.com>; Yipeng Wang
> > > <yipeng1.w...@intel.com>; Tyler Retzlaff
> > > <roret...@linux.microsoft.com>
> > > Subject: [PATCH v2 09/19] rcu: use rte optional stdatomic API
> > >
> > > Replace the use of gcc builtin __atomic_xxx intrinsics with
> > > corresponding rte_atomic_xxx optional stdatomic API
> > >
> > > Signed-off-by: Tyler Retzlaff <roret...@linux.microsoft.com>
> > > ---
> > > lib/rcu/rte_rcu_qsbr.c | 48 +++++++++++++++++------------------
> > > lib/rcu/rte_rcu_qsbr.h | 68
> > > +++++++++++++++++++++++++-------------------------
> > > 2 files changed, 58 insertions(+), 58 deletions(-)
> > >
> > > diff --git a/lib/rcu/rte_rcu_qsbr.c b/lib/rcu/rte_rcu_qsbr.c index
> > > 17be93e..4dc7714 100644
> > > --- a/lib/rcu/rte_rcu_qsbr.c
> > > +++ b/lib/rcu/rte_rcu_qsbr.c
> > > @@ -102,21 +102,21 @@
> > > * go out of sync. Hence, additional checks are required.
> > > */
> > > /* Check if the thread is already registered */
> > > - old_bmap = __atomic_load_n(__RTE_QSBR_THRID_ARRAY_ELM(v, i),
> > > - __ATOMIC_RELAXED);
> > > + old_bmap = rte_atomic_load_explicit(__RTE_QSBR_THRID_ARRAY_ELM(v, i),
> > > + rte_memory_order_relaxed);
> > > if (old_bmap & 1UL << id)
> > > return 0;
> > >
> > > do {
> > > new_bmap = old_bmap | (1UL << id);
> > > - success = __atomic_compare_exchange(
> > > + success = rte_atomic_compare_exchange_strong_explicit(
> > > __RTE_QSBR_THRID_ARRAY_ELM(v, i),
> > > - &old_bmap, &new_bmap, 0,
> > > - __ATOMIC_RELEASE, __ATOMIC_RELAXED);
> > > + &old_bmap, new_bmap,
> > > + rte_memory_order_release,
> > > rte_memory_order_relaxed);
> > >
> > > if (success)
> > > - __atomic_fetch_add(&v->num_threads,
> > > - 1, __ATOMIC_RELAXED);
> > > + rte_atomic_fetch_add_explicit(&v->num_threads,
> > > + 1, rte_memory_order_relaxed);
> > > else if (old_bmap & (1UL << id))
> > > /* Someone else registered this thread.
> > > * Counter should not be incremented.
> > > @@ -154,8 +154,8 @@
> > > * go out of sync. Hence, additional checks are required.
> > > */
> > > /* Check if the thread is already unregistered */
> > > - old_bmap = __atomic_load_n(__RTE_QSBR_THRID_ARRAY_ELM(v, i),
> > > - __ATOMIC_RELAXED);
> > > + old_bmap = rte_atomic_load_explicit(__RTE_QSBR_THRID_ARRAY_ELM(v, i),
> > > + rte_memory_order_relaxed);
> > > if (!(old_bmap & (1UL << id)))
> > > return 0;
> > >
> > > @@ -165,14 +165,14 @@
> > > * completed before removal of the thread from the list of
> > > * reporting threads.
> > > */
> > > - success = __atomic_compare_exchange(
> > > + success = rte_atomic_compare_exchange_strong_explicit(
> > > __RTE_QSBR_THRID_ARRAY_ELM(v, i),
> > > - &old_bmap, &new_bmap, 0,
> > > - __ATOMIC_RELEASE, __ATOMIC_RELAXED);
> > > + &old_bmap, new_bmap,
> > > + rte_memory_order_release,
> > > rte_memory_order_relaxed);
> > >
> > > if (success)
> > > - __atomic_fetch_sub(&v->num_threads,
> > > - 1, __ATOMIC_RELAXED);
> > > + rte_atomic_fetch_sub_explicit(&v->num_threads,
> > > + 1, rte_memory_order_relaxed);
> > > else if (!(old_bmap & (1UL << id)))
> > > /* Someone else unregistered this thread.
> > > * Counter should not be incremented.
> > > @@ -227,8 +227,8 @@
> > >
> > > fprintf(f, " Registered thread IDs = ");
> > > for (i = 0; i < v->num_elems; i++) {
> > > - bmap = __atomic_load_n(__RTE_QSBR_THRID_ARRAY_ELM(v, i),
> > > - __ATOMIC_ACQUIRE);
> > > + bmap = rte_atomic_load_explicit(__RTE_QSBR_THRID_ARRAY_ELM(v,
> > > i),
> > > + rte_memory_order_acquire);
> > > id = i << __RTE_QSBR_THRID_INDEX_SHIFT;
> > > while (bmap) {
> > > t = __builtin_ctzl(bmap);
> > > @@ -241,26 +241,26 @@
> > > fprintf(f, "\n");
> > >
> > > fprintf(f, " Token = %" PRIu64 "\n",
> > > - __atomic_load_n(&v->token, __ATOMIC_ACQUIRE));
> > > + rte_atomic_load_explicit(&v->token,
> > > rte_memory_order_acquire));
> > >
> > > fprintf(f, " Least Acknowledged Token = %" PRIu64 "\n",
> > > - __atomic_load_n(&v->acked_token, __ATOMIC_ACQUIRE));
> > > + rte_atomic_load_explicit(&v->acked_token,
> > > +rte_memory_order_acquire));
> > >
> > > fprintf(f, "Quiescent State Counts for readers:\n");
> > > for (i = 0; i < v->num_elems; i++) {
> > > - bmap = __atomic_load_n(__RTE_QSBR_THRID_ARRAY_ELM(v, i),
> > > - __ATOMIC_ACQUIRE);
> > > + bmap = rte_atomic_load_explicit(__RTE_QSBR_THRID_ARRAY_ELM(v,
> > > i),
> > > + rte_memory_order_acquire);
> > > id = i << __RTE_QSBR_THRID_INDEX_SHIFT;
> > > while (bmap) {
> > > t = __builtin_ctzl(bmap);
> > > fprintf(f, "thread ID = %u, count = %" PRIu64 ", lock
> > > count = %u\n",
> > > id + t,
> > > - __atomic_load_n(
> > > + rte_atomic_load_explicit(
> > > &v->qsbr_cnt[id + t].cnt,
> > > - __ATOMIC_RELAXED),
> > > - __atomic_load_n(
> > > + rte_memory_order_relaxed),
> > > + rte_atomic_load_explicit(
> > > &v->qsbr_cnt[id + t].lock_cnt,
> > > - __ATOMIC_RELAXED));
> > > + rte_memory_order_relaxed));
> > > bmap &= ~(1UL << t);
> > > }
> > > }
> > > diff --git a/lib/rcu/rte_rcu_qsbr.h b/lib/rcu/rte_rcu_qsbr.h index
> > > 87e1b55..9f4aed2 100644
> > > --- a/lib/rcu/rte_rcu_qsbr.h
> > > +++ b/lib/rcu/rte_rcu_qsbr.h
> > > @@ -63,11 +63,11 @@
> > > * Given thread id needs to be converted to index into the array and
> > > * the id within the array element.
> > > */
> > > -#define __RTE_QSBR_THRID_ARRAY_ELM_SIZE (sizeof(uint64_t) * 8)
> > > +#define __RTE_QSBR_THRID_ARRAY_ELM_SIZE
> > > +(sizeof(RTE_ATOMIC(uint64_t)) *
> > > +8)
> > > #define __RTE_QSBR_THRID_ARRAY_SIZE(max_threads) \
> > > RTE_ALIGN(RTE_ALIGN_MUL_CEIL(max_threads, \
> > > __RTE_QSBR_THRID_ARRAY_ELM_SIZE) >> 3, RTE_CACHE_LINE_SIZE)
> > > -#define __RTE_QSBR_THRID_ARRAY_ELM(v, i) ((uint64_t *) \
> > > +#define __RTE_QSBR_THRID_ARRAY_ELM(v, i) ((uint64_t __rte_atomic *)
> > > +\
> >
> > Is it equivalent to ((RTE_ATOMIC(uint64_t) *)?
>
> i'm not sure if you're asking about the resultant type of the expression or
> not?
I see other places are using specifier hence the question.
>
> in this context we aren't specifying an atomic type but rather adding the
> atomic qualifier
> to what should already be a variable that has an atomic specified type with a
> cast which
> is why we use __rte_atomic.
I read from document [1] that atomic qualified type may have a different size
from the original type.
If that is the case, the size difference could cause issue in bitmap array
accessing.
Did I misunderstand?
[1] https://en.cppreference.com/w/c/language/atomic
>
> >
> > > ((struct rte_rcu_qsbr_cnt *)(v + 1) + v->max_threads) + i)
> > > #define __RTE_QSBR_THRID_INDEX_SHIFT 6 #define
> > > __RTE_QSBR_THRID_MASK 0x3f @@ -75,13 +75,13 @@
> > >
> >
> > <snip>
