[dpdk-dev] [PATCH 0/9] virtio fixes

[dpdk-dev] [PATCH 0/9] virtio fixes

[Olivier Matz]

This patchset addresses several issues related to offload and
Rx/Tx handler selection in virtio PMD.

Olivier Matz (9):
  net/virtio: revert "do not claim to support LRO"
  net/virtio: revert "do not falsely claim to do IP checksum"
  doc: fix description of L4 Rx checksum offload
  net/virtio: fix log levels in configure
  net/virtio: fix mbuf port for simple Rx function
  net/virtio: fix queue setup consistency
  net/virtio: rationalize setting of Rx/Tx handlers
  net/virtio: keep Rx handler whatever the Tx queue config
  net/virtio: fix Rx handler when checksum is requested

 doc/guides/nics/features.rst            |   1 +
 drivers/net/virtio/virtio_ethdev.c      | 116 ++++++++++++++++++++++++++------
 drivers/net/virtio/virtio_ethdev.h      |   6 ++
 drivers/net/virtio/virtio_pci.h         |   3 +-
 drivers/net/virtio/virtio_rxtx.c        |  71 +++++++++++--------
 drivers/net/virtio/virtio_rxtx_simple.c |   2 +
 drivers/net/virtio/virtio_user_ethdev.c |   3 +-
 7 files changed, 151 insertions(+), 51 deletions(-)

-- 
2.11.0

[dpdk-dev] [PATCH 1/9] net/virtio: revert "do not claim to support LRO"

[Olivier Matz]

This reverts
commit 701a64622c26 ("net/virtio: do not claim to support LRO")

Setting rxmode->enable_lro is a way to tell the host that the guest is
ok to receive tso packets. From the guest point of view, it is like
enabling LRO on a physical driver.

Fixes: 701a64622c26 ("net/virtio: do not claim to support LRO")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index e320811ed..eb2d95acd 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1659,9 +1659,11 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 {
 	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
 	struct virtio_hw *hw = dev->data->dev_private;
+	uint64_t req_features;
 	int ret;
 
 	PMD_INIT_LOG(DEBUG, "configure");
+	req_features = VIRTIO_PMD_DEFAULT_GUEST_FEATURES;
 
 	if (dev->data->dev_conf.intr_conf.rxq) {
 		ret = virtio_init_device(dev, hw->req_guest_features);
@@ -1675,10 +1677,23 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			    "virtio does not support IP checksum");
 		return -ENOTSUP;
 	}
+	if (rxmode->enable_lro)
+		req_features |=
+			(1ULL << VIRTIO_NET_F_GUEST_TSO4) |
+			(1ULL << VIRTIO_NET_F_GUEST_TSO6);
 
-	if (rxmode->enable_lro) {
+	/* if request features changed, reinit the device */
+	if (req_features != hw->req_guest_features) {
+		ret = virtio_init_device(dev, req_features);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (rxmode->enable_lro &&
+		(!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4) ||
+			!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4))) {
 		PMD_DRV_LOG(NOTICE,
-			    "virtio does not support Large Receive Offload");
+			"Large Receive Offload not available on this host");
 		return -ENOTSUP;
 	}
 
@@ -1904,6 +1919,8 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	}
 	tso_mask = (1ULL << VIRTIO_NET_F_GUEST_TSO4) |
 		(1ULL << VIRTIO_NET_F_GUEST_TSO6);
+	if ((host_features & tso_mask) == tso_mask)
+		dev_info->rx_offload_capa |= DEV_RX_OFFLOAD_TCP_LRO;
 
 	dev_info->tx_offload_capa = 0;
 	if (hw->guest_features & (1ULL << VIRTIO_NET_F_CSUM)) {
-- 
2.11.0

[dpdk-dev] [PATCH 2/9] net/virtio: revert "do not falsely claim to do IP checksum"

[Olivier Matz]

This reverts
commit 4dab342b7522 ("net/virtio: do not falsely claim to do IP checksum").

The description of rxmode->hw_ip_checksum is:

     hw_ip_checksum   : 1, /**< IP/UDP/TCP checksum offload enable. */

Despite its name, this field can be set by an application to enable L3
and L4 checksums. In case of virtio, only L4 checksum is supported and
L3 checksums flags will always be set to "unknown".

Fixes: 4dab342b7522 ("net/virtio: do not falsely claim to do IP checksum")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index eb2d95acd..7a84817e5 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1671,12 +1671,13 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			return ret;
 	}
 
-	/* Virtio does L4 checksum but not L3! */
-	if (rxmode->hw_ip_checksum) {
-		PMD_DRV_LOG(NOTICE,
-			    "virtio does not support IP checksum");
-		return -ENOTSUP;
-	}
+	/* The name hw_ip_checksum is a bit confusing since it can be
+	 * set by the application to request L3 and/or L4 checksums. In
+	 * case of virtio, only L4 checksum is supported.
+	 */
+	if (rxmode->hw_ip_checksum)
+		req_features |= (1ULL << VIRTIO_NET_F_GUEST_CSUM);
+
 	if (rxmode->enable_lro)
 		req_features |=
 			(1ULL << VIRTIO_NET_F_GUEST_TSO4) |
@@ -1689,6 +1690,13 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			return ret;
 	}
 
+	if (rxmode->hw_ip_checksum &&
+		!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_CSUM)) {
+		PMD_DRV_LOG(NOTICE,
+			"rx checksum not available on this host");
+		return -ENOTSUP;
+	}
+
 	if (rxmode->enable_lro &&
 		(!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4) ||
 			!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4))) {
-- 
2.11.0

[dpdk-dev] [PATCH 3/9] doc: fix description of L4 Rx checksum offload

[Olivier Matz]

As described in API documentation, the field hw_ip_checksum
requests both L3 and L4 offload.

Fixes: dad1ec72a377 ("doc: document NIC features")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 doc/guides/nics/features.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
index 37ffbc68c..4430f09d1 100644
--- a/doc/guides/nics/features.rst
+++ b/doc/guides/nics/features.rst
@@ -557,6 +557,7 @@ L4 checksum offload
 
 Supports L4 checksum offload.
 
+* **[uses]     user config**: ``dev_conf.rxmode.hw_ip_checksum``.
 * **[uses]     mbuf**: ``mbuf.ol_flags:PKT_TX_IPV4`` | ``PKT_TX_IPV6``,
   ``mbuf.ol_flags:PKT_TX_L4_NO_CKSUM`` | ``PKT_TX_TCP_CKSUM`` |
   ``PKT_TX_SCTP_CKSUM`` | ``PKT_TX_UDP_CKSUM``.
-- 
2.11.0

[dpdk-dev] [PATCH 4/9] net/virtio: fix log levels in configure

[Olivier Matz]

On error, we should log with error level.

Fixes: 9f4f2846ef76 ("virtio: support vlan filtering")
Fixes: 86d59b21468a ("net/virtio: support LRO")
Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 7a84817e5..8eee3ff80 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1692,7 +1692,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 
 	if (rxmode->hw_ip_checksum &&
 		!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_CSUM)) {
-		PMD_DRV_LOG(NOTICE,
+		PMD_DRV_LOG(ERR,
 			"rx checksum not available on this host");
 		return -ENOTSUP;
 	}
@@ -1700,7 +1700,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))) {
-		PMD_DRV_LOG(NOTICE,
+		PMD_DRV_LOG(ERR,
 			"Large Receive Offload not available on this host");
 		return -ENOTSUP;
 	}
@@ -1713,7 +1713,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 
 	if (rxmode->hw_vlan_filter
 	    && !vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_VLAN)) {
-		PMD_DRV_LOG(NOTICE,
+		PMD_DRV_LOG(ERR,
 			    "vlan filtering not available on this host");
 		return -ENOTSUP;
 	}
-- 
2.11.0

[dpdk-dev] [PATCH 5/9] net/virtio: fix mbuf port for simple Rx function

[Olivier Matz]

The mbuf->port was was not properly set for the first received
mbufs. Fix this by setting it in virtqueue_enqueue_recv_refill_simple(),
which is used to enqueue the first mbuf in the ring.

The function virtio_rxq_rearm_vec(), which is used to rearm the ring
with new mbufs, is correct and does not need to be updated.

Fixes: cab0461234e7 ("virtio: fill Rx avail ring with blank mbufs")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_rxtx_simple.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/virtio/virtio_rxtx_simple.c b/drivers/net/virtio/virtio_rxtx_simple.c
index 542cf805d..54ababae9 100644
--- a/drivers/net/virtio/virtio_rxtx_simple.c
+++ b/drivers/net/virtio/virtio_rxtx_simple.c
@@ -65,6 +65,8 @@ virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
 	struct vring_desc *start_dp;
 	uint16_t desc_idx;
 
+	cookie->port = vq->rxq.port_id;
+
 	desc_idx = vq->vq_avail_idx & (vq->vq_nentries - 1);
 	dxp = &vq->vq_descx[desc_idx];
 	dxp->cookie = (void *)cookie;
-- 
2.11.0

[dpdk-dev] [PATCH 6/9] net/virtio: fix queue setup consistency

[Olivier Matz]

In rx/tx queue setup functions, some code is executed only if
use_simple_rxtx == 1. The value of this variable can change depending on
the offload flags or sse support. If Rx queue setup is called before Tx
queue setup, it can result in an invalid configuration:

- dev_configure is called: use_simple_rxtx is initialized to 0
- rx queue setup is called: queues are initialized without simple path
  support
- tx queue setup is called: use_simple_rxtx switch to 1, and simple
  Rx/Tx handlers are selected

Fix this by postponing a part of Rx/Tx queue initialization in
dev_start(), as it was the case in the initial implementation.

Fixes: 48cec290a3d2 ("net/virtio: move queue configure code to proper place")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 13 +++++++++++++
 drivers/net/virtio/virtio_ethdev.h |  6 ++++++
 drivers/net/virtio/virtio_rxtx.c   | 40 ++++++++++++++++++++++++++++++--------
 3 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 8eee3ff80..c7888f103 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1737,6 +1737,19 @@ virtio_dev_start(struct rte_eth_dev *dev)
 	struct virtnet_rx *rxvq;
 	struct virtnet_tx *txvq __rte_unused;
 	struct virtio_hw *hw = dev->data->dev_private;
+	int ret;
+
+	/* Finish the initialization of the queues */
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		ret = virtio_dev_rx_queue_setup_finish(dev, i);
+		if (ret < 0)
+			return ret;
+	}
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		ret = virtio_dev_tx_queue_setup_finish(dev, i);
+		if (ret < 0)
+			return ret;
+	}
 
 	/* check if lsc interrupt feature is enabled */
 	if (dev->data->dev_conf.intr_conf.lsc) {
diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index c3413c6d9..2039bc547 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -92,10 +92,16 @@ int  virtio_dev_rx_queue_setup(struct rte_eth_dev *dev, uint16_t rx_queue_id,
 		const struct rte_eth_rxconf *rx_conf,
 		struct rte_mempool *mb_pool);
 
+int virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev,
+				uint16_t rx_queue_id);
+
 int  virtio_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,
 		uint16_t nb_tx_desc, unsigned int socket_id,
 		const struct rte_eth_txconf *tx_conf);
 
+int virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
+				uint16_t tx_queue_id);
+
 uint16_t virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 		uint16_t nb_pkts);
 
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index e30377c51..a32e3229f 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -421,9 +421,6 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	struct virtio_hw *hw = dev->data->dev_private;
 	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
 	struct virtnet_rx *rxvq;
-	int error, nbufs;
-	struct rte_mbuf *m;
-	uint16_t desc_idx;
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -440,10 +437,24 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	}
 	dev->data->rx_queues[queue_idx] = rxvq;
 
+	return 0;
+}
+
+int
+virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t queue_idx)
+{
+	uint16_t vtpci_queue_idx = 2 * queue_idx + VTNET_SQ_RQ_QUEUE_IDX;
+	struct virtio_hw *hw = dev->data->dev_private;
+	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
+	struct virtnet_rx *rxvq = &vq->rxq;
+	struct rte_mbuf *m;
+	uint16_t desc_idx;
+	int error, nbufs;
+
+	PMD_INIT_FUNC_TRACE();
 
 	/* Allocate blank mbufs for the each rx descriptor */
 	nbufs = 0;
-	error = ENOSPC;
 
 	if (hw->use_simple_rxtx) {
 		for (desc_idx = 0; desc_idx < vq->vq_nentries;
@@ -534,7 +545,6 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
 	struct virtnet_tx *txvq;
 	uint16_t tx_free_thresh;
-	uint16_t desc_idx;
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -563,9 +573,24 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 
 	vq->vq_free_thresh = tx_free_thresh;
 
-	if (hw->use_simple_rxtx) {
-		uint16_t mid_idx  = vq->vq_nentries >> 1;
+	dev->data->tx_queues[queue_idx] = txvq;
+	return 0;
+}
+
+int
+virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
+				uint16_t queue_idx)
+{
+	uint8_t vtpci_queue_idx = 2 * queue_idx + VTNET_SQ_TQ_QUEUE_IDX;
+	struct virtio_hw *hw = dev->data->dev_private;
+	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
+	uint16_t mid_idx = vq->vq_nentries >> 1;
+	struct virtnet_tx *txvq = &vq->txq;
+	uint16_t desc_idx;
 
+	PMD_INIT_FUNC_TRACE();
+
+	if (hw->use_simple_rxtx) {
 		for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
 			vq->vq_ring.avail->ring[desc_idx] =
 				desc_idx + mid_idx;
@@ -587,7 +612,6 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 
 	VIRTQUEUE_DUMP(vq);
 
-	dev->data->tx_queues[queue_idx] = txvq;
 	return 0;
 }
 
-- 
2.11.0

[dpdk-dev] [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers

[Olivier Matz]

The selection of Rx/Tx handlers is done at several places,
group them in one function set_rxtx_funcs().

The update of hw->use_simple_rxtx is also rationalized:
- initialized to 1 (prefer simple path)
- in dev configure or rx/tx queue setup, if something prevents from
  using the simple path, change it to 0.
- in dev start, set the handlers according to hw->use_simple_rxtx.

Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 52 +++++++++++++++++++++++++++++---------
 drivers/net/virtio/virtio_rxtx.c   | 23 +++++------------
 2 files changed, 46 insertions(+), 29 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index c7888f103..8dad3095f 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1235,14 +1235,36 @@ virtio_interrupt_handler(void *param)
 
 }
 
+/* set rx and tx handlers according to what is supported */
 static void
-rx_func_get(struct rte_eth_dev *eth_dev)
+set_rxtx_funcs(struct rte_eth_dev *eth_dev)
 {
 	struct virtio_hw *hw = eth_dev->data->dev_private;
-	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
+
+	if (hw->use_simple_rxtx) {
+		PMD_INIT_LOG(INFO, "virtio: using simple Rx path on port %u",
+			eth_dev->data->port_id);
+		eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
+	} else if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
+		PMD_INIT_LOG(INFO,
+			"virtio: using mergeable buffer Rx path on port %u",
+			eth_dev->data->port_id);
 		eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts;
-	else
+	} else {
+		PMD_INIT_LOG(INFO, "virtio: using standard Rx path on port %u",
+			eth_dev->data->port_id);
 		eth_dev->rx_pkt_burst = &virtio_recv_pkts;
+	}
+
+	if (hw->use_simple_rxtx) {
+		PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
+			eth_dev->data->port_id);
+		eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
+	} else {
+		PMD_INIT_LOG(INFO, "virtio: using standard Tx path on port %u",
+			eth_dev->data->port_id);
+		eth_dev->tx_pkt_burst = virtio_xmit_pkts;
+	}
 }
 
 /* Only support 1:1 queue/interrupt mapping so far.
@@ -1367,8 +1389,6 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 	else
 		eth_dev->data->dev_flags &= ~RTE_ETH_DEV_INTR_LSC;
 
-	rx_func_get(eth_dev);
-
 	/* Setting up rx_header size for the device */
 	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF) ||
 	    vtpci_with_feature(hw, VIRTIO_F_VERSION_1))
@@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
 
 	eth_dev->dev_ops = &virtio_eth_dev_ops;
-	eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
 
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
 		if (!hw->virtio_user_dev) {
@@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 		}
 
 		virtio_set_vtpci_ops(hw);
-		if (hw->use_simple_rxtx) {
-			eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
-			eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
-		} else {
-			rx_func_get(eth_dev);
-		}
+		set_rxtx_funcs(eth_dev);
+
 		return 0;
 	}
 
@@ -1726,6 +1741,18 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			return -EBUSY;
 		}
 
+	hw->use_simple_rxtx = 1;
+
+#if defined RTE_ARCH_X86
+	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
+		hw->use_simple_rxtx = 0;
+#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
+	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
+		hw->use_simple_rxtx = 0;
+#endif
+	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
+		hw->use_simple_rxtx = 0;
+
 	return 0;
 }
 
@@ -1802,6 +1829,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
 		VIRTQUEUE_DUMP(txvq->vq);
 	}
 
+	set_rxtx_funcs(dev);
 	hw->started = 1;
 
 	/* Initialize Link state */
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index a32e3229f..c9d97b643 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -505,25 +505,14 @@ static void
 virtio_update_rxtx_handler(struct rte_eth_dev *dev,
 			   const struct rte_eth_txconf *tx_conf)
 {
-	uint8_t use_simple_rxtx = 0;
 	struct virtio_hw *hw = dev->data->dev_private;
+	uint8_t use_simple_rxtx = hw->use_simple_rxtx;
 
-#if defined RTE_ARCH_X86
-	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
-		use_simple_rxtx = 1;
-#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
-	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
-		use_simple_rxtx = 1;
-#endif
-	/* Use simple rx/tx func if single segment and no offloads */
-	if (use_simple_rxtx &&
-	    (tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == VIRTIO_SIMPLE_FLAGS &&
-	    !vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
-		PMD_INIT_LOG(INFO, "Using simple rx/tx path");
-		dev->tx_pkt_burst = virtio_xmit_pkts_simple;
-		dev->rx_pkt_burst = virtio_recv_pkts_vec;
-		hw->use_simple_rxtx = use_simple_rxtx;
-	}
+	/* cannot use simple rxtx funcs with multisegs or offloads */
+	if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) != VIRTIO_SIMPLE_FLAGS)
+		use_simple_rxtx = 0;
+
+	hw->use_simple_rxtx = use_simple_rxtx;
 }
 
 /*
-- 
2.11.0

[dpdk-dev] [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config

[Olivier Matz]

Split use_simple_rxtx into use_simple_rx and use_simple_tx,
and ensure that only use_simple_tx is updated when txq flags
forces to use the standard Tx handler.

This change is also useful for next commit (disable simple Rx
path when Rx checksum is requested).

Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c      | 25 ++++++++++++++++---------
 drivers/net/virtio/virtio_pci.h         |  3 ++-
 drivers/net/virtio/virtio_rxtx.c        | 18 +++++++++---------
 drivers/net/virtio/virtio_user_ethdev.c |  3 ++-
 4 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 8dad3095f..4845d44b0 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1241,7 +1241,7 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
 {
 	struct virtio_hw *hw = eth_dev->data->dev_private;
 
-	if (hw->use_simple_rxtx) {
+	if (hw->use_simple_rx) {
 		PMD_INIT_LOG(INFO, "virtio: using simple Rx path on port %u",
 			eth_dev->data->port_id);
 		eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
@@ -1256,7 +1256,7 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
 		eth_dev->rx_pkt_burst = &virtio_recv_pkts;
 	}
 
-	if (hw->use_simple_rxtx) {
+	if (hw->use_simple_tx) {
 		PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
 			eth_dev->data->port_id);
 		eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
@@ -1741,17 +1741,24 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			return -EBUSY;
 		}
 
-	hw->use_simple_rxtx = 1;
+	hw->use_simple_rx = 1;
+	hw->use_simple_tx = 1;
 
 #if defined RTE_ARCH_X86
-	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
-		hw->use_simple_rxtx = 0;
+	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3)) {
+		hw->use_simple_rx = 0;
+		hw->use_simple_tx = 0;
+	}
 #elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
-	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
-		hw->use_simple_rxtx = 0;
+	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON)) {
+		hw->use_simple_rx = 0;
+		hw->use_simple_tx = 0;
+	}
 #endif
-	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
-		hw->use_simple_rxtx = 0;
+	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
+		hw->use_simple_rx = 0;
+		hw->use_simple_tx = 0;
+	}
 
 	return 0;
 }
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 18caebdd7..2ff526c88 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -259,7 +259,8 @@ struct virtio_hw {
 	uint8_t	    vlan_strip;
 	uint8_t	    use_msix;
 	uint8_t     modern;
-	uint8_t     use_simple_rxtx;
+	uint8_t     use_simple_rx;
+	uint8_t     use_simple_tx;
 	uint8_t     port_id;
 	uint8_t     mac_addr[ETHER_ADDR_LEN];
 	uint32_t    notify_off_multiplier;
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index c9d97b643..b81ba0d4a 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -456,7 +456,7 @@ virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t queue_idx)
 	/* Allocate blank mbufs for the each rx descriptor */
 	nbufs = 0;
 
