[dpdk-dev] [PATCH 0/2] support tso and mrg-rxbuf disabling

[dpdk-dev] [PATCH 0/2] support tso and mrg-rxbuf disabling

[Noa Ezra]

Add support for disabling TSO and mrg-rxbuf in vhost.

Noa Ezra (2):
  net/vhost: support TSO disabling
  net/vhost: support mrg-rxbuf disabling

 doc/guides/nics/vhost.rst         | 10 +++++++++
 drivers/net/vhost/rte_eth_vhost.c | 45 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 52 insertions(+), 3 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH 1/2] net/vhost: support TSO disabling

[Noa Ezra]

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

[dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Noa Ezra]

Rx mergeable buffers is a virtio feature that allows chaining of
multiple virtio descriptors to handle large packet size.
This behavior is supported and enabled by default, however in case
the user knows that rx mergeable buffers are not needed, he can disable
the feature.
The user should also set mrg_rxbuf=off in 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 | 17 ++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/doc/guides/nics/vhost.rst b/doc/guides/nics/vhost.rst
index 8cfda4d..2a455b5 100644
--- a/doc/guides/nics/vhost.rst
+++ b/doc/guides/nics/vhost.rst
@@ -81,6 +81,11 @@ The user can specify below arguments in `--vdev` option.
     It is used to disable tso support in vhost library.
     (Default: 1 (enabled))
 
+#.  ``mrg-rxbuf``:
+
+    It is used to disable mrg rxbuf 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 a38c235..9a54020 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -32,6 +32,7 @@
 #define ETH_VHOST_IOMMU_SUPPORT		"iommu-support"
 #define ETH_VHOST_POSTCOPY_SUPPORT	"postcopy-support"
 #define ETH_VHOST_VIRTIO_NET_F_HOST_TSO "tso"
+#define ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF "mrg-rxbuf"
 #define VHOST_MAX_PKT_BURST 32
 
 static const char *valid_arguments[] = {
@@ -42,6 +43,7 @@
 	ETH_VHOST_IOMMU_SUPPORT,
 	ETH_VHOST_POSTCOPY_SUPPORT,
 	ETH_VHOST_VIRTIO_NET_F_HOST_TSO,
+	ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF,
 	NULL
 };
 
@@ -1348,6 +1350,7 @@ struct vhost_xstats_name_off {
 	int iommu_support = 0;
 	int postcopy_support = 0;
 	int tso = 1;
+	int mrg_rxbuf = 1;
 	struct rte_eth_dev *eth_dev;
 	const char *name = rte_vdev_device_name(dev);
 
@@ -1442,6 +1445,17 @@ struct vhost_xstats_name_off {
 		}
 	}
 
+	if (rte_kvargs_count(kvlist, ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF) == 1) {
+		ret = rte_kvargs_process(kvlist,
+				ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF,
+				&open_int, &mrg_rxbuf);
+		if (ret < 0)
+			goto out_free;
+
+ 		if (mrg_rxbuf == 0)
+			disable_flags |= (1ULL << VIRTIO_NET_F_MRG_RXBUF);
+	}
+
 	if (dev->device.numa_node == SOCKET_ID_ANY)
 		dev->device.numa_node = rte_socket_id();
 
@@ -1494,7 +1508,8 @@ struct vhost_xstats_name_off {
 	"dequeue-zero-copy=<0|1> "
 	"iommu-support=<0|1> "
 	"postcopy-support=<0|1> "
-	"tso=<0|1>");
+	"tso=<0|1> "
+	"mrg-rxbuf=<0|1>");
 
 RTE_INIT(vhost_init_log)
 {
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Maxime Coquelin]

Hi Noa,

On 6/19/19 8:13 AM, Noa Ezra wrote:
> Rx mergeable buffers is a virtio feature that allows chaining of
> multiple virtio descriptors to handle large packet size.
> This behavior is supported and enabled by default, however in case
> the user knows that rx mergeable buffers are not needed, he can disable
> the feature.
> The user should also set mrg_rxbuf=off in virtual machine's xml.

I'm not sure to understand why it is needed, as the vhost-user library
supports the feature, it's better to let it being advertised.

As you say, it is up to the user to disable it in the VM's XML.
Done this way, the feature won't be negotiated.

