[Qemu-devel] [PATCH] vhost-user: Slave crashes as Master unmaps vrings during guest reboot

[Qemu-devel] [PATCH] vhost-user: Slave crashes as Master unmaps vrings during guest reboot

[Shesha Sreenivasamurthy]

Problem:
--------
If a guest has vhost-user enabled, then on reboot vhost_virtqueue_stop
is invoked. This unmaps vring memory mappings. However, it will not give
any indication to the underlying DPDK slave application about it.
Therefore, a pollmode DPDK driver tries to read the ring to check for
packets and segfaults.

Solution:
----------
VHOST_USER_RESET_OWNER API is issued by QEMU so that DPDK slave
application is informed that mappings will be soon gone so that
it can take necessary steps.

Shesha Sreenivasamurthy (1):
  vhost-user: Slave crashes as Master unmaps vrings during guest reboot

 hw/virtio/vhost.c | 5 +++++
 1 file changed, 5 insertions(+)

-- 
1.9.5 (Apple Git-50.3)

[Qemu-devel] [PATCH] vhost-user: Slave crashes as Master unmaps vrings during guest reboot

[Shesha Sreenivasamurthy]

Send VHOST_USER_RESET_OWNER when the device is stopped.

Signed-off-by: Shesha Sreenivasamurthy <shesha@cisco.com>
---
 hw/virtio/vhost.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index de29968..808184f 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1256,6 +1256,11 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
                              hdev->vq_index + i);
     }
 
+    if (hdev->vhost_ops->vhost_reset_device(hdev) < 0) {
+        fprintf(stderr, "vhost reset device %s failed\n", vdev->name);
+        fflush(stderr);
+    }
+
     vhost_log_put(hdev, true);
     hdev->started = false;
     hdev->log = NULL;
-- 
1.9.5 (Apple Git-50.3)

[Qemu-devel] [PATCH] vhost-user: Slave crashes as Master unmaps vrings during guest reboot

[Shesha Sreenivasamurthy]

Problem:
--------
If a guest has vhost-user enabled, then on reboot vhost_virtqueue_stop
is invoked. This unmaps vring memory mappings. However, it will not give
any indication to the underlying DPDK slave application about it.
Therefore, a pollmode DPDK driver tries to read the ring to check for
packets and segfaults.

Solution:
----------
VHOST_USER_RESET_OWNER API is issued by QEMU so that DPDK slave
application is informed that mappings will be soon gone so that
it can take necessary steps.

Shesha Sreenivasamurthy (1):
  vhost-user: Slave crashes as Master unmaps vrings during guest reboot

 hw/virtio/vhost.c | 5 +++++
 1 file changed, 5 insertions(+)

-- 
1.9.5 (Apple Git-50.3)

[Qemu-devel] [PATCH] vhost-user: Slave crashes as Master unmaps vrings during guest reboot

[Shesha Sreenivasamurthy]

Send VHOST_USER_RESET_OWNER when the device is stopped.

Signed-off-by: Shesha Sreenivasamurthy <shesha@cisco.com>
---
 hw/virtio/vhost.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index de29968..808184f 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1256,6 +1256,11 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
                              hdev->vq_index + i);
     }
 
+    if (hdev->vhost_ops->vhost_reset_device(hdev) < 0) {
+        fprintf(stderr, "vhost reset device %s failed\n", vdev->name);
+        fflush(stderr);
+    }
+
     vhost_log_put(hdev, true);
     hdev->started = false;
     hdev->log = NULL;
-- 
1.9.5 (Apple Git-50.3)

Re: [Qemu-devel] [PATCH] vhost-user: Slave crashes as Master unmaps vrings during guest reboot

[Michael S. Tsirkin]

On Fri, Jan 15, 2016 at 12:12:43PM -0800, Shesha Sreenivasamurthy wrote:
> Problem:
> --------
> If a guest has vhost-user enabled, then on reboot vhost_virtqueue_stop
> is invoked. This unmaps vring memory mappings. However, it will not give
> any indication to the underlying DPDK slave application about it.
> Therefore, a pollmode DPDK driver tries to read the ring to check for
> packets and segfaults.

The spec currently says:
Client must start ring upon receiving a kick (that is, detecting that file
descriptor is readable) on the descriptor specified by
VHOST_USER_SET_VRING_KICK, and stop ring upon receiving
VHOST_USER_GET_VRING_BASE.

