* [PATCH v2 01/10] net/virtio: revert "do not claim to support LRO"
2017-09-07 12:13 ` [PATCH v2 00/10] virtio fixes Olivier Matz
@ 2017-09-07 12:13 ` Olivier Matz
2017-09-07 12:13 ` [PATCH v2 02/10] net/virtio: revert "do not falsely claim to do IP checksum" Olivier Matz
` (9 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Olivier Matz @ 2017-09-07 12:13 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen, stable
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v2 02/10] net/virtio: revert "do not falsely claim to do IP checksum"
2017-09-07 12:13 ` [PATCH v2 00/10] virtio fixes Olivier Matz
2017-09-07 12:13 ` [PATCH v2 01/10] net/virtio: revert "do not claim to support LRO" Olivier Matz
@ 2017-09-07 12:13 ` Olivier Matz
2017-09-07 12:13 ` [PATCH v2 03/10] doc: fix description of L4 Rx checksum offload Olivier Matz
` (8 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Olivier Matz @ 2017-09-07 12:13 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen, stable
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v2 03/10] doc: fix description of L4 Rx checksum offload
2017-09-07 12:13 ` [PATCH v2 00/10] virtio fixes Olivier Matz
2017-09-07 12:13 ` [PATCH v2 01/10] net/virtio: revert "do not claim to support LRO" Olivier Matz
2017-09-07 12:13 ` [PATCH v2 02/10] net/virtio: revert "do not falsely claim to do IP checksum" Olivier Matz
@ 2017-09-07 12:13 ` Olivier Matz
2017-09-07 12:13 ` [PATCH v2 04/10] net/virtio: fix log levels in configure Olivier Matz
` (7 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Olivier Matz @ 2017-09-07 12:13 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen, stable
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v2 04/10] net/virtio: fix log levels in configure
2017-09-07 12:13 ` [PATCH v2 00/10] virtio fixes Olivier Matz
` (2 preceding siblings ...)
2017-09-07 12:13 ` [PATCH v2 03/10] doc: fix description of L4 Rx checksum offload Olivier Matz
@ 2017-09-07 12:13 ` Olivier Matz
2017-09-07 12:13 ` [PATCH v2 05/10] net/virtio: fix mbuf port for simple Rx function Olivier Matz
` (6 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Olivier Matz @ 2017-09-07 12:13 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen, stable
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v2 05/10] net/virtio: fix mbuf port for simple Rx function
2017-09-07 12:13 ` [PATCH v2 00/10] virtio fixes Olivier Matz
` (3 preceding siblings ...)
2017-09-07 12:13 ` [PATCH v2 04/10] net/virtio: fix log levels in configure Olivier Matz
@ 2017-09-07 12:13 ` Olivier Matz
2017-09-07 12:13 ` [PATCH v2 06/10] net/virtio: fix queue setup consistency Olivier Matz
` (5 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Olivier Matz @ 2017-09-07 12:13 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen, stable
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v2 06/10] net/virtio: fix queue setup consistency
2017-09-07 12:13 ` [PATCH v2 00/10] virtio fixes Olivier Matz
` (4 preceding siblings ...)
2017-09-07 12:13 ` [PATCH v2 05/10] net/virtio: fix mbuf port for simple Rx function Olivier Matz
@ 2017-09-07 12:13 ` Olivier Matz
2017-12-06 5:25 ` Tiwei Bie
2018-02-01 3:14 ` Yao, Lei A
2017-09-07 12:13 ` [PATCH v2 07/10] net/virtio: rationalize setting of Rx/Tx handlers Olivier Matz
` (4 subsequent siblings)
10 siblings, 2 replies; 45+ messages in thread
From: Olivier Matz @ 2017-09-07 12:13 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen, stable
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 06/10] net/virtio: fix queue setup consistency
2017-09-07 12:13 ` [PATCH v2 06/10] net/virtio: fix queue setup consistency Olivier Matz
@ 2017-12-06 5:25 ` Tiwei Bie
2017-12-07 14:14 ` Olivier MATZ
2018-02-01 3:14 ` Yao, Lei A
1 sibling, 1 reply; 45+ messages in thread
From: Tiwei Bie @ 2017-12-06 5:25 UTC (permalink / raw)
To: Olivier Matz, maxime.coquelin
Cc: yliu, stephen, dev, stable, antonio.fischetti
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 06/10] net/virtio: fix queue setup consistency
2017-12-06 5:25 ` Tiwei Bie
@ 2017-12-07 14:14 ` Olivier MATZ
2017-12-08 2:17 ` Tiwei Bie
0 siblings, 1 reply; 45+ messages in thread
From: Olivier MATZ @ 2017-12-07 14:14 UTC (permalink / raw)
To: Tiwei Bie; +Cc: maxime.coquelin, yliu, stephen, dev, stable, antonio.fischetti
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
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 06/10] net/virtio: fix queue setup consistency
2017-12-07 14:14 ` Olivier MATZ
@ 2017-12-08 2:17 ` Tiwei Bie
0 siblings, 0 replies; 45+ messages in thread
From: Tiwei Bie @ 2017-12-08 2:17 UTC (permalink / raw)
To: Olivier MATZ
Cc: maxime.coquelin, yliu, stephen, dev, stable, antonio.fischetti
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 06/10] net/virtio: fix queue setup consistency
2017-09-07 12:13 ` [PATCH v2 06/10] net/virtio: fix queue setup consistency Olivier Matz
2017-12-06 5:25 ` Tiwei Bie
@ 2018-02-01 3:14 ` Yao, Lei A
2018-02-01 8:27 ` Olivier Matz
1 sibling, 1 reply; 45+ messages in thread
From: Yao, Lei A @ 2018-02-01 3:14 UTC (permalink / raw)
To: Olivier Matz, dev, yliu, maxime.coquelin, Thomas Monjalon; +Cc: stable
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
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 06/10] net/virtio: fix queue setup consistency
2018-02-01 3:14 ` Yao, Lei A
@ 2018-02-01 8:27 ` Olivier Matz
2018-02-07 8:31 ` Xu, Qian Q
0 siblings, 1 reply; 45+ messages in thread
From: Olivier Matz @ 2018-02-01 8:27 UTC (permalink / raw)
To: Yao, Lei A; +Cc: dev, yliu, maxime.coquelin, Thomas Monjalon, stable
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
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 06/10] net/virtio: fix queue setup consistency
2018-02-01 8:27 ` Olivier Matz
@ 2018-02-07 8:31 ` Xu, Qian Q
2018-02-07 22:01 ` Olivier Matz
0 siblings, 1 reply; 45+ messages in thread
From: Xu, Qian Q @ 2018-02-07 8:31 UTC (permalink / raw)
To: Olivier Matz, Yao, Lei A
Cc: dev, yliu, maxime.coquelin, Thomas Monjalon, stable
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
> >
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 06/10] net/virtio: fix queue setup consistency
2018-02-07 8:31 ` Xu, Qian Q
@ 2018-02-07 22:01 ` Olivier Matz
2018-02-09 5:44 ` Wang, Zhihong
0 siblings, 1 reply; 45+ messages in thread
From: Olivier Matz @ 2018-02-07 22:01 UTC (permalink / raw)
To: Xu, Qian Q
Cc: Yao, Lei A, dev, yliu, maxime.coquelin, Thomas Monjalon, stable
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
> > >
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 06/10] net/virtio: fix queue setup consistency
2018-02-07 22:01 ` Olivier Matz
@ 2018-02-09 5:44 ` Wang, Zhihong
2018-02-09 8:59 ` Maxime Coquelin
0 siblings, 1 reply; 45+ messages in thread
From: Wang, Zhihong @ 2018-02-09 5:44 UTC (permalink / raw)
To: Olivier Matz, Xu, Qian Q
Cc: Yao, Lei A, dev, yliu, maxime.coquelin, Thomas Monjalon, stable
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
> > > >
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 06/10] net/virtio: fix queue setup consistency
2018-02-09 5:44 ` Wang, Zhihong
@ 2018-02-09 8:59 ` Maxime Coquelin
2018-02-09 9:40 ` Maxime Coquelin
0 siblings, 1 reply; 45+ messages in thread
From: Maxime Coquelin @ 2018-02-09 8:59 UTC (permalink / raw)
To: Wang, Zhihong, Olivier Matz, Xu, Qian Q
Cc: Yao, Lei A, dev, yliu, Thomas Monjalon, stable
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
>>>>>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 06/10] net/virtio: fix queue setup consistency
2018-02-09 8:59 ` Maxime Coquelin
@ 2018-02-09 9:40 ` Maxime Coquelin
0 siblings, 0 replies; 45+ messages in thread
From: Maxime Coquelin @ 2018-02-09 9:40 UTC (permalink / raw)
To: Wang, Zhihong, Olivier Matz, Xu, Qian Q
Cc: Yao, Lei A, dev, yliu, Thomas Monjalon, stable
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
>>>>>>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 07/10] net/virtio: rationalize setting of Rx/Tx handlers
2017-09-07 12:13 ` [PATCH v2 00/10] virtio fixes Olivier Matz
` (5 preceding siblings ...)
2017-09-07 12:13 ` [PATCH v2 06/10] net/virtio: fix queue setup consistency Olivier Matz
@ 2017-09-07 12:13 ` Olivier Matz
2017-09-07 12:13 ` [PATCH v2 08/10] net/virtio: remove SSE check Olivier Matz
` (3 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Olivier Matz @ 2017-09-07 12:13 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v2 08/10] net/virtio: remove SSE check
2017-09-07 12:13 ` [PATCH v2 00/10] virtio fixes Olivier Matz
` (6 preceding siblings ...)
2017-09-07 12:13 ` [PATCH v2 07/10] net/virtio: rationalize setting of Rx/Tx handlers Olivier Matz
@ 2017-09-07 12:13 ` Olivier Matz
2017-09-07 12:13 ` [PATCH v2 09/10] net/virtio: keep Rx handler whatever the Tx queue config Olivier Matz
` (2 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Olivier Matz @ 2017-09-07 12:13 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v2 09/10] net/virtio: keep Rx handler whatever the Tx queue config
2017-09-07 12:13 ` [PATCH v2 00/10] virtio fixes Olivier Matz
` (7 preceding siblings ...)
2017-09-07 12:13 ` [PATCH v2 08/10] net/virtio: remove SSE check Olivier Matz
@ 2017-09-07 12:13 ` Olivier Matz
2017-09-07 12:13 ` [PATCH v2 10/10] net/virtio: fix Rx handler when checksum is requested Olivier Matz
2017-09-12 2:31 ` [PATCH v2 00/10] virtio fixes Yuanhan Liu
10 siblings, 0 replies; 45+ messages in thread
From: Olivier Matz @ 2017-09-07 12:13 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v2 10/10] net/virtio: fix Rx handler when checksum is requested
2017-09-07 12:13 ` [PATCH v2 00/10] virtio fixes Olivier Matz
` (8 preceding siblings ...)
2017-09-07 12:13 ` [PATCH v2 09/10] net/virtio: keep Rx handler whatever the Tx queue config Olivier Matz
@ 2017-09-07 12:13 ` Olivier Matz
2017-09-12 2:31 ` [PATCH v2 00/10] virtio fixes Yuanhan Liu
10 siblings, 0 replies; 45+ messages in thread
From: Olivier Matz @ 2017-09-07 12:13 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 00/10] virtio fixes
2017-09-07 12:13 ` [PATCH v2 00/10] virtio fixes Olivier Matz
` (9 preceding siblings ...)
2017-09-07 12:13 ` [PATCH v2 10/10] net/virtio: fix Rx handler when checksum is requested Olivier Matz
@ 2017-09-12 2:31 ` Yuanhan Liu
10 siblings, 0 replies; 45+ messages in thread
From: Yuanhan Liu @ 2017-09-12 2:31 UTC (permalink / raw)
To: Olivier Matz; +Cc: dev, maxime.coquelin, stephen
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
^ permalink raw reply [flat|nested] 45+ messages in thread