diff mbox series

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

Message ID 20200928190334.40624-1-honnappa.nagarahalli@arm.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers show
Series [v2,1/8] net/memif: do not update local copy of tail in tx function | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

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

Comments

Stephen Hemminger Sept. 28, 2020, 8:53 p.m. UTC | #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. UTC | #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. UTC | #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. UTC | #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.
diff mbox series

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;