-	if (hw->use_simple_rxtx) {
+	if (hw->use_simple_rx) {
 		for (desc_idx = 0; desc_idx < vq->vq_nentries;
 		     desc_idx++) {
 			vq->vq_ring.avail->ring[desc_idx] = desc_idx;
@@ -478,7 +478,7 @@ virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t queue_idx)
 			break;
 
 		/* Enqueue allocated buffers */
-		if (hw->use_simple_rxtx)
+		if (hw->use_simple_rx)
 			error = virtqueue_enqueue_recv_refill_simple(vq, m);
 		else
 			error = virtqueue_enqueue_recv_refill(vq, m);
@@ -502,17 +502,17 @@ virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t queue_idx)
 }
 
 static void
-virtio_update_rxtx_handler(struct rte_eth_dev *dev,
+virtio_update_tx_handler(struct rte_eth_dev *dev,
 			   const struct rte_eth_txconf *tx_conf)
 {
 	struct virtio_hw *hw = dev->data->dev_private;
-	uint8_t use_simple_rxtx = hw->use_simple_rxtx;
+	uint8_t use_simple_tx = hw->use_simple_tx;
 
-	/* cannot use simple rxtx funcs with multisegs or offloads */
+	/* use simple tx func if single segment and no offloads */
 	if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) != VIRTIO_SIMPLE_FLAGS)
-		use_simple_rxtx = 0;
+		use_simple_tx = 0;
 
-	hw->use_simple_rxtx = use_simple_rxtx;
+	hw->use_simple_tx = use_simple_tx;
 }
 
 /*
@@ -537,7 +537,7 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	virtio_update_rxtx_handler(dev, tx_conf);
+	virtio_update_tx_handler(dev, tx_conf);
 
 	if (nb_desc == 0 || nb_desc > vq->vq_nentries)
 		nb_desc = vq->vq_nentries;
@@ -579,7 +579,7 @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	if (hw->use_simple_rxtx) {
+	if (hw->use_simple_tx) {
 		for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
 			vq->vq_ring.avail->ring[desc_idx] =
 				desc_idx + mid_idx;
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index c96144434..57c964d6d 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -369,7 +369,8 @@ virtio_user_eth_dev_alloc(struct rte_vdev_device *vdev)
 	 */
 	hw->use_msix = 1;
 	hw->modern   = 0;
-	hw->use_simple_rxtx = 0;
+	hw->use_simple_rx = 0;
+	hw->use_simple_tx = 0;
 	hw->virtio_user_dev = dev;
 	data->dev_flags = RTE_ETH_DEV_DETACHABLE;
 	return eth_dev;
-- 
2.11.0

[dpdk-dev] [PATCH 9/9] net/virtio: fix Rx handler when checksum is requested

[Olivier Matz]

The simple Rx handler is selected even if Rx checksum offload is
requested by the application, but this handler does not support
offloads. This results in broken received packets (no checksum flag but
invalid checksum in the mbuf data).

Disable the simple Rx handler in that case.

Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 4845d44b0..0e616bcf0 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1760,6 +1760,9 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 		hw->use_simple_tx = 0;
 	}
 
+	if (rxmode->hw_ip_checksum)
+		hw->use_simple_rx = 0;
+
 	return 0;
 }
 
-- 
2.11.0

Re: [dpdk-dev] [PATCH 2/9] net/virtio: revert "do not falsely claim to do IP checksum"

[Olivier MATZ]

Validation:


Platform description
--------------------

guest (dpdk)
+----------------+
|                |
|                |
|    port0       |
+----------------+
       |
       | virtio
       |
+----------------+
|     tap0       |
|                |
|                |
+----------------+
host (linux, vhost-net)

Host configuration
------------------

Start qemu with:

- a ne2k management interface to avoi any conflict with dpdk
- a virtio net device, connected to a tap interface through vhost-net

  /usr/bin/qemu-system-x86_64 -k fr -daemonize --enable-kvm -m 2G -cpu host \
    -smp 3 -serial telnet::40564,server,nowait -serial null \
    -qmp tcp::44340,server,nowait -monitor telnet::49229,server,nowait \
    -device ne2k_pci,mac=de:ad:de:01:02:03,netdev=user.0,addr=03 \
    -netdev user,id=user.0,hostfwd=tcp::34965-:22 \
    -netdev type=tap,id=vhostnet0,script=no,vhost=on,queues=8 \
    -device virtio-net-pci,netdev=vhostnet0,mq=on,vectors=17 \
    -hda "${VM_PATH}/ubuntu-16.04-template.qcow2" \
    -snapshot -vga none -display none

Guest configuration
-------------------

Compile dpdk:

  cd dpdk.org
  make config T=x86_64-native-linuxapp-gcc
  sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=y,' build/.config
  sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=y,' build/.config
  make -j4

Prepare environment:

  mkdir -p /mnt/huge
  mount -t hugetlbfs nodev /mnt/huge
  echo 256 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
  modprobe uio_pci_generic
  python usertools/dpdk-devbind.py -b uio_pci_generic 0000:00:02.0

  ./build/app/testpmd -l 0,1 --log-level 8 -- --total-num-mbufs=16384 \
    -i --port-topology=chained --disable-hw-vlan-filter \
    --disable-hw-vlan-strip --enable-rx-cksum --enable-lro \
    --txqflags=0

Before the reverts (patch 1 and 2 of the patchset)
------------------

testpmd cannot start

...
PMD: virtio_dev_configure(): virtio does not support IP checksum

After the reverts
-----------------

 testpmd starts properly, and receives packets with csum flags

testpmd> set fwd rxonly
Set rxonly packet forwarding mode
testpmd> set verbose 1
Change verbose level from 0 to 1
testpmd> start
# tcp packet sent from the host
port 0/queue 0: received 1 packets
  src=16:9A:CA:76:89:BC - dst=52:54:00:12:34:56 - type=0x0800 - length=74 - nb_segs=1 - hw ptype: L2_ETHER L3_IPV4 L4_TCP  - sw ptype: L2_ETHER L3_IPV4 L4_TCP  - l2_len=14 - l3_len=20 - l4_len=40 - Receive queue=0x0
  ol_flags: PKT_RX_L4_CKSUM_NONE PKT_RX_IP_CKSUM_UNKNOWN

Re: [dpdk-dev] [PATCH 5/9] net/virtio: fix mbuf port for simple Rx function

[Olivier MATZ]

Validation:

Platform description
--------------------

guest (dpdk)
+----------------+
|                |
|                |
|    port0       |
+----------------+
       |
       | virtio
       |
+----------------+
|     tap0       |
|                |
|                |
+----------------+
host (linux, vhost-net)

Host configuration
------------------

Start qemu with:

- a ne2k management interface to avoi any conflict with dpdk
- a virtio net device, connected to a tap interface through vhost-net
- mergeable buffers disabled

  /usr/bin/qemu-system-x86_64 -k fr -daemonize --enable-kvm -m 2G -cpu host \
    -smp 3 -serial telnet::40564,server,nowait -serial null \
    -qmp tcp::44340,server,nowait -monitor telnet::49229,server,nowait \
    -device ne2k_pci,mac=de:ad:de:01:02:03,netdev=user.0,addr=03 \
    -netdev user,id=user.0,hostfwd=tcp::34965-:22 \
    -netdev type=tap,id=vhostnet0,script=no,vhost=on,queues=8 \
    -device virtio-net-pci,mrg_rxbuf=off,netdev=vhostnet0,mq=on,vectors=17 \
    -hda "${VM_PATH}/ubuntu-16.04-template.qcow2" \
    -snapshot -vga none -display none

Guest configuration
-------------------

Apply a simple patch to display m->port in test-pmd/rxonly.c.

  --- a/app/test-pmd/rxonly.c
  +++ b/app/test-pmd/rxonly.c
  @@ -139,9 +139,9 @@ pkt_burst_receive(struct fwd_stream *fs)
  
                  print_ether_addr("  src=", &eth_hdr->s_addr);
                  print_ether_addr(" - dst=", &eth_hdr->d_addr);
  -               printf(" - type=0x%04x - length=%u - nb_segs=%d",
  +               printf(" - type=0x%04x - length=%u - nb_segs=%d - port=%d",
                         eth_type, (unsigned) mb->pkt_len,
  -                      (int)mb->nb_segs);
  +                      (int)mb->nb_segs, mb->port);
                  if (ol_flags & PKT_RX_RSS_HASH) {
                          printf(" - RSS hash=0x%x", (unsigned) mb->hash.rss);
                          printf(" - RSS queue=0x%x",(unsigned) fs->rx_queue);

Compile dpdk:

  cd dpdk.org
  make config T=x86_64-native-linuxapp-gcc
  sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=y,' build/.config
  sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=y,' build/.config
  make -j4

Prepare environment:

  mkdir -p /mnt/huge
  mount -t hugetlbfs nodev /mnt/huge
  echo 256 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
  modprobe uio_pci_generic
  python usertools/dpdk-devbind.py -b uio_pci_generic 0000:00:02.0

  ./build/app/testpmd -l 0,1 --log-level 7 -- --total-num-mbufs=16384 \
    -i --port-topology=chained --disable-hw-vlan-filter \
    --disable-hw-vlan-strip --txqflags=0xf01

  ...
  PMD: virtio_update_rxtx_handler(): Using simple rx/tx path
  ...

Configure testpmd:

  set fwd rxonly
  set verbose 1
  start

The first 128 received packets have **a wrong m->port**):

  src=00:00:00:00:00:00 - dst=FF:FF:FF:FF:FF:FF - type=0x0800 - length=42 - nb_segs=1 - port=255 - sw ptype: L2_ETHER L3_IPV4  - l2_len=14 - l3_len=20 - Receive queue=0x0
  ol_flags: PKT_RX_L4_CKSUM_UNKNOWN PKT_RX_IP_CKSUM_UNKNOWN

After 128 packets, it's ok:

  src=00:00:00:00:00:00 - dst=FF:FF:FF:FF:FF:FF - type=0x0800 - length=42 - nb_segs=1 - port=0 - sw ptype: L2_ETHER L3_IPV4  - l2_len=14 - l3_len=20 - Receive queue=0x0
  ol_flags: PKT_RX_L4_CKSUM_UNKNOWN PKT_RX_IP_CKSUM_UNKNOWN

This is not reproduced with --txqflags=0 (standard Rx path), or with
the fix applied.

Re: [dpdk-dev] [PATCH 6/9] net/virtio: fix queue setup consistency

[Olivier MATZ]

Platform description
--------------------

guest (dpdk)
+----------------+
|                |
|                |
|    port0       |
+----------------+
       |
       | virtio
       |
+----------------+
|     tap0       |
|                |
|                |
+----------------+
host (linux, vhost-net)

Host configuration
------------------

Start qemu with:

- a ne2k management interface to avoi any conflict with dpdk
- a virtio net device, connected to a tap interface through vhost-net
- mergeable buffers disabled

  /usr/bin/qemu-system-x86_64 -k fr -daemonize --enable-kvm -m 2G -cpu host \
    -smp 3 -serial telnet::40564,server,nowait -serial null \
    -qmp tcp::44340,server,nowait -monitor telnet::49229,server,nowait \
    -device ne2k_pci,mac=de:ad:de:01:02:03,netdev=user.0,addr=03 \
    -netdev user,id=user.0,hostfwd=tcp::34965-:22 \
    -netdev type=tap,id=vhostnet0,script=no,vhost=on,queues=8 \
    -device virtio-net-pci,mrg_rxbuf=off,netdev=vhostnet0,mq=on,vectors=17 \
    -hda "${VM_PATH}/ubuntu-16.04-template.qcow2" \
    -snapshot -vga none -display none

Guest configuration
-------------------

Apply a patch that reverts initialization of queues in testpmd
(initialize rx queue first), and displays some logs in virtio:

  --- a/app/test-pmd/testpmd.c
  +++ b/app/test-pmd/testpmd.c
  @@ -1461,34 +1461,10 @@ start_port(portid_t pid)
                  }
                  if (port->need_reconfig_queues > 0) {
                          port->need_reconfig_queues = 0;
  -                       /* setup tx queues */
  -                       for (qi = 0; qi < nb_txq; qi++) {
  -                               if ((numa_support) &&
  -                                       (txring_numa[pi] != NUMA_NO_CONFIG))
  -                                       diag = rte_eth_tx_queue_setup(pi, qi,
  -                                               nb_txd,txring_numa[pi],
  -                                               &(port->tx_conf));
  -                               else
  -                                       diag = rte_eth_tx_queue_setup(pi, qi,
  -                                               nb_txd,port->socket_id,
  -                                               &(port->tx_conf));
  -
  -                               if (diag == 0)
  -                                       continue;
  -
  -                               /* Fail to setup tx queue, return */
  -                               if (rte_atomic16_cmpset(&(port->port_status),
  -                                                       RTE_PORT_HANDLING,
  -                                                       RTE_PORT_STOPPED) == 0)
  -                                       printf("Port %d can not be set back "
  -                                                       "to stopped\n", pi);
  -                               printf("Fail to configure port %d tx queues\n", pi);
  -                               /* try to reconfigure queues next time */
  -                               port->need_reconfig_queues = 1;
  -                               return -1;
  -                       }
                          /* setup rx queues */
                          for (qi = 0; qi < nb_rxq; qi++) {
  +                               printf("rte_eth_rx_queue_setup %d %d\n",
  +                                       pi, qi);
                                  if ((numa_support) &&
                                          (rxring_numa[pi] != NUMA_NO_CONFIG)) {
                                          struct rte_mempool * mp =
  @@ -1500,7 +1476,6 @@ start_port(portid_t pid)
                                                          rxring_numa[pi]);
                                                  return -1;
                                          }
  -
                                          diag = rte_eth_rx_queue_setup(pi, qi,
                                               nb_rxd,rxring_numa[pi],
                                               &(port->rx_conf),mp);
  @@ -1532,6 +1507,34 @@ start_port(portid_t pid)
                                  port->need_reconfig_queues = 1;
                                  return -1;
                          }
  +                       /* setup tx queues */
  +                       for (qi = 0; qi < nb_txq; qi++) {
  +                               printf("rte_eth_tx_queue_setup %d %d\n",
  +                                       pi, qi);
  +                               if ((numa_support) &&
  +                                       (txring_numa[pi] != NUMA_NO_CONFIG))
  +                                       diag = rte_eth_tx_queue_setup(pi, qi,
  +                                               nb_txd,txring_numa[pi],
  +                                               &(port->tx_conf));
  +                               else
  +                                       diag = rte_eth_tx_queue_setup(pi, qi,
  +                                               nb_txd,port->socket_id,
  +                                               &(port->tx_conf));
  +
  +                               if (diag == 0)
  +                                       continue;
  +
  +                               /* Fail to setup tx queue, return */
  +                               if (rte_atomic16_cmpset(&(port->port_status),
  +                                                       RTE_PORT_HANDLING,
  +                                                       RTE_PORT_STOPPED) == 0)
  +                                       printf("Port %d can not be set back "
  +                                                       "to stopped\n", pi);
  +                               printf("Fail to configure port %d tx queues\n", pi);
  +                               /* try to reconfigure queues next time */
  +                               port->need_reconfig_queues = 1;
  +                               return -1;
  +                       }
                  }
   
                  for (event_type = RTE_ETH_EVENT_UNKNOWN;
  --- a/drivers/net/virtio/virtio_rxtx.c
  +++ b/drivers/net/virtio/virtio_rxtx.c
  @@ -445,6 +445,8 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
          nbufs = 0;
          error = ENOSPC;
   
  +        printf("rx_queue_setup() use_simple_rxtx=%d\n",
  +               hw->use_simple_rxtx);
          if (hw->use_simple_rxtx) {
                  for (desc_idx = 0; desc_idx < vq->vq_nentries;
                       desc_idx++) {
  @@ -563,6 +565,8 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
   
          vq->vq_free_thresh = tx_free_thresh;
   
  +        printf("tx_queue_setup() use_simple_rxtx=%d\n",
  +               hw->use_simple_rxtx);
          if (hw->use_simple_rxtx) {
                  uint16_t mid_idx  = vq->vq_nentries >> 1;
   

Compile dpdk:

  cd dpdk.org
  make config T=x86_64-native-linuxapp-gcc
  sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=y,' build/.config
  sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=y,' build/.config
  make -j4

Prepare environment:

  mkdir -p /mnt/huge
  mount -t hugetlbfs nodev /mnt/huge
  echo 256 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
  modprobe uio_pci_generic
  python usertools/dpdk-devbind.py -b uio_pci_generic 0000:00:02.0

  ./build/app/testpmd -l 0,1 --log-level 7 -- --total-num-mbufs=16384 \
    -i --port-topology=chained --disable-hw-vlan-filter \
    --disable-hw-vlan-strip --txqflags=0xf01

  ...
  Configuring Port 0 (socket 0)
  rte_eth_rx_queue_setup 0 0
  rx_queue_setup() use_simple_rxtx=0
  rte_eth_tx_queue_setup 0 0
  PMD: virtio_update_rxtx_handler(): Using simple rx/tx path
  tx_queue_setup() use_simple_rxtx=1
  ...

Configure testpmd:

  set fwd rxonly
  set verbose 1
  start

Without the fix, there is a segfault in virtio_recv_pkts_vec()

It works ok with the patch.

Re: [dpdk-dev] [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config

[Olivier MATZ]

Platform description
--------------------

guest (dpdk)
+----------------+
|                |
|                |
|    port0       |
+----------------+
       |
       | virtio
       |
+----------------+
|     tap0       |
|                |
|                |
+----------------+
host (linux, vhost-net)

Host configuration
------------------

Start qemu with:

- a ne2k management interface to avoi any conflict with dpdk
- a virtio net device, connected to a tap interface through vhost-net
- mergeable buffers disabled

  /usr/bin/qemu-system-x86_64 -k fr -daemonize --enable-kvm -m 2G -cpu host \
    -smp 3 -serial telnet::40564,server,nowait -serial null \
    -qmp tcp::44340,server,nowait -monitor telnet::49229,server,nowait \
    -device ne2k_pci,mac=de:ad:de:01:02:03,netdev=user.0,addr=03 \
    -netdev user,id=user.0,hostfwd=tcp::34965-:22 \
    -netdev type=tap,id=vhostnet0,script=no,vhost=on,queues=8 \
    -device virtio-net-pci,mrg_rxbuf=off,netdev=vhostnet0,mq=on,vectors=17 \
    -hda "${VM_PATH}/ubuntu-16.04-template.qcow2" \
    -snapshot -vga none -display none

Guest configuration
-------------------

Compile dpdk:

  cd dpdk.org
  make config T=x86_64-native-linuxapp-gcc
  sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=y,' build/.config
  sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=y,' build/.config
  make -j4

Prepare environment:

  mkdir -p /mnt/huge
  mount -t hugetlbfs nodev /mnt/huge
  echo 256 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
  modprobe uio_pci_generic
  python usertools/dpdk-devbind.py -b uio_pci_generic 0000:00:02.0

  ./build/app/testpmd -l 0,1 --log-level 7 -- --total-num-mbufs=16384 \
    -i --port-topology=chained --disable-hw-vlan-filter \
    --disable-hw-vlan-strip --txqflags=0

Without the fix, standard path is used in rx and tx, but simple rx could
be used:

  ...
  PMD: set_rxtx_funcs(): virtio: using standard Rx path on port 0
  PMD: set_rxtx_funcs(): virtio: using standard Tx path on port 0
  ...

With the fix:

  ...
  PMD: set_rxtx_funcs(): virtio: using simple Rx path on port 0
  PMD: set_rxtx_funcs(): virtio: using standard Tx path on port 0
  ...

Re: [dpdk-dev] [PATCH 9/9] net/virtio: fix Rx handler when checksum is requested

[Olivier MATZ]

Platform description
--------------------

guest (dpdk)
+----------------+
|                |
|                |
|    port0       |
+----------------+
       |
       | virtio
       |
+----------------+
|     tap0       |
|                |
|                |
+----------------+
host (linux, vhost-net)

Host configuration
------------------

Start qemu with:

- a ne2k management interface to avoi any conflict with dpdk
- a virtio net device, connected to a tap interface through vhost-net
- mergeable buffers disabled

  /usr/bin/qemu-system-x86_64 -k fr -daemonize --enable-kvm -m 2G -cpu host \
    -smp 3 -serial telnet::40564,server,nowait -serial null \
    -qmp tcp::44340,server,nowait -monitor telnet::49229,server,nowait \
    -device ne2k_pci,mac=de:ad:de:01:02:03,netdev=user.0,addr=03 \
    -netdev user,id=user.0,hostfwd=tcp::34965-:22 \
    -netdev type=tap,id=vhostnet0,script=no,vhost=on,queues=8 \
    -device virtio-net-pci,mrg_rxbuf=off,netdev=vhostnet0,mq=on,vectors=17 \
    -hda "${VM_PATH}/ubuntu-16.04-template.qcow2" \
    -snapshot -vga none -display none

Guest configuration
-------------------

Compile dpdk:

  cd dpdk.org
  make config T=x86_64-native-linuxapp-gcc
  sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=y,' build/.config
  sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=y,' build/.config
  make -j4

Prepare environment:

  mkdir -p /mnt/huge
  mount -t hugetlbfs nodev /mnt/huge
  echo 256 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
  modprobe uio_pci_generic
  python usertools/dpdk-devbind.py -b uio_pci_generic 0000:00:02.0

  ./build/app/testpmd -l 0,1 --log-level 7 -- --total-num-mbufs=16384 \
    -i --port-topology=chained --disable-hw-vlan-filter \
    --enable-rx-cksum --disable-hw-vlan-strip --txqflags=0

Without the fix, simple path is used for rx despite it does not
support rx checksum:

  ...
  PMD: set_rxtx_funcs(): virtio: using simple Rx path on port 0
  PMD: set_rxtx_funcs(): virtio: using standard Tx path on port 0
  ...

Configure testpmd:

  set fwd rxonly
  set verbose 1
  start

Without the fix, the received packets don't have the proper checksum
flags (should be PKT_RX_L4_CKSUM_NONE):

port 0/queue 0: received 1 packets
  src=1A:3F:FB:C6:FF:14 - dst=52:54:00:12:34:56 - type=0x0800 - length=74 - nb_segs=1 - sw ptype: L2_ETHER L3_IPV4 L4_TCP  - l2_len=14 - l3_len=20 - l4_len=40 - Receive queue=0x0
  ol_flags: PKT_RX_L4_CKSUM_UNKNOWN PKT_RX_IP_CKSUM_UNKNOWN

With the fix, standard rx path is used and the flags are correct:

  ...
  PMD: set_rxtx_funcs(): virtio: using standard Rx path on port 0
  PMD: set_rxtx_funcs(): virtio: using standard Tx path on port 0
  ...
  port 0/queue 0: received 1 packets
    src=1A:3F:FB:C6:FF:14 - dst=52:54:00:12:34:56 - type=0x0800 - length=74 - nb_segs=1 - hw ptype: L2_ETHER L3_IPV4 L4_TCP  - sw ptype: L2_ETHER L3_IPV4 L4_TCP  - l2_len=14 - l3_len=20 - l4_len=40 - Receive queue=0x0
    ol_flags: PKT_RX_L4_CKSUM_NONE PKT_RX_IP_CKSUM_UNKNOWN

Re: [dpdk-dev] [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers

[Yuanhan Liu]

On Thu, Aug 31, 2017 at 03:40:13PM +0200, Olivier Matz wrote:
> The selection of Rx/Tx handlers is done at several places,
> group them in one function set_rxtx_funcs().
> 
> The update of hw->use_simple_rxtx is also rationalized:
> - initialized to 1 (prefer simple path)
> - in dev configure or rx/tx queue setup, if something prevents from
>   using the simple path, change it to 0.
> - in dev start, set the handlers according to hw->use_simple_rxtx.
> 
> Cc: stable@dpdk.org

This patch looks like a refactoring to me, that I don't think it's really
a good idea to back port it to a stable release.

...
> @@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>  	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
>  
>  	eth_dev->dev_ops = &virtio_eth_dev_ops;
> -	eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
>  
>  	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
>  		if (!hw->virtio_user_dev) {
> @@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>  		}
>  
>  		virtio_set_vtpci_ops(hw);
> -		if (hw->use_simple_rxtx) {
> -			eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> -			eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> -		} else {
> -			rx_func_get(eth_dev);
> -		}
> +		set_rxtx_funcs(eth_dev);

No need to invoke it here?

> +
>  		return 0;
>  	}
>  
> @@ -1726,6 +1741,18 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>  			return -EBUSY;
>  		}
>  
> +	hw->use_simple_rxtx = 1;
> +
> +#if defined RTE_ARCH_X86
> +	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))

DPDK now requires SSE4.2. You might want to add another patch to remove
this testing.

> +		hw->use_simple_rxtx = 0;
> +#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> +	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> +		hw->use_simple_rxtx = 0;
> +#endif
> +	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
> +		hw->use_simple_rxtx = 0;
> +
>  	return 0;
>  }
>  
> @@ -1802,6 +1829,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
>  		VIRTQUEUE_DUMP(txvq->vq);
>  	}
>  
> +	set_rxtx_funcs(dev);
>  	hw->started = 1;
>  
>  	/* Initialize Link state */
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index a32e3229f..c9d97b643 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -505,25 +505,14 @@ static void
>  virtio_update_rxtx_handler(struct rte_eth_dev *dev,
>  			   const struct rte_eth_txconf *tx_conf)

