All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] various fixes and cleanups for virtio PMD
@ 2017-12-07  5:30 Tiwei Bie
  2017-12-07  5:30 ` [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing Tiwei Bie
                   ` (5 more replies)
  0 siblings, 6 replies; 24+ messages in thread
From: Tiwei Bie @ 2017-12-07  5:30 UTC (permalink / raw)
  To: yliu, maxime.coquelin, dev

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(-)

-- 
2.13.3

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing
  2017-12-07  5:30 [PATCH 0/5] various fixes and cleanups for virtio PMD Tiwei Bie
@ 2017-12-07  5:30 ` Tiwei Bie
  2017-12-07  9:14   ` Maxime Coquelin
  2017-12-07 16:00   ` Kevin Traynor
  2017-12-07  5:30 ` [PATCH 2/5] net/virtio: fix typo in LRO support Tiwei Bie
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 24+ messages in thread
From: Tiwei Bie @ 2017-12-07  5:30 UTC (permalink / raw)
  To: yliu, maxime.coquelin, dev; +Cc: antonio.fischetti, stable

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: stable@dpdk.org

Reported-by: Antonio Fischetti <antonio.fischetti@intel.com>
Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 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(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index e0328f61d..64a0cc608 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -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);
 	}
 
diff --git a/drivers/net/virtio/virtqueue.c b/drivers/net/virtio/virtqueue.c
index c3a536f8a..696d0e4a4 100644
--- a/drivers/net/virtio/virtqueue.c
+++ b/drivers/net/virtio/virtqueue.c
@@ -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);
+		}
 	}
 }
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 2305d91a4..ab466c2db 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -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)
-- 
2.13.3

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/5] net/virtio: fix typo in LRO support
  2017-12-07  5:30 [PATCH 0/5] various fixes and cleanups for virtio PMD Tiwei Bie
  2017-12-07  5:30 ` [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing Tiwei Bie
@ 2017-12-07  5:30 ` Tiwei Bie
  2017-12-07  9:15   ` Maxime Coquelin
  2017-12-07  5:30 ` [PATCH 3/5] net/virtio: remove a redundant macro definition for ctrl vq Tiwei Bie
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 24+ messages in thread
From: Tiwei Bie @ 2017-12-07  5:30 UTC (permalink / raw)
  To: yliu, maxime.coquelin, dev; +Cc: stable

Fixes: 86d59b21468a ("net/virtio: support LRO")
Fixes: ec9f3d122a58 ("net/virtio: revert not claiming LRO support")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 64a0cc608..9caa133c9 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -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;
-- 
2.13.3

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 3/5] net/virtio: remove a redundant macro definition for ctrl vq
  2017-12-07  5:30 [PATCH 0/5] various fixes and cleanups for virtio PMD Tiwei Bie
  2017-12-07  5:30 ` [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing Tiwei Bie
  2017-12-07  5:30 ` [PATCH 2/5] net/virtio: fix typo in LRO support Tiwei Bie
