[dpdk-dev] [PATCH v2 0/5] net/virtio: Tx path selection and offload improvements

[dpdk-dev] [PATCH v2 0/5] net/virtio: Tx path selection and offload improvements

[Maxime Coquelin]

This series addresses a problem seen when running benchmars,
where we see a big difference in Tx performance depending on
whether Rx mergeable is enabled or not.
With patch 1, Tx simple path is selected even when Rx-mrg is
negotiated.

Digging a bit further, I found that Tx simple path could be
selected even when offload features had been negotiated.
With patch 2, guest does not try to negotiate Tx offload
features that haven't been selected by the application.
It means that without restarting the guest, we can now switch
from Tx simple path when application do no request offloads,
to Tx standard path when the application request offloads.
Note that to do so, one must stop the port first [0].
Another advantage of doing this than using Tx simple path
when guest application does not use offload features, is
that on host side, the virtio net header parsing is skipped
in dequeue function.

Patch 3 fixes an issue with Rx offload, as simple path was
be used if application requested TCP LRO only.

Finally, patch 4 aims at improving the offload feature checks
in the standard paths.

Below are benchmarks results when offloads are enabled at
device level and simple Tx is disabled (default).

Rx-mrg=off benchmarks:
+------------+-------+-------------+-------------+----------+
|    Run     |  PVP  | Guest->Host | Host->Guest | Loopback |
+------------+-------+-------------+-------------+----------+
| v18.05     | 14.47 |       17.02 |       17.57 |    13.15 |
| + series   | 11.73 |       13.90 |       17.50 |    13.19 |
+------------+-------+-------------+-------------+----------+

Rx-mrg=on benchmarks:
+------------+-------+-------------+-------------+----------+
|    Run     |  PVP  | Guest->Host | Host->Guest | Loopback |
+------------+-------+-------------+-------------+----------+
| v18.05     |  9.53 |       13.80 |       16.70 |    13.11 |
| + series   |  9.58 |       13.93 |       16.70 |    13.11 |
+------------+-------+-------------+-------------+----------+

Below are benchmarks results when offloads are enabled at
device level and simple Tx is unlocked (with passing
'simple_tx_support=1' as Virtio device devarg).

Rx-mrg=off benchmarks:
+------------+-------+-------------+-------------+----------+
|    Run     |  PVP  | Guest->Host | Host->Guest | Loopback |
+------------+-------+-------------+-------------+----------+
| v18.05     | 14.47 |       17.02 |       17.57 |    13.15 |
| + series   | 14.88 |       19.64 |       17.50 |    13.14 |
+------------+-------+-------------+-------------+----------+

Rx-mrg=on benchmarks:
+------------+-------+-------------+-------------+----------+
|    Run     |  PVP  | Guest->Host | Host->Guest | Loopback |
+------------+-------+-------------+-------------+----------+
| v18.05     |  9.53 |       13.80 |       16.70 |    13.11 |
| + series   | 12.62 |       19.69 |       16.70 |    13.11 |
+------------+-------+-------------+-------------+----------+


[0]:
testpmd> port start 0
EAL: Error disabling MSI-X interrupts for fd 17
set_rxtx_funcs(): virtio: using mergeable buffer Rx path on port 0
set_rxtx_funcs(): virtio: using simple Tx path on port 0
Port 0: 52:54:00:58:D7:01
Checking link statuses...
Done
testpmd> show port 0 tx_offload capabilities 
Tx Offloading Capabilities of port 0 :
  Per Queue :
  Per Port  : VLAN_INSERT UDP_CKSUM TCP_CKSUM TCP_TSO MULTI_SEGS
         
testpmd> show port 0 tx_offload configuration
Tx Offloading Configuration of port 0 :
  Port :
  Queue[ 0] :

testpmd> port stop 0
Stopping ports...
Checking link statuses...
Done                
testpmd> csum set tcp hw 0
Parse tunnel is off
IP checksum offload is sw
UDP checksum offload is sw
TCP checksum offload is hw
SCTP checksum offload is sw
Outer-Ip checksum offload is sw
testpmd> port start 0
Configuring Port 0 (socket 0)
EAL: Error disabling MSI-X interrupts for fd 17
set_rxtx_funcs(): virtio: using mergeable buffer Rx path on port 0
set_rxtx_funcs(): virtio: using standard Tx path on port 0
Port 0: 52:54:00:58:D7:01
Checking link statuses...
Done                                         
testpmd> show port 0 tx_offload configuration
Tx Offloading Configuration of port 0 :
  Port : TCP_CKSUM
  Queue[ 0] :


Changes in v2:
==============
- Introduce a devarg to disable simple Tx path by default (Tiwei)
- Use standard PATH if VLAN strip or insert offloads are requested (Tiwei)
- Use boolean instead of int/uint8_t for has_tx/rx_offload (Tiwei)

