various fixes and cleanups for virtio PMD

PATCH Superseded Tiwei Bie 5 participants 24 messages Dec 7, 2017 – Dec 27, 2017 Network drivers

Share & tools

Tiwei Bie

author

2017-12-07 05:31 UTC #0 lore raw

cover various fixes and cleanups for virtio PMD

Tiwei Bie (5):
net/virtio: fix vector Rx break caused by rxq flushing net/virtio: fix typo in LRO support
net/virtio: remove a redundant macro definition for ctrl vq net/virtio: remove redundant macro definitions for vector Rx net/virtio: squeeze repeated blank lines

 drivers/net/virtio/virtio_ethdev.c           |  5 ++---
 drivers/net/virtio/virtio_logs.h             |  1 -
 drivers/net/virtio/virtio_pci.c              |  1 -
 drivers/net/virtio/virtio_pci.h              |  2 --
 drivers/net/virtio/virtio_rxtx.c             |  2 --
 drivers/net/virtio/virtio_rxtx_simple_neon.c |  2 --
 drivers/net/virtio/virtio_rxtx_simple_sse.c  |  2 --
 drivers/net/virtio/virtqueue.c               | 31 +++++++++++++++++++++-------
 drivers/net/virtio/virtqueue.h               |  4 +---
 9 files changed, 27 insertions(+), 23 deletions(-)

Signature · 1 line

2.13.3

Tiwei Bie

author

2017-12-07 05:30 UTC #1 lore raw

PATCH 1/5 net/virtio: fix vector Rx break caused by rxq flushing

The vector Rx will be broken if backend has consumed all the descs in the avail ring before the device is started. Because in current implementation, vector Rx will return immediately without refilling the avail ring if the used ring is empty. So we have to refill the avail ring after flushing the elements in the used ring for vector Rx.

Besides, vector Rx has a different ring layout assumption and mbuf management. So we need to handle it differently.

Fixes d8227497ec5c ("net/virtio: flush Rx queues on start")

Cc [email protected]

Reported-by Antonio Fischetti <[email protected]>

Signed-off-by Tiwei Bie <[email protected]>

---
 drivers/net/virtio/virtio_ethdev.c |  2 +-
 drivers/net/virtio/virtqueue.c     | 31 ++++++++++++++++++++++++-------
 drivers/net/virtio/virtqueue.h     |  2 +-
 3 files changed, 26 insertions(+), 9 deletions(-)

drivers/net/virtio/virtio_ethdev.c +1 -1

index e0328f61d..64a0cc608 100644
@@ -1860,7 +1860,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		rxvq = dev->data->rx_queues[i];
 		/* Flush the old packets */
-		virtqueue_flush(rxvq->vq);
+		virtqueue_rxvq_flush(rxvq->vq);
 		virtqueue_notify(rxvq->vq);
 	}

drivers/net/virtio/virtqueue.c +24 -7

index c3a536f8a..696d0e4a4 100644
@@ -37,6 +37,7 @@
 #include "virtqueue.h"
 #include "virtio_logs.h"
 #include "virtio_pci.h"
+#include "virtio_rxtx_simple.h"
 
 /*
  * Two types of mbuf to be cleaned:

@@ -62,8 +63,10 @@ virtqueue_detatch_unused(struct virtqueue *vq)
 
 /* Flush the elements in the used ring. */
 void