@ 2017-12-07  5:30 ` Tiwei Bie
  2017-12-07  9:16   ` Maxime Coquelin
  2017-12-07  5:30 ` [PATCH 4/5] net/virtio: remove redundant macro definitions for vector Rx Tiwei Bie
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 24+ messages in thread
From: Tiwei Bie @ 2017-12-07  5:30 UTC (permalink / raw)
  To: yliu, maxime.coquelin, dev

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 <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtqueue.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index ab466c2db..eaf9de13e 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -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.
-- 
2.13.3

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 4/5] net/virtio: remove redundant macro definitions for vector Rx
  2017-12-07  5:30 [PATCH 0/5] various fixes and cleanups for virtio PMD Tiwei Bie
                   ` (2 preceding siblings ...)
  2017-12-07  5:30 ` [PATCH 3/5] net/virtio: remove a redundant macro definition for ctrl vq Tiwei Bie
@ 2017-12-07  5:30 ` Tiwei Bie
  2017-12-07  9:18   ` Maxime Coquelin
  2017-12-07  5:30 ` [PATCH 5/5] net/virtio: squeeze repeated blank lines Tiwei Bie
  2017-12-11  5:13 ` [PATCH v2 0/4] various fixes and cleanups for virtio PMD Tiwei Bie
  5 siblings, 1 reply; 24+ messages in thread
From: Tiwei Bie @ 2017-12-07  5:30 UTC (permalink / raw)
  To: yliu, maxime.coquelin, dev

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 <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtio_rxtx_simple_neon.c | 2 --
 drivers/net/virtio/virtio_rxtx_simple_sse.c  | 2 --
 2 files changed, 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx_simple_neon.c b/drivers/net/virtio/virtio_rxtx_simple_neon.c
index b8b93551f..db15c9f5d 100644
--- a/drivers/net/virtio/virtio_rxtx_simple_neon.c
+++ b/drivers/net/virtio/virtio_rxtx_simple_neon.c
@@ -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)
  *
diff --git a/drivers/net/virtio/virtio_rxtx_simple_sse.c b/drivers/net/virtio/virtio_rxtx_simple_sse.c
index 94f65143b..512a69c71 100644
--- a/drivers/net/virtio/virtio_rxtx_simple_sse.c
+++ b/drivers/net/virtio/virtio_rxtx_simple_sse.c
@@ -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)
  *
-- 
2.13.3

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 5/5] net/virtio: squeeze repeated blank lines
  2017-12-07  5:30 [PATCH 0/5] various fixes and cleanups for virtio PMD Tiwei Bie
                   ` (3 preceding siblings ...)
  2017-12-07  5:30 ` [PATCH 4/5] net/virtio: remove redundant macro definitions for vector Rx Tiwei Bie
@ 2017-12-07  5:30 ` Tiwei Bie
  2017-12-07  9:22   ` Maxime Coquelin
  2017-12-11  5:13 ` [PATCH v2 0/4] various fixes and cleanups for virtio PMD Tiwei Bie
  5 siblings, 1 reply; 24+ messages in thread
From: Tiwei Bie @ 2017-12-07  5:30 UTC (permalink / raw)
  To: yliu, maxime.coquelin, dev

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 <tiwei.bie@intel.com>
---
 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(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 9caa133c9..aa6d494ca 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1801,7 +1801,6 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 	return 0;
 }
 
-
 static int
 virtio_dev_start(struct rte_eth_dev *dev)
 {
diff --git a/drivers/net/virtio/virtio_logs.h b/drivers/net/virtio/virtio_logs.h
index 90a79eaa5..6ce3149d5 100644
--- a/drivers/net/virtio/virtio_logs.h
+++ b/drivers/net/virtio/virtio_logs.h
@@ -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)
diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index 9574498fb..684ef560c 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -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)
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 3c5ce66ce..e42962a99 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -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
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 390c137c8..7ccd1da34 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -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)
 {
-- 
2.13.3

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing
  2017-12-07  5:30 ` [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing Tiwei Bie
@ 2017-12-07  9:14   ` Maxime Coquelin
  2017-12-07  9:30     ` Fischetti, Antonio
  2017-12-07 16:00   ` Kevin Traynor
  1 sibling, 1 reply; 24+ messages in thread
From: Maxime Coquelin @ 2017-12-07  9:14 UTC (permalink / raw)
  To: Tiwei Bie, yliu, dev; +Cc: antonio.fischetti, stable



On 12/07/2017 06:30 AM, Tiwei Bie wrote:
> 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: stable@dpdk.org
> 
> Reported-by: Antonio Fischetti <antonio.fischetti@intel.com>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   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 <maxime.coquelin@redhat.com>

Thanks,
Maxime

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/5] net/virtio: fix typo in LRO support
  2017-12-07  5:30 ` [PATCH 2/5] net/virtio: fix typo in LRO support Tiwei Bie
@ 2017-12-07  9:15   ` Maxime Coquelin
  0 siblings, 0 replies; 24+ messages in thread