Maxime Coquelin (5):
  net/virtio: prevent simple Tx path selection by default
  net/virtio: use simple path for Tx even if Rx mergeable
  net/vhost: improve Tx path selection
  net/virtio: don't use simple Rx if TCP LRO or VLAN strip requested
  net/virtio: improve offload check performance

 doc/guides/nics/virtio.rst         |   9 +++
 drivers/net/virtio/virtio_ethdev.c | 117 +++++++++++++++++++++++++++++++++++--
 drivers/net/virtio/virtio_ethdev.h |   3 -
 drivers/net/virtio/virtio_pci.h    |   4 ++
 drivers/net/virtio/virtio_rxtx.c   |  31 ++--------
 5 files changed, 130 insertions(+), 34 deletions(-)

-- 
2.14.3

[dpdk-dev] [PATCH v2 1/5] net/virtio: prevent simple Tx path selection by default

[Maxime Coquelin]

Simple Tx path is not compliant with the Virtio specification,
as it assumes the device will use the descriptors in order.

VIRTIO_F_IN_ORDER feature has been introduced recently, but the
simple Tx path is not compliant with it as VIRTIO_F_IN_ORDER
requires that chained descriptors are used sequentially, which
is not the case in simple Tx path.

This patch introduces 'simple_tx_support' devarg to unlock
Tx simple path selection.

Reported-by: Tiwei Bie <tiwei.bie@intel.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 doc/guides/nics/virtio.rst         |  9 +++++
 drivers/net/virtio/virtio_ethdev.c | 72 +++++++++++++++++++++++++++++++++++++-
 drivers/net/virtio/virtio_pci.h    |  1 +
 3 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/doc/guides/nics/virtio.rst b/doc/guides/nics/virtio.rst
index 8922f9c0b..53ce1c12a 100644
--- a/doc/guides/nics/virtio.rst
+++ b/doc/guides/nics/virtio.rst
@@ -222,6 +222,9 @@ Tx callbacks:
 
 #. ``virtio_xmit_pkts_simple``:
    Vector version fixes the available ring indexes to optimize performance.
+   This implementation does not comply with the Virtio specification, and so
+   is not selectable by default. "simple_tx_support=1" devarg must be passed
+   to unlock it.
 
 
 By default, the non-vector callbacks are used:
@@ -331,3 +334,9 @@ The user can specify below argument in devargs.
     driver, and works as a HW vhost backend. This argument is used to specify
     a virtio device needs to work in vDPA mode.
     (Default: 0 (disabled))
+
+#.  ``simple_tx_support``:
+
+    This argument enables support for the simple Tx path, which is not
+    compliant with the Virtio specification.
+    (Default: 0 (disabled))
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 5833dad73..bdc4f09d5 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1331,6 +1331,8 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
 	if (hw->use_simple_tx) {
 		PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
 			eth_dev->data->port_id);
