[dpdk-stable] [PATCH 1/4] net/memif: do not update local copy of tail in tx function

[dpdk-stable] [PATCH 1/4] net/memif: do not update local copy of tail in tx function

[Honnappa Nagarahalli]

In the case of S2M queues, the receiver synchronizes with the sender
(i.e. informs of the packets it has received) using ring->tail.
Hence, the sender does not need to update last_tail.

In the case of M2S queues, the receiver uses last_tail to
keep track of the descriptors it has received. The
sender is not required to update the last_tail. Updating
the last_tail makes it a shared variable between the
transmitter and receiver affecting the performance.

Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
Cc: jgrajcia@cisco.com
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
---
 drivers/net/memif/rte_eth_memif.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
index c1c7e9f8d..fd7dbc53e 100644
--- a/drivers/net/memif/rte_eth_memif.c
+++ b/drivers/net/memif/rte_eth_memif.c
@@ -568,12 +568,10 @@ eth_memif_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	ring_size = 1 << mq->log2_ring_size;
 	mask = ring_size - 1;
 
-	n_free = __atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE) - mq->last_tail;
-	mq->last_tail += n_free;
-
 	if (type == MEMIF_RING_S2M) {
 		slot = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE);
-		n_free = ring_size - slot + mq->last_tail;
+		n_free = ring_size - slot +
+				__atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE);
 	} else {
 		slot = __atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE);
 		n_free = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE) - slot;
-- 
2.17.1

[dpdk-stable] [PATCH 2/4] net/memif: relax the load of ring tail pointer for M2S ring

[Honnappa Nagarahalli]

For M2S rings, ring->tail is updated by the sender and eth_memif_tx
function is called in the context of sending thread. The loads in
the sender do not need to synchronize with its own stores.

Fixes: a2aafb9aa651 ("net/memif: optimize with one-way barrier")
Cc: phil.yang@arm.com
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
---
 drivers/net/memif/rte_eth_memif.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
index fd7dbc53e..0d064c8fa 100644
--- a/drivers/net/memif/rte_eth_memif.c
+++ b/drivers/net/memif/rte_eth_memif.c
@@ -573,7 +573,13 @@ eth_memif_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		n_free = ring_size - slot +
 				__atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE);
 	} else {
-		slot = __atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE);
+		/* For M2S queues ring->tail is updated by the sender and
+		 * this function is called in the context of sending thread.
+		 * The loads in the sender do not need to synchronize with
+		 * its own stores. Hence, the following load can be a
+		 * relaxed load.
+		 */
+		slot = __atomic_load_n(&ring->tail, __ATOMIC_RELAXED);
 		n_free = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE) - slot;
 	}
 
-- 
2.17.1

[dpdk-stable] [PATCH 3/4] net/memif: relax the load of ring head pointer for S2M ring

[Honnappa Nagarahalli]

For S2M rings, ring->head is updated by the sender and eth_memif_tx
function is called in the context of sending thread. The loads in
the sender do not need to synchronize with its own stores.

Fixes: a2aafb9aa651 ("net/memif: optimize with one-way barrier")
Cc: phil.yang@arm.com
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
---
 drivers/net/memif/rte_eth_memif.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
