[PATCH] vhost: don't set vring call if no enabled msix

[PATCH] vhost: don't set vring call if no enabled msix

[lyx634449800]

When conducting performance testing using testpmd in the guest os,
it was observed that the performance was lower compared to the
scenario of direct vfio-pci usage.

In the virtual machine operating system, even if the virtio device
does not use msix interrupts, vhost still sets vring call fd. This
leads to unnecessary performance overhead. If the guest driver does
not enable msix capability (e.g virtio-net pmd), we should also
check and clear the vring call fd.

Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>
---
 hw/virtio/vhost.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index f50180e60e..b972c84e67 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1266,13 +1266,15 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
         vhost_virtqueue_mask(dev, vdev, idx, false);
     }
 
-    if (k->query_guest_notifiers &&
-        k->query_guest_notifiers(qbus->parent) &&
-        virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR) {
-        file.fd = -1;
-        r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
-        if (r) {
-            goto fail_vector;
+    if (k->query_guest_notifiers) {
+        if (!k->query_guest_notifiers(qbus->parent) ||
+            (k->query_guest_notifiers(qbus->parent) &&
+            virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR)) {
+            file.fd = -1;
+            r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
+            if (r) {
+                goto fail_vector;
+            }
         }
     }
 
-- 
2.43.0

Re: [PATCH] vhost: don't set vring call if no enabled msix

[Jason Wang]

On Mon, Apr 8, 2024 at 2:09 PM lyx634449800 <yuxue.liu@jaguarmicro.com> wrote:
>
> When conducting performance testing using testpmd in the guest os,
> it was observed that the performance was lower compared to the
> scenario of direct vfio-pci usage.
>
> In the virtual machine operating system, even if the virtio device
> does not use msix interrupts, vhost still sets vring call fd. This
> leads to unnecessary performance overhead. If the guest driver does
> not enable msix capability (e.g virtio-net pmd), we should also
> check and clear the vring call fd.
>
> Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>

Patch looks good, I would like to do the following tweaks:

1) explain what is not enough since commit:

commit 96a3d98d2cdbd897ff5ab33427aa4cfb94077665
Author: Jason Wang <jasowang@redhat.com>
Date:   Mon Aug 1 16:07:58 2016 +0800

    vhost: don't set vring call if no vector

2) tweak the title to "vhost: don't set vring call if guest notifiers
is not enabled" as it's not necessarily pci but also ccw.

Thanks