+		PMD_INIT_LOG(WARNING,
+				"virtio: simple Tx path does not comply with Virtio spec");
 		eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
 	} else {
 		PMD_INIT_LOG(INFO, "virtio: using standard Tx path on port %u",
@@ -1790,6 +1792,65 @@ rte_virtio_pmd_init(void)
 	rte_pci_register(&rte_virtio_pmd);
 }
 
+#define VIRTIO_SIMPLE_TX_SUPPORT "simple_tx_support"
+
+static int virtio_dev_args_check(const char *key, const char *val,
+		void *opaque)
+{
+	struct rte_eth_dev *dev = opaque;
+	struct virtio_hw *hw = dev->data->dev_private;
+	unsigned long tmp;
+	int ret = 0;
+
+	errno = 0;
+	tmp = strtoul(val, NULL, 0);
+	if (errno) {
+		PMD_INIT_LOG(INFO, "%s: \"%s\" is not a valid integer", key, val);
+		return errno;
+	}
+
+	if (strcmp(VIRTIO_SIMPLE_TX_SUPPORT, key) == 0)
+		hw->support_simple_tx = !!tmp;
+
+	return ret;
+}
+
+static int
+virtio_dev_args(struct rte_eth_dev *dev)
+{
+	struct rte_kvargs *kvlist;
+	struct rte_devargs *devargs;
+	const char *valid_args[] = {
+		VIRTIO_SIMPLE_TX_SUPPORT,
+		NULL,
+	};
+	int ret;
+	int i;
+
+	devargs = dev->device->devargs;
+	if (!devargs)
+		return 0; /* return success */
+
+	kvlist = rte_kvargs_parse(devargs->args, valid_args);
+	if (kvlist == NULL)
+		return -EINVAL;
+
+	 /* Process parameters. */
+	for (i = 0; (valid_args[i] != NULL); ++i) {
+		if (rte_kvargs_count(kvlist, valid_args[i])) {
+			ret = rte_kvargs_process(kvlist, valid_args[i],
+						 virtio_dev_args_check, dev);
+			if (ret) {
+				rte_kvargs_free(kvlist);
+				return ret;
+			}
+		}
+	}
+	rte_kvargs_free(kvlist);
+
+	return 0;
+}
+
 /*
  * Configure virtio device
  * It returns 0 on success.
@@ -1804,6 +1865,10 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 	int ret;
 
 	PMD_INIT_LOG(DEBUG, "configure");
+
+	if (virtio_dev_args(dev))
+		return -ENOTSUP;
+
 	req_features = VIRTIO_PMD_DEFAULT_GUEST_FEATURES;
 
 	if (dev->data->dev_conf.intr_conf.rxq) {
@@ -1869,7 +1934,12 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 	rte_spinlock_init(&hw->state_lock);
 
 	hw->use_simple_rx = 1;
-	hw->use_simple_tx = 1;
+	/*
+	 * Simple Tx does not comply with Virtio spec,
+	 * "simple_tx_support=1" devarg needs to be passed
+	 * to unlock it.
+	 */
+	hw->use_simple_tx = hw->support_simple_tx;
 
 #if defined RTE_ARCH_ARM64 || defined RTE_ARCH_ARM
 	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON)) {
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index a28ba8339..7318bb318 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -231,6 +231,7 @@ struct virtio_hw {
 	uint8_t	    vlan_strip;
 	uint8_t	    use_msix;
 	uint8_t     modern;
+	uint8_t	    support_simple_tx;
 	uint8_t     use_simple_rx;
 	uint8_t     use_simple_tx;
 	uint16_t    port_id;
-- 
2.14.3

[dpdk-dev] [PATCH v2 2/5] net/virtio: use simple path for Tx even if Rx mergeable

[Maxime Coquelin]

Having Rx mergeable buffers feature enabled should not be
a reason to not use Tx simple path.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index bdc4f09d5..73e6d6b6b 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1949,7 +1949,6 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 #endif
 	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
 		hw->use_simple_rx = 0;
-		hw->use_simple_tx = 0;
 	}
 
 	if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
-- 
2.14.3

[dpdk-dev] [PATCH v2 3/5] net/vhost: improve Tx path selection

[Maxime Coquelin]

This patch improves the Tx path selection depending on
whether the application request for offloads, and on whether
offload features have been negotiated.

When the application doesn't request for Tx offload features,
the corresponding features bits aren't negotiated.

When Tx offload virtio features have been negotiated, ensure
the simple Tx path isn't selected.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 21 +++++++++++++++++++--
 drivers/net/virtio/virtio_ethdev.h |  3 ---
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 73e6d6b6b..b023ec02e 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1859,8 +1859,10 @@ static int
 virtio_dev_configure(struct rte_eth_dev *dev)
 {
 	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
+	const struct rte_eth_txmode *txmode = &dev->data->dev_conf.txmode;
 	struct virtio_hw *hw = dev->data->dev_private;
 	uint64_t rx_offloads = rxmode->offloads;
+	uint64_t tx_offloads = txmode->offloads;
 	uint64_t req_features;
 	int ret;
 
@@ -1886,6 +1888,15 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			(1ULL << VIRTIO_NET_F_GUEST_TSO4) |
 			(1ULL << VIRTIO_NET_F_GUEST_TSO6);
 
+	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
+				DEV_TX_OFFLOAD_UDP_CKSUM))
+		req_features |= (1ULL << VIRTIO_NET_F_CSUM);
+
+	if (tx_offloads & DEV_TX_OFFLOAD_TCP_TSO)
+		req_features |=
+			(1ULL << VIRTIO_NET_F_HOST_TSO4) |
+			(1ULL << VIRTIO_NET_F_HOST_TSO6);
+
 	/* if request features changed, reinit the device */
 	if (req_features != hw->req_guest_features) {
 		ret = virtio_init_device(dev, req_features);
@@ -1955,6 +1966,12 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			   DEV_RX_OFFLOAD_TCP_CKSUM))
 		hw->use_simple_rx = 0;
 
+	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
+				DEV_TX_OFFLOAD_UDP_CKSUM |
+				DEV_TX_OFFLOAD_TCP_TSO |
+				DEV_TX_OFFLOAD_VLAN_INSERT))
+		hw->use_simple_tx = 0;
+
 	return 0;
 }
 
@@ -2208,14 +2225,14 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 
 	dev_info->tx_offload_capa = DEV_TX_OFFLOAD_MULTI_SEGS |
 				    DEV_TX_OFFLOAD_VLAN_INSERT;
-	if (hw->guest_features & (1ULL << VIRTIO_NET_F_CSUM)) {
+	if (host_features & (1ULL << VIRTIO_NET_F_CSUM)) {
 		dev_info->tx_offload_capa |=
 			DEV_TX_OFFLOAD_UDP_CKSUM |
 			DEV_TX_OFFLOAD_TCP_CKSUM;
 	}
 	tso_mask = (1ULL << VIRTIO_NET_F_HOST_TSO4) |
 		(1ULL << VIRTIO_NET_F_HOST_TSO6);
-	if ((hw->guest_features & tso_mask) == tso_mask)
+	if ((host_features & tso_mask) == tso_mask)
 		dev_info->tx_offload_capa |= DEV_TX_OFFLOAD_TCP_TSO;
 }
 
diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index bb40064ea..b603665c7 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -28,9 +28,6 @@
 	 1u << VIRTIO_NET_F_CTRL_VQ	  |	\
 	 1u << VIRTIO_NET_F_CTRL_RX	  |	\
 	 1u << VIRTIO_NET_F_CTRL_VLAN	  |	\
