diff mbox series

[1/2] net/vhost: support TSO disabling

Message ID 1560924825-220648-2-git-send-email-noae@mellanox.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series support tso and mrg-rxbuf disabling |

Checks

Context Check Description
ci/Performance-Testing fail build patch failure
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Noa Ezra June 19, 2019, 6:13 a.m. UTC 
  TSO (TCP Segmentation Offload) is enabled by default on vhost.
Add the ability to disable TSO on vhost.
The user should also disable the feature on the virtual machine's xml.

Signed-off-by: Noa Ezra <noae@mellanox.com>
Reviewed-by: Matan Azrad <matan@mellanox.com>
---
 doc/guides/nics/vhost.rst         |  5 +++++
 drivers/net/vhost/rte_eth_vhost.c | 30 +++++++++++++++++++++++++++---
 2 files changed, 32 insertions(+), 3 deletions(-)

Comments

Maxime Coquelin June 19, 2019, 9:53 a.m. UTC | #1 
On 6/19/19 8:13 AM, Noa Ezra wrote:
> TSO (TCP Segmentation Offload) is enabled by default on vhost.
> Add the ability to disable TSO on vhost.
> The user should also disable the feature on the virtual machine's xml.

For TSO, I think it make sense to have the option to disable it,
as it requires the application to support it.

I even wonder whether it should not be disabled by default.
Any thoughts?

Also, next time, please add other maintainers as recipients,
it can be done by checking the MAINTAINERS file, or even by
using the get_maintainers.sh script.

Thanks,
Maxime