This function name doesn't quite make sense now after your refactoring.

>  {
> -	uint8_t use_simple_rxtx = 0;
>  	struct virtio_hw *hw = dev->data->dev_private;
> +	uint8_t use_simple_rxtx = hw->use_simple_rxtx;
>  
> -#if defined RTE_ARCH_X86
> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
> -		use_simple_rxtx = 1;
> -#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> -		use_simple_rxtx = 1;
> -#endif
> -	/* Use simple rx/tx func if single segment and no offloads */
> -	if (use_simple_rxtx &&
> -	    (tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == VIRTIO_SIMPLE_FLAGS &&
> -	    !vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
> -		PMD_INIT_LOG(INFO, "Using simple rx/tx path");
> -		dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> -		dev->rx_pkt_burst = virtio_recv_pkts_vec;
> -		hw->use_simple_rxtx = use_simple_rxtx;
> -	}
> +	/* cannot use simple rxtx funcs with multisegs or offloads */
> +	if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) != VIRTIO_SIMPLE_FLAGS)
> +		use_simple_rxtx = 0;

And that's what left in this function. How about just un-folding it?

	--yliu

Re: [dpdk-dev] [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config

[Yuanhan Liu]

On Thu, Aug 31, 2017 at 03:40:14PM +0200, Olivier Matz wrote:
> Split use_simple_rxtx into use_simple_rx and use_simple_tx,
> and ensure that only use_simple_tx is updated when txq flags
> forces to use the standard Tx handler.

I think it's a good idea to split it.

> This change is also useful for next commit (disable simple Rx
> path when Rx checksum is requested).
> 
> Cc: stable@dpdk.org

But again, I don't think it's a good idea to put them (including the
next patch) to stable releases.

	--yliu

Re: [dpdk-dev] [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers

[Olivier MATZ]

Hi Yuanhan,

On Fri, Sep 01, 2017 at 05:19:16PM +0800, Yuanhan Liu wrote:
> On Thu, Aug 31, 2017 at 03:40:13PM +0200, Olivier Matz wrote:
> > The selection of Rx/Tx handlers is done at several places,
> > group them in one function set_rxtx_funcs().
> > 
> > The update of hw->use_simple_rxtx is also rationalized:
> > - initialized to 1 (prefer simple path)
> > - in dev configure or rx/tx queue setup, if something prevents from
> >   using the simple path, change it to 0.
> > - in dev start, set the handlers according to hw->use_simple_rxtx.
> > 
> > Cc: stable@dpdk.org
> 
> This patch looks like a refactoring to me, that I don't think it's really
> a good idea to back port it to a stable release.

I CCed stable for this commit because next ones, which are fixes, depend
on it. If you consider they should not be included in stable, we can also
drop this one.

> ...
> > @@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> >  	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
> >  
> >  	eth_dev->dev_ops = &virtio_eth_dev_ops;
> > -	eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
> >  
> >  	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> >  		if (!hw->virtio_user_dev) {
> > @@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> >  		}
> >  
> >  		virtio_set_vtpci_ops(hw);
> > -		if (hw->use_simple_rxtx) {
> > -			eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> > -			eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> > -		} else {
> > -			rx_func_get(eth_dev);
> > -		}
> > +		set_rxtx_funcs(eth_dev);
> 
> No need to invoke it here?

I wanted to stay consistent with previous code.

I'm not very used to work with secondary processes, so I'm not 100% it
is ok, but in my understanding, in that case the configuration is done
first by the primary process, and the secondary the pointers to the
rx/tx funcs. I suppose their value can be different than primary ones.

> 
> > +
> >  		return 0;
> >  	}
> >  
> > @@ -1726,6 +1741,18 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> >  			return -EBUSY;
> >  		}
> >  
> > +	hw->use_simple_rxtx = 1;
> > +
> > +#if defined RTE_ARCH_X86
> > +	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
> 
> DPDK now requires SSE4.2. You might want to add another patch to remove
> this testing.

ok, will do.

> 
> > +		hw->use_simple_rxtx = 0;
> > +#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> > +	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> > +		hw->use_simple_rxtx = 0;
> > +#endif
> > +	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
> > +		hw->use_simple_rxtx = 0;
> > +
> >  	return 0;
> >  }
> >  
> > @@ -1802,6 +1829,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
> >  		VIRTQUEUE_DUMP(txvq->vq);
> >  	}
> >  
> > +	set_rxtx_funcs(dev);
> >  	hw->started = 1;
> >  
> >  	/* Initialize Link state */
> > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> > index a32e3229f..c9d97b643 100644
> > --- a/drivers/net/virtio/virtio_rxtx.c
> > +++ b/drivers/net/virtio/virtio_rxtx.c
> > @@ -505,25 +505,14 @@ static void
> >  virtio_update_rxtx_handler(struct rte_eth_dev *dev,
> >  			   const struct rte_eth_txconf *tx_conf)
> 
> This function name doesn't quite make sense now after your refactoring.

It updates hw->use_simple_rxtx, which at the end will change the
rxtx handler. I didn't find a better name.

My other (poor) ideas were:
virtio_update_[rxtx_]handler_requirements
virtio_update_[rxtx_]handler_constraints
virtio_update_[rxtx_]handler_selector


> 
> >  {
> > -	uint8_t use_simple_rxtx = 0;
> >  	struct virtio_hw *hw = dev->data->dev_private;
> > +	uint8_t use_simple_rxtx = hw->use_simple_rxtx;
> >  
> > -#if defined RTE_ARCH_X86
> > -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
> > -		use_simple_rxtx = 1;
> > -#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> > -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> > -		use_simple_rxtx = 1;
> > -#endif
> > -	/* Use simple rx/tx func if single segment and no offloads */
> > -	if (use_simple_rxtx &&
> > -	    (tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == VIRTIO_SIMPLE_FLAGS &&
> > -	    !vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
> > -		PMD_INIT_LOG(INFO, "Using simple rx/tx path");
> > -		dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> > -		dev->rx_pkt_burst = virtio_recv_pkts_vec;
> > -		hw->use_simple_rxtx = use_simple_rxtx;
> > -	}
> > +	/* cannot use simple rxtx funcs with multisegs or offloads */
> > +	if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) != VIRTIO_SIMPLE_FLAGS)
> > +		use_simple_rxtx = 0;
> 
> And that's what left in this function. How about just un-folding it?
> 
> 	--yliu

yep, we can inline it.

