Re: [PATCH v4 7/7] vdpa: move memory listener register to vhost_vdpa_init

[Si-Wei Liu]

On 5/14/2025 10:42 PM, Michael S. Tsirkin wrote:
On Wed, May 07, 2025 at 02:46:47PM -0400, Jonah Palmer wrote:
From: Eugenio Pérez <epere...@redhat.com>

Current memory operations like pinning may take a lot of time at the
destination.  Currently they are done after the source of the migration is
stopped, and before the workload is resumed at the destination.  This is a
period where neigher traffic can flow, nor the VM workload can continue
(downtime).

We can do better as we know the memory layout of the guest RAM at the
destination from the moment that all devices are initializaed.  So
moving that operation allows QEMU to communicate the kernel the maps
while the workload is still running in the source, so Linux can start
mapping them.

As a small drawback, there is a time in the initialization where QEMU
cannot respond to QMP etc.  By some testing, this time is about
0.2seconds.  This may be further reduced (or increased) depending on the
vdpa driver and the platform hardware, and it is dominated by the cost
of memory pinning.

This matches the time that we move out of the called downtime window.
The downtime is measured as checking the trace timestamp from the moment
the source suspend the device to the moment the destination starts the
eight and last virtqueue pair.  For a 39G guest, it goes from ~2.2526
secs to 2.0949.

Signed-off-by: Eugenio Pérez <epere...@redhat.com>
Signed-off-by: Jonah Palmer <jonah.pal...@oracle.com>

v3:
---
just know that everything beyond this line is not going into
git commit log.
I guess that was the intent? Should be fine without them in the commit 
log I think. These are interim to capture what change was made to fix 
specific bug *in previous posted versions*.

(having said, please help edit the log and remove the "v3:" line which 
should be after the --- separator line, thx!)

-Siwei



Move memory listener unregistration from vhost_vdpa_reset_status to
vhost_vdpa_reset_device. By unregistering the listener here, we can
guarantee that every reset leaves the device in an expected state.
Also remove the duplicate call in vhost_vdpa_reset_status.

Reported-by: Lei Yang <leiy...@redhat.com>
Suggested-by: Si-Wei Liu <si-wei....@oracle.com>

--
v2:
Move the memory listener registration to vhost_vdpa_set_owner function.
In case of hotplug the vdpa device, the memory is already set up, and
leaving memory listener register call in the init function made maps
occur before set owner call.

To be 100% safe, let's put it right after set_owner call.

Reported-by: Lei Yang <leiy...@redhat.com>
---
  hw/virtio/vhost-vdpa.c | 35 ++++++++++++++++++++++++++++-------
  1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index de834f2ebd..e20da95f30 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -894,8 +894,14 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev)
  
      ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
      trace_vhost_vdpa_reset_device(dev);
+    if (ret) {
+        return ret;
+    }
+
+    memory_listener_unregister(&v->shared->listener);
+    v->shared->listener_registered = false;
      v->suspended = false;
-    return ret;
+    return 0;
  }
  
  static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
@@ -1379,6 +1385,11 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, 
bool started)
                           "IOMMU and try again");
              return -1;
          }
+        if (v->shared->listener_registered &&
+            dev->vdev->dma_as != v->shared->listener.address_space) {
+            memory_listener_unregister(&v->shared->listener);
+            v->shared->listener_registered = false;
+        }
          if (!v->shared->listener_registered) {
              memory_listener_register(&v->shared->listener, dev->vdev->dma_as);
              v->shared->listener_registered = true;
@@ -1392,8 +1403,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, 
bool started)
  
  static void vhost_vdpa_reset_status(struct vhost_dev *dev)
  {
-    struct vhost_vdpa *v = dev->opaque;
-
      if (!vhost_vdpa_last_dev(dev)) {
          return;
      }
@@ -1401,9 +1410,6 @@ static void vhost_vdpa_reset_status(struct vhost_dev *dev)
      vhost_vdpa_reset_device(dev);
      vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
                                 VIRTIO_CONFIG_S_DRIVER);
-    memory_listener_unregister(&v->shared->listener);
-    v->shared->listener_registered = false;
-
  }
  
  static int vhost_vdpa_set_log_base(struct vhost_dev *dev, uint64_t base,
@@ -1537,12 +1543,27 @@ static int vhost_vdpa_get_features(struct vhost_dev 
*dev,
  
  static int vhost_vdpa_set_owner(struct vhost_dev *dev)
  {
+    int r;
+    struct vhost_vdpa *v;
+
      if (!vhost_vdpa_first_dev(dev)) {
          return 0;
      }
  
      trace_vhost_vdpa_set_owner(dev);
-    return vhost_vdpa_call(dev, VHOST_SET_OWNER, NULL);
+    r = vhost_vdpa_call(dev, VHOST_SET_OWNER, NULL);
+    if (unlikely(r < 0)) {
+        return r;
+    }
+
+    /*
+     * Being optimistic and listening address space memory. If the device
+     * uses vIOMMU, it is changed at vhost_vdpa_dev_start.
+     */
+    v = dev->opaque;
+    memory_listener_register(&v->shared->listener, &address_space_memory);
+    v->shared->listener_registered = true;
+    return 0;
  }
  
  static int vhost_vdpa_vq_get_addr(struct vhost_dev *dev,
--
2.43.5