From: "Carrillo, Erik G" <erik.g.carrillo@intel.com>
To: Phil Yang <Phil.Yang@arm.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: "drc@linux.vnet.ibm.com" <drc@linux.vnet.ibm.com>,
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
Ruifeng Wang <Ruifeng.Wang@arm.com>,
"Dharmik Thakkar" <Dharmik.Thakkar@arm.com>, nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH 3/3] eventdev: relax smp barriers with c11 atomics
Date: Mon, 29 Jun 2020 18:07:29 +0000 [thread overview]
Message-ID: <CY4PR1101MB21183397FD2BD3008947BB08B96E0@CY4PR1101MB2118.namprd11.prod.outlook.com> (raw)
In-Reply-To: <VE1PR08MB46407509F18B1D5C67941516E9910@VE1PR08MB4640.eurprd08.prod.outlook.com>
> -----Original Message-----
> From: Phil Yang <Phil.Yang@arm.com>
> Sent: Sunday, June 28, 2020 12:34 PM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; dev@dpdk.org
> Cc: drc@linux.vnet.ibm.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Dharmik Thakkar
> <Dharmik.Thakkar@arm.com>; nd <nd@arm.com>
> Subject: RE: [dpdk-dev] [PATCH 3/3] eventdev: relax smp barriers with c11
> atomics
>
> Hi Erick,
>
> > -----Original Message-----
> > From: Carrillo, Erik G <erik.g.carrillo@intel.com>
> > Sent: Wednesday, June 24, 2020 3:39 AM
> > To: Phil Yang <Phil.Yang@arm.com>; dev@dpdk.org
> > Cc: drc@linux.vnet.ibm.com; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>;
> > Dharmik Thakkar <Dharmik.Thakkar@arm.com>; nd <nd@arm.com>
> > Subject: RE: [dpdk-dev] [PATCH 3/3] eventdev: relax smp barriers with
> > c11 atomics
> >
> > Hi Phil,
> >
> > Comment in-line:
> >
> > > -----Original Message-----
> > > From: Phil Yang <Phil.Yang@arm.com>
> > > Sent: Monday, June 22, 2020 5:12 AM
> > > To: Phil Yang <Phil.Yang@arm.com>; dev@dpdk.org; Carrillo, Erik G
> > > <erik.g.carrillo@intel.com>
> > > Cc: drc@linux.vnet.ibm.com; Honnappa Nagarahalli
> > > <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>;
> > > Dharmik Thakkar <Dharmik.Thakkar@arm.com>; nd <nd@arm.com>
> > > Subject: RE: [dpdk-dev] [PATCH 3/3] eventdev: relax smp barriers
> > > with c11 atomics
> > >
> > > > -----Original Message-----
> > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Phil Yang
> > > > Sent: Friday, June 12, 2020 7:20 PM
> > > > To: dev@dpdk.org; erik.g.carrillo@intel.com
> > > > Cc: drc@linux.vnet.ibm.com; Honnappa Nagarahalli
> > > > <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> > > <Ruifeng.Wang@arm.com>;
> > > > Dharmik Thakkar <Dharmik.Thakkar@arm.com>; nd <nd@arm.com>
> > > > Subject: [dpdk-dev] [PATCH 3/3] eventdev: relax smp barriers with
> > > > c11 atomics
> > > >
> > > > The implementation-specific opaque data is shared between arm and
> > > > cancel operations. The state flag acts as a guard variable to make
> > > > sure the update of opaque data is synchronized. This patch uses
> > > > c11 atomics with explicit one way memory barrier instead of full
> > > > barriers
> > > > rte_smp_w/rmb() to synchronize the opaque data between timer arm
> > and
> > > cancel threads.
> > > >
> > > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > > Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > ---
> > > > lib/librte_eventdev/rte_event_timer_adapter.c | 55
> > > > ++++++++++++++++++---------
> > > > lib/librte_eventdev/rte_event_timer_adapter.h | 2 +-
> > > > 2 files changed, 38 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/lib/librte_eventdev/rte_event_timer_adapter.c
> > > > b/lib/librte_eventdev/rte_event_timer_adapter.c
> > > > index 6947efb..0a26501 100644
> > > > --- a/lib/librte_eventdev/rte_event_timer_adapter.c
> > > > +++ b/lib/librte_eventdev/rte_event_timer_adapter.c
> > > > @@ -629,7 +629,8 @@ swtim_callback(struct rte_timer *tim)
> > > > sw->expired_timers[sw->n_expired_timers++] = tim;
> > > > sw->stats.evtim_exp_count++;
> > > >
> > > > - evtim->state = RTE_EVENT_TIMER_NOT_ARMED;
> > > > + __atomic_store_n(&evtim->state,
> > > > RTE_EVENT_TIMER_NOT_ARMED,
> > > > + __ATOMIC_RELEASE);
> > > > }
> > > >
> > > > if (event_buffer_batch_ready(&sw->buffer)) { @@ -1020,6 +1021,7
> > > @@
> > > > __swtim_arm_burst(const struct rte_event_timer_adapter *adapter,
> > > > int n_lcores;
> > > > /* Timer is not armed state */
> > > > int16_t exp_state = 0;
> > > > + enum rte_event_timer_state n_state;
> > > >
> > > > #ifdef RTE_LIBRTE_EVENTDEV_DEBUG
> > > > /* Check that the service is running. */ @@ -1060,30 +1062,36 @@
> > > > __swtim_arm_burst(const struct rte_event_timer_adapter *adapter,
> > > > }
> > > >
> > > > for (i = 0; i < nb_evtims; i++) {
> > > > - /* Don't modify the event timer state in these cases */
> > > > - if (evtims[i]->state == RTE_EVENT_TIMER_ARMED) {
> > > > + n_state = __atomic_load_n(&evtims[i]->state,
> > > > __ATOMIC_RELAXED);
> > > > + if (n_state == RTE_EVENT_TIMER_ARMED) {
> > > > rte_errno = EALREADY;
> > > > break;
> > > > - } else if (!(evtims[i]->state ==
> > > > RTE_EVENT_TIMER_NOT_ARMED ||
> > > > - evtims[i]->state ==
> > > > RTE_EVENT_TIMER_CANCELED)) {
> > > > + } else if (!(n_state == RTE_EVENT_TIMER_NOT_ARMED ||
> > > > + n_state == RTE_EVENT_TIMER_CANCELED)) {
> > > > rte_errno = EINVAL;
> > > > break;
> > > > }
> > > >
> > > > ret = check_timeout(evtims[i], adapter);
> > > > if (unlikely(ret == -1)) {
> > > > - evtims[i]->state =
> > > > RTE_EVENT_TIMER_ERROR_TOOLATE;
> > > > + __atomic_store_n(&evtims[i]->state,
> > > > +
> > > RTE_EVENT_TIMER_ERROR_TOOLATE,
> > > > + __ATOMIC_RELAXED);
> > > > rte_errno = EINVAL;
> > > > break;
> > > > } else if (unlikely(ret == -2)) {
> > > > - evtims[i]->state =
> > > > RTE_EVENT_TIMER_ERROR_TOOEARLY;
> > > > + __atomic_store_n(&evtims[i]->state,
> > > > +
> > > > RTE_EVENT_TIMER_ERROR_TOOEARLY,
> > > > + __ATOMIC_RELAXED);
> > > > rte_errno = EINVAL;
> > > > break;
> > > > }
> > > >
> > > > if (unlikely(check_destination_event_queue(evtims[i],
> > > > adapter) < 0)) {
> > > > - evtims[i]->state = RTE_EVENT_TIMER_ERROR;
> > > > + __atomic_store_n(&evtims[i]->state,
> > > > + RTE_EVENT_TIMER_ERROR,
> > > > + __ATOMIC_RELAXED);
> > > > rte_errno = EINVAL;
> > > > break;
> > > > }
> > > > @@ -1099,13 +1107,18 @@ __swtim_arm_burst(const struct
> > > > rte_event_timer_adapter *adapter,
> > > > SINGLE, lcore_id, NULL, evtims[i]);
> > > > if (ret < 0) {
> > > > /* tim was in RUNNING or CONFIG state */
> > > > - evtims[i]->state = RTE_EVENT_TIMER_ERROR;
> > > > + __atomic_store_n(&evtims[i]->state,
> > > > + RTE_EVENT_TIMER_ERROR,
> > > > + __ATOMIC_RELEASE);
> > > > break;
> > > > }
> > > >
> > > > - rte_smp_wmb();
> > > > EVTIM_LOG_DBG("armed an event timer");
> > > > - evtims[i]->state = RTE_EVENT_TIMER_ARMED;
> > > > + /* RELEASE ordering guarantees the adapter specific value
> > > > + * changes observed before the update of state.
> > > > + */
> > > > + __atomic_store_n(&evtims[i]->state,
> > > > RTE_EVENT_TIMER_ARMED,
> > > > + __ATOMIC_RELEASE);
> > > > }
> > > >
> > > > if (i < nb_evtims)
> > > > @@ -1132,6 +1145,7 @@ swtim_cancel_burst(const struct
> > > > rte_event_timer_adapter *adapter,
> > > > struct rte_timer *timp;
> > > > uint64_t opaque;
> > > > struct swtim *sw = swtim_pmd_priv(adapter);
> > > > + enum rte_event_timer_state n_state;
> > > >
> > > > #ifdef RTE_LIBRTE_EVENTDEV_DEBUG
> > > > /* Check that the service is running. */ @@ -1143,16 +1157,18 @@
> > > > swtim_cancel_burst(const struct rte_event_timer_adapter *adapter,
> > > >
> > > > for (i = 0; i < nb_evtims; i++) {
> > > > /* Don't modify the event timer state in these cases */
> > > > - if (evtims[i]->state == RTE_EVENT_TIMER_CANCELED) {
> > > > + /* ACQUIRE ordering guarantees the access of
> > > > implementation
> > > > + * specific opague data under the correct state.
> > > > + */
> > > > + n_state = __atomic_load_n(&evtims[i]->state,
> > > > __ATOMIC_ACQUIRE);
> > > > + if (n_state == RTE_EVENT_TIMER_CANCELED) {
> > > > rte_errno = EALREADY;
> > > > break;
> > > > - } else if (evtims[i]->state != RTE_EVENT_TIMER_ARMED) {
> > > > + } else if (n_state != RTE_EVENT_TIMER_ARMED) {
> > > > rte_errno = EINVAL;
> > > > break;
> > > > }
> > > >
> > > > - rte_smp_rmb();
> > > > -
> > > > opaque = evtims[i]->impl_opaque[0];
> > > > timp = (struct rte_timer *)(uintptr_t)opaque;
> > > > RTE_ASSERT(timp != NULL);
> > > > @@ -1166,11 +1182,14 @@ swtim_cancel_burst(const struct
> > > > rte_event_timer_adapter *adapter,
> > > >
> > > > rte_mempool_put(sw->tim_pool, (void **)timp);
> > > >
> > > > - evtims[i]->state = RTE_EVENT_TIMER_CANCELED;
> > > > + __atomic_store_n(&evtims[i]->state,
> > > > RTE_EVENT_TIMER_CANCELED,
> > > > + __ATOMIC_RELAXED);
> > > > evtims[i]->impl_opaque[0] = 0;
> > > > evtims[i]->impl_opaque[1] = 0;
> > >
> > > Is that safe to remove impl_opaque cleanup above?
> > >
> > > Once the soft timer canceled, the __swtim_arm_burst cannot access
> > > these two fields under the RTE_EVENT_TIMER_CANCELED state.
> > > After new timer armed, it refills these two fields in the
> > > __swtim_arm_burst thread, which is the only producer of these two
> fields.
> > > I think the only risk is that the values of these two field might be
> > > unknow after swtim_cancel_burst.
> > > So it should be safe to remove them if no other thread access them
> > > after canceling the timer.
> > >
> > > Any comments on this?
> > > If we remove these two instructions, we can also remove the
> > > __atomic_thread_fence below to save performance penalty.
> > >
> > > Thanks,
> > > Phil
> > >
> >
> > In this case, I see the fence as (more importantly) ensuring the state
> update
> > is visible to other threads... do I misunderstand? I suppose we could also
> > update the state with an __atomic_store(..., __ATOMIC_RELEASE), but
> > perhaps that roughly equivalent?
>
> Yeah. In my understanding, the fence ensures the state and the
> implementation-specific opaque data update are visible between other
> timer arm and cancel threads.
> Actually, we only care about the state's value here.
> The atomic RELEASE can also make sure all writes in the current thread are
> visible in other threads that acquire the same atomic variable.
> So I think we can remove the fence and update the state with RELEASE then
> load the state with ACQUIRE in the timer arm and the cancel threads to
> achieve the same goal.
Ok, that sounds good to me.
Thanks,
Erik
>
> >
> > > > -
> > > > - rte_smp_wmb();
> > > > + /* The RELEASE fence make sure the clean up
> > > > + * of opaque data observed between threads.
> > > > + */
> > > > + __atomic_thread_fence(__ATOMIC_RELEASE);
> > > > }
> > > >
> > > > return i;
> > > > diff --git a/lib/librte_eventdev/rte_event_timer_adapter.h
> > > > b/lib/librte_eventdev/rte_event_timer_adapter.h
> > > > index d2ebcb0..6f64b90 100644
> > > > --- a/lib/librte_eventdev/rte_event_timer_adapter.h
> > > > +++ b/lib/librte_eventdev/rte_event_timer_adapter.h
> > > > @@ -467,7 +467,7 @@ struct rte_event_timer {
> > > > * - op: RTE_EVENT_OP_NEW
> > > > * - event_type: RTE_EVENT_TYPE_TIMER
> > > > */
> > > > - volatile enum rte_event_timer_state state;
> > > > + enum rte_event_timer_state state;
> > > > /**< State of the event timer. */
> > > > uint64_t timeout_ticks;
> > > > /**< Expiry timer ticks expressed in number of *timer_ticks_ns*
> > > from
> > > > --
> > > > 2.7.4
next prev parent reply other threads:[~2020-06-29 18:07 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-12 11:19 [dpdk-dev] [PATCH 1/3] eventdev: fix race condition on timer list counter Phil Yang
2020-06-12 11:19 ` [dpdk-dev] [PATCH 2/3] eventdev: use c11 atomics for lcore timer armed flag Phil Yang
2020-06-23 21:01 ` Carrillo, Erik G
2020-06-28 16:12 ` Phil Yang
2020-06-23 21:20 ` Stephen Hemminger
2020-06-23 21:31 ` Carrillo, Erik G
2020-06-28 16:32 ` Phil Yang
2020-06-12 11:19 ` [dpdk-dev] [PATCH 3/3] eventdev: relax smp barriers with c11 atomics Phil Yang
2020-06-22 10:12 ` Phil Yang
2020-06-23 19:38 ` Carrillo, Erik G
2020-06-28 17:33 ` Phil Yang
2020-06-29 18:07 ` Carrillo, Erik G [this message]
2020-06-18 15:17 ` [dpdk-dev] [PATCH 1/3] eventdev: fix race condition on timer list counter Carrillo, Erik G
2020-06-18 18:25 ` Honnappa Nagarahalli
2020-06-22 9:48 ` Phil Yang
2020-07-01 11:22 ` Jerin Jacob
2020-07-02 3:28 ` Phil Yang
2020-07-02 3:26 ` Phil Yang
2020-07-02 3:56 ` Honnappa Nagarahalli
2020-07-02 21:15 ` Carrillo, Erik G
2020-07-02 21:30 ` Honnappa Nagarahalli
2020-06-22 9:09 ` Phil Yang
2020-07-02 5:26 ` [dpdk-dev] [PATCH v2 1/4] " Phil Yang
2020-07-02 5:26 ` [dpdk-dev] [PATCH v2 2/4] eventdev: use c11 atomics for lcore timer armed flag Phil Yang
2020-07-02 20:21 ` Carrillo, Erik G
2020-07-02 5:26 ` [dpdk-dev] [PATCH v2 3/4] eventdev: remove redundant code Phil Yang
2020-07-03 3:35 ` Dharmik Thakkar
2020-07-02 5:26 ` [dpdk-dev] [PATCH v2 4/4] eventdev: relax smp barriers with c11 atomics Phil Yang
2020-07-02 20:30 ` Carrillo, Erik G
2020-07-03 10:50 ` Jerin Jacob
2020-07-06 10:04 ` Thomas Monjalon
2020-07-06 15:32 ` Phil Yang
2020-07-06 15:40 ` Thomas Monjalon
2020-07-07 11:13 ` [dpdk-dev] [PATCH v3 1/4] eventdev: fix race condition on timer list counter Phil Yang
2020-07-07 11:13 ` [dpdk-dev] [PATCH v3 2/4] eventdev: use C11 atomics for lcore timer armed flag Phil Yang
2020-07-07 11:13 ` [dpdk-dev] [PATCH v3 3/4] eventdev: remove redundant code Phil Yang
2020-07-07 11:13 ` [dpdk-dev] [PATCH v3 4/4] eventdev: relax smp barriers with C11 atomics Phil Yang
2020-07-07 14:29 ` Jerin Jacob
2020-07-07 15:56 ` Phil Yang
2020-07-07 15:54 ` [dpdk-dev] [PATCH v4 1/4] eventdev: fix race condition on timer list counter Phil Yang
2020-07-07 15:54 ` [dpdk-dev] [PATCH v4 2/4] eventdev: use C11 atomics for lcore timer armed flag Phil Yang
2020-07-07 15:54 ` [dpdk-dev] [PATCH v4 3/4] eventdev: remove redundant code Phil Yang
2020-07-07 15:54 ` [dpdk-dev] [PATCH v4 4/4] eventdev: relax smp barriers with C11 atomics Phil Yang
2020-07-08 13:30 ` [dpdk-dev] [PATCH v4 1/4] eventdev: fix race condition on timer list counter Jerin Jacob
2020-07-08 15:01 ` 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=CY4PR1101MB21183397FD2BD3008947BB08B96E0@CY4PR1101MB2118.namprd11.prod.outlook.com \
--to=erik.g.carrillo@intel.com \
--cc=Dharmik.Thakkar@arm.com \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=Phil.Yang@arm.com \
--cc=Ruifeng.Wang@arm.com \
--cc=dev@dpdk.org \
--cc=drc@linux.vnet.ibm.com \
--cc=nd@arm.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).