<snip>

quoted

quoted

quoted

quoted

Hi Phil,

Good catch - thanks for the fix.   I've commented in-line:

quoted

-----Original Message-----
From: Phil Yang <redacted>
Sent: Friday, June 12, 2020 6:20 AM
To: dev@dpdk.org; Carrillo, Erik G <redacted>
Cc: drc@linux.vnet.ibm.com; honnappa.nagarahalli@arm.com;
ruifeng.wang@arm.com; dharmik.thakkar@arm.com; nd@arm.com;
stable@dpdk.org
Subject: [PATCH 1/3] eventdev: fix race condition on timer
list counter

The n_poll_lcores counter and poll_lcore array are shared
between lcores and the update of these variables are out of
the protection of spinlock on each lcore timer list. The
read-modify-write operations of the counter are not atomic, so
it has the potential of race condition

between lcores.

quoted

Use c11 atomics with RELAXED ordering to prevent confliction.

Fixes: cc7b73ea9e3b ("eventdev: add new software timer
adapter")
Cc: erik.g.carrillo@intel.com
Cc: stable@dpdk.org

Signed-off-by: Phil Yang <redacted>
Reviewed-by: Dharmik Thakkar <redacted>
Reviewed-by: Ruifeng Wang <redacted>
---
 lib/librte_eventdev/rte_event_timer_adapter.c | 16
++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eventdev/rte_event_timer_adapter.c

b/lib/librte_eventdev/rte_event_timer_adapter.c
index 005459f..6a0e283 100644

--- a/lib/librte_eventdev/rte_event_timer_adapter.c
+++ b/lib/librte_eventdev/rte_event_timer_adapter.c

@@ -583,6 +583,7 @@ swtim_callback(struct rte_timer *tim)

uint16_t nb_evs_invalid = 0;  uint64_t opaque;  int ret;
+int n_lcores;

 opaque = evtim->impl_opaque[1];  adapter = (struct
rte_event_timer_adapter *)(uintptr_t)opaque; @@
-605,8 +606,12 @@ swtim_callback(struct rte_timer *tim)
       "with immediate expiry value");  }

-if (unlikely(rte_atomic16_test_and_set(&sw-

quoted

in_use[lcore].v)))

-sw->poll_lcores[sw->n_poll_lcores++] = lcore;
+if (unlikely(rte_atomic16_test_and_set(&sw-

quoted

in_use[lcore].v))) {

+n_lcores = __atomic_fetch_add(&sw->n_poll_lcores,
1,
+__ATOMIC_RELAXED);

Since this commit will be back ported, we should prefer to use
rte_atomic APIs for this commit. Otherwise, we will have a mix of
rte_atomic and C11 APIs.
My suggestion is to fix this bug using rte_atomic so that
backported code will have only rte_atomic APIs. Add another commit
(if
required) in this series to make the bug fix use C11 APIs (this
commit will not be

backported).

Hi Honnappa,

It doesn't have an applicable rte_atomic_XXX API to fix this issue.
The rte_atomic32_inc doesn't return the original value of the input
parameter and rte_atomic32_add_return can only return the new value.

Ok, understood.

quoted

Meanwhile, the rte_timer_alt_manage & rte_timer_stop_all API not
support rte_atomic type parameters. We might need to rewrite these
two APIs if we want to use rte_atomic operations for n_pol_lcores
and

poll_lcores array.

quoted

So, a better solution could be to backport the entire c11 solution
to stable releases.

I am ok with the approach.
Erik, are you ok with this?

Yes, I'm OK with that as well.

Thanks Erik. I think we need to add stable@dpdk.org to Cc for all the commits in the series.

Thanks,
Erik

quoted

quoted

Thanks,
Phil

quoted

quoted

Just a nit, but let's align the continued line with the opening
parentheses in this location and below.  With these changes:

Acked-by: Erik Gabriel Carrillo <redacted>

quoted

+__atomic_store_n(&sw->poll_lcores[n_lcores],
lcore,
+__ATOMIC_RELAXED);
+}
 } else {
 EVTIM_BUF_LOG_DBG("buffered an event timer expiry event");

@@ -1011,6 +1016,7 @@ __swtim_arm_burst(const struct

rte_event_timer_adapter *adapter,  uint32_t lcore_id =
rte_lcore_id();  struct rte_timer *tim, *tims[nb_evtims];
uint64_t cycles;
+int n_lcores;

 #ifdef RTE_LIBRTE_EVENTDEV_DEBUG
 /* Check that the service is running. */ @@ -1033,8 +1039,10

@@ __swtim_arm_burst(const struct rte_event_timer_adapter

*adapter, if
(unlikely(rte_atomic16_test_and_set(&sw->in_use[lcore_id].v)))
{  EVTIM_LOG_DBG("Adding lcore id = %u to list of lcores to

poll",

quoted

       lcore_id);
-sw->poll_lcores[sw->n_poll_lcores] = lcore_id;
-++sw->n_poll_lcores;
+n_lcores = __atomic_fetch_add(&sw->n_poll_lcores, 1,
+__ATOMIC_RELAXED); __atomic_store_n(&sw-

poll_lcores[n_lcores],

quoted

quoted

quoted

+lcore_id, __ATOMIC_RELAXED);
 }

 ret = rte_mempool_get_bulk(sw->tim_pool, (void **)tims,
--
2.7.4