[Qemu-devel] [PATCH 0/2] vhost-user: set_mem_table updates

[Qemu-devel] [PATCH 0/2] vhost-user: set_mem_table updates

[Maxime Coquelin]

The series first restore the GET_FEATURES call in vhost_set_mem_table(),
as the hang it introduced was due to the use of TCG in the vhost-user-test,
mode which is no more used since commit cdafe929 ("vhost-user-test: Use libqos
instead of pxe-virtio.rom").

Second patch aims at being as close as possible to original behaviour,
and so only request a sync after having updated the mem table when necessary.

I'll be glad to get your feedback, especially on patch 2, to confirm my
understanding is correct, and the sync is not required in the listed cases.

Thanks,
Maxime

Maxime Coquelin (2):
  Revert "Revert "vhost-user: Attempt to fix a race with
    set_mem_table.""
  vhost-user: only seek a reply if needed in set_mem_table

 hw/virtio/vhost-user.c    | 134 +++++++++++++++++++++++++---------------------
 hw/virtio/vhost.c         |  10 ++++
 include/hw/virtio/vhost.h |   1 +
 3 files changed, 85 insertions(+), 60 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] Revert "Revert "vhost-user: Attempt to fix a race with set_mem_table.""

[Maxime Coquelin]

This reverts commit 94c9cb31c04737f86be29afefbff401cd23bc24d.

Analysis of the race shows that it would happen only when QEMU relies
on TCG.

Since commit cdafe929 ("vhost-user-test: Use libqos instead of
pxe-virtio.rom"), vhost-user-test don't use TCG, so the race no more
appear.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marc-André Lureau <mlureau@redhat.com>
Cc: Prerna Saxena <prerna.saxena@nutanix.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 hw/virtio/vhost-user.c | 127 ++++++++++++++++++++++++++-----------------------
 1 file changed, 67 insertions(+), 60 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index b57454a..1a7d53c 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -263,66 +263,6 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
     return 0;
 }
 
-static int vhost_user_set_mem_table(struct vhost_dev *dev,
-                                    struct vhost_memory *mem)
-{
-    int fds[VHOST_MEMORY_MAX_NREGIONS];
-    int i, fd;
-    size_t fd_num = 0;
-    bool reply_supported = virtio_has_feature(dev->protocol_features,
-                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
-
-    VhostUserMsg msg = {
-        .request = VHOST_USER_SET_MEM_TABLE,
-        .flags = VHOST_USER_VERSION,
-    };
-
-    if (reply_supported) {
-        msg.flags |= VHOST_USER_NEED_REPLY_MASK;
-    }
-
-    for (i = 0; i < dev->mem->nregions; ++i) {
-        struct vhost_memory_region *reg = dev->mem->regions + i;
-        ram_addr_t offset;
-        MemoryRegion *mr;
-
-        assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
-        mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
-                                     &offset);
-        fd = memory_region_get_fd(mr);
-        if (fd > 0) {
-            msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
-            msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
-            msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
-            msg.payload.memory.regions[fd_num].mmap_offset = offset;
-            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
-            fds[fd_num++] = fd;
-        }
-    }
-
-    msg.payload.memory.nregions = fd_num;
-
-    if (!fd_num) {
-        error_report("Failed initializing vhost-user memory map, "
-                     "consider using -object memory-backend-file share=on");
-        return -1;
-    }
-
-    msg.size = sizeof(msg.payload.memory.nregions);
-    msg.size += sizeof(msg.payload.memory.padding);
-    msg.size += fd_num * sizeof(VhostUserMemoryRegion);
-
-    if (vhost_user_write(dev, &msg, fds, fd_num) < 0) {
-        return -1;
-    }
-
-    if (reply_supported) {
-        return process_message_reply(dev, msg.request);
-    }
-
-    return 0;
-}
-
 static int vhost_user_set_vring_addr(struct vhost_dev *dev,
                                      struct vhost_vring_addr *addr)
 {
@@ -537,6 +477,73 @@ static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
     return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features);
 }
 