index 0d064c8fa..435c6345c 100644
--- a/drivers/net/memif/rte_eth_memif.c
+++ b/drivers/net/memif/rte_eth_memif.c
@@ -569,7 +569,13 @@ eth_memif_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	mask = ring_size - 1;
 
 	if (type == MEMIF_RING_S2M) {
-		slot = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE);
+		/* For S2M queues ring->head is updated by the sender and
+		 * this function is called in the context of sending thread.
+		 * The loads in the sender do not need to synchronize with
+		 * its own stores. Hence, the following load can be a
+		 * relaxed load.
+		 */
+		slot = __atomic_load_n(&ring->head, __ATOMIC_RELAXED);
 		n_free = ring_size - slot +
 				__atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE);
 	} else {
-- 
2.17.1

[dpdk-stable] [PATCH 4/4] net/memif: relax the load of ring head pointer for M2S ring

[Honnappa Nagarahalli]

For M2S rings, ring->head is updated by the receiver and eth_memif_rx
function is called in the context of receiving thread. The loads in
the sender do not need to synchronize with its own stores.

Fixes: a2aafb9aa651 ("net/memif: optimize with one-way barrier")
Cc: phil.yang@arm.com
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
---
 drivers/net/memif/rte_eth_memif.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
index 435c6345c..1cbb1fd7a 100644
--- a/drivers/net/memif/rte_eth_memif.c
+++ b/drivers/net/memif/rte_eth_memif.c
@@ -405,7 +405,11 @@ eth_memif_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 
 refill:
 	if (type == MEMIF_RING_M2S) {
-		head = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE);
+		/* ring->head is updated by the receiver and this function
+		 * is called in the context of receiver thread. The loads in
+		 * the receiver do not need to synchronize with its own stores.
+		 */
+		head = __atomic_load_n(&ring->head, __ATOMIC_RELAXED);
 		n_slots = ring_size - head + mq->last_tail;
 
 		while (n_slots--) {
-- 
2.17.1

[dpdk-stable] [PATCH v2 1/8] net/memif: do not update local copy of tail in tx function

[Honnappa Nagarahalli]

In the case of S2M queues, the receiver synchronizes with the sender
(i.e. informs of the packets it has received) using ring->tail.
Hence, the sender does not need to update last_tail.

In the case of M2S queues, the receiver uses last_tail to
keep track of the descriptors it has received. The
sender is not required to update the last_tail. Updating
the last_tail makes it a shared variable between the
transmitter and receiver affecting the performance.

Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
Cc: jgrajcia@cisco.com
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 drivers/net/memif/rte_eth_memif.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
index a19c0f3e6..130099f2e 100644
--- a/drivers/net/memif/rte_eth_memif.c
+++ b/drivers/net/memif/rte_eth_memif.c
@@ -580,12 +580,10 @@ eth_memif_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	ring_size = 1 << mq->log2_ring_size;
 	mask = ring_size - 1;
 
-	n_free = __atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE) - mq->last_tail;
-	mq->last_tail += n_free;
-
 	if (type == MEMIF_RING_S2M) {
 		slot = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE);
-		n_free = ring_size - slot + mq->last_tail;
+		n_free = ring_size - slot +
+				__atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE);
 	} else {
 		slot = __atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE);
 		n_free = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE) - slot;
-- 
2.17.1

[dpdk-stable] [PATCH v2 2/8] net/memif: relax the load of ring tail pointer for M2S ring

[Honnappa Nagarahalli]

For M2S rings, ring->tail is updated by the sender and eth_memif_tx
function is called in the context of sending thread. The loads in
the sender do not need to synchronize with its own stores.

Fixes: a2aafb9aa651 ("net/memif: optimize with one-way barrier")
Cc: phil.yang@arm.com
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 drivers/net/memif/rte_eth_memif.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
index 130099f2e..8bacacaa8 100644
--- a/drivers/net/memif/rte_eth_memif.c
+++ b/drivers/net/memif/rte_eth_memif.c
@@ -585,7 +585,13 @@ eth_memif_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		n_free = ring_size - slot +
 				__atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE);
 	} else {
-		slot = __atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE);
+		/* For M2S queues ring->tail is updated by the sender and
+		 * this function is called in the context of sending thread.
+		 * The loads in the sender do not need to synchronize with
+		 * its own stores. Hence, the following load can be a
+		 * relaxed load.
+		 */
+		slot = __atomic_load_n(&ring->tail, __ATOMIC_RELAXED);
 		n_free = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE) - slot;
 	}
 
-- 
2.17.1

[dpdk-stable] [PATCH v2 3/8] net/memif: relax the load of ring head pointer for M2S ring

[Honnappa Nagarahalli]

For M2S rings, ring->head is updated by the receiver and eth_memif_rx
function is called in the context of receiving thread. The loads in
the receiver do not need to synchronize with its own stores.

