net/memif: relax barrier for zero copy path
diff mbox series

Message ID 1599802699-20876-1-git-send-email-phil.yang@arm.com
State Accepted, archived
Delegated to: Ferruh Yigit
Headers show
Series
  • net/memif: relax barrier for zero copy path
Related show

Checks

Context Check Description
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/travis-robot success Travis build: errored
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/checkpatch success coding style OK

Commit Message

Phil Yang Sept. 11, 2020, 5:38 a.m. UTC
Using 'rte_mb' to synchronize the shared ring head/tail between producer
and consumer will stall the pipeline and damage performance on the weak
memory model platforms, such like aarch64.

Relax the expensive barrier with c11 atomic with explicit memory
ordering can improve 3.6% performance on throughput.

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 drivers/net/memif/rte_eth_memif.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

Comments

Ferruh Yigit Sept. 18, 2020, 11:58 a.m. UTC | #1
On 9/11/2020 6:38 AM, Phil Yang wrote:
> Using 'rte_mb' to synchronize the shared ring head/tail between producer
> and consumer will stall the pipeline and damage performance on the weak
> memory model platforms, such like aarch64.
> 
> Relax the expensive barrier with c11 atomic with explicit memory
> ordering can improve 3.6% performance on throughput.
> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

Hi Jakub,

Can you please check/test this patch?

Thanks,
ferruh
Honnappa Nagarahalli Sept. 18, 2020, 10:49 p.m. UTC | #2
Hi Jakub,
	I am trying to review this patch. I am having difficulty in understanding the implementation for the queue/ring, appreciate if you could help me understand the logic.

1) The S2M queues - are used to send packets from slave to master. My understanding is that, the slave thread would call 'eth_memif_tx_zc' and the master thread would call 'eth_memif_rx_zc'. Is this correct?
2) The M2S queues - are used to send packets from master to slave. Here the slave thread would call 'eth_memif_rx_zc' and the master thread would call 'eth_memif_tx_zc'. Is this correct?

Thank you,
Honnappa