Re: [dpdk-dev] [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config

[Olivier MATZ]

On Fri, Sep 01, 2017 at 05:25:38PM +0800, Yuanhan Liu wrote:
> On Thu, Aug 31, 2017 at 03:40:14PM +0200, Olivier Matz wrote:
> > Split use_simple_rxtx into use_simple_rx and use_simple_tx,
> > and ensure that only use_simple_tx is updated when txq flags
> > forces to use the standard Tx handler.
> 
> I think it's a good idea to split it.
> 
> > This change is also useful for next commit (disable simple Rx
> > path when Rx checksum is requested).
> > 
> > Cc: stable@dpdk.org
> 
> But again, I don't think it's a good idea to put them (including the
> next patch) to stable releases.

Yes, you're right this one is more an enhancement than a fix: it
just selects a faster handler if it's possible.

But next one is a fix: it must not select the simple handler if
offload is requested.

If these commits are not pushed in stable, it's not a problem for
me, so I let you decide. In that case, I will remove the Cc: stable
from the last 3 commits. Do you confirm?

Thanks for the review.
Olivier

Re: [dpdk-dev] [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config

[Yuanhan Liu]

On Fri, Sep 01, 2017 at 11:58:07AM +0200, Olivier MATZ wrote:
> On Fri, Sep 01, 2017 at 05:25:38PM +0800, Yuanhan Liu wrote:
> > On Thu, Aug 31, 2017 at 03:40:14PM +0200, Olivier Matz wrote:
> > > Split use_simple_rxtx into use_simple_rx and use_simple_tx,
> > > and ensure that only use_simple_tx is updated when txq flags
> > > forces to use the standard Tx handler.
> > 
> > I think it's a good idea to split it.
> > 
> > > This change is also useful for next commit (disable simple Rx
> > > path when Rx checksum is requested).
> > > 
> > > Cc: stable@dpdk.org
> > 
> > But again, I don't think it's a good idea to put them (including the
> > next patch) to stable releases.
> 
> Yes, you're right this one is more an enhancement than a fix: it
> just selects a faster handler if it's possible.

Exactly.

> But next one is a fix: it must not select the simple handler if
> offload is requested.
> 
> If these commits are not pushed in stable, it's not a problem for
> me, so I let you decide. In that case, I will remove the Cc: stable
> from the last 3 commits. Do you confirm?

Yes, I think it's better to drop the cc stable tag.

> Thanks for the review.

Thanks for the work!

	--yliu

Re: [dpdk-dev] [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers

[Yuanhan Liu]

On Fri, Sep 01, 2017 at 11:52:17AM +0200, Olivier MATZ wrote:
> > > @@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > >  	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
> > >  
> > >  	eth_dev->dev_ops = &virtio_eth_dev_ops;
> > > -	eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
> > >  
> > >  	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > >  		if (!hw->virtio_user_dev) {
> > > @@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > >  		}
> > >  
> > >  		virtio_set_vtpci_ops(hw);
> > > -		if (hw->use_simple_rxtx) {
> > > -			eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> > > -			eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> > > -		} else {
> > > -			rx_func_get(eth_dev);
> > > -		}
> > > +		set_rxtx_funcs(eth_dev);
> > 
> > No need to invoke it here?
> 
> I wanted to stay consistent with previous code.
> 
> I'm not very used to work with secondary processes, so I'm not 100% it
> is ok, but in my understanding, in that case the configuration is done
> first by the primary process, and the secondary the pointers to the
> rx/tx funcs. I suppose their value can be different than primary ones.

It probably needs some testing, but I think it should be okay, because
the rx/tx funcs will always be updated at dev_start in this patch.

	--yliu

Re: [dpdk-dev] [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers

[Olivier MATZ]

On Fri, Sep 01, 2017 at 08:31:06PM +0800, Yuanhan Liu wrote:
> On Fri, Sep 01, 2017 at 11:52:17AM +0200, Olivier MATZ wrote:
> > > > @@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > >  	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
> > > >  
> > > >  	eth_dev->dev_ops = &virtio_eth_dev_ops;
> > > > -	eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
> > > >  
> > > >  	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > > >  		if (!hw->virtio_user_dev) {
> > > > @@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > >  		}
> > > >  
> > > >  		virtio_set_vtpci_ops(hw);
> > > > -		if (hw->use_simple_rxtx) {
> > > > -			eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> > > > -			eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> > > > -		} else {
> > > > -			rx_func_get(eth_dev);
> > > > -		}
> > > > +		set_rxtx_funcs(eth_dev);
> > > 
> > > No need to invoke it here?
> > 
> > I wanted to stay consistent with previous code.
> > 
> > I'm not very used to work with secondary processes, so I'm not 100% it
> > is ok, but in my understanding, in that case the configuration is done
> > first by the primary process, and the secondary the pointers to the
> > rx/tx funcs. I suppose their value can be different than primary ones.
> 
> It probably needs some testing, but I think it should be okay, because
> the rx/tx funcs will always be updated at dev_start in this patch.

I still have one doubt about this: looking in
examples/multi_process/symmetric_mp/main.c, I can see that rte_eth_dev_start()
is only called on the primary process. So if I remove set_rxtx_funcs() from
eth_virtio_dev_init(), it looks that the rx functions won't be initialized.

Again, I'm not a user of multi_proc, so I may be wrong, but I think
it would be safer to keep it.

Olivier

Re: [dpdk-dev] [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers

[Yuanhan Liu]

On Wed, Sep 06, 2017 at 04:40:12PM +0200, Olivier MATZ wrote:
> On Fri, Sep 01, 2017 at 08:31:06PM +0800, Yuanhan Liu wrote:
> > On Fri, Sep 01, 2017 at 11:52:17AM +0200, Olivier MATZ wrote:
> > > > > @@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > > >  	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
> > > > >  
> > > > >  	eth_dev->dev_ops = &virtio_eth_dev_ops;
> > > > > -	eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
> > > > >  
> > > > >  	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > > > >  		if (!hw->virtio_user_dev) {
> > > > > @@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > > >  		}
> > > > >  
> > > > >  		virtio_set_vtpci_ops(hw);
> > > > > -		if (hw->use_simple_rxtx) {
> > > > > -			eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> > > > > -			eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> > > > > -		} else {
> > > > > -			rx_func_get(eth_dev);
> > > > > -		}
> > > > > +		set_rxtx_funcs(eth_dev);
> > > > 
> > > > No need to invoke it here?
> > > 
> > > I wanted to stay consistent with previous code.
> > > 
> > > I'm not very used to work with secondary processes, so I'm not 100% it
> > > is ok, but in my understanding, in that case the configuration is done
> > > first by the primary process, and the secondary the pointers to the
> > > rx/tx funcs. I suppose their value can be different than primary ones.
> > 
> > It probably needs some testing, but I think it should be okay, because
> > the rx/tx funcs will always be updated at dev_start in this patch.
> 
> I still have one doubt about this: looking in
> examples/multi_process/symmetric_mp/main.c, I can see that rte_eth_dev_start()
> is only called on the primary process. So if I remove set_rxtx_funcs() from
> eth_virtio_dev_init(), it looks that the rx functions won't be initialized.
> 
> Again, I'm not a user of multi_proc, so I may be wrong, but I think
> it would be safer to keep it.

Yes, you are right.

	--yliu

[dpdk-dev] [PATCH v2 00/10] virtio fixes

[Olivier Matz]

This patchset addresses several issues related to offload and
Rx/Tx handler selection in virtio PMD.

v1 -> v2:
- add one patch to remove uneeded SSE check on x86
- remove Cc stable on last patches
- inline virtio_update_rxtx_handler() in tx queue setup

Olivier Matz (10):
  net/virtio: revert "do not claim to support LRO"
  net/virtio: revert "do not falsely claim to do IP checksum"
  doc: fix description of L4 Rx checksum offload
  net/virtio: fix log levels in configure
  net/virtio: fix mbuf port for simple Rx function
  net/virtio: fix queue setup consistency
  net/virtio: rationalize setting of Rx/Tx handlers
  net/virtio: remove SSE check
  net/virtio: keep Rx handler whatever the Tx queue config
  net/virtio: fix Rx handler when checksum is requested

 doc/guides/nics/features.rst            |   1 +
 drivers/net/virtio/virtio_ethdev.c      | 111 ++++++++++++++++++++++++++------
 drivers/net/virtio/virtio_ethdev.h      |   6 ++
 drivers/net/virtio/virtio_pci.h         |   3 +-
 drivers/net/virtio/virtio_rxtx.c        |  73 ++++++++++-----------
 drivers/net/virtio/virtio_rxtx_simple.c |   2 +
 drivers/net/virtio/virtio_user_ethdev.c |   3 +-
 7 files changed, 141 insertions(+), 58 deletions(-)

-- 
2.11.0

[dpdk-dev] [PATCH v2 01/10] net/virtio: revert "do not claim to support LRO"

[Olivier Matz]

This reverts
commit 701a64622c26 ("net/virtio: do not claim to support LRO")

Setting rxmode->enable_lro is a way to tell the host that the guest is
ok to receive tso packets. From the guest point of view, it is like
enabling LRO on a physical driver.

Fixes: 701a64622c26 ("net/virtio: do not claim to support LRO")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index e320811ed..eb2d95acd 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1659,9 +1659,11 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 {
 	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
 	struct virtio_hw *hw = dev->data->dev_private;
+	uint64_t req_features;
 	int ret;
 
 	PMD_INIT_LOG(DEBUG, "configure");
+	req_features = VIRTIO_PMD_DEFAULT_GUEST_FEATURES;
 
 	if (dev->data->dev_conf.intr_conf.rxq) {
 		ret = virtio_init_device(dev, hw->req_guest_features);
@@ -1675,10 +1677,23 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			    "virtio does not support IP checksum");
 		return -ENOTSUP;
 	}
+	if (rxmode->enable_lro)
+		req_features |=
+			(1ULL << VIRTIO_NET_F_GUEST_TSO4) |
+			(1ULL << VIRTIO_NET_F_GUEST_TSO6);
 
-	if (rxmode->enable_lro) {
+	/* if request features changed, reinit the device */
+	if (req_features != hw->req_guest_features) {
+		ret = virtio_init_device(dev, req_features);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (rxmode->enable_lro &&
+		(!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4) ||
+			!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4))) {
 		PMD_DRV_LOG(NOTICE,
-			    "virtio does not support Large Receive Offload");
+			"Large Receive Offload not available on this host");
 		return -ENOTSUP;
 	}
 
@@ -1904,6 +1919,8 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	}
 	tso_mask = (1ULL << VIRTIO_NET_F_GUEST_TSO4) |
 		(1ULL << VIRTIO_NET_F_GUEST_TSO6);
+	if ((host_features & tso_mask) == tso_mask)
+		dev_info->rx_offload_capa |= DEV_RX_OFFLOAD_TCP_LRO;
 
 	dev_info->tx_offload_capa = 0;
 	if (hw->guest_features & (1ULL << VIRTIO_NET_F_CSUM)) {
-- 
2.11.0

[dpdk-dev] [PATCH v2 02/10] net/virtio: revert "do not falsely claim to do IP checksum"

[Olivier Matz]

This reverts
commit 4dab342b7522 ("net/virtio: do not falsely claim to do IP checksum").

The description of rxmode->hw_ip_checksum is:

     hw_ip_checksum   : 1, /**< IP/UDP/TCP checksum offload enable. */

Despite its name, this field can be set by an application to enable L3
and L4 checksums. In case of virtio, only L4 checksum is supported and
L3 checksums flags will always be set to "unknown".

Fixes: 4dab342b7522 ("net/virtio: do not falsely claim to do IP checksum")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index eb2d95acd..7a84817e5 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1671,12 +1671,13 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			return ret;
 	}
 
-	/* Virtio does L4 checksum but not L3! */
-	if (rxmode->hw_ip_checksum) {
-		PMD_DRV_LOG(NOTICE,
-			    "virtio does not support IP checksum");
-		return -ENOTSUP;
-	}
+	/* The name hw_ip_checksum is a bit confusing since it can be
+	 * set by the application to request L3 and/or L4 checksums. In
+	 * case of virtio, only L4 checksum is supported.
+	 */
+	if (rxmode->hw_ip_checksum)
+		req_features |= (1ULL << VIRTIO_NET_F_GUEST_CSUM);
+
 	if (rxmode->enable_lro)
 		req_features |=
 			(1ULL << VIRTIO_NET_F_GUEST_TSO4) |
@@ -1689,6 +1690,13 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			return ret;
 	}
 
+	if (rxmode->hw_ip_checksum &&
+		!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_CSUM)) {
+		PMD_DRV_LOG(NOTICE,
+			"rx checksum not available on this host");
+		return -ENOTSUP;
+	}
+
 	if (rxmode->enable_lro &&
 		(!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4) ||
 			!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4))) {
-- 
2.11.0

[dpdk-dev] [PATCH v2 03/10] doc: fix description of L4 Rx checksum offload

[Olivier Matz]

As described in API documentation, the field hw_ip_checksum
requests both L3 and L4 offload.

Fixes: dad1ec72a377 ("doc: document NIC features")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 doc/guides/nics/features.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
index 37ffbc68c..4430f09d1 100644
--- a/doc/guides/nics/features.rst
+++ b/doc/guides/nics/features.rst
@@ -557,6 +557,7 @@ L4 checksum offload
 
 Supports L4 checksum offload.
 
+* **[uses]     user config**: ``dev_conf.rxmode.hw_ip_checksum``.
 * **[uses]     mbuf**: ``mbuf.ol_flags:PKT_TX_IPV4`` | ``PKT_TX_IPV6``,
   ``mbuf.ol_flags:PKT_TX_L4_NO_CKSUM`` | ``PKT_TX_TCP_CKSUM`` |
   ``PKT_TX_SCTP_CKSUM`` | ``PKT_TX_UDP_CKSUM``.
-- 
2.11.0

[dpdk-dev] [PATCH v2 04/10] net/virtio: fix log levels in configure

[Olivier Matz]

On error, we should log with error level.

Fixes: 9f4f2846ef76 ("virtio: support vlan filtering")
Fixes: 86d59b21468a ("net/virtio: support LRO")
Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 7a84817e5..8eee3ff80 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1692,7 +1692,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 
 	if (rxmode->hw_ip_checksum &&
 		!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_CSUM)) {
-		PMD_DRV_LOG(NOTICE,
+		PMD_DRV_LOG(ERR,
 			"rx checksum not available on this host");
 		return -ENOTSUP;
 	}
@@ -1700,7 +1700,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))) {
-		PMD_DRV_LOG(NOTICE,
+		PMD_DRV_LOG(ERR,
 			"Large Receive Offload not available on this host");
 		return -ENOTSUP;
 	}
@@ -1713,7 +1713,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 
 	if (rxmode->hw_vlan_filter
 	    && !vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_VLAN)) {
-		PMD_DRV_LOG(NOTICE,
+		PMD_DRV_LOG(ERR,
 			    "vlan filtering not available on this host");
 		return -ENOTSUP;
 	}
-- 
2.11.0

[dpdk-dev] [PATCH v2 05/10] net/virtio: fix mbuf port for simple Rx function

[Olivier Matz]

The mbuf->port was was not properly set for the first received
mbufs. Fix this by setting it in virtqueue_enqueue_recv_refill_simple(),
which is used to enqueue the first mbuf in the ring.

The function virtio_rxq_rearm_vec(), which is used to rearm the ring
with new mbufs, is correct and does not need to be updated.

Fixes: cab0461234e7 ("virtio: fill Rx avail ring with blank mbufs")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_rxtx_simple.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/virtio/virtio_rxtx_simple.c b/drivers/net/virtio/virtio_rxtx_simple.c
index 542cf805d..54ababae9 100644
--- a/drivers/net/virtio/virtio_rxtx_simple.c
+++ b/drivers/net/virtio/virtio_rxtx_simple.c
@@ -65,6 +65,8 @@ virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
 	struct vring_desc *start_dp;
 	uint16_t desc_idx;
 
+	cookie->port = vq->rxq.port_id;
+
 	desc_idx = vq->vq_avail_idx & (vq->vq_nentries - 1);
 	dxp = &vq->vq_descx[desc_idx];
 	dxp->cookie = (void *)cookie;
-- 
2.11.0

[dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup consistency

[Olivier Matz]

In rx/tx queue setup functions, some code is executed only if
use_simple_rxtx == 1. The value of this variable can change depending on
the offload flags or sse support. If Rx queue setup is called before Tx
queue setup, it can result in an invalid configuration:

- dev_configure is called: use_simple_rxtx is initialized to 0
- rx queue setup is called: queues are initialized without simple path
  support
- tx queue setup is called: use_simple_rxtx switch to 1, and simple
  Rx/Tx handlers are selected

Fix this by postponing a part of Rx/Tx queue initialization in
dev_start(), as it was the case in the initial implementation.

Fixes: 48cec290a3d2 ("net/virtio: move queue configure code to proper place")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 13 +++++++++++++
 drivers/net/virtio/virtio_ethdev.h |  6 ++++++
 drivers/net/virtio/virtio_rxtx.c   | 40 ++++++++++++++++++++++++++++++--------
 3 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 8eee3ff80..c7888f103 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1737,6 +1737,19 @@ virtio_dev_start(struct rte_eth_dev *dev)
 	struct virtnet_rx *rxvq;
 	struct virtnet_tx *txvq __rte_unused;
 	struct virtio_hw *hw = dev->data->dev_private;
+	int ret;
+
+	/* Finish the initialization of the queues */
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		ret = virtio_dev_rx_queue_setup_finish(dev, i);
+		if (ret < 0)
+			return ret;
+	}
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		ret = virtio_dev_tx_queue_setup_finish(dev, i);
+		if (ret < 0)
+			return ret;
+	}
 
 	/* check if lsc interrupt feature is enabled */
 	if (dev->data->dev_conf.intr_conf.lsc) {
diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index c3413c6d9..2039bc547 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -92,10 +92,16 @@ int  virtio_dev_rx_queue_setup(struct rte_eth_dev *dev, uint16_t rx_queue_id,
 		const struct rte_eth_rxconf *rx_conf,
 		struct rte_mempool *mb_pool);
 
+int virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev,
+				uint16_t rx_queue_id);
+
 int  virtio_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,
 		uint16_t nb_tx_desc, unsigned int socket_id,
 		const struct rte_eth_txconf *tx_conf);
 
+int virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
+				uint16_t tx_queue_id);
+
 uint16_t virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 		uint16_t nb_pkts);
 
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index e30377c51..a32e3229f 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -421,9 +421,6 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	struct virtio_hw *hw = dev->data->dev_private;
 	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
 	struct virtnet_rx *rxvq;
-	int error, nbufs;
-	struct rte_mbuf *m;
-	uint16_t desc_idx;
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -440,10 +437,24 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	}
 	dev->data->rx_queues[queue_idx] = rxvq;
 
+	return 0;
+}
+
+int
+virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t queue_idx)
+{
+	uint16_t vtpci_queue_idx = 2 * queue_idx + VTNET_SQ_RQ_QUEUE_IDX;
+	struct virtio_hw *hw = dev->data->dev_private;
+	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
+	struct virtnet_rx *rxvq = &vq->rxq;
+	struct rte_mbuf *m;
+	uint16_t desc_idx;
+	int error, nbufs;
+
+	PMD_INIT_FUNC_TRACE();
 
 	/* Allocate blank mbufs for the each rx descriptor */
 	nbufs = 0;
-	error = ENOSPC;
 
 	if (hw->use_simple_rxtx) {
 		for (desc_idx = 0; desc_idx < vq->vq_nentries;
@@ -534,7 +545,6 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
 	struct virtnet_tx *txvq;
 	uint16_t tx_free_thresh;
-	uint16_t desc_idx;
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -563,9 +573,24 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 
 	vq->vq_free_thresh = tx_free_thresh;
 
-	if (hw->use_simple_rxtx) {
-		uint16_t mid_idx  = vq->vq_nentries >> 1;
+	dev->data->tx_queues[queue_idx] = txvq;
+	return 0;
+}
+
+int
+virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
+				uint16_t queue_idx)
+{
+	uint8_t vtpci_queue_idx = 2 * queue_idx + VTNET_SQ_TQ_QUEUE_IDX;
+	struct virtio_hw *hw = dev->data->dev_private;
+	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
+	uint16_t mid_idx = vq->vq_nentries >> 1;
+	struct virtnet_tx *txvq = &vq->txq;
+	uint16_t desc_idx;
 
+	PMD_INIT_FUNC_TRACE();
+
+	if (hw->use_simple_rxtx) {
 		for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
 			vq->vq_ring.avail->ring[desc_idx] =
 				desc_idx + mid_idx;
@@ -587,7 +612,6 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 
 	VIRTQUEUE_DUMP(vq);
 
-	dev->data->tx_queues[queue_idx] = txvq;
 	return 0;
 }
 
-- 
2.11.0

[dpdk-dev] [PATCH v2 07/10] net/virtio: rationalize setting of Rx/Tx handlers

[Olivier Matz]

The selection of Rx/Tx handlers is done at several places,
group them in one function set_rxtx_funcs().

The update of hw->use_simple_rxtx is also rationalized:
- initialized to 1 (prefer simple path)
- in dev configure or rx/tx queue setup, if something prevents from
  using the simple path, change it to 0.
- in dev start, set the handlers according to hw->use_simple_rxtx.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 52 +++++++++++++++++++++++++++++---------
 drivers/net/virtio/virtio_rxtx.c   | 29 +++------------------
 2 files changed, 43 insertions(+), 38 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index c7888f103..8dad3095f 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1235,14 +1235,36 @@ virtio_interrupt_handler(void *param)
 
 }
 
+/* set rx and tx handlers according to what is supported */
 static void
-rx_func_get(struct rte_eth_dev *eth_dev)
+set_rxtx_funcs(struct rte_eth_dev *eth_dev)
 {
 	struct virtio_hw *hw = eth_dev->data->dev_private;
-	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
+
+	if (hw->use_simple_rxtx) {
+		PMD_INIT_LOG(INFO, "virtio: using simple Rx path on port %u",
+			eth_dev->data->port_id);
+		eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
+	} else if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
+		PMD_INIT_LOG(INFO,
+			"virtio: using mergeable buffer Rx path on port %u",
+			eth_dev->data->port_id);
 		eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts;
-	else
+	} else {
+		PMD_INIT_LOG(INFO, "virtio: using standard Rx path on port %u",
+			eth_dev->data->port_id);
 		eth_dev->rx_pkt_burst = &virtio_recv_pkts;
+	}
+
+	if (hw->use_simple_rxtx) {
+		PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
+			eth_dev->data->port_id);
+		eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
+	} else {
+		PMD_INIT_LOG(INFO, "virtio: using standard Tx path on port %u",
+			eth_dev->data->port_id);
+		eth_dev->tx_pkt_burst = virtio_xmit_pkts;
+	}
 }
 
 /* Only support 1:1 queue/interrupt mapping so far.
@@ -1367,8 +1389,6 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 	else
 		eth_dev->data->dev_flags &= ~RTE_ETH_DEV_INTR_LSC;
 
-	rx_func_get(eth_dev);
-
 	/* Setting up rx_header size for the device */
 	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF) ||
 	    vtpci_with_feature(hw, VIRTIO_F_VERSION_1))
@@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
 
 	eth_dev->dev_ops = &virtio_eth_dev_ops;
-	eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
 
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
 		if (!hw->virtio_user_dev) {
@@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 		}
 
 		virtio_set_vtpci_ops(hw);
-		if (hw->use_simple_rxtx) {
-			eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
-			eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
-		} else {
-			rx_func_get(eth_dev);
-		}
+		set_rxtx_funcs(eth_dev);
+
 		return 0;
 	}
 
@@ -1726,6 +1741,18 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			return -EBUSY;
 		}
 
+	hw->use_simple_rxtx = 1;
+
+#if defined RTE_ARCH_X86
+	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
+		hw->use_simple_rxtx = 0;
+#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
+	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
+		hw->use_simple_rxtx = 0;
+#endif
+	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
+		hw->use_simple_rxtx = 0;
+
 	return 0;
 }
 
@@ -1802,6 +1829,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
 		VIRTQUEUE_DUMP(txvq->vq);
 	}
 
+	set_rxtx_funcs(dev);
 	hw->started = 1;
 
 	/* Initialize Link state */
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index a32e3229f..ef75ff5bd 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -501,31 +501,6 @@ virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t queue_idx)
 	return 0;
 }
 
-static void
-virtio_update_rxtx_handler(struct rte_eth_dev *dev,
-			   const struct rte_eth_txconf *tx_conf)
-{
-	uint8_t use_simple_rxtx = 0;
-	struct virtio_hw *hw = dev->data->dev_private;
-
-#if defined RTE_ARCH_X86
-	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
-		use_simple_rxtx = 1;
-#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
-	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
-		use_simple_rxtx = 1;
-#endif
-	/* Use simple rx/tx func if single segment and no offloads */
-	if (use_simple_rxtx &&
-	    (tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == VIRTIO_SIMPLE_FLAGS &&
-	    !vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
-		PMD_INIT_LOG(INFO, "Using simple rx/tx path");
-		dev->tx_pkt_burst = virtio_xmit_pkts_simple;
-		dev->rx_pkt_burst = virtio_recv_pkts_vec;
-		hw->use_simple_rxtx = use_simple_rxtx;
-	}
-}
-
 /*
  * struct rte_eth_dev *dev: Used to update dev
  * uint16_t nb_desc: Defaults to values read from config space
@@ -548,7 +523,9 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	virtio_update_rxtx_handler(dev, tx_conf);
+	/* cannot use simple rxtx funcs with multisegs or offloads */
+	if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) != VIRTIO_SIMPLE_FLAGS)
+		hw->use_simple_rxtx = 0;
 
 	if (nb_desc == 0 || nb_desc > vq->vq_nentries)
 		nb_desc = vq->vq_nentries;
-- 
2.11.0

[dpdk-dev] [PATCH v2 08/10] net/virtio: remove SSE check

[Olivier Matz]

Since commit f27769f796a0 ("mk: require SSE4.2 support on all x86 platforms"),
SSE4.2 is a requirement when compiling on x86 platforms.

We can remove this check in the virtio driver.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 8dad3095f..0a4c677d7 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1743,10 +1743,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 
 	hw->use_simple_rxtx = 1;
 
-#if defined RTE_ARCH_X86
-	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
-		hw->use_simple_rxtx = 0;
-#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
+#if defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
 	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
 		hw->use_simple_rxtx = 0;
 #endif
-- 
2.11.0

[dpdk-dev] [PATCH v2 09/10] net/virtio: keep Rx handler whatever the Tx queue config

[Olivier Matz]

Split use_simple_rxtx into use_simple_rx and use_simple_tx,
and ensure that only use_simple_tx is updated when txq flags
forces to use the standard Tx handler.

This change is also useful for next commit (disable simple Rx
path when Rx checksum is requested).

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c      | 19 ++++++++++++-------
 drivers/net/virtio/virtio_pci.h         |  3 ++-
 drivers/net/virtio/virtio_rxtx.c        |  8 ++++----
 drivers/net/virtio/virtio_user_ethdev.c |  3 ++-
 4 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 0a4c677d7..271ebaedf 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1241,7 +1241,7 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
 {
 	struct virtio_hw *hw = eth_dev->data->dev_private;
 
-	if (hw->use_simple_rxtx) {
+	if (hw->use_simple_rx) {
 		PMD_INIT_LOG(INFO, "virtio: using simple Rx path on port %u",
 			eth_dev->data->port_id);
 		eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
@@ -1256,7 +1256,7 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
 		eth_dev->rx_pkt_burst = &virtio_recv_pkts;
 	}
 
-	if (hw->use_simple_rxtx) {
+	if (hw->use_simple_tx) {
 		PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
 			eth_dev->data->port_id);
 		eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
@@ -1741,14 +1741,19 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			return -EBUSY;
 		}
 
-	hw->use_simple_rxtx = 1;
+	hw->use_simple_rx = 1;
+	hw->use_simple_tx = 1;
 
 #if defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
-	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
-		hw->use_simple_rxtx = 0;
+	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON)) {
+		hw->use_simple_rx = 0;
+		hw->use_simple_tx = 0;
+	}
 #endif
-	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
-		hw->use_simple_rxtx = 0;
+	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
+		hw->use_simple_rx = 0;
+		hw->use_simple_tx = 0;
+	}
 
 	return 0;
 }
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 18caebdd7..2ff526c88 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -259,7 +259,8 @@ struct virtio_hw {
 	uint8_t	    vlan_strip;
 	uint8_t	    use_msix;
 	uint8_t     modern;
-	uint8_t     use_simple_rxtx;
+	uint8_t     use_simple_rx;
+	uint8_t     use_simple_tx;
 	uint8_t     port_id;
 	uint8_t     mac_addr[ETHER_ADDR_LEN];
 	uint32_t    notify_off_multiplier;
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index ef75ff5bd..45a9c919a 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -456,7 +456,7 @@ virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t queue_idx)
 	/* Allocate blank mbufs for the each rx descriptor */
 	nbufs = 0;
 
-	if (hw->use_simple_rxtx) {
+	if (hw->use_simple_rx) {
 		for (desc_idx = 0; desc_idx < vq->vq_nentries;
 		     desc_idx++) {
 			vq->vq_ring.avail->ring[desc_idx] = desc_idx;
@@ -478,7 +478,7 @@ virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t queue_idx)
 			break;
 
 		/* Enqueue allocated buffers */
-		if (hw->use_simple_rxtx)
+		if (hw->use_simple_rx)
 			error = virtqueue_enqueue_recv_refill_simple(vq, m);
 		else
 			error = virtqueue_enqueue_recv_refill(vq, m);
@@ -525,7 +525,7 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 
 	/* cannot use simple rxtx funcs with multisegs or offloads */
 	if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) != VIRTIO_SIMPLE_FLAGS)