-	 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	| \
 	 1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE |	\
-- 
2.14.3

[dpdk-dev] [PATCH v2 4/5] net/virtio: don't use simple Rx if TCP LRO or VLAN strip requested

[Maxime Coquelin]

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index b023ec02e..357968fdd 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1963,7 +1963,9 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 	}
 
 	if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
-			   DEV_RX_OFFLOAD_TCP_CKSUM))
+			   DEV_RX_OFFLOAD_TCP_CKSUM |
+			   DEV_RX_OFFLOAD_TCP_LRO |
+			   DEV_RX_OFFLOAD_VLAN_STRIP))
 		hw->use_simple_rx = 0;
 
 	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
-- 
2.14.3

[dpdk-dev] [PATCH v2 5/5] net/virtio: improve offload check performance

[Maxime Coquelin]

Instead of checking the multiple Virtio features bits for
every packet, let's do the check once at configure time and
store it in virtio_hw struct.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 19 +++++++++++++++++++
 drivers/net/virtio/virtio_pci.h    |  3 +++
 drivers/net/virtio/virtio_rxtx.c   | 31 +++++--------------------------
 3 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 357968fdd..3d4b28be8 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1851,6 +1851,22 @@ virtio_dev_args(struct rte_eth_dev *dev)
 	return 0;
 }
 
+static bool
+rx_offload_enabled(struct virtio_hw *hw)
+{
+	return vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_CSUM) ||
+		vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4) ||
+		vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO6);
+}
+
+static bool
+tx_offload_enabled(struct virtio_hw *hw)
+{
+	return vtpci_with_feature(hw, VIRTIO_NET_F_CSUM) ||
+		vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO4) ||
+		vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO6);
+}
+
 /*
  * Configure virtio device
  * It returns 0 on success.
@@ -1934,6 +1950,9 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 		return -ENOTSUP;
 	}
 
+	hw->has_tx_offload = tx_offload_enabled(hw);
+	hw->has_rx_offload = rx_offload_enabled(hw);
+
 	if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
 		/* Enable vector (0) for Link State Intrerrupt */
 		if (VTPCI_OPS(hw)->set_config_irq(hw, 0) ==
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 7318bb318..337dc6180 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -6,6 +6,7 @@
 #define _VIRTIO_PCI_H_
 
 #include <stdint.h>
+#include <stdbool.h>
 
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
@@ -234,6 +235,8 @@ struct virtio_hw {
 	uint8_t	    support_simple_tx;
 	uint8_t     use_simple_rx;
 	uint8_t     use_simple_tx;
+	bool        has_tx_offload;
+	bool        has_rx_offload;
 	uint16_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 92fab2174..8579a7567 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -225,13 +225,6 @@ virtio_tso_fix_cksum(struct rte_mbuf *m)
 	}
 }
 
-static inline int
-tx_offload_enabled(struct virtio_hw *hw)
-{
-	return vtpci_with_feature(hw, VIRTIO_NET_F_CSUM) ||
-		vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO4) ||
-		vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO6);
-}
 
 /* avoid write operation when necessary, to lessen cache issues */
 #define ASSIGN_UNLESS_EQUAL(var, val) do {	\
@@ -251,9 +244,7 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
 	uint16_t head_idx, idx;
 	uint16_t head_size = vq->hw->vtnet_hdr_size;
 	struct virtio_net_hdr *hdr;
-	int offload;
 
-	offload = tx_offload_enabled(vq->hw);
 	head_idx = vq->vq_desc_head_idx;
 	idx = head_idx;
 	dxp = &vq->vq_descx[idx];
@@ -270,8 +261,7 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
 		 * which is wrong. Below subtract restores correct pkt size.
 		 */
 		cookie->pkt_len -= head_size;
-		/* if offload disabled, it is not zeroed below, do it now */
-		if (offload == 0) {
+		if (!vq->hw->has_tx_offload) {
 			ASSIGN_UNLESS_EQUAL(hdr->csum_start, 0);
 			ASSIGN_UNLESS_EQUAL(hdr->csum_offset, 0);
 			ASSIGN_UNLESS_EQUAL(hdr->flags, 0);
@@ -309,7 +299,7 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
 	}
 
 	/* Checksum Offload / TSO */
-	if (offload) {
+	if (vq->hw->has_tx_offload) {
 		if (cookie->ol_flags & PKT_TX_TCP_SEG)
 			cookie->ol_flags |= PKT_TX_TCP_CKSUM;
 
@@ -686,14 +676,6 @@ virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
 	return 0;
 }
 
-static inline int
-rx_offload_enabled(struct virtio_hw *hw)
-{
-	return vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_CSUM) ||
-		vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4) ||
-		vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO6);
-}
-
 #define VIRTIO_MBUF_BURST_SZ 64
 #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc))
 uint16_t
@@ -709,7 +691,6 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 	int error;
 	uint32_t i, nb_enqueued;
 	uint32_t hdr_size;
-	int offload;
 	struct virtio_net_hdr *hdr;
 
 	nb_rx = 0;
@@ -731,7 +712,6 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 
 	nb_enqueued = 0;
 	hdr_size = hw->vtnet_hdr_size;