> ---
>  hw/virtio/vhost.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index f50180e60e..b972c84e67 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1266,13 +1266,15 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
>          vhost_virtqueue_mask(dev, vdev, idx, false);
>      }
>
> -    if (k->query_guest_notifiers &&
> -        k->query_guest_notifiers(qbus->parent) &&
> -        virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR) {
> -        file.fd = -1;
> -        r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
> -        if (r) {
> -            goto fail_vector;
> +    if (k->query_guest_notifiers) {
> +        if (!k->query_guest_notifiers(qbus->parent) ||
> +            (k->query_guest_notifiers(qbus->parent) &&
> +            virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR)) {
> +            file.fd = -1;
> +            r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
> +            if (r) {
> +                goto fail_vector;
> +            }
>          }
>      }
>
> --
> 2.43.0
>

[PATCH v2] vhost: don't set vring call if guest notifiers is not enabled

[lyx634449800]

When conducting performance testing using testpmd in the guest os,
it was observed that the performance was lower compared to the
scenario of direct vfio-pci usage.

In the commit 96a3d98d2cdbd897ff5ab33427aa4cfb94077665, the author
provided a good solution. However, because the guest OS's
driver(e.g., virtio-net pmd) may not enable the msix capability, the
function k->query_guest_notifiers(qbus->parent) may return false,
resulting in the expected effect not being achieved. To address this
issue, modify the conditional statement.

Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>
---
V2: Update commit description and title

 hw/virtio/vhost.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index f50180e60e..b972c84e67 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1266,13 +1266,15 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
         vhost_virtqueue_mask(dev, vdev, idx, false);
     }
 
-    if (k->query_guest_notifiers &&
-        k->query_guest_notifiers(qbus->parent) &&
-        virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR) {
-        file.fd = -1;
-        r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
-        if (r) {
-            goto fail_vector;
+    if (k->query_guest_notifiers) {
+        if (!k->query_guest_notifiers(qbus->parent) ||
+            (k->query_guest_notifiers(qbus->parent) &&
+            virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR)) {
+            file.fd = -1;
+            r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
+            if (r) {
+                goto fail_vector;
+            }
         }
     }
 
-- 
2.43.0

Re: [PATCH v2] vhost: don't set vring call if guest notifiers is not enabled

[Jason Wang]

On Mon, Apr 8, 2024 at 3:33 PM lyx634449800 <yuxue.liu@jaguarmicro.com> wrote:
>
> When conducting performance testing using testpmd in the guest os,
> it was observed that the performance was lower compared to the
> scenario of direct vfio-pci usage.
>
> In the commit 96a3d98d2cdbd897ff5ab33427aa4cfb94077665, the author
> provided a good solution. However, because the guest OS's
> driver(e.g., virtio-net pmd) may not enable the msix capability, the
> function k->query_guest_notifiers(qbus->parent) may return false,
> resulting in the expected effect not being achieved. To address this
> issue, modify the conditional statement.
>
> Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

Re: [PATCH v2] vhost: don't set vring call if guest notifiers is not enabled

[Michael S. Tsirkin]

On Mon, Apr 08, 2024 at 03:33:11PM +0800, lyx634449800 wrote:
> When conducting performance testing using testpmd in the guest os,
> it was observed that the performance was lower compared to the
> scenario of direct vfio-pci usage.
> 
> In the commit 96a3d98d2cdbd897ff5ab33427aa4cfb94077665, the author
> provided a good solution. However, because the guest OS's
> driver(e.g., virtio-net pmd) may not enable the msix capability, the
> function k->query_guest_notifiers(qbus->parent) may return false,
> resulting in the expected effect not being achieved. To address this
> issue, modify the conditional statement.
> 
> Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>


I tested v1 and it fails. Sent as reply on that patch.
Since all you did is tweak description and title the
problem is probably still there in v2.

> ---
> V2: Update commit description and title
> 
>  hw/virtio/vhost.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index f50180e60e..b972c84e67 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1266,13 +1266,15 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
>          vhost_virtqueue_mask(dev, vdev, idx, false);
>      }
>  
> -    if (k->query_guest_notifiers &&
> -        k->query_guest_notifiers(qbus->parent) &&
> -        virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR) {
> -        file.fd = -1;
> -        r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
> -        if (r) {
> -            goto fail_vector;
> +    if (k->query_guest_notifiers) {
> +        if (!k->query_guest_notifiers(qbus->parent) ||
> +            (k->query_guest_notifiers(qbus->parent) &&
> +            virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR)) {
> +            file.fd = -1;
> +            r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
> +            if (r) {
> +                goto fail_vector;
> +            }
>          }
>      }
>  
> -- 
> 2.43.0

Re: [PATCH] vhost: don't set vring call if no enabled msix

[Michael S. Tsirkin]

On Mon, Apr 08, 2024 at 02:08:42PM +0800, lyx634449800 wrote:
> When conducting performance testing using testpmd in the guest os,
> it was observed that the performance was lower compared to the
> scenario of direct vfio-pci usage.
> 
> In the virtual machine operating system, even if the virtio device
> does not use msix interrupts, vhost still sets vring call fd. This
> leads to unnecessary performance overhead. If the guest driver does
> not enable msix capability (e.g virtio-net pmd), we should also
> check and clear the vring call fd.
> 
> Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>


Fails testing under cross-i686-tci:

https://gitlab.com/mstredhat/qemu/-/jobs/6578881990

36/258 qemu:qtest+qtest-i386 / qtest-i386/ioh3420-test                    OK               0.15s   1 subtests passed
▶  37/258 ERROR:../tests/qtest/qos-test.c:191:subprocess_run_one_test: child process (/aarch64/virt/generic-pcihost/pci-bus-generic/pci-bus/virtio-net-pci/virtio-net/virtio-net-tests/vhost-user/migrate/subprocess [22197]) failed unexpectedly ERROR         
 38/258 qemu:qtest+qtest-i386 / qtest-i386/lpc-ich9-test                   OK               0.16s   1 subtests passed
 37/258 qemu:qtest+qtest-aarch64 / qtest-aarch64/qos-test                  ERROR           13.20s   killed by signal 6 SIGABRT
>>> G_TEST_DBUS_DAEMON=/builds/mstredhat/qemu/tests/dbus-vmstate-daemon.sh PYTHON=/builds/mstredhat/qemu/build/pyvenv/bin/python3 MALLOC_PERTURB_=144 QTEST_QEMU_BINARY=./qemu-system-aarch64 /builds/mstredhat/qemu/build/tests/qtest/qos-test --tap -k
――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
stderr:
**
ERROR:../tests/qtest/vhost-user-test.c:468:chr_read: assertion failed (err == NULL): Bad file descriptor (g-unix-error-quark, 0)
**
ERROR:../tests/qtest/qos-test.c:191:subprocess_run_one_test: child process (/aarch64/virt/generic-pcihost/pci-bus-generic/pci-bus/virtio-net-pci/virtio-net/virtio-net-tests/vhost-user/migrate/subprocess [22197]) failed unexpectedly
(test program exited with status code -6)
――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――


> ---
>  hw/virtio/vhost.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index f50180e60e..b972c84e67 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1266,13 +1266,15 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
>          vhost_virtqueue_mask(dev, vdev, idx, false);
>      }
>  
> -    if (k->query_guest_notifiers &&
> -        k->query_guest_notifiers(qbus->parent) &&
> -        virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR) {
> -        file.fd = -1;
> -        r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
> -        if (r) {
> -            goto fail_vector;
> +    if (k->query_guest_notifiers) {
> +        if (!k->query_guest_notifiers(qbus->parent) ||
> +            (k->query_guest_notifiers(qbus->parent) &&
> +            virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR)) {
> +            file.fd = -1;
> +            r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
> +            if (r) {
> +                goto fail_vector;
> +            }
>          }
>      }
>  
> -- 
> 2.43.0

Re: [PATCH v2] vhost: don't set vring call if guest notifiers is not enabled

[Gavin Liu]

Hi Michael,

――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
stderr:
**
ERROR:../tests/qtest/vhost-user-test.c:468:chr_read: assertion failed (err == NULL): Bad file descriptor (g-unix-error-quark, 0)
**
ERROR:../tests/qtest/qos-test.c:191:subprocess_run_one_test: child process (/aarch64/virt/generic-pcihost/pci-bus-generic/pci-bus/virtio-net-pci/virtio-net/virtio-net-tests/vhost-user/migrate/subprocess [22197]) failed unexpectedly (test program exited with status code -6) ――
___________________________________________________________________________________________________________________________


When I analyze the problem, the scenario might be like this：
一、vhost user server：
   1：set call fd -1
   2：The function vhost_user_set_vring_call invokes vhost_set_vring_file to send the file descriptor information, where fd=-1, thus fd_num=0
   3: The function call vhost_user_write(dev, &msg, fds, fd_num) sends to the vhost user client.  
   
二、vhost user client
   1: The function call vhost_user_read receives the file descriptor information sent by the server.
   2: SocketChardev *s = SOCKET_CHARDEV(chr); Update s->read_msgfds_num = msgfds_num = 0.
   
三、tests/qtest/vhost-user-test
   1：vhost-user-test invokes the chr_read function to handle the messages sent by the server.
   2：The chr_read function initializes the local variable fd to -1.
   3：
        case VHOST_USER_SET_VRING_CALL:
			qemu_chr_fe_get_msgfds(chr, &fd, 1); 
			
!!! Because s->read_msgfds_num = 0, after the function qemu_chr_fe_get_msgfds(chr, &fd, 1) is executed, fd=-1.!!!

			g_unix_set_fd_nonblocking(fd, true, &err); 
			
!!! An exception will occur when setting nonblocking when fd < 0. !!!	
			g_assert_no_error(err);
						
四、Offer a modification suggestion
   1：Refer to the tcp_chr_recv function:
						static ssize_t tcp_chr_recv(Chardev *chr, char *buf, size_t len)
						{
							SocketChardev *s = SOCKET_CHARDEV(chr);
							  -----
							  ----
							for (i = 0; i < s->read_msgfds_num; i++) {
								int fd = s->read_msgfds[i];
								if (fd < 0) {
				!!!!When fd < 0, the qemu_socket_set_block function will not be called !!!!			
									continue;
								}

								/* O_NONBLOCK is preserved across SCM_RIGHTS so reset it */
								qemu_socket_set_block(fd);
				
   2: modify chr_read function 

				case VHOST_USER_SET_VRING_CALL:
						qemu_chr_fe_get_msgfds(chr, &fd, 1); 
						if (fd <0)   /* Add a condition check. */
							break;
						g_unix_set_fd_nonblocking(fd, true, &err); 
						g_assert_no_error(err);

Best regards,
Yuxue Liu


__________________________________________________________________________________________________

----- Original Message -----
From: Michael S. Tsirkin mst@redhat.com
Sent: April 9, 2024 17:31
To: Gavin Liu gavin.liu@jaguarmicro.com
Cc: jasowang@redhat.com; qemu-stable@nongnu.org; qemu-devel@nongnu.org
Subject: Re: [PATCH] vhost: don't set vring call if no enabled msix



External Mail: This email originated from OUTSIDE of the organization!
Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe.


On Mon, Apr 08, 2024 at 03:33:11PM +0800, lyx634449800 wrote:
> When conducting performance testing using testpmd in the guest os, it 
> was observed that the performance was lower compared to the scenario 
> of direct vfio-pci usage.
>
> In the commit 96a3d98d2cdbd897ff5ab33427aa4cfb94077665, the author 
> provided a good solution. However, because the guest OS's driver(e.g., 
> virtio-net pmd) may not enable the msix capability, the function 
> k->query_guest_notifiers(qbus->parent) may return false, resulting in 
> the expected effect not being achieved. To address this issue, modify 
> the conditional statement.
>
> Signed-off-by: Yuxue Liu <yuxue.liu@jaguarmicro.com>


I tested v1 and it fails. Sent as reply on that patch.
Since all you did is tweak description and title the problem is probably still there in v2.

> ---
> V2: Update commit description and title
>
>  hw/virtio/vhost.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 
> f50180e60e..b972c84e67 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1266,13 +1266,15 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
>          vhost_virtqueue_mask(dev, vdev, idx, false);
>      }
>
> -    if (k->query_guest_notifiers &&
> -        k->query_guest_notifiers(qbus->parent) &&
> -        virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR) {
> -        file.fd = -1;
> -        r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
> -        if (r) {
> -            goto fail_vector;
> +    if (k->query_guest_notifiers) {
> +        if (!k->query_guest_notifiers(qbus->parent) ||
> +            (k->query_guest_notifiers(qbus->parent) &&
> +            virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR)) {
> +            file.fd = -1;
> +            r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
> +            if (r) {
> +                goto fail_vector;
> +            }
>          }
>      }
>
> --
> 2.43.0