> Signed-off-by: Noa Ezra <noae@mellanox.com>
> Reviewed-by: Matan Azrad <matan@mellanox.com>
> ---
>   doc/guides/nics/vhost.rst         |  5 +++++
>   drivers/net/vhost/rte_eth_vhost.c | 30 +++++++++++++++++++++++++++---
>   2 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/guides/nics/vhost.rst b/doc/guides/nics/vhost.rst
> index 23f2e87..8cfda4d 100644
> --- a/doc/guides/nics/vhost.rst
> +++ b/doc/guides/nics/vhost.rst
> @@ -76,6 +76,11 @@ The user can specify below arguments in `--vdev` option.
>       It is used to enable postcopy live-migration support in vhost library.
>       (Default: 0 (disabled))
>   
> +#.  ``tso``:
> +
> +    It is used to disable tso support in vhost library.
> +    (Default: 1 (enabled))
> +
>   Vhost PMD event handling
>   ------------------------
>   
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index b2cda04..a38c235 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -31,6 +31,7 @@
>   #define ETH_VHOST_DEQUEUE_ZERO_COPY	"dequeue-zero-copy"
>   #define ETH_VHOST_IOMMU_SUPPORT		"iommu-support"
>   #define ETH_VHOST_POSTCOPY_SUPPORT	"postcopy-support"
> +#define ETH_VHOST_VIRTIO_NET_F_HOST_TSO "tso"
>   #define VHOST_MAX_PKT_BURST 32
>   
>   static const char *valid_arguments[] = {
> @@ -40,6 +41,7 @@
>   	ETH_VHOST_DEQUEUE_ZERO_COPY,
>   	ETH_VHOST_IOMMU_SUPPORT,
>   	ETH_VHOST_POSTCOPY_SUPPORT,
> +	ETH_VHOST_VIRTIO_NET_F_HOST_TSO,
>   	NULL
>   };
>   
> @@ -1200,7 +1202,8 @@ struct vhost_xstats_name_off {
>   
>   static int
>   eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
> -	int16_t queues, const unsigned int numa_node, uint64_t flags)
> +	int16_t queues, const unsigned int numa_node, uint64_t flags,
> +	uint64_t disable_flags)
>   {
>   	const char *name = rte_vdev_device_name(dev);
>   	struct rte_eth_dev_data *data;
> @@ -1272,6 +1275,11 @@ struct vhost_xstats_name_off {
>   	if (rte_vhost_driver_register(iface_name, flags))
>   		goto error;
>   
> +	if (disable_flags) {
> +		if (rte_vhost_driver_disable_features(iface_name, disable_flags))
> +			goto error;
> +	}
> +
>   	if (rte_vhost_driver_callback_register(iface_name, &vhost_ops) < 0) {
>   		VHOST_LOG(ERR, "Can't register callbacks\n");
>   		goto error;
> @@ -1334,10 +1342,12 @@ struct vhost_xstats_name_off {
>   	char *iface_name;
>   	uint16_t queues;
>   	uint64_t flags = 0;
> +	uint64_t disable_flags = 0;
>   	int client_mode = 0;
>   	int dequeue_zero_copy = 0;
>   	int iommu_support = 0;
>   	int postcopy_support = 0;
> +	int tso = 1;
>   	struct rte_eth_dev *eth_dev;
>   	const char *name = rte_vdev_device_name(dev);
>   
> @@ -1419,11 +1429,24 @@ struct vhost_xstats_name_off {
>   			flags |= RTE_VHOST_USER_POSTCOPY_SUPPORT;
>   	}
>   
> +	if (rte_kvargs_count(kvlist, ETH_VHOST_VIRTIO_NET_F_HOST_TSO) == 1) {
> +		ret = rte_kvargs_process(kvlist,
> +				ETH_VHOST_VIRTIO_NET_F_HOST_TSO,
> +				&open_int, &tso);
> +		if (ret < 0)
> +			goto out_free;
> +
> +		if (tso == 0) {
> +			disable_flags |= (1ULL << VIRTIO_NET_F_HOST_TSO4);
> +			disable_flags |= (1ULL << VIRTIO_NET_F_HOST_TSO6);
> +		}
> +	}
> +
>   	if (dev->device.numa_node == SOCKET_ID_ANY)
>   		dev->device.numa_node = rte_socket_id();
>   
>   	eth_dev_vhost_create(dev, iface_name, queues, dev->device.numa_node,
> -		flags);
> +		flags, disable_flags);
>   
>   out_free:
>   	rte_kvargs_free(kvlist);
> @@ -1470,7 +1493,8 @@ struct vhost_xstats_name_off {
>   	"client=<0|1> "
>   	"dequeue-zero-copy=<0|1> "
>   	"iommu-support=<0|1> "
> -	"postcopy-support=<0|1>");
> +	"postcopy-support=<0|1> "
> +	"tso=<0|1>");
>   
>   RTE_INIT(vhost_init_log)
>   {
>
Tiwei Bie June 20, 2019, 2:26 a.m. UTC | #2 
On Wed, Jun 19, 2019 at 11:53:05AM +0200, Maxime Coquelin wrote:
> On 6/19/19 8:13 AM, Noa Ezra wrote:
> > TSO (TCP Segmentation Offload) is enabled by default on vhost.
> > Add the ability to disable TSO on vhost.
> > The user should also disable the feature on the virtual machine's xml.
> 
> For TSO, I think it make sense to have the option to disable it,
> as it requires the application to support it.
> 
> I even wonder whether it should not be disabled by default.
> Any thoughts?

Agree, it looks better to disable it by default.

Thanks,
Tiwei
Matan Azrad June 20, 2019, 6:08 a.m. UTC | #3 
Hi Tiwei, Maxim
From: Tiwei Bie 
> Sent: Thursday, June 20, 2019 5:26 AM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>
> Cc: Noa Ezra <noae@mellanox.com>; Matan Azrad <matan@mellanox.com>;
> dev@dpdk.org
> Subject: Re: [PATCH 1/2] net/vhost: support TSO disabling
> 
> On Wed, Jun 19, 2019 at 11:53:05AM +0200, Maxime Coquelin wrote:
> > On 6/19/19 8:13 AM, Noa Ezra wrote:
> > > TSO (TCP Segmentation Offload) is enabled by default on vhost.
> > > Add the ability to disable TSO on vhost.
> > > The user should also disable the feature on the virtual machine's xml.
> >
> > For TSO, I think it make sense to have the option to disable it, as it
> > requires the application to support it.
> >
> > I even wonder whether it should not be disabled by default.
> > Any thoughts?
> 
> Agree, it looks better to disable it by default.

The issue to disable it by default is that it will break the current applications/users which use TSO by default.

Maybe we need kind of notice to the user to break it in the next version.

What do you think?  
 
> Thanks,
> Tiwei
Maxime Coquelin June 20, 2019, 7:25 a.m. UTC | #4 
On 6/20/19 8:08 AM, Matan Azrad wrote:
> 
> 
> Hi Tiwei, Maxim
> From: Tiwei Bie
>> Sent: Thursday, June 20, 2019 5:26 AM
>> To: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Cc: Noa Ezra <noae@mellanox.com>; Matan Azrad <matan@mellanox.com>;
>> dev@dpdk.org
>> Subject: Re: [PATCH 1/2] net/vhost: support TSO disabling
>>
>> On Wed, Jun 19, 2019 at 11:53:05AM +0200, Maxime Coquelin wrote:
>>> On 6/19/19 8:13 AM, Noa Ezra wrote:
>>>> TSO (TCP Segmentation Offload) is enabled by default on vhost.
>>>> Add the ability to disable TSO on vhost.
>>>> The user should also disable the feature on the virtual machine's xml.
>>>
>>> For TSO, I think it make sense to have the option to disable it, as it
>>> requires the application to support it.
>>>
>>> I even wonder whether it should not be disabled by default.
>>> Any thoughts?
>>
>> Agree, it looks better to disable it by default.
> 
> The issue to disable it by default is that it will break the current applications/users which use TSO by default.
> 
> Maybe we need kind of notice to the user to break it in the next version.
> 
> What do you think?


I think that's a good idea, let's add a notice in the release note for
v19.08, and disable it by default in 19.11.

Thanks,
Maxime

>> Thanks,
>> Tiwei
Maxime Coquelin Aug. 30, 2019, 8:44 a.m. UTC | #5 
On 6/19/19 8:13 AM, Noa Ezra wrote:
> TSO (TCP Segmentation Offload) is enabled by default on vhost.
> Add the ability to disable TSO on vhost.
> The user should also disable the feature on the virtual machine's xml.
> 
> Signed-off-by: Noa Ezra <noae@mellanox.com>
> Reviewed-by: Matan Azrad <matan@mellanox.com>
> ---
>  doc/guides/nics/vhost.rst         |  5 +++++
>  drivers/net/vhost/rte_eth_vhost.c | 30 +++++++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/guides/nics/vhost.rst b/doc/guides/nics/vhost.rst
> index 23f2e87..8cfda4d 100644
> --- a/doc/guides/nics/vhost.rst
> +++ b/doc/guides/nics/vhost.rst
> @@ -76,6 +76,11 @@ The user can specify below arguments in `--vdev` option.
>      It is used to enable postcopy live-migration support in vhost library.
>      (Default: 0 (disabled))
>  
> +#.  ``tso``:
> +
> +    It is used to disable tso support in vhost library.
> +    (Default: 1 (enabled))
> +
>  Vhost PMD event handling
>  ------------------------
>  
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index b2cda04..a38c235 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -31,6 +31,7 @@
>  #define ETH_VHOST_DEQUEUE_ZERO_COPY	"dequeue-zero-copy"
>  #define ETH_VHOST_IOMMU_SUPPORT		"iommu-support"
>  #define ETH_VHOST_POSTCOPY_SUPPORT	"postcopy-support"
> +#define ETH_VHOST_VIRTIO_NET_F_HOST_TSO "tso"
>  #define VHOST_MAX_PKT_BURST 32
>  
>  static const char *valid_arguments[] = {
> @@ -40,6 +41,7 @@
>  	ETH_VHOST_DEQUEUE_ZERO_COPY,
>  	ETH_VHOST_IOMMU_SUPPORT,
>  	ETH_VHOST_POSTCOPY_SUPPORT,
> +	ETH_VHOST_VIRTIO_NET_F_HOST_TSO,
>  	NULL
>  };
>  
> @@ -1200,7 +1202,8 @@ struct vhost_xstats_name_off {
>  
>  static int
>  eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
> -	int16_t queues, const unsigned int numa_node, uint64_t flags)
> +	int16_t queues, const unsigned int numa_node, uint64_t flags,
> +	uint64_t disable_flags)
>  {
>  	const char *name = rte_vdev_device_name(dev);
>  	struct rte_eth_dev_data *data;
> @@ -1272,6 +1275,11 @@ struct vhost_xstats_name_off {
>  	if (rte_vhost_driver_register(iface_name, flags))
>  		goto error;
>  
> +	if (disable_flags) {
> +		if (rte_vhost_driver_disable_features(iface_name, disable_flags))
> +			goto error;
> +	}
> +
>  	if (rte_vhost_driver_callback_register(iface_name, &vhost_ops) < 0) {
>  		VHOST_LOG(ERR, "Can't register callbacks\n");
>  		goto error;
> @@ -1334,10 +1342,12 @@ struct vhost_xstats_name_off {
>  	char *iface_name;
>  	uint16_t queues;
>  	uint64_t flags = 0;
> +	uint64_t disable_flags = 0;
>  	int client_mode = 0;
>  	int dequeue_zero_copy = 0;
>  	int iommu_support = 0;
>  	int postcopy_support = 0;
> +	int tso = 1;
>  	struct rte_eth_dev *eth_dev;
>  	const char *name = rte_vdev_device_name(dev);
>  
> @@ -1419,11 +1429,24 @@ struct vhost_xstats_name_off {
>  			flags |= RTE_VHOST_USER_POSTCOPY_SUPPORT;
>  	}
>  
> +	if (rte_kvargs_count(kvlist, ETH_VHOST_VIRTIO_NET_F_HOST_TSO) == 1) {
> +		ret = rte_kvargs_process(kvlist,
> +				ETH_VHOST_VIRTIO_NET_F_HOST_TSO,
> +				&open_int, &tso);
> +		if (ret < 0)
> +			goto out_free;
> +
> +		if (tso == 0) {
> +			disable_flags |= (1ULL << VIRTIO_NET_F_HOST_TSO4);
> +			disable_flags |= (1ULL << VIRTIO_NET_F_HOST_TSO6);
> +		}
> +	}
> +
>  	if (dev->device.numa_node == SOCKET_ID_ANY)
>  		dev->device.numa_node = rte_socket_id();
>  
>  	eth_dev_vhost_create(dev, iface_name, queues, dev->device.numa_node,
> -		flags);
> +		flags, disable_flags);
>  
>  out_free:
>  	rte_kvargs_free(kvlist);
> @@ -1470,7 +1493,8 @@ struct vhost_xstats_name_off {
>  	"client=<0|1> "
>  	"dequeue-zero-copy=<0|1> "
>  	"iommu-support=<0|1> "
> -	"postcopy-support=<0|1>");
> +	"postcopy-support=<0|1> "
> +	"tso=<0|1>");
>  
>  RTE_INIT(vhost_init_log)
>  {
> 

With changing the default to disabled:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Do you want me to do the change while applying or you prefer sending the
v2?

Thanks,
Maxime
Maxime Coquelin Sept. 30, 2019, 9:02 a.m. UTC | #6 
On 6/19/19 8:13 AM, Noa Ezra wrote:
> TSO (TCP Segmentation Offload) is enabled by default on vhost.
> Add the ability to disable TSO on vhost.
> The user should also disable the feature on the virtual machine's xml.
> 
> Signed-off-by: Noa Ezra <noae@mellanox.com>
> Reviewed-by: Matan Azrad <matan@mellanox.com>
> ---
>  doc/guides/nics/vhost.rst         |  5 +++++
>  drivers/net/vhost/rte_eth_vhost.c | 30 +++++++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 3 deletions(-)
> 



Applied to dpdk-next-virtio/master with changing the defaul to disabled.

Thanks,
Maxime
diff mbox series

Patch

diff --git a/doc/guides/nics/vhost.rst b/doc/guides/nics/vhost.rst
index 23f2e87..8cfda4d 100644
--- a/doc/guides/nics/vhost.rst
+++ b/doc/guides/nics/vhost.rst
@@ -76,6 +76,11 @@  The user can specify below arguments in `--vdev` option.
     It is used to enable postcopy live-migration support in vhost library.
     (Default: 0 (disabled))
 
+#.  ``tso``:
+
+    It is used to disable tso support in vhost library.
+    (Default: 1 (enabled))
+
 Vhost PMD event handling
 ------------------------
 
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index b2cda04..a38c235 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -31,6 +31,7 @@ 
 #define ETH_VHOST_DEQUEUE_ZERO_COPY	"dequeue-zero-copy"
 #define ETH_VHOST_IOMMU_SUPPORT		"iommu-support"
 #define ETH_VHOST_POSTCOPY_SUPPORT	"postcopy-support"
+#define ETH_VHOST_VIRTIO_NET_F_HOST_TSO "tso"
 #define VHOST_MAX_PKT_BURST 32
 
 static const char *valid_arguments[] = {
@@ -40,6 +41,7 @@ 
 	ETH_VHOST_DEQUEUE_ZERO_COPY,
 	ETH_VHOST_IOMMU_SUPPORT,
 	ETH_VHOST_POSTCOPY_SUPPORT,
+	ETH_VHOST_VIRTIO_NET_F_HOST_TSO,
 	NULL
 };
 
@@ -1200,7 +1202,8 @@  struct vhost_xstats_name_off {
 
 static int
 eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
-	int16_t queues, const unsigned int numa_node, uint64_t flags)
+	int16_t queues, const unsigned int numa_node, uint64_t flags,
+	uint64_t disable_flags)
 {
 	const char *name = rte_vdev_device_name(dev);
 	struct rte_eth_dev_data *data;
@@ -1272,6 +1275,11 @@  struct vhost_xstats_name_off {
 	if (rte_vhost_driver_register(iface_name, flags))
 		goto error;
 
+	if (disable_flags) {
+		if (rte_vhost_driver_disable_features(iface_name, disable_flags))
+			goto error;
+	}
+
 	if (rte_vhost_driver_callback_register(iface_name, &vhost_ops) < 0) {
 		VHOST_LOG(ERR, "Can't register callbacks\n");
 		goto error;
@@ -1334,10 +1342,12 @@  struct vhost_xstats_name_off {
 	char *iface_name;
 	uint16_t queues;
 	uint64_t flags = 0;
+	uint64_t disable_flags = 0;
 	int client_mode = 0;
 	int dequeue_zero_copy = 0;
 	int iommu_support = 0;
 	int postcopy_support = 0;
+	int tso = 1;
 	struct rte_eth_dev *eth_dev;
 	const char *name = rte_vdev_device_name(dev);
 
@@ -1419,11 +1429,24 @@  struct vhost_xstats_name_off {
 			flags |= RTE_VHOST_USER_POSTCOPY_SUPPORT;
 	}
 
+	if (rte_kvargs_count(kvlist, ETH_VHOST_VIRTIO_NET_F_HOST_TSO) == 1) {
+		ret = rte_kvargs_process(kvlist,
+				ETH_VHOST_VIRTIO_NET_F_HOST_TSO,
+				&open_int, &tso);
+		if (ret < 0)
+			goto out_free;
+
+		if (tso == 0) {
+			disable_flags |= (1ULL << VIRTIO_NET_F_HOST_TSO4);
+			disable_flags |= (1ULL << VIRTIO_NET_F_HOST_TSO6);
+		}
+	}
+
 	if (dev->device.numa_node == SOCKET_ID_ANY)
 		dev->device.numa_node = rte_socket_id();
 
 	eth_dev_vhost_create(dev, iface_name, queues, dev->device.numa_node,
-		flags);
+		flags, disable_flags);
 
 out_free:
 	rte_kvargs_free(kvlist);
@@ -1470,7 +1493,8 @@  struct vhost_xstats_name_off {
 	"client=<0|1> "
 	"dequeue-zero-copy=<0|1> "
 	"iommu-support=<0|1> "
-	"postcopy-support=<0|1>");
+	"postcopy-support=<0|1> "
+	"tso=<0|1>");
 
 RTE_INIT(vhost_init_log)
 {