From: Olivier Matz <olivier.matz@6wind.com>
To: Joyce Kong <Joyce.Kong@arm.com>
Cc: "thomas@monjalon.net" <thomas@monjalon.net>,
"david.marchand@redhat.com" <david.marchand@redhat.com>,
"roretzla@linux.microsoft.com" <roretzla@linux.microsoft.com>,
"stephen@networkplumber.org" <stephen@networkplumber.org>,
"andrew.rybchenko@oktetlabs.ru" <andrew.rybchenko@oktetlabs.ru>,
"harry.van.haaren@intel.com" <harry.van.haaren@intel.com>,
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
Ruifeng Wang <Ruifeng.Wang@arm.com>,
"dev@dpdk.org" <dev@dpdk.org>, nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH v3 4/8] test/mcslock: use compiler atomics for lcores sync
Date: Thu, 29 Jul 2021 09:58:14 +0200 [thread overview]
Message-ID: <YQJflq66PoZeWm9n@platinum> (raw)
In-Reply-To: <AS8PR08MB69357E192881A89EC8A45DB492EB9@AS8PR08MB6935.eurprd08.prod.outlook.com>
On Thu, Jul 29, 2021 at 07:19:13AM +0000, Joyce Kong wrote:
> Hi Olivier,
>
> > -----Original Message-----
> > From: Olivier Matz <olivier.matz@6wind.com>
> > Sent: Wednesday, July 28, 2021 5:57 PM
> > To: Joyce Kong <Joyce.Kong@arm.com>
> > Cc: thomas@monjalon.net; david.marchand@redhat.com;
> > roretzla@linux.microsoft.com; stephen@networkplumber.org;
> > andrew.rybchenko@oktetlabs.ru; harry.van.haaren@intel.com; Honnappa
> > Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> > <Ruifeng.Wang@arm.com>; dev@dpdk.org; nd <nd@arm.com>
> > Subject: Re: [PATCH v3 4/8] test/mcslock: use compiler atomics for lcores
> > sync
> >
> > Hi Joyce,
> >
> > On Mon, Jul 19, 2021 at 10:51:21PM -0500, Joyce Kong wrote:
> > > Convert rte_atomic usages to compiler atomic built-ins for lcores sync
> > > in mcslock testcases.
> > >
> > > Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> > > ---
> > > app/test/test_mcslock.c | 14 ++++++--------
> > > 1 file changed, 6 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/app/test/test_mcslock.c b/app/test/test_mcslock.c index
> > > 80eaecc90a..52e45e7e2a 100644
> > > --- a/app/test/test_mcslock.c
> > > +++ b/app/test/test_mcslock.c
> > > @@ -17,7 +17,6 @@
> > > #include <rte_lcore.h>
> > > #include <rte_cycles.h>
> > > #include <rte_mcslock.h>
> > > -#include <rte_atomic.h>
> > >
> > > #include "test.h"
> > >
> > > @@ -43,7 +42,7 @@ rte_mcslock_t *p_ml_perf;
> > >
> > > static unsigned int count;
> > >
> > > -static rte_atomic32_t synchro;
> > > +static uint32_t synchro;
> > >
> > > static int
> > > test_mcslock_per_core(__rte_unused void *arg) @@ -76,8 +75,7 @@
> > > load_loop_fn(void *func_param)
> > > rte_mcslock_t ml_perf_me;
> > >
> > > /* wait synchro */
> > > - while (rte_atomic32_read(&synchro) == 0)
> > > - ;
> > > + rte_wait_until_equal_32(&synchro, 1, __ATOMIC_RELAXED);
> > >
> > > begin = rte_get_timer_cycles();
> > > while (lcount < MAX_LOOP) {
> > > @@ -102,15 +100,15 @@ test_mcslock_perf(void)
> > > const unsigned int lcore = rte_lcore_id();
> > >
> > > printf("\nTest with no lock on single core...\n");
> > > - rte_atomic32_set(&synchro, 1);
> > > + __atomic_store_n(&synchro, 1, __ATOMIC_RELAXED);
> > > load_loop_fn(&lock);
> > > printf("Core [%u] Cost Time = %"PRIu64" us\n",
> > > lcore, time_count[lcore]);
> > > memset(time_count, 0, sizeof(time_count));
> > >
> > > printf("\nTest with lock on single core...\n");
> > > + __atomic_store_n(&synchro, 1, __ATOMIC_RELAXED);
> > > lock = 1;
> > > - rte_atomic32_set(&synchro, 1);
> >
> > nit: is there a reason for moving this line?
>
> I meant to use __atomic_store_n() instead of rte_atomic32_set() to set synchro,
> but put the operation to the line up 'lock=1' by mistake, will change it.
>
> > >
> > > load_loop_fn(&lock);
> > > printf("Core [%u] Cost Time = %"PRIu64" us\n",
> > > lcore, time_count[lcore]);
> > > @@ -118,11 +116,11 @@ test_mcslock_perf(void)
> > >
> > > printf("\nTest with lock on %u cores...\n", (rte_lcore_count()));
> > >
> > > - rte_atomic32_set(&synchro, 0);
> > > + __atomic_store_n(&synchro, 0, __ATOMIC_RELAXED);
> > > rte_eal_mp_remote_launch(load_loop_fn, &lock, SKIP_MAIN);
> > >
> > > /* start synchro and launch test on main */
> > > - rte_atomic32_set(&synchro, 1);
> > > + __atomic_store_n(&synchro, 1, __ATOMIC_RELAXED);
> > > load_loop_fn(&lock);
> >
> > I have a more general question. Please forgive my ignorance about the
> > C++11 atomic builtins and memory model. Both gcc manual and C11
> > standard
> > are not that easy to understand :)
> >
> > In all the patches of this patchset, __ATOMIC_RELAXED is used. My
> > understanding is that it does not add any inter-thread ordering constraint. I
> > suppose that in this particular case, we rely on the call to
> > rte_eal_mp_remote_launch() being a compiler barrier, and the function itself
> > to be a memory barrier. This ensures that worker threads sees synchro=0
> > until it is set to 1 by the master.
> > Is it correct?
> >
>
> Yes, you are right. __ATOMIC_RELAXED would introduce no barrier, and the worker
> threads would sync with master thread by 'synchro'.
>
> > What is the reason for using the atomic API here? Wouldn't a standard
> > affectation work too? (I mean "synchro = 1;")
> >
>
> Here, __atomic_store_n(__ATOMIC_RELAXED) is used to ensure worker threads
> see 'synchro=1' after it is changed by the master. And a standard affection can not
> ensure worker threads get the new value.
So, if I understand correctly, using __atomic_store() acts as if the variable is
volatile, and this is indeed needed to ensure visibility from other worker
threads.
I did some tests to convince myself: https://godbolt.org/z/3qWYeneGf
Thank you for the clarification.
> >
> > >
> > > rte_eal_mp_wait_lcore();
> > > --
> > > 2.17.1
> > >
next prev parent reply other threads:[~2021-07-29 7:58 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-04 9:46 [dpdk-dev] [PATCH v1 0/8] use GCC's C11 atomic builtins for test Joyce Kong
2021-06-04 9:46 ` [dpdk-dev] [PATCH v1 1/8] test/ticketlock: use GCC atomic builtins for lcores sync Joyce Kong
2021-06-04 9:46 ` [dpdk-dev] [PATCH v1 2/8] test/spinlock: " Joyce Kong
2021-06-04 9:46 ` [dpdk-dev] [PATCH v1 3/8] test/rwlock: " Joyce Kong
2021-06-04 9:46 ` [dpdk-dev] [PATCH v1 4/8] test/mcslock: " Joyce Kong
2021-06-04 9:46 ` [dpdk-dev] [PATCH v1 5/8] test/mempool: remove unused variable " Joyce Kong
2021-06-04 9:46 ` [dpdk-dev] [PATCH v1 6/8] test/mempool_perf: use GCC atomic builtins " Joyce Kong
2021-06-04 9:46 ` [dpdk-dev] [PATCH v1 7/8] test/service_cores: use GCC atomic builtins for lock sync Joyce Kong
2021-06-04 9:46 ` [dpdk-dev] [PATCH v1 8/8] test/rcu_perf: use GCC atomic builtins for data sync Joyce Kong
2021-06-04 19:57 ` [dpdk-dev] [PATCH v1 0/8] use GCC's C11 atomic builtins for test Stephen Hemminger
2021-06-11 8:40 ` David Marchand
2021-06-11 10:45 ` Joyce Kong
2021-06-16 2:54 ` [dpdk-dev] [PATCH v2 " Joyce Kong
2021-06-16 2:54 ` [dpdk-dev] [PATCH v2 1/8] test/ticketlock: use GCC atomic builtins for lcores sync Joyce Kong
2021-06-16 2:54 ` [dpdk-dev] [PATCH v2 2/8] test/spinlock: " Joyce Kong
2021-06-16 2:54 ` [dpdk-dev] [PATCH v2 3/8] test/rwlock: " Joyce Kong
2021-06-16 2:54 ` [dpdk-dev] [PATCH v2 4/8] test/mcslock: " Joyce Kong
2021-06-16 2:54 ` [dpdk-dev] [PATCH v2 5/8] test/mempool: remove unused variable " Joyce Kong
2021-06-16 2:54 ` [dpdk-dev] [PATCH v2 6/8] test/mempool_perf: use GCC atomic builtins " Joyce Kong
2021-06-16 2:54 ` [dpdk-dev] [PATCH v2 7/8] test/service_cores: use GCC atomic builtins for lock sync Joyce Kong
2021-06-16 2:54 ` [dpdk-dev] [PATCH v2 8/8] test/rcu: use GCC atomic builtins for data sync Joyce Kong
2021-06-17 15:21 ` [dpdk-dev] [PATCH v2 0/8] use GCC's C11 atomic builtins for test Tyler Retzlaff
2021-06-17 23:26 ` Honnappa Nagarahalli
2021-06-23 10:24 ` Thomas Monjalon
2021-06-23 16:02 ` Tyler Retzlaff
2021-06-29 17:04 ` Honnappa Nagarahalli
2021-06-30 18:51 ` Tyler Retzlaff
2021-06-30 19:06 ` Honnappa Nagarahalli
2021-06-30 19:38 ` Tyler Retzlaff
2021-06-30 20:25 ` Honnappa Nagarahalli
2021-06-30 21:49 ` Tyler Retzlaff
2021-06-30 22:41 ` Honnappa Nagarahalli
2021-07-13 7:28 ` Joyce Kong
2021-07-14 11:44 ` Thomas Monjalon
2021-07-20 3:51 ` [dpdk-dev] [PATCH v3 0/8] use compiler " Joyce Kong
2021-07-20 3:51 ` [dpdk-dev] [PATCH v3 1/8] test/ticketlock: use compiler atomics for lcores sync Joyce Kong
2021-07-20 3:51 ` [dpdk-dev] [PATCH v3 2/8] test/spinlock: use compile " Joyce Kong
2021-07-20 3:51 ` [dpdk-dev] [PATCH v3 3/8] test/rwlock: use compiler " Joyce Kong
2021-07-20 3:51 ` [dpdk-dev] [PATCH v3 4/8] test/mcslock: " Joyce Kong
2021-07-28 9:56 ` Olivier Matz
2021-07-29 7:19 ` Joyce Kong
2021-07-29 7:58 ` Olivier Matz [this message]
2021-07-20 3:51 ` [dpdk-dev] [PATCH v3 5/8] test/mempool: remove unused variable " Joyce Kong
2021-07-20 3:51 ` [dpdk-dev] [PATCH v3 6/8] test/mempool_perf: use compiler atomics " Joyce Kong
2021-07-20 3:51 ` [dpdk-dev] [PATCH v3 7/8] test/service_cores: use compiler atomics for lock sync Joyce Kong
2021-07-20 3:51 ` [dpdk-dev] [PATCH v3 8/8] test/rcu: use compiler atomics for data sync Joyce Kong
2021-07-23 19:52 ` Andrew Rybchenko
2021-07-28 7:07 ` Joyce Kong
2021-07-30 21:58 ` [dpdk-dev] [PATCH v3 0/8] use compiler atomic builtins for test Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YQJflq66PoZeWm9n@platinum \
--to=olivier.matz@6wind.com \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=Joyce.Kong@arm.com \
--cc=Ruifeng.Wang@arm.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=harry.van.haaren@intel.com \
--cc=nd@arm.com \
--cc=roretzla@linux.microsoft.com \
--cc=stephen@networkplumber.org \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.