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

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

Signed-off-by: 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

Olivier Matz

2017-08-31 13:40:08 UTC

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

Signed-off-by: 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

Olivier MATZ

2017-08-31 13:47:42 UTC

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

Olivier Matz

2017-08-31 13:40:09 UTC

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

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

Signed-off-by: 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

Olivier Matz

2017-08-31 13:40:10 UTC

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

Signed-off-by: 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

Olivier Matz

2017-08-31 13:40:13 UTC

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

Signed-off-by: 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

Yuanhan Liu

2017-09-01 09:19:16 UTC

Post by Olivier Matz
The selection of Rx/Tx handlers is done at several places,
group them in one function set_rxtx_funcs().
- 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.

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.

...

Post by Olivier Matz
@@ -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?

Post by Olivier Matz
+
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.

Post by Olivier Matz
+ 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.

Post by Olivier Matz
{
- 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

Olivier MATZ

2017-09-01 09:52:17 UTC

Hi Yuanhan,

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.

Post by Yuanhan Liu
...

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.

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

ok, will do.

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

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

yep, we can inline it.

Yuanhan Liu

2017-09-01 12:31:06 UTC

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

Olivier MATZ

2017-09-06 14:40:12 UTC

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

Yuanhan Liu

2017-09-07 08:13:30 UTC

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

Olivier Matz

2017-08-31 13:40:11 UTC

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

Signed-off-by: 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

Olivier MATZ

2017-08-31 13:48:36 UTC

Validation:

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

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

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

Start qemu with:

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

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

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

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

--- a/app/test-pmd/rxonly.c
+++ b/app/test-pmd/rxonly.c
@@ -139,9 +139,9 @@ pkt_burst_receive(struct fwd_stream *fs)

print_ether_addr(" src=", &eth_hdr->s_addr);
print_ether_addr(" - dst=", &eth_hdr->d_addr);
- printf(" - type=0x%04x - length=%u - nb_segs=%d",
+ printf(" - type=0x%04x - length=%u - nb_segs=%d - port=%d",
eth_type, (unsigned) mb->pkt_len,
- (int)mb->nb_segs);
+ (int)mb->nb_segs, mb->port);
if (ol_flags & PKT_RX_RSS_HASH) {
printf(" - RSS hash=0x%x", (unsigned) mb->hash.rss);
printf(" - RSS queue=0x%x",(unsigned) fs->rx_queue);

Compile dpdk:

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

Prepare environment:

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

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

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

Configure testpmd:

set fwd rxonly
set verbose 1
start

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

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

After 128 packets, it's ok:

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

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

Olivier Matz

2017-08-31 13:40:12 UTC

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

Signed-off-by: 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

Olivier MATZ

2017-08-31 13:49:58 UTC

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.

Olivier Matz

2017-08-31 13:40:15 UTC

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

Signed-off-by: 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

Olivier MATZ

2017-08-31 13:51:16 UTC

Olivier Matz

2017-08-31 13:40:14 UTC

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

Signed-off-by: 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

Olivier MATZ

2017-08-31 13:50:38 UTC

Yuanhan Liu

2017-09-01 09:25:38 UTC

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

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

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

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

--yliu

Olivier MATZ

2017-09-01 09:58:07 UTC

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

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

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

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

Yuanhan Liu

2017-09-01 12:22:26 UTC

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

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

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

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.

Post by Olivier MATZ
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.

Post by Olivier MATZ
Thanks for the review.

Thanks for the work!

--yliu

Olivier Matz

2017-09-07 12:13:37 UTC

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

Olivier Matz

2017-09-07 12:13:38 UTC

--
2.11.0

Olivier Matz

2017-09-07 12:13:40 UTC

--
2.11.0

Olivier Matz

2017-09-07 12:13:39 UTC

--
2.11.0

Olivier Matz

2017-09-07 12:13:47 UTC

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 <***@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

Olivier Matz

2017-09-07 12:13:41 UTC

--
2.11.0

Olivier Matz

2017-09-07 12:13:46 UTC

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 <***@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

Olivier Matz

2017-09-07 12:13:43 UTC

--
2.11.0

Tiwei Bie

2017-12-06 05:25:29 UTC

Hi Maxime and Olivier:

On Thu, Sep 07, 2017 at 02:13:43PM +0200, Olivier Matz wrote:
[...]

Post by Olivier Matz
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

Olivier MATZ

2017-12-07 14:14:44 UTC

Hi Tiwei,

Post by Tiwei Bie
[...]

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

Tiwei Bie

2017-12-08 02:17:44 UTC

Hi Olivier,

Post by Tiwei Bie
[...]

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.

Post by Olivier MATZ
- 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
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

Yao, Lei A

2018-02-01 03:14:15 UTC

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-----
Sent: Thursday, September 7, 2017 8:14 PM
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
- 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")
---
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

Olivier Matz

2018-02-01 08:27:35 UTC

Hi Lei,

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

Olivier

Post by Yao, Lei A
Hi, Olivier
This is Lei from DPDK validation team in Intel. During our DPDK 18.02-rc1 test,
the traffic can't resume after stop/start the virtio device.
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

Xu, Qian Q

2018-02-07 08:31:07 UTC

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

-----Original Message-----
Sent: Thursday, February 1, 2018 4:28 PM
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

Post by Yao, Lei A
Hi, Olivier
This is Lei from DPDK validation team in Intel. During our DPDK
18.02-rc1 test, I find the following patch will cause one serious issue with virtio
the traffic can't resume after stop/start the virtio device.
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!

Post by Yao, Lei A
BRs
Lei

-----Original Message-----
Sent: Thursday, September 7, 2017 8:14 PM
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
- 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")
---
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) {

+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

Olivier Matz

2018-02-07 22:01:03 UTC

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

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

Thanks a lot!

Post by Yao, Lei A
BRs
Lei

-----Original Message-----
Sent: Thursday, September 7, 2017 8:14 PM
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
- 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")
---
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) {

+545,6

Wang, Zhihong

2018-02-09 05:44:56 UTC

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-----
Sent: Thursday, February 8, 2018 6:01 AM
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

Post by Xu, Qian Q
Any update, Olivier?
We are near to release, and the bug-fix is important for the virtio vector

path usage. Thanks.

-----Original Message-----
Sent: Thursday, February 1, 2018 4:28 PM
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

Post by Yao, Lei A
Hi, Olivier
This is Lei from DPDK validation team in Intel. During our DPDK
18.02-rc1 test, I find the following patch will cause one serious issue

with virtio

Post by Yao, Lei A
the traffic can't resume after stop/start the virtio device.
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!

Post by Yao, Lei A
BRs
Lei

++++++++++++++++++++++++++++++-

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

534,7

+545,6

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

Maxime Coquelin

2018-02-09 08:59:41 UTC

Hi Zhihong,

Post by Tiwei Bie
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

Post by Tiwei Bie
Thanks
-Zhihong

Post by Xu, Qian Q
Any update, Olivier?
We are near to release, and the bug-fix is important for the virtio vector

path usage. Thanks.

-----Original Message-----
Sent: Thursday, February 1, 2018 4:28 PM
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

Post by Yao, Lei A
Hi, Olivier
This is Lei from DPDK validation team in Intel. During our DPDK
18.02-rc1 test, I find the following patch will cause one serious issue

with virtio

Thanks a lot!

Post by Yao, Lei A
BRs
Lei

++++++++++++++++++++++++++++++-

-------
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,

rte_eth_dev *dev,

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

534,7

+545,6

rte_eth_dev

rte_eth_dev

*dev,
VIRTQUEUE_DUMP(vq);
- dev->data->tx_queues[queue_idx] = txvq;
return 0;
}
--
2.11.0

Maxime Coquelin

2018-02-09 09:40:39 UTC

Post by Maxime Coquelin
Hi Zhihong,

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

Post by Maxime Coquelin
Cheers,
Maxime

Post by Tiwei Bie
Thanks
-Zhihong

Post by Xu, Qian Q
Any update, Olivier?
We are near to release, and the bug-fix is important for the virtio vector

path usage. Thanks.

-----Original Message-----
Sent: Thursday, February 1, 2018 4:28 PM
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

Post by Yao, Lei A
Hi, Olivier
This is Lei from DPDK validation team in Intel. During our DPDK
18.02-rc1 test, I find the following patch will cause one serious issue

with virtio

Thanks a lot!

Post by Yao, Lei A
BRs
Lei

++++++++++++++++++++++++++++++-

-------
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,

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

534,7

+545,6

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

Olivier Matz

2017-09-07 12:13:45 UTC

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 <***@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

Olivier Matz

2017-09-07 12:13:44 UTC

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 <***@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

Olivier Matz

2017-09-07 12:13:42 UTC

--
2.11.0

Yuanhan Liu

2017-09-12 02:31:39 UTC