From: Maxime Coquelin @ 2017-12-07  9:15 UTC (permalink / raw)
  To: Tiwei Bie, yliu, dev; +Cc: stable



On 12/07/2017 06:30 AM, Tiwei Bie wrote:
> Fixes: 86d59b21468a ("net/virtio: support LRO")
> Fixes: ec9f3d122a58 ("net/virtio: revert not claiming LRO support")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 64a0cc608..9caa133c9 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -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 <maxime.coquelin@redhat.com>

Thanks,
Maxime

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 3/5] net/virtio: remove a redundant macro definition for ctrl vq
  2017-12-07  5:30 ` [PATCH 3/5] net/virtio: remove a redundant macro definition for ctrl vq Tiwei Bie
@ 2017-12-07  9:16   ` Maxime Coquelin
  0 siblings, 0 replies; 24+ messages in thread
From: Maxime Coquelin @ 2017-12-07  9:16 UTC (permalink / raw)
  To: Tiwei Bie, yliu, dev



On 12/07/2017 06:30 AM, Tiwei Bie wrote:
> 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 <tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtqueue.h | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index ab466c2db..eaf9de13e 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -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 <maxime.coquelin@redhat.com>

Thanks,
Maxime

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/5] net/virtio: remove redundant macro definitions for vector Rx
  2017-12-07  5:30 ` [PATCH 4/5] net/virtio: remove redundant macro definitions for vector Rx Tiwei Bie
@ 2017-12-07  9:18   ` Maxime Coquelin
  2017-12-07 10:39     ` Tiwei Bie
  0 siblings, 1 reply; 24+ messages in thread
From: Maxime Coquelin @ 2017-12-07  9:18 UTC (permalink / raw)
  To: Tiwei Bie, yliu, dev



On 12/07/2017 06:30 AM, Tiwei Bie wrote:
> 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/

> work. So remove the redundant definitions from the *.c files.
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   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 <maxime.coquelin@redhat.com>

Thanks,
Maxime

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 5/5] net/virtio: squeeze repeated blank lines
  2017-12-07  5:30 ` [PATCH 5/5] net/virtio: squeeze repeated blank lines Tiwei Bie
@ 2017-12-07  9:22   ` Maxime Coquelin
  2017-12-07 10:32     ` Tiwei Bie
  0 siblings, 1 reply; 24+ messages in thread
From: Maxime Coquelin @ 2017-12-07  9:22 UTC (permalink / raw)
  To: Tiwei Bie, yliu, dev



On 12/07/2017 06:30 AM, Tiwei Bie wrote:
> 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 <tiwei.bie@intel.com>
> ---
>   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

> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 9caa133c9..aa6d494ca 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1801,7 +1801,6 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>   	return 0;
>   }
>   
> -
>   static int
>   virtio_dev_start(struct rte_eth_dev *dev)
>   {
> diff --git a/drivers/net/virtio/virtio_logs.h b/drivers/net/virtio/virtio_logs.h
> index 90a79eaa5..6ce3149d5 100644
> --- a/drivers/net/virtio/virtio_logs.h
> +++ b/drivers/net/virtio/virtio_logs.h
> @@ -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)
> diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
> index 9574498fb..684ef560c 100644
> --- a/drivers/net/virtio/virtio_pci.c
> +++ b/drivers/net/virtio/virtio_pci.c
> @@ -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)
> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
> index 3c5ce66ce..e42962a99 100644
> --- a/drivers/net/virtio/virtio_pci.h
> +++ b/drivers/net/virtio/virtio_pci.h
> @@ -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
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 390c137c8..7ccd1da34 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -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)
>   {
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing
  2017-12-07  9:14   ` Maxime Coquelin
@ 2017-12-07  9:30     ` Fischetti, Antonio
  2017-12-07 18:10       ` Fischetti, Antonio
  0 siblings, 1 reply; 24+ messages in thread
From: Fischetti, Antonio @ 2017-12-07  9:30 UTC (permalink / raw)
  To: Maxime Coquelin, Bie, Tiwei, yliu, dev; +Cc: stable

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

Antonio

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Thursday, December 7, 2017 9:15 AM
> To: Bie, Tiwei <tiwei.bie@intel.com>; yliu@fridaylinux.org; dev@dpdk.org
> Cc: Fischetti, Antonio <antonio.fischetti@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq
> flushing
> 
> 
> 
> On 12/07/2017 06:30 AM, Tiwei Bie wrote:
> > 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: stable@dpdk.org
> >
> > Reported-by: Antonio Fischetti <antonio.fischetti@intel.com>
> > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > ---
> >   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 <maxime.coquelin@redhat.com>
> 
> Thanks,
> Maxime

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 5/5] net/virtio: squeeze repeated blank lines
  2017-12-07  9:22   ` Maxime Coquelin
@ 2017-12-07 10:32     ` Tiwei Bie
  0 siblings, 0 replies; 24+ messages in thread
From: Tiwei Bie @ 2017-12-07 10:32 UTC (permalink / raw)
  To: Maxime Coquelin; +Cc: yliu, dev

On Thu, Dec 07, 2017 at 10:22:43AM +0100, Maxime Coquelin wrote:
> On 12/07/2017 06:30 AM, Tiwei Bie wrote:
> > 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 <tiwei.bie@intel.com>
> > ---
> >   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.
> 

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

Best regards,
Tiwei Bie

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/5] net/virtio: remove redundant macro definitions for vector Rx
  2017-12-07  9:18   ` Maxime Coquelin
@ 2017-12-07 10:39     ` Tiwei Bie
  0 siblings, 0 replies; 24+ messages in thread
From: Tiwei Bie @ 2017-12-07 10:39 UTC (permalink / raw)
  To: Maxime Coquelin; +Cc: yliu, dev

On Thu, Dec 07, 2017 at 10:18:14AM +0100, Maxime Coquelin wrote:
> On 12/07/2017 06:30 AM, Tiwei Bie wrote:
> > 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

> > work. So remove the redundant definitions from the *.c files.
> > 
> > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > ---
> >   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 <maxime.coquelin@redhat.com>
> 
> Thanks,
> Maxime

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing
  2017-12-07  5:30 ` [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing Tiwei Bie
  2017-12-07  9:14   ` Maxime Coquelin
@ 2017-12-07 16:00   ` Kevin Traynor
  2017-12-08  2:30     ` Tiwei Bie
  1 sibling, 1 reply; 24+ messages in thread
From: Kevin Traynor @ 2017-12-07 16:00 UTC (permalink / raw)
  To: Tiwei Bie, yliu, maxime.coquelin, dev
  Cc: antonio.fischetti, stable, Kavanagh, Mark B

On 12/07/2017 05:30 AM, Tiwei Bie wrote:
> 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.

> Fixes: d8227497ec5c ("net/virtio: flush Rx queues on start")
> Cc: stable@dpdk.org
> 
> Reported-by: Antonio Fischetti <antonio.fischetti@intel.com>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>  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(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index e0328f61d..64a0cc608 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -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);
>  	}
>  
> diff --git a/drivers/net/virtio/virtqueue.c b/drivers/net/virtio/virtqueue.c
> index c3a536f8a..696d0e4a4 100644
> --- a/drivers/net/virtio/virtqueue.c
> +++ b/drivers/net/virtio/virtqueue.c
> @@ -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);
> +		}
>  	}
>  }
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index 2305d91a4..ab466c2db 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -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)
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing
  2017-12-07  9:30     ` Fischetti, Antonio
@ 2017-12-07 18:10       ` Fischetti, Antonio
  0 siblings, 0 replies; 24+ messages in thread
From: Fischetti, Antonio @ 2017-12-07 18:10 UTC (permalink / raw)
  To: Fischetti, Antonio, Maxime Coquelin, Bie, Tiwei, yliu, dev; +Cc: stable

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 <antonio.fischetti@intel.com>



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Fischetti, Antonio
> Sent: Thursday, December 7, 2017 9:30 AM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Bie, Tiwei
> <tiwei.bie@intel.com>; yliu@fridaylinux.org; dev@dpdk.org
> Cc: stable@dpdk.org
> 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
> 
> > -----Original Message-----
> > From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> > Sent: Thursday, December 7, 2017 9:15 AM
> > To: Bie, Tiwei <tiwei.bie@intel.com>; yliu@fridaylinux.org;
> dev@dpdk.org
> > Cc: Fischetti, Antonio <antonio.fischetti@intel.com>; stable@dpdk.org
> > Subject: Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq
> > flushing
> >
> >
> >
> > On 12/07/2017 06:30 AM, Tiwei Bie wrote:
> > > 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: stable@dpdk.org
> > >
> > > Reported-by: Antonio Fischetti <antonio.fischetti@intel.com>
> > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > > ---
> > >   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 <maxime.coquelin@redhat.com>
> >
> > Thanks,
> > Maxime

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing
  2017-12-07 16:00   ` Kevin Traynor
@ 2017-12-08  2:30     ` Tiwei Bie
  2017-12-08 10:17       ` Kevin Traynor
  0 siblings, 1 reply; 24+ messages in thread
From: Tiwei Bie @ 2017-12-08  2:30 UTC (permalink / raw)
  To: Kevin Traynor
  Cc: yliu, maxime.coquelin, dev, antonio.fischetti, stable, Kavanagh, Mark B

On Thu, Dec 07, 2017 at 04:00:57PM +0000, Kevin Traynor wrote:
> On 12/07/2017 05:30 AM, Tiwei Bie wrote:
> > 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?

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing
  2017-12-08  2:30     ` Tiwei Bie
@ 2017-12-08 10:17       ` Kevin Traynor
  0 siblings, 0 replies; 24+ messages in thread
From: Kevin Traynor @ 2017-12-08 10:17 UTC (permalink / raw)
  To: Tiwei Bie
  Cc: yliu, maxime.coquelin, dev, antonio.fischetti, stable, Kavanagh, Mark B

On 12/08/2017 02:30 AM, Tiwei Bie wrote:
> On Thu, Dec 07, 2017 at 04:00:57PM +0000, Kevin Traynor wrote:
>> On 12/07/2017 05:30 AM, Tiwei Bie wrote:
>>> 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?
> 
> 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.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 0/4] various fixes and cleanups for virtio PMD
  2017-12-07  5:30 [PATCH 0/5] various fixes and cleanups for virtio PMD Tiwei Bie
                   ` (4 preceding siblings ...)
  2017-12-07  5:30 ` [PATCH 5/5] net/virtio: squeeze repeated blank lines Tiwei Bie
@ 2017-12-11  5:13 ` Tiwei Bie
  2017-12-11  5:13   ` [PATCH v2 1/4] net/virtio: fix vector Rx break caused by rxq flushing Tiwei Bie
                     ` (4 more replies)
  5 siblings, 5 replies; 24+ messages in thread
From: Tiwei Bie @ 2017-12-11  5:13 UTC (permalink / raw)
  To: yliu, maxime.coquelin, dev

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(-)

-- 
2.13.3

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 1/4] net/virtio: fix vector Rx break caused by rxq flushing
  2017-12-11  5:13 ` [PATCH v2 0/4] various fixes and cleanups for virtio PMD Tiwei Bie
@ 2017-12-11  5:13   ` Tiwei Bie
  2017-12-11  5:13   ` [PATCH v2 2/4] net/virtio: fix typo in LRO support Tiwei Bie
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 24+ messages in thread
From: Tiwei Bie @ 2017-12-11  5:13 UTC (permalink / raw)
  To: yliu, maxime.coquelin, dev; +Cc: stable

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: stable@dpdk.org

Reported-by: Antonio Fischetti <antonio.fischetti@intel.com>
Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Tested-by: Antonio Fischetti <antonio.fischetti@intel.com>
---
 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(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index e0328f61d..64a0cc608 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -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);
 	}
 
