diff mbox series

[v1,5/6] distributor: fix missing handshake synchronization

Message ID 20200915193449.13310-6-l.wojciechow@partner.samsung.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series fix distributor synchronization issues |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Lukasz Wojciechowski Sept. 15, 2020, 7:34 p.m. UTC 
  rte_distributor_return_pkt function which is run on worker cores
must wait for distributor core to clear handshake on retptr64
before using those buffers. While the handshake is set distributor
core controls buffers and any operations on worker side might overwrite
buffers which are unread yet.

Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 lib/librte_distributor/rte_distributor.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Hunt, David Sept. 17, 2020, 1:22 p.m. UTC | #1 
Hi Lukasz,

On 15/9/2020 8:34 PM, Lukasz Wojciechowski wrote:
> rte_distributor_return_pkt function which is run on worker cores
> must wait for distributor core to clear handshake on retptr64
> before using those buffers. While the handshake is set distributor
> core controls buffers and any operations on worker side might overwrite
> buffers which are unread yet.
>
> Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
> Cc: david.hunt@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> ---
>   lib/librte_distributor/rte_distributor.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
> index 1c047f065..89493c331 100644
> --- a/lib/librte_distributor/rte_distributor.c
> +++ b/lib/librte_distributor/rte_distributor.c
> @@ -160,6 +160,7 @@ rte_distributor_return_pkt(struct rte_distributor *d,
>   {
>   	struct rte_distributor_buffer *buf = &d->bufs[worker_id];
>   	unsigned int i;
> +	volatile int64_t *retptr64;
>   
>   	if (unlikely(d->alg_type == RTE_DIST_ALG_SINGLE)) {
>   		if (num == 1)
> @@ -169,6 +170,19 @@ rte_distributor_return_pkt(struct rte_distributor *d,
>   			return -EINVAL;
>   	}
>   
> +	retptr64 = &(buf->retptr64[0]);
> +	/* Spin while handshake bits are set (scheduler clears it).
> +	 * Sync with worker on GET_BUF flag.
> +	 */
> +	while (unlikely(__atomic_load_n(retptr64, __ATOMIC_ACQUIRE)
> +			& RTE_DISTRIB_GET_BUF)) {
> +		rte_pause();
> +		uint64_t t = rte_rdtsc()+100;
> +
> +		while (rte_rdtsc() < t)
> +			rte_pause();
> +	}
> +


The 'unlikely' is appropriate, but when it does occur, this looks to be 
a necessary addition.
And I've confirmed no loss in performance on my system.

Acked-by: David Hunt <david.hunt@intel.com>
diff mbox series

Patch

diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
index 1c047f065..89493c331 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -160,6 +160,7 @@  rte_distributor_return_pkt(struct rte_distributor *d,
 {
 	struct rte_distributor_buffer *buf = &d->bufs[worker_id];
 	unsigned int i;
+	volatile int64_t *retptr64;
 
 	if (unlikely(d->alg_type == RTE_DIST_ALG_SINGLE)) {
 		if (num == 1)
@@ -169,6 +170,19 @@  rte_distributor_return_pkt(struct rte_distributor *d,
 			return -EINVAL;
 	}
 
+	retptr64 = &(buf->retptr64[0]);
+	/* Spin while handshake bits are set (scheduler clears it).
+	 * Sync with worker on GET_BUF flag.
+	 */
+	while (unlikely(__atomic_load_n(retptr64, __ATOMIC_ACQUIRE)
+			& RTE_DISTRIB_GET_BUF)) {
+		rte_pause();
+		uint64_t t = rte_rdtsc()+100;
+
+		while (rte_rdtsc() < t)
+			rte_pause();
+	}
+
 	/* Sync with distributor to acquire retptrs */
 	__atomic_thread_fence(__ATOMIC_ACQUIRE);
 	for (i = 0; i < RTE_DIST_BURST_SIZE; i++)