+static int vhost_user_set_mem_table(struct vhost_dev *dev,
+                                    struct vhost_memory *mem)
+{
+    int fds[VHOST_MEMORY_MAX_NREGIONS];
+    int i, fd;
+    size_t fd_num = 0;
+    uint64_t features;
+    bool reply_supported = virtio_has_feature(dev->protocol_features,
+                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
+
+    VhostUserMsg msg = {
+        .request = VHOST_USER_SET_MEM_TABLE,
+        .flags = VHOST_USER_VERSION,
+    };
+
+    if (reply_supported) {
+        msg.flags |= VHOST_USER_NEED_REPLY_MASK;
+    }
+
+    for (i = 0; i < dev->mem->nregions; ++i) {
+        struct vhost_memory_region *reg = dev->mem->regions + i;
+        ram_addr_t offset;
+        MemoryRegion *mr;
+
+        assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
+        mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
+                                     &offset);
+        fd = memory_region_get_fd(mr);
+        if (fd > 0) {
+            msg.payload.memory.regions[fd_num].userspace_addr
+                                             = reg->userspace_addr;
+            msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
+            msg.payload.memory.regions[fd_num].guest_phys_addr
+                                             = reg->guest_phys_addr;
+            msg.payload.memory.regions[fd_num].mmap_offset = offset;
+            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
+            fds[fd_num++] = fd;
+        }
+    }
+
+    msg.payload.memory.nregions = fd_num;
+
+    if (!fd_num) {
+        error_report("Failed initializing vhost-user memory map, "
+                     "consider using -object memory-backend-file share=on");
+        return -1;
+    }
+
+    msg.size = sizeof(msg.payload.memory.nregions);
+    msg.size += sizeof(msg.payload.memory.padding);
+    msg.size += fd_num * sizeof(VhostUserMemoryRegion);
+
+    vhost_user_write(dev, &msg, fds, fd_num);
+
+    if (reply_supported) {
+        return process_message_reply(dev, msg.request);
+    } else {
+        /* Note: It is (yet) unknown when the client application has finished
+         * remapping the GPA.
+         * Attempt to prevent a race by sending a command that requires a reply.
+         */
+        vhost_user_get_features(dev, &features);
+    }
+
+    return 0;
+}
+
 static int vhost_user_set_owner(struct vhost_dev *dev)
 {
     VhostUserMsg msg = {
-- 
2.7.4

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

[Maxime Coquelin]

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.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Cc: Prerna Saxena <prerna.saxena@nutanix.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 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 {
@@ -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");
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

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

[Prerna Saxena]

Hi Maxime,


On 08/09/16 2:04 pm, "Maxime Coquelin" <maxime.coquelin@redhat.com> 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 don’t think so. 
This patch is not helping us solve the issue.
The hang introduced by original use of get_features() in set_mem_table was traced down to use of TCG mode for vhost-user test. This has now been fixed via:

-----
commit cdafe929615ec5eca71bcd5a3d12bab5678e5886
Author: Eduardo Habkost <ehabkost@redhat.com>
Date:   Fri Sep 2 15:59:43 2016 -0300


    vhost-user-test: Use libqos instead of pxe-virtio.rom
    
    vhost-user-test relies on iPXE just to initialize the virtio-net
    device, and doesn't do any actual packet tx/rx testing.
    
    In addition to that, the test relies on TCG, which is
    imcompatible with vhost. The test only worked by accident: a bug
    the memory backend initialization made memory regions not have
    the DIRTY_MEMORY_CODE bit set in dirty_log_mask.
    
    This changes vhost-user-test to initialize the virtio-net device
    using libqos, and not use TCG nor pxe-virtio.rom.
    
    Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

-------

So I think the original hang seems to have been fixed with Patch 1/2 of this series alone.

Regarding Patch 2/2:
This patch seems to filter responses from set_mem_table only for certain updates of memory regions. It violates the definition of the REPLY_ACK feature. This feature expects the client to send a response for every call of set_mem_table. And here, qemu exits the set_mem_table() function in some cases without even waiting for the reply that is going to come in.

As for use of this approach with get_features, we have already debated that on the list before : https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg00689.html
To quote:
"I do not entirely agree with that. The first set_mem_table command is not much
different from subsequent set_mem_table calls."

Regards,
Prerna

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

[Marc-André Lureau]

Hi

----- Original Message -----
> Regarding Patch 2/2:
> This patch seems to filter responses from set_mem_table only for certain
> updates of memory regions. It violates the definition of the REPLY_ACK
> feature. This feature expects the client to send a response for every call
> of set_mem_table. And here, qemu exits the set_mem_table() function in some
> cases without even waiting for the reply that is going to come in.
> 

Agreed with Prerna here,

Furthermore, I haven't followed closely the recents developments, and the commit message doesn't explain why it should not be necessary to sync the first time set-mem-table is called. Could you develop that?

thanks

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 07:56:38AM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > Regarding Patch 2/2:
> > This patch seems to filter responses from set_mem_table only for certain
> > updates of memory regions. It violates the definition of the REPLY_ACK
> > feature. This feature expects the client to send a response for every call
> > of set_mem_table. And here, qemu exits the set_mem_table() function in some
> > cases without even waiting for the reply that is going to come in.
> > 
> 
> Agreed with Prerna here,
> 
> Furthermore, I haven't followed closely the recents developments, and the commit message doesn't explain why it should not be necessary to sync the first time set-mem-table is called. Could you develop that?
> 
> thanks

Basically if we send set mem table when backend is not started,
there is no reason to wait for a response.

-- 
MST

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 <mst@redhat.com>
> Cc: Prerna Saxena <prerna.saxena@nutanix.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  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

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 11:33:34AM +0000, Prerna Saxena wrote:
> Hi Maxime,
> 
> 
> On 08/09/16 2:04 pm, "Maxime Coquelin" <maxime.coquelin@redhat.com> 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 don’t think so. 
> This patch is not helping us solve the issue.
> The hang introduced by original use of get_features() in set_mem_table
> was traced down to use of TCG mode for vhost-user test.

Right but we don't know for sure there are no backends
that do this kind of simplistic thing, and assume that
get features does not happen later. And for most setups
without memory hotplug, they would be right.


> This has now
> been fixed via:
> 
> -----
> commit cdafe929615ec5eca71bcd5a3d12bab5678e5886
> Author: Eduardo Habkost <ehabkost@redhat.com>
> Date:   Fri Sep 2 15:59:43 2016 -0300
> 
> 
>     vhost-user-test: Use libqos instead of pxe-virtio.rom
>     
>     vhost-user-test relies on iPXE just to initialize the virtio-net
>     device, and doesn't do any actual packet tx/rx testing.
>     
>     In addition to that, the test relies on TCG, which is
>     imcompatible with vhost. The test only worked by accident: a bug
>     the memory backend initialization made memory regions not have
>     the DIRTY_MEMORY_CODE bit set in dirty_log_mask.
>     
>     This changes vhost-user-test to initialize the virtio-net device
>     using libqos, and not use TCG nor pxe-virtio.rom.
>     
>     Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> -------
> 
> So I think the original hang seems to have been fixed with Patch 1/2 of this series alone.
> 
> Regarding Patch 2/2:
> This patch seems to filter responses from set_mem_table only for certain updates of memory regions. It violates the definition of the REPLY_ACK feature. This feature expects the client to send a response for every call of set_mem_table. And here, qemu exits the set_mem_table() function in some cases without even waiting for the reply that is going to come in.
> 
> As for use of this approach with get_features, we have already debated that on the list before : https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg00689.html
> To quote:
> "I do not entirely agree with that. The first set_mem_table command is not much
> different from subsequent set_mem_table calls."
> 
> Regards,
> Prerna
> 

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

[Maxime Coquelin]

On 09/08/2016 05:15 PM, Michael S. Tsirkin wrote:
> 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?

You are right, this is not working for the second part,
it was a misunderstanding from my side.

Now, for the first set_mem_table_call, Prerna is right,
the client having negotiated the reply_ack, we should wait for it.


I propose we drop patch 2, and only pick the first one.

Michael, ok for you?

Thanks,
Maxime