-	offload = rx_offload_enabled(hw);
 
 	for (i = 0; i < num ; i++) {
 		rxm = rcv_pkts[i];
@@ -760,7 +740,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 		if (hw->vlan_strip)
 			rte_vlan_strip(rxm);
 
-		if (offload && virtio_rx_offload(rxm, hdr) < 0) {
+		if (hw->has_rx_offload && virtio_rx_offload(rxm, hdr) < 0) {
 			virtio_discard_rxbuf(vq, rxm);
 			rxvq->stats.errors++;
 			continue;
@@ -825,7 +805,6 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 	uint16_t extra_idx;
 	uint32_t seg_res;
 	uint32_t hdr_size;
-	int offload;
 
 	nb_rx = 0;
 	if (unlikely(hw->started == 0))
@@ -843,7 +822,6 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 	extra_idx = 0;
 	seg_res = 0;
 	hdr_size = hw->vtnet_hdr_size;
-	offload = rx_offload_enabled(hw);
 
 	while (i < nb_used) {
 		struct virtio_net_hdr_mrg_rxbuf *header;
@@ -888,7 +866,8 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 		rx_pkts[nb_rx] = rxm;
 		prev = rxm;
 
-		if (offload && virtio_rx_offload(rxm, &header->hdr) < 0) {
+		if (hw->has_rx_offload &&
+				virtio_rx_offload(rxm, &header->hdr) < 0) {
 			virtio_discard_rxbuf(vq, rxm);
 			rxvq->stats.errors++;
 			continue;
-- 
2.14.3

Re: [dpdk-dev] [PATCH v2 5/5] net/virtio: improve offload check performance

[Tiwei Bie]

On Wed, Jun 06, 2018 at 02:31:28PM +0200, Maxime Coquelin wrote:
> Instead of checking the multiple Virtio features bits for
> every packet, let's do the check once at configure time and
> store it in virtio_hw struct.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
[...]
> @@ -270,8 +261,7 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
>  		 * which is wrong. Below subtract restores correct pkt size.
>  		 */
>  		cookie->pkt_len -= head_size;
> -		/* if offload disabled, it is not zeroed below, do it now */

I think there is no need to remove this comment.

Apart from that,

Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>

> -		if (offload == 0) {
> +		if (!vq->hw->has_tx_offload) {
>  			ASSIGN_UNLESS_EQUAL(hdr->csum_start, 0);
>  			ASSIGN_UNLESS_EQUAL(hdr->csum_offset, 0);
>  			ASSIGN_UNLESS_EQUAL(hdr->flags, 0);
[...]

Re: [dpdk-dev] [PATCH v2 4/5] net/virtio: don't use simple Rx if TCP LRO or VLAN strip requested

[Tiwei Bie]

On Wed, Jun 06, 2018 at 02:31:27PM +0200, Maxime Coquelin wrote:
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>

> ---
>  drivers/net/virtio/virtio_ethdev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index b023ec02e..357968fdd 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1963,7 +1963,9 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>  	}
>  
>  	if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
> -			   DEV_RX_OFFLOAD_TCP_CKSUM))
> +			   DEV_RX_OFFLOAD_TCP_CKSUM |
> +			   DEV_RX_OFFLOAD_TCP_LRO |
> +			   DEV_RX_OFFLOAD_VLAN_STRIP))
>  		hw->use_simple_rx = 0;
>  
>  	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
> -- 
> 2.14.3
> 

Re: [dpdk-dev] [PATCH v2 3/5] net/vhost: improve Tx path selection

[Tiwei Bie]

On Wed, Jun 06, 2018 at 02:31:26PM +0200, Maxime Coquelin wrote:
[...]
> @@ -1886,6 +1888,15 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>  			(1ULL << VIRTIO_NET_F_GUEST_TSO4) |
>  			(1ULL << VIRTIO_NET_F_GUEST_TSO6);
>  
> +	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
> +				DEV_TX_OFFLOAD_UDP_CKSUM))
> +		req_features |= (1ULL << VIRTIO_NET_F_CSUM);

Hmm.. I still prefer to keep the DEV_TX_OFFLOAD_TCP_CKSUM
and DEV_TX_OFFLOAD_UDP_CKSUM aligned to make the coding
style consistent with the existing code of Rx:

1901 ¦       if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
1902 ¦       ¦       ¦          DEV_RX_OFFLOAD_TCP_CKSUM))
1903 ¦       ¦       req_features |= (1ULL << VIRTIO_NET_F_GUEST_CSUM);

But it's up to you.

> +
> +	if (tx_offloads & DEV_TX_OFFLOAD_TCP_TSO)
> +		req_features |=
> +			(1ULL << VIRTIO_NET_F_HOST_TSO4) |
> +			(1ULL << VIRTIO_NET_F_HOST_TSO6);
> +
>  	/* if request features changed, reinit the device */
>  	if (req_features != hw->req_guest_features) {
>  		ret = virtio_init_device(dev, req_features);
> @@ -1955,6 +1966,12 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>  			   DEV_RX_OFFLOAD_TCP_CKSUM))
>  		hw->use_simple_rx = 0;
>  
> +	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
> +				DEV_TX_OFFLOAD_UDP_CKSUM |
> +				DEV_TX_OFFLOAD_TCP_TSO |
> +				DEV_TX_OFFLOAD_VLAN_INSERT))
> +		hw->use_simple_tx = 0;

