[dpdk-dev,v3,1/5] net/virtio: forbid simple Tx path by default
diff mbox series

Message ID 20180607092616.27720-2-maxime.coquelin@redhat.com
State Superseded, archived
Delegated to: Maxime Coquelin
Headers show
Series
  • net/virtio: Tx path selection and offload improvements
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch warning coding style issues

Commit Message

Maxime Coquelin June 7, 2018, 9:26 a.m. UTC
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 | 73 +++++++++++++++++++++++++++++++++++++-
 drivers/net/virtio/virtio_pci.h    |  1 +
 3 files changed, 82 insertions(+), 1 deletion(-)

Comments

Tiwei Bie June 12, 2018, 6:35 a.m. UTC | #1
On Thu, Jun 07, 2018 at 11:26:12AM +0200, Maxime Coquelin wrote:
> 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 | 73 +++++++++++++++++++++++++++++++++++++-
>  drivers/net/virtio/virtio_pci.h    |  1 +
>  3 files changed, 82 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))

I tried this patch on my server. Virtio-user will
fail to probe when simple_tx_support is specified
dues to the check in virtio_user_pmd_probe():

PMD: Error parsing device, invalid key <simple_tx_support>
virtio_user_pmd_probe(): error when parsing param
vdev_probe(): failed to initialize virtio_user0 device
EAL: Bus (vdev) probe failed.


> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 5833dad73..052dd056a 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,66 @@ 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,
> +	};

checkpatch is complaining about above definition:

WARNING:STATIC_CONST_CHAR_ARRAY: char * array declaration might be
better as static const
#96: FILE: drivers/net/virtio/virtio_ethdev.c:1824:
+       const char *valid_args[] = {

Best regards,
Tiwei Bie
Maxime Coquelin June 12, 2018, 11:52 a.m. UTC | #2
On 06/12/2018 08:35 AM, Tiwei Bie wrote:
> On Thu, Jun 07, 2018 at 11:26:12AM +0200, Maxime Coquelin wrote:
>> 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 | 73 +++++++++++++++++++++++++++++++++++++-
>>   drivers/net/virtio/virtio_pci.h    |  1 +
>>   3 files changed, 82 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))
> 
> I tried this patch on my server. Virtio-user will
> fail to probe when simple_tx_support is specified
> dues to the check in virtio_user_pmd_probe():
> 
> PMD: Error parsing device, invalid key <simple_tx_support>
> virtio_user_pmd_probe(): error when parsing param
> vdev_probe(): failed to initialize virtio_user0 device
> EAL: Bus (vdev) probe failed.


Thanks for the heads-up, I hadn't tried with virtio-user.
I'll post a new version with this fixed soon.

> 
>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>> index 5833dad73..052dd056a 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,66 @@ 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,
>> +	};
> 
> checkpatch is complaining about above definition:
> 
> WARNING:STATIC_CONST_CHAR_ARRAY: char * array declaration might be
> better as static const
> #96: FILE: drivers/net/virtio/virtio_ethdev.c:1824:
> +       const char *valid_args[] = {

Yeah, I missed the warning when running my build scripts and noticed
it from the upstream CI mail.

It will be fixed in next version.

Thanks!
Maxime

> Best regards,
> Tiwei Bie
>

Patch
diff mbox series

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..052dd056a 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,66 @@  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, NULL);
+	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 +1866,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 +1935,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;