> -----Original Message-----
> From: Phil Yang <phil.yang@arm.com>
> Sent: Friday, September 11, 2020 12:38 AM
> To: jgrajcia@cisco.com; dev@dpdk.org
> Cc: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> Subject: [PATCH] net/memif: relax barrier for zero copy path
> 
> Using 'rte_mb' to synchronize the shared ring head/tail between producer
> and consumer will stall the pipeline and damage performance on the weak
> memory model platforms, such like aarch64.
> 
> Relax the expensive barrier with c11 atomic with explicit memory ordering
> can improve 3.6% performance on throughput.
> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>  drivers/net/memif/rte_eth_memif.c | 35 +++++++++++++++++++++++++------
> ----
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/memif/rte_eth_memif.c
> b/drivers/net/memif/rte_eth_memif.c
> index c1c7e9f..a19c0f3 100644
> --- a/drivers/net/memif/rte_eth_memif.c
> +++ b/drivers/net/memif/rte_eth_memif.c
> @@ -253,7 +253,12 @@ memif_free_stored_mbufs(struct
> pmd_process_private *proc_private, struct memif_q
>  	memif_ring_t *ring = memif_get_ring_from_queue(proc_private,
> mq);
> 
>  	/* FIXME: improve performance */
> -	while (mq->last_tail != ring->tail) {
> +	/* The ring->tail acts as a guard variable between Tx and Rx
> +	 * threads, so using load-acquire pairs with store-release
> +	 * to synchronize it between threads.
> +	 */
> +	while (mq->last_tail != __atomic_load_n(&ring->tail,
> +						__ATOMIC_ACQUIRE)) {
>  		RTE_MBUF_PREFETCH_TO_FREE(mq->buffers[(mq->last_tail
> + 1) & mask]);
>  		/* Decrement refcnt and free mbuf. (current segment) */
>  		rte_mbuf_refcnt_update(mq->buffers[mq->last_tail & mask],
> -1); @@ -455,7 +460,11 @@ eth_memif_rx_zc(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
>  	mask = ring_size - 1;
> 
>  	cur_slot = mq->last_tail;
> -	last_slot = ring->tail;
> +	/* The ring->tail acts as a guard variable between Tx and Rx
> +	 * threads, so using load-acquire pairs with store-release
> +	 * to synchronize it between threads.
> +	 */
> +	last_slot = __atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE);
>  	if (cur_slot == last_slot)
>  		goto refill;
>  	n_slots = last_slot - cur_slot;
> @@ -501,7 +510,11 @@ eth_memif_rx_zc(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
> 
>  /* Supply master with new buffers */
>  refill:
> -	head = ring->head;
> +	/* The ring->head acts as a guard variable between Tx and Rx
> +	 * threads, so using load-acquire pairs with store-release
> +	 * to synchronize it between threads.
> +	 */
> +	head = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE);
>  	n_slots = ring_size - head + mq->last_tail;
> 
>  	if (n_slots < 32)
> @@ -526,8 +539,7 @@ eth_memif_rx_zc(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
>  			(uint8_t *)proc_private->regions[d0->region]->addr;
>  	}
>  no_free_mbufs:
> -	rte_mb();
> -	ring->head = head;
> +	__atomic_store_n(&ring->head, head, __ATOMIC_RELEASE);
> 
>  	mq->n_pkts += n_rx_pkts;
> 
> @@ -723,8 +735,12 @@ eth_memif_tx_zc(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
>  	memif_free_stored_mbufs(proc_private, mq);
> 
>  	/* ring type always MEMIF_RING_S2M */
> -	slot = ring->head;
> -	n_free = ring_size - ring->head + mq->last_tail;
> +	/* The ring->head acts as a guard variable between Tx and Rx
> +	 * threads, so using load-acquire pairs with store-release
> +	 * to synchronize it between threads.
> +	 */
> +	slot = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE);
> +	n_free = ring_size - slot + mq->last_tail;
> 
>  	int used_slots;
> 
> @@ -778,12 +794,11 @@ eth_memif_tx_zc(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
>  	}
> 
>  no_free_slots:
> -	rte_mb();
>  	/* update ring pointers */
>  	if (type == MEMIF_RING_S2M)
> -		ring->head = slot;
> +		__atomic_store_n(&ring->head, slot, __ATOMIC_RELEASE);
>  	else
> -		ring->tail = slot;
> +		__atomic_store_n(&ring->tail, slot, __ATOMIC_RELEASE);
> 
>  	/* Send interrupt, if enabled. */
>  	if ((ring->flags & MEMIF_RING_FLAG_MASK_INT) == 0) {
> --
> 2.7.4
Jakub Grajciar Sept. 21, 2020, 9:03 a.m. UTC | #3
Hi Honnappa,

Inline comments...

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Saturday, September 19, 2020 12:49 AM
> To: Phil Yang <Phil.Yang@arm.com>; Jakub Grajciar -X (jgrajcia - PANTHEON
> TECH SRO at Cisco) <jgrajcia@cisco.com>; dev@dpdk.org
> Cc: Ruifeng Wang <Ruifeng.Wang@arm.com>; nd <nd@arm.com>; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: RE: [PATCH] net/memif: relax barrier for zero copy path
> 
> Hi Jakub,
> 	I am trying to review this patch. I am having difficulty in understanding
> the implementation for the queue/ring, appreciate if you could help me
> understand the logic.

'ring' refers to a ring buffer holding packet descriptors. These descriptors hold metadata about the packet (packet buffer address, length, etc..).
'queues' are a representation of rings and buffers  (+ some metadata). In more detail, one ring (S2M) and packet buffers allocated for this ring would be represented as 'tx queue' for the slave and 'rx queue' for the master.

> 
> 1) The S2M queues - are used to send packets from slave to master. My
> understanding is that, the slave thread would call 'eth_memif_tx_zc' and the
> master thread would call 'eth_memif_rx_zc'. Is this correct?
> 2) The M2S queues - are used to send packets from master to slave. Here the
> slave thread would call 'eth_memif_rx_zc' and the master thread would call
> 'eth_memif_tx_zc'. Is this correct?

This is inded correct.

> 
> Thank you,
> Honnappa
> 
> > -----Original Message-----
> > From: Phil Yang <phil.yang@arm.com>
> > Sent: Friday, September 11, 2020 12:38 AM
> > To: jgrajcia@cisco.com; dev@dpdk.org
> > Cc: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> > <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> > Subject: [PATCH] net/memif: relax barrier for zero copy path
> >
> > Using 'rte_mb' to synchronize the shared ring head/tail between
> > producer and consumer will stall the pipeline and damage performance
> > on the weak memory model platforms, such like aarch64.
> >
> > Relax the expensive barrier with c11 atomic with explicit memory
> > ordering can improve 3.6% performance on throughput.

My question here is: `rte_mb` is supposed to make sure that head/tail pointer are not updated before the packets are written into shared memory. Does the atomic ensures that the packets are written into shared memory before head/tail pointers are updated?
Phil Yang Sept. 21, 2020, 10:22 a.m. UTC | #4
Jakub Grajciar -X (jgrajcia - PANTHEON TECH SRO at Cisco) writes:

<snip>

> > Subject: RE: [PATCH] net/memif: relax barrier for zero copy path
> >
> > Hi Jakub,
> > 	I am trying to review this patch. I am having difficulty in
> understanding
> > the implementation for the queue/ring, appreciate if you could help me
> > understand the logic.
> 
> 'ring' refers to a ring buffer holding packet descriptors. These descriptors
> hold metadata about the packet (packet buffer address, length, etc..).
> 'queues' are a representation of rings and buffers  (+ some metadata). In
> more detail, one ring (S2M) and packet buffers allocated for this ring would
> be represented as 'tx queue' for the slave and 'rx queue' for the master.
> 
> >
> > 1) The S2M queues - are used to send packets from slave to master. My
> > understanding is that, the slave thread would call 'eth_memif_tx_zc' and
> the
> > master thread would call 'eth_memif_rx_zc'. Is this correct?
> > 2) The M2S queues - are used to send packets from master to slave. Here
> the
> > slave thread would call 'eth_memif_rx_zc' and the master thread would call
> > 'eth_memif_tx_zc'. Is this correct?
> 
> This is inded correct.
 