Ditto. Below is the code for Rx:

1987 ¦       if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
1988 ¦       ¦       ¦          DEV_RX_OFFLOAD_TCP_CKSUM |
1989 ¦       ¦       ¦          DEV_RX_OFFLOAD_TCP_LRO |
1990 ¦       ¦       ¦          DEV_RX_OFFLOAD_VLAN_STRIP))
1991 ¦       ¦       hw->use_simple_rx = 0;

So, instead of:

1987 ¦       if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
1988 ¦       ¦       ¦          DEV_RX_OFFLOAD_TCP_CKSUM |
1989 ¦       ¦       ¦          DEV_RX_OFFLOAD_TCP_LRO |
1990 ¦       ¦       ¦          DEV_RX_OFFLOAD_VLAN_STRIP))
1991 ¦       ¦       hw->use_simple_rx = 0;
1992
1993 ¦       if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
1994 ¦       ¦       ¦       ¦       DEV_TX_OFFLOAD_UDP_CKSUM |
1995 ¦       ¦       ¦       ¦       DEV_TX_OFFLOAD_TCP_TSO |
1996 ¦       ¦       ¦       ¦       DEV_TX_OFFLOAD_VLAN_INSERT))
1997 ¦       ¦       hw->use_simple_tx = 0;

I would prefer:

1987 ¦       if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
1988 ¦       ¦       ¦          DEV_RX_OFFLOAD_TCP_CKSUM |
1989 ¦       ¦       ¦          DEV_RX_OFFLOAD_TCP_LRO |
1990 ¦       ¦       ¦          DEV_RX_OFFLOAD_VLAN_STRIP))
1991 ¦       ¦       hw->use_simple_rx = 0;
1992
1993 ¦       if (tx_offloads & (DEV_TX_OFFLOAD_UDP_CKSUM |
1994 ¦       ¦       ¦          DEV_TX_OFFLOAD_TCP_CKSUM |
1995 ¦       ¦       ¦          DEV_TX_OFFLOAD_TCP_TSO |
1996 ¦       ¦       ¦          DEV_TX_OFFLOAD_VLAN_INSERT))
1997 ¦       ¦       hw->use_simple_tx = 0;

But anyway, it's up to you. :)

Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>

Best regards,
Tiwei Bie

Re: [dpdk-dev] [PATCH v2 2/5] net/virtio: use simple path for Tx even if Rx mergeable

[Tiwei Bie]

On Wed, Jun 06, 2018 at 02:31:25PM +0200, Maxime Coquelin wrote:
> Having Rx mergeable buffers feature enabled should not be
> a reason to not use Tx simple path.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index bdc4f09d5..73e6d6b6b 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1949,7 +1949,6 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>  #endif
>  	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
>  		hw->use_simple_rx = 0;
> -		hw->use_simple_tx = 0;
>  	}

Maybe it's better to also remove { and }.

Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>

>  
>  	if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
> -- 
> 2.14.3
> 

Re: [dpdk-dev] [PATCH v2 1/5] net/virtio: prevent simple Tx path selection by default

[Tiwei Bie]

On Wed, Jun 06, 2018 at 02:31:24PM +0200, Maxime Coquelin wrote:
[...]
> +
> +static int
> +virtio_dev_args(struct rte_eth_dev *dev)
> +{
> +	struct rte_kvargs *kvlist;
> +	struct rte_devargs *devargs;
> +	const char *valid_args[] = {
> +		VIRTIO_SIMPLE_TX_SUPPORT,
> +		NULL,
> +	};
> +	int ret;
> +	int i;
> +
> +	devargs = dev->device->devargs;
> +	if (!devargs)
> +		return 0; /* return success */
> +
> +	kvlist = rte_kvargs_parse(devargs->args, valid_args);
> +	if (kvlist == NULL)
> +		return -EINVAL;

Virtio-user has defined some other mandatory devargs.
The parse will fail when other devargs have been
specified.

> +
> +	 /* Process parameters. */
> +	for (i = 0; (valid_args[i] != NULL); ++i) {

There is an extra space before the comment.
The () around `valid_args[i] != NULL` isn't necessary.

> +		if (rte_kvargs_count(kvlist, valid_args[i])) {
> +			ret = rte_kvargs_process(kvlist, valid_args[i],
> +						 virtio_dev_args_check, dev);
> +			if (ret) {
> +				rte_kvargs_free(kvlist);
> +				return ret;
> +			}
> +		}
> +	}
> +	rte_kvargs_free(kvlist);
> +
> +	return 0;
> +}
> +
[...]

Re: [dpdk-dev] [PATCH v2 5/5] net/virtio: improve offload check performance

[Maxime Coquelin]

