From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41277) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bi13H-0005Gr-Vb for qemu-devel@nongnu.org; Thu, 08 Sep 2016 11:15:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bi13B-0005bk-E3 for qemu-devel@nongnu.org; Thu, 08 Sep 2016 11:15:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57868) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bi13B-0005av-1y for qemu-devel@nongnu.org; Thu, 08 Sep 2016 11:15:37 -0400 Date: Thu, 8 Sep 2016 18:15:35 +0300 From: "Michael S. Tsirkin" Message-ID: <20160908180744-mutt-send-email-mst@kernel.org> References: <1473323650-13298-1-git-send-email-maxime.coquelin@redhat.com> <1473323650-13298-3-git-send-email-maxime.coquelin@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1473323650-13298-3-git-send-email-maxime.coquelin@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/2] vhost-user: only seek a reply if needed in set_mem_table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Maxime Coquelin Cc: qemu-devel@nongnu.org, prerna.saxena@nutanix.com, marcandre.lureau@redhat.com, peter.maydell@linaro.org 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. >=20 > 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 > Cc: Prerna Saxena > Cc: Marc-Andr=E9 Lureau > Signed-off-by: Maxime Coquelin > --- > hw/virtio/vhost-user.c | 7 +++++++ > hw/virtio/vhost.c | 10 ++++++++++ > include/hw/virtio/vhost.h | 1 + > 3 files changed, 18 insertions(+) >=20 > 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_d= ev *dev, > =20 > vhost_user_write(dev, &msg, fds, fd_num); > =20 > + 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_de= v *dev, > vhost_user_get_features(dev, &features); > } > =20 > + dev->mem_changed_req_sync =3D false; > + > return 0; > } > =20 > 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_d= ev *dev, > reg->guest_phys_addr =3D start_addr; > reg->userspace_addr =3D uaddr; > ++to; > + } else { > + /* Existing mapping updated, sync is required */ > + dev->mem_changed_req_sync =3D true; > } > + > assert(to <=3D dev->mem->nregions + 1); > dev->mem->nregions =3D to; > } > @@ -533,6 +537,7 @@ static void vhost_set_memory(MemoryListener *listen= er, > } else { > /* Remove old mapping for this memory, if any. */ > vhost_dev_unassign_memory(dev, start_addr, size); > + dev->mem_changed_req_sync =3D true; > } > dev->mem_changed_start_addr =3D MIN(dev->mem_changed_start_addr, s= tart_addr); > dev->mem_changed_end_addr =3D 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 =3D false; > hdev->started =3D false; > hdev->memory_changed =3D false; > + hdev->mem_changed_req_sync =3D false; > memory_listener_register(&hdev->memory_listener, &address_space_me= mory); > QLIST_INSERT_HEAD(&vhost_devices, hdev, entry); > return 0; > @@ -1301,6 +1307,10 @@ int vhost_dev_start(struct vhost_dev *hdev, Virt= IODevice *vdev) > if (r < 0) { > goto fail_features; > } > + > + /* First time the mem table is set, skip sync for completion */ > + hdev->mem_changed_req_sync =3D false; > + > r =3D 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; > --=20 > 2.7.4