diff --git a/drivers/net/virtio/virtqueue.c b/drivers/net/virtio/virtqueue.c
index c3a536f8a..696d0e4a4 100644
--- a/drivers/net/virtio/virtqueue.c
+++ b/drivers/net/virtio/virtqueue.c
@@ -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);
+		}
 	}
 }
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 2305d91a4..ab466c2db 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -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)
-- 
2.13.3

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 2/4] net/virtio: fix typo in LRO support
  2017-12-11  5:13 ` [PATCH v2 0/4] various fixes and cleanups for virtio PMD Tiwei Bie
  2017-12-11  5:13   ` [PATCH v2 1/4] net/virtio: fix vector Rx break caused by rxq flushing Tiwei Bie
@ 2017-12-11  5:13   ` Tiwei Bie
  2017-12-11  5:13   ` [PATCH v2 3/4] net/virtio: remove a redundant macro definition for ctrl vq Tiwei Bie
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 24+ messages in thread
From: Tiwei Bie @ 2017-12-11  5:13 UTC (permalink / raw)
  To: yliu, maxime.coquelin, dev; +Cc: stable

Fixes: 86d59b21468a ("net/virtio: support LRO")
Fixes: ec9f3d122a58 ("net/virtio: revert not claiming LRO support")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 64a0cc608..9caa133c9 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -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;
-- 
2.13.3

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 3/4] net/virtio: remove a redundant macro definition for ctrl vq
  2017-12-11  5:13 ` [PATCH v2 0/4] various fixes and cleanups for virtio PMD Tiwei Bie
  2017-12-11  5:13   ` [PATCH v2 1/4] net/virtio: fix vector Rx break caused by rxq flushing Tiwei Bie
  2017-12-11  5:13   ` [PATCH v2 2/4] net/virtio: fix typo in LRO support Tiwei Bie
@ 2017-12-11  5:13   ` Tiwei Bie
  2017-12-11  5:13   ` [PATCH v2 4/4] net/virtio: remove redundant macro definitions for vector Rx Tiwei Bie
  2017-12-27 14:46   ` [PATCH v2 0/4] various fixes and cleanups for virtio PMD Yuanhan Liu
  4 siblings, 0 replies; 24+ messages in thread