On 06/07/2018 06:51 AM, Tiwei Bie wrote:
> On Wed, Jun 06, 2018 at 02:31:28PM +0200, Maxime Coquelin wrote:
>> Instead of checking the multiple Virtio features bits for
>> every packet, let's do the check once at configure time and
>> store it in virtio_hw struct.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
> [...]
>> @@ -270,8 +261,7 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
>>   		 * which is wrong. Below subtract restores correct pkt size.
>>   		 */
>>   		cookie->pkt_len -= head_size;
>> -		/* if offload disabled, it is not zeroed below, do it now */
> 
> I think there is no need to remove this comment.

Oh right, that was not intentional.
Will add it again.

> Apart from that,
> 
> Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>


Thanks,
Maxime
>> -		if (offload == 0) {
>> +		if (!vq->hw->has_tx_offload) {
>>   			ASSIGN_UNLESS_EQUAL(hdr->csum_start, 0);
>>   			ASSIGN_UNLESS_EQUAL(hdr->csum_offset, 0);
>>   			ASSIGN_UNLESS_EQUAL(hdr->flags, 0);
> [...]
> 

Re: [dpdk-dev] [PATCH v2 3/5] net/vhost: improve Tx path selection

[Maxime Coquelin]

On 06/07/2018 07:13 AM, Tiwei Bie wrote:
> On Wed, Jun 06, 2018 at 02:31:26PM +0200, Maxime Coquelin wrote:
> [...]
>> @@ -1886,6 +1888,15 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>>   			(1ULL << VIRTIO_NET_F_GUEST_TSO4) |
>>   			(1ULL << VIRTIO_NET_F_GUEST_TSO6);
>>   
>> +	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
>> +				DEV_TX_OFFLOAD_UDP_CKSUM))
>> +		req_features |= (1ULL << VIRTIO_NET_F_CSUM);
> 
> Hmm.. I still prefer to keep the DEV_TX_OFFLOAD_TCP_CKSUM
> and DEV_TX_OFFLOAD_UDP_CKSUM aligned to make the coding
> style consistent with the existing code of Rx:
> 
> 1901 ¦       if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
> 1902 ¦       ¦       ¦          DEV_RX_OFFLOAD_TCP_CKSUM))
> 1903 ¦       ¦       req_features |= (1ULL << VIRTIO_NET_F_GUEST_CSUM);
> 
> But it's up to you.

Yes, it is better to keep same alignment, even if the rx one looks a bit
weird to me.

Also, I take the opportunity to switch UDP and TXP CKSUM defines for
consistency with Rx.

>> +
>> +	if (tx_offloads & DEV_TX_OFFLOAD_TCP_TSO)
>> +		req_features |=
>> +			(1ULL << VIRTIO_NET_F_HOST_TSO4) |
>> +			(1ULL << VIRTIO_NET_F_HOST_TSO6);
>> +
>>   	/* if request features changed, reinit the device */
>>   	if (req_features != hw->req_guest_features) {
>>   		ret = virtio_init_device(dev, req_features);
>> @@ -1955,6 +1966,12 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>>   			   DEV_RX_OFFLOAD_TCP_CKSUM))
>>   		hw->use_simple_rx = 0;
>>   
>> +	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
>> +				DEV_TX_OFFLOAD_UDP_CKSUM |
>> +				DEV_TX_OFFLOAD_TCP_TSO |
>> +				DEV_TX_OFFLOAD_VLAN_INSERT))
>> +		hw->use_simple_tx = 0;
> 
> Ditto. Below is the code for Rx:
> 
> 1987 ¦       if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
> 1988 ¦       ¦       ¦          DEV_RX_OFFLOAD_TCP_CKSUM |
> 1989 ¦       ¦       ¦          DEV_RX_OFFLOAD_TCP_LRO |
> 1990 ¦       ¦       ¦          DEV_RX_OFFLOAD_VLAN_STRIP))
> 1991 ¦       ¦       hw->use_simple_rx = 0;
> 
> So, instead of:
> 
> 1987 ¦       if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
> 1988 ¦       ¦       ¦          DEV_RX_OFFLOAD_TCP_CKSUM |
> 1989 ¦       ¦       ¦          DEV_RX_OFFLOAD_TCP_LRO |
> 1990 ¦       ¦       ¦          DEV_RX_OFFLOAD_VLAN_STRIP))
> 1991 ¦       ¦       hw->use_simple_rx = 0;
> 1992
> 1993 ¦       if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
> 1994 ¦       ¦       ¦       ¦       DEV_TX_OFFLOAD_UDP_CKSUM |
> 1995 ¦       ¦       ¦       ¦       DEV_TX_OFFLOAD_TCP_TSO |
> 1996 ¦       ¦       ¦       ¦       DEV_TX_OFFLOAD_VLAN_INSERT))
> 1997 ¦       ¦       hw->use_simple_tx = 0;
> 
> I would prefer:
> 
> 1987 ¦       if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
> 1988 ¦       ¦       ¦          DEV_RX_OFFLOAD_TCP_CKSUM |
> 1989 ¦       ¦       ¦          DEV_RX_OFFLOAD_TCP_LRO |
> 1990 ¦       ¦       ¦          DEV_RX_OFFLOAD_VLAN_STRIP))
> 1991 ¦       ¦       hw->use_simple_rx = 0;
> 1992
> 1993 ¦       if (tx_offloads & (DEV_TX_OFFLOAD_UDP_CKSUM |
> 1994 ¦       ¦       ¦          DEV_TX_OFFLOAD_TCP_CKSUM |
> 1995 ¦       ¦       ¦          DEV_TX_OFFLOAD_TCP_TSO |
> 1996 ¦       ¦       ¦          DEV_TX_OFFLOAD_VLAN_INSERT))
> 1997 ¦       ¦       hw->use_simple_tx = 0;
> 
> But anyway, it's up to you. :)

