Re: [Qemu-devel] [PATCH 2/2] vhost-user: only seek a reply if needed in set_mem_table

[Michael S. Tsirkin]

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 <address@hidden>
> Cc: Prerna Saxena <address@hidden>
> Cc: Marc-André Lureau <address@hidden>
> Signed-off-by: Maxime Coquelin <address@hidden>
> ---
>  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