From: Tiwei Bie @ 2017-12-11  5:13 UTC (permalink / raw)
  To: yliu, maxime.coquelin, dev

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 <tiwei.bie@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtqueue.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index ab466c2db..eaf9de13e 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -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.
-- 
2.13.3

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 4/4] net/virtio: remove redundant macro definitions for vector Rx
  2017-12-11  5:13 ` [PATCH v2 0/4] various fixes and cleanups for virtio PMD Tiwei Bie
                     ` (2 preceding siblings ...)
  2017-12-11  5:13   ` [PATCH v2 3/4] net/virtio: remove a redundant macro definition for ctrl vq Tiwei Bie
@ 2017-12-11  5:13   ` Tiwei Bie
  2017-12-27 14:46   ` [PATCH v2 0/4] various fixes and cleanups for virtio PMD Yuanhan Liu
  4 siblings, 0 replies; 24+ messages in thread
From: Tiwei Bie @ 2017-12-11  5:13 UTC (permalink / raw)
  To: yliu, maxime.coquelin, dev

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 <tiwei.bie@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_rxtx_simple_neon.c | 2 --
 drivers/net/virtio/virtio_rxtx_simple_sse.c  | 2 --
 2 files changed, 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx_simple_neon.c b/drivers/net/virtio/virtio_rxtx_simple_neon.c
index b8b93551f..db15c9f5d 100644
--- a/drivers/net/virtio/virtio_rxtx_simple_neon.c
+++ b/drivers/net/virtio/virtio_rxtx_simple_neon.c
@@ -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)
  *
diff --git a/drivers/net/virtio/virtio_rxtx_simple_sse.c b/drivers/net/virtio/virtio_rxtx_simple_sse.c
index 94f65143b..512a69c71 100644
--- a/drivers/net/virtio/virtio_rxtx_simple_sse.c
+++ b/drivers/net/virtio/virtio_rxtx_simple_sse.c
@@ -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)
  *
-- 
2.13.3

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 0/4] various fixes and cleanups for virtio PMD
  2017-12-11  5:13 ` [PATCH v2 0/4] various fixes and cleanups for virtio PMD Tiwei Bie
                     ` (3 preceding siblings ...)
  2017-12-11  5:13   ` [PATCH v2 4/4] net/virtio: remove redundant macro definitions for vector Rx Tiwei Bie
