* [dpdk-dev] [PATCH 1/2] lib/ethdev: replace full barrier with relaxed barrier
@ 2020-10-02 0:07 Honnappa Nagarahalli
2020-10-02 0:07 ` [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions Honnappa Nagarahalli
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Honnappa Nagarahalli @ 2020-10-02 0:07 UTC (permalink / raw)
To: dev, honnappa.nagarahalli, phil.yang, thomas, arybchenko, ferruh.yigit
Cc: abhinandan.gujjar, nd
From: Phil Yang <phil.yang@arm.com>
While registering the call back functions full write barrier
can be replaced with one-way write barrier.
Signed-off-by: Phil Yang <phil.yang@arm.com>
Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
lib/librte_ethdev/rte_ethdev.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 7858ad5f1..59a41c07f 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -26,7 +26,6 @@
#include <rte_eal.h>
#include <rte_per_lcore.h>
#include <rte_lcore.h>
-#include <rte_atomic.h>
#include <rte_branch_prediction.h>
#include <rte_common.h>
#include <rte_mempool.h>
@@ -4527,8 +4526,12 @@ rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
rte_spinlock_lock(&rte_eth_rx_cb_lock);
/* Add the callbacks at first position */
cb->next = rte_eth_devices[port_id].post_rx_burst_cbs[queue_id];
- rte_smp_wmb();
- rte_eth_devices[port_id].post_rx_burst_cbs[queue_id] = cb;
+ /* Stores to cb->fn, cb->param and cb->next should complete before
+ * cb is visible to data plane threads.
+ */
+ __atomic_store_n(
+ &rte_eth_devices[port_id].post_rx_burst_cbs[queue_id],
+ cb, __ATOMIC_RELEASE);
rte_spinlock_unlock(&rte_eth_rx_cb_lock);
return cb;
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions
2020-10-02 0:07 [dpdk-dev] [PATCH 1/2] lib/ethdev: replace full barrier with relaxed barrier Honnappa Nagarahalli
@ 2020-10-02 0:07 ` Honnappa Nagarahalli
2020-10-09 2:34 ` Phil Yang
` (2 more replies)
2020-10-13 10:30 ` [dpdk-dev] [PATCH 1/2] lib/ethdev: replace full barrier with relaxed barrier Ferruh Yigit
` (2 subsequent siblings)
3 siblings, 3 replies; 11+ messages in thread
From: Honnappa Nagarahalli @ 2020-10-02 0:07 UTC (permalink / raw)
To: dev, honnappa.nagarahalli, phil.yang, thomas, arybchenko, ferruh.yigit
Cc: abhinandan.gujjar, nd, bruce.richardson, john.mcnamara,
reshma.pattan, stable
Call back functions are registered on the control plane. They
are accessed from the data plane. Hence, correct memory orderings
should be used to avoid race conditions.
Fixes: 4dc294158cac ("ethdev: support optional Rx and Tx callbacks")
Fixes: c8231c63ddcb ("ethdev: insert Rx callback as head of list")
Cc: bruce.richardson@intel.com
Cc: john.mcnamara@intel.com
Cc: reshma.pattan@intel.com
Cc: stable@dpdk.org
Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
---
lib/librte_ethdev/rte_ethdev.c | 28 +++++++++++++++++++++------
lib/librte_ethdev/rte_ethdev.h | 35 ++++++++++++++++++++++++++--------
2 files changed, 49 insertions(+), 14 deletions(-)
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 59a41c07f..d89fcdc77 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -4486,12 +4486,20 @@ rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
rte_eth_devices[port_id].post_rx_burst_cbs[queue_id];
if (!tail) {
- rte_eth_devices[port_id].post_rx_burst_cbs[queue_id] = cb;
+ /* Stores to cb->fn and cb->param should complete before
+ * cb is visible to data plane.
+ */
+ __atomic_store_n(
+ &rte_eth_devices[port_id].post_rx_burst_cbs[queue_id],
+ cb, __ATOMIC_RELEASE);
} else {
while (tail->next)
tail = tail->next;
- tail->next = cb;
+ /* Stores to cb->fn and cb->param should complete before
+ * cb is visible to data plane.
+ */
+ __atomic_store_n(&tail->next, cb, __ATOMIC_RELEASE);
}
rte_spinlock_unlock(&rte_eth_rx_cb_lock);
@@ -4576,12 +4584,20 @@ rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id];
if (!tail) {
- rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id] = cb;
+ /* Stores to cb->fn and cb->param should complete before
+ * cb is visible to data plane.
+ */
+ __atomic_store_n(
+ &rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id],
+ cb, __ATOMIC_RELEASE);
} else {
while (tail->next)
tail = tail->next;
- tail->next = cb;
+ /* Stores to cb->fn and cb->param should complete before
+ * cb is visible to data plane.
+ */
+ __atomic_store_n(&tail->next, cb, __ATOMIC_RELEASE);
}
rte_spinlock_unlock(&rte_eth_tx_cb_lock);
@@ -4612,7 +4628,7 @@ rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
cb = *prev_cb;
if (cb == user_cb) {
/* Remove the user cb from the callback list. */
- *prev_cb = cb->next;
+ __atomic_store_n(prev_cb, cb->next, __ATOMIC_RELAXED);
ret = 0;
break;
}
@@ -4646,7 +4662,7 @@ rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id,
cb = *prev_cb;
if (cb == user_cb) {
/* Remove the user cb from the callback list. */
- *prev_cb = cb->next;
+ __atomic_store_n(prev_cb, cb->next, __ATOMIC_RELAXED);
ret = 0;
break;
}
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 70295d7ab..d810e3e38 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -3734,7 +3734,8 @@ struct rte_eth_rxtx_callback;
* The callback function
* @param user_param
* A generic pointer parameter which will be passed to each invocation of the
- * callback function on this port and queue.
+ * callback function on this port and queue. Inter-thread synchronization
+ * of any user data changes is the responsibility of the user.
*
* @return
* NULL on error.
@@ -3763,7 +3764,8 @@ rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
* The callback function
* @param user_param
* A generic pointer parameter which will be passed to each invocation of the
- * callback function on this port and queue.
+ * callback function on this port and queue. Inter-thread synchronization
+ * of any user data changes is the responsibility of the user.
*
* @return
* NULL on error.
@@ -3791,7 +3793,8 @@ rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
* The callback function
* @param user_param
* A generic pointer parameter which will be passed to each invocation of the
- * callback function on this port and queue.
+ * callback function on this port and queue. Inter-thread synchronization
+ * of any user data changes is the responsibility of the user.
*
* @return
* NULL on error.
@@ -3816,7 +3819,9 @@ rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
* on that queue.
*
* - After a short delay - where the delay is sufficient to allow any
- * in-flight callbacks to complete.
+ * in-flight callbacks to complete. Alternately, the RCU mechanism can be
+ * used to detect when data plane threads have ceased referencing the
+ * callback memory.
*
* @param port_id
* The port identifier of the Ethernet device.
@@ -3849,7 +3854,9 @@ int rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
* on that queue.
*
* - After a short delay - where the delay is sufficient to allow any
- * in-flight callbacks to complete.
+ * in-flight callbacks to complete. Alternately, the RCU mechanism can be
+ * used to detect when data plane threads have ceased referencing the
+ * callback memory.
*
* @param port_id
* The port identifier of the Ethernet device.
@@ -4510,10 +4517,16 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
rx_pkts, nb_pkts);
#ifdef RTE_ETHDEV_RXTX_CALLBACKS
- if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
- struct rte_eth_rxtx_callback *cb =
- dev->post_rx_burst_cbs[queue_id];
+ /* __ATOMIC_RELEASE memory order was used when the
+ * call back was inserted into the list.
+ * Since there is a clear dependency between loading
+ * cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory order is
+ * not required.
+ */
+ struct rte_eth_rxtx_callback *cb =
+ dev->post_rx_burst_cbs[queue_id];
+ if (unlikely(cb != NULL)) {
do {
nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
nb_pkts, cb->param);
@@ -4775,6 +4788,12 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
#endif
#ifdef RTE_ETHDEV_RXTX_CALLBACKS
+ /* __ATOMIC_RELEASE memory order was used when the
+ * call back was inserted into the list.
+ * Since there is a clear dependency between loading
+ * cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory order is
+ * not required.
+ */
struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
if (unlikely(cb != NULL)) {
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions
2020-10-02 0:07 ` [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions Honnappa Nagarahalli
@ 2020-10-09 2:34 ` Phil Yang
2020-10-13 10:32 ` Ferruh Yigit
2020-10-13 14:25 ` Ananyev, Konstantin
2 siblings, 0 replies; 11+ messages in thread
From: Phil Yang @ 2020-10-09 2:34 UTC (permalink / raw)
To: Honnappa Nagarahalli, dev, Honnappa Nagarahalli, thomas,
arybchenko, ferruh.yigit
Cc: abhinandan.gujjar, nd, bruce.richardson, john.mcnamara,
reshma.pattan, stable, nd
> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Sent: Friday, October 2, 2020 8:07 AM
> To: dev@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Phil Yang <Phil.Yang@arm.com>;
> thomas@monjalon.net; arybchenko@solarflare.com; ferruh.yigit@intel.com
> Cc: abhinandan.gujjar@intel.com; nd <nd@arm.com>;
> bruce.richardson@intel.com; john.mcnamara@intel.com;
> reshma.pattan@intel.com; stable@dpdk.org
> Subject: [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions
>
> Call back functions are registered on the control plane. They
> are accessed from the data plane. Hence, correct memory orderings
> should be used to avoid race conditions.
>
> Fixes: 4dc294158cac ("ethdev: support optional Rx and Tx callbacks")
> Fixes: c8231c63ddcb ("ethdev: insert Rx callback as head of list")
> Cc: bruce.richardson@intel.com
> Cc: john.mcnamara@intel.com
> Cc: reshma.pattan@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
> ---
> lib/librte_ethdev/rte_ethdev.c | 28 +++++++++++++++++++++------
> lib/librte_ethdev/rte_ethdev.h | 35 ++++++++++++++++++++++++++-------
> -
> 2 files changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 59a41c07f..d89fcdc77 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -4486,12 +4486,20 @@ rte_eth_add_rx_callback(uint16_t port_id,
> uint16_t queue_id,
> rte_eth_devices[port_id].post_rx_burst_cbs[queue_id];
>
> if (!tail) {
> - rte_eth_devices[port_id].post_rx_burst_cbs[queue_id] = cb;
> + /* Stores to cb->fn and cb->param should complete before
> + * cb is visible to data plane.
> + */
> + __atomic_store_n(
> +
> &rte_eth_devices[port_id].post_rx_burst_cbs[queue_id],
> + cb, __ATOMIC_RELEASE);
>
> } else {
> while (tail->next)
> tail = tail->next;
> - tail->next = cb;
> + /* Stores to cb->fn and cb->param should complete before
> + * cb is visible to data plane.
> + */
> + __atomic_store_n(&tail->next, cb, __ATOMIC_RELEASE);
> }
> rte_spinlock_unlock(&rte_eth_rx_cb_lock);
>
> @@ -4576,12 +4584,20 @@ rte_eth_add_tx_callback(uint16_t port_id,
> uint16_t queue_id,
> rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id];
>
> if (!tail) {
> - rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id] = cb;
> + /* Stores to cb->fn and cb->param should complete before
> + * cb is visible to data plane.
> + */
> + __atomic_store_n(
> +
> &rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id],
> + cb, __ATOMIC_RELEASE);
>
> } else {
> while (tail->next)
> tail = tail->next;
> - tail->next = cb;
> + /* Stores to cb->fn and cb->param should complete before
> + * cb is visible to data plane.
> + */
> + __atomic_store_n(&tail->next, cb, __ATOMIC_RELEASE);
> }
> rte_spinlock_unlock(&rte_eth_tx_cb_lock);
>
> @@ -4612,7 +4628,7 @@ rte_eth_remove_rx_callback(uint16_t port_id,
> uint16_t queue_id,
> cb = *prev_cb;
> if (cb == user_cb) {
> /* Remove the user cb from the callback list. */
> - *prev_cb = cb->next;
> + __atomic_store_n(prev_cb, cb->next,
> __ATOMIC_RELAXED);
> ret = 0;
> break;
> }
> @@ -4646,7 +4662,7 @@ rte_eth_remove_tx_callback(uint16_t port_id,
> uint16_t queue_id,
> cb = *prev_cb;
> if (cb == user_cb) {
> /* Remove the user cb from the callback list. */
> - *prev_cb = cb->next;
> + __atomic_store_n(prev_cb, cb->next,
> __ATOMIC_RELAXED);
> ret = 0;
> break;
> }
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 70295d7ab..d810e3e38 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -3734,7 +3734,8 @@ struct rte_eth_rxtx_callback;
> * The callback function
> * @param user_param
> * A generic pointer parameter which will be passed to each invocation of
> the
> - * callback function on this port and queue.
> + * callback function on this port and queue. Inter-thread synchronization
> + * of any user data changes is the responsibility of the user.
> *
> * @return
> * NULL on error.
> @@ -3763,7 +3764,8 @@ rte_eth_add_rx_callback(uint16_t port_id, uint16_t
> queue_id,
> * The callback function
> * @param user_param
> * A generic pointer parameter which will be passed to each invocation of
> the
> - * callback function on this port and queue.
> + * callback function on this port and queue. Inter-thread synchronization
> + * of any user data changes is the responsibility of the user.
> *
> * @return
> * NULL on error.
> @@ -3791,7 +3793,8 @@ rte_eth_add_first_rx_callback(uint16_t port_id,
> uint16_t queue_id,
> * The callback function
> * @param user_param
> * A generic pointer parameter which will be passed to each invocation of
> the
> - * callback function on this port and queue.
> + * callback function on this port and queue. Inter-thread synchronization
> + * of any user data changes is the responsibility of the user.
> *
> * @return
> * NULL on error.
> @@ -3816,7 +3819,9 @@ rte_eth_add_tx_callback(uint16_t port_id, uint16_t
> queue_id,
> * on that queue.
> *
> * - After a short delay - where the delay is sufficient to allow any
> - * in-flight callbacks to complete.
> + * in-flight callbacks to complete. Alternately, the RCU mechanism can be
> + * used to detect when data plane threads have ceased referencing the
> + * callback memory.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
> @@ -3849,7 +3854,9 @@ int rte_eth_remove_rx_callback(uint16_t port_id,
> uint16_t queue_id,
> * on that queue.
> *
> * - After a short delay - where the delay is sufficient to allow any
> - * in-flight callbacks to complete.
> + * in-flight callbacks to complete. Alternately, the RCU mechanism can be
> + * used to detect when data plane threads have ceased referencing the
> + * callback memory.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
> @@ -4510,10 +4517,16 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t
> queue_id,
> rx_pkts, nb_pkts);
>
> #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> - if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> - struct rte_eth_rxtx_callback *cb =
> - dev->post_rx_burst_cbs[queue_id];
> + /* __ATOMIC_RELEASE memory order was used when the
> + * call back was inserted into the list.
> + * Since there is a clear dependency between loading
> + * cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory order is
> + * not required.
> + */
> + struct rte_eth_rxtx_callback *cb =
> + dev->post_rx_burst_cbs[queue_id];
>
> + if (unlikely(cb != NULL)) {
> do {
> nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
> nb_pkts, cb->param);
> @@ -4775,6 +4788,12 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t
> queue_id,
> #endif
>
> #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> + /* __ATOMIC_RELEASE memory order was used when the
> + * call back was inserted into the list.
> + * Since there is a clear dependency between loading
> + * cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory order is
> + * not required.
> + */
> struct rte_eth_rxtx_callback *cb = dev-
> >pre_tx_burst_cbs[queue_id];
>
> if (unlikely(cb != NULL)) {
> --
> 2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] lib/ethdev: replace full barrier with relaxed barrier
2020-10-02 0:07 [dpdk-dev] [PATCH 1/2] lib/ethdev: replace full barrier with relaxed barrier Honnappa Nagarahalli
2020-10-02 0:07 ` [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions Honnappa Nagarahalli
@ 2020-10-13 10:30 ` Ferruh Yigit
2020-10-13 14:24 ` Ananyev, Konstantin
2020-10-13 16:25 ` [dpdk-dev] [PATCH v2 " Honnappa Nagarahalli
3 siblings, 0 replies; 11+ messages in thread
From: Ferruh Yigit @ 2020-10-13 10:30 UTC (permalink / raw)
To: Honnappa Nagarahalli, dev, phil.yang, thomas, arybchenko,
Konstantin Ananyev, jerinj
Cc: abhinandan.gujjar, nd
On 10/2/2020 1:07 AM, Honnappa Nagarahalli wrote:
> From: Phil Yang <phil.yang@arm.com>
>
> While registering the call back functions full write barrier
> can be replaced with one-way write barrier.
>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
+Konstantin & Jerin,
Can you please help reviewing this patch?
> ---
> lib/librte_ethdev/rte_ethdev.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 7858ad5f1..59a41c07f 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -26,7 +26,6 @@
> #include <rte_eal.h>
> #include <rte_per_lcore.h>
> #include <rte_lcore.h>
> -#include <rte_atomic.h>
> #include <rte_branch_prediction.h>
> #include <rte_common.h>
> #include <rte_mempool.h>
> @@ -4527,8 +4526,12 @@ rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
> rte_spinlock_lock(&rte_eth_rx_cb_lock);
> /* Add the callbacks at first position */
> cb->next = rte_eth_devices[port_id].post_rx_burst_cbs[queue_id];
> - rte_smp_wmb();
> - rte_eth_devices[port_id].post_rx_burst_cbs[queue_id] = cb;
> + /* Stores to cb->fn, cb->param and cb->next should complete before
> + * cb is visible to data plane threads.
> + */
> + __atomic_store_n(
> + &rte_eth_devices[port_id].post_rx_burst_cbs[queue_id],
> + cb, __ATOMIC_RELEASE);
> rte_spinlock_unlock(&rte_eth_rx_cb_lock);
>
> return cb;
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions
2020-10-02 0:07 ` [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions Honnappa Nagarahalli
2020-10-09 2:34 ` Phil Yang
@ 2020-10-13 10:32 ` Ferruh Yigit
2020-10-13 15:25 ` Honnappa Nagarahalli
2020-10-13 14:25 ` Ananyev, Konstantin
2 siblings, 1 reply; 11+ messages in thread
From: Ferruh Yigit @ 2020-10-13 10:32 UTC (permalink / raw)
To: Honnappa Nagarahalli, dev, phil.yang, thomas, arybchenko, ZY Qiu,
Konstantin Ananyev, jerinj
Cc: abhinandan.gujjar, nd, bruce.richardson, john.mcnamara,
reshma.pattan, stable
On 10/2/2020 1:07 AM, Honnappa Nagarahalli wrote:
> Call back functions are registered on the control plane. They
> are accessed from the data plane. Hence, correct memory orderings
> should be used to avoid race conditions.
>
> Fixes: 4dc294158cac ("ethdev: support optional Rx and Tx callbacks")
> Fixes: c8231c63ddcb ("ethdev: insert Rx callback as head of list")
> Cc: bruce.richardson@intel.com
> Cc: john.mcnamara@intel.com
> Cc: reshma.pattan@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
+Konstantin & Jerin,
Can you please help reviewing this one too?
@Honnappa,
Is this patch supersedes the following one in the queue by ZY Qiu:
https://patches.dpdk.org/patch/66301/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] lib/ethdev: replace full barrier with relaxed barrier
2020-10-02 0:07 [dpdk-dev] [PATCH 1/2] lib/ethdev: replace full barrier with relaxed barrier Honnappa Nagarahalli
2020-10-02 0:07 ` [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions Honnappa Nagarahalli
2020-10-13 10:30 ` [dpdk-dev] [PATCH 1/2] lib/ethdev: replace full barrier with relaxed barrier Ferruh Yigit
@ 2020-10-13 14:24 ` Ananyev, Konstantin
2020-10-13 16:25 ` [dpdk-dev] [PATCH v2 " Honnappa Nagarahalli
3 siblings, 0 replies; 11+ messages in thread
From: Ananyev, Konstantin @ 2020-10-13 14:24 UTC (permalink / raw)
To: Honnappa Nagarahalli, dev, phil.yang, thomas, arybchenko, Yigit, Ferruh
Cc: Gujjar, Abhinandan S, nd
> From: Phil Yang <phil.yang@arm.com>
>
> While registering the call back functions full write barrier
> can be replaced with one-way write barrier.
>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> lib/librte_ethdev/rte_ethdev.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 7858ad5f1..59a41c07f 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -26,7 +26,6 @@
> #include <rte_eal.h>
> #include <rte_per_lcore.h>
> #include <rte_lcore.h>
> -#include <rte_atomic.h>
> #include <rte_branch_prediction.h>
> #include <rte_common.h>
> #include <rte_mempool.h>
> @@ -4527,8 +4526,12 @@ rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
> rte_spinlock_lock(&rte_eth_rx_cb_lock);
> /* Add the callbacks at first position */
> cb->next = rte_eth_devices[port_id].post_rx_burst_cbs[queue_id];
> - rte_smp_wmb();
> - rte_eth_devices[port_id].post_rx_burst_cbs[queue_id] = cb;
> + /* Stores to cb->fn, cb->param and cb->next should complete before
> + * cb is visible to data plane threads.
> + */
> + __atomic_store_n(
> + &rte_eth_devices[port_id].post_rx_burst_cbs[queue_id],
> + cb, __ATOMIC_RELEASE);
> rte_spinlock_unlock(&rte_eth_rx_cb_lock);
>
> return cb;
> --
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions
2020-10-02 0:07 ` [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions Honnappa Nagarahalli
2020-10-09 2:34 ` Phil Yang
2020-10-13 10:32 ` Ferruh Yigit
@ 2020-10-13 14:25 ` Ananyev, Konstantin
2 siblings, 0 replies; 11+ messages in thread
From: Ananyev, Konstantin @ 2020-10-13 14:25 UTC (permalink / raw)
To: Honnappa Nagarahalli, dev, phil.yang, thomas, arybchenko, Yigit, Ferruh
Cc: Gujjar, Abhinandan S, nd, Richardson, Bruce, Mcnamara, John,
Pattan, Reshma, stable
>
> Call back functions are registered on the control plane. They
> are accessed from the data plane. Hence, correct memory orderings
> should be used to avoid race conditions.
>
> Fixes: 4dc294158cac ("ethdev: support optional Rx and Tx callbacks")
> Fixes: c8231c63ddcb ("ethdev: insert Rx callback as head of list")
> Cc: bruce.richardson@intel.com
> Cc: john.mcnamara@intel.com
> Cc: reshma.pattan@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
> ---
> lib/librte_ethdev/rte_ethdev.c | 28 +++++++++++++++++++++------
> lib/librte_ethdev/rte_ethdev.h | 35 ++++++++++++++++++++++++++--------
> 2 files changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 59a41c07f..d89fcdc77 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -4486,12 +4486,20 @@ rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
> rte_eth_devices[port_id].post_rx_burst_cbs[queue_id];
>
> if (!tail) {
> - rte_eth_devices[port_id].post_rx_burst_cbs[queue_id] = cb;
> + /* Stores to cb->fn and cb->param should complete before
> + * cb is visible to data plane.
> + */
> + __atomic_store_n(
> + &rte_eth_devices[port_id].post_rx_burst_cbs[queue_id],
> + cb, __ATOMIC_RELEASE);
>
> } else {
> while (tail->next)
> tail = tail->next;
> - tail->next = cb;
> + /* Stores to cb->fn and cb->param should complete before
> + * cb is visible to data plane.
> + */
> + __atomic_store_n(&tail->next, cb, __ATOMIC_RELEASE);
> }
> rte_spinlock_unlock(&rte_eth_rx_cb_lock);
>
> @@ -4576,12 +4584,20 @@ rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
> rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id];
>
> if (!tail) {
> - rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id] = cb;
> + /* Stores to cb->fn and cb->param should complete before
> + * cb is visible to data plane.
> + */
> + __atomic_store_n(
> + &rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id],
> + cb, __ATOMIC_RELEASE);
>
> } else {
> while (tail->next)
> tail = tail->next;
> - tail->next = cb;
> + /* Stores to cb->fn and cb->param should complete before
> + * cb is visible to data plane.
> + */
> + __atomic_store_n(&tail->next, cb, __ATOMIC_RELEASE);
> }
> rte_spinlock_unlock(&rte_eth_tx_cb_lock);
>
> @@ -4612,7 +4628,7 @@ rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
> cb = *prev_cb;
> if (cb == user_cb) {
> /* Remove the user cb from the callback list. */
> - *prev_cb = cb->next;
> + __atomic_store_n(prev_cb, cb->next, __ATOMIC_RELAXED);
> ret = 0;
> break;
> }
> @@ -4646,7 +4662,7 @@ rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id,
> cb = *prev_cb;
> if (cb == user_cb) {
> /* Remove the user cb from the callback list. */
> - *prev_cb = cb->next;
> + __atomic_store_n(prev_cb, cb->next, __ATOMIC_RELAXED);
> ret = 0;
> break;
> }
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 70295d7ab..d810e3e38 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -3734,7 +3734,8 @@ struct rte_eth_rxtx_callback;
> * The callback function
> * @param user_param
> * A generic pointer parameter which will be passed to each invocation of the
> - * callback function on this port and queue.
> + * callback function on this port and queue. Inter-thread synchronization
> + * of any user data changes is the responsibility of the user.
> *
> * @return
> * NULL on error.
> @@ -3763,7 +3764,8 @@ rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
> * The callback function
> * @param user_param
> * A generic pointer parameter which will be passed to each invocation of the
> - * callback function on this port and queue.
> + * callback function on this port and queue. Inter-thread synchronization
> + * of any user data changes is the responsibility of the user.
> *
> * @return
> * NULL on error.
> @@ -3791,7 +3793,8 @@ rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
> * The callback function
> * @param user_param
> * A generic pointer parameter which will be passed to each invocation of the
> - * callback function on this port and queue.
> + * callback function on this port and queue. Inter-thread synchronization
> + * of any user data changes is the responsibility of the user.
> *
> * @return
> * NULL on error.
> @@ -3816,7 +3819,9 @@ rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
> * on that queue.
> *
> * - After a short delay - where the delay is sufficient to allow any
> - * in-flight callbacks to complete.
> + * in-flight callbacks to complete. Alternately, the RCU mechanism can be
> + * used to detect when data plane threads have ceased referencing the
> + * callback memory.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
> @@ -3849,7 +3854,9 @@ int rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
> * on that queue.
> *
> * - After a short delay - where the delay is sufficient to allow any
> - * in-flight callbacks to complete.
> + * in-flight callbacks to complete. Alternately, the RCU mechanism can be
> + * used to detect when data plane threads have ceased referencing the
> + * callback memory.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
> @@ -4510,10 +4517,16 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> rx_pkts, nb_pkts);
>
> #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> - if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> - struct rte_eth_rxtx_callback *cb =
> - dev->post_rx_burst_cbs[queue_id];
> + /* __ATOMIC_RELEASE memory order was used when the
> + * call back was inserted into the list.
> + * Since there is a clear dependency between loading
> + * cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory order is
> + * not required.
> + */
> + struct rte_eth_rxtx_callback *cb =
> + dev->post_rx_burst_cbs[queue_id];
>
> + if (unlikely(cb != NULL)) {
> do {
> nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
> nb_pkts, cb->param);
> @@ -4775,6 +4788,12 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
> #endif
>
> #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> + /* __ATOMIC_RELEASE memory order was used when the
> + * call back was inserted into the list.
> + * Since there is a clear dependency between loading
> + * cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory order is
> + * not required.
> + */
> struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
>
> if (unlikely(cb != NULL)) {
> --
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions
2020-10-13 10:32 ` Ferruh Yigit
@ 2020-10-13 15:25 ` Honnappa Nagarahalli
0 siblings, 0 replies; 11+ messages in thread
From: Honnappa Nagarahalli @ 2020-10-13 15:25 UTC (permalink / raw)
To: Ferruh Yigit, dev, Phil Yang, thomas, arybchenko, ZY Qiu,
Konstantin Ananyev, jerinj
Cc: abhinandan.gujjar, nd, bruce.richardson, john.mcnamara,
reshma.pattan, stable, Honnappa Nagarahalli, nd
<snip>
> Subject: Re: [PATCH 2/2] lib/ethdev: fix memory ordering for call back
> functions
>
> On 10/2/2020 1:07 AM, Honnappa Nagarahalli wrote:
> > Call back functions are registered on the control plane. They are
> > accessed from the data plane. Hence, correct memory orderings should
> > be used to avoid race conditions.
> >
> > Fixes: 4dc294158cac ("ethdev: support optional Rx and Tx callbacks")
> > Fixes: c8231c63ddcb ("ethdev: insert Rx callback as head of list")
> > Cc: bruce.richardson@intel.com
> > Cc: john.mcnamara@intel.com
> > Cc: reshma.pattan@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
>
> +Konstantin & Jerin,
>
> Can you please help reviewing this one too?
>
> @Honnappa,
>
> Is this patch supersedes the following one in the queue by ZY Qiu:
> https://patches.dpdk.org/patch/66301/
Yes, my patch should supersede. I will create a V2 to address the issue ZY Qiu's patch is trying to address.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [dpdk-dev] [PATCH v2 1/2] lib/ethdev: replace full barrier with relaxed barrier
2020-10-02 0:07 [dpdk-dev] [PATCH 1/2] lib/ethdev: replace full barrier with relaxed barrier Honnappa Nagarahalli
` (2 preceding siblings ...)
2020-10-13 14:24 ` Ananyev, Konstantin
@ 2020-10-13 16:25 ` Honnappa Nagarahalli
2020-10-13 16:25 ` [dpdk-dev] [PATCH v2 2/2] lib/ethdev: fix memory ordering for call back functions Honnappa Nagarahalli
2020-10-14 16:36 ` [dpdk-dev] [PATCH v2 1/2] lib/ethdev: replace full barrier with relaxed barrier Ferruh Yigit
3 siblings, 2 replies; 11+ messages in thread
From: Honnappa Nagarahalli @ 2020-10-13 16:25 UTC (permalink / raw)
To: dev, honnappa.nagarahalli, phil.yang, thomas, arybchenko,
ferruh.yigit, konstantin.ananyev, jerinj, tgw_team
Cc: abhinandan.gujjar, nd
From: Phil Yang <phil.yang@arm.com>
While registering the call back functions full write barrier
can be replaced with one-way write barrier.
Signed-off-by: Phil Yang <phil.yang@arm.com>
Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
v2 - no change
lib/librte_ethdev/rte_ethdev.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 7858ad5f1..59a41c07f 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -26,7 +26,6 @@
#include <rte_eal.h>
#include <rte_per_lcore.h>
#include <rte_lcore.h>
-#include <rte_atomic.h>
#include <rte_branch_prediction.h>
#include <rte_common.h>
#include <rte_mempool.h>
@@ -4527,8 +4526,12 @@ rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
rte_spinlock_lock(&rte_eth_rx_cb_lock);
/* Add the callbacks at first position */
cb->next = rte_eth_devices[port_id].post_rx_burst_cbs[queue_id];
- rte_smp_wmb();
- rte_eth_devices[port_id].post_rx_burst_cbs[queue_id] = cb;
+ /* Stores to cb->fn, cb->param and cb->next should complete before
+ * cb is visible to data plane threads.
+ */
+ __atomic_store_n(
+ &rte_eth_devices[port_id].post_rx_burst_cbs[queue_id],
+ cb, __ATOMIC_RELEASE);
rte_spinlock_unlock(&rte_eth_rx_cb_lock);
return cb;
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [dpdk-dev] [PATCH v2 2/2] lib/ethdev: fix memory ordering for call back functions
2020-10-13 16:25 ` [dpdk-dev] [PATCH v2 " Honnappa Nagarahalli
@ 2020-10-13 16:25 ` Honnappa Nagarahalli
2020-10-14 16:36 ` [dpdk-dev] [PATCH v2 1/2] lib/ethdev: replace full barrier with relaxed barrier Ferruh Yigit
1 sibling, 0 replies; 11+ messages in thread
From: Honnappa Nagarahalli @ 2020-10-13 16:25 UTC (permalink / raw)
To: dev, honnappa.nagarahalli, phil.yang, thomas, arybchenko,
ferruh.yigit, konstantin.ananyev, jerinj, tgw_team
Cc: abhinandan.gujjar, nd, bruce.richardson, john.mcnamara,
reshma.pattan, stable
Call back functions are registered on the control plane. They
are accessed from the data plane. Hence, correct memory orderings
should be used to avoid race conditions.
Fixes: 4dc294158cac ("ethdev: support optional Rx and Tx callbacks")
Fixes: c8231c63ddcb ("ethdev: insert Rx callback as head of list")
Cc: bruce.richardson@intel.com
Cc: john.mcnamara@intel.com
Cc: reshma.pattan@intel.com
Cc: stable@dpdk.org
Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
v2 - load the call back function pointer using __atomic_load_n
lib/librte_ethdev/rte_ethdev.c | 28 ++++++++++++++++++-----
lib/librte_ethdev/rte_ethdev.h | 42 ++++++++++++++++++++++++++--------
2 files changed, 55 insertions(+), 15 deletions(-)
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 59a41c07f..d89fcdc77 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -4486,12 +4486,20 @@ rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
rte_eth_devices[port_id].post_rx_burst_cbs[queue_id];
if (!tail) {
- rte_eth_devices[port_id].post_rx_burst_cbs[queue_id] = cb;
+ /* Stores to cb->fn and cb->param should complete before
+ * cb is visible to data plane.
+ */
+ __atomic_store_n(
+ &rte_eth_devices[port_id].post_rx_burst_cbs[queue_id],
+ cb, __ATOMIC_RELEASE);
} else {
while (tail->next)
tail = tail->next;
- tail->next = cb;
+ /* Stores to cb->fn and cb->param should complete before
+ * cb is visible to data plane.
+ */
+ __atomic_store_n(&tail->next, cb, __ATOMIC_RELEASE);
}
rte_spinlock_unlock(&rte_eth_rx_cb_lock);
@@ -4576,12 +4584,20 @@ rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id];
if (!tail) {
- rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id] = cb;
+ /* Stores to cb->fn and cb->param should complete before
+ * cb is visible to data plane.
+ */
+ __atomic_store_n(
+ &rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id],
+ cb, __ATOMIC_RELEASE);
} else {
while (tail->next)
tail = tail->next;
- tail->next = cb;
+ /* Stores to cb->fn and cb->param should complete before
+ * cb is visible to data plane.
+ */
+ __atomic_store_n(&tail->next, cb, __ATOMIC_RELEASE);
}
rte_spinlock_unlock(&rte_eth_tx_cb_lock);
@@ -4612,7 +4628,7 @@ rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
cb = *prev_cb;
if (cb == user_cb) {
/* Remove the user cb from the callback list. */
- *prev_cb = cb->next;
+ __atomic_store_n(prev_cb, cb->next, __ATOMIC_RELAXED);
ret = 0;
break;
}
@@ -4646,7 +4662,7 @@ rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id,
cb = *prev_cb;
if (cb == user_cb) {
/* Remove the user cb from the callback list. */
- *prev_cb = cb->next;
+ __atomic_store_n(prev_cb, cb->next, __ATOMIC_RELAXED);
ret = 0;
break;
}
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 70295d7ab..6ad2a549b 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -3734,7 +3734,8 @@ struct rte_eth_rxtx_callback;
* The callback function
* @param user_param
* A generic pointer parameter which will be passed to each invocation of the
- * callback function on this port and queue.
+ * callback function on this port and queue. Inter-thread synchronization
+ * of any user data changes is the responsibility of the user.
*
* @return
* NULL on error.
@@ -3763,7 +3764,8 @@ rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
* The callback function
* @param user_param
* A generic pointer parameter which will be passed to each invocation of the
- * callback function on this port and queue.
+ * callback function on this port and queue. Inter-thread synchronization
+ * of any user data changes is the responsibility of the user.
*
* @return
* NULL on error.
@@ -3791,7 +3793,8 @@ rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
* The callback function
* @param user_param
* A generic pointer parameter which will be passed to each invocation of the
- * callback function on this port and queue.
+ * callback function on this port and queue. Inter-thread synchronization
+ * of any user data changes is the responsibility of the user.
*
* @return
* NULL on error.
@@ -3816,7 +3819,9 @@ rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
* on that queue.
*
* - After a short delay - where the delay is sufficient to allow any
- * in-flight callbacks to complete.
+ * in-flight callbacks to complete. Alternately, the RCU mechanism can be
+ * used to detect when data plane threads have ceased referencing the
+ * callback memory.
*
* @param port_id
* The port identifier of the Ethernet device.
@@ -3849,7 +3854,9 @@ int rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
* on that queue.
*
* - After a short delay - where the delay is sufficient to allow any
- * in-flight callbacks to complete.
+ * in-flight callbacks to complete. Alternately, the RCU mechanism can be
+ * used to detect when data plane threads have ceased referencing the
+ * callback memory.
*
* @param port_id
* The port identifier of the Ethernet device.
@@ -4510,10 +4517,18 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
rx_pkts, nb_pkts);
#ifdef RTE_ETHDEV_RXTX_CALLBACKS
- if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
- struct rte_eth_rxtx_callback *cb =
- dev->post_rx_burst_cbs[queue_id];
+ struct rte_eth_rxtx_callback *cb;
+ /* __ATOMIC_RELEASE memory order was used when the
+ * call back was inserted into the list.
+ * Since there is a clear dependency between loading
+ * cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory order is
+ * not required.
+ */
+ cb = __atomic_load_n(&dev->post_rx_burst_cbs[queue_id],
+ __ATOMIC_RELAXED);
+
+ if (unlikely(cb != NULL)) {
do {
nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
nb_pkts, cb->param);
@@ -4775,7 +4790,16 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
#endif
#ifdef RTE_ETHDEV_RXTX_CALLBACKS
- struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
+ struct rte_eth_rxtx_callback *cb;
+
+ /* __ATOMIC_RELEASE memory order was used when the
+ * call back was inserted into the list.
+ * Since there is a clear dependency between loading
+ * cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory order is
+ * not required.
+ */
+ cb = __atomic_load_n(&dev->pre_tx_burst_cbs[queue_id],
+ __ATOMIC_RELAXED);
if (unlikely(cb != NULL)) {
do {
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH v2 1/2] lib/ethdev: replace full barrier with relaxed barrier
2020-10-13 16:25 ` [dpdk-dev] [PATCH v2 " Honnappa Nagarahalli
2020-10-13 16:25 ` [dpdk-dev] [PATCH v2 2/2] lib/ethdev: fix memory ordering for call back functions Honnappa Nagarahalli
@ 2020-10-14 16:36 ` Ferruh Yigit
1 sibling, 0 replies; 11+ messages in thread
From: Ferruh Yigit @ 2020-10-14 16:36 UTC (permalink / raw)
To: Honnappa Nagarahalli, dev, phil.yang, thomas, arybchenko,
konstantin.ananyev, jerinj, tgw_team
Cc: abhinandan.gujjar, nd
On 10/13/2020 5:25 PM, Honnappa Nagarahalli wrote:
> From: Phil Yang <phil.yang@arm.com>
>
> While registering the call back functions full write barrier
> can be replaced with one-way write barrier.
>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Series applied to dpdk-next-net/main, thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-10-14 16:38 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-02 0:07 [dpdk-dev] [PATCH 1/2] lib/ethdev: replace full barrier with relaxed barrier Honnappa Nagarahalli
2020-10-02 0:07 ` [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions Honnappa Nagarahalli
2020-10-09 2:34 ` Phil Yang
2020-10-13 10:32 ` Ferruh Yigit
2020-10-13 15:25 ` Honnappa Nagarahalli
2020-10-13 14:25 ` Ananyev, Konstantin
2020-10-13 10:30 ` [dpdk-dev] [PATCH 1/2] lib/ethdev: replace full barrier with relaxed barrier Ferruh Yigit
2020-10-13 14:24 ` Ananyev, Konstantin
2020-10-13 16:25 ` [dpdk-dev] [PATCH v2 " Honnappa Nagarahalli
2020-10-13 16:25 ` [dpdk-dev] [PATCH v2 2/2] lib/ethdev: fix memory ordering for call back functions Honnappa Nagarahalli
2020-10-14 16:36 ` [dpdk-dev] [PATCH v2 1/2] lib/ethdev: replace full barrier with relaxed barrier Ferruh Yigit
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).