-		hw->use_simple_rxtx = 0;
+		hw->use_simple_tx = 0;
 
 	if (nb_desc == 0 || nb_desc > vq->vq_nentries)
 		nb_desc = vq->vq_nentries;
@@ -567,7 +567,7 @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	if (hw->use_simple_rxtx) {
+	if (hw->use_simple_tx) {
 		for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
 			vq->vq_ring.avail->ring[desc_idx] =
 				desc_idx + mid_idx;
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index c96144434..57c964d6d 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -369,7 +369,8 @@ virtio_user_eth_dev_alloc(struct rte_vdev_device *vdev)
 	 */
 	hw->use_msix = 1;
 	hw->modern   = 0;
-	hw->use_simple_rxtx = 0;
+	hw->use_simple_rx = 0;
+	hw->use_simple_tx = 0;
 	hw->virtio_user_dev = dev;
 	data->dev_flags = RTE_ETH_DEV_DETACHABLE;
 	return eth_dev;
-- 
2.11.0

[dpdk-dev] [PATCH v2 10/10] net/virtio: fix Rx handler when checksum is requested

[Olivier Matz]

The simple Rx handler is selected even if Rx checksum offload is
requested by the application, but this handler does not support
offloads. This results in broken received packets (no checksum flag but
invalid checksum in the mbuf data).

Disable the simple Rx handler in that case.

Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 271ebaedf..440c2d3b1 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1755,6 +1755,9 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 		hw->use_simple_tx = 0;
 	}
 
+	if (rxmode->hw_ip_checksum)
+		hw->use_simple_rx = 0;
+
 	return 0;
 }
 
-- 
2.11.0

Re: [dpdk-dev] [PATCH v2 00/10] virtio fixes

[Yuanhan Liu]

On Thu, Sep 07, 2017 at 02:13:37PM +0200, Olivier Matz wrote:
> This patchset addresses several issues related to offload and
> Rx/Tx handler selection in virtio PMD.

Thanks a lot for working on it. Seires applied to dpdk-next-virtio.

	--yliu
> 
> v1 -> v2:
> - add one patch to remove uneeded SSE check on x86
> - remove Cc stable on last patches
> - inline virtio_update_rxtx_handler() in tx queue setup
> 
> Olivier Matz (10):
>   net/virtio: revert "do not claim to support LRO"
>   net/virtio: revert "do not falsely claim to do IP checksum"
>   doc: fix description of L4 Rx checksum offload
>   net/virtio: fix log levels in configure
>   net/virtio: fix mbuf port for simple Rx function
>   net/virtio: fix queue setup consistency
>   net/virtio: rationalize setting of Rx/Tx handlers
>   net/virtio: remove SSE check
>   net/virtio: keep Rx handler whatever the Tx queue config
>   net/virtio: fix Rx handler when checksum is requested
> 
>  doc/guides/nics/features.rst            |   1 +
>  drivers/net/virtio/virtio_ethdev.c      | 111 ++++++++++++++++++++++++++------
>  drivers/net/virtio/virtio_ethdev.h      |   6 ++
>  drivers/net/virtio/virtio_pci.h         |   3 +-
>  drivers/net/virtio/virtio_rxtx.c        |  73 ++++++++++-----------
>  drivers/net/virtio/virtio_rxtx_simple.c |   2 +
>  drivers/net/virtio/virtio_user_ethdev.c |   3 +-
>  7 files changed, 141 insertions(+), 58 deletions(-)
> 
> -- 
> 2.11.0

Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup consistency

[Tiwei Bie]

Hi Maxime and Olivier:

On Thu, Sep 07, 2017 at 02:13:43PM +0200, Olivier Matz wrote:
[...]
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 8eee3ff80..c7888f103 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1737,6 +1737,19 @@ virtio_dev_start(struct rte_eth_dev *dev)
>  	struct virtnet_rx *rxvq;
>  	struct virtnet_tx *txvq __rte_unused;
>  	struct virtio_hw *hw = dev->data->dev_private;
> +	int ret;
> +
> +	/* Finish the initialization of the queues */
> +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +		ret = virtio_dev_rx_queue_setup_finish(dev, i);
> +		if (ret < 0)
> +			return ret;
> +	}

I'm trying to fix an issue [1] reported by Antonio. And during
the debugging, I found that vector Rx of virtio PMD has been
broken (when doing port stop/start) since below two patches were
applied:

25bf7a0b0936 ("vhost: make error handling consistent in Rx path")
   -- needed on the Tx side (testpmd/vhost-pmd in below test)
efc83a1e7fc3 ("net/virtio: fix queue setup consistency")
   -- needed on the Rx side (testpmd/virtio-user in below test)

Below are the steps to reproduce the issue:

#0. Checkout the commit

# 25bf7a0b0936 was applied after efc83a1e7fc3
git checkout 25bf7a0b0936

(There is another vector Rx bug caused by rxq flushing on the
 HEAD. So it's better to checkout the old commit first.)

#1. Apply below patch to disable mergeable Rx, and build DPDK

diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index 2039bc5..d45ffa5 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -65,7 +65,6 @@
 	 1u << VIRTIO_NET_F_CSUM	  |	\
 	 1u << VIRTIO_NET_F_HOST_TSO4	  |	\
 	 1u << VIRTIO_NET_F_HOST_TSO6	  |	\
-	 1u << VIRTIO_NET_F_MRG_RXBUF	  |	\
 	 1u << VIRTIO_NET_F_MTU	| \
 	 1u << VIRTIO_RING_F_INDIRECT_DESC |    \
 	 1ULL << VIRTIO_F_VERSION_1       |	\

#2. Launch testpmd/vhost-pmd:

./x86_64-native-linuxapp-gcc/app/testpmd -l 1,2 \
	--socket-mem 1024,1024 \
	--file-prefix=vhost \
	--no-pci \
	--vdev=net_vhost0,iface=/tmp/socket-0,queues=1 \
	-- \
	--port-topology=chained \
	-i \
	--nb-cores=1

#3. Launch testpmd/virtio-user:

./x86_64-native-linuxapp-gcc/app/testpmd -l 5,6 \
	--socket-mem 1024,1024 \
	--file-prefix=virtio-user \
	--no-pci \
	--vdev=net_virtio_user0,path=/tmp/socket-0 \
	-- \
	--port-topology=chained \
	-i \
	--nb-cores=1 \
	--disable-hw-vlan \
	--txqflags=0xf01

#4. In testpmd/virtio-user run below commands:

testpmd> set fwd rxonly
testpmd> start

#5. In testpmd/vhost-pmd run below commands:

testpmd> set burst 1
testpmd> set fwd rxonly
testpmd> start tx_first 1
testpmd> stop

#6. In testpmd/virtio-user run below commands:

testpmd> stop
testpmd> port stop all
testpmd> port start all
testpmd> start

#7. In testpmd/vhost-pmd run below commands:

testpmd> set fwd txonly
testpmd> start

#8. In testpmd/virtio-user run below commands:

testpmd> show port stats all

And you will see that there is no traffic any more after
receiving a few hundred packets.

[1] http://dpdk.org/ml/archives/dev/2017-December/082983.html

Best regards,
Tiwei Bie

Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup consistency

[Olivier MATZ]

Hi Tiwei,

On Wed, Dec 06, 2017 at 01:25:29PM +0800, Tiwei Bie wrote:
> Hi Maxime and Olivier:
> 
> On Thu, Sep 07, 2017 at 02:13:43PM +0200, Olivier Matz wrote:
> [...]
> > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> > index 8eee3ff80..c7888f103 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -1737,6 +1737,19 @@ virtio_dev_start(struct rte_eth_dev *dev)
> >  	struct virtnet_rx *rxvq;
> >  	struct virtnet_tx *txvq __rte_unused;
> >  	struct virtio_hw *hw = dev->data->dev_private;
> > +	int ret;
> > +
> > +	/* Finish the initialization of the queues */
> > +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > +		ret = virtio_dev_rx_queue_setup_finish(dev, i);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> 
> I'm trying to fix an issue [1] reported by Antonio. And during
> the debugging, I found that vector Rx of virtio PMD has been
> broken (when doing port stop/start) since below two patches were
> applied:
> 
> 25bf7a0b0936 ("vhost: make error handling consistent in Rx path")
>    -- needed on the Tx side (testpmd/vhost-pmd in below test)
> efc83a1e7fc3 ("net/virtio: fix queue setup consistency")
>    -- needed on the Rx side (testpmd/virtio-user in below test)

Just to be sure I understand properly: each of these 2 patches
break a different part your test case?

I tried to reproduce your test case (the working case first):
- on 0c4f909c17 (the commit before the efc83a1e7fc3)
- without the patch disabling mergeable Rx

No packet is received. Am I doing something wrong? Please see the
log:

   cd /root/dpdk.org
   git checkout -b test 0c4f909c17
   rm -rf build && make config T=x86_64-native-linuxapp-gcc && make -j32
   insmod build/kmod/igb_uio.ko
   echo 1000 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
   echo 1000 > /sys/devices/system/node/node1/hugepages/hugepages-2048kB/nr_hugepages
   mkdir -p /mnt/huge
   mount -t hugetlbfs none /mnt/huge

   # term 1: testpmd/vhost-pmd
   /root/dpdk.org/build/app/testpmd -l 1,2 \
    --socket-mem 512,512 \
    --file-prefix=vhost \
    --no-pci \
    --vdev=net_vhost0,iface=/tmp/socket-0,queues=1 \
    -- \
    --port-topology=chained \
    -i \
    --nb-cores=1

   # term 2: virtio-user
   /root/dpdk.org/build/app/testpmd -l 5,6 \
    --socket-mem 512,512 \
    --file-prefix=virtio-user \
    --no-pci \
    --vdev=net_virtio_user0,path=/tmp/socket-0 \
    -- \
    --port-topology=chained \
    -i \
    --nb-cores=1 \
    --disable-hw-vlan \
    --txqflags=0xf01
   testpmd> set fwd rxonly
   testpmd> start

   # back to term1: vhost
   testpmd> set burst 1
   testpmd> set fwd rxonly
   testpmd> start tx_first 1
   testpmd> stop

Result on term1:

  ---------------------- Forward statistics for port 0  ----------------------
  RX-packets: 0              RX-dropped: 0             RX-total: 0
  TX-packets: 0              TX-dropped: 1             TX-total: 1
  ----------------------------------------------------------------------------
  
  +++++++++++++++ Accumulated forward statistics for all ports+++++++++++++++
  RX-packets: 0              RX-dropped: 0             RX-total: 0
  TX-packets: 0              TX-dropped: 1             TX-total: 1
  ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++


Olivier

Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup consistency

[Tiwei Bie]

Hi Olivier,

On Thu, Dec 07, 2017 at 03:14:44PM +0100, Olivier MATZ wrote:
> On Wed, Dec 06, 2017 at 01:25:29PM +0800, Tiwei Bie wrote:
> > Hi Maxime and Olivier:
> > 
> > On Thu, Sep 07, 2017 at 02:13:43PM +0200, Olivier Matz wrote:
> > [...]
> > > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> > > index 8eee3ff80..c7888f103 100644
> > > --- a/drivers/net/virtio/virtio_ethdev.c
> > > +++ b/drivers/net/virtio/virtio_ethdev.c
> > > @@ -1737,6 +1737,19 @@ virtio_dev_start(struct rte_eth_dev *dev)
> > >  	struct virtnet_rx *rxvq;
> > >  	struct virtnet_tx *txvq __rte_unused;
> > >  	struct virtio_hw *hw = dev->data->dev_private;
> > > +	int ret;
> > > +
> > > +	/* Finish the initialization of the queues */
> > > +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > +		ret = virtio_dev_rx_queue_setup_finish(dev, i);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	}
> > 
> > I'm trying to fix an issue [1] reported by Antonio. And during
> > the debugging, I found that vector Rx of virtio PMD has been
> > broken (when doing port stop/start) since below two patches were
> > applied:
> > 
> > 25bf7a0b0936 ("vhost: make error handling consistent in Rx path")
> >    -- needed on the Tx side (testpmd/vhost-pmd in below test)
> > efc83a1e7fc3 ("net/virtio: fix queue setup consistency")
> >    -- needed on the Rx side (testpmd/virtio-user in below test)
> 
> Just to be sure I understand properly: each of these 2 patches
> break a different part your test case?
> 

Thank you for looking into this! ;-)

I mean the above test case won't pass when we have both
of them applied. And the first patch changes the Tx side,
and the second one changes the Rx side.

I haven't done thorough analysis on the first patch, so
I'm not sure what would be affected in the non-mergeable
Rx and vector Rx of virtio-PMD after changing the error
handling in vhost.

But I think there is something wrong with this patch (i.e.
the second patch). From my understanding, it seems that
virtio_rxq_rearm_vec() has an assumption that each time
it's called, the starting 'desc_idx' should be multiple
times of RTE_VIRTIO_VPMD_RX_REARM_THRESH (or 0). After
introducing virtio_dev_rx_queue_setup_finish() in device
start, the rxq will be fully refilled no matter where
the 'desc_idx' is after a device stop/start. And it could
break such assumption.

> I tried to reproduce your test case (the working case first):
> - on 0c4f909c17 (the commit before the efc83a1e7fc3)
> - without the patch disabling mergeable Rx
> 
> No packet is received. Am I doing something wrong? Please see the
> log:
> 
>    cd /root/dpdk.org
>    git checkout -b test 0c4f909c17
>    rm -rf build && make config T=x86_64-native-linuxapp-gcc && make -j32
>    insmod build/kmod/igb_uio.ko
>    echo 1000 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
>    echo 1000 > /sys/devices/system/node/node1/hugepages/hugepages-2048kB/nr_hugepages

