On Thu, Sep 08, 2016 at 10:34:10AM +0200, Maxime Coquelin wrote: > The goal of this patch is to only request a sync (reply_ack, > or get_features) in set_mem_table only when necessary. > > It should not be necessary the first time we set the table, > or when we add a new regions which hadn't been merged with an > existing ones.
I'm not sure I get the second part. If we don't sync, can't use of memory by guest bypass the request? Might this cause the backend to fail? I guess backend could try to recover by flushing the message queue, but if so, we probably should document this. And if not, why do we care about merged regions? > Suggested-by: Michael S. Tsirkin <[email protected]> > Cc: Prerna Saxena <[email protected]> > Cc: Marc-André Lureau <[email protected]> > Signed-off-by: Maxime Coquelin <[email protected]> > --- > hw/virtio/vhost-user.c | 7 +++++++ > hw/virtio/vhost.c | 10 ++++++++++ > include/hw/virtio/vhost.h | 1 + > 3 files changed, 18 insertions(+) > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 1a7d53c..ca41728 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -531,6 +531,11 @@ static int vhost_user_set_mem_table(struct vhost_dev > *dev, > > vhost_user_write(dev, &msg, fds, fd_num); > > + if (!dev->mem_changed_req_sync) { > + /* The update only add regions, skip the sync */ > + return 0; > + } > + > if (reply_supported) { > return process_message_reply(dev, msg.request); > } else { This still sets VHOST_USER_NEED_REPLY_MASK - I think we should clear reply_supported and avoid setting that in requests. > @@ -541,6 +546,8 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev, > vhost_user_get_features(dev, &features); > } > > + dev->mem_changed_req_sync = false; > + > return 0; > } > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 3d0c807..e653067 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -303,7 +303,11 @@ static void vhost_dev_assign_memory(struct vhost_dev > *dev, > reg->guest_phys_addr = start_addr; > reg->userspace_addr = uaddr; > ++to; > + } else { > + /* Existing mapping updated, sync is required */ > + dev->mem_changed_req_sync = true; > } > + > assert(to <= dev->mem->nregions + 1); > dev->mem->nregions = to; > } > @@ -533,6 +537,7 @@ static void vhost_set_memory(MemoryListener *listener, > } else { > /* Remove old mapping for this memory, if any. */ > vhost_dev_unassign_memory(dev, start_addr, size); > + dev->mem_changed_req_sync = true; > } > dev->mem_changed_start_addr = MIN(dev->mem_changed_start_addr, > start_addr); > dev->mem_changed_end_addr = MAX(dev->mem_changed_end_addr, start_addr + > size - 1); > @@ -1126,6 +1131,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, > hdev->log_enabled = false; > hdev->started = false; > hdev->memory_changed = false; > + hdev->mem_changed_req_sync = false; > memory_listener_register(&hdev->memory_listener, &address_space_memory); > QLIST_INSERT_HEAD(&vhost_devices, hdev, entry); > return 0; > @@ -1301,6 +1307,10 @@ int vhost_dev_start(struct vhost_dev *hdev, > VirtIODevice *vdev) > if (r < 0) { > goto fail_features; > } > + > + /* First time the mem table is set, skip sync for completion */ > + hdev->mem_changed_req_sync = false; > + > r = hdev->vhost_ops->vhost_set_mem_table(hdev, hdev->mem); > if (r < 0) { > VHOST_OPS_DEBUG("vhost_set_mem_table failed"); Kind of asymmetrical. How about we set it to false on stop, and to true on start? Seems cleaner to me ... > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h > index e433089..4bbf36a 100644 > --- a/include/hw/virtio/vhost.h > +++ b/include/hw/virtio/vhost.h > @@ -55,6 +55,7 @@ struct vhost_dev { > uint64_t log_size; > Error *migration_blocker; > bool memory_changed; > + bool mem_changed_req_sync; > hwaddr mem_changed_start_addr; > hwaddr mem_changed_end_addr; > const VhostOps *vhost_ops; > -- > 2.7.4