Fixes: a2aafb9aa651 ("net/memif: optimize with one-way barrier")
Cc: phil.yang@arm.com
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 drivers/net/memif/rte_eth_memif.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
index 8bacacaa8..159b45180 100644
--- a/drivers/net/memif/rte_eth_memif.c
+++ b/drivers/net/memif/rte_eth_memif.c
@@ -410,7 +410,11 @@ eth_memif_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 
 refill:
 	if (type == MEMIF_RING_M2S) {
-		head = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE);
+		/* ring->head is updated by the receiver and this function
+		 * is called in the context of receiver thread. The loads in
+		 * the receiver do not need to synchronize with its own stores.
+		 */
+		head = __atomic_load_n(&ring->head, __ATOMIC_RELAXED);
 		n_slots = ring_size - head + mq->last_tail;
 
 		while (n_slots--) {
-- 
2.17.1

[dpdk-stable] [PATCH v2 4/8] net/memif: relax the load of ring head pointer for S2M ring

[Honnappa Nagarahalli]

For S2M rings, ring->head is updated by the sender and eth_memif_tx
function is called in the context of sending thread. The loads in
the sender do not need to synchronize with its own stores.

Fixes: a2aafb9aa651 ("net/memif: optimize with one-way barrier")
Cc: phil.yang@arm.com
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 drivers/net/memif/rte_eth_memif.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
index 159b45180..96db76121 100644
--- a/drivers/net/memif/rte_eth_memif.c
+++ b/drivers/net/memif/rte_eth_memif.c
@@ -585,7 +585,13 @@ eth_memif_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	mask = ring_size - 1;
 
 	if (type == MEMIF_RING_S2M) {
-		slot = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE);
+		/* For S2M queues ring->head is updated by the sender and
+		 * this function is called in the context of sending thread.
+		 * The loads in the sender do not need to synchronize with
+		 * its own stores. Hence, the following load can be a
+		 * relaxed load.
+		 */
+		slot = __atomic_load_n(&ring->head, __ATOMIC_RELAXED);
 		n_free = ring_size - slot +
 				__atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE);
 	} else {
-- 
2.17.1

Re: [dpdk-stable] [dpdk-dev] [PATCH v2 1/8] net/memif: do not update local copy of tail in tx function

[Stephen Hemminger]

On Mon, 28 Sep 2020 14:03:27 -0500
Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> wrote:

> In the case of S2M queues, the receiver synchronizes with the sender
> (i.e. informs of the packets it has received) using ring->tail.
> Hence, the sender does not need to update last_tail.
> 
> In the case of M2S queues, the receiver uses last_tail to
> keep track of the descriptors it has received. The
> sender is not required to update the last_tail. Updating
> the last_tail makes it a shared variable between the
> transmitter and receiver affecting the performance.
> 
> Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
> Cc: jgrajcia@cisco.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

This patch series will conflict with the pending master/slave patchset.
Please let the master/slave renaming go in first.

Re: [dpdk-stable] [dpdk-dev] [PATCH v2 1/8] net/memif: do not update local copy of tail in tx function

[Honnappa Nagarahalli]

<snip>

> 
> On Mon, 28 Sep 2020 14:03:27 -0500
> Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> wrote:
> 
> > In the case of S2M queues, the receiver synchronizes with the sender
> > (i.e. informs of the packets it has received) using ring->tail.
> > Hence, the sender does not need to update last_tail.
> >
> > In the case of M2S queues, the receiver uses last_tail to keep track
> > of the descriptors it has received. The sender is not required to
> > update the last_tail. Updating the last_tail makes it a shared
> > variable between the transmitter and receiver affecting the
> > performance.
> >
> > Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
> > Cc: jgrajcia@cisco.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> 
> This patch series will conflict with the pending master/slave patchset.
> Please let the master/slave renaming go in first.
+1, review can go on.

Re: [dpdk-stable] [PATCH v2 1/8] net/memif: do not update local copy of tail in tx function

[Honnappa Nagarahalli]

Hi Jakub,
	Appreciate if you could review this series and provide any comments you might have.

Thank you,
Honnappa

> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Sent: Monday, September 28, 2020 2:03 PM
> To: dev@dpdk.org; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> Phil Yang <Phil.Yang@arm.com>; jgrajcia@cisco.com; ferruh.yigit@intel.com
> Cc: nd <nd@arm.com>; stable@dpdk.org
> Subject: [PATCH v2 1/8] net/memif: do not update local copy of tail in tx
> function
> 
> In the case of S2M queues, the receiver synchronizes with the sender (i.e.
> informs of the packets it has received) using ring->tail.
> Hence, the sender does not need to update last_tail.
> 
> In the case of M2S queues, the receiver uses last_tail to keep track of the
> descriptors it has received. The sender is not required to update the last_tail.
> Updating the last_tail makes it a shared variable between the transmitter and
> receiver affecting the performance.
> 
> Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
> Cc: jgrajcia@cisco.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>  drivers/net/memif/rte_eth_memif.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/memif/rte_eth_memif.c
> b/drivers/net/memif/rte_eth_memif.c
> index a19c0f3e6..130099f2e 100644
> --- a/drivers/net/memif/rte_eth_memif.c
> +++ b/drivers/net/memif/rte_eth_memif.c
> @@ -580,12 +580,10 @@ eth_memif_tx(void *queue, struct rte_mbuf **bufs,
> uint16_t nb_pkts)
>  	ring_size = 1 << mq->log2_ring_size;
>  	mask = ring_size - 1;
> 
> -	n_free = __atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE) - mq-
> >last_tail;
> -	mq->last_tail += n_free;
> -
>  	if (type == MEMIF_RING_S2M) {
>  		slot = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE);
> -		n_free = ring_size - slot + mq->last_tail;
> +		n_free = ring_size - slot +
> +				__atomic_load_n(&ring->tail,
> __ATOMIC_ACQUIRE);
>  	} else {
>  		slot = __atomic_load_n(&ring->tail, __ATOMIC_ACQUIRE);
>  		n_free = __atomic_load_n(&ring->head, __ATOMIC_ACQUIRE)
> - slot;
> --
> 2.17.1

Re: [dpdk-stable] [PATCH v2 1/8] net/memif: do not update local copy of tail in tx function

[Jakub Grajciar -X (jgrajcia - PANTHEON TECH SRO at Cisco)]

> > -----Original Message-----
> > From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Sent: Monday, September 28, 2020 2:03 PM
> > To: dev@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>;
> > Phil Yang <Phil.Yang@arm.com>; jgrajcia@cisco.com;
> > ferruh.yigit@intel.com
> > Cc: nd <nd@arm.com>; stable@dpdk.org
> > Subject: [PATCH v2 1/8] net/memif: do not update local copy of tail in
> > tx function
> >
> > In the case of S2M queues, the receiver synchronizes with the sender (i.e.
> > informs of the packets it has received) using ring->tail.
> > Hence, the sender does not need to update last_tail.
> >
> > In the case of M2S queues, the receiver uses last_tail to keep track
> > of the descriptors it has received. The sender is not required to update the
> last_tail.
> > Updating the last_tail makes it a shared variable between the
> > transmitter and receiver affecting the performance.

Hi Honnappa,

The patch series is looking good.

Reviewed-by: Jakub Grajciar <jgrajcia@cisco.com>

Re: [dpdk-stable] [PATCH v2 1/8] net/memif: do not update local copy of tail in tx function

[Ferruh Yigit]

On 10/9/2020 12:23 PM, Jakub Grajciar -X (jgrajcia - PANTHEON TECH SRO at Cisco) 
wrote:
>>> -----Original Message-----
>>> From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>> Sent: Monday, September 28, 2020 2:03 PM
>>> To: dev@dpdk.org; Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com>;
>>> Phil Yang <Phil.Yang@arm.com>; jgrajcia@cisco.com;
>>> ferruh.yigit@intel.com
>>> Cc: nd <nd@arm.com>; stable@dpdk.org
>>> Subject: [PATCH v2 1/8] net/memif: do not update local copy of tail in
>>> tx function
>>>
>>> In the case of S2M queues, the receiver synchronizes with the sender (i.e.
>>> informs of the packets it has received) using ring->tail.
>>> Hence, the sender does not need to update last_tail.
>>>
>>> In the case of M2S queues, the receiver uses last_tail to keep track
>>> of the descriptors it has received. The sender is not required to update the
>> last_tail.
>>> Updating the last_tail makes it a shared variable between the
>>> transmitter and receiver affecting the performance.
> 
> Hi Honnappa,
> 
> The patch series is looking good.
> 
> Reviewed-by: Jakub Grajciar <jgrajcia@cisco.com>
> 

Series applied to dpdk-next-net/main, thanks.