Why isn't this sufficient?

> Solution:
> ----------
> VHOST_USER_RESET_OWNER API is issued by QEMU so that DPDK slave
> application is informed that mappings will be soon gone so that
> it can take necessary steps.
> 
> Shesha Sreenivasamurthy (1):
>   vhost-user: Slave crashes as Master unmaps vrings during guest reboot
> 
>  hw/virtio/vhost.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> -- 
> 1.9.5 (Apple Git-50.3)

Re: [Qemu-devel] [PATCH] vhost-user: Slave crashes as Master unmaps vrings during guest reboot

[Michael S. Tsirkin]

On Fri, Jan 15, 2016 at 12:12:44PM -0800, Shesha Sreenivasamurthy wrote:
> Send VHOST_USER_RESET_OWNER when the device is stopped.
> 
> Signed-off-by: Shesha Sreenivasamurthy <shesha@cisco.com>

That's a bad commit log.  A good one should describe why changes are
made, not what they are (that can be seen from the change diff).
The cover letter is no good for that, it
should just give some introduction in case of a large patchset.

I commented on the cover letter for now since that is where
you put the motivation.

> ---
>  hw/virtio/vhost.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index de29968..808184f 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1256,6 +1256,11 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
>                               hdev->vq_index + i);
>      }
>  

If rings keep going at this point then the index
retrieved above will be wrong, and we will not
be able to re-start the device.

> +    if (hdev->vhost_ops->vhost_reset_device(hdev) < 0) {
> +        fprintf(stderr, "vhost reset device %s failed\n", vdev->name);
> +        fflush(stderr);
> +    }
> +


Looks more or less like a revert of
	commit 12b8cbac3c8243b3dd485aaebb82547aefa06adb
	Author: Yuanhan Liu <yuanhan.liu@linux.intel.com>
	Date:   Fri Nov 13 15:24:10 2015 +0800

	    vhost: don't send RESET_OWNER at stop

If you still think it's a good idea, pls
copy people signed on that patch.


>      vhost_log_put(hdev, true);
>      hdev->started = false;
>      hdev->log = NULL;
> -- 
> 1.9.5 (Apple Git-50.3)

Re: [Qemu-devel] [PATCH] vhost-user: Slave crashes as Master unmaps vrings during guest reboot

[shesha Sreenivasamurthy (shesha)]

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

Got it. Thanks, I missed that line while reading the spec. Is docs/specs/vhost-user.txt the official spec ?

--
- Thanks
char * (*shesha) (uint64_t cache, uint8_t F00D)
{ return 0x0000C0DE; }

From: "Michael S. Tsirkin" <mst@redhat.com<mailto:mst@redhat.com>>
Date: Sunday, January 17, 2016 at 3:23 AM
To: Cisco Employee <shesha@cisco.com<mailto:shesha@cisco.com>>
Cc: "qemu-devel@nongnu.org<mailto:qemu-devel@nongnu.org>" <qemu-devel@nongnu.org<mailto:qemu-devel@nongnu.org>>
Subject: Re: [PATCH] vhost-user: Slave crashes as Master unmaps vrings during guest reboot

On Fri, Jan 15, 2016 at 12:12:43PM -0800, Shesha Sreenivasamurthy wrote:
Problem:
--------
If a guest has vhost-user enabled, then on reboot vhost_virtqueue_stop
is invoked. This unmaps vring memory mappings. However, it will not give
any indication to the underlying DPDK slave application about it.
Therefore, a pollmode DPDK driver tries to read the ring to check for
packets and segfaults.

The spec currently says:
Client must start ring upon receiving a kick (that is, detecting that file
descriptor is readable) on the descriptor specified by
VHOST_USER_SET_VRING_KICK, and stop ring upon receiving
VHOST_USER_GET_VRING_BASE.

Why isn't this sufficient?

Solution:
----------
VHOST_USER_RESET_OWNER API is issued by QEMU so that DPDK slave
application is informed that mappings will be soon gone so that
it can take necessary steps.
Shesha Sreenivasamurthy (1):
   vhost-user: Slave crashes as Master unmaps vrings during guest reboot
  hw/virtio/vhost.c | 5 +++++
  1 file changed, 5 insertions(+)
--
1.9.5 (Apple Git-50.3)


[-- Attachment #2: Type: text/html, Size: 4059 bytes --]