Moreover, the proposal deadline for v19.08 is over by more than 2 weeks.

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 | 17 ++++++++++++++++-
>   2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/guides/nics/vhost.rst b/doc/guides/nics/vhost.rst
> index 8cfda4d..2a455b5 100644
> --- a/doc/guides/nics/vhost.rst
> +++ b/doc/guides/nics/vhost.rst
> @@ -81,6 +81,11 @@ The user can specify below arguments in `--vdev` option.
>       It is used to disable tso support in vhost library.
>       (Default: 1 (enabled))
>   
> +#.  ``mrg-rxbuf``:
> +
> +    It is used to disable mrg rxbuf 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 a38c235..9a54020 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -32,6 +32,7 @@
>   #define ETH_VHOST_IOMMU_SUPPORT		"iommu-support"
>   #define ETH_VHOST_POSTCOPY_SUPPORT	"postcopy-support"
>   #define ETH_VHOST_VIRTIO_NET_F_HOST_TSO "tso"
> +#define ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF "mrg-rxbuf"
>   #define VHOST_MAX_PKT_BURST 32
>   
>   static const char *valid_arguments[] = {
> @@ -42,6 +43,7 @@
>   	ETH_VHOST_IOMMU_SUPPORT,
>   	ETH_VHOST_POSTCOPY_SUPPORT,
>   	ETH_VHOST_VIRTIO_NET_F_HOST_TSO,
> +	ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF,
>   	NULL
>   };
>   
> @@ -1348,6 +1350,7 @@ struct vhost_xstats_name_off {
>   	int iommu_support = 0;
>   	int postcopy_support = 0;
>   	int tso = 1;
> +	int mrg_rxbuf = 1;
>   	struct rte_eth_dev *eth_dev;
>   	const char *name = rte_vdev_device_name(dev);
>   
> @@ -1442,6 +1445,17 @@ struct vhost_xstats_name_off {
>   		}
>   	}
>   
> +	if (rte_kvargs_count(kvlist, ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF) == 1) {
> +		ret = rte_kvargs_process(kvlist,
> +				ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF,
> +				&open_int, &mrg_rxbuf);
> +		if (ret < 0)
> +			goto out_free;
> +
> + 		if (mrg_rxbuf == 0)
> +			disable_flags |= (1ULL << VIRTIO_NET_F_MRG_RXBUF);
> +	}
> +
>   	if (dev->device.numa_node == SOCKET_ID_ANY)
>   		dev->device.numa_node = rte_socket_id();
>   
> @@ -1494,7 +1508,8 @@ struct vhost_xstats_name_off {
>   	"dequeue-zero-copy=<0|1> "
>   	"iommu-support=<0|1> "
>   	"postcopy-support=<0|1> "
> -	"tso=<0|1>");
> +	"tso=<0|1> "
> +	"mrg-rxbuf=<0|1>");
>   
>   RTE_INIT(vhost_init_log)
>   {
> 

Re: [dpdk-dev] [PATCH 1/2] net/vhost: support TSO disabling

[Maxime Coquelin]

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

Re: [dpdk-dev] [PATCH 0/2] support tso and mrg-rxbuf disabling

[Aaron Conole]

Noa Ezra <noae@mellanox.com> writes:

> Add support for disabling TSO and mrg-rxbuf in vhost.
>
> Noa Ezra (2):
>   net/vhost: support TSO disabling
>   net/vhost: support mrg-rxbuf disabling
>
>  doc/guides/nics/vhost.rst         | 10 +++++++++
>  drivers/net/vhost/rte_eth_vhost.c | 45 ++++++++++++++++++++++++++++++++++++---
>  2 files changed, 52 insertions(+), 3 deletions(-)

Some errors building this series (In addition to the comments provided by Maxime):

https://travis-ci.com/ovsrobot/dpdk/builds/116071254

Re: [dpdk-dev] [PATCH 1/2] net/vhost: support TSO disabling

[Tiwei Bie]

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

Re: [dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Noa Ezra]

Hi Maxime,
Thanks for your comment, please see below.

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Wednesday, June 19, 2019 12:10 PM
> To: Noa Ezra <noae@mellanox.com>
> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
> Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
> 
> Hi Noa,
> 
> On 6/19/19 8:13 AM, Noa Ezra wrote:
> > Rx mergeable buffers is a virtio feature that allows chaining of
> > multiple virtio descriptors to handle large packet size.
> > This behavior is supported and enabled by default, however in case the
> > user knows that rx mergeable buffers are not needed, he can disable
> > the feature.
> > The user should also set mrg_rxbuf=off in virtual machine's xml.
> 
> I'm not sure to understand why it is needed, as the vhost-user library
> supports the feature, it's better to let it being advertised.
> 
> As you say, it is up to the user to disable it in the VM's XML.
> Done this way, the feature won't be negotiated.
> 
I agree with you, I'll remove this patch from the series.

> Moreover, the proposal deadline for v19.08 is over by more than 2 weeks.
I'm sorry, the mail was sent  a long time but didn't get to the mailing list.
In any case, I'll prepare a new patch with only TSO (the first patch) and send it to the next release.
 
> 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 | 17 ++++++++++++++++-
> >   2 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/doc/guides/nics/vhost.rst b/doc/guides/nics/vhost.rst
> > index 8cfda4d..2a455b5 100644
> > --- a/doc/guides/nics/vhost.rst
> > +++ b/doc/guides/nics/vhost.rst
> > @@ -81,6 +81,11 @@ The user can specify below arguments in `--vdev`
> option.
> >       It is used to disable tso support in vhost library.
> >       (Default: 1 (enabled))
> >
> > +#.  ``mrg-rxbuf``:
> > +
> > +    It is used to disable mrg rxbuf 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 a38c235..9a54020 100644
> > --- a/drivers/net/vhost/rte_eth_vhost.c
> > +++ b/drivers/net/vhost/rte_eth_vhost.c
> > @@ -32,6 +32,7 @@
> >   #define ETH_VHOST_IOMMU_SUPPORT		"iommu-support"
> >   #define ETH_VHOST_POSTCOPY_SUPPORT	"postcopy-support"
> >   #define ETH_VHOST_VIRTIO_NET_F_HOST_TSO "tso"
> > +#define ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF "mrg-rxbuf"
> >   #define VHOST_MAX_PKT_BURST 32
> >
> >   static const char *valid_arguments[] = { @@ -42,6 +43,7 @@
> >   	ETH_VHOST_IOMMU_SUPPORT,
> >   	ETH_VHOST_POSTCOPY_SUPPORT,
> >   	ETH_VHOST_VIRTIO_NET_F_HOST_TSO,
> > +	ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF,
> >   	NULL
> >   };
> >
> > @@ -1348,6 +1350,7 @@ struct vhost_xstats_name_off {
> >   	int iommu_support = 0;
> >   	int postcopy_support = 0;
> >   	int tso = 1;
> > +	int mrg_rxbuf = 1;
> >   	struct rte_eth_dev *eth_dev;
> >   	const char *name = rte_vdev_device_name(dev);
> >
> > @@ -1442,6 +1445,17 @@ struct vhost_xstats_name_off {
> >   		}
> >   	}
> >
> > +	if (rte_kvargs_count(kvlist,
> ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF) == 1) {
> > +		ret = rte_kvargs_process(kvlist,
> > +				ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF,
> > +				&open_int, &mrg_rxbuf);
> > +		if (ret < 0)
> > +			goto out_free;
> > +
> > + 		if (mrg_rxbuf == 0)
> > +			disable_flags |= (1ULL <<
> VIRTIO_NET_F_MRG_RXBUF);
> > +	}
> > +
> >   	if (dev->device.numa_node == SOCKET_ID_ANY)
> >   		dev->device.numa_node = rte_socket_id();
> >
> > @@ -1494,7 +1508,8 @@ struct vhost_xstats_name_off {
> >   	"dequeue-zero-copy=<0|1> "
> >   	"iommu-support=<0|1> "
> >   	"postcopy-support=<0|1> "
> > -	"tso=<0|1>");
> > +	"tso=<0|1> "
> > +	"mrg-rxbuf=<0|1>");
> >
> >   RTE_INIT(vhost_init_log)
> >   {
> >

Re: [dpdk-dev] [PATCH 1/2] net/vhost: support TSO disabling

[Matan Azrad]

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

Re: [dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Matan Azrad]

Hi all

> -----Original Message-----
> From: Noa Ezra
> Sent: Thursday, June 20, 2019 8:58 AM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>
> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
> Subject: RE: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
> 
> Hi Maxime,
> Thanks for your comment, please see below.
> 
> > -----Original Message-----
> > From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> > Sent: Wednesday, June 19, 2019 12:10 PM
> > To: Noa Ezra <noae@mellanox.com>
> > Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
> > Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
> >
> > Hi Noa,
> >
> > On 6/19/19 8:13 AM, Noa Ezra wrote:
> > > Rx mergeable buffers is a virtio feature that allows chaining of
> > > multiple virtio descriptors to handle large packet size.
> > > This behavior is supported and enabled by default, however in case
> > > the user knows that rx mergeable buffers are not needed, he can
> > > disable the feature.
> > > The user should also set mrg_rxbuf=off in virtual machine's xml.
> >
> > I'm not sure to understand why it is needed, as the vhost-user library
> > supports the feature, it's better to let it being advertised.
> >
> > As you say, it is up to the user to disable it in the VM's XML.
> > Done this way, the feature won't be negotiated.
> >
> I agree with you, I'll remove this patch from the series.

Are you sure that no performance impact exists for redundant merg-rx-buf configuration here?
What if the second side want it and the current side no?
It may be that the vhost PMD user may want to disable it to save performance from some reasons, no?

Re: [dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Maxime Coquelin]

On 6/20/19 8:52 AM, Matan Azrad wrote:
> Hi all
> 
>> -----Original Message-----
>> From: Noa Ezra
>> Sent: Thursday, June 20, 2019 8:58 AM
>> To: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
>> Subject: RE: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
>>
>> Hi Maxime,
>> Thanks for your comment, please see below.
>>
>>> -----Original Message-----
>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>> Sent: Wednesday, June 19, 2019 12:10 PM
>>> To: Noa Ezra <noae@mellanox.com>
>>> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
>>> Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
>>>
>>> Hi Noa,
>>>
>>> On 6/19/19 8:13 AM, Noa Ezra wrote:
>>>> Rx mergeable buffers is a virtio feature that allows chaining of
>>>> multiple virtio descriptors to handle large packet size.
>>>> This behavior is supported and enabled by default, however in case
>>>> the user knows that rx mergeable buffers are not needed, he can
>>>> disable the feature.
>>>> The user should also set mrg_rxbuf=off in virtual machine's xml.
>>>
>>> I'm not sure to understand why it is needed, as the vhost-user library
>>> supports the feature, it's better to let it being advertised.
>>>
>>> As you say, it is up to the user to disable it in the VM's XML.
>>> Done this way, the feature won't be negotiated.
>>>
>> I agree with you, I'll remove this patch from the series.
> 
> Are you sure that no performance impact exists for redundant merg-rx-buf configuration here?

I'm not sure to understand what you mean, could you please elaborate?

> What if the second side want it and the current side no?

The feature won't be negotiated, assuming it has been disabled in QEMU
cmdline (or via libvirt).

> It may be that the vhost PMD user may want to disable it to save performance from some reasons, no?
> 

Then this user should disable it at QEMU level.

Regards,
Maxime

Re: [dpdk-dev] [PATCH 1/2] net/vhost: support TSO disabling

[Maxime Coquelin]

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

Re: [dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Maxime Coquelin]

On 6/20/19 7:57 AM, Noa Ezra wrote:
> Hi Maxime,
> Thanks for your comment, please see below.
> 
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Wednesday, June 19, 2019 12:10 PM
>> To: Noa Ezra <noae@mellanox.com>
>> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
>> Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
>>
>> Hi Noa,
>>
>> On 6/19/19 8:13 AM, Noa Ezra wrote:
>>> Rx mergeable buffers is a virtio feature that allows chaining of
>>> multiple virtio descriptors to handle large packet size.
>>> This behavior is supported and enabled by default, however in case the
>>> user knows that rx mergeable buffers are not needed, he can disable
>>> the feature.
>>> The user should also set mrg_rxbuf=off in virtual machine's xml.
>>
>> I'm not sure to understand why it is needed, as the vhost-user library
>> supports the feature, it's better to let it being advertised.
>>
>> As you say, it is up to the user to disable it in the VM's XML.
>> Done this way, the feature won't be negotiated.
>>
> I agree with you, I'll remove this patch from the series.
> 
>> Moreover, the proposal deadline for v19.08 is over by more than 2 weeks.
> I'm sorry, the mail was sent  a long time but didn't get to the mailing list.

No problem, I just wanted to make you aware I could not take it anyway
in v19.08.

> In any case, I'll prepare a new patch with only TSO (the first patch) and send it to the next release.

As I replied to Matan, could you add the notice for the TSO disabled by
default in the release not for this release?

Thanks,
Maxime
>> 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 | 17 ++++++++++++++++-
>>>    2 files changed, 21 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/doc/guides/nics/vhost.rst b/doc/guides/nics/vhost.rst
>>> index 8cfda4d..2a455b5 100644
>>> --- a/doc/guides/nics/vhost.rst
>>> +++ b/doc/guides/nics/vhost.rst
>>> @@ -81,6 +81,11 @@ The user can specify below arguments in `--vdev`
>> option.
>>>        It is used to disable tso support in vhost library.
>>>        (Default: 1 (enabled))
>>>
>>> +#.  ``mrg-rxbuf``:
>>> +
>>> +    It is used to disable mrg rxbuf 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 a38c235..9a54020 100644
>>> --- a/drivers/net/vhost/rte_eth_vhost.c
>>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>>> @@ -32,6 +32,7 @@
>>>    #define ETH_VHOST_IOMMU_SUPPORT		"iommu-support"
>>>    #define ETH_VHOST_POSTCOPY_SUPPORT	"postcopy-support"
>>>    #define ETH_VHOST_VIRTIO_NET_F_HOST_TSO "tso"
>>> +#define ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF "mrg-rxbuf"
>>>    #define VHOST_MAX_PKT_BURST 32
>>>
>>>    static const char *valid_arguments[] = { @@ -42,6 +43,7 @@
>>>    	ETH_VHOST_IOMMU_SUPPORT,
>>>    	ETH_VHOST_POSTCOPY_SUPPORT,
>>>    	ETH_VHOST_VIRTIO_NET_F_HOST_TSO,
>>> +	ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF,
>>>    	NULL
>>>    };
>>>
>>> @@ -1348,6 +1350,7 @@ struct vhost_xstats_name_off {
>>>    	int iommu_support = 0;
>>>    	int postcopy_support = 0;
>>>    	int tso = 1;
>>> +	int mrg_rxbuf = 1;
>>>    	struct rte_eth_dev *eth_dev;
>>>    	const char *name = rte_vdev_device_name(dev);
>>>
>>> @@ -1442,6 +1445,17 @@ struct vhost_xstats_name_off {
>>>    		}
>>>    	}
>>>
>>> +	if (rte_kvargs_count(kvlist,
>> ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF) == 1) {
>>> +		ret = rte_kvargs_process(kvlist,
>>> +				ETH_VHOST_VIRTIO_NET_F_MRG_RXBUF,
>>> +				&open_int, &mrg_rxbuf);
>>> +		if (ret < 0)
>>> +			goto out_free;
>>> +
>>> + 		if (mrg_rxbuf == 0)
>>> +			disable_flags |= (1ULL <<
>> VIRTIO_NET_F_MRG_RXBUF);
>>> +	}
>>> +
>>>    	if (dev->device.numa_node == SOCKET_ID_ANY)
>>>    		dev->device.numa_node = rte_socket_id();
>>>
>>> @@ -1494,7 +1508,8 @@ struct vhost_xstats_name_off {
>>>    	"dequeue-zero-copy=<0|1> "
>>>    	"iommu-support=<0|1> "
>>>    	"postcopy-support=<0|1> "
>>> -	"tso=<0|1>");
>>> +	"tso=<0|1> "
>>> +	"mrg-rxbuf=<0|1>");
>>>
>>>    RTE_INIT(vhost_init_log)
>>>    {
>>>

Re: [dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Matan Azrad]

From: Maxime Coquelin
> Sent: Thursday, June 20, 2019 10:19 AM
> To: Matan Azrad <matan@mellanox.com>; Noa Ezra <noae@mellanox.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
> 
> 
> 
> On 6/20/19 8:52 AM, Matan Azrad wrote:
> > Hi all
> >
> >> -----Original Message-----
> >> From: Noa Ezra
> >> Sent: Thursday, June 20, 2019 8:58 AM
> >> To: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
> >> Subject: RE: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
> >>
> >> Hi Maxime,
> >> Thanks for your comment, please see below.
> >>
> >>> -----Original Message-----
> >>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> >>> Sent: Wednesday, June 19, 2019 12:10 PM
> >>> To: Noa Ezra <noae@mellanox.com>
> >>> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
> >>> Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
> >>>
> >>> Hi Noa,
> >>>
> >>> On 6/19/19 8:13 AM, Noa Ezra wrote:
> >>>> Rx mergeable buffers is a virtio feature that allows chaining of
> >>>> multiple virtio descriptors to handle large packet size.
> >>>> This behavior is supported and enabled by default, however in case
> >>>> the user knows that rx mergeable buffers are not needed, he can
> >>>> disable the feature.
> >>>> The user should also set mrg_rxbuf=off in virtual machine's xml.
> >>>
> >>> I'm not sure to understand why it is needed, as the vhost-user
> >>> library supports the feature, it's better to let it being advertised.
> >>>
> >>> As you say, it is up to the user to disable it in the VM's XML.
> >>> Done this way, the feature won't be negotiated.
> >>>
> >> I agree with you, I'll remove this patch from the series.
> >
> > Are you sure that no performance impact exists for redundant merg-rx-buf
> configuration here?
> 
> I'm not sure to understand what you mean, could you please elaborate?
> 
I guess that if this feature is enabled and the feature actually are not used (no packets are scattered or merged) it will hurt the performance. 

So if one of the sides doesn't want to use it because of performance, it may want to disable it.

> > What if the second side want it and the current side no?
> 
> The feature won't be negotiated, assuming it has been disabled in QEMU
> cmdline (or via libvirt).
> > It may be that the vhost PMD user may want to disable it to save
> performance from some reasons, no?
> >
> 
> Then this user should disable it at QEMU level.
>
So the vhost PMD is not one of the sides to decide?
If so, why do we need the APIs to configure the features?

Looks like also the qemu is configured with the feature the VM\host sides may decide in some cases to disable it.


> Regards,
> Maxime

Re: [dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Matan Azrad]

Hi Maxim

Any response here? 

Besides that,

Regarding the TSO and this patch:
I think we shouldn't be so strict to not take them for this version:
1. The later time was a technical issue with the mailer - a mistake.
2. The patches don't change any default and makes sense - will not hurt anyone.

So I think we can do it beyond the letter of the law.

 From: Maxime Coquelin
 > Sent: Thursday, June 20, 2019 10:19 AM
 > To: Matan Azrad <matan@mellanox.com>; Noa Ezra
 <noae@mellanox.com>
 > Cc: dev@dpdk.org
 > Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
 >
 >
 >
 > On 6/20/19 8:52 AM, Matan Azrad wrote:
 > > Hi all
 > >
 > >> -----Original Message-----
 > >> From: Noa Ezra
 > >> Sent: Thursday, June 20, 2019 8:58 AM
 > >> To: Maxime Coquelin <maxime.coquelin@redhat.com>
 > >> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
 > >> Subject: RE: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
 > >>
 > >> Hi Maxime,
 > >> Thanks for your comment, please see below.
 > >>
 > >>> -----Original Message-----
 > >>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
 > >>> Sent: Wednesday, June 19, 2019 12:10 PM
 > >>> To: Noa Ezra <noae@mellanox.com>
 > >>> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
 > >>> Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
 > >>>
 > >>> Hi Noa,
 > >>>
 > >>> On 6/19/19 8:13 AM, Noa Ezra wrote:
 > >>>> Rx mergeable buffers is a virtio feature that allows chaining of
 > >>>> multiple virtio descriptors to handle large packet size.
 > >>>> This behavior is supported and enabled by default, however in
 > >>>> case the user knows that rx mergeable buffers are not needed, he
 > >>>> can disable the feature.
 > >>>> The user should also set mrg_rxbuf=off in virtual machine's xml.
 > >>>
 > >>> I'm not sure to understand why it is needed, as the vhost-user
 > >>> library supports the feature, it's better to let it being advertised.
 > >>>
 > >>> As you say, it is up to the user to disable it in the VM's XML.
 > >>> Done this way, the feature won't be negotiated.
 > >>>
 > >> I agree with you, I'll remove this patch from the series.
 > >
 > > Are you sure that no performance impact exists for redundant
 > > merg-rx-buf
 > configuration here?
 >
 > I'm not sure to understand what you mean, could you please elaborate?
 >
 I guess that if this feature is enabled and the feature actually are not used
 (no packets are scattered or merged) it will hurt the performance.
 
 So if one of the sides doesn't want to use it because of performance, it may
 want to disable it.
 
 > > What if the second side want it and the current side no?
 >
 > The feature won't be negotiated, assuming it has been disabled in QEMU
 > cmdline (or via libvirt).
 > > It may be that the vhost PMD user may want to disable it to save
 > performance from some reasons, no?
 > >
 >
 > Then this user should disable it at QEMU level.
 >
 So the vhost PMD is not one of the sides to decide?
 If so, why do we need the APIs to configure the features?
 
 Looks like also the qemu is configured with the feature the VM\host sides
 may decide in some cases to disable it.
 
 
 > Regards,
 > Maxime

Re: [dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Maxime Coquelin]

On 6/26/19 9:50 AM, Matan Azrad wrote:
> Hi Maxim
> 
> Any response here?
> 
> Besides that,
> 
> Regarding the TSO and this patch:
> I think we shouldn't be so strict to not take them for this version:
> 1. The later time was a technical issue with the mailer - a mistake.
> 2. The patches don't change any default and makes sense - will not hurt anyone.
> 
> So I think we can do it beyond the letter of the law.
> 
>   From: Maxime Coquelin
>   > Sent: Thursday, June 20, 2019 10:19 AM
>   > To: Matan Azrad <matan@mellanox.com>; Noa Ezra
>   <noae@mellanox.com>
>   > Cc: dev@dpdk.org
>   > Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
>   >
>   >
>   >
>   > On 6/20/19 8:52 AM, Matan Azrad wrote:
>   > > Hi all
>   > >
>   > >> -----Original Message-----
>   > >> From: Noa Ezra
>   > >> Sent: Thursday, June 20, 2019 8:58 AM
>   > >> To: Maxime Coquelin <maxime.coquelin@redhat.com>
>   > >> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
>   > >> Subject: RE: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
>   > >>
>   > >> Hi Maxime,
>   > >> Thanks for your comment, please see below.
>   > >>
>   > >>> -----Original Message-----
>   > >>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>   > >>> Sent: Wednesday, June 19, 2019 12:10 PM
>   > >>> To: Noa Ezra <noae@mellanox.com>
>   > >>> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
>   > >>> Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
>   > >>>
>   > >>> Hi Noa,
>   > >>>
>   > >>> On 6/19/19 8:13 AM, Noa Ezra wrote:
>   > >>>> Rx mergeable buffers is a virtio feature that allows chaining of
>   > >>>> multiple virtio descriptors to handle large packet size.
>   > >>>> This behavior is supported and enabled by default, however in
>   > >>>> case the user knows that rx mergeable buffers are not needed, he
>   > >>>> can disable the feature.
>   > >>>> The user should also set mrg_rxbuf=off in virtual machine's xml.
>   > >>>
>   > >>> I'm not sure to understand why it is needed, as the vhost-user
>   > >>> library supports the feature, it's better to let it being advertised.
>   > >>>
>   > >>> As you say, it is up to the user to disable it in the VM's XML.
>   > >>> Done this way, the feature won't be negotiated.
>   > >>>
>   > >> I agree with you, I'll remove this patch from the series.
>   > >
>   > > Are you sure that no performance impact exists for redundant
>   > > merg-rx-buf
>   > configuration here?
>   >
>   > I'm not sure to understand what you mean, could you please elaborate?
>   >
>   I guess that if this feature is enabled and the feature actually are not used
>   (no packets are scattered or merged) it will hurt the performance.

Well, latest performance measurements does not show a big impact now on 
enabling mergeable buffers feature unconditionaly.


>   So if one of the sides doesn't want to use it because of performance, it may
>   want to disable it.

And even if there is an impact, the way to disable it is through
Libvirt/Qemu.

>   > > What if the second side want it and the current side no?
>   >
>   > The feature won't be negotiated, assuming it has been disabled in QEMU
>   > cmdline (or via libvirt).
>   > > It may be that the vhost PMD user may want to disable it to save
>   > performance from some reasons, no?
>   > >
>   >
>   > Then this user should disable it at QEMU level.
>   >
>   So the vhost PMD is not one of the sides to decide?
>   If so, why do we need the APIs to configure the features?

Are you talking about the rte_vhost_driver_set_features() and related
APIs?

This is used for example by the external backends that support features
specific to the backend type (e.g. crypto), or also used by OVS-DPDK, to
disable TSO. So these usages are for functional reasons, not tuning.

>   Looks like also the qemu is configured with the feature the VM\host sides
>   may decide in some cases to disable it.

For functional reasons, I agree. So I that's why I agree with your tso
patch as the application has to support it, but that's not the case of
the mergeable buffers features.

Tiwei, what's your opinion on this?

>   > Regards,
>   > Maxime
> 

Re: [dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Matan Azrad]

From: Maxime Coquelin
> On 6/26/19 9:50 AM, Matan Azrad wrote:
> > Hi Maxim
> >
> > Any response here?
> >
> > Besides that,
> >
> > Regarding the TSO and this patch:
> > I think we shouldn't be so strict to not take them for this version:
> > 1. The later time was a technical issue with the mailer - a mistake.
> > 2. The patches don't change any default and makes sense - will not hurt
> anyone.
> >
> > So I think we can do it beyond the letter of the law.
> >
> >   From: Maxime Coquelin
> >   > Sent: Thursday, June 20, 2019 10:19 AM
> >   > To: Matan Azrad <matan@mellanox.com>; Noa Ezra
> >   <noae@mellanox.com>
> >   > Cc: dev@dpdk.org
> >   > Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
> >   >
> >   >
> >   >
> >   > On 6/20/19 8:52 AM, Matan Azrad wrote:
> >   > > Hi all
> >   > >
> >   > >> -----Original Message-----
> >   > >> From: Noa Ezra
> >   > >> Sent: Thursday, June 20, 2019 8:58 AM
> >   > >> To: Maxime Coquelin <maxime.coquelin@redhat.com>
> >   > >> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
> >   > >> Subject: RE: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
> >   > >>
> >   > >> Hi Maxime,
> >   > >> Thanks for your comment, please see below.
> >   > >>
> >   > >>> -----Original Message-----
> >   > >>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> >   > >>> Sent: Wednesday, June 19, 2019 12:10 PM
> >   > >>> To: Noa Ezra <noae@mellanox.com>
> >   > >>> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
> >   > >>> Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
> >   > >>>
> >   > >>> Hi Noa,
> >   > >>>
> >   > >>> On 6/19/19 8:13 AM, Noa Ezra wrote:
> >   > >>>> Rx mergeable buffers is a virtio feature that allows chaining of
> >   > >>>> multiple virtio descriptors to handle large packet size.
> >   > >>>> This behavior is supported and enabled by default, however in
> >   > >>>> case the user knows that rx mergeable buffers are not needed, he
> >   > >>>> can disable the feature.
> >   > >>>> The user should also set mrg_rxbuf=off in virtual machine's xml.
> >   > >>>
> >   > >>> I'm not sure to understand why it is needed, as the vhost-user
> >   > >>> library supports the feature, it's better to let it being advertised.
> >   > >>>
> >   > >>> As you say, it is up to the user to disable it in the VM's XML.
> >   > >>> Done this way, the feature won't be negotiated.
> >   > >>>
> >   > >> I agree with you, I'll remove this patch from the series.
> >   > >
> >   > > Are you sure that no performance impact exists for redundant
> >   > > merg-rx-buf
> >   > configuration here?
> >   >
> >   > I'm not sure to understand what you mean, could you please elaborate?
> >   >
> >   I guess that if this feature is enabled and the feature actually are not used
> >   (no packets are scattered or merged) it will hurt the performance.
> 
> Well, latest performance measurements does not show a big impact now on
> enabling mergeable buffers feature unconditionaly.
 
Did you test small packets \ big?

> >   So if one of the sides doesn't want to use it because of performance, it
> may
> >   want to disable it.
> 
> And even if there is an impact, the way to disable it is through
> Libvirt/Qemu.

Not sure, as TSO application may decide to not do it in spite of it is configured in Qemu. 
 
> >   > > What if the second side want it and the current side no?
> >   >
> >   > The feature won't be negotiated, assuming it has been disabled in
> QEMU
> >   > cmdline (or via libvirt).
> >   > > It may be that the vhost PMD user may want to disable it to save
> >   > performance from some reasons, no?
> >   > >
> >   >
> >   > Then this user should disable it at QEMU level.
> >   >
> >   So the vhost PMD is not one of the sides to decide?
> >   If so, why do we need the APIs to configure the features?
> 
> Are you talking about the rte_vhost_driver_set_features() and related
> APIs?
 
Yes

> This is used for example by the external backends that support features
> specific to the backend type (e.g. crypto), or also used by OVS-DPDK, to
> disable TSO. So these usages are for functional reasons, not tuning.

Exactly, applications (like OVS) may decide to disable features because a lot of reasons. 

> >   Looks like also the qemu is configured with the feature the VM\host sides
> >   may decide in some cases to disable it.
> 
> For functional reasons, I agree. So I that's why I agree with your tso
> patch as the application has to support it, but that's not the case of
> the mergeable buffers features.

Performance reasons are not good enough?

> Tiwei, what's your opinion on this?
> 
> >   > Regards,
> >   > Maxime
> >

Re: [dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Maxime Coquelin]

On 6/26/19 1:18 PM, Matan Azrad wrote:
> 
> 
> From: Maxime Coquelin
>> On 6/26/19 9:50 AM, Matan Azrad wrote:
>>> Hi Maxim
>>>
>>> Any response here?
>>>
>>> Besides that,
>>>
>>> Regarding the TSO and this patch:
>>> I think we shouldn't be so strict to not take them for this version:
>>> 1. The later time was a technical issue with the mailer - a mistake.
>>> 2. The patches don't change any default and makes sense - will not hurt
>> anyone.
>>>
>>> So I think we can do it beyond the letter of the law.
>>>
>>>    From: Maxime Coquelin
>>>    > Sent: Thursday, June 20, 2019 10:19 AM
>>>    > To: Matan Azrad <matan@mellanox.com>; Noa Ezra
>>>    <noae@mellanox.com>
>>>    > Cc: dev@dpdk.org
>>>    > Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
>>>    >
>>>    >
>>>    >
>>>    > On 6/20/19 8:52 AM, Matan Azrad wrote:
>>>    > > Hi all
>>>    > >
>>>    > >> -----Original Message-----
>>>    > >> From: Noa Ezra
>>>    > >> Sent: Thursday, June 20, 2019 8:58 AM
>>>    > >> To: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>    > >> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
>>>    > >> Subject: RE: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
>>>    > >>
>>>    > >> Hi Maxime,
>>>    > >> Thanks for your comment, please see below.
>>>    > >>
>>>    > >>> -----Original Message-----
>>>    > >>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>>    > >>> Sent: Wednesday, June 19, 2019 12:10 PM
>>>    > >>> To: Noa Ezra <noae@mellanox.com>
>>>    > >>> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
>>>    > >>> Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
>>>    > >>>
>>>    > >>> Hi Noa,
>>>    > >>>
>>>    > >>> On 6/19/19 8:13 AM, Noa Ezra wrote:
>>>    > >>>> Rx mergeable buffers is a virtio feature that allows chaining of
>>>    > >>>> multiple virtio descriptors to handle large packet size.
>>>    > >>>> This behavior is supported and enabled by default, however in
>>>    > >>>> case the user knows that rx mergeable buffers are not needed, he
>>>    > >>>> can disable the feature.
>>>    > >>>> The user should also set mrg_rxbuf=off in virtual machine's xml.
>>>    > >>>
>>>    > >>> I'm not sure to understand why it is needed, as the vhost-user
>>>    > >>> library supports the feature, it's better to let it being advertised.
>>>    > >>>
>>>    > >>> As you say, it is up to the user to disable it in the VM's XML.
>>>    > >>> Done this way, the feature won't be negotiated.
>>>    > >>>
>>>    > >> I agree with you, I'll remove this patch from the series.
>>>    > >
>>>    > > Are you sure that no performance impact exists for redundant
>>>    > > merg-rx-buf
>>>    > configuration here?
>>>    >
>>>    > I'm not sure to understand what you mean, could you please elaborate?
>>>    >
>>>    I guess that if this feature is enabled and the feature actually are not used
>>>    (no packets are scattered or merged) it will hurt the performance.
>>
>> Well, latest performance measurements does not show a big impact now on
>> enabling mergeable buffers feature unconditionaly.
>   
> Did you test small packets \ big?

64B packets, in non-vector mode on Virtio PMD side.

> 
>>>    So if one of the sides doesn't want to use it because of performance, it
>> may
>>>    want to disable it.
>>
>> And even if there is an impact, the way to disable it is through
>> Libvirt/Qemu.
> 
> Not sure, as TSO application may decide to not do it in spite of it is configured in Qemu.
>   
>>>    > > What if the second side want it and the current side no?
>>>    >
>>>    > The feature won't be negotiated, assuming it has been disabled in
>> QEMU
>>>    > cmdline (or via libvirt).
>>>    > > It may be that the vhost PMD user may want to disable it to save
>>>    > performance from some reasons, no?
>>>    > >
>>>    >
>>>    > Then this user should disable it at QEMU level.
>>>    >
>>>    So the vhost PMD is not one of the sides to decide?
>>>    If so, why do we need the APIs to configure the features?
>>
>> Are you talking about the rte_vhost_driver_set_features() and related
>> APIs?
>   
> Yes
> 
>> This is used for example by the external backends that support features
>> specific to the backend type (e.g. crypto), or also used by OVS-DPDK, to
>> disable TSO. So these usages are for functional reasons, not tuning.
> 
> Exactly, applications (like OVS) may decide to disable features because a lot of reasons.
> 
>>>    Looks like also the qemu is configured with the feature the VM\host sides
>>>    may decide in some cases to disable it.
>>
>> For functional reasons, I agree. So I that's why I agree with your tso
>> patch as the application has to support it, but that's not the case of
>> the mergeable buffers features.
> 
> Performance reasons are not good enough?

No, that's not what I mean.
I mean that the application should be able to disable a feature when it
does not meet the functional requirement.

For performance tuning, the qemu way is available, and enough.

> 
>> Tiwei, what's your opinion on this?
>>
>>>    > Regards,
>>>    > Maxime
>>>

Re: [dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Matan Azrad]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, June 26, 2019 3:06 PM
> To: Matan Azrad <matan@mellanox.com>; Noa Ezra <noae@mellanox.com>
> Cc: dev@dpdk.org; Tiwei Bie <tiwei.bie@intel.com>
> Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
> 
> 
> 
> On 6/26/19 1:18 PM, Matan Azrad wrote:
> >
> >
> > From: Maxime Coquelin
> >> On 6/26/19 9:50 AM, Matan Azrad wrote:
> >>> Hi Maxim
> >>>
> >>> Any response here?
> >>>
> >>> Besides that,
> >>>
> >>> Regarding the TSO and this patch:
> >>> I think we shouldn't be so strict to not take them for this version:
> >>> 1. The later time was a technical issue with the mailer - a mistake.
> >>> 2. The patches don't change any default and makes sense - will not
> >>> hurt
> >> anyone.
> >>>
> >>> So I think we can do it beyond the letter of the law.
> >>>
> >>>    From: Maxime Coquelin
> >>>    > Sent: Thursday, June 20, 2019 10:19 AM
> >>>    > To: Matan Azrad <matan@mellanox.com>; Noa Ezra
> >>>    <noae@mellanox.com>
> >>>    > Cc: dev@dpdk.org
> >>>    > Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
> >>>    >
> >>>    >
> >>>    >
> >>>    > On 6/20/19 8:52 AM, Matan Azrad wrote:
> >>>    > > Hi all
> >>>    > >
> >>>    > >> -----Original Message-----
> >>>    > >> From: Noa Ezra
> >>>    > >> Sent: Thursday, June 20, 2019 8:58 AM
> >>>    > >> To: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>    > >> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
> >>>    > >> Subject: RE: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
> >>>    > >>
> >>>    > >> Hi Maxime,
> >>>    > >> Thanks for your comment, please see below.
> >>>    > >>
> >>>    > >>> -----Original Message-----
> >>>    > >>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> >>>    > >>> Sent: Wednesday, June 19, 2019 12:10 PM
> >>>    > >>> To: Noa Ezra <noae@mellanox.com>
> >>>    > >>> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
> >>>    > >>> Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
> >>>    > >>>
> >>>    > >>> Hi Noa,
> >>>    > >>>
> >>>    > >>> On 6/19/19 8:13 AM, Noa Ezra wrote:
> >>>    > >>>> Rx mergeable buffers is a virtio feature that allows chaining of
> >>>    > >>>> multiple virtio descriptors to handle large packet size.
> >>>    > >>>> This behavior is supported and enabled by default, however in
> >>>    > >>>> case the user knows that rx mergeable buffers are not needed,
> he
> >>>    > >>>> can disable the feature.
> >>>    > >>>> The user should also set mrg_rxbuf=off in virtual machine's xml.
> >>>    > >>>
> >>>    > >>> I'm not sure to understand why it is needed, as the vhost-user
> >>>    > >>> library supports the feature, it's better to let it being advertised.
> >>>    > >>>
> >>>    > >>> As you say, it is up to the user to disable it in the VM's XML.
> >>>    > >>> Done this way, the feature won't be negotiated.
> >>>    > >>>
> >>>    > >> I agree with you, I'll remove this patch from the series.
> >>>    > >
> >>>    > > Are you sure that no performance impact exists for redundant
> >>>    > > merg-rx-buf
> >>>    > configuration here?
> >>>    >
> >>>    > I'm not sure to understand what you mean, could you please
> elaborate?
> >>>    >
> >>>    I guess that if this feature is enabled and the feature actually are not
> used
> >>>    (no packets are scattered or merged) it will hurt the performance.
> >>
> >> Well, latest performance measurements does not show a big impact now
> >> on enabling mergeable buffers feature unconditionaly.
> >
> > Did you test small packets \ big?
> 
> 64B packets, in non-vector mode on Virtio PMD side.
> 
> >
> >>>    So if one of the sides doesn't want to use it because of
> >>> performance, it
> >> may
> >>>    want to disable it.
> >>
> >> And even if there is an impact, the way to disable it is through
> >> Libvirt/Qemu.
> >
> > Not sure, as TSO application may decide to not do it in spite of it is
> configured in Qemu.
> >
> >>>    > > What if the second side want it and the current side no?
> >>>    >
> >>>    > The feature won't be negotiated, assuming it has been disabled
> >>> in
> >> QEMU
> >>>    > cmdline (or via libvirt).
> >>>    > > It may be that the vhost PMD user may want to disable it to save
> >>>    > performance from some reasons, no?
> >>>    > >
> >>>    >
> >>>    > Then this user should disable it at QEMU level.
> >>>    >
> >>>    So the vhost PMD is not one of the sides to decide?
> >>>    If so, why do we need the APIs to configure the features?
> >>
> >> Are you talking about the rte_vhost_driver_set_features() and related
> >> APIs?
> >
> > Yes
> >
> >> This is used for example by the external backends that support
> >> features specific to the backend type (e.g. crypto), or also used by
> >> OVS-DPDK, to disable TSO. So these usages are for functional reasons, not
> tuning.
> >
> > Exactly, applications (like OVS) may decide to disable features because a lot
> of reasons.
> >
> >>>    Looks like also the qemu is configured with the feature the VM\host
> sides
> >>>    may decide in some cases to disable it.
> >>
> >> For functional reasons, I agree. So I that's why I agree with your
> >> tso patch as the application has to support it, but that's not the
> >> case of the mergeable buffers features.
> >
> > Performance reasons are not good enough?
> 
> No, that's not what I mean.
> I mean that the application should be able to disable a feature when it does
> not meet the functional requirement.
> 
> For performance tuning, the qemu way is available, and enough.
> 

I think that this is the point we are not agree on.

I think that application may want to disable the feature in some cases because of performance reasons (maybe others too), 
And in some other cases to work with the feature.

So, it makes sense IMO to let the application to decide what it wants without any concern about the QEMU configuration.

Why to not allow to the PMD user to do it by the application (using prob parameters)?


> >
> >> Tiwei, what's your opinion on this?
> >>
> >>>    > Regards,
> >>>    > Maxime
> >>>

Re: [dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Maxime Coquelin]

On 6/26/19 3:24 PM, Matan Azrad wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Wednesday, June 26, 2019 3:06 PM
>> To: Matan Azrad <matan@mellanox.com>; Noa Ezra <noae@mellanox.com>
>> Cc: dev@dpdk.org; Tiwei Bie <tiwei.bie@intel.com>
>> Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
>>
>>
>>
>> On 6/26/19 1:18 PM, Matan Azrad wrote:
>>>
>>>
>>> From: Maxime Coquelin
>>>> On 6/26/19 9:50 AM, Matan Azrad wrote:
>>>>> Hi Maxim
>>>>>
>>>>> Any response here?
>>>>>
>>>>> Besides that,
>>>>>
>>>>> Regarding the TSO and this patch:
>>>>> I think we shouldn't be so strict to not take them for this version:
>>>>> 1. The later time was a technical issue with the mailer - a mistake.
>>>>> 2. The patches don't change any default and makes sense - will not
>>>>> hurt
>>>> anyone.
>>>>>
>>>>> So I think we can do it beyond the letter of the law.
>>>>>
>>>>>     From: Maxime Coquelin
>>>>>     > Sent: Thursday, June 20, 2019 10:19 AM
>>>>>     > To: Matan Azrad <matan@mellanox.com>; Noa Ezra
>>>>>     <noae@mellanox.com>
>>>>>     > Cc: dev@dpdk.org
>>>>>     > Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
>>>>>     >
>>>>>     >
>>>>>     >
>>>>>     > On 6/20/19 8:52 AM, Matan Azrad wrote:
>>>>>     > > Hi all
>>>>>     > >
>>>>>     > >> -----Original Message-----
>>>>>     > >> From: Noa Ezra
>>>>>     > >> Sent: Thursday, June 20, 2019 8:58 AM
>>>>>     > >> To: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>     > >> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
>>>>>     > >> Subject: RE: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
>>>>>     > >>
>>>>>     > >> Hi Maxime,
>>>>>     > >> Thanks for your comment, please see below.
>>>>>     > >>
>>>>>     > >>> -----Original Message-----
>>>>>     > >>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>>>>     > >>> Sent: Wednesday, June 19, 2019 12:10 PM
>>>>>     > >>> To: Noa Ezra <noae@mellanox.com>
>>>>>     > >>> Cc: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
>>>>>     > >>> Subject: Re: [PATCH 2/2] net/vhost: support mrg-rxbuf disabling
>>>>>     > >>>
>>>>>     > >>> Hi Noa,
>>>>>     > >>>
>>>>>     > >>> On 6/19/19 8:13 AM, Noa Ezra wrote:
>>>>>     > >>>> Rx mergeable buffers is a virtio feature that allows chaining of
>>>>>     > >>>> multiple virtio descriptors to handle large packet size.
>>>>>     > >>>> This behavior is supported and enabled by default, however in
>>>>>     > >>>> case the user knows that rx mergeable buffers are not needed,
>> he
>>>>>     > >>>> can disable the feature.
>>>>>     > >>>> The user should also set mrg_rxbuf=off in virtual machine's xml.
>>>>>     > >>>
>>>>>     > >>> I'm not sure to understand why it is needed, as the vhost-user
>>>>>     > >>> library supports the feature, it's better to let it being advertised.
>>>>>     > >>>
>>>>>     > >>> As you say, it is up to the user to disable it in the VM's XML.
>>>>>     > >>> Done this way, the feature won't be negotiated.
>>>>>     > >>>
>>>>>     > >> I agree with you, I'll remove this patch from the series.
>>>>>     > >
>>>>>     > > Are you sure that no performance impact exists for redundant
>>>>>     > > merg-rx-buf
>>>>>     > configuration here?
>>>>>     >
>>>>>     > I'm not sure to understand what you mean, could you please
>> elaborate?
>>>>>     >
>>>>>     I guess that if this feature is enabled and the feature actually are not
>> used
>>>>>     (no packets are scattered or merged) it will hurt the performance.
>>>>
>>>> Well, latest performance measurements does not show a big impact now
>>>> on enabling mergeable buffers feature unconditionaly.
>>>
>>> Did you test small packets \ big?
>>
>> 64B packets, in non-vector mode on Virtio PMD side.
>>
>>>
>>>>>     So if one of the sides doesn't want to use it because of
>>>>> performance, it
>>>> may
>>>>>     want to disable it.
>>>>
>>>> And even if there is an impact, the way to disable it is through
>>>> Libvirt/Qemu.
>>>
>>> Not sure, as TSO application may decide to not do it in spite of it is
>> configured in Qemu.
>>>
>>>>>     > > What if the second side want it and the current side no?
>>>>>     >
>>>>>     > The feature won't be negotiated, assuming it has been disabled
>>>>> in
>>>> QEMU
>>>>>     > cmdline (or via libvirt).
>>>>>     > > It may be that the vhost PMD user may want to disable it to save
>>>>>     > performance from some reasons, no?
>>>>>     > >
>>>>>     >
>>>>>     > Then this user should disable it at QEMU level.
>>>>>     >
>>>>>     So the vhost PMD is not one of the sides to decide?
>>>>>     If so, why do we need the APIs to configure the features?
>>>>
>>>> Are you talking about the rte_vhost_driver_set_features() and related
>>>> APIs?
>>>
>>> Yes
>>>
>>>> This is used for example by the external backends that support
>>>> features specific to the backend type (e.g. crypto), or also used by
>>>> OVS-DPDK, to disable TSO. So these usages are for functional reasons, not
>> tuning.
>>>
>>> Exactly, applications (like OVS) may decide to disable features because a lot
>> of reasons.
>>>
>>>>>     Looks like also the qemu is configured with the feature the VM\host
>> sides
>>>>>     may decide in some cases to disable it.
>>>>
>>>> For functional reasons, I agree. So I that's why I agree with your
>>>> tso patch as the application has to support it, but that's not the
>>>> case of the mergeable buffers features.
>>>
>>> Performance reasons are not good enough?
>>
>> No, that's not what I mean.
>> I mean that the application should be able to disable a feature when it does
>> not meet the functional requirement.
>>
>> For performance tuning, the qemu way is available, and enough.
>>
> 
> I think that this is the point we are not agree on.
> 
> I think that application may want to disable the feature in some cases because of performance reasons (maybe others too),
> And in some other cases to work with the feature.
> 
> So, it makes sense IMO to let the application to decide what it wants without any concern about the QEMU configuration.
> 
> Why to not allow to the PMD user to do it by the application (using prob parameters)?

I think we should restrict the Virtio features from the Vhost PMD
parameter at as min as possible, only to ensure compatibility with the
application (iommu, postcopy, tso, ...). One problem I see with
providing the possibility to change any Virtio feature at runtime
is reconnection.

For example, you start your application with enabling mergeable buffers,
stop it and restart it without the feature enabled by the application.
As the negotiation with the driver is not done again at reconnect time,
Qemu will fail.

> 
> 
>>>
>>>> Tiwei, what's your opinion on this?
>>>>
>>>>>     > Regards,
>>>>>     > Maxime
>>>>>

Re: [dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Matan Azrad]

From: Maxime Coquelin 
> >>>> For functional reasons, I agree. So I that's why I agree with your
> >>>> tso patch as the application has to support it, but that's not the
> >>>> case of the mergeable buffers features.
> >>>
> >>> Performance reasons are not good enough?
> >>
> >> No, that's not what I mean.
> >> I mean that the application should be able to disable a feature when
> >> it does not meet the functional requirement.
> >>
> >> For performance tuning, the qemu way is available, and enough.
> >>
> >
> > I think that this is the point we are not agree on.
> >
> > I think that application may want to disable the feature in some cases
> > because of performance reasons (maybe others too), And in some other
> cases to work with the feature.
> >
> > So, it makes sense IMO to let the application to decide what it wants
> without any concern about the QEMU configuration.
> >
> > Why to not allow to the PMD user to do it by the application (using prob
> parameters)?
> 
> I think we should restrict the Virtio features from the Vhost PMD parameter
> at as min as possible, only to ensure compatibility with the application
> (iommu, postcopy, tso, ...). One problem I see with providing the possibility
> to change any Virtio feature at runtime is reconnection.
> 
> For example, you start your application with enabling mergeable buffers,
> stop it and restart it without the feature enabled by the application.
> As the negotiation with the driver is not done again at reconnect time, Qemu
> will fail.

Looks like you are describing a new issue in the vhost PMD, it must close the connection when the PMD is closed\removed.
So, every probe(hotplug add) it will start from scratch.

Re: [dpdk-dev] [PATCH 1/2] net/vhost: support TSO disabling

[Maxime Coquelin]

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

Re: [dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Maxime Coquelin]

On 6/27/19 7:04 AM, Matan Azrad wrote:
> 
> 
> From: Maxime Coquelin 
>>>>>> For functional reasons, I agree. So I that's why I agree with your
>>>>>> tso patch as the application has to support it, but that's not the
>>>>>> case of the mergeable buffers features.
>>>>>
>>>>> Performance reasons are not good enough?
>>>>
>>>> No, that's not what I mean.
>>>> I mean that the application should be able to disable a feature when
>>>> it does not meet the functional requirement.
>>>>
>>>> For performance tuning, the qemu way is available, and enough.
>>>>
>>>
>>> I think that this is the point we are not agree on.
>>>
>>> I think that application may want to disable the feature in some cases
>>> because of performance reasons (maybe others too), And in some other
>> cases to work with the feature.
>>>
>>> So, it makes sense IMO to let the application to decide what it wants
>> without any concern about the QEMU configuration.
>>>
>>> Why to not allow to the PMD user to do it by the application (using prob
>> parameters)?
>>
>> I think we should restrict the Virtio features from the Vhost PMD parameter
>> at as min as possible, only to ensure compatibility with the application
>> (iommu, postcopy, tso, ...). One problem I see with providing the possibility
>> to change any Virtio feature at runtime is reconnection.
>>
>> For example, you start your application with enabling mergeable buffers,
>> stop it and restart it without the feature enabled by the application.
>> As the negotiation with the driver is not done again at reconnect time, Qemu
>> will fail.
> 
> Looks like you are describing a new issue in the vhost PMD, it must close the connection when the PMD is closed\removed.
> So, every probe(hotplug add) it will start from scratch.
> 

No, you can close the application and restart it for example without
having to restart the guest. In this case, the feature negotiation is
not done again.

So I remain convinced we should not provide the possibility to disable
any feature that is not dependent on the application.

Megeable buffers is not dependent on the application, as it is only
related to the ring implementation, and it is supported by the DPDK
Vhost library.

Regards,
Maxime

Re: [dpdk-dev] [PATCH 1/2] net/vhost: support TSO disabling

[Maxime Coquelin]

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

Re: [dpdk-dev] [PATCH 2/2] net/vhost: support mrg-rxbuf disabling

[Maxime Coquelin]

On 6/19/19 8:13 AM, Noa Ezra wrote:
> Rx mergeable buffers is a virtio feature that allows chaining of
> multiple virtio descriptors to handle large packet size.
> This behavior is supported and enabled by default, however in case
> the user knows that rx mergeable buffers are not needed, he can disable
> the feature.
> The user should also set mrg_rxbuf=off in 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 | 17 ++++++++++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 

As stated earlier, I will not apply this patch because feature disabling
should be done at Vhost-user master side.

The exception are for features that require specific support from the
application, such as TSO or postcopy.

Regards,
Maxime