Sorry, I forgot to mention that, 1G hugepage is required
to use virtio-user (2M hugepage won't work). For more
details about it, you could refer to the "Limitations"
section in below doc:

http://dpdk.org/doc/guides/howto/virtio_user_for_container_networking.html#limitations

Best regards,
Tiwei Bie

Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup consistency

[Yao, Lei A]

Hi, Olivier

This is Lei from DPDK validation team in Intel. During our DPDK 18.02-rc1 test,
I find the following patch will cause one serious issue with virtio vector path: 
the traffic can't resume after stop/start the virtio device. 

The step like following:
1. Launch vhost-user port using testpmd at Host 
2. Launch VM with virtio device, mergeable is off
3. Bind the virtio device to pmd driver, launch testpmd, let the tx/rx use vector path
    virtio_xmit_pkts_simple 
    virtio_recv_pkts_vec     
4. Send traffic to virtio device from vhost side, then stop the virtio device
5. Start the virtio device again
After step 5, the traffic can't resume. 

Could you help check this and give a fix? This issue will impact the virtio pmd user 
experience heavily. By the way, this patch is already included into V17.11. Looks like
we need give a patch to this LTS version.  Thanks a lot!

BRs
Lei
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Thursday, September 7, 2017 8:14 PM
> To: dev@dpdk.org; yliu@fridaylinux.org; maxime.coquelin@redhat.com
> Cc: stephen@networkplumber.org; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup consistency
> 
> In rx/tx queue setup functions, some code is executed only if
> use_simple_rxtx == 1. The value of this variable can change depending on
> the offload flags or sse support. If Rx queue setup is called before Tx
> queue setup, it can result in an invalid configuration:
> 
> - dev_configure is called: use_simple_rxtx is initialized to 0
> - rx queue setup is called: queues are initialized without simple path
>   support
> - tx queue setup is called: use_simple_rxtx switch to 1, and simple
>   Rx/Tx handlers are selected
> 
> Fix this by postponing a part of Rx/Tx queue initialization in
> dev_start(), as it was the case in the initial implementation.
> 
> Fixes: 48cec290a3d2 ("net/virtio: move queue configure code to proper
> place")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 13 +++++++++++++
>  drivers/net/virtio/virtio_ethdev.h |  6 ++++++
>  drivers/net/virtio/virtio_rxtx.c   | 40 ++++++++++++++++++++++++++++++-
> -------
>  3 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index 8eee3ff80..c7888f103 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1737,6 +1737,19 @@ virtio_dev_start(struct rte_eth_dev *dev)
>  	struct virtnet_rx *rxvq;
>  	struct virtnet_tx *txvq __rte_unused;
>  	struct virtio_hw *hw = dev->data->dev_private;
> +	int ret;
> +
> +	/* Finish the initialization of the queues */
> +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +		ret = virtio_dev_rx_queue_setup_finish(dev, i);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> +		ret = virtio_dev_tx_queue_setup_finish(dev, i);
> +		if (ret < 0)
> +			return ret;
> +	}
> 
>  	/* check if lsc interrupt feature is enabled */
>  	if (dev->data->dev_conf.intr_conf.lsc) {
> diff --git a/drivers/net/virtio/virtio_ethdev.h
> b/drivers/net/virtio/virtio_ethdev.h
> index c3413c6d9..2039bc547 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -92,10 +92,16 @@ int  virtio_dev_rx_queue_setup(struct rte_eth_dev
> *dev, uint16_t rx_queue_id,
>  		const struct rte_eth_rxconf *rx_conf,
>  		struct rte_mempool *mb_pool);
> 
> +int virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev,
> +				uint16_t rx_queue_id);
> +
>  int  virtio_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t
> tx_queue_id,
>  		uint16_t nb_tx_desc, unsigned int socket_id,
>  		const struct rte_eth_txconf *tx_conf);
> 
> +int virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
> +				uint16_t tx_queue_id);
> +
>  uint16_t virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>  		uint16_t nb_pkts);
> 
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index e30377c51..a32e3229f 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -421,9 +421,6 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
>  	struct virtio_hw *hw = dev->data->dev_private;
>  	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
>  	struct virtnet_rx *rxvq;
> -	int error, nbufs;
> -	struct rte_mbuf *m;
> -	uint16_t desc_idx;
> 
>  	PMD_INIT_FUNC_TRACE();
> 
> @@ -440,10 +437,24 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev
> *dev,
>  	}
>  	dev->data->rx_queues[queue_idx] = rxvq;
> 
> +	return 0;
> +}
> +
> +int
> +virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t
> queue_idx)
> +{
> +	uint16_t vtpci_queue_idx = 2 * queue_idx +
> VTNET_SQ_RQ_QUEUE_IDX;
> +	struct virtio_hw *hw = dev->data->dev_private;
> +	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> +	struct virtnet_rx *rxvq = &vq->rxq;
> +	struct rte_mbuf *m;
> +	uint16_t desc_idx;
> +	int error, nbufs;
> +
> +	PMD_INIT_FUNC_TRACE();
> 
>  	/* Allocate blank mbufs for the each rx descriptor */
>  	nbufs = 0;
> -	error = ENOSPC;
> 
>  	if (hw->use_simple_rxtx) {
>  		for (desc_idx = 0; desc_idx < vq->vq_nentries;
> @@ -534,7 +545,6 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
>  	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
>  	struct virtnet_tx *txvq;
>  	uint16_t tx_free_thresh;
> -	uint16_t desc_idx;
> 
>  	PMD_INIT_FUNC_TRACE();
> 
> @@ -563,9 +573,24 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev
> *dev,
> 
>  	vq->vq_free_thresh = tx_free_thresh;
> 
> -	if (hw->use_simple_rxtx) {
> -		uint16_t mid_idx  = vq->vq_nentries >> 1;
> +	dev->data->tx_queues[queue_idx] = txvq;
> +	return 0;
> +}
> +
> +int
> +virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
> +				uint16_t queue_idx)
> +{
> +	uint8_t vtpci_queue_idx = 2 * queue_idx +
> VTNET_SQ_TQ_QUEUE_IDX;
> +	struct virtio_hw *hw = dev->data->dev_private;
> +	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> +	uint16_t mid_idx = vq->vq_nentries >> 1;
> +	struct virtnet_tx *txvq = &vq->txq;
> +	uint16_t desc_idx;
> 
> +	PMD_INIT_FUNC_TRACE();
> +
> +	if (hw->use_simple_rxtx) {
>  		for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
>  			vq->vq_ring.avail->ring[desc_idx] =
>  				desc_idx + mid_idx;
> @@ -587,7 +612,6 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
> 
>  	VIRTQUEUE_DUMP(vq);
> 
> -	dev->data->tx_queues[queue_idx] = txvq;
>  	return 0;
>  }
> 
> --
> 2.11.0

Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup consistency

[Olivier Matz]

Hi Lei,

It's on my todo list, I'll check this as soon as possible.

Olivier


On Thu, Feb 01, 2018 at 03:14:15AM +0000, Yao, Lei A wrote:
> Hi, Olivier
> 
> This is Lei from DPDK validation team in Intel. During our DPDK 18.02-rc1 test,
> I find the following patch will cause one serious issue with virtio vector path: 
> the traffic can't resume after stop/start the virtio device. 
> 
> The step like following:
> 1. Launch vhost-user port using testpmd at Host 
> 2. Launch VM with virtio device, mergeable is off
> 3. Bind the virtio device to pmd driver, launch testpmd, let the tx/rx use vector path
>     virtio_xmit_pkts_simple 
>     virtio_recv_pkts_vec     
> 4. Send traffic to virtio device from vhost side, then stop the virtio device
> 5. Start the virtio device again
> After step 5, the traffic can't resume. 
> 
> Could you help check this and give a fix? This issue will impact the virtio pmd user 
> experience heavily. By the way, this patch is already included into V17.11. Looks like
> we need give a patch to this LTS version.  Thanks a lot!
> 
> BRs
> Lei
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> > Sent: Thursday, September 7, 2017 8:14 PM
> > To: dev@dpdk.org; yliu@fridaylinux.org; maxime.coquelin@redhat.com
> > Cc: stephen@networkplumber.org; stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup consistency
> > 
> > In rx/tx queue setup functions, some code is executed only if
> > use_simple_rxtx == 1. The value of this variable can change depending on
> > the offload flags or sse support. If Rx queue setup is called before Tx
> > queue setup, it can result in an invalid configuration:
> > 
> > - dev_configure is called: use_simple_rxtx is initialized to 0
> > - rx queue setup is called: queues are initialized without simple path
> >   support
> > - tx queue setup is called: use_simple_rxtx switch to 1, and simple
> >   Rx/Tx handlers are selected
> > 
> > Fix this by postponing a part of Rx/Tx queue initialization in
> > dev_start(), as it was the case in the initial implementation.
> > 
> > Fixes: 48cec290a3d2 ("net/virtio: move queue configure code to proper
> > place")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > ---
> >  drivers/net/virtio/virtio_ethdev.c | 13 +++++++++++++
> >  drivers/net/virtio/virtio_ethdev.h |  6 ++++++
> >  drivers/net/virtio/virtio_rxtx.c   | 40 ++++++++++++++++++++++++++++++-
> > -------
> >  3 files changed, 51 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/net/virtio/virtio_ethdev.c
> > b/drivers/net/virtio/virtio_ethdev.c
> > index 8eee3ff80..c7888f103 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -1737,6 +1737,19 @@ virtio_dev_start(struct rte_eth_dev *dev)
> >  	struct virtnet_rx *rxvq;
> >  	struct virtnet_tx *txvq __rte_unused;
> >  	struct virtio_hw *hw = dev->data->dev_private;
> > +	int ret;
> > +
> > +	/* Finish the initialization of the queues */
> > +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > +		ret = virtio_dev_rx_queue_setup_finish(dev, i);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > +		ret = virtio_dev_tx_queue_setup_finish(dev, i);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > 
> >  	/* check if lsc interrupt feature is enabled */
> >  	if (dev->data->dev_conf.intr_conf.lsc) {
> > diff --git a/drivers/net/virtio/virtio_ethdev.h
> > b/drivers/net/virtio/virtio_ethdev.h
> > index c3413c6d9..2039bc547 100644
> > --- a/drivers/net/virtio/virtio_ethdev.h
> > +++ b/drivers/net/virtio/virtio_ethdev.h
> > @@ -92,10 +92,16 @@ int  virtio_dev_rx_queue_setup(struct rte_eth_dev
> > *dev, uint16_t rx_queue_id,
> >  		const struct rte_eth_rxconf *rx_conf,
> >  		struct rte_mempool *mb_pool);
> > 
> > +int virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev,
> > +				uint16_t rx_queue_id);
> > +
> >  int  virtio_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t
> > tx_queue_id,
> >  		uint16_t nb_tx_desc, unsigned int socket_id,
> >  		const struct rte_eth_txconf *tx_conf);
> > 
> > +int virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
> > +				uint16_t tx_queue_id);
> > +
> >  uint16_t virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
> >  		uint16_t nb_pkts);
> > 
> > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> > index e30377c51..a32e3229f 100644
> > --- a/drivers/net/virtio/virtio_rxtx.c
> > +++ b/drivers/net/virtio/virtio_rxtx.c
> > @@ -421,9 +421,6 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
> >  	struct virtio_hw *hw = dev->data->dev_private;
> >  	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> >  	struct virtnet_rx *rxvq;
> > -	int error, nbufs;
> > -	struct rte_mbuf *m;
> > -	uint16_t desc_idx;
> > 
> >  	PMD_INIT_FUNC_TRACE();
> > 
> > @@ -440,10 +437,24 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev
> > *dev,
> >  	}
> >  	dev->data->rx_queues[queue_idx] = rxvq;
> > 
> > +	return 0;
> > +}
> > +
> > +int
> > +virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t
> > queue_idx)
> > +{
> > +	uint16_t vtpci_queue_idx = 2 * queue_idx +
> > VTNET_SQ_RQ_QUEUE_IDX;
> > +	struct virtio_hw *hw = dev->data->dev_private;
> > +	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> > +	struct virtnet_rx *rxvq = &vq->rxq;
> > +	struct rte_mbuf *m;
> > +	uint16_t desc_idx;
> > +	int error, nbufs;
> > +
> > +	PMD_INIT_FUNC_TRACE();
> > 
> >  	/* Allocate blank mbufs for the each rx descriptor */
> >  	nbufs = 0;
> > -	error = ENOSPC;
> > 
> >  	if (hw->use_simple_rxtx) {
> >  		for (desc_idx = 0; desc_idx < vq->vq_nentries;
> > @@ -534,7 +545,6 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
> >  	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> >  	struct virtnet_tx *txvq;
> >  	uint16_t tx_free_thresh;
> > -	uint16_t desc_idx;
> > 
> >  	PMD_INIT_FUNC_TRACE();
> > 
> > @@ -563,9 +573,24 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev
> > *dev,
> > 
> >  	vq->vq_free_thresh = tx_free_thresh;
> > 
> > -	if (hw->use_simple_rxtx) {
> > -		uint16_t mid_idx  = vq->vq_nentries >> 1;
> > +	dev->data->tx_queues[queue_idx] = txvq;
> > +	return 0;
> > +}
> > +
> > +int
> > +virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
> > +				uint16_t queue_idx)
> > +{
> > +	uint8_t vtpci_queue_idx = 2 * queue_idx +
> > VTNET_SQ_TQ_QUEUE_IDX;
> > +	struct virtio_hw *hw = dev->data->dev_private;
> > +	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> > +	uint16_t mid_idx = vq->vq_nentries >> 1;
> > +	struct virtnet_tx *txvq = &vq->txq;
> > +	uint16_t desc_idx;
> > 
> > +	PMD_INIT_FUNC_TRACE();
> > +
> > +	if (hw->use_simple_rxtx) {
> >  		for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
> >  			vq->vq_ring.avail->ring[desc_idx] =
> >  				desc_idx + mid_idx;
> > @@ -587,7 +612,6 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
> > 
> >  	VIRTQUEUE_DUMP(vq);
> > 
> > -	dev->data->tx_queues[queue_idx] = txvq;
> >  	return 0;
> >  }
> > 
> > --
> > 2.11.0
> 

Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup consistency

[Xu, Qian Q]

Any update, Olivier? 
We are near to release, and the bug-fix is important for the virtio vector path usage. Thanks. 

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Thursday, February 1, 2018 4:28 PM
> To: Yao, Lei A <lei.a.yao@intel.com>
> Cc: dev@dpdk.org; yliu@fridaylinux.org; maxime.coquelin@redhat.com;
> Thomas Monjalon <thomas@monjalon.net>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup consistency
> 
> Hi Lei,
> 
> It's on my todo list, I'll check this as soon as possible.
> 
> Olivier
> 
> 
> On Thu, Feb 01, 2018 at 03:14:15AM +0000, Yao, Lei A wrote:
> > Hi, Olivier
> >
> > This is Lei from DPDK validation team in Intel. During our DPDK
> > 18.02-rc1 test, I find the following patch will cause one serious issue with virtio
> vector path:
> > the traffic can't resume after stop/start the virtio device.
> >
> > The step like following:
> > 1. Launch vhost-user port using testpmd at Host 2. Launch VM with
> > virtio device, mergeable is off 3. Bind the virtio device to pmd
> > driver, launch testpmd, let the tx/rx use vector path
> >     virtio_xmit_pkts_simple
> >     virtio_recv_pkts_vec
> > 4. Send traffic to virtio device from vhost side, then stop the virtio
> > device 5. Start the virtio device again After step 5, the traffic
> > can't resume.
> >
> > Could you help check this and give a fix? This issue will impact the
> > virtio pmd user experience heavily. By the way, this patch is already
> > included into V17.11. Looks like we need give a patch to this LTS version.
> Thanks a lot!
> >
> > BRs
> > Lei
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> > > Sent: Thursday, September 7, 2017 8:14 PM
> > > To: dev@dpdk.org; yliu@fridaylinux.org; maxime.coquelin@redhat.com
> > > Cc: stephen@networkplumber.org; stable@dpdk.org
> > > Subject: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup
> > > consistency
> > >
> > > In rx/tx queue setup functions, some code is executed only if
> > > use_simple_rxtx == 1. The value of this variable can change
> > > depending on the offload flags or sse support. If Rx queue setup is
> > > called before Tx queue setup, it can result in an invalid configuration:
> > >
> > > - dev_configure is called: use_simple_rxtx is initialized to 0
> > > - rx queue setup is called: queues are initialized without simple path
> > >   support
> > > - tx queue setup is called: use_simple_rxtx switch to 1, and simple
> > >   Rx/Tx handlers are selected
> > >
> > > Fix this by postponing a part of Rx/Tx queue initialization in
> > > dev_start(), as it was the case in the initial implementation.
> > >
> > > Fixes: 48cec290a3d2 ("net/virtio: move queue configure code to
> > > proper
> > > place")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > > ---
> > >  drivers/net/virtio/virtio_ethdev.c | 13 +++++++++++++
> > > drivers/net/virtio/virtio_ethdev.h |  6 ++++++
> > >  drivers/net/virtio/virtio_rxtx.c   | 40 ++++++++++++++++++++++++++++++-
> > > -------
> > >  3 files changed, 51 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio/virtio_ethdev.c
> > > b/drivers/net/virtio/virtio_ethdev.c
> > > index 8eee3ff80..c7888f103 100644
> > > --- a/drivers/net/virtio/virtio_ethdev.c
> > > +++ b/drivers/net/virtio/virtio_ethdev.c
> > > @@ -1737,6 +1737,19 @@ virtio_dev_start(struct rte_eth_dev *dev)
> > >  	struct virtnet_rx *rxvq;
> > >  	struct virtnet_tx *txvq __rte_unused;
> > >  	struct virtio_hw *hw = dev->data->dev_private;
> > > +	int ret;
> > > +
> > > +	/* Finish the initialization of the queues */
> > > +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > +		ret = virtio_dev_rx_queue_setup_finish(dev, i);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	}
> > > +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > > +		ret = virtio_dev_tx_queue_setup_finish(dev, i);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	}
> > >
> > >  	/* check if lsc interrupt feature is enabled */
> > >  	if (dev->data->dev_conf.intr_conf.lsc) { diff --git
> > > a/drivers/net/virtio/virtio_ethdev.h
> > > b/drivers/net/virtio/virtio_ethdev.h
> > > index c3413c6d9..2039bc547 100644
> > > --- a/drivers/net/virtio/virtio_ethdev.h
> > > +++ b/drivers/net/virtio/virtio_ethdev.h
> > > @@ -92,10 +92,16 @@ int  virtio_dev_rx_queue_setup(struct
> > > rte_eth_dev *dev, uint16_t rx_queue_id,
> > >  		const struct rte_eth_rxconf *rx_conf,
> > >  		struct rte_mempool *mb_pool);
> > >
> > > +int virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev,
> > > +				uint16_t rx_queue_id);
> > > +
> > >  int  virtio_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t
> > > tx_queue_id,
> > >  		uint16_t nb_tx_desc, unsigned int socket_id,
> > >  		const struct rte_eth_txconf *tx_conf);
> > >
> > > +int virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
> > > +				uint16_t tx_queue_id);
> > > +
> > >  uint16_t virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
> > >  		uint16_t nb_pkts);
> > >
> > > diff --git a/drivers/net/virtio/virtio_rxtx.c
> > > b/drivers/net/virtio/virtio_rxtx.c
> > > index e30377c51..a32e3229f 100644
> > > --- a/drivers/net/virtio/virtio_rxtx.c
> > > +++ b/drivers/net/virtio/virtio_rxtx.c
> > > @@ -421,9 +421,6 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
> > >  	struct virtio_hw *hw = dev->data->dev_private;
> > >  	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> > >  	struct virtnet_rx *rxvq;
> > > -	int error, nbufs;
> > > -	struct rte_mbuf *m;
> > > -	uint16_t desc_idx;
> > >
> > >  	PMD_INIT_FUNC_TRACE();
> > >
> > > @@ -440,10 +437,24 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev
> > > *dev,
> > >  	}
> > >  	dev->data->rx_queues[queue_idx] = rxvq;
> > >
> > > +	return 0;
> > > +}
> > > +
> > > +int
> > > +virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t
> > > queue_idx)
> > > +{
> > > +	uint16_t vtpci_queue_idx = 2 * queue_idx +
> > > VTNET_SQ_RQ_QUEUE_IDX;
> > > +	struct virtio_hw *hw = dev->data->dev_private;
> > > +	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> > > +	struct virtnet_rx *rxvq = &vq->rxq;
> > > +	struct rte_mbuf *m;
> > > +	uint16_t desc_idx;
> > > +	int error, nbufs;
> > > +
> > > +	PMD_INIT_FUNC_TRACE();
> > >
> > >  	/* Allocate blank mbufs for the each rx descriptor */
> > >  	nbufs = 0;
> > > -	error = ENOSPC;
> > >
> > >  	if (hw->use_simple_rxtx) {
> > >  		for (desc_idx = 0; desc_idx < vq->vq_nentries; @@ -534,7
> +545,6
> > > @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
> > >  	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> > >  	struct virtnet_tx *txvq;
> > >  	uint16_t tx_free_thresh;
> > > -	uint16_t desc_idx;
> > >
> > >  	PMD_INIT_FUNC_TRACE();
> > >
> > > @@ -563,9 +573,24 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev
> > > *dev,
> > >
> > >  	vq->vq_free_thresh = tx_free_thresh;
> > >
> > > -	if (hw->use_simple_rxtx) {
> > > -		uint16_t mid_idx  = vq->vq_nentries >> 1;
> > > +	dev->data->tx_queues[queue_idx] = txvq;
> > > +	return 0;
> > > +}
> > > +
> > > +int
> > > +virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
> > > +				uint16_t queue_idx)
> > > +{
> > > +	uint8_t vtpci_queue_idx = 2 * queue_idx +
> > > VTNET_SQ_TQ_QUEUE_IDX;
> > > +	struct virtio_hw *hw = dev->data->dev_private;
> > > +	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> > > +	uint16_t mid_idx = vq->vq_nentries >> 1;
> > > +	struct virtnet_tx *txvq = &vq->txq;
> > > +	uint16_t desc_idx;
> > >
> > > +	PMD_INIT_FUNC_TRACE();
> > > +
> > > +	if (hw->use_simple_rxtx) {
> > >  		for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
> > >  			vq->vq_ring.avail->ring[desc_idx] =
> > >  				desc_idx + mid_idx;
> > > @@ -587,7 +612,6 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev
> > > *dev,
> > >
> > >  	VIRTQUEUE_DUMP(vq);
> > >
> > > -	dev->data->tx_queues[queue_idx] = txvq;
> > >  	return 0;
> > >  }
> > >
> > > --
> > > 2.11.0
> >

Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup consistency

[Olivier Matz]

Hi,

It's in my short plans, but unfortunately some other high priority tasks
were inserted before. Honnestly, I'm not sure I'll be able to make it
for the release, but I'll do my best.

Olivier



On Wed, Feb 07, 2018 at 08:31:07AM +0000, Xu, Qian Q wrote:
> Any update, Olivier? 
> We are near to release, and the bug-fix is important for the virtio vector path usage. Thanks. 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> > Sent: Thursday, February 1, 2018 4:28 PM
> > To: Yao, Lei A <lei.a.yao@intel.com>
> > Cc: dev@dpdk.org; yliu@fridaylinux.org; maxime.coquelin@redhat.com;
> > Thomas Monjalon <thomas@monjalon.net>; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup consistency
> > 
> > Hi Lei,
> > 
> > It's on my todo list, I'll check this as soon as possible.
> > 
> > Olivier
> > 
> > 
> > On Thu, Feb 01, 2018 at 03:14:15AM +0000, Yao, Lei A wrote:
> > > Hi, Olivier
> > >
> > > This is Lei from DPDK validation team in Intel. During our DPDK
> > > 18.02-rc1 test, I find the following patch will cause one serious issue with virtio
> > vector path:
> > > the traffic can't resume after stop/start the virtio device.
> > >
> > > The step like following:
> > > 1. Launch vhost-user port using testpmd at Host 2. Launch VM with
> > > virtio device, mergeable is off 3. Bind the virtio device to pmd
> > > driver, launch testpmd, let the tx/rx use vector path
> > >     virtio_xmit_pkts_simple
> > >     virtio_recv_pkts_vec
> > > 4. Send traffic to virtio device from vhost side, then stop the virtio
> > > device 5. Start the virtio device again After step 5, the traffic
> > > can't resume.
> > >
> > > Could you help check this and give a fix? This issue will impact the
> > > virtio pmd user experience heavily. By the way, this patch is already
> > > included into V17.11. Looks like we need give a patch to this LTS version.
> > Thanks a lot!
> > >
> > > BRs
> > > Lei
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> > > > Sent: Thursday, September 7, 2017 8:14 PM
> > > > To: dev@dpdk.org; yliu@fridaylinux.org; maxime.coquelin@redhat.com
> > > > Cc: stephen@networkplumber.org; stable@dpdk.org
> > > > Subject: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup
> > > > consistency
> > > >
> > > > In rx/tx queue setup functions, some code is executed only if
> > > > use_simple_rxtx == 1. The value of this variable can change
> > > > depending on the offload flags or sse support. If Rx queue setup is
> > > > called before Tx queue setup, it can result in an invalid configuration:
> > > >
> > > > - dev_configure is called: use_simple_rxtx is initialized to 0
> > > > - rx queue setup is called: queues are initialized without simple path
> > > >   support
> > > > - tx queue setup is called: use_simple_rxtx switch to 1, and simple
> > > >   Rx/Tx handlers are selected
> > > >
> > > > Fix this by postponing a part of Rx/Tx queue initialization in
> > > > dev_start(), as it was the case in the initial implementation.
> > > >
> > > > Fixes: 48cec290a3d2 ("net/virtio: move queue configure code to
> > > > proper
> > > > place")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > > > ---
> > > >  drivers/net/virtio/virtio_ethdev.c | 13 +++++++++++++
> > > > drivers/net/virtio/virtio_ethdev.h |  6 ++++++
> > > >  drivers/net/virtio/virtio_rxtx.c   | 40 ++++++++++++++++++++++++++++++-
> > > > -------
> > > >  3 files changed, 51 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/net/virtio/virtio_ethdev.c
> > > > b/drivers/net/virtio/virtio_ethdev.c
> > > > index 8eee3ff80..c7888f103 100644
> > > > --- a/drivers/net/virtio/virtio_ethdev.c
> > > > +++ b/drivers/net/virtio/virtio_ethdev.c
> > > > @@ -1737,6 +1737,19 @@ virtio_dev_start(struct rte_eth_dev *dev)
> > > >  	struct virtnet_rx *rxvq;
> > > >  	struct virtnet_tx *txvq __rte_unused;
> > > >  	struct virtio_hw *hw = dev->data->dev_private;
> > > > +	int ret;
> > > > +
> > > > +	/* Finish the initialization of the queues */
> > > > +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > > +		ret = virtio_dev_rx_queue_setup_finish(dev, i);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +	}
> > > > +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > > > +		ret = virtio_dev_tx_queue_setup_finish(dev, i);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +	}
> > > >
> > > >  	/* check if lsc interrupt feature is enabled */
> > > >  	if (dev->data->dev_conf.intr_conf.lsc) { diff --git
> > > > a/drivers/net/virtio/virtio_ethdev.h
> > > > b/drivers/net/virtio/virtio_ethdev.h
> > > > index c3413c6d9..2039bc547 100644
> > > > --- a/drivers/net/virtio/virtio_ethdev.h
> > > > +++ b/drivers/net/virtio/virtio_ethdev.h
> > > > @@ -92,10 +92,16 @@ int  virtio_dev_rx_queue_setup(struct
> > > > rte_eth_dev *dev, uint16_t rx_queue_id,
> > > >  		const struct rte_eth_rxconf *rx_conf,
> > > >  		struct rte_mempool *mb_pool);
> > > >
> > > > +int virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev,
> > > > +				uint16_t rx_queue_id);
> > > > +
> > > >  int  virtio_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t
> > > > tx_queue_id,
> > > >  		uint16_t nb_tx_desc, unsigned int socket_id,
> > > >  		const struct rte_eth_txconf *tx_conf);
> > > >
> > > > +int virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
> > > > +				uint16_t tx_queue_id);
> > > > +
> > > >  uint16_t virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
> > > >  		uint16_t nb_pkts);
> > > >
> > > > diff --git a/drivers/net/virtio/virtio_rxtx.c
> > > > b/drivers/net/virtio/virtio_rxtx.c
> > > > index e30377c51..a32e3229f 100644
> > > > --- a/drivers/net/virtio/virtio_rxtx.c
> > > > +++ b/drivers/net/virtio/virtio_rxtx.c
> > > > @@ -421,9 +421,6 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
> > > >  	struct virtio_hw *hw = dev->data->dev_private;
> > > >  	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> > > >  	struct virtnet_rx *rxvq;
> > > > -	int error, nbufs;
> > > > -	struct rte_mbuf *m;
> > > > -	uint16_t desc_idx;
> > > >
> > > >  	PMD_INIT_FUNC_TRACE();
> > > >
> > > > @@ -440,10 +437,24 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev
> > > > *dev,
> > > >  	}
> > > >  	dev->data->rx_queues[queue_idx] = rxvq;
> > > >
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +int
> > > > +virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t
> > > > queue_idx)
> > > > +{
> > > > +	uint16_t vtpci_queue_idx = 2 * queue_idx +
> > > > VTNET_SQ_RQ_QUEUE_IDX;
> > > > +	struct virtio_hw *hw = dev->data->dev_private;
> > > > +	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> > > > +	struct virtnet_rx *rxvq = &vq->rxq;
> > > > +	struct rte_mbuf *m;
> > > > +	uint16_t desc_idx;
> > > > +	int error, nbufs;
> > > > +
> > > > +	PMD_INIT_FUNC_TRACE();
> > > >
> > > >  	/* Allocate blank mbufs for the each rx descriptor */
> > > >  	nbufs = 0;
> > > > -	error = ENOSPC;
> > > >
> > > >  	if (hw->use_simple_rxtx) {
> > > >  		for (desc_idx = 0; desc_idx < vq->vq_nentries; @@ -534,7
> > +545,6
> > > > @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
> > > >  	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> > > >  	struct virtnet_tx *txvq;
> > > >  	uint16_t tx_free_thresh;
> > > > -	uint16_t desc_idx;
> > > >
> > > >  	PMD_INIT_FUNC_TRACE();
> > > >
> > > > @@ -563,9 +573,24 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev
> > > > *dev,
> > > >
> > > >  	vq->vq_free_thresh = tx_free_thresh;
> > > >
> > > > -	if (hw->use_simple_rxtx) {
> > > > -		uint16_t mid_idx  = vq->vq_nentries >> 1;
> > > > +	dev->data->tx_queues[queue_idx] = txvq;
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +int
> > > > +virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
> > > > +				uint16_t queue_idx)
> > > > +{
> > > > +	uint8_t vtpci_queue_idx = 2 * queue_idx +
> > > > VTNET_SQ_TQ_QUEUE_IDX;
> > > > +	struct virtio_hw *hw = dev->data->dev_private;
> > > > +	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> > > > +	uint16_t mid_idx = vq->vq_nentries >> 1;
> > > > +	struct virtnet_tx *txvq = &vq->txq;
> > > > +	uint16_t desc_idx;
> > > >
> > > > +	PMD_INIT_FUNC_TRACE();
> > > > +
> > > > +	if (hw->use_simple_rxtx) {
> > > >  		for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
> > > >  			vq->vq_ring.avail->ring[desc_idx] =
> > > >  				desc_idx + mid_idx;
> > > > @@ -587,7 +612,6 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev
> > > > *dev,
> > > >
> > > >  	VIRTQUEUE_DUMP(vq);
> > > >
> > > > -	dev->data->tx_queues[queue_idx] = txvq;
> > > >  	return 0;
> > > >  }
> > > >
> > > > --
> > > > 2.11.0
> > >

Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup consistency

[Wang, Zhihong]

Hi Olivier,

Given the situation that the vec path can be selected silently now once
condition is met. So theoretically speaking this issue impacts the whole
virtio pmd. If you plan to fix it in the next release, do you want to do
a temporary workaround to disable the vec path selection till then?

Thanks
-Zhihong

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Thursday, February 8, 2018 6:01 AM
> To: Xu, Qian Q <qian.q.xu@intel.com>
> Cc: Yao, Lei A <lei.a.yao@intel.com>; dev@dpdk.org; yliu@fridaylinux.org;
> maxime.coquelin@redhat.com; Thomas Monjalon <thomas@monjalon.net>;
> stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup
> consistency
> 
> Hi,
> 
> It's in my short plans, but unfortunately some other high priority tasks
> were inserted before. Honnestly, I'm not sure I'll be able to make it
> for the release, but I'll do my best.
> 
> Olivier
> 
> 
> 
> On Wed, Feb 07, 2018 at 08:31:07AM +0000, Xu, Qian Q wrote:
> > Any update, Olivier?
> > We are near to release, and the bug-fix is important for the virtio vector
> path usage. Thanks.
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> > > Sent: Thursday, February 1, 2018 4:28 PM
> > > To: Yao, Lei A <lei.a.yao@intel.com>
> > > Cc: dev@dpdk.org; yliu@fridaylinux.org; maxime.coquelin@redhat.com;
> > > Thomas Monjalon <thomas@monjalon.net>; stable@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup
> consistency
> > >
> > > Hi Lei,
> > >
> > > It's on my todo list, I'll check this as soon as possible.
> > >
> > > Olivier
> > >
> > >
> > > On Thu, Feb 01, 2018 at 03:14:15AM +0000, Yao, Lei A wrote:
> > > > Hi, Olivier
> > > >
> > > > This is Lei from DPDK validation team in Intel. During our DPDK
> > > > 18.02-rc1 test, I find the following patch will cause one serious issue
> with virtio
> > > vector path:
> > > > the traffic can't resume after stop/start the virtio device.
> > > >
> > > > The step like following:
> > > > 1. Launch vhost-user port using testpmd at Host 2. Launch VM with
> > > > virtio device, mergeable is off 3. Bind the virtio device to pmd
> > > > driver, launch testpmd, let the tx/rx use vector path
> > > >     virtio_xmit_pkts_simple
> > > >     virtio_recv_pkts_vec
> > > > 4. Send traffic to virtio device from vhost side, then stop the virtio
> > > > device 5. Start the virtio device again After step 5, the traffic
> > > > can't resume.
> > > >
> > > > Could you help check this and give a fix? This issue will impact the
> > > > virtio pmd user experience heavily. By the way, this patch is already
> > > > included into V17.11. Looks like we need give a patch to this LTS version.
> > > Thanks a lot!
> > > >
> > > > BRs
> > > > Lei
> > > > > -----Original Message-----
> > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> > > > > Sent: Thursday, September 7, 2017 8:14 PM
> > > > > To: dev@dpdk.org; yliu@fridaylinux.org;
> maxime.coquelin@redhat.com
> > > > > Cc: stephen@networkplumber.org; stable@dpdk.org
> > > > > Subject: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup
> > > > > consistency
> > > > >
> > > > > In rx/tx queue setup functions, some code is executed only if
> > > > > use_simple_rxtx == 1. The value of this variable can change
> > > > > depending on the offload flags or sse support. If Rx queue setup is
> > > > > called before Tx queue setup, it can result in an invalid configuration:
> > > > >
> > > > > - dev_configure is called: use_simple_rxtx is initialized to 0
> > > > > - rx queue setup is called: queues are initialized without simple path
> > > > >   support
> > > > > - tx queue setup is called: use_simple_rxtx switch to 1, and simple
> > > > >   Rx/Tx handlers are selected
> > > > >
> > > > > Fix this by postponing a part of Rx/Tx queue initialization in
> > > > > dev_start(), as it was the case in the initial implementation.
> > > > >
> > > > > Fixes: 48cec290a3d2 ("net/virtio: move queue configure code to
> > > > > proper
> > > > > place")
> > > > > Cc: stable@dpdk.org
> > > > >
> > > > > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > > > > ---
> > > > >  drivers/net/virtio/virtio_ethdev.c | 13 +++++++++++++
> > > > > drivers/net/virtio/virtio_ethdev.h |  6 ++++++
> > > > >  drivers/net/virtio/virtio_rxtx.c   | 40
> ++++++++++++++++++++++++++++++-
> > > > > -------
> > > > >  3 files changed, 51 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/virtio/virtio_ethdev.c
> > > > > b/drivers/net/virtio/virtio_ethdev.c
> > > > > index 8eee3ff80..c7888f103 100644
> > > > > --- a/drivers/net/virtio/virtio_ethdev.c
> > > > > +++ b/drivers/net/virtio/virtio_ethdev.c
> > > > > @@ -1737,6 +1737,19 @@ virtio_dev_start(struct rte_eth_dev *dev)
> > > > >  	struct virtnet_rx *rxvq;
> > > > >  	struct virtnet_tx *txvq __rte_unused;
> > > > >  	struct virtio_hw *hw = dev->data->dev_private;
> > > > > +	int ret;
> > > > > +
> > > > > +	/* Finish the initialization of the queues */
> > > > > +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > > > +		ret = virtio_dev_rx_queue_setup_finish(dev, i);
> > > > > +		if (ret < 0)
> > > > > +			return ret;
> > > > > +	}
> > > > > +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > > > > +		ret = virtio_dev_tx_queue_setup_finish(dev, i);
> > > > > +		if (ret < 0)
> > > > > +			return ret;
> > > > > +	}
> > > > >
> > > > >  	/* check if lsc interrupt feature is enabled */
> > > > >  	if (dev->data->dev_conf.intr_conf.lsc) { diff --git
> > > > > a/drivers/net/virtio/virtio_ethdev.h
> > > > > b/drivers/net/virtio/virtio_ethdev.h
> > > > > index c3413c6d9..2039bc547 100644
> > > > > --- a/drivers/net/virtio/virtio_ethdev.h
> > > > > +++ b/drivers/net/virtio/virtio_ethdev.h
> > > > > @@ -92,10 +92,16 @@ int  virtio_dev_rx_queue_setup(struct
> > > > > rte_eth_dev *dev, uint16_t rx_queue_id,
> > > > >  		const struct rte_eth_rxconf *rx_conf,
> > > > >  		struct rte_mempool *mb_pool);
> > > > >
> > > > > +int virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev,
> > > > > +				uint16_t rx_queue_id);
> > > > > +
> > > > >  int  virtio_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t
> > > > > tx_queue_id,
> > > > >  		uint16_t nb_tx_desc, unsigned int socket_id,
> > > > >  		const struct rte_eth_txconf *tx_conf);
> > > > >
> > > > > +int virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
> > > > > +				uint16_t tx_queue_id);
> > > > > +
> > > > >  uint16_t virtio_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts,
> > > > >  		uint16_t nb_pkts);
> > > > >
> > > > > diff --git a/drivers/net/virtio/virtio_rxtx.c
> > > > > b/drivers/net/virtio/virtio_rxtx.c
> > > > > index e30377c51..a32e3229f 100644
> > > > > --- a/drivers/net/virtio/virtio_rxtx.c
> > > > > +++ b/drivers/net/virtio/virtio_rxtx.c
> > > > > @@ -421,9 +421,6 @@ virtio_dev_rx_queue_setup(struct
> rte_eth_dev *dev,
> > > > >  	struct virtio_hw *hw = dev->data->dev_private;
> > > > >  	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> > > > >  	struct virtnet_rx *rxvq;
> > > > > -	int error, nbufs;
> > > > > -	struct rte_mbuf *m;
> > > > > -	uint16_t desc_idx;
> > > > >
> > > > >  	PMD_INIT_FUNC_TRACE();
> > > > >
> > > > > @@ -440,10 +437,24 @@ virtio_dev_rx_queue_setup(struct
> rte_eth_dev
> > > > > *dev,
> > > > >  	}
> > > > >  	dev->data->rx_queues[queue_idx] = rxvq;
> > > > >
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +int
> > > > > +virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev,
> uint16_t
> > > > > queue_idx)
> > > > > +{
> > > > > +	uint16_t vtpci_queue_idx = 2 * queue_idx +
> > > > > VTNET_SQ_RQ_QUEUE_IDX;
> > > > > +	struct virtio_hw *hw = dev->data->dev_private;
> > > > > +	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> > > > > +	struct virtnet_rx *rxvq = &vq->rxq;
> > > > > +	struct rte_mbuf *m;
> > > > > +	uint16_t desc_idx;
> > > > > +	int error, nbufs;
> > > > > +
> > > > > +	PMD_INIT_FUNC_TRACE();
> > > > >
> > > > >  	/* Allocate blank mbufs for the each rx descriptor */
> > > > >  	nbufs = 0;
> > > > > -	error = ENOSPC;
> > > > >
> > > > >  	if (hw->use_simple_rxtx) {
> > > > >  		for (desc_idx = 0; desc_idx < vq->vq_nentries; @@ -
> 534,7
> > > +545,6
> > > > > @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
> > > > >  	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> > > > >  	struct virtnet_tx *txvq;
> > > > >  	uint16_t tx_free_thresh;
> > > > > -	uint16_t desc_idx;
> > > > >
> > > > >  	PMD_INIT_FUNC_TRACE();
> > > > >
> > > > > @@ -563,9 +573,24 @@ virtio_dev_tx_queue_setup(struct
> rte_eth_dev
> > > > > *dev,
> > > > >
> > > > >  	vq->vq_free_thresh = tx_free_thresh;
> > > > >
> > > > > -	if (hw->use_simple_rxtx) {
> > > > > -		uint16_t mid_idx  = vq->vq_nentries >> 1;
> > > > > +	dev->data->tx_queues[queue_idx] = txvq;
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +int
> > > > > +virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
> > > > > +				uint16_t queue_idx)
> > > > > +{
> > > > > +	uint8_t vtpci_queue_idx = 2 * queue_idx +
> > > > > VTNET_SQ_TQ_QUEUE_IDX;
> > > > > +	struct virtio_hw *hw = dev->data->dev_private;
> > > > > +	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
> > > > > +	uint16_t mid_idx = vq->vq_nentries >> 1;
> > > > > +	struct virtnet_tx *txvq = &vq->txq;
> > > > > +	uint16_t desc_idx;
> > > > >
> > > > > +	PMD_INIT_FUNC_TRACE();
> > > > > +
> > > > > +	if (hw->use_simple_rxtx) {
> > > > >  		for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
> > > > >  			vq->vq_ring.avail->ring[desc_idx] =
> > > > >  				desc_idx + mid_idx;
> > > > > @@ -587,7 +612,6 @@ virtio_dev_tx_queue_setup(struct
> rte_eth_dev
> > > > > *dev,
> > > > >
> > > > >  	VIRTQUEUE_DUMP(vq);
> > > > >
> > > > > -	dev->data->tx_queues[queue_idx] = txvq;
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > --
> > > > > 2.11.0
> > > >

Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup consistency

[Maxime Coquelin]

Hi Zhihong,

On 02/09/2018 06:44 AM, Wang, Zhihong wrote:
> Hi Olivier,
> 
> Given the situation that the vec path can be selected silently now once
> condition is met. So theoretically speaking this issue impacts the whole
> virtio pmd. If you plan to fix it in the next release, do you want to do
> a temporary workaround to disable the vec path selection till then?

That may be the less worse solution if we don't fix it quickly.
Reverting the patch isn't trivial, so this is not an option.

I'm trying to reproduce it now, I'll let you know if I find something.

Cheers,
Maxime
> Thanks
> -Zhihong
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
>> Sent: Thursday, February 8, 2018 6:01 AM
>> To: Xu, Qian Q <qian.q.xu@intel.com>
>> Cc: Yao, Lei A <lei.a.yao@intel.com>; dev@dpdk.org; yliu@fridaylinux.org;
>> maxime.coquelin@redhat.com; Thomas Monjalon <thomas@monjalon.net>;
>> stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup
>> consistency
>>
>> Hi,
>>
>> It's in my short plans, but unfortunately some other high priority tasks
>> were inserted before. Honnestly, I'm not sure I'll be able to make it
>> for the release, but I'll do my best.
>>
>> Olivier
>>
>>
>>
>> On Wed, Feb 07, 2018 at 08:31:07AM +0000, Xu, Qian Q wrote:
>>> Any update, Olivier?
>>> We are near to release, and the bug-fix is important for the virtio vector
>> path usage. Thanks.
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
>>>> Sent: Thursday, February 1, 2018 4:28 PM
>>>> To: Yao, Lei A <lei.a.yao@intel.com>
>>>> Cc: dev@dpdk.org; yliu@fridaylinux.org; maxime.coquelin@redhat.com;
>>>> Thomas Monjalon <thomas@monjalon.net>; stable@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup
>> consistency
>>>>
>>>> Hi Lei,
>>>>
>>>> It's on my todo list, I'll check this as soon as possible.
>>>>
>>>> Olivier
>>>>
>>>>
>>>> On Thu, Feb 01, 2018 at 03:14:15AM +0000, Yao, Lei A wrote:
>>>>> Hi, Olivier
>>>>>
>>>>> This is Lei from DPDK validation team in Intel. During our DPDK
>>>>> 18.02-rc1 test, I find the following patch will cause one serious issue
>> with virtio
>>>> vector path:
>>>>> the traffic can't resume after stop/start the virtio device.
>>>>>
>>>>> The step like following:
>>>>> 1. Launch vhost-user port using testpmd at Host 2. Launch VM with
>>>>> virtio device, mergeable is off 3. Bind the virtio device to pmd
>>>>> driver, launch testpmd, let the tx/rx use vector path
>>>>>      virtio_xmit_pkts_simple
>>>>>      virtio_recv_pkts_vec
>>>>> 4. Send traffic to virtio device from vhost side, then stop the virtio
>>>>> device 5. Start the virtio device again After step 5, the traffic
>>>>> can't resume.
>>>>>
>>>>> Could you help check this and give a fix? This issue will impact the
>>>>> virtio pmd user experience heavily. By the way, this patch is already
>>>>> included into V17.11. Looks like we need give a patch to this LTS version.
>>>> Thanks a lot!
>>>>>
>>>>> BRs
>>>>> Lei
>>>>>> -----Original Message-----
>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
>>>>>> Sent: Thursday, September 7, 2017 8:14 PM
>>>>>> To: dev@dpdk.org; yliu@fridaylinux.org;
>> maxime.coquelin@redhat.com
>>>>>> Cc: stephen@networkplumber.org; stable@dpdk.org
>>>>>> Subject: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup
>>>>>> consistency
>>>>>>
>>>>>> In rx/tx queue setup functions, some code is executed only if
>>>>>> use_simple_rxtx == 1. The value of this variable can change
>>>>>> depending on the offload flags or sse support. If Rx queue setup is
>>>>>> called before Tx queue setup, it can result in an invalid configuration:
>>>>>>
>>>>>> - dev_configure is called: use_simple_rxtx is initialized to 0
>>>>>> - rx queue setup is called: queues are initialized without simple path
>>>>>>    support
>>>>>> - tx queue setup is called: use_simple_rxtx switch to 1, and simple
>>>>>>    Rx/Tx handlers are selected
>>>>>>
>>>>>> Fix this by postponing a part of Rx/Tx queue initialization in
>>>>>> dev_start(), as it was the case in the initial implementation.
>>>>>>
>>>>>> Fixes: 48cec290a3d2 ("net/virtio: move queue configure code to
>>>>>> proper
>>>>>> place")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>>>>>> ---
>>>>>>   drivers/net/virtio/virtio_ethdev.c | 13 +++++++++++++
>>>>>> drivers/net/virtio/virtio_ethdev.h |  6 ++++++
>>>>>>   drivers/net/virtio/virtio_rxtx.c   | 40
>> ++++++++++++++++++++++++++++++-
>>>>>> -------
>>>>>>   3 files changed, 51 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/virtio/virtio_ethdev.c
>>>>>> b/drivers/net/virtio/virtio_ethdev.c
>>>>>> index 8eee3ff80..c7888f103 100644
>>>>>> --- a/drivers/net/virtio/virtio_ethdev.c
>>>>>> +++ b/drivers/net/virtio/virtio_ethdev.c
>>>>>> @@ -1737,6 +1737,19 @@ virtio_dev_start(struct rte_eth_dev *dev)
>>>>>>   	struct virtnet_rx *rxvq;
>>>>>>   	struct virtnet_tx *txvq __rte_unused;
>>>>>>   	struct virtio_hw *hw = dev->data->dev_private;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	/* Finish the initialization of the queues */
>>>>>> +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
>>>>>> +		ret = virtio_dev_rx_queue_setup_finish(dev, i);
>>>>>> +		if (ret < 0)
>>>>>> +			return ret;
>>>>>> +	}
>>>>>> +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
>>>>>> +		ret = virtio_dev_tx_queue_setup_finish(dev, i);
>>>>>> +		if (ret < 0)
>>>>>> +			return ret;
>>>>>> +	}
>>>>>>
>>>>>>   	/* check if lsc interrupt feature is enabled */
>>>>>>   	if (dev->data->dev_conf.intr_conf.lsc) { diff --git
>>>>>> a/drivers/net/virtio/virtio_ethdev.h
>>>>>> b/drivers/net/virtio/virtio_ethdev.h
>>>>>> index c3413c6d9..2039bc547 100644
>>>>>> --- a/drivers/net/virtio/virtio_ethdev.h
>>>>>> +++ b/drivers/net/virtio/virtio_ethdev.h
>>>>>> @@ -92,10 +92,16 @@ int  virtio_dev_rx_queue_setup(struct
>>>>>> rte_eth_dev *dev, uint16_t rx_queue_id,
>>>>>>   		const struct rte_eth_rxconf *rx_conf,
>>>>>>   		struct rte_mempool *mb_pool);
>>>>>>
>>>>>> +int virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev,
>>>>>> +				uint16_t rx_queue_id);
>>>>>> +
>>>>>>   int  virtio_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t
>>>>>> tx_queue_id,
>>>>>>   		uint16_t nb_tx_desc, unsigned int socket_id,
>>>>>>   		const struct rte_eth_txconf *tx_conf);
>>>>>>
>>>>>> +int virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
>>>>>> +				uint16_t tx_queue_id);
>>>>>> +
>>>>>>   uint16_t virtio_recv_pkts(void *rx_queue, struct rte_mbuf
>> **rx_pkts,
>>>>>>   		uint16_t nb_pkts);
>>>>>>
>>>>>> diff --git a/drivers/net/virtio/virtio_rxtx.c
>>>>>> b/drivers/net/virtio/virtio_rxtx.c
>>>>>> index e30377c51..a32e3229f 100644
>>>>>> --- a/drivers/net/virtio/virtio_rxtx.c
>>>>>> +++ b/drivers/net/virtio/virtio_rxtx.c
>>>>>> @@ -421,9 +421,6 @@ virtio_dev_rx_queue_setup(struct
>> rte_eth_dev *dev,
>>>>>>   	struct virtio_hw *hw = dev->data->dev_private;
>>>>>>   	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
>>>>>>   	struct virtnet_rx *rxvq;
>>>>>> -	int error, nbufs;
>>>>>> -	struct rte_mbuf *m;
>>>>>> -	uint16_t desc_idx;
>>>>>>
>>>>>>   	PMD_INIT_FUNC_TRACE();
>>>>>>
>>>>>> @@ -440,10 +437,24 @@ virtio_dev_rx_queue_setup(struct
>> rte_eth_dev
>>>>>> *dev,
>>>>>>   	}
>>>>>>   	dev->data->rx_queues[queue_idx] = rxvq;
>>>>>>
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +int
>>>>>> +virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev,
>> uint16_t
>>>>>> queue_idx)
>>>>>> +{
>>>>>> +	uint16_t vtpci_queue_idx = 2 * queue_idx +
>>>>>> VTNET_SQ_RQ_QUEUE_IDX;
>>>>>> +	struct virtio_hw *hw = dev->data->dev_private;
>>>>>> +	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
>>>>>> +	struct virtnet_rx *rxvq = &vq->rxq;
>>>>>> +	struct rte_mbuf *m;
>>>>>> +	uint16_t desc_idx;
>>>>>> +	int error, nbufs;
>>>>>> +
>>>>>> +	PMD_INIT_FUNC_TRACE();
>>>>>>
>>>>>>   	/* Allocate blank mbufs for the each rx descriptor */
>>>>>>   	nbufs = 0;
>>>>>> -	error = ENOSPC;
>>>>>>
>>>>>>   	if (hw->use_simple_rxtx) {
>>>>>>   		for (desc_idx = 0; desc_idx < vq->vq_nentries; @@ -
>> 534,7
>>>> +545,6
>>>>>> @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
>>>>>>   	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
>>>>>>   	struct virtnet_tx *txvq;
>>>>>>   	uint16_t tx_free_thresh;
>>>>>> -	uint16_t desc_idx;
>>>>>>
>>>>>>   	PMD_INIT_FUNC_TRACE();
>>>>>>
>>>>>> @@ -563,9 +573,24 @@ virtio_dev_tx_queue_setup(struct
>> rte_eth_dev
>>>>>> *dev,
>>>>>>
>>>>>>   	vq->vq_free_thresh = tx_free_thresh;
>>>>>>
>>>>>> -	if (hw->use_simple_rxtx) {
>>>>>> -		uint16_t mid_idx  = vq->vq_nentries >> 1;
>>>>>> +	dev->data->tx_queues[queue_idx] = txvq;
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +int
>>>>>> +virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
>>>>>> +				uint16_t queue_idx)
>>>>>> +{
>>>>>> +	uint8_t vtpci_queue_idx = 2 * queue_idx +
>>>>>> VTNET_SQ_TQ_QUEUE_IDX;
>>>>>> +	struct virtio_hw *hw = dev->data->dev_private;
>>>>>> +	struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
>>>>>> +	uint16_t mid_idx = vq->vq_nentries >> 1;
>>>>>> +	struct virtnet_tx *txvq = &vq->txq;
>>>>>> +	uint16_t desc_idx;
>>>>>>
>>>>>> +	PMD_INIT_FUNC_TRACE();
>>>>>> +
>>>>>> +	if (hw->use_simple_rxtx) {
>>>>>>   		for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
>>>>>>   			vq->vq_ring.avail->ring[desc_idx] =
>>>>>>   				desc_idx + mid_idx;
>>>>>> @@ -587,7 +612,6 @@ virtio_dev_tx_queue_setup(struct
>> rte_eth_dev
>>>>>> *dev,
>>>>>>
>>>>>>   	VIRTQUEUE_DUMP(vq);
>>>>>>
>>>>>> -	dev->data->tx_queues[queue_idx] = txvq;
>>>>>>   	return 0;
>>>>>>   }
>>>>>>
>>>>>> --
>>>>>> 2.11.0
>>>>>

Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup consistency

[Maxime Coquelin]

On 02/09/2018 09:59 AM, Maxime Coquelin wrote:
> Hi Zhihong,
> 
> On 02/09/2018 06:44 AM, Wang, Zhihong wrote:
>> Hi Olivier,
>>
>> Given the situation that the vec path can be selected silently now once
>> condition is met. So theoretically speaking this issue impacts the whole
>> virtio pmd. If you plan to fix it in the next release, do you want to do
>> a temporary workaround to disable the vec path selection till then?
> 
> That may be the less worse solution if we don't fix it quickly.
> Reverting the patch isn't trivial, so this is not an option.
> 
> I'm trying to reproduce it now, I'll let you know if I find something.

Hmm, I reproduced Tiwei instructions, and in my case, Vhost's testpmd
crashes because Virtio-user makes it doing an out of bound access.

Could you provide a patch to disable vector path selection? I'll
continue to debug, but we can start reviewing it so that it is ready if
we need it.

Thanks,
Maxime

> 
> Cheers,
> Maxime
>> Thanks
>> -Zhihong
>>
>>> -----Original Message-----
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
>>> Sent: Thursday, February 8, 2018 6:01 AM
>>> To: Xu, Qian Q <qian.q.xu@intel.com>
>>> Cc: Yao, Lei A <lei.a.yao@intel.com>; dev@dpdk.org; 
>>> yliu@fridaylinux.org;
>>> maxime.coquelin@redhat.com; Thomas Monjalon <thomas@monjalon.net>;
>>> stable@dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup
>>> consistency
>>>
>>> Hi,
>>>
>>> It's in my short plans, but unfortunately some other high priority tasks
>>> were inserted before. Honnestly, I'm not sure I'll be able to make it
>>> for the release, but I'll do my best.
>>>
>>> Olivier
>>>
>>>
>>>
>>> On Wed, Feb 07, 2018 at 08:31:07AM +0000, Xu, Qian Q wrote:
>>>> Any update, Olivier?
>>>> We are near to release, and the bug-fix is important for the virtio 
>>>> vector
>>> path usage. Thanks.
>>>>
>>>>> -----Original Message-----
>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
>>>>> Sent: Thursday, February 1, 2018 4:28 PM
>>>>> To: Yao, Lei A <lei.a.yao@intel.com>
>>>>> Cc: dev@dpdk.org; yliu@fridaylinux.org; maxime.coquelin@redhat.com;
>>>>> Thomas Monjalon <thomas@monjalon.net>; stable@dpdk.org
>>>>> Subject: Re: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup
>>> consistency
>>>>>
>>>>> Hi Lei,
>>>>>
>>>>> It's on my todo list, I'll check this as soon as possible.
>>>>>
>>>>> Olivier
>>>>>
>>>>>
>>>>> On Thu, Feb 01, 2018 at 03:14:15AM +0000, Yao, Lei A wrote:
>>>>>> Hi, Olivier
>>>>>>
>>>>>> This is Lei from DPDK validation team in Intel. During our DPDK
>>>>>> 18.02-rc1 test, I find the following patch will cause one serious 
>>>>>> issue
>>> with virtio
>>>>> vector path:
>>>>>> the traffic can't resume after stop/start the virtio device.
>>>>>>
>>>>>> The step like following:
>>>>>> 1. Launch vhost-user port using testpmd at Host 2. Launch VM with
>>>>>> virtio device, mergeable is off 3. Bind the virtio device to pmd
>>>>>> driver, launch testpmd, let the tx/rx use vector path
>>>>>>      virtio_xmit_pkts_simple
>>>>>>      virtio_recv_pkts_vec
>>>>>> 4. Send traffic to virtio device from vhost side, then stop the 
>>>>>> virtio
>>>>>> device 5. Start the virtio device again After step 5, the traffic
>>>>>> can't resume.
>>>>>>
>>>>>> Could you help check this and give a fix? This issue will impact the
>>>>>> virtio pmd user experience heavily. By the way, this patch is already
>>>>>> included into V17.11. Looks like we need give a patch to this LTS 
>>>>>> version.
>>>>> Thanks a lot!
>>>>>>
>>>>>> BRs
>>>>>> Lei
>>>>>>> -----Original Message-----
>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
>>>>>>> Sent: Thursday, September 7, 2017 8:14 PM
>>>>>>> To: dev@dpdk.org; yliu@fridaylinux.org;
>>> maxime.coquelin@redhat.com
>>>>>>> Cc: stephen@networkplumber.org; stable@dpdk.org
>>>>>>> Subject: [dpdk-dev] [PATCH v2 06/10] net/virtio: fix queue setup
>>>>>>> consistency
>>>>>>>
>>>>>>> In rx/tx queue setup functions, some code is executed only if
>>>>>>> use_simple_rxtx == 1. The value of this variable can change
>>>>>>> depending on the offload flags or sse support. If Rx queue setup is
>>>>>>> called before Tx queue setup, it can result in an invalid 
>>>>>>> configuration:
>>>>>>>
>>>>>>> - dev_configure is called: use_simple_rxtx is initialized to 0
>>>>>>> - rx queue setup is called: queues are initialized without simple 
>>>>>>> path
>>>>>>>    support
>>>>>>> - tx queue setup is called: use_simple_rxtx switch to 1, and simple
>>>>>>>    Rx/Tx handlers are selected
>>>>>>>
>>>>>>> Fix this by postponing a part of Rx/Tx queue initialization in
>>>>>>> dev_start(), as it was the case in the initial implementation.
>>>>>>>
>>>>>>> Fixes: 48cec290a3d2 ("net/virtio: move queue configure code to
>>>>>>> proper
>>>>>>> place")
>>>>>>> Cc: stable@dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>>>>>>> ---
>>>>>>>   drivers/net/virtio/virtio_ethdev.c | 13 +++++++++++++
>>>>>>> drivers/net/virtio/virtio_ethdev.h |  6 ++++++
>>>>>>>   drivers/net/virtio/virtio_rxtx.c   | 40
>>> ++++++++++++++++++++++++++++++-
>>>>>>> -------
>>>>>>>   3 files changed, 51 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/virtio/virtio_ethdev.c
>>>>>>> b/drivers/net/virtio/virtio_ethdev.c
>>>>>>> index 8eee3ff80..c7888f103 100644
>>>>>>> --- a/drivers/net/virtio/virtio_ethdev.c
>>>>>>> +++ b/drivers/net/virtio/virtio_ethdev.c
>>>>>>> @@ -1737,6 +1737,19 @@ virtio_dev_start(struct rte_eth_dev *dev)
>>>>>>>       struct virtnet_rx *rxvq;
>>>>>>>       struct virtnet_tx *txvq __rte_unused;
>>>>>>>       struct virtio_hw *hw = dev->data->dev_private;
>>>>>>> +    int ret;
>>>>>>> +
>>>>>>> +    /* Finish the initialization of the queues */
>>>>>>> +    for (i = 0; i < dev->data->nb_rx_queues; i++) {
>>>>>>> +        ret = virtio_dev_rx_queue_setup_finish(dev, i);
>>>>>>> +        if (ret < 0)
>>>>>>> +            return ret;
>>>>>>> +    }
>>>>>>> +    for (i = 0; i < dev->data->nb_tx_queues; i++) {
>>>>>>> +        ret = virtio_dev_tx_queue_setup_finish(dev, i);
>>>>>>> +        if (ret < 0)
>>>>>>> +            return ret;
>>>>>>> +    }
>>>>>>>
>>>>>>>       /* check if lsc interrupt feature is enabled */
>>>>>>>       if (dev->data->dev_conf.intr_conf.lsc) { diff --git
>>>>>>> a/drivers/net/virtio/virtio_ethdev.h
>>>>>>> b/drivers/net/virtio/virtio_ethdev.h
>>>>>>> index c3413c6d9..2039bc547 100644
>>>>>>> --- a/drivers/net/virtio/virtio_ethdev.h
>>>>>>> +++ b/drivers/net/virtio/virtio_ethdev.h
>>>>>>> @@ -92,10 +92,16 @@ int  virtio_dev_rx_queue_setup(struct
>>>>>>> rte_eth_dev *dev, uint16_t rx_queue_id,
>>>>>>>           const struct rte_eth_rxconf *rx_conf,
>>>>>>>           struct rte_mempool *mb_pool);
>>>>>>>
>>>>>>> +int virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev,
>>>>>>> +                uint16_t rx_queue_id);
>>>>>>> +
>>>>>>>   int  virtio_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t
>>>>>>> tx_queue_id,
>>>>>>>           uint16_t nb_tx_desc, unsigned int socket_id,
>>>>>>>           const struct rte_eth_txconf *tx_conf);
>>>>>>>
>>>>>>> +int virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
>>>>>>> +                uint16_t tx_queue_id);
>>>>>>> +
>>>>>>>   uint16_t virtio_recv_pkts(void *rx_queue, struct rte_mbuf
>>> **rx_pkts,
>>>>>>>           uint16_t nb_pkts);
>>>>>>>
>>>>>>> diff --git a/drivers/net/virtio/virtio_rxtx.c
>>>>>>> b/drivers/net/virtio/virtio_rxtx.c
>>>>>>> index e30377c51..a32e3229f 100644
>>>>>>> --- a/drivers/net/virtio/virtio_rxtx.c
>>>>>>> +++ b/drivers/net/virtio/virtio_rxtx.c
>>>>>>> @@ -421,9 +421,6 @@ virtio_dev_rx_queue_setup(struct
>>> rte_eth_dev *dev,
>>>>>>>       struct virtio_hw *hw = dev->data->dev_private;
>>>>>>>       struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
>>>>>>>       struct virtnet_rx *rxvq;
>>>>>>> -    int error, nbufs;
>>>>>>> -    struct rte_mbuf *m;
>>>>>>> -    uint16_t desc_idx;
>>>>>>>
>>>>>>>       PMD_INIT_FUNC_TRACE();
>>>>>>>
>>>>>>> @@ -440,10 +437,24 @@ virtio_dev_rx_queue_setup(struct
>>> rte_eth_dev
>>>>>>> *dev,
>>>>>>>       }
>>>>>>>       dev->data->rx_queues[queue_idx] = rxvq;
>>>>>>>
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +int
>>>>>>> +virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev,
>>> uint16_t
>>>>>>> queue_idx)
>>>>>>> +{
>>>>>>> +    uint16_t vtpci_queue_idx = 2 * queue_idx +
>>>>>>> VTNET_SQ_RQ_QUEUE_IDX;
>>>>>>> +    struct virtio_hw *hw = dev->data->dev_private;
>>>>>>> +    struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
>>>>>>> +    struct virtnet_rx *rxvq = &vq->rxq;
>>>>>>> +    struct rte_mbuf *m;
>>>>>>> +    uint16_t desc_idx;
>>>>>>> +    int error, nbufs;
>>>>>>> +
>>>>>>> +    PMD_INIT_FUNC_TRACE();
>>>>>>>
>>>>>>>       /* Allocate blank mbufs for the each rx descriptor */
>>>>>>>       nbufs = 0;
>>>>>>> -    error = ENOSPC;
>>>>>>>
>>>>>>>       if (hw->use_simple_rxtx) {
>>>>>>>           for (desc_idx = 0; desc_idx < vq->vq_nentries; @@ -
>>> 534,7
>>>>> +545,6
>>>>>>> @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
>>>>>>>       struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
>>>>>>>       struct virtnet_tx *txvq;
>>>>>>>       uint16_t tx_free_thresh;
>>>>>>> -    uint16_t desc_idx;
>>>>>>>
>>>>>>>       PMD_INIT_FUNC_TRACE();
>>>>>>>
>>>>>>> @@ -563,9 +573,24 @@ virtio_dev_tx_queue_setup(struct
>>> rte_eth_dev
>>>>>>> *dev,
>>>>>>>
>>>>>>>       vq->vq_free_thresh = tx_free_thresh;
>>>>>>>
>>>>>>> -    if (hw->use_simple_rxtx) {
>>>>>>> -        uint16_t mid_idx  = vq->vq_nentries >> 1;
>>>>>>> +    dev->data->tx_queues[queue_idx] = txvq;
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +int
>>>>>>> +virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
>>>>>>> +                uint16_t queue_idx)
>>>>>>> +{
>>>>>>> +    uint8_t vtpci_queue_idx = 2 * queue_idx +
>>>>>>> VTNET_SQ_TQ_QUEUE_IDX;
>>>>>>> +    struct virtio_hw *hw = dev->data->dev_private;
>>>>>>> +    struct virtqueue *vq = hw->vqs[vtpci_queue_idx];
>>>>>>> +    uint16_t mid_idx = vq->vq_nentries >> 1;
>>>>>>> +    struct virtnet_tx *txvq = &vq->txq;
>>>>>>> +    uint16_t desc_idx;
>>>>>>>
>>>>>>> +    PMD_INIT_FUNC_TRACE();
>>>>>>> +
>>>>>>> +    if (hw->use_simple_rxtx) {
>>>>>>>           for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
>>>>>>>               vq->vq_ring.avail->ring[desc_idx] =
>>>>>>>                   desc_idx + mid_idx;
>>>>>>> @@ -587,7 +612,6 @@ virtio_dev_tx_queue_setup(struct
>>> rte_eth_dev
>>>>>>> *dev,
>>>>>>>
>>>>>>>       VIRTQUEUE_DUMP(vq);
>>>>>>>
>>>>>>> -    dev->data->tx_queues[queue_idx] = txvq;
>>>>>>>       return 0;
>>>>>>>   }
>>>>>>>
>>>>>>> -- 
>>>>>>> 2.11.0
>>>>>>