<snip>

> > >
> > > Using 'rte_mb' to synchronize the shared ring head/tail between
> > > producer and consumer will stall the pipeline and damage performance
> > > on the weak memory model platforms, such like aarch64.
> > >
> > > Relax the expensive barrier with c11 atomic with explicit memory
> > > ordering can improve 3.6% performance on throughput.
> 
> My question here is: `rte_mb` is supposed to make sure that head/tail
> pointer are not updated before the packets are written into shared memory.
> Does the atomic ensures that the packets are written into shared memory
> before head/tail pointers are updated?

Yes, it does. 
The atomic store-release acts as a one-way barrier here to make sure all the memory accesses before the store-release are observed before it.


Thanks,
Phil
Jakub Grajciar Sept. 21, 2020, 12:21 p.m. UTC | #5
> > > >
> > > > Using 'rte_mb' to synchronize the shared ring head/tail between
> > > > producer and consumer will stall the pipeline and damage
> > > > performance on the weak memory model platforms, such like aarch64.
> > > >
> > > > Relax the expensive barrier with c11 atomic with explicit memory
> > > > ordering can improve 3.6% performance on throughput.
> >
> > My question here is: `rte_mb` is supposed to make sure that head/tail
> > pointer are not updated before the packets are written into shared
> memory.
> > Does the atomic ensures that the packets are written into shared
> > memory before head/tail pointers are updated?
> 
> Yes, it does.
> The atomic store-release acts as a one-way barrier here to make sure all the
> memory accesses before the store-release are observed before it.

Ok then, since the sync is there and the rest seems fine to me.

Thanks,
Jakub

Reviewed-by: Jakub Grajciar <jgrajcia@cisco.com>
Ferruh Yigit Sept. 21, 2020, 1:27 p.m. UTC | #6
On 9/21/2020 1:21 PM, Jakub Grajciar -X (jgrajcia - PANTHEON TECH SRO at 
Cisco) wrote:
>>>>>
>>>>> Using 'rte_mb' to synchronize the shared ring head/tail between
>>>>> producer and consumer will stall the pipeline and damage
>>>>> performance on the weak memory model platforms, such like aarch64.
>>>>>
>>>>> Relax the expensive barrier with c11 atomic with explicit memory
>>>>> ordering can improve 3.6% performance on throughput.
>>>
>>> My question here is: `rte_mb` is supposed to make sure that head/tail
>>> pointer are not updated before the packets are written into shared
>> memory.
>>> Does the atomic ensures that the packets are written into shared
>>> memory before head/tail pointers are updated?
>>
>> Yes, it does.
>> The atomic store-release acts as a one-way barrier here to make sure all the
>> memory accesses before the store-release are observed before it.
> 
> Ok then, since the sync is there and the rest seems fine to me.
> 
> Thanks,
> Jakub
> 
> Reviewed-by: Jakub Grajciar <jgrajcia@cisco.com>
> 

Applied to dpdk-next-net/main, thanks.
Honnappa Nagarahalli Sept. 21, 2020, 7:28 p.m. UTC | #7
<snip>

> >
> > Hi Jakub,
> > 	I am trying to review this patch. I am having difficulty in
> > understanding the implementation for the queue/ring, appreciate if you
> > could help me understand the logic.
> 
> 'ring' refers to a ring buffer holding packet descriptors. These descriptors
> hold metadata about the packet (packet buffer address, length, etc..).
> 'queues' are a representation of rings and buffers  (+ some metadata). In
> more detail, one ring (S2M) and packet buffers allocated for this ring would
> be represented as 'tx queue' for the slave and 'rx queue' for the master.
Thanks Jakub. I could run the testpmd app and was able to understand the implementation. I found some issues and I have sent out a patch series [1].

[1] https://patches.dpdk.org/patch/78212/

