Re: [PATCH v6 1/2] vhost-user: remove VirtQ notifier restore

["Xueming(Steven) Li" <xuemingl@nvidia.com>]

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

v7 sent with more description, please check.


Thanks,

Xueming


On Fri, 2022-02-04 at 07:25 -0500, Michael S. Tsirkin wrote:

I dropped this for now as I'm a bit lost with this patchset.

Cc Raphael maybe he'll understand it better.


On Wed, Jan 12, 2022 at 03:05:15PM +0000, Xueming(Steven) Li wrote:

On Wed, 2021-11-03 at 16:30 -0400, Michael S. Tsirkin wrote:

On Wed, Nov 03, 2021 at 02:48:41PM +0000, Xueming(Steven) Li wrote:

On Tue, 2021-11-02 at 02:49 -0400, Michael S. Tsirkin wrote:

On Tue, Nov 02, 2021 at 06:08:39AM +0000, Xueming(Steven) Li wrote:

On Mon, 2021-11-01 at 17:06 -0400, Michael S. Tsirkin wrote:

On Mon, Nov 01, 2021 at 04:38:12PM +0800, Xueming Li wrote:

When vhost-user vdpa client suspend, backend may close all resources,

VQ notifier mmap address become invalid, restore MR which contains

the invalid address is wrong. vdpa client will set VQ notifier after

reconnect.


This patch removes VQ notifier restore and related flags to avoid reusing

invalid address.


Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")

Cc: qemu-stable@nongnu.org<mailto:qemu-stable@nongnu.org>

Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com<mailto:zhangyuwei.9149@bytedance.com>>

Signed-off-by: Xueming Li <xuemingl@nvidia.com<mailto:xuemingl@nvidia.com>>

---

 hw/virtio/vhost-user.c         | 19 +------------------

 include/hw/virtio/vhost-user.h |  1 -

 2 files changed, 1 insertion(+), 19 deletions(-)


diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c

index bf6e50223c..c671719e9b 100644

--- a/hw/virtio/vhost-user.c

+++ b/hw/virtio/vhost-user.c

@@ -1143,19 +1143,6 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,

     return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);

 }



-static void vhost_user_host_notifier_restore(struct vhost_dev *dev,

-                                             int queue_idx)

-{

-    struct vhost_user *u = dev->opaque;

-    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];

-    VirtIODevice *vdev = dev->vdev;

-

-    if (n->addr && !n->set) {

-        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true);

-        n->set = true;

-    }

-}

-

 static void vhost_user_host_notifier_remove(struct vhost_dev *dev,

                                             int queue_idx)

 {

@@ -1163,17 +1150,14 @@ static void vhost_user_host_notifier_remove(struct vhost_dev *dev,

     VhostUserHostNotifier *n = &u->user->notifier[queue_idx];

     VirtIODevice *vdev = dev->vdev;



-    if (n->addr && n->set) {

+    if (n->addr) {

         virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);

-        n->set = false;

     }

 }



So on vq stop we still remove the notifier...



 static int vhost_user_set_vring_base(struct vhost_dev *dev,

                                      struct vhost_vring_state *ring)

 {

-    vhost_user_host_notifier_restore(dev, ring->index);

-

     return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);

 }




but on vq start we do not reinstate it? Does this not mean that

notifiers won't work after stop then start?



Yes, backend initially work w/o host notifier, request VM driver to

install notifier if needed - call this function through slave socket.


I think it's cleaner if qemu handles this itself like it did before, it

knows vm is stopped without getting called.


If vhost play as server, there are 2 scenario that remove the notifier:

1. VM suspend: backend still there, it's okay to keep mmap address.

2. vhost backend stopped or process killed: resources from backend

should be released. That's why patch 2/2 munmap in notifier remove

function. Then the restore function get nothing to restore, maybe I

shouldn't reverse patch order.


I can't say I understand what you mean here. Do you plan to change

the patchset in some way?

When you do, pls include a cover letter with a changelog and

Cc all people you include on patches on the cover letter too.


Here is the detail of the problem I encountered, my vhost backend is

DPDK vdpa user space application. Notifier address is set when vdpa ask

qemu to mmap a FD and an offset from vdpa, see function

vhost_user_slave_handle_vring_host_notifier(). If the vdpa application

restart of get killed for some reason,

vhost_user_host_notifier_remove() is called and notifier MR is

uninstalled, the notifier address that retrieved from mmap is

referencing to an invalid FD, not released. This will cause HW

resources on kernel side still referenced, most important, when vdpa

connection restored, this invalid notifier will be be restored as

notifier MR.


To resolve it, have to remove the notifer restore mechanism, vDPA

application will issue client socket request again to install notifier

to VM, so no concern that the notifier will be lost after resume.


Since vdpa might be killed, no chance to notify qemu to remove

notifier. An alternative solution is to detect sock disconnection and

unmmap notifier, but it looks more complex to me. How do you think?







@@ -1538,7 +1522,6 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,

     }



     n->addr = addr;

-    n->set = true;



     return 0;

 }

diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h

index a9abca3288..f6012b2078 100644

--- a/include/hw/virtio/vhost-user.h

+++ b/include/hw/virtio/vhost-user.h

@@ -14,7 +14,6 @@

 typedef struct VhostUserHostNotifier {

     MemoryRegion mr;

     void *addr;

-    bool set;

 } VhostUserHostNotifier;



 typedef struct VhostUserState {

--

2.33.0










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