[dpdk-dev] [PATCH 1/2] lib/ethdev: replace full barrier with relaxed barrier

[dpdk-dev] [PATCH 1/2] lib/ethdev: replace full barrier with relaxed barrier

[Honnappa Nagarahalli]

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

[dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions

[Honnappa Nagarahalli]

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

Re: [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions

[Phil Yang]

> -----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

Re: [dpdk-dev] [PATCH 1/2] lib/ethdev: replace full barrier with relaxed barrier

[Ferruh Yigit]

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;
> 

Re: [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions

[Ferruh Yigit]

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/

Re: [dpdk-dev] [PATCH 1/2] lib/ethdev: replace full barrier with relaxed barrier

[Ananyev, Konstantin]

> 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

Re: [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions

[Ananyev, Konstantin]

> 
> 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

Re: [dpdk-dev] [PATCH 2/2] lib/ethdev: fix memory ordering for call back functions

[Honnappa Nagarahalli]

<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.

[dpdk-dev] [PATCH v2 1/2] lib/ethdev: replace full barrier with relaxed barrier

[Honnappa Nagarahalli]

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

[dpdk-dev] [PATCH v2 2/2] lib/ethdev: fix memory ordering for call back functions

[Honnappa Nagarahalli]

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

Re: [dpdk-dev] [PATCH v2 1/2] lib/ethdev: replace full barrier with relaxed barrier

[Ferruh Yigit]

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.