@ 2017-12-27 14:46   ` Yuanhan Liu
  4 siblings, 0 replies; 24+ messages in thread
From: Yuanhan Liu @ 2017-12-27 14:46 UTC (permalink / raw)
  To: Tiwei Bie; +Cc: maxime.coquelin, dev

On Mon, Dec 11, 2017 at 01:13:28PM +0800, Tiwei Bie wrote:
> 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
> 
> 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(-)
> 
> -- 
> 2.13.3

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2017-12-27 14:46 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-07  5:30 [PATCH 0/5] various fixes and cleanups for virtio PMD Tiwei Bie
2017-12-07  5:30 ` [PATCH 1/5] net/virtio: fix vector Rx break caused by rxq flushing Tiwei Bie
2017-12-07  9:14   ` Maxime Coquelin
2017-12-07  9:30     ` Fischetti, Antonio
2017-12-07 18:10       ` Fischetti, Antonio
2017-12-07 16:00   ` Kevin Traynor
2017-12-08  2:30     ` Tiwei Bie
2017-12-08 10:17       ` Kevin Traynor
2017-12-07  5:30 ` [PATCH 2/5] net/virtio: fix typo in LRO support Tiwei Bie
2017-12-07  9:15   ` Maxime Coquelin
2017-12-07  5:30 ` [PATCH 3/5] net/virtio: remove a redundant macro definition for ctrl vq Tiwei Bie
2017-12-07  9:16   ` Maxime Coquelin
2017-12-07  5:30 ` [PATCH 4/5] net/virtio: remove redundant macro definitions for vector Rx Tiwei Bie
2017-12-07  9:18   ` Maxime Coquelin
2017-12-07 10:39     ` Tiwei Bie
2017-12-07  5:30 ` [PATCH 5/5] net/virtio: squeeze repeated blank lines Tiwei Bie
2017-12-07  9:22   ` Maxime Coquelin
2017-12-07 10:32     ` Tiwei Bie
2017-12-11  5:13 ` [PATCH v2 0/4] various fixes and cleanups for virtio PMD Tiwei Bie
2017-12-11  5:13   ` [PATCH v2 1/4] net/virtio: fix vector Rx break caused by rxq flushing Tiwei Bie
2017-12-11  5:13   ` [PATCH v2 2/4] net/virtio: fix typo in LRO support Tiwei Bie
2017-12-11  5:13   ` [PATCH v2 3/4] net/virtio: remove a redundant macro definition for ctrl vq Tiwei Bie
2017-12-11  5:13   ` [PATCH v2 4/4] net/virtio: remove redundant macro definitions for vector Rx Tiwei Bie
2017-12-27 14:46   ` [PATCH v2 0/4] various fixes and cleanups for virtio PMD Yuanhan Liu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.