> 
> >
> > 1) The S2M queues - are used to send packets from slave to master. My
> > understanding is that, the slave thread would call 'eth_memif_tx_zc'
> > and the master thread would call 'eth_memif_rx_zc'. Is this correct?
> > 2) The M2S queues - are used to send packets from master to slave.
> > Here the slave thread would call 'eth_memif_rx_zc' and the master
> > thread would call 'eth_memif_tx_zc'. Is this correct?
> 
> This is inded correct.
> 
> >
> > Thank you,
> > Honnappa
> >
> > > -----Original Message-----
> > > From: Phil Yang <phil.yang@arm.com>
> > > Sent: Friday, September 11, 2020 12:38 AM
> > > To: jgrajcia@cisco.com; dev@dpdk.org
> > > Cc: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng
> > > Wang <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> > > Subject: [PATCH] net/memif: relax barrier for zero copy path
> > >
> > > Using 'rte_mb' to synchronize the shared ring head/tail between
> > > producer and consumer will stall the pipeline and damage performance
> > > on the weak memory model platforms, such like aarch64.
> > >
> > > Relax the expensive barrier with c11 atomic with explicit memory
> > > ordering can improve 3.6% performance on throughput.
> 
> My question here is: `rte_mb` is supposed to make sure that head/tail
> pointer are not updated before the packets are written into shared memory.
> Does the atomic ensures that the packets are written into shared memory
> before head/tail pointers are updated?

Patch
diff mbox series

diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
index c1c7e9f..a19c0f3 100644
--- a/drivers/net/memif/rte_eth_memif.c
+++ b/drivers/net/memif/rte_eth_memif.c
@@ -253,7 +253,12 @@  memif_free_stored_mbufs(struct pmd_process_private *proc_private, struct memif_q
 	memif_ring_t *ring = memif_get_ring_from_queue(proc_private, mq);
 
 	/* FIXME: improve performance */
-	while (mq->last_tail != ring->tail) {
+	/* The ring->tail acts as a guard variable between Tx and Rx
+	 * threads, so using load-acquire pairs with store-release
+	 * to synchronize it between threads.
+	 */
+	while (mq->last_tail != __atomic_load_n(&ring->tail,
+						__ATOMIC_ACQUIRE)) {
 		RTE_MBUF_PREFETCH_TO_FREE(mq->buffers[(mq->last_tail + 1) & mask]);
 		/* Decrement refcnt and free mbuf. (current segment) */
 		rte_mbuf_refcnt_update(mq->buffers[mq->last_tail & mask], -1);
@@ -455,7 +460,11 @@  eth_memif_rx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	mask = ring_size - 1;
 
 	cur_slot = mq->last_tail;
-	last_slot = ring->tail;
+	/* The ring->tail acts as a guard variable between Tx and Rx
+	 * threads, so using load-acquire pairs with store-release
+	 * to synchronize it between threads.
+	 */
+	last_slot = __atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE);
 	if (cur_slot == last_slot)
 		goto refill;
 	n_slots = last_slot - cur_slot;
@@ -501,7 +510,11 @@  eth_memif_rx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 
 /* Supply master with new buffers */
 refill:
-	head = ring->head;
+	/* The ring->head acts as a guard variable between Tx and Rx
+	 * threads, so using load-acquire pairs with store-release
+	 * to synchronize it between threads.
+	 */
+	head = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE);
 	n_slots = ring_size - head + mq->last_tail;
 
 	if (n_slots < 32)
@@ -526,8 +539,7 @@  eth_memif_rx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 			(uint8_t *)proc_private->regions[d0->region]->addr;
 	}
 no_free_mbufs:
-	rte_mb();
-	ring->head = head;
+	__atomic_store_n(&ring->head, head, __ATOMIC_RELEASE);
 
 	mq->n_pkts += n_rx_pkts;
 
@@ -723,8 +735,12 @@  eth_memif_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	memif_free_stored_mbufs(proc_private, mq);
 
 	/* ring type always MEMIF_RING_S2M */
-	slot = ring->head;
-	n_free = ring_size - ring->head + mq->last_tail;
+	/* The ring->head acts as a guard variable between Tx and Rx
+	 * threads, so using load-acquire pairs with store-release
+	 * to synchronize it between threads.
+	 */
+	slot = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE);
+	n_free = ring_size - slot + mq->last_tail;
 
 	int used_slots;
 
@@ -778,12 +794,11 @@  eth_memif_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	}
 
 no_free_slots:
-	rte_mb();
 	/* update ring pointers */
 	if (type == MEMIF_RING_S2M)
-		ring->head = slot;
+		__atomic_store_n(&ring->head, slot, __ATOMIC_RELEASE);
 	else
-		ring->tail = slot;
+		__atomic_store_n(&ring->tail, slot, __ATOMIC_RELEASE);
 
 	/* Send interrupt, if enabled. */
 	if ((ring->flags & MEMIF_RING_FLAG_MASK_INT) == 0) {