-virtqueue_flush(struct virtqueue *vq)
+virtqueue_rxvq_flush(struct virtqueue *vq)
 {
+	struct virtnet_rx *rxq = &vq->rxq;
+	struct virtio_hw *hw = vq->hw;
 	struct vring_used_elem *uep;
 	struct vq_desc_extra *dxp;
 	uint16_t used_idx, desc_idx;

@@ -74,13 +77,27 @@ virtqueue_flush(struct virtqueue *vq)
 	for (i = 0; i < nb_used; i++) {
 		used_idx = vq->vq_used_cons_idx & (vq->vq_nentries - 1);
 		uep = &vq->vq_ring.used->ring[used_idx];
-		desc_idx = (uint16_t)uep->id;
-		dxp = &vq->vq_descx[desc_idx];
-		if (dxp->cookie != NULL) {
-			rte_pktmbuf_free(dxp->cookie);
-			dxp->cookie = NULL;
+		if (hw->use_simple_rx) {
+			desc_idx = used_idx;
+			rte_pktmbuf_free(vq->sw_ring[desc_idx]);
+			vq->vq_free_cnt++;
+		} else {
+			desc_idx = (uint16_t)uep->id;
+			dxp = &vq->vq_descx[desc_idx];
+			if (dxp->cookie != NULL) {
+				rte_pktmbuf_free(dxp->cookie);
+				dxp->cookie = NULL;
+			}
+			vq_ring_free_chain(vq, desc_idx);
 		}
 		vq->vq_used_cons_idx++;
-		vq_ring_free_chain(vq, desc_idx);
+	}
+
+	if (hw->use_simple_rx) {
+		while (vq->vq_free_cnt >= RTE_VIRTIO_VPMD_RX_REARM_THRESH) {
+			virtio_rxq_rearm_vec(rxq);
+			if (virtqueue_kick_prepare(vq))
+				virtqueue_notify(vq);
+		}
 	}
 }

drivers/net/virtio/virtqueue.h +1 -1

index 2305d91a4..ab466c2db 100644
@@ -304,7 +304,7 @@ void virtqueue_dump(struct virtqueue *vq);
 struct rte_mbuf *virtqueue_detatch_unused(struct virtqueue *vq);
 
 /* Flush the elements in the used ring. */
-void virtqueue_flush(struct virtqueue *vq);
+void virtqueue_rxvq_flush(struct virtqueue *vq);
 
 static inline int
 virtqueue_full(const struct virtqueue *vq)

Signature · 1 line

2.13.3

Maxime Coquelin

Reviewed-by on v1 1/5

2017-12-07 09:14 UTC #2 lore raw

Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing

Quoted 13 lines · Tiwei Bie

The vector Rx will be broken if backend has consumed all the descs in the avail ring before the device is started. Because in current implementation, vector Rx will return immediately without refilling the avail ring if the used ring is empty. So we have to refill the avail ring after flushing the elements in the used ring for vector Rx.
Besides, vector Rx has a different ring layout assumption and mbuf management. So we need to handle it differently.
Fixes d8227497ec5c ("net/virtio: flush Rx queues on start")
Cc [email protected]

Quoted 7 lines

Reported-by Antonio Fischetti <[email protected]>

Signed-off-by Tiwei Bie <[email protected]>

---
  drivers/net/virtio/virtio_ethdev.c |  2 +-
  drivers/net/virtio/virtqueue.c     | 31 ++++++++++++++++++++++++-------
  drivers/net/virtio/virtqueue.h     |  2 +-
  3 files changed, 26 insertions(+), 9 deletions(-)

Reviewed-by Maxime Coquelin <[email protected]>

Thanks,
Maxime

Fischetti, Antonio

2017-12-07 09:30 UTC #3 lore raw

Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing

Thanks Tiwei for working on this, I'll give it a try soon.

Antonio

Quoted 37 lines

-----Original Message-----
From: Maxime Coquelin [mailto:[email protected]]
Sent: Thursday, December 7, 2017 9:15 AM
To: Bie, Tiwei <[email protected]>; [email protected]; [email protected]
Cc: Fischetti, Antonio <[email protected]>; [email protected]
Subject: Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing
Quoted 10 lines · Tiwei Bie
The vector Rx will be broken if backend has consumed all the descs in the avail ring before the device is started. Because in current implementation, vector Rx will return immediately without refilling the avail ring if the used ring is empty. So we have to refill the avail ring after flushing the elements in the used ring for vector Rx.
Besides, vector Rx has a different ring layout assumption and mbuf management. So we need to handle it differently.
Quoted 8 lines
Fixes d8227497ec5c ("net/virtio: flush Rx queues on start")
Cc [email protected]
Reported-by Antonio Fischetti <[email protected]>
Signed-off-by Tiwei Bie <[email protected]>
---
  drivers/net/virtio/virtio_ethdev.c |  2 +-
  drivers/net/virtio/virtqueue.c     | 31 ++++++++++++++++++++++++----
---
Quoted 2 lines
  drivers/net/virtio/virtqueue.h     |  2 +-
  3 files changed, 26 insertions(+), 9 deletions(-)
Reviewed-by Maxime Coquelin <[email protected]>
Thanks,
Maxime

Fischetti, Antonio

Tested-by

2017-12-07 18:10 UTC #4 lore raw

Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing

Hi Tiwei,
I've tested this patch on my 2 test cases described in the previous threads
http://www.dpdk.org/ml/archives/dev/2017-November/081839.html
http://www.dpdk.org/ml/archives/dev/2017-December/082801.html

1. testpmd on the host and testpmd in the guest
2. OvS-DPDK on the host and testpmd in the guest

and it works fine.

I'm using DPDK v17.11 + this patch and I see traffic is now flowing through the VM.

Tested-by Antonio Fischetti <[email protected]>

Quoted 52 lines

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Fischetti, Antonio
Sent: Thursday, December 7, 2017 9:30 AM
To: Maxime Coquelin <[email protected]>; Bie, Tiwei <[email protected]>; [email protected]; [email protected]
Cc: [email protected]
Subject: Re: [dpdk-dev] [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing
Thanks Tiwei for working on this, I'll give it a try soon.
Antonio
Quoted 4 lines
-----Original Message-----
From: Maxime Coquelin [mailto:[email protected]]
Sent: Thursday, December 7, 2017 9:15 AM
To: Bie, Tiwei <[email protected]>; [email protected];
[email protected]
Quoted 6 lines
Cc: Fischetti, Antonio <[email protected]>; [email protected]
Subject: Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing
Quoted 19 lines
Quoted 18 lines · Tiwei Bie
The vector Rx will be broken if backend has consumed all the descs in the avail ring before the device is started. Because in current implementation, vector Rx will return immediately without refilling the avail ring if the used ring is empty. So we have to refill the avail ring after flushing the elements in the used ring for vector Rx.
Besides, vector Rx has a different ring layout assumption and mbuf management. So we need to handle it differently.
Fixes d8227497ec5c ("net/virtio: flush Rx queues on start")
Cc [email protected]
Reported-by Antonio Fischetti <[email protected]>
Signed-off-by Tiwei Bie <[email protected]>
---
  drivers/net/virtio/virtio_ethdev.c |  2 +-
  drivers/net/virtio/virtqueue.c     | 31 ++++++++++++++++++++++++--
--
Quoted 8 lines
---
Quoted 2 lines
  drivers/net/virtio/virtqueue.h     |  2 +-
  3 files changed, 26 insertions(+), 9 deletions(-)
Reviewed-by Maxime Coquelin <[email protected]>
Thanks,
Maxime

Kevin Traynor

2017-12-07 16:00 UTC #5 lore raw

Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing

In reply to Tiwei Bie

Quoted 10 lines · Tiwei Bie

The vector Rx will be broken if backend has consumed all the descs in the avail ring before the device is started. Because in current implementation, vector Rx will return immediately without refilling the avail ring if the used ring is empty. So we have to refill the avail ring after flushing the elements in the used ring for vector Rx.
Besides, vector Rx has a different ring layout assumption and mbuf management. So we need to handle it differently.

Hi Tiwei, does the issue only occur with the vector rx? How about if the simple rx is used because VIRTIO_NET_F_MRG_RXBUF is set?

Kevin.

Quoted 96 lines

Fixes d8227497ec5c ("net/virtio: flush Rx queues on start")

Cc [email protected]

Reported-by Antonio Fischetti <[email protected]>

Signed-off-by Tiwei Bie <[email protected]>

---
 drivers/net/virtio/virtio_ethdev.c |  2 +-
 drivers/net/virtio/virtqueue.c     | 31 ++++++++++++++++++++++++-------
 drivers/net/virtio/virtqueue.h     |  2 +-
 3 files changed, 26 insertions(+), 9 deletions(-)

drivers/net/virtio/virtio_ethdev.c +1 -1

index e0328f61d..64a0cc608 100644
@@ -1860,7 +1860,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		rxvq = dev->data->rx_queues[i];
 		/* Flush the old packets */
-		virtqueue_flush(rxvq->vq);
+		virtqueue_rxvq_flush(rxvq->vq);
 		virtqueue_notify(rxvq->vq);
 	}

drivers/net/virtio/virtqueue.c +24 -7

index c3a536f8a..696d0e4a4 100644
@@ -37,6 +37,7 @@
 #include "virtqueue.h"
 #include "virtio_logs.h"
 #include "virtio_pci.h"
+#include "virtio_rxtx_simple.h"
 
 /*
  * Two types of mbuf to be cleaned:

@@ -62,8 +63,10 @@ virtqueue_detatch_unused(struct virtqueue *vq)
 
 /* Flush the elements in the used ring. */
 void
-virtqueue_flush(struct virtqueue *vq)
+virtqueue_rxvq_flush(struct virtqueue *vq)
 {
+	struct virtnet_rx *rxq = &vq->rxq;
+	struct virtio_hw *hw = vq->hw;
 	struct vring_used_elem *uep;
 	struct vq_desc_extra *dxp;
 	uint16_t used_idx, desc_idx;

@@ -74,13 +77,27 @@ virtqueue_flush(struct virtqueue *vq)
 	for (i = 0; i < nb_used; i++) {
 		used_idx = vq->vq_used_cons_idx & (vq->vq_nentries - 1);
 		uep = &vq->vq_ring.used->ring[used_idx];
-		desc_idx = (uint16_t)uep->id;
-		dxp = &vq->vq_descx[desc_idx];
-		if (dxp->cookie != NULL) {
-			rte_pktmbuf_free(dxp->cookie);
-			dxp->cookie = NULL;
+		if (hw->use_simple_rx) {
+			desc_idx = used_idx;
+			rte_pktmbuf_free(vq->sw_ring[desc_idx]);
+			vq->vq_free_cnt++;
+		} else {
+			desc_idx = (uint16_t)uep->id;
+			dxp = &vq->vq_descx[desc_idx];
+			if (dxp->cookie != NULL) {
+				rte_pktmbuf_free(dxp->cookie);
+				dxp->cookie = NULL;
+			}
+			vq_ring_free_chain(vq, desc_idx);
 		}
 		vq->vq_used_cons_idx++;
-		vq_ring_free_chain(vq, desc_idx);
+	}
+
+	if (hw->use_simple_rx) {
+		while (vq->vq_free_cnt >= RTE_VIRTIO_VPMD_RX_REARM_THRESH) {
+			virtio_rxq_rearm_vec(rxq);
+			if (virtqueue_kick_prepare(vq))
+				virtqueue_notify(vq);
+		}
 	}
 }

drivers/net/virtio/virtqueue.h +1 -1

index 2305d91a4..ab466c2db 100644
@@ -304,7 +304,7 @@ void virtqueue_dump(struct virtqueue *vq);
 struct rte_mbuf *virtqueue_detatch_unused(struct virtqueue *vq);
 
 /* Flush the elements in the used ring. */
-void virtqueue_flush(struct virtqueue *vq);
+void virtqueue_rxvq_flush(struct virtqueue *vq);
 
 static inline int
 virtqueue_full(const struct virtqueue *vq)

Tiwei Bie

author

2017-12-08 02:30 UTC #6 lore raw

Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing

Quoted 8 lines · Kevin Traynor

Quoted 7 lines · Tiwei Bie
The vector Rx will be broken if backend has consumed all the descs in the avail ring before the device is started. Because in current implementation, vector Rx will return immediately without refilling the avail ring if the used ring is empty. So we have to refill the avail ring after flushing the elements in the used ring for vector Rx.

Quoted 6 lines

Quoted 3 lines
Besides, vector Rx has a different ring layout assumption and mbuf management. So we need to handle it differently.
Hi Tiwei, does the issue only occur with the vector rx? How about if the simple rx is used because VIRTIO_NET_F_MRG_RXBUF is set?

Hi Kevin,

Yes, you are right! The issue only occurs with the vector Rx. The vector Rx (i.e. the simple Rx) won't be used if VIRTIO_NET_F_MRG_RXBUF is set.

Best regards,
Tiwei Bie

Kevin Traynor

2017-12-08 10:17 UTC #7 lore raw

Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing

Quoted 16 lines · Tiwei Bie

Quoted 8 lines · Kevin Traynor
Quoted 7 lines · Tiwei Bie
The vector Rx will be broken if backend has consumed all the descs in the avail ring before the device is started. Because in current implementation, vector Rx will return immediately without refilling the avail ring if the used ring is empty. So we have to refill the avail ring after flushing the elements in the used ring for vector Rx.
Quoted 6 lines
Quoted 3 lines
Besides, vector Rx has a different ring layout assumption and mbuf management. So we need to handle it differently.
Hi Tiwei, does the issue only occur with the vector rx? How about if the simple rx is used because VIRTIO_NET_F_MRG_RXBUF is set?

Quoted 9 lines

Hi Kevin,
Yes, you are right! The issue only occurs with the vector Rx. The vector Rx (i.e. the simple Rx) won't be used if VIRTIO_NET_F_MRG_RXBUF is set.
Best regards,
Tiwei Bie

Thanks for clarifying this Tiwei,

Kevin.

Tiwei Bie

author

2017-12-07 05:30 UTC #8 lore raw

PATCH 2/5 net/virtio: fix typo in LRO support

In reply to Tiwei Bie

Fixes 86d59b21468a ("net/virtio: support LRO")

Fixes ec9f3d122a58 ("net/virtio: revert not claiming LRO support")

Cc [email protected]

Signed-off-by Tiwei Bie <[email protected]>

---
 drivers/net/virtio/virtio_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/virtio/virtio_ethdev.c +1 -1

index 64a0cc608..9caa133c9 100644
@@ -1754,7 +1754,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 
 	if (rxmode->enable_lro &&
 		(!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4) ||
-			!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4))) {
+			!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO6))) {
 		PMD_DRV_LOG(ERR,
 			"Large Receive Offload not available on this host");
 		return -ENOTSUP;

Signature · 1 line

2.13.3

Maxime Coquelin

Reviewed-by on v1 2/5

2017-12-07 09:15 UTC #9 lore raw

Re: [PATCH 2/5] net/virtio: fix typo in LRO support

Quoted 15 lines · Tiwei Bie

Fixes 86d59b21468a ("net/virtio: support LRO")
Fixes ec9f3d122a58 ("net/virtio: revert not claiming LRO support")
Cc [email protected]
Signed-off-by Tiwei Bie <[email protected]>
---
  drivers/net/virtio/virtio_ethdev.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/virtio/virtio_ethdev.c +0 -0
index 64a0cc608..9caa133c9 100644

drivers/net/virtio/virtio_ethdev.c virtio_dev_configure(struct rte_eth_dev *dev) · line 1760

Quoted 8 lines

drivers/net/virtio/virtio_ethdev.c +1 -1

@@ -1754,7 +1754,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 
  	if (rxmode->enable_lro &&
  		(!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4) ||
-			!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4))) {
+			!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO6))) {
  		PMD_DRV_LOG(ERR,
  			"Large Receive Offload not available on this host");
  		return -ENOTSUP;

Good catch!

Reviewed-by Maxime Coquelin <[email protected]>

Thanks,
Maxime

Tiwei Bie

author

2017-12-07 05:32 UTC #10 lore raw

PATCH 3/5 net/virtio: remove a redundant macro definition for ctrl vq

In reply to Tiwei Bie

VIRTIO_NET_CTRL_MAC_ADDR_SET is defined two times in virtqueue.h, the second one is obviously not wanted.

Signed-off-by Tiwei Bie <[email protected]>

---
 drivers/net/virtio/virtqueue.h | 2 --
 1 file changed, 2 deletions(-)

drivers/net/virtio/virtqueue.h +0 -2

index ab466c2db..eaf9de13e 100644
@@ -226,8 +226,6 @@ struct virtqueue {
 #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN        1
 #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX        0x8000
 
-#define VIRTIO_NET_CTRL_MAC_ADDR_SET         1
-
 /**
  * This is the first element of the scatter-gather list.  If you don't
  * specify GSO or CSUM features, you can simply ignore the header.

Signature · 1 line

2.13.3

Maxime Coquelin

Reviewed-by on v1 3/5

2017-12-07 09:16 UTC #11 lore raw

Re: [PATCH 3/5] net/virtio: remove a redundant macro definition for ctrl vq

Quoted 14 lines · Tiwei Bie

VIRTIO_NET_CTRL_MAC_ADDR_SET is defined two times in virtqueue.h, the second one is obviously not wanted.
Signed-off-by Tiwei Bie <[email protected]>
---
  drivers/net/virtio/virtqueue.h | 2 --
  1 file changed, 2 deletions(-)
drivers/net/virtio/virtqueue.h +0 -0
index ab466c2db..eaf9de13e 100644

drivers/net/virtio/virtqueue.h struct virtqueue { · line 231

Quoted 8 lines

drivers/net/virtio/virtqueue.h +0 -2

@@ -226,8 +226,6 @@ struct virtqueue {
  #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN        1
  #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX        0x8000
 
-#define VIRTIO_NET_CTRL_MAC_ADDR_SET         1
-
  /**
   * This is the first element of the scatter-gather list.  If you don't
   * specify GSO or CSUM features, you can simply ignore the header.

Reviewed-by Maxime Coquelin <[email protected]>

Thanks,
Maxime

Tiwei Bie

author

2017-12-07 05:32 UTC #12 lore raw

PATCH 4/5 net/virtio: remove redundant macro definitions for vector Rx

In reply to Tiwei Bie

RTE_VIRTIO_VPMD_RX_BURST and RTE_VIRTIO_VPMD_RX_REARM_THRESH have been defined and used in virtio_rxtx_simple.h, but are defined agained in virtio_rxtx_simple_*.c. It just happens to work. So remove the redundant definitions from the *.c files.

Signed-off-by Tiwei Bie <[email protected]>

---
 drivers/net/virtio/virtio_rxtx_simple_neon.c | 2 --
 drivers/net/virtio/virtio_rxtx_simple_sse.c  | 2 --
 2 files changed, 4 deletions(-)

drivers/net/virtio/virtio_rxtx_simple_neon.c +0 -2

index b8b93551f..db15c9f5d 100644
@@ -52,9 +52,7 @@
 
 #include "virtio_rxtx_simple.h"
 
-#define RTE_VIRTIO_VPMD_RX_BURST 32
 #define RTE_VIRTIO_DESC_PER_LOOP 8
-#define RTE_VIRTIO_VPMD_RX_REARM_THRESH RTE_VIRTIO_VPMD_RX_BURST
 
 /* virtio vPMD receive routine, only accept(nb_pkts >= RTE_VIRTIO_DESC_PER_LOOP)
  *

drivers/net/virtio/virtio_rxtx_simple_sse.c +0 -2

index 94f65143b..512a69c71 100644
@@ -54,9 +54,7 @@
 
 #include "virtio_rxtx_simple.h"
 
-#define RTE_VIRTIO_VPMD_RX_BURST 32
 #define RTE_VIRTIO_DESC_PER_LOOP 8
-#define RTE_VIRTIO_VPMD_RX_REARM_THRESH RTE_VIRTIO_VPMD_RX_BURST
 
 /* virtio vPMD receive routine, only accept(nb_pkts >= RTE_VIRTIO_DESC_PER_LOOP)
  *

Signature · 1 line

2.13.3

Maxime Coquelin

Reviewed-by on v1 4/5

2017-12-07 09:18 UTC #13 lore raw

Re: [PATCH 4/5] net/virtio: remove redundant macro definitions for vector Rx

Quoted 3 lines · Tiwei Bie

RTE_VIRTIO_VPMD_RX_BURST and RTE_VIRTIO_VPMD_RX_REARM_THRESH have been defined and used in virtio_rxtx_simple.h, but are defined agained in virtio_rxtx_simple_*.c. It just happens to

s/agained/again/

Quoted 7 lines

work. So remove the redundant definitions from the *.c files.
Signed-off-by Tiwei Bie <[email protected]>
---
  drivers/net/virtio/virtio_rxtx_simple_neon.c | 2 --
  drivers/net/virtio/virtio_rxtx_simple_sse.c  | 2 --
  2 files changed, 4 deletions(-)

Apart from above typo:

Reviewed-by Maxime Coquelin <[email protected]>

Thanks,
Maxime

Tiwei Bie

author

2017-12-07 10:39 UTC #14 lore raw

Re: [PATCH 4/5] net/virtio: remove redundant macro definitions for vector Rx

Quoted 6 lines · Maxime Coquelin

Quoted 3 lines · Tiwei Bie
RTE_VIRTIO_VPMD_RX_BURST and RTE_VIRTIO_VPMD_RX_REARM_THRESH have been defined and used in virtio_rxtx_simple.h, but are defined agained in virtio_rxtx_simple_*.c. It just happens to
s/agained/again/

Thanks!

Best regards,
Tiwei Bie

Quoted 14 lines

Quoted 7 lines
work. So remove the redundant definitions from the *.c files.
Signed-off-by Tiwei Bie <[email protected]>
---
  drivers/net/virtio/virtio_rxtx_simple_neon.c | 2 --
  drivers/net/virtio/virtio_rxtx_simple_sse.c  | 2 --
  2 files changed, 4 deletions(-)
Apart from above typo:
Reviewed-by Maxime Coquelin <[email protected]>
Thanks,
Maxime

Tiwei Bie

author

2017-12-07 05:32 UTC #15 lore raw

PATCH 5/5 net/virtio: squeeze repeated blank lines

In reply to Tiwei Bie

Squeeze repeated blank lines with below scripts:

for i in $(find . -name "*.[ch]"); do \
    cat -s $i > /tmp/x && mv /tmp/x $i; done

Signed-off-by Tiwei Bie <[email protected]>

---
 drivers/net/virtio/virtio_ethdev.c | 1 -
 drivers/net/virtio/virtio_logs.h   | 1 -
 drivers/net/virtio/virtio_pci.c    | 1 -
 drivers/net/virtio/virtio_pci.h    | 2 --
 drivers/net/virtio/virtio_rxtx.c   | 2 --
 5 files changed, 7 deletions(-)

drivers/net/virtio/virtio_ethdev.c +0 -1

index 9caa133c9..aa6d494ca 100644
@@ -1801,7 +1801,6 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 	return 0;
 }
 
-
 static int
 virtio_dev_start(struct rte_eth_dev *dev)
 {

drivers/net/virtio/virtio_logs.h +0 -1

index 90a79eaa5..6ce3149d5 100644
@@ -59,7 +59,6 @@
 #define PMD_TX_LOG(level, fmt, args...) do { } while(0)
 #endif
 
-
 #ifdef RTE_LIBRTE_VIRTIO_DEBUG_DRIVER
 #define PMD_DRV_LOG(level, fmt, args...) \
 	RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)

drivers/net/virtio/virtio_pci.c +0 -1

index 9574498fb..684ef560c 100644
@@ -481,7 +481,6 @@ const struct virtio_pci_ops modern_ops = {
 	.notify_queue	= modern_notify_queue,
 };
 
-
 void
 vtpci_read_dev_config(struct virtio_hw *hw, size_t offset,
 		      void *dst, int length)

drivers/net/virtio/virtio_pci.h +0 -2

index 3c5ce66ce..e42962a99 100644
@@ -274,7 +274,6 @@ struct virtio_hw {
 	struct virtqueue **vqs;
 };
 
-
 /*
  * While virtio_hw is stored in shared memory, this structure stores
  * some infos that may vary in the multiple process model locally.

@@ -290,7 +289,6 @@ struct virtio_hw_internal {
 
 extern struct virtio_hw_internal virtio_hw_internal[RTE_MAX_ETHPORTS];
 
-
 /*
  * This structure is just a reference to read
  * net device specific config space; it just a chodu structure

drivers/net/virtio/virtio_rxtx.c +0 -2

index 390c137c8..7ccd1da34 100644
@@ -66,7 +66,6 @@
 #define  VIRTIO_DUMP_PACKET(m, len) do { } while (0)
 #endif
 
-
 #define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
 	ETH_TXQ_FLAGS_NOOFFLOADS)

@@ -175,7 +174,6 @@ virtio_xmit_cleanup(struct virtqueue *vq, uint16_t num)
 	}
 }
 
-
 static inline int
 virtqueue_enqueue_recv_refill(struct virtqueue *vq, struct rte_mbuf *cookie)
 {

Signature · 1 line

2.13.3

Maxime Coquelin

2017-12-07 09:22 UTC #16 lore raw

Re: [PATCH 5/5] net/virtio: squeeze repeated blank lines

Quoted 5 lines · Tiwei Bie

Squeeze repeated blank lines with below scripts:
for i in $(find . -name "*.[ch]"); do \
     cat -s $i > /tmp/x && mv /tmp/x $i; done

Quoted 8 lines

Signed-off-by Tiwei Bie <[email protected]>

---
  drivers/net/virtio/virtio_ethdev.c | 1 -
  drivers/net/virtio/virtio_logs.h   | 1 -
  drivers/net/virtio/virtio_pci.c    | 1 -
  drivers/net/virtio/virtio_pci.h    | 2 --
  drivers/net/virtio/virtio_rxtx.c   | 2 --
  5 files changed, 7 deletions(-)

I don't really like such cleaning patches.
It does not bring added value, but can create conflicts later when backporting or rebasing.

Maxime

Quoted 77 lines

drivers/net/virtio/virtio_ethdev.c +0 -1

index 9caa133c9..aa6d494ca 100644
@@ -1801,7 +1801,6 @@ virtio_dev_configure(struct rte_eth_dev *dev)
  	return 0;
  }
 
-
  static int
  virtio_dev_start(struct rte_eth_dev *dev)
  {

drivers/net/virtio/virtio_logs.h +0 -1

index 90a79eaa5..6ce3149d5 100644
@@ -59,7 +59,6 @@
  #define PMD_TX_LOG(level, fmt, args...) do { } while(0)
  #endif
 
-
  #ifdef RTE_LIBRTE_VIRTIO_DEBUG_DRIVER
  #define PMD_DRV_LOG(level, fmt, args...) \
  	RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)

drivers/net/virtio/virtio_pci.c +0 -1

index 9574498fb..684ef560c 100644
@@ -481,7 +481,6 @@ const struct virtio_pci_ops modern_ops = {
  	.notify_queue	= modern_notify_queue,
  };
 
-
  void
  vtpci_read_dev_config(struct virtio_hw *hw, size_t offset,
  		      void *dst, int length)

drivers/net/virtio/virtio_pci.h +0 -2

index 3c5ce66ce..e42962a99 100644
@@ -274,7 +274,6 @@ struct virtio_hw {
  	struct virtqueue **vqs;
  };
 
-
  /*
   * While virtio_hw is stored in shared memory, this structure stores
   * some infos that may vary in the multiple process model locally.

@@ -290,7 +289,6 @@ struct virtio_hw_internal {
 
  extern struct virtio_hw_internal virtio_hw_internal[RTE_MAX_ETHPORTS];
 
-
  /*
   * This structure is just a reference to read
   * net device specific config space; it just a chodu structure

drivers/net/virtio/virtio_rxtx.c +0 -2

index 390c137c8..7ccd1da34 100644
@@ -66,7 +66,6 @@
  #define  VIRTIO_DUMP_PACKET(m, len) do { } while (0)
  #endif
 
-
  #define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
  	ETH_TXQ_FLAGS_NOOFFLOADS)

@@ -175,7 +174,6 @@ virtio_xmit_cleanup(struct virtqueue *vq, uint16_t num)
  	}
  }
 
-
  static inline int
  virtqueue_enqueue_recv_refill(struct virtqueue *vq, struct rte_mbuf *cookie)
  {

Tiwei Bie

author

2017-12-07 10:33 UTC #17 lore raw

Re: [PATCH 5/5] net/virtio: squeeze repeated blank lines

Quoted 16 lines · Maxime Coquelin

Quoted 5 lines · Tiwei Bie

Squeeze repeated blank lines with below scripts:
for i in $(find . -name "*.[ch]"); do \
     cat -s $i > /tmp/x && mv /tmp/x $i; done

Quoted 8 lines

Signed-off-by Tiwei Bie <[email protected]>

---
  drivers/net/virtio/virtio_ethdev.c | 1 -
  drivers/net/virtio/virtio_logs.h   | 1 -
  drivers/net/virtio/virtio_pci.c    | 1 -
  drivers/net/virtio/virtio_pci.h    | 2 --
  drivers/net/virtio/virtio_rxtx.c   | 2 --
  5 files changed, 7 deletions(-)

Quoted 4 lines

I don't really like such cleaning patches.
It does not bring added value, but can create conflicts later when backporting or rebasing.

Okay. I'll drop this patch in the next version. Thanks!

Best regards,
Tiwei Bie

Tiwei Bie

author

2017-12-11 05:14 UTC #18 lore raw

v2 cover various fixes and cleanups for virtio PMD

In reply to Tiwei Bie

v2:
- refine indent for patch 2
- fix a typo in commit log for patch 4
- drop the blank lines squeezing patch

Tiwei Bie (4):
net/virtio: fix vector Rx break caused by rxq flushing net/virtio: fix typo in LRO support
net/virtio: remove a redundant macro definition for ctrl vq net/virtio: remove redundant macro definitions for vector Rx

 drivers/net/virtio/virtio_ethdev.c           |  4 ++--
 drivers/net/virtio/virtio_rxtx_simple_neon.c |  2 --
 drivers/net/virtio/virtio_rxtx_simple_sse.c  |  2 --
 drivers/net/virtio/virtqueue.c               | 31 +++++++++++++++++++++-------
 drivers/net/virtio/virtqueue.h               |  4 +---
 5 files changed, 27 insertions(+), 16 deletions(-)

Signature · 1 line

2.13.3

Tiwei Bie

author

2017-12-11 05:13 UTC #19 lore raw

PATCH v2 1/4 net/virtio: fix vector Rx break caused by rxq flushing

Besides, vector Rx has a different ring layout assumption and mbuf management. So we need to handle it differently.

Fixes d8227497ec5c ("net/virtio: flush Rx queues on start")

Cc [email protected]

Reported-by Antonio Fischetti <[email protected]>

Signed-off-by Tiwei Bie <[email protected]>

Reviewed-by Maxime Coquelin <[email protected]>

Tested-by Antonio Fischetti <[email protected]>

---
 drivers/net/virtio/virtio_ethdev.c |  2 +-
 drivers/net/virtio/virtqueue.c     | 31 ++++++++++++++++++++++++-------
 drivers/net/virtio/virtqueue.h     |  2 +-
 3 files changed, 26 insertions(+), 9 deletions(-)

drivers/net/virtio/virtio_ethdev.c +1 -1

index e0328f61d..64a0cc608 100644
@@ -1860,7 +1860,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		rxvq = dev->data->rx_queues[i];
 		/* Flush the old packets */
-		virtqueue_flush(rxvq->vq);
+		virtqueue_rxvq_flush(rxvq->vq);
 		virtqueue_notify(rxvq->vq);
 	}

drivers/net/virtio/virtqueue.c +24 -7

index c3a536f8a..696d0e4a4 100644
@@ -37,6 +37,7 @@
 #include "virtqueue.h"
 #include "virtio_logs.h"
 #include "virtio_pci.h"
+#include "virtio_rxtx_simple.h"
 
 /*
  * Two types of mbuf to be cleaned:

@@ -62,8 +63,10 @@ virtqueue_detatch_unused(struct virtqueue *vq)
 
 /* Flush the elements in the used ring. */
 void
-virtqueue_flush(struct virtqueue *vq)
+virtqueue_rxvq_flush(struct virtqueue *vq)
 {
+	struct virtnet_rx *rxq = &vq->rxq;
+	struct virtio_hw *hw = vq->hw;
 	struct vring_used_elem *uep;
 	struct vq_desc_extra *dxp;
 	uint16_t used_idx, desc_idx;

@@ -74,13 +77,27 @@ virtqueue_flush(struct virtqueue *vq)
 	for (i = 0; i < nb_used; i++) {
 		used_idx = vq->vq_used_cons_idx & (vq->vq_nentries - 1);
 		uep = &vq->vq_ring.used->ring[used_idx];
-		desc_idx = (uint16_t)uep->id;
-		dxp = &vq->vq_descx[desc_idx];
-		if (dxp->cookie != NULL) {
-			rte_pktmbuf_free(dxp->cookie);
-			dxp->cookie = NULL;
+		if (hw->use_simple_rx) {
+			desc_idx = used_idx;
+			rte_pktmbuf_free(vq->sw_ring[desc_idx]);
+			vq->vq_free_cnt++;
+		} else {
+			desc_idx = (uint16_t)uep->id;
+			dxp = &vq->vq_descx[desc_idx];
+			if (dxp->cookie != NULL) {
+				rte_pktmbuf_free(dxp->cookie);
+				dxp->cookie = NULL;
+			}
+			vq_ring_free_chain(vq, desc_idx);
 		}
 		vq->vq_used_cons_idx++;
-		vq_ring_free_chain(vq, desc_idx);
+	}
+
+	if (hw->use_simple_rx) {
+		while (vq->vq_free_cnt >= RTE_VIRTIO_VPMD_RX_REARM_THRESH) {
+			virtio_rxq_rearm_vec(rxq);
+			if (virtqueue_kick_prepare(vq))
+				virtqueue_notify(vq);
+		}
 	}
 }

drivers/net/virtio/virtqueue.h +1 -1

index 2305d91a4..ab466c2db 100644
@@ -304,7 +304,7 @@ void virtqueue_dump(struct virtqueue *vq);
 struct rte_mbuf *virtqueue_detatch_unused(struct virtqueue *vq);
 
 /* Flush the elements in the used ring. */
-void virtqueue_flush(struct virtqueue *vq);
+void virtqueue_rxvq_flush(struct virtqueue *vq);
 
 static inline int
 virtqueue_full(const struct virtqueue *vq)

Signature · 1 line

2.13.3

Tiwei Bie

author

2017-12-11 05:13 UTC #20 lore raw

PATCH v2 2/4 net/virtio: fix typo in LRO support

In reply to Tiwei Bie

Fixes 86d59b21468a ("net/virtio: support LRO")

Fixes ec9f3d122a58 ("net/virtio: revert not claiming LRO support")

Cc [email protected]

Signed-off-by Tiwei Bie <[email protected]>

Reviewed-by Maxime Coquelin <[email protected]>

---
 drivers/net/virtio/virtio_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/virtio/virtio_ethdev.c +1 -1

index 64a0cc608..9caa133c9 100644
@@ -1754,7 +1754,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 
 	if (rxmode->enable_lro &&
 		(!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4) ||
-			!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4))) {
+		 !vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO6))) {
 		PMD_DRV_LOG(ERR,
 			"Large Receive Offload not available on this host");
 		return -ENOTSUP;

Signature · 1 line

2.13.3

Tiwei Bie

author

2017-12-11 05:15 UTC #21 lore raw

PATCH v2 3/4 net/virtio: remove a redundant macro definition for ctrl vq

In reply to Tiwei Bie

VIRTIO_NET_CTRL_MAC_ADDR_SET is defined two times in virtqueue.h, the second one is obviously not wanted.

Signed-off-by Tiwei Bie <[email protected]>

Reviewed-by Maxime Coquelin <[email protected]>

---
 drivers/net/virtio/virtqueue.h | 2 --
 1 file changed, 2 deletions(-)

drivers/net/virtio/virtqueue.h +0 -2

index ab466c2db..eaf9de13e 100644
@@ -226,8 +226,6 @@ struct virtqueue {
 #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN        1
 #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX        0x8000
 
-#define VIRTIO_NET_CTRL_MAC_ADDR_SET         1
-
 /**
  * This is the first element of the scatter-gather list.  If you don't
  * specify GSO or CSUM features, you can simply ignore the header.

Signature · 1 line

2.13.3

Tiwei Bie

author

2017-12-11 05:15 UTC #22 lore raw

PATCH v2 4/4 net/virtio: remove redundant macro definitions for vector Rx

In reply to Tiwei Bie

RTE_VIRTIO_VPMD_RX_BURST and RTE_VIRTIO_VPMD_RX_REARM_THRESH have been defined and used in virtio_rxtx_simple.h, but are defined again in virtio_rxtx_simple_*.c. It just happens to work. So remove the redundant definitions from the *.c files.

Signed-off-by Tiwei Bie <[email protected]>

Reviewed-by Maxime Coquelin <[email protected]>

---
 drivers/net/virtio/virtio_rxtx_simple_neon.c | 2 --
 drivers/net/virtio/virtio_rxtx_simple_sse.c  | 2 --
 2 files changed, 4 deletions(-)

drivers/net/virtio/virtio_rxtx_simple_neon.c +0 -2

index b8b93551f..db15c9f5d 100644
@@ -52,9 +52,7 @@
 
 #include "virtio_rxtx_simple.h"
 
-#define RTE_VIRTIO_VPMD_RX_BURST 32
 #define RTE_VIRTIO_DESC_PER_LOOP 8
-#define RTE_VIRTIO_VPMD_RX_REARM_THRESH RTE_VIRTIO_VPMD_RX_BURST
 
 /* virtio vPMD receive routine, only accept(nb_pkts >= RTE_VIRTIO_DESC_PER_LOOP)
  *

drivers/net/virtio/virtio_rxtx_simple_sse.c +0 -2

index 94f65143b..512a69c71 100644
@@ -54,9 +54,7 @@
 
 #include "virtio_rxtx_simple.h"
 
-#define RTE_VIRTIO_VPMD_RX_BURST 32
 #define RTE_VIRTIO_DESC_PER_LOOP 8
-#define RTE_VIRTIO_VPMD_RX_REARM_THRESH RTE_VIRTIO_VPMD_RX_BURST
 
 /* virtio vPMD receive routine, only accept(nb_pkts >= RTE_VIRTIO_DESC_PER_LOOP)
  *

Signature · 1 line

2.13.3

Yuanhan Liu

2017-12-27 14:46 UTC #23 lore raw

In reply to Tiwei Bie

Quoted 4 lines · Tiwei Bie

v2:
- refine indent for patch 2
- fix a typo in commit log for patch 4
- drop the blank lines squeezing patch

Series applied to dpdk-next-virtio.

Thanks.

	--yliu

Quoted 16 lines

Tiwei Bie (4):
net/virtio: fix vector Rx break caused by rxq flushing net/virtio: fix typo in LRO support
net/virtio: remove a redundant macro definition for ctrl vq net/virtio: remove redundant macro definitions for vector Rx
 drivers/net/virtio/virtio_ethdev.c           |  4 ++--
 drivers/net/virtio/virtio_rxtx_simple_neon.c |  2 --
 drivers/net/virtio/virtio_rxtx_simple_sse.c  |  2 --
 drivers/net/virtio/virtqueue.c               | 31 +++++++++++++++++++++-------
 drivers/net/virtio/virtqueue.h               |  4 +---
 5 files changed, 27 insertions(+), 16 deletions(-)
Signature · 1 line
2.13.3