* [PATCH 0/9] virtio fixes
@ 2017-08-31 13:40 Olivier Matz
2017-08-31 13:40 ` [PATCH 1/9] net/virtio: revert "do not claim to support LRO" Olivier Matz
` (9 more replies)
0 siblings, 10 replies; 45+ messages in thread
From: Olivier Matz @ 2017-08-31 13:40 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen
This patchset addresses several issues related to offload and
Rx/Tx handler selection in virtio PMD.
Olivier Matz (9):
net/virtio: revert "do not claim to support LRO"
net/virtio: revert "do not falsely claim to do IP checksum"
doc: fix description of L4 Rx checksum offload
net/virtio: fix log levels in configure
net/virtio: fix mbuf port for simple Rx function
net/virtio: fix queue setup consistency
net/virtio: rationalize setting of Rx/Tx handlers
net/virtio: keep Rx handler whatever the Tx queue config
net/virtio: fix Rx handler when checksum is requested
doc/guides/nics/features.rst | 1 +
drivers/net/virtio/virtio_ethdev.c | 116 ++++++++++++++++++++++++++------
drivers/net/virtio/virtio_ethdev.h | 6 ++
drivers/net/virtio/virtio_pci.h | 3 +-
drivers/net/virtio/virtio_rxtx.c | 71 +++++++++++--------
drivers/net/virtio/virtio_rxtx_simple.c | 2 +
drivers/net/virtio/virtio_user_ethdev.c | 3 +-
7 files changed, 151 insertions(+), 51 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 1/9] net/virtio: revert "do not claim to support LRO"
2017-08-31 13:40 [PATCH 0/9] virtio fixes Olivier Matz
@ 2017-08-31 13:40 ` Olivier Matz
2017-08-31 13:40 ` [PATCH 2/9] net/virtio: revert "do not falsely claim to do IP checksum" Olivier Matz
` (8 subsequent siblings)
9 siblings, 0 replies; 45+ messages in thread
From: Olivier Matz @ 2017-08-31 13:40 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 2/9] net/virtio: revert "do not falsely claim to do IP checksum"
2017-08-31 13:40 [PATCH 0/9] virtio fixes Olivier Matz
2017-08-31 13:40 ` [PATCH 1/9] net/virtio: revert "do not claim to support LRO" Olivier Matz
@ 2017-08-31 13:40 ` Olivier Matz
2017-08-31 13:47 ` Olivier MATZ
2017-08-31 13:40 ` [PATCH 3/9] doc: fix description of L4 Rx checksum offload Olivier Matz
` (7 subsequent siblings)
9 siblings, 1 reply; 45+ messages in thread
From: Olivier Matz @ 2017-08-31 13:40 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 3/9] doc: fix description of L4 Rx checksum offload
2017-08-31 13:40 [PATCH 0/9] virtio fixes Olivier Matz
2017-08-31 13:40 ` [PATCH 1/9] net/virtio: revert "do not claim to support LRO" Olivier Matz
2017-08-31 13:40 ` [PATCH 2/9] net/virtio: revert "do not falsely claim to do IP checksum" Olivier Matz
@ 2017-08-31 13:40 ` Olivier Matz
2017-08-31 13:40 ` [PATCH 4/9] net/virtio: fix log levels in configure Olivier Matz
` (6 subsequent siblings)
9 siblings, 0 replies; 45+ messages in thread
From: Olivier Matz @ 2017-08-31 13:40 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 4/9] net/virtio: fix log levels in configure
2017-08-31 13:40 [PATCH 0/9] virtio fixes Olivier Matz
` (2 preceding siblings ...)
2017-08-31 13:40 ` [PATCH 3/9] doc: fix description of L4 Rx checksum offload Olivier Matz
@ 2017-08-31 13:40 ` Olivier Matz
2017-08-31 13:40 ` [PATCH 5/9] net/virtio: fix mbuf port for simple Rx function Olivier Matz
` (5 subsequent siblings)
9 siblings, 0 replies; 45+ messages in thread
From: Olivier Matz @ 2017-08-31 13:40 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 5/9] net/virtio: fix mbuf port for simple Rx function
2017-08-31 13:40 [PATCH 0/9] virtio fixes Olivier Matz
` (3 preceding siblings ...)
2017-08-31 13:40 ` [PATCH 4/9] net/virtio: fix log levels in configure Olivier Matz
@ 2017-08-31 13:40 ` Olivier Matz
2017-08-31 13:48 ` Olivier MATZ
2017-08-31 13:40 ` [PATCH 6/9] net/virtio: fix queue setup consistency Olivier Matz
` (4 subsequent siblings)
9 siblings, 1 reply; 45+ messages in thread
From: Olivier Matz @ 2017-08-31 13:40 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 6/9] net/virtio: fix queue setup consistency
2017-08-31 13:40 [PATCH 0/9] virtio fixes Olivier Matz
` (4 preceding siblings ...)
2017-08-31 13:40 ` [PATCH 5/9] net/virtio: fix mbuf port for simple Rx function Olivier Matz
@ 2017-08-31 13:40 ` Olivier Matz
2017-08-31 13:49 ` Olivier MATZ
2017-08-31 13:40 ` [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers Olivier Matz
` (3 subsequent siblings)
9 siblings, 1 reply; 45+ messages in thread
From: Olivier Matz @ 2017-08-31 13:40 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
* [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers
2017-08-31 13:40 [PATCH 0/9] virtio fixes Olivier Matz
` (5 preceding siblings ...)
2017-08-31 13:40 ` [PATCH 6/9] net/virtio: fix queue setup consistency Olivier Matz
@ 2017-08-31 13:40 ` Olivier Matz
2017-09-01 9:19 ` Yuanhan Liu
2017-08-31 13:40 ` [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config Olivier Matz
` (2 subsequent siblings)
9 siblings, 1 reply; 45+ messages in thread
From: Olivier Matz @ 2017-08-31 13:40 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen, stable
The selection of Rx/Tx handlers is done at several places,
group them in one function set_rxtx_funcs().
The update of hw->use_simple_rxtx is also rationalized:
- initialized to 1 (prefer simple path)
- in dev configure or rx/tx queue setup, if something prevents from
using the simple path, change it to 0.
- in dev start, set the handlers according to hw->use_simple_rxtx.
Cc: stable@dpdk.org
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
drivers/net/virtio/virtio_ethdev.c | 52 +++++++++++++++++++++++++++++---------
drivers/net/virtio/virtio_rxtx.c | 23 +++++------------
2 files changed, 46 insertions(+), 29 deletions(-)
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index c7888f103..8dad3095f 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1235,14 +1235,36 @@ virtio_interrupt_handler(void *param)
}
+/* set rx and tx handlers according to what is supported */
static void
-rx_func_get(struct rte_eth_dev *eth_dev)
+set_rxtx_funcs(struct rte_eth_dev *eth_dev)
{
struct virtio_hw *hw = eth_dev->data->dev_private;
- if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
+
+ if (hw->use_simple_rxtx) {
+ PMD_INIT_LOG(INFO, "virtio: using simple Rx path on port %u",
+ eth_dev->data->port_id);
+ eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
+ } else if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
+ PMD_INIT_LOG(INFO,
+ "virtio: using mergeable buffer Rx path on port %u",
+ eth_dev->data->port_id);
eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts;
- else
+ } else {
+ PMD_INIT_LOG(INFO, "virtio: using standard Rx path on port %u",
+ eth_dev->data->port_id);
eth_dev->rx_pkt_burst = &virtio_recv_pkts;
+ }
+
+ if (hw->use_simple_rxtx) {
+ PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
+ eth_dev->data->port_id);
+ eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
+ } else {
+ PMD_INIT_LOG(INFO, "virtio: using standard Tx path on port %u",
+ eth_dev->data->port_id);
+ eth_dev->tx_pkt_burst = virtio_xmit_pkts;
+ }
}
/* Only support 1:1 queue/interrupt mapping so far.
@@ -1367,8 +1389,6 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
else
eth_dev->data->dev_flags &= ~RTE_ETH_DEV_INTR_LSC;
- rx_func_get(eth_dev);
-
/* Setting up rx_header size for the device */
if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF) ||
vtpci_with_feature(hw, VIRTIO_F_VERSION_1))
@@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
eth_dev->dev_ops = &virtio_eth_dev_ops;
- eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
if (!hw->virtio_user_dev) {
@@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
}
virtio_set_vtpci_ops(hw);
- if (hw->use_simple_rxtx) {
- eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
- eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
- } else {
- rx_func_get(eth_dev);
- }
+ set_rxtx_funcs(eth_dev);
+
return 0;
}
@@ -1726,6 +1741,18 @@ virtio_dev_configure(struct rte_eth_dev *dev)
return -EBUSY;
}
+ hw->use_simple_rxtx = 1;
+
+#if defined RTE_ARCH_X86
+ if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
+ hw->use_simple_rxtx = 0;
+#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
+ if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
+ hw->use_simple_rxtx = 0;
+#endif
+ if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
+ hw->use_simple_rxtx = 0;
+
return 0;
}
@@ -1802,6 +1829,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
VIRTQUEUE_DUMP(txvq->vq);
}
+ set_rxtx_funcs(dev);
hw->started = 1;
/* Initialize Link state */
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index a32e3229f..c9d97b643 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -505,25 +505,14 @@ static void
virtio_update_rxtx_handler(struct rte_eth_dev *dev,
const struct rte_eth_txconf *tx_conf)
{
- uint8_t use_simple_rxtx = 0;
struct virtio_hw *hw = dev->data->dev_private;
+ uint8_t use_simple_rxtx = hw->use_simple_rxtx;
-#if defined RTE_ARCH_X86
- if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
- use_simple_rxtx = 1;
-#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
- if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
- use_simple_rxtx = 1;
-#endif
- /* Use simple rx/tx func if single segment and no offloads */
- if (use_simple_rxtx &&
- (tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == VIRTIO_SIMPLE_FLAGS &&
- !vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
- PMD_INIT_LOG(INFO, "Using simple rx/tx path");
- dev->tx_pkt_burst = virtio_xmit_pkts_simple;
- dev->rx_pkt_burst = virtio_recv_pkts_vec;
- hw->use_simple_rxtx = use_simple_rxtx;
- }
+ /* cannot use simple rxtx funcs with multisegs or offloads */
+ if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) != VIRTIO_SIMPLE_FLAGS)
+ use_simple_rxtx = 0;
+
+ hw->use_simple_rxtx = use_simple_rxtx;
}
/*
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config
2017-08-31 13:40 [PATCH 0/9] virtio fixes Olivier Matz
` (6 preceding siblings ...)
2017-08-31 13:40 ` [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers Olivier Matz
@ 2017-08-31 13:40 ` Olivier Matz
2017-08-31 13:50 ` Olivier MATZ
2017-09-01 9:25 ` Yuanhan Liu
2017-08-31 13:40 ` [PATCH 9/9] net/virtio: fix Rx handler when checksum is requested Olivier Matz
2017-09-07 12:13 ` [PATCH v2 00/10] virtio fixes Olivier Matz
9 siblings, 2 replies; 45+ messages in thread
From: Olivier Matz @ 2017-08-31 13:40 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen, stable
Split use_simple_rxtx into use_simple_rx and use_simple_tx,
and ensure that only use_simple_tx is updated when txq flags
forces to use the standard Tx handler.
This change is also useful for next commit (disable simple Rx
path when Rx checksum is requested).
Cc: stable@dpdk.org
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
drivers/net/virtio/virtio_ethdev.c | 25 ++++++++++++++++---------
drivers/net/virtio/virtio_pci.h | 3 ++-
drivers/net/virtio/virtio_rxtx.c | 18 +++++++++---------
drivers/net/virtio/virtio_user_ethdev.c | 3 ++-
4 files changed, 29 insertions(+), 20 deletions(-)
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 8dad3095f..4845d44b0 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1241,7 +1241,7 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
{
struct virtio_hw *hw = eth_dev->data->dev_private;
- if (hw->use_simple_rxtx) {
+ if (hw->use_simple_rx) {
PMD_INIT_LOG(INFO, "virtio: using simple Rx path on port %u",
eth_dev->data->port_id);
eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
@@ -1256,7 +1256,7 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
eth_dev->rx_pkt_burst = &virtio_recv_pkts;
}
- if (hw->use_simple_rxtx) {
+ if (hw->use_simple_tx) {
PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
eth_dev->data->port_id);
eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
@@ -1741,17 +1741,24 @@ virtio_dev_configure(struct rte_eth_dev *dev)
return -EBUSY;
}
- hw->use_simple_rxtx = 1;
+ hw->use_simple_rx = 1;
+ hw->use_simple_tx = 1;
#if defined RTE_ARCH_X86
- if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
- hw->use_simple_rxtx = 0;
+ if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3)) {
+ hw->use_simple_rx = 0;
+ hw->use_simple_tx = 0;
+ }
#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
- if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
- hw->use_simple_rxtx = 0;
+ if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON)) {
+ hw->use_simple_rx = 0;
+ hw->use_simple_tx = 0;
+ }
#endif
- if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
- hw->use_simple_rxtx = 0;
+ if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
+ hw->use_simple_rx = 0;
+ hw->use_simple_tx = 0;
+ }
return 0;
}
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 18caebdd7..2ff526c88 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -259,7 +259,8 @@ struct virtio_hw {
uint8_t vlan_strip;
uint8_t use_msix;
uint8_t modern;
- uint8_t use_simple_rxtx;
+ uint8_t use_simple_rx;
+ uint8_t use_simple_tx;
uint8_t port_id;
uint8_t mac_addr[ETHER_ADDR_LEN];
uint32_t notify_off_multiplier;
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index c9d97b643..b81ba0d4a 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -456,7 +456,7 @@ virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t queue_idx)
/* Allocate blank mbufs for the each rx descriptor */
nbufs = 0;
- if (hw->use_simple_rxtx) {
+ if (hw->use_simple_rx) {
for (desc_idx = 0; desc_idx < vq->vq_nentries;
desc_idx++) {
vq->vq_ring.avail->ring[desc_idx] = desc_idx;
@@ -478,7 +478,7 @@ virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t queue_idx)
break;
/* Enqueue allocated buffers */
- if (hw->use_simple_rxtx)
+ if (hw->use_simple_rx)
error = virtqueue_enqueue_recv_refill_simple(vq, m);
else
error = virtqueue_enqueue_recv_refill(vq, m);
@@ -502,17 +502,17 @@ virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t queue_idx)
}
static void
-virtio_update_rxtx_handler(struct rte_eth_dev *dev,
+virtio_update_tx_handler(struct rte_eth_dev *dev,
const struct rte_eth_txconf *tx_conf)
{
struct virtio_hw *hw = dev->data->dev_private;
- uint8_t use_simple_rxtx = hw->use_simple_rxtx;
+ uint8_t use_simple_tx = hw->use_simple_tx;
- /* cannot use simple rxtx funcs with multisegs or offloads */
+ /* use simple tx func if single segment and no offloads */
if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) != VIRTIO_SIMPLE_FLAGS)
- use_simple_rxtx = 0;
+ use_simple_tx = 0;
- hw->use_simple_rxtx = use_simple_rxtx;
+ hw->use_simple_tx = use_simple_tx;
}
/*
@@ -537,7 +537,7 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
PMD_INIT_FUNC_TRACE();
- virtio_update_rxtx_handler(dev, tx_conf);
+ virtio_update_tx_handler(dev, tx_conf);
if (nb_desc == 0 || nb_desc > vq->vq_nentries)
nb_desc = vq->vq_nentries;
@@ -579,7 +579,7 @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
PMD_INIT_FUNC_TRACE();
- if (hw->use_simple_rxtx) {
+ if (hw->use_simple_tx) {
for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
vq->vq_ring.avail->ring[desc_idx] =
desc_idx + mid_idx;
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index c96144434..57c964d6d 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -369,7 +369,8 @@ virtio_user_eth_dev_alloc(struct rte_vdev_device *vdev)
*/
hw->use_msix = 1;
hw->modern = 0;
- hw->use_simple_rxtx = 0;
+ hw->use_simple_rx = 0;
+ hw->use_simple_tx = 0;
hw->virtio_user_dev = dev;
data->dev_flags = RTE_ETH_DEV_DETACHABLE;
return eth_dev;
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 9/9] net/virtio: fix Rx handler when checksum is requested
2017-08-31 13:40 [PATCH 0/9] virtio fixes Olivier Matz
` (7 preceding siblings ...)
2017-08-31 13:40 ` [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config Olivier Matz
@ 2017-08-31 13:40 ` Olivier Matz
2017-08-31 13:51 ` Olivier MATZ
2017-09-07 12:13 ` [PATCH v2 00/10] virtio fixes Olivier Matz
9 siblings, 1 reply; 45+ messages in thread
From: Olivier Matz @ 2017-08-31 13:40 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen, stable
The simple Rx handler is selected even if Rx checksum offload is
requested by the application, but this handler does not support
offloads. This results in broken received packets (no checksum flag but
invalid checksum in the mbuf data).
Disable the simple Rx handler in that case.
Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")
Cc: stable@dpdk.org
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
drivers/net/virtio/virtio_ethdev.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 4845d44b0..0e616bcf0 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1760,6 +1760,9 @@ virtio_dev_configure(struct rte_eth_dev *dev)
hw->use_simple_tx = 0;
}
+ if (rxmode->hw_ip_checksum)
+ hw->use_simple_rx = 0;
+
return 0;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH 2/9] net/virtio: revert "do not falsely claim to do IP checksum"
2017-08-31 13:40 ` [PATCH 2/9] net/virtio: revert "do not falsely claim to do IP checksum" Olivier Matz
@ 2017-08-31 13:47 ` Olivier MATZ
0 siblings, 0 replies; 45+ messages in thread
From: Olivier MATZ @ 2017-08-31 13:47 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen, stable
Validation:
Platform description
--------------------
guest (dpdk)
+----------------+
| |
| |
| port0 |
+----------------+
|
| virtio
|
+----------------+
| tap0 |
| |
| |
+----------------+
host (linux, vhost-net)
Host configuration
------------------
Start qemu with:
- a ne2k management interface to avoi any conflict with dpdk
- a virtio net device, connected to a tap interface through vhost-net
/usr/bin/qemu-system-x86_64 -k fr -daemonize --enable-kvm -m 2G -cpu host \
-smp 3 -serial telnet::40564,server,nowait -serial null \
-qmp tcp::44340,server,nowait -monitor telnet::49229,server,nowait \
-device ne2k_pci,mac=de:ad:de:01:02:03,netdev=user.0,addr=03 \
-netdev user,id=user.0,hostfwd=tcp::34965-:22 \
-netdev type=tap,id=vhostnet0,script=no,vhost=on,queues=8 \
-device virtio-net-pci,netdev=vhostnet0,mq=on,vectors=17 \
-hda "${VM_PATH}/ubuntu-16.04-template.qcow2" \
-snapshot -vga none -display none
Guest configuration
-------------------
Compile dpdk:
cd dpdk.org
make config T=x86_64-native-linuxapp-gcc
sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=y,' build/.config
sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=y,' build/.config
make -j4
Prepare environment:
mkdir -p /mnt/huge
mount -t hugetlbfs nodev /mnt/huge
echo 256 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
modprobe uio_pci_generic
python usertools/dpdk-devbind.py -b uio_pci_generic 0000:00:02.0
./build/app/testpmd -l 0,1 --log-level 8 -- --total-num-mbufs=16384 \
-i --port-topology=chained --disable-hw-vlan-filter \
--disable-hw-vlan-strip --enable-rx-cksum --enable-lro \
--txqflags=0
Before the reverts (patch 1 and 2 of the patchset)
------------------
testpmd cannot start
...
PMD: virtio_dev_configure(): virtio does not support IP checksum
After the reverts
-----------------
testpmd starts properly, and receives packets with csum flags
testpmd> set fwd rxonly
Set rxonly packet forwarding mode
testpmd> set verbose 1
Change verbose level from 0 to 1
testpmd> start
# tcp packet sent from the host
port 0/queue 0: received 1 packets
src=16:9A:CA:76:89:BC - dst=52:54:00:12:34:56 - type=0x0800 - length=74 - nb_segs=1 - hw ptype: L2_ETHER L3_IPV4 L4_TCP - sw ptype: L2_ETHER L3_IPV4 L4_TCP - l2_len=14 - l3_len=20 - l4_len=40 - Receive queue=0x0
ol_flags: PKT_RX_L4_CKSUM_NONE PKT_RX_IP_CKSUM_UNKNOWN
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 5/9] net/virtio: fix mbuf port for simple Rx function
2017-08-31 13:40 ` [PATCH 5/9] net/virtio: fix mbuf port for simple Rx function Olivier Matz
@ 2017-08-31 13:48 ` Olivier MATZ
0 siblings, 0 replies; 45+ messages in thread
From: Olivier MATZ @ 2017-08-31 13:48 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen, stable
Validation:
Platform description
--------------------
guest (dpdk)
+----------------+
| |
| |
| port0 |
+----------------+
|
| virtio
|
+----------------+
| tap0 |
| |
| |
+----------------+
host (linux, vhost-net)
Host configuration
------------------
Start qemu with:
- a ne2k management interface to avoi any conflict with dpdk
- a virtio net device, connected to a tap interface through vhost-net
- mergeable buffers disabled
/usr/bin/qemu-system-x86_64 -k fr -daemonize --enable-kvm -m 2G -cpu host \
-smp 3 -serial telnet::40564,server,nowait -serial null \
-qmp tcp::44340,server,nowait -monitor telnet::49229,server,nowait \
-device ne2k_pci,mac=de:ad:de:01:02:03,netdev=user.0,addr=03 \
-netdev user,id=user.0,hostfwd=tcp::34965-:22 \
-netdev type=tap,id=vhostnet0,script=no,vhost=on,queues=8 \
-device virtio-net-pci,mrg_rxbuf=off,netdev=vhostnet0,mq=on,vectors=17 \
-hda "${VM_PATH}/ubuntu-16.04-template.qcow2" \
-snapshot -vga none -display none
Guest configuration
-------------------
Apply a simple patch to display m->port in test-pmd/rxonly.c.
--- a/app/test-pmd/rxonly.c
+++ b/app/test-pmd/rxonly.c
@@ -139,9 +139,9 @@ pkt_burst_receive(struct fwd_stream *fs)
print_ether_addr(" src=", ð_hdr->s_addr);
print_ether_addr(" - dst=", ð_hdr->d_addr);
- printf(" - type=0x%04x - length=%u - nb_segs=%d",
+ printf(" - type=0x%04x - length=%u - nb_segs=%d - port=%d",
eth_type, (unsigned) mb->pkt_len,
- (int)mb->nb_segs);
+ (int)mb->nb_segs, mb->port);
if (ol_flags & PKT_RX_RSS_HASH) {
printf(" - RSS hash=0x%x", (unsigned) mb->hash.rss);
printf(" - RSS queue=0x%x",(unsigned) fs->rx_queue);
Compile dpdk:
cd dpdk.org
make config T=x86_64-native-linuxapp-gcc
sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=y,' build/.config
sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=y,' build/.config
make -j4
Prepare environment:
mkdir -p /mnt/huge
mount -t hugetlbfs nodev /mnt/huge
echo 256 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
modprobe uio_pci_generic
python usertools/dpdk-devbind.py -b uio_pci_generic 0000:00:02.0
./build/app/testpmd -l 0,1 --log-level 7 -- --total-num-mbufs=16384 \
-i --port-topology=chained --disable-hw-vlan-filter \
--disable-hw-vlan-strip --txqflags=0xf01
...
PMD: virtio_update_rxtx_handler(): Using simple rx/tx path
...
Configure testpmd:
set fwd rxonly
set verbose 1
start
The first 128 received packets have **a wrong m->port**):
src=00:00:00:00:00:00 - dst=FF:FF:FF:FF:FF:FF - type=0x0800 - length=42 - nb_segs=1 - port=255 - sw ptype: L2_ETHER L3_IPV4 - l2_len=14 - l3_len=20 - Receive queue=0x0
ol_flags: PKT_RX_L4_CKSUM_UNKNOWN PKT_RX_IP_CKSUM_UNKNOWN
After 128 packets, it's ok:
src=00:00:00:00:00:00 - dst=FF:FF:FF:FF:FF:FF - type=0x0800 - length=42 - nb_segs=1 - port=0 - sw ptype: L2_ETHER L3_IPV4 - l2_len=14 - l3_len=20 - Receive queue=0x0
ol_flags: PKT_RX_L4_CKSUM_UNKNOWN PKT_RX_IP_CKSUM_UNKNOWN
This is not reproduced with --txqflags=0 (standard Rx path), or with
the fix applied.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 6/9] net/virtio: fix queue setup consistency
2017-08-31 13:40 ` [PATCH 6/9] net/virtio: fix queue setup consistency Olivier Matz
@ 2017-08-31 13:49 ` Olivier MATZ
0 siblings, 0 replies; 45+ messages in thread
From: Olivier MATZ @ 2017-08-31 13:49 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen, stable
Platform description
--------------------
guest (dpdk)
+----------------+
| |
| |
| port0 |
+----------------+
|
| virtio
|
+----------------+
| tap0 |
| |
| |
+----------------+
host (linux, vhost-net)
Host configuration
------------------
Start qemu with:
- a ne2k management interface to avoi any conflict with dpdk
- a virtio net device, connected to a tap interface through vhost-net
- mergeable buffers disabled
/usr/bin/qemu-system-x86_64 -k fr -daemonize --enable-kvm -m 2G -cpu host \
-smp 3 -serial telnet::40564,server,nowait -serial null \
-qmp tcp::44340,server,nowait -monitor telnet::49229,server,nowait \
-device ne2k_pci,mac=de:ad:de:01:02:03,netdev=user.0,addr=03 \
-netdev user,id=user.0,hostfwd=tcp::34965-:22 \
-netdev type=tap,id=vhostnet0,script=no,vhost=on,queues=8 \
-device virtio-net-pci,mrg_rxbuf=off,netdev=vhostnet0,mq=on,vectors=17 \
-hda "${VM_PATH}/ubuntu-16.04-template.qcow2" \
-snapshot -vga none -display none
Guest configuration
-------------------
Apply a patch that reverts initialization of queues in testpmd
(initialize rx queue first), and displays some logs in virtio:
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1461,34 +1461,10 @@ start_port(portid_t pid)
}
if (port->need_reconfig_queues > 0) {
port->need_reconfig_queues = 0;
- /* setup tx queues */
- for (qi = 0; qi < nb_txq; qi++) {
- if ((numa_support) &&
- (txring_numa[pi] != NUMA_NO_CONFIG))
- diag = rte_eth_tx_queue_setup(pi, qi,
- nb_txd,txring_numa[pi],
- &(port->tx_conf));
- else
- diag = rte_eth_tx_queue_setup(pi, qi,
- nb_txd,port->socket_id,
- &(port->tx_conf));
-
- if (diag == 0)
- continue;
-
- /* Fail to setup tx queue, return */
- if (rte_atomic16_cmpset(&(port->port_status),
- RTE_PORT_HANDLING,
- RTE_PORT_STOPPED) == 0)
- printf("Port %d can not be set back "
- "to stopped\n", pi);
- printf("Fail to configure port %d tx queues\n", pi);
- /* try to reconfigure queues next time */
- port->need_reconfig_queues = 1;
- return -1;
- }
/* setup rx queues */
for (qi = 0; qi < nb_rxq; qi++) {
+ printf("rte_eth_rx_queue_setup %d %d\n",
+ pi, qi);
if ((numa_support) &&
(rxring_numa[pi] != NUMA_NO_CONFIG)) {
struct rte_mempool * mp =
@@ -1500,7 +1476,6 @@ start_port(portid_t pid)
rxring_numa[pi]);
return -1;
}
-
diag = rte_eth_rx_queue_setup(pi, qi,
nb_rxd,rxring_numa[pi],
&(port->rx_conf),mp);
@@ -1532,6 +1507,34 @@ start_port(portid_t pid)
port->need_reconfig_queues = 1;
return -1;
}
+ /* setup tx queues */
+ for (qi = 0; qi < nb_txq; qi++) {
+ printf("rte_eth_tx_queue_setup %d %d\n",
+ pi, qi);
+ if ((numa_support) &&
+ (txring_numa[pi] != NUMA_NO_CONFIG))
+ diag = rte_eth_tx_queue_setup(pi, qi,
+ nb_txd,txring_numa[pi],
+ &(port->tx_conf));
+ else
+ diag = rte_eth_tx_queue_setup(pi, qi,
+ nb_txd,port->socket_id,
+ &(port->tx_conf));
+
+ if (diag == 0)
+ continue;
+
+ /* Fail to setup tx queue, return */
+ if (rte_atomic16_cmpset(&(port->port_status),
+ RTE_PORT_HANDLING,
+ RTE_PORT_STOPPED) == 0)
+ printf("Port %d can not be set back "
+ "to stopped\n", pi);
+ printf("Fail to configure port %d tx queues\n", pi);
+ /* try to reconfigure queues next time */
+ port->need_reconfig_queues = 1;
+ return -1;
+ }
}
for (event_type = RTE_ETH_EVENT_UNKNOWN;
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -445,6 +445,8 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
nbufs = 0;
error = ENOSPC;
+ printf("rx_queue_setup() use_simple_rxtx=%d\n",
+ hw->use_simple_rxtx);
if (hw->use_simple_rxtx) {
for (desc_idx = 0; desc_idx < vq->vq_nentries;
desc_idx++) {
@@ -563,6 +565,8 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
vq->vq_free_thresh = tx_free_thresh;
+ printf("tx_queue_setup() use_simple_rxtx=%d\n",
+ hw->use_simple_rxtx);
if (hw->use_simple_rxtx) {
uint16_t mid_idx = vq->vq_nentries >> 1;
Compile dpdk:
cd dpdk.org
make config T=x86_64-native-linuxapp-gcc
sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=y,' build/.config
sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=y,' build/.config
make -j4
Prepare environment:
mkdir -p /mnt/huge
mount -t hugetlbfs nodev /mnt/huge
echo 256 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
modprobe uio_pci_generic
python usertools/dpdk-devbind.py -b uio_pci_generic 0000:00:02.0
./build/app/testpmd -l 0,1 --log-level 7 -- --total-num-mbufs=16384 \
-i --port-topology=chained --disable-hw-vlan-filter \
--disable-hw-vlan-strip --txqflags=0xf01
...
Configuring Port 0 (socket 0)
rte_eth_rx_queue_setup 0 0
rx_queue_setup() use_simple_rxtx=0
rte_eth_tx_queue_setup 0 0
PMD: virtio_update_rxtx_handler(): Using simple rx/tx path
tx_queue_setup() use_simple_rxtx=1
...
Configure testpmd:
set fwd rxonly
set verbose 1
start
Without the fix, there is a segfault in virtio_recv_pkts_vec()
It works ok with the patch.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config
2017-08-31 13:40 ` [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config Olivier Matz
@ 2017-08-31 13:50 ` Olivier MATZ
2017-09-01 9:25 ` Yuanhan Liu
1 sibling, 0 replies; 45+ messages in thread
From: Olivier MATZ @ 2017-08-31 13:50 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen, stable
Platform description
--------------------
guest (dpdk)
+----------------+
| |
| |
| port0 |
+----------------+
|
| virtio
|
+----------------+
| tap0 |
| |
| |
+----------------+
host (linux, vhost-net)
Host configuration
------------------
Start qemu with:
- a ne2k management interface to avoi any conflict with dpdk
- a virtio net device, connected to a tap interface through vhost-net
- mergeable buffers disabled
/usr/bin/qemu-system-x86_64 -k fr -daemonize --enable-kvm -m 2G -cpu host \
-smp 3 -serial telnet::40564,server,nowait -serial null \
-qmp tcp::44340,server,nowait -monitor telnet::49229,server,nowait \
-device ne2k_pci,mac=de:ad:de:01:02:03,netdev=user.0,addr=03 \
-netdev user,id=user.0,hostfwd=tcp::34965-:22 \
-netdev type=tap,id=vhostnet0,script=no,vhost=on,queues=8 \
-device virtio-net-pci,mrg_rxbuf=off,netdev=vhostnet0,mq=on,vectors=17 \
-hda "${VM_PATH}/ubuntu-16.04-template.qcow2" \
-snapshot -vga none -display none
Guest configuration
-------------------
Compile dpdk:
cd dpdk.org
make config T=x86_64-native-linuxapp-gcc
sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=y,' build/.config
sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=y,' build/.config
make -j4
Prepare environment:
mkdir -p /mnt/huge
mount -t hugetlbfs nodev /mnt/huge
echo 256 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
modprobe uio_pci_generic
python usertools/dpdk-devbind.py -b uio_pci_generic 0000:00:02.0
./build/app/testpmd -l 0,1 --log-level 7 -- --total-num-mbufs=16384 \
-i --port-topology=chained --disable-hw-vlan-filter \
--disable-hw-vlan-strip --txqflags=0
Without the fix, standard path is used in rx and tx, but simple rx could
be used:
...
PMD: set_rxtx_funcs(): virtio: using standard Rx path on port 0
PMD: set_rxtx_funcs(): virtio: using standard Tx path on port 0
...
With the fix:
...
PMD: set_rxtx_funcs(): virtio: using simple Rx path on port 0
PMD: set_rxtx_funcs(): virtio: using standard Tx path on port 0
...
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 9/9] net/virtio: fix Rx handler when checksum is requested
2017-08-31 13:40 ` [PATCH 9/9] net/virtio: fix Rx handler when checksum is requested Olivier Matz
@ 2017-08-31 13:51 ` Olivier MATZ
0 siblings, 0 replies; 45+ messages in thread
From: Olivier MATZ @ 2017-08-31 13:51 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen, stable
Platform description
--------------------
guest (dpdk)
+----------------+
| |
| |
| port0 |
+----------------+
|
| virtio
|
+----------------+
| tap0 |
| |
| |
+----------------+
host (linux, vhost-net)
Host configuration
------------------
Start qemu with:
- a ne2k management interface to avoi any conflict with dpdk
- a virtio net device, connected to a tap interface through vhost-net
- mergeable buffers disabled
/usr/bin/qemu-system-x86_64 -k fr -daemonize --enable-kvm -m 2G -cpu host \
-smp 3 -serial telnet::40564,server,nowait -serial null \
-qmp tcp::44340,server,nowait -monitor telnet::49229,server,nowait \
-device ne2k_pci,mac=de:ad:de:01:02:03,netdev=user.0,addr=03 \
-netdev user,id=user.0,hostfwd=tcp::34965-:22 \
-netdev type=tap,id=vhostnet0,script=no,vhost=on,queues=8 \
-device virtio-net-pci,mrg_rxbuf=off,netdev=vhostnet0,mq=on,vectors=17 \
-hda "${VM_PATH}/ubuntu-16.04-template.qcow2" \
-snapshot -vga none -display none
Guest configuration
-------------------
Compile dpdk:
cd dpdk.org
make config T=x86_64-native-linuxapp-gcc
sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=y,' build/.config
sed -i 's,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=n,CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=y,' build/.config
make -j4
Prepare environment:
mkdir -p /mnt/huge
mount -t hugetlbfs nodev /mnt/huge
echo 256 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
modprobe uio_pci_generic
python usertools/dpdk-devbind.py -b uio_pci_generic 0000:00:02.0
./build/app/testpmd -l 0,1 --log-level 7 -- --total-num-mbufs=16384 \
-i --port-topology=chained --disable-hw-vlan-filter \
--enable-rx-cksum --disable-hw-vlan-strip --txqflags=0
Without the fix, simple path is used for rx despite it does not
support rx checksum:
...
PMD: set_rxtx_funcs(): virtio: using simple Rx path on port 0
PMD: set_rxtx_funcs(): virtio: using standard Tx path on port 0
...
Configure testpmd:
set fwd rxonly
set verbose 1
start
Without the fix, the received packets don't have the proper checksum
flags (should be PKT_RX_L4_CKSUM_NONE):
port 0/queue 0: received 1 packets
src=1A:3F:FB:C6:FF:14 - dst=52:54:00:12:34:56 - type=0x0800 - length=74 - nb_segs=1 - sw ptype: L2_ETHER L3_IPV4 L4_TCP - l2_len=14 - l3_len=20 - l4_len=40 - Receive queue=0x0
ol_flags: PKT_RX_L4_CKSUM_UNKNOWN PKT_RX_IP_CKSUM_UNKNOWN
With the fix, standard rx path is used and the flags are correct:
...
PMD: set_rxtx_funcs(): virtio: using standard Rx path on port 0
PMD: set_rxtx_funcs(): virtio: using standard Tx path on port 0
...
port 0/queue 0: received 1 packets
src=1A:3F:FB:C6:FF:14 - dst=52:54:00:12:34:56 - type=0x0800 - length=74 - nb_segs=1 - hw ptype: L2_ETHER L3_IPV4 L4_TCP - sw ptype: L2_ETHER L3_IPV4 L4_TCP - l2_len=14 - l3_len=20 - l4_len=40 - Receive queue=0x0
ol_flags: PKT_RX_L4_CKSUM_NONE PKT_RX_IP_CKSUM_UNKNOWN
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers
2017-08-31 13:40 ` [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers Olivier Matz
@ 2017-09-01 9:19 ` Yuanhan Liu
2017-09-01 9:52 ` Olivier MATZ
0 siblings, 1 reply; 45+ messages in thread
From: Yuanhan Liu @ 2017-09-01 9:19 UTC (permalink / raw)
To: Olivier Matz; +Cc: dev, maxime.coquelin, stephen, stable
On Thu, Aug 31, 2017 at 03:40:13PM +0200, Olivier Matz wrote:
> The selection of Rx/Tx handlers is done at several places,
> group them in one function set_rxtx_funcs().
>
> The update of hw->use_simple_rxtx is also rationalized:
> - initialized to 1 (prefer simple path)
> - in dev configure or rx/tx queue setup, if something prevents from
> using the simple path, change it to 0.
> - in dev start, set the handlers according to hw->use_simple_rxtx.
>
> Cc: stable@dpdk.org
This patch looks like a refactoring to me, that I don't think it's really
a good idea to back port it to a stable release.
...
> @@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
>
> eth_dev->dev_ops = &virtio_eth_dev_ops;
> - eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
>
> if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> if (!hw->virtio_user_dev) {
> @@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> }
>
> virtio_set_vtpci_ops(hw);
> - if (hw->use_simple_rxtx) {
> - eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> - eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> - } else {
> - rx_func_get(eth_dev);
> - }
> + set_rxtx_funcs(eth_dev);
No need to invoke it here?
> +
> return 0;
> }
>
> @@ -1726,6 +1741,18 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> return -EBUSY;
> }
>
> + hw->use_simple_rxtx = 1;
> +
> +#if defined RTE_ARCH_X86
> + if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
DPDK now requires SSE4.2. You might want to add another patch to remove
this testing.
> + hw->use_simple_rxtx = 0;
> +#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> + if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> + hw->use_simple_rxtx = 0;
> +#endif
> + if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
> + hw->use_simple_rxtx = 0;
> +
> return 0;
> }
>
> @@ -1802,6 +1829,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
> VIRTQUEUE_DUMP(txvq->vq);
> }
>
> + set_rxtx_funcs(dev);
> hw->started = 1;
>
> /* Initialize Link state */
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index a32e3229f..c9d97b643 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -505,25 +505,14 @@ static void
> virtio_update_rxtx_handler(struct rte_eth_dev *dev,
> const struct rte_eth_txconf *tx_conf)
This function name doesn't quite make sense now after your refactoring.
> {
> - uint8_t use_simple_rxtx = 0;
> struct virtio_hw *hw = dev->data->dev_private;
> + uint8_t use_simple_rxtx = hw->use_simple_rxtx;
>
> -#if defined RTE_ARCH_X86
> - if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
> - use_simple_rxtx = 1;
> -#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> - if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> - use_simple_rxtx = 1;
> -#endif
> - /* Use simple rx/tx func if single segment and no offloads */
> - if (use_simple_rxtx &&
> - (tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == VIRTIO_SIMPLE_FLAGS &&
> - !vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
> - PMD_INIT_LOG(INFO, "Using simple rx/tx path");
> - dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> - dev->rx_pkt_burst = virtio_recv_pkts_vec;
> - hw->use_simple_rxtx = use_simple_rxtx;
> - }
> + /* cannot use simple rxtx funcs with multisegs or offloads */
> + if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) != VIRTIO_SIMPLE_FLAGS)
> + use_simple_rxtx = 0;
And that's what left in this function. How about just un-folding it?
--yliu
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config
2017-08-31 13:40 ` [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config Olivier Matz
2017-08-31 13:50 ` Olivier MATZ
@ 2017-09-01 9:25 ` Yuanhan Liu
2017-09-01 9:58 ` Olivier MATZ
1 sibling, 1 reply; 45+ messages in thread
From: Yuanhan Liu @ 2017-09-01 9:25 UTC (permalink / raw)
To: Olivier Matz; +Cc: dev, maxime.coquelin, stephen, stable
On Thu, Aug 31, 2017 at 03:40:14PM +0200, Olivier Matz wrote:
> Split use_simple_rxtx into use_simple_rx and use_simple_tx,
> and ensure that only use_simple_tx is updated when txq flags
> forces to use the standard Tx handler.
I think it's a good idea to split it.
> This change is also useful for next commit (disable simple Rx
> path when Rx checksum is requested).
>
> Cc: stable@dpdk.org
But again, I don't think it's a good idea to put them (including the
next patch) to stable releases.
--yliu
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers
2017-09-01 9:19 ` Yuanhan Liu
@ 2017-09-01 9:52 ` Olivier MATZ
2017-09-01 12:31 ` Yuanhan Liu
0 siblings, 1 reply; 45+ messages in thread
From: Olivier MATZ @ 2017-09-01 9:52 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: dev, maxime.coquelin, stephen, stable
Hi Yuanhan,
On Fri, Sep 01, 2017 at 05:19:16PM +0800, Yuanhan Liu wrote:
> On Thu, Aug 31, 2017 at 03:40:13PM +0200, Olivier Matz wrote:
> > The selection of Rx/Tx handlers is done at several places,
> > group them in one function set_rxtx_funcs().
> >
> > The update of hw->use_simple_rxtx is also rationalized:
> > - initialized to 1 (prefer simple path)
> > - in dev configure or rx/tx queue setup, if something prevents from
> > using the simple path, change it to 0.
> > - in dev start, set the handlers according to hw->use_simple_rxtx.
> >
> > Cc: stable@dpdk.org
>
> This patch looks like a refactoring to me, that I don't think it's really
> a good idea to back port it to a stable release.
I CCed stable for this commit because next ones, which are fixes, depend
on it. If you consider they should not be included in stable, we can also
drop this one.
> ...
> > @@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
> >
> > eth_dev->dev_ops = &virtio_eth_dev_ops;
> > - eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
> >
> > if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > if (!hw->virtio_user_dev) {
> > @@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > }
> >
> > virtio_set_vtpci_ops(hw);
> > - if (hw->use_simple_rxtx) {
> > - eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> > - eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> > - } else {
> > - rx_func_get(eth_dev);
> > - }
> > + set_rxtx_funcs(eth_dev);
>
> No need to invoke it here?
I wanted to stay consistent with previous code.
I'm not very used to work with secondary processes, so I'm not 100% it
is ok, but in my understanding, in that case the configuration is done
first by the primary process, and the secondary the pointers to the
rx/tx funcs. I suppose their value can be different than primary ones.
>
> > +
> > return 0;
> > }
> >
> > @@ -1726,6 +1741,18 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> > return -EBUSY;
> > }
> >
> > + hw->use_simple_rxtx = 1;
> > +
> > +#if defined RTE_ARCH_X86
> > + if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
>
> DPDK now requires SSE4.2. You might want to add another patch to remove
> this testing.
ok, will do.
>
> > + hw->use_simple_rxtx = 0;
> > +#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> > + if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> > + hw->use_simple_rxtx = 0;
> > +#endif
> > + if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
> > + hw->use_simple_rxtx = 0;
> > +
> > return 0;
> > }
> >
> > @@ -1802,6 +1829,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
> > VIRTQUEUE_DUMP(txvq->vq);
> > }
> >
> > + set_rxtx_funcs(dev);
> > hw->started = 1;
> >
> > /* Initialize Link state */
> > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> > index a32e3229f..c9d97b643 100644
> > --- a/drivers/net/virtio/virtio_rxtx.c
> > +++ b/drivers/net/virtio/virtio_rxtx.c
> > @@ -505,25 +505,14 @@ static void
> > virtio_update_rxtx_handler(struct rte_eth_dev *dev,
> > const struct rte_eth_txconf *tx_conf)
>
> This function name doesn't quite make sense now after your refactoring.
It updates hw->use_simple_rxtx, which at the end will change the
rxtx handler. I didn't find a better name.
My other (poor) ideas were:
virtio_update_[rxtx_]handler_requirements
virtio_update_[rxtx_]handler_constraints
virtio_update_[rxtx_]handler_selector
>
> > {
> > - uint8_t use_simple_rxtx = 0;
> > struct virtio_hw *hw = dev->data->dev_private;
> > + uint8_t use_simple_rxtx = hw->use_simple_rxtx;
> >
> > -#if defined RTE_ARCH_X86
> > - if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
> > - use_simple_rxtx = 1;
> > -#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> > - if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> > - use_simple_rxtx = 1;
> > -#endif
> > - /* Use simple rx/tx func if single segment and no offloads */
> > - if (use_simple_rxtx &&
> > - (tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == VIRTIO_SIMPLE_FLAGS &&
> > - !vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
> > - PMD_INIT_LOG(INFO, "Using simple rx/tx path");
> > - dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> > - dev->rx_pkt_burst = virtio_recv_pkts_vec;
> > - hw->use_simple_rxtx = use_simple_rxtx;
> > - }
> > + /* cannot use simple rxtx funcs with multisegs or offloads */
> > + if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) != VIRTIO_SIMPLE_FLAGS)
> > + use_simple_rxtx = 0;
>
> And that's what left in this function. How about just un-folding it?
>
> --yliu
yep, we can inline it.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config
2017-09-01 9:25 ` Yuanhan Liu
@ 2017-09-01 9:58 ` Olivier MATZ
2017-09-01 12:22 ` Yuanhan Liu
0 siblings, 1 reply; 45+ messages in thread
From: Olivier MATZ @ 2017-09-01 9:58 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: dev, maxime.coquelin, stephen, stable
On Fri, Sep 01, 2017 at 05:25:38PM +0800, Yuanhan Liu wrote:
> On Thu, Aug 31, 2017 at 03:40:14PM +0200, Olivier Matz wrote:
> > Split use_simple_rxtx into use_simple_rx and use_simple_tx,
> > and ensure that only use_simple_tx is updated when txq flags
> > forces to use the standard Tx handler.
>
> I think it's a good idea to split it.
>
> > This change is also useful for next commit (disable simple Rx
> > path when Rx checksum is requested).
> >
> > Cc: stable@dpdk.org
>
> But again, I don't think it's a good idea to put them (including the
> next patch) to stable releases.
Yes, you're right this one is more an enhancement than a fix: it
just selects a faster handler if it's possible.
But next one is a fix: it must not select the simple handler if
offload is requested.
If these commits are not pushed in stable, it's not a problem for
me, so I let you decide. In that case, I will remove the Cc: stable
from the last 3 commits. Do you confirm?
Thanks for the review.
Olivier
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config
2017-09-01 9:58 ` Olivier MATZ
@ 2017-09-01 12:22 ` Yuanhan Liu
0 siblings, 0 replies; 45+ messages in thread
From: Yuanhan Liu @ 2017-09-01 12:22 UTC (permalink / raw)
To: Olivier MATZ; +Cc: dev, maxime.coquelin, stephen, stable
On Fri, Sep 01, 2017 at 11:58:07AM +0200, Olivier MATZ wrote:
> On Fri, Sep 01, 2017 at 05:25:38PM +0800, Yuanhan Liu wrote:
> > On Thu, Aug 31, 2017 at 03:40:14PM +0200, Olivier Matz wrote:
> > > Split use_simple_rxtx into use_simple_rx and use_simple_tx,
> > > and ensure that only use_simple_tx is updated when txq flags
> > > forces to use the standard Tx handler.
> >
> > I think it's a good idea to split it.
> >
> > > This change is also useful for next commit (disable simple Rx
> > > path when Rx checksum is requested).
> > >
> > > Cc: stable@dpdk.org
> >
> > But again, I don't think it's a good idea to put them (including the
> > next patch) to stable releases.
>
> Yes, you're right this one is more an enhancement than a fix: it
> just selects a faster handler if it's possible.
Exactly.
> But next one is a fix: it must not select the simple handler if
> offload is requested.
>
> If these commits are not pushed in stable, it's not a problem for
> me, so I let you decide. In that case, I will remove the Cc: stable
> from the last 3 commits. Do you confirm?
Yes, I think it's better to drop the cc stable tag.
> Thanks for the review.
Thanks for the work!
--yliu
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers
2017-09-01 9:52 ` Olivier MATZ
@ 2017-09-01 12:31 ` Yuanhan Liu
2017-09-06 14:40 ` Olivier MATZ
0 siblings, 1 reply; 45+ messages in thread
From: Yuanhan Liu @ 2017-09-01 12:31 UTC (permalink / raw)
To: Olivier MATZ; +Cc: dev, maxime.coquelin, stephen, stable
On Fri, Sep 01, 2017 at 11:52:17AM +0200, Olivier MATZ wrote:
> > > @@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
> > >
> > > eth_dev->dev_ops = &virtio_eth_dev_ops;
> > > - eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
> > >
> > > if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > > if (!hw->virtio_user_dev) {
> > > @@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > }
> > >
> > > virtio_set_vtpci_ops(hw);
> > > - if (hw->use_simple_rxtx) {
> > > - eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> > > - eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> > > - } else {
> > > - rx_func_get(eth_dev);
> > > - }
> > > + set_rxtx_funcs(eth_dev);
> >
> > No need to invoke it here?
>
> I wanted to stay consistent with previous code.
>
> I'm not very used to work with secondary processes, so I'm not 100% it
> is ok, but in my understanding, in that case the configuration is done
> first by the primary process, and the secondary the pointers to the
> rx/tx funcs. I suppose their value can be different than primary ones.
It probably needs some testing, but I think it should be okay, because
the rx/tx funcs will always be updated at dev_start in this patch.
--yliu
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers
2017-09-01 12:31 ` Yuanhan Liu
@ 2017-09-06 14:40 ` Olivier MATZ
2017-09-07 8:13 ` Yuanhan Liu
0 siblings, 1 reply; 45+ messages in thread
From: Olivier MATZ @ 2017-09-06 14:40 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: dev, maxime.coquelin, stephen, stable
On Fri, Sep 01, 2017 at 08:31:06PM +0800, Yuanhan Liu wrote:
> On Fri, Sep 01, 2017 at 11:52:17AM +0200, Olivier MATZ wrote:
> > > > @@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > > RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
> > > >
> > > > eth_dev->dev_ops = &virtio_eth_dev_ops;
> > > > - eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
> > > >
> > > > if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > > > if (!hw->virtio_user_dev) {
> > > > @@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > > }
> > > >
> > > > virtio_set_vtpci_ops(hw);
> > > > - if (hw->use_simple_rxtx) {
> > > > - eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> > > > - eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> > > > - } else {
> > > > - rx_func_get(eth_dev);
> > > > - }
> > > > + set_rxtx_funcs(eth_dev);
> > >
> > > No need to invoke it here?
> >
> > I wanted to stay consistent with previous code.
> >
> > I'm not very used to work with secondary processes, so I'm not 100% it
> > is ok, but in my understanding, in that case the configuration is done
> > first by the primary process, and the secondary the pointers to the
> > rx/tx funcs. I suppose their value can be different than primary ones.
>
> It probably needs some testing, but I think it should be okay, because
> the rx/tx funcs will always be updated at dev_start in this patch.
I still have one doubt about this: looking in
examples/multi_process/symmetric_mp/main.c, I can see that rte_eth_dev_start()
is only called on the primary process. So if I remove set_rxtx_funcs() from
eth_virtio_dev_init(), it looks that the rx functions won't be initialized.
Again, I'm not a user of multi_proc, so I may be wrong, but I think
it would be safer to keep it.
Olivier
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers
2017-09-06 14:40 ` Olivier MATZ
@ 2017-09-07 8:13 ` Yuanhan Liu
0 siblings, 0 replies; 45+ messages in thread
From: Yuanhan Liu @ 2017-09-07 8:13 UTC (permalink / raw)
To: Olivier MATZ; +Cc: dev, maxime.coquelin, stephen, stable
On Wed, Sep 06, 2017 at 04:40:12PM +0200, Olivier MATZ wrote:
> On Fri, Sep 01, 2017 at 08:31:06PM +0800, Yuanhan Liu wrote:
> > On Fri, Sep 01, 2017 at 11:52:17AM +0200, Olivier MATZ wrote:
> > > > > @@ -1534,7 +1554,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > > > RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
> > > > >
> > > > > eth_dev->dev_ops = &virtio_eth_dev_ops;
> > > > > - eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
> > > > >
> > > > > if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > > > > if (!hw->virtio_user_dev) {
> > > > > @@ -1544,12 +1563,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > > > > }
> > > > >
> > > > > virtio_set_vtpci_ops(hw);
> > > > > - if (hw->use_simple_rxtx) {
> > > > > - eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> > > > > - eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> > > > > - } else {
> > > > > - rx_func_get(eth_dev);
> > > > > - }
> > > > > + set_rxtx_funcs(eth_dev);
> > > >
> > > > No need to invoke it here?
> > >
> > > I wanted to stay consistent with previous code.
> > >
> > > I'm not very used to work with secondary processes, so I'm not 100% it
> > > is ok, but in my understanding, in that case the configuration is done
> > > first by the primary process, and the secondary the pointers to the
> > > rx/tx funcs. I suppose their value can be different than primary ones.
> >
> > It probably needs some testing, but I think it should be okay, because
> > the rx/tx funcs will always be updated at dev_start in this patch.
>
> I still have one doubt about this: looking in
> examples/multi_process/symmetric_mp/main.c, I can see that rte_eth_dev_start()
> is only called on the primary process. So if I remove set_rxtx_funcs() from
> eth_virtio_dev_init(), it looks that the rx functions won't be initialized.
>
> Again, I'm not a user of multi_proc, so I may be wrong, but I think
> it would be safer to keep it.
Yes, you are right.
--yliu
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 00/10] virtio fixes
2017-08-31 13:40 [PATCH 0/9] virtio fixes Olivier Matz
` (8 preceding siblings ...)
2017-08-31 13:40 ` [PATCH 9/9] net/virtio: fix Rx handler when checksum is requested Olivier Matz
@ 2017-09-07 12:13 ` Olivier Matz
2017-09-07 12:13 ` [PATCH v2 01/10] net/virtio: revert "do not claim to support LRO" Olivier Matz
` (10 more replies)
9 siblings, 11 replies; 45+ messages in thread
From: Olivier Matz @ 2017-09-07 12:13 UTC (permalink / raw)
To: dev, yliu, maxime.coquelin; +Cc: stephen
This patchset addresses several issues related to offload and
Rx/Tx handler selection in virtio PMD.
v1 -> v2:
- add one patch to remove uneeded SSE check on x86
- remove Cc stable on last patches
- inline virtio_update_rxtx_handler() in tx queue setup
Olivier Matz (10):
net/virtio: revert "do not claim to support LRO"
net/virtio: revert "do not falsely claim to do IP checksum"
doc: fix description of L4 Rx checksum offload
net/virtio: fix log levels in configure
net/virtio: fix mbuf port for simple Rx function
net/virtio: fix queue setup consistency
net/virtio: rationalize setting of Rx/Tx handlers
net/virtio: remove SSE check
net/virtio: keep Rx handler whatever the Tx queue config
net/virtio: fix Rx handler when checksum is requested
doc/guides/nics/features.rst | 1 +
drivers/net/virtio/virtio_ethdev.c | 111 ++++++++++++++++++++++++++------
drivers/net/virtio/virtio_ethdev.h | 6 ++
drivers/net/virtio/virtio_pci.h | 3 +-
drivers/net/virtio/virtio_rxtx.c | 73 ++++++++++-----------
drivers/net/virtio/virtio_rxtx_simple.c | 2 +
drivers/net/virtio/virtio_user_ethdev.c | 3 +-
7 files changed, 141 insertions(+), 58 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 45+ messages in thread
* [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
* [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
* 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
end of thread, other threads:[~2018-02-09 9:40 UTC | newest]
Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-31 13:40 [PATCH 0/9] virtio fixes Olivier Matz
2017-08-31 13:40 ` [PATCH 1/9] net/virtio: revert "do not claim to support LRO" Olivier Matz
2017-08-31 13:40 ` [PATCH 2/9] net/virtio: revert "do not falsely claim to do IP checksum" Olivier Matz
2017-08-31 13:47 ` Olivier MATZ
2017-08-31 13:40 ` [PATCH 3/9] doc: fix description of L4 Rx checksum offload Olivier Matz
2017-08-31 13:40 ` [PATCH 4/9] net/virtio: fix log levels in configure Olivier Matz
2017-08-31 13:40 ` [PATCH 5/9] net/virtio: fix mbuf port for simple Rx function Olivier Matz
2017-08-31 13:48 ` Olivier MATZ
2017-08-31 13:40 ` [PATCH 6/9] net/virtio: fix queue setup consistency Olivier Matz
2017-08-31 13:49 ` Olivier MATZ
2017-08-31 13:40 ` [PATCH 7/9] net/virtio: rationalize setting of Rx/Tx handlers Olivier Matz
2017-09-01 9:19 ` Yuanhan Liu
2017-09-01 9:52 ` Olivier MATZ
2017-09-01 12:31 ` Yuanhan Liu
2017-09-06 14:40 ` Olivier MATZ
2017-09-07 8:13 ` Yuanhan Liu
2017-08-31 13:40 ` [PATCH 8/9] net/virtio: keep Rx handler whatever the Tx queue config Olivier Matz
2017-08-31 13:50 ` Olivier MATZ
2017-09-01 9:25 ` Yuanhan Liu
2017-09-01 9:58 ` Olivier MATZ
2017-09-01 12:22 ` Yuanhan Liu
2017-08-31 13:40 ` [PATCH 9/9] net/virtio: fix Rx handler when checksum is requested Olivier Matz
2017-08-31 13:51 ` Olivier MATZ
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 ` [PATCH v2 03/10] doc: fix description of L4 Rx checksum offload Olivier Matz
2017-09-07 12:13 ` [PATCH v2 04/10] net/virtio: fix log levels in configure Olivier Matz
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 ` [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
2017-12-08 2:17 ` Tiwei Bie
2018-02-01 3:14 ` Yao, Lei A
2018-02-01 8:27 ` Olivier Matz
2018-02-07 8:31 ` Xu, Qian Q
2018-02-07 22:01 ` Olivier Matz
2018-02-09 5:44 ` Wang, Zhihong
2018-02-09 8:59 ` Maxime Coquelin
2018-02-09 9:40 ` Maxime Coquelin
2017-09-07 12:13 ` [PATCH v2 07/10] net/virtio: rationalize setting of Rx/Tx handlers Olivier Matz
2017-09-07 12:13 ` [PATCH v2 08/10] net/virtio: remove SSE check Olivier Matz
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 ` [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
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.