Yep, I fix it now.

> Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>

Thanks,
Maxime
> Best regards,
> Tiwei Bie
> 

Re: [dpdk-dev] [PATCH v2 1/5] net/virtio: prevent simple Tx path selection by default

[Maxime Coquelin]

On 06/07/2018 07:43 AM, Tiwei Bie wrote:
> On Wed, Jun 06, 2018 at 02:31:24PM +0200, Maxime Coquelin wrote:
> [...]
>> +
>> +static int
>> +virtio_dev_args(struct rte_eth_dev *dev)
>> +{
>> +	struct rte_kvargs *kvlist;
>> +	struct rte_devargs *devargs;
>> +	const char *valid_args[] = {
>> +		VIRTIO_SIMPLE_TX_SUPPORT,
>> +		NULL,
>> +	};
>> +	int ret;
>> +	int i;
>> +
>> +	devargs = dev->device->devargs;
>> +	if (!devargs)
>> +		return 0; /* return success */
>> +
>> +	kvlist = rte_kvargs_parse(devargs->args, valid_args);
>> +	if (kvlist == NULL)
>> +		return -EINVAL;
> 
> Virtio-user has defined some other mandatory devargs.
> The parse will fail when other devargs have been
> specified.

Ok, so IIUC, just returning 0 here should do the trick, right?

>> +
>> +	 /* Process parameters. */
>> +	for (i = 0; (valid_args[i] != NULL); ++i) {
> 
> There is an extra space before the comment.
> The () around `valid_args[i] != NULL` isn't necessary.

Fixed.

>> +		if (rte_kvargs_count(kvlist, valid_args[i])) {
>> +			ret = rte_kvargs_process(kvlist, valid_args[i],
>> +						 virtio_dev_args_check, dev);
>> +			if (ret) {
>> +				rte_kvargs_free(kvlist);
>> +				return ret;
>> +			}
>> +		}
>> +	}
>> +	rte_kvargs_free(kvlist);
>> +
>> +	return 0;
>> +}
>> +
> [...]
> 

Thanks!
Maxime

Re: [dpdk-dev] [PATCH v2 1/5] net/virtio: prevent simple Tx path selection by default

[Tiwei Bie]

On Thu, Jun 07, 2018 at 09:40:35AM +0200, Maxime Coquelin wrote:
> On 06/07/2018 07:43 AM, Tiwei Bie wrote:
> > On Wed, Jun 06, 2018 at 02:31:24PM +0200, Maxime Coquelin wrote:
> > [...]
> > > +
> > > +static int
> > > +virtio_dev_args(struct rte_eth_dev *dev)
> > > +{
> > > +	struct rte_kvargs *kvlist;
> > > +	struct rte_devargs *devargs;
> > > +	const char *valid_args[] = {
> > > +		VIRTIO_SIMPLE_TX_SUPPORT,
> > > +		NULL,
> > > +	};
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	devargs = dev->device->devargs;
> > > +	if (!devargs)
> > > +		return 0; /* return success */
> > > +
> > > +	kvlist = rte_kvargs_parse(devargs->args, valid_args);
> > > +	if (kvlist == NULL)
> > > +		return -EINVAL;
> > 
> > Virtio-user has defined some other mandatory devargs.
> > The parse will fail when other devargs have been
> > specified.
> 
> Ok, so IIUC, just returning 0 here should do the trick, right?

I think you can't just return 0 in this case,
because you still need to find and parse the
VIRTIO_SIMPLE_TX_SUPPORT devarg.

I didn't look into the kvargs code. It seems
that you can pass NULL as the second param
when calling rte_kvargs_parse(), i.e. just
get the KV list without valid keys check.

Best regards,
Tiwei Bie

> 
> > > +
> > > +	 /* Process parameters. */
> > > +	for (i = 0; (valid_args[i] != NULL); ++i) {
> > 
> > There is an extra space before the comment.
> > The () around `valid_args[i] != NULL` isn't necessary.
> 
> Fixed.
> 
> > > +		if (rte_kvargs_count(kvlist, valid_args[i])) {
> > > +			ret = rte_kvargs_process(kvlist, valid_args[i],
> > > +						 virtio_dev_args_check, dev);
> > > +			if (ret) {
> > > +				rte_kvargs_free(kvlist);
> > > +				return ret;
> > > +			}
> > > +		}
> > > +	}
> > > +	rte_kvargs_free(kvlist);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > [...]
> > 
> 
> Thanks!
> Maxime