[net-next,2/3] vhost_net: support in order feature
diff mbox series

Message ID 20181123030016.4924-3-jasowang@redhat.com
State New, archived
Headers show
Series
  • basic in order support for vhost_net
Related show

Commit Message

Jason Wang Nov. 23, 2018, 3 a.m. UTC
This makes vhost_net to support in order feature. This is as simple as
use datacopy path when it was negotiated. An alternative is not to
advertise in order when zerocopy is enabled which tends to be
suboptimal consider zerocopy may suffer from e.g HOL issues.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vhost/net.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Michael S. Tsirkin Nov. 23, 2018, 3:49 p.m. UTC | #1
On Fri, Nov 23, 2018 at 11:00:15AM +0800, Jason Wang wrote:
> This makes vhost_net to support in order feature. This is as simple as
> use datacopy path when it was negotiated. An alternative is not to
> advertise in order when zerocopy is enabled which tends to be
> suboptimal consider zerocopy may suffer from e.g HOL issues.

Well IIRC vhost_zerocopy_signal_used is used to
actually reorder used ring to match available ring.
So with a big comment explaining why it is so,
we could just enable IN_ORDER there too.

> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/vhost/net.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index d919284f103b..bdf5de5a7eb2 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -74,7 +74,8 @@ enum {
>  	VHOST_NET_FEATURES = VHOST_FEATURES |
>  			 (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
>  			 (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
> -			 (1ULL << VIRTIO_F_IOMMU_PLATFORM)
> +			 (1ULL << VIRTIO_F_IOMMU_PLATFORM) |
> +	                 (1ULL << VIRTIO_F_IN_ORDER)
>  };
>  
>  enum {
> @@ -971,7 +972,8 @@ static void handle_tx(struct vhost_net *net)
>  	vhost_disable_notify(&net->dev, vq);
>  	vhost_net_disable_vq(net, vq);
>  
> -	if (vhost_sock_zcopy(sock))
> +	if (vhost_sock_zcopy(sock) &&
> +	    !vhost_has_feature(vq, VIRTIO_F_IN_ORDER))
>  		handle_tx_zerocopy(net, sock);
>  	else
>  		handle_tx_copy(net, sock);
> -- 
> 2.17.1
Jason Wang Nov. 26, 2018, 3:52 a.m. UTC | #2
On 2018/11/23 下午11:49, Michael S. Tsirkin wrote:
> On Fri, Nov 23, 2018 at 11:00:15AM +0800, Jason Wang wrote:
>> This makes vhost_net to support in order feature. This is as simple as
>> use datacopy path when it was negotiated. An alternative is not to
>> advertise in order when zerocopy is enabled which tends to be
>> suboptimal consider zerocopy may suffer from e.g HOL issues.
> Well IIRC vhost_zerocopy_signal_used is used to
> actually reorder used ring to match available ring.
> So with a big comment explaining why it is so,
> we could just enable IN_ORDER there too.
>

The problem is we allow switching between zerocopy and datacopy.

And what's more important, if we allow in order for zerocopy, a single 
packet delay may hang all the rest.

Thanks

Patch
diff mbox series

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index d919284f103b..bdf5de5a7eb2 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -74,7 +74,8 @@  enum {
 	VHOST_NET_FEATURES = VHOST_FEATURES |
 			 (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
 			 (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
-			 (1ULL << VIRTIO_F_IOMMU_PLATFORM)
+			 (1ULL << VIRTIO_F_IOMMU_PLATFORM) |
+	                 (1ULL << VIRTIO_F_IN_ORDER)
 };
 
 enum {
@@ -971,7 +972,8 @@  static void handle_tx(struct vhost_net *net)
 	vhost_disable_notify(&net->dev, vq);
 	vhost_net_disable_vq(net, vq);
 
-	if (vhost_sock_zcopy(sock))
+	if (vhost_sock_zcopy(sock) &&
+	    !vhost_has_feature(vq, VIRTIO_F_IN_ORDER))
 		handle_tx_zerocopy(net, sock);
 	else
 		handle_tx_copy(net, sock);