[v2,1/8] net/memif: do not update local copy of tail in tx function

Message ID 20200928190334.40624-1-honnappa.nagarahalli@arm.com
State New
Headers show
Series
  • [v2,1/8] net/memif: do not update local copy of tail in tx function
Related show

Commit Message

Honnappa Nagarahalli Sept. 28, 2020, 7:03 p.m.
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(-)

-- 
2.17.1

Comments

Stephen Hemminger Sept. 28, 2020, 8:53 p.m. | #1
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.
Honnappa Nagarahalli Sept. 29, 2020, 5:24 a.m. | #2
<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.
Honnappa Nagarahalli Oct. 7, 2020, 5:08 p.m. | #3
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
> > -----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>
Ferruh Yigit Oct. 9, 2020, 3:59 p.m. | #5
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.

Patch

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;