[Qemu-devel] [PATCH 0/2] vhost: two fixes

[Qemu-devel] [PATCH 0/2] vhost: two fixes

[Jay Zhou]

Jay Zhou (2):
  vhost: add used memslot number for vhost-user
  vhost: double check memslot number

 hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++
 hw/virtio/vhost.c                 | 49 ++++++++++++++++++++++++++++++++++-----
 include/hw/virtio/vhost-backend.h |  4 ++++
 3 files changed, 78 insertions(+), 6 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] vhost: add used memslot number for vhost-user

[Jay Zhou]

Used_memslots is equal to dev->mem->nregions now, it is true for
vhost kernel, but not for vhost user, which uses the memory regions
that have file descriptor. In fact, not all of the memory regions
have file descriptor.
It is usefully in some scenarios, e.g. used_memslots is 8, and only
5 memory slots can be used by vhost user, it is failed to hot plug
a new memory RAM because vhost_has_free_slot just returned false,
but we can hot plug it safely in fact.

Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
Signed-off-by: Zhe Liu <gary.liuzhe@huawei.com>
---
 hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++++++++
 hw/virtio/vhost.c                 | 25 +++++++++++++++++++++----
 include/hw/virtio/vhost-backend.h |  4 ++++
 3 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 093675e..58a4cec 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -122,6 +122,8 @@ static VhostUserMsg m __attribute__ ((unused));
 /* The version of the protocol we support */
 #define VHOST_USER_VERSION    (0x1)
 
+static unsigned int vhost_user_used_memslots;
+
 struct vhost_user {
     CharBackend *chr;
     int slave_fd;
@@ -922,6 +924,33 @@ static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
     /* No-op as the receive channel is not dedicated to IOTLB messages. */
 }
 
+static void vhost_user_set_used_memslots(struct vhost_dev *dev)
+{
+    int counter = 0;
+    int i;
+
+    for (i = 0; i < dev->mem->nregions; ++i) {
+        struct vhost_memory_region *reg = dev->mem->regions + i;
+        ram_addr_t offset;
+        MemoryRegion *mr;
+        int fd;
+
+        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) {
+            counter++;
+        }
+    }
+    vhost_user_used_memslots = counter;
+}
+
+static unsigned int vhost_user_get_used_memslots(void)
+{
+    return vhost_user_used_memslots;
+}
+
 const VhostOps user_ops = {
         .backend_type = VHOST_BACKEND_TYPE_USER,
         .vhost_backend_init = vhost_user_init,
@@ -948,4 +977,6 @@ const VhostOps user_ops = {
         .vhost_net_set_mtu = vhost_user_net_set_mtu,
         .vhost_set_iotlb_callback = vhost_user_set_iotlb_callback,
         .vhost_send_device_iotlb_msg = vhost_user_send_device_iotlb_msg,
+        .vhost_set_used_memslots = vhost_user_set_used_memslots,
+        .vhost_get_used_memslots = vhost_user_get_used_memslots,
 };
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index e4290ce..0cf8a53 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -49,14 +49,21 @@ static QLIST_HEAD(, vhost_dev) vhost_devices =
 
 bool vhost_has_free_slot(void)
 {
-    unsigned int slots_limit = ~0U;
     struct vhost_dev *hdev;
 
     QLIST_FOREACH(hdev, &vhost_devices, entry) {
         unsigned int r = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
-        slots_limit = MIN(slots_limit, r);
+
+        if (hdev->vhost_ops->vhost_get_used_memslots) {
+            if (hdev->vhost_ops->vhost_get_used_memslots() >= r) {
+                return false;
+            }
+        } else if (used_memslots >= r) {
+            return false;
+        }
     }
-    return slots_limit > used_memslots;
+
+    return true;
 }
 
 static void vhost_dev_sync_region(struct vhost_dev *dev,
@@ -607,6 +614,9 @@ static void vhost_set_memory(MemoryListener *listener,
     dev->mem_changed_end_addr = MAX(dev->mem_changed_end_addr, start_addr + size - 1);
     dev->memory_changed = true;
     used_memslots = dev->mem->nregions;
+    if (dev->vhost_ops->vhost_set_used_memslots) {
+        dev->vhost_ops->vhost_set_used_memslots(dev);
+    }
 }
 
 static bool vhost_section(MemoryRegionSection *section)
@@ -1239,6 +1249,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     uint64_t features;
     int i, r, n_initialized_vqs = 0;
     Error *local_err = NULL;
+    unsigned int n_memslots = 0;
 
     hdev->vdev = NULL;
     hdev->migration_blocker = NULL;
@@ -1251,7 +1262,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
         goto fail;
     }
 
-    if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
+    if (hdev->vhost_ops->vhost_get_used_memslots) {
+        n_memslots = hdev->vhost_ops->vhost_get_used_memslots();
+    } else {
+        n_memslots = used_memslots;
+    }
+
+    if (n_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
         error_report("vhost backend memory slots limit is less"
                 " than current number of present memory slots");
         r = -1;
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index a7a5f22..19961b5 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -84,6 +84,8 @@ typedef void (*vhost_set_iotlb_callback_op)(struct vhost_dev *dev,
                                            int enabled);
 typedef int (*vhost_send_device_iotlb_msg_op)(struct vhost_dev *dev,
                                               struct vhost_iotlb_msg *imsg);
+typedef void (*vhost_set_used_memslots_op)(struct vhost_dev *dev);
+typedef unsigned int (*vhost_get_used_memslots_op)(void);
 
 typedef struct VhostOps {
     VhostBackendType backend_type;
@@ -118,6 +120,8 @@ typedef struct VhostOps {
     vhost_vsock_set_running_op vhost_vsock_set_running;
     vhost_set_iotlb_callback_op vhost_set_iotlb_callback;
     vhost_send_device_iotlb_msg_op vhost_send_device_iotlb_msg;
+    vhost_set_used_memslots_op vhost_set_used_memslots;
+    vhost_get_used_memslots_op vhost_get_used_memslots;
 } VhostOps;
 
 extern const VhostOps user_ops;
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] vhost: double check memslot number

[Jay Zhou]

If the VM already has N(N>=8) available memory slots for vhost user,
the VM will be crashed in vhost_user_set_mem_table if we try to
hotplug the first vhost user NIC.
This patch checks if memslot number exceeded or not after updating
vhost_user_used_memslots.

Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
---
 hw/virtio/vhost.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 0cf8a53..33aed1f 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1243,13 +1243,31 @@ static void vhost_virtqueue_cleanup(struct vhost_virtqueue *vq)
     event_notifier_cleanup(&vq->masked_notifier);
 }
 
+static bool vhost_dev_memslots_is_exceeded(struct vhost_dev *hdev)
+{
+    unsigned int n_memslots = 0;
+
+    if (hdev->vhost_ops->vhost_get_used_memslots) {
+        n_memslots = hdev->vhost_ops->vhost_get_used_memslots();
+    } else {
+        n_memslots = used_memslots;
+    }
+
+    if (n_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
+        error_report("vhost backend memory slots limit is less"
+                " than current number of present memory slots");
+        return true;
+    }
+
+    return false;
+}
+
 int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
                    VhostBackendType backend_type, uint32_t busyloop_timeout)
 {
     uint64_t features;
     int i, r, n_initialized_vqs = 0;
     Error *local_err = NULL;
-    unsigned int n_memslots = 0;
 
     hdev->vdev = NULL;
     hdev->migration_blocker = NULL;
@@ -1262,15 +1280,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
         goto fail;
     }
 
-    if (hdev->vhost_ops->vhost_get_used_memslots) {
-        n_memslots = hdev->vhost_ops->vhost_get_used_memslots();
-    } else {
-        n_memslots = used_memslots;
-    }
-
-    if (n_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
-        error_report("vhost backend memory slots limit is less"
-                " than current number of present memory slots");
+    if (vhost_dev_memslots_is_exceeded(hdev)) {
         r = -1;
         goto fail;
     }
@@ -1356,6 +1366,16 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     hdev->memory_changed = false;
     memory_listener_register(&hdev->memory_listener, &address_space_memory);
     QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
+
+    if (vhost_dev_memslots_is_exceeded(hdev)) {
+        r = -1;
+        if (busyloop_timeout) {
+            goto fail_busyloop;
+        } else {
+            goto fail;
+        }
+    }
+
     return 0;
 
 fail_busyloop:
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/2] vhost: two fixes

[Michael S. Tsirkin]

On Fri, Dec 15, 2017 at 12:36:30AM +0800, Jay Zhou wrote:
> Jay Zhou (2):
>   vhost: add used memslot number for vhost-user
>   vhost: double check memslot number
> 
>  hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++
>  hw/virtio/vhost.c                 | 49 ++++++++++++++++++++++++++++++++++-----
>  include/hw/virtio/vhost-backend.h |  4 ++++
>  3 files changed, 78 insertions(+), 6 deletions(-)

Cc two developers working on these files right now.

> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH 0/2] vhost: two fixes

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Fri, Dec 15, 2017 at 12:36:30AM +0800, Jay Zhou wrote:
> > Jay Zhou (2):
> >   vhost: add used memslot number for vhost-user
> >   vhost: double check memslot number
> > 
> >  hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++
> >  hw/virtio/vhost.c                 | 49 ++++++++++++++++++++++++++++++++++-----
> >  include/hw/virtio/vhost-backend.h |  4 ++++
> >  3 files changed, 78 insertions(+), 6 deletions(-)
> 
> Cc two developers working on these files right now.

I have to admit to not understanding the 'used_memslots' variable.

* It's a global in vhost.c
* but set by vhost_set_memory that's called from the listener associated
  with each individual vhost
* While they're probably always the same, the merging code calls
  the vhost_backend_can_merge method for each device, so the number
  of regions can be different.

Dave

> > -- 
> > 1.8.3.1
> > 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/2] vhost: two fixes

[Zhoujian (jay)]

Hi Dave,

> -----Original Message-----
> From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> Sent: Friday, December 15, 2017 3:49 AM
> To: Michael S. Tsirkin <mst@redhat.com>
> Cc: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org;
> Huangweidong (C) <weidong.huang@huawei.com>; Gonglei (Arei)
> <arei.gonglei@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>;
> Liuzhe (Cloud Open Labs, NFV) <gary.liuzhe@huawei.com>; Igor Mammedov
> <imammedo@redhat.com>
> Subject: Re: [PATCH 0/2] vhost: two fixes
> 
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Fri, Dec 15, 2017 at 12:36:30AM +0800, Jay Zhou wrote:
> > > Jay Zhou (2):
> > >   vhost: add used memslot number for vhost-user
> > >   vhost: double check memslot number
> > >
> > >  hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++
> > >  hw/virtio/vhost.c                 | 49
> ++++++++++++++++++++++++++++++++++-----
> > >  include/hw/virtio/vhost-backend.h |  4 ++++
> > >  3 files changed, 78 insertions(+), 6 deletions(-)
> >
> > Cc two developers working on these files right now.
> 
> I have to admit to not understanding the 'used_memslots' variable.
> 
> * It's a global in vhost.c
> * but set by vhost_set_memory that's called from the listener associated
>   with each individual vhost
> * While they're probably always the same, the merging code calls
>   the vhost_backend_can_merge method for each device, so the number
>   of regions can be different.
> 

Your mean for some devices the new added MemoryRegionSection can be merged,
but for others it can not be merged? IIUC the vhost_mem for each vhost_dev
is the same.

Meanwhile, I think it is more reasonable to add globals in vhost-backend.c
and vhost-user.c respectively instead of 'used_memslots'. The reason
is explained in patch 1. What do you think?

Regards,
Jay

Re: [Qemu-devel] [PATCH 0/2] vhost: two fixes

[Michael S. Tsirkin]

On Fri, Dec 15, 2017 at 02:38:35AM +0000, Zhoujian (jay) wrote:
> Hi Dave,
> 
> > -----Original Message-----
> > From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> > Sent: Friday, December 15, 2017 3:49 AM
> > To: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org;
> > Huangweidong (C) <weidong.huang@huawei.com>; Gonglei (Arei)
> > <arei.gonglei@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>;
> > Liuzhe (Cloud Open Labs, NFV) <gary.liuzhe@huawei.com>; Igor Mammedov
> > <imammedo@redhat.com>
> > Subject: Re: [PATCH 0/2] vhost: two fixes
> > 
> > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > > On Fri, Dec 15, 2017 at 12:36:30AM +0800, Jay Zhou wrote:
> > > > Jay Zhou (2):
> > > >   vhost: add used memslot number for vhost-user
> > > >   vhost: double check memslot number
> > > >
> > > >  hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++
> > > >  hw/virtio/vhost.c                 | 49
> > ++++++++++++++++++++++++++++++++++-----
> > > >  include/hw/virtio/vhost-backend.h |  4 ++++
> > > >  3 files changed, 78 insertions(+), 6 deletions(-)
> > >
> > > Cc two developers working on these files right now.
> > 
> > I have to admit to not understanding the 'used_memslots' variable.
> > 
> > * It's a global in vhost.c
> > * but set by vhost_set_memory that's called from the listener associated
> >   with each individual vhost
> > * While they're probably always the same, the merging code calls
> >   the vhost_backend_can_merge method for each device, so the number
> >   of regions can be different.
> > 
> 
> Your mean for some devices the new added MemoryRegionSection can be merged,
> but for others it can not be merged? IIUC the vhost_mem for each vhost_dev
> is the same.
> 
> Meanwhile, I think it is more reasonable to add globals in vhost-backend.c
> and vhost-user.c respectively instead of 'used_memslots'. The reason
> is explained in patch 1. What do you think?
> 
> Regards,
> Jay

I'd rather avoid globals completely if possible.

-- 
MST

Re: [Qemu-devel] [PATCH 0/2] vhost: two fixes

[Zhoujian (jay)]

Hi Michael,

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Friday, December 15, 2017 12:36 PM
> To: Zhoujian (jay) <jianjay.zhou@huawei.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>; qemu-devel@nongnu.org;
> Huangweidong (C) <weidong.huang@huawei.com>; Gonglei (Arei)
> <arei.gonglei@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>;
> Liuzhe (Cloud Open Labs, NFV) <gary.liuzhe@huawei.com>; Igor Mammedov
> <imammedo@redhat.com>
> Subject: Re: [PATCH 0/2] vhost: two fixes
> 
> On Fri, Dec 15, 2017 at 02:38:35AM +0000, Zhoujian (jay) wrote:
> > Hi Dave,
> >
> > > -----Original Message-----
> > > From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> > > Sent: Friday, December 15, 2017 3:49 AM
> > > To: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org;
> > > Huangweidong (C) <weidong.huang@huawei.com>; Gonglei (Arei)
> > > <arei.gonglei@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>;
> > > Liuzhe (Cloud Open Labs, NFV) <gary.liuzhe@huawei.com>; Igor
> > > Mammedov <imammedo@redhat.com>
> > > Subject: Re: [PATCH 0/2] vhost: two fixes
> > >
> > > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > > > On Fri, Dec 15, 2017 at 12:36:30AM +0800, Jay Zhou wrote:
> > > > > Jay Zhou (2):
> > > > >   vhost: add used memslot number for vhost-user
> > > > >   vhost: double check memslot number
> > > > >
> > > > >  hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++
> > > > >  hw/virtio/vhost.c                 | 49
> > > ++++++++++++++++++++++++++++++++++-----
> > > > >  include/hw/virtio/vhost-backend.h |  4 ++++
> > > > >  3 files changed, 78 insertions(+), 6 deletions(-)
> > > >
> > > > Cc two developers working on these files right now.
> > >
> > > I have to admit to not understanding the 'used_memslots' variable.
> > >
> > > * It's a global in vhost.c
> > > * but set by vhost_set_memory that's called from the listener
> associated
> > >   with each individual vhost
> > > * While they're probably always the same, the merging code calls
> > >   the vhost_backend_can_merge method for each device, so the number
> > >   of regions can be different.
> > >
> >
> > Your mean for some devices the new added MemoryRegionSection can be
> > merged, but for others it can not be merged? IIUC the vhost_mem for
> > each vhost_dev is the same.
> >
> > Meanwhile, I think it is more reasonable to add globals in
> > vhost-backend.c and vhost-user.c respectively instead of
> > 'used_memslots'. The reason is explained in patch 1. What do you think?
> >
> > Regards,
> > Jay
> 
> I'd rather avoid globals completely if possible.
> 

It is possible, we could add a 'used_memslots' variable in struct vhost_dev
for per device. I will try to do it in v2.

Regards,
Jay

Re: [Qemu-devel] [PATCH 0/2] vhost: two fixes

[Zhoujian (jay)]

Hi Michael,

> -----Original Message-----
> From: Zhoujian (jay)
> Sent: Friday, December 15, 2017 12:52 PM
> To: 'Michael S. Tsirkin' <mst@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>; qemu-devel@nongnu.org;
> Huangweidong (C) <weidong.huang@huawei.com>; Gonglei (Arei)
> <arei.gonglei@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>;
> Liuzhe (Cloud Open Labs, NFV) <gary.liuzhe@huawei.com>; Igor Mammedov
> <imammedo@redhat.com>
> Subject: RE: [PATCH 0/2] vhost: two fixes
> 
> Hi Michael,
> 
> > -----Original Message-----
> > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > Sent: Friday, December 15, 2017 12:36 PM
> > To: Zhoujian (jay) <jianjay.zhou@huawei.com>
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>;
> > qemu-devel@nongnu.org; Huangweidong (C) <weidong.huang@huawei.com>;
> > Gonglei (Arei) <arei.gonglei@huawei.com>; wangxin (U)
> > <wangxinxin.wang@huawei.com>; Liuzhe (Cloud Open Labs, NFV)
> > <gary.liuzhe@huawei.com>; Igor Mammedov <imammedo@redhat.com>
> > Subject: Re: [PATCH 0/2] vhost: two fixes
> >
> > On Fri, Dec 15, 2017 at 02:38:35AM +0000, Zhoujian (jay) wrote:
> > > Hi Dave,
> > >
> > > > -----Original Message-----
> > > > From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> > > > Sent: Friday, December 15, 2017 3:49 AM
> > > > To: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Zhoujian (jay) <jianjay.zhou@huawei.com>;
> > > > qemu-devel@nongnu.org; Huangweidong (C)
> > > > <weidong.huang@huawei.com>; Gonglei (Arei)
> > > > <arei.gonglei@huawei.com>; wangxin (U)
> > > > <wangxinxin.wang@huawei.com>; Liuzhe (Cloud Open Labs, NFV)
> > > > <gary.liuzhe@huawei.com>; Igor Mammedov <imammedo@redhat.com>
> > > > Subject: Re: [PATCH 0/2] vhost: two fixes
> > > >
> > > > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > > > > On Fri, Dec 15, 2017 at 12:36:30AM +0800, Jay Zhou wrote:
> > > > > > Jay Zhou (2):
> > > > > >   vhost: add used memslot number for vhost-user
> > > > > >   vhost: double check memslot number
> > > > > >
> > > > > >  hw/virtio/vhost-user.c            | 31
> +++++++++++++++++++++++++
> > > > > >  hw/virtio/vhost.c                 | 49
> > > > ++++++++++++++++++++++++++++++++++-----
> > > > > >  include/hw/virtio/vhost-backend.h |  4 ++++
> > > > > >  3 files changed, 78 insertions(+), 6 deletions(-)
> > > > >
> > > > > Cc two developers working on these files right now.
> > > >
> > > > I have to admit to not understanding the 'used_memslots' variable.
> > > >
> > > > * It's a global in vhost.c
> > > > * but set by vhost_set_memory that's called from the listener
> > associated
> > > >   with each individual vhost
> > > > * While they're probably always the same, the merging code calls
> > > >   the vhost_backend_can_merge method for each device, so the number
> > > >   of regions can be different.
> > > >
> > >
> > > Your mean for some devices the new added MemoryRegionSection can be
> > > merged, but for others it can not be merged? IIUC the vhost_mem for
> > > each vhost_dev is the same.
> > >
> > > Meanwhile, I think it is more reasonable to add globals in
> > > vhost-backend.c and vhost-user.c respectively instead of
> > > 'used_memslots'. The reason is explained in patch 1. What do you think?
> > >
> > > Regards,
> > > Jay
> >
> > I'd rather avoid globals completely if possible.
> >
> 
> It is possible, we could add a 'used_memslots' variable in struct
> vhost_dev for per device. I will try to do it in v2.
> 

If the globals don't exist, the disadvantage I found is that the check
"if memslots number exceeds" will be moved from the beginning to the end
in vhost_dev_init, does it acceptable? Or are there other ideas to avoid
globals?

To be honest, I prefer to add globals in vhost-backend.c and vhost-user.c
respectively, the value of used_memslots for the same type of backend is the
same.

Regards,
Jay

Re: [Qemu-devel] [PATCH 0/2] vhost: two fixes

[Dr. David Alan Gilbert]

* Zhoujian (jay) (jianjay.zhou@huawei.com) wrote:
> Hi Dave,
> 
> > -----Original Message-----
> > From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> > Sent: Friday, December 15, 2017 3:49 AM
> > To: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org;
> > Huangweidong (C) <weidong.huang@huawei.com>; Gonglei (Arei)
> > <arei.gonglei@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>;
> > Liuzhe (Cloud Open Labs, NFV) <gary.liuzhe@huawei.com>; Igor Mammedov
> > <imammedo@redhat.com>
> > Subject: Re: [PATCH 0/2] vhost: two fixes
> > 
> > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > > On Fri, Dec 15, 2017 at 12:36:30AM +0800, Jay Zhou wrote:
> > > > Jay Zhou (2):
> > > >   vhost: add used memslot number for vhost-user
> > > >   vhost: double check memslot number
> > > >
> > > >  hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++
> > > >  hw/virtio/vhost.c                 | 49
> > ++++++++++++++++++++++++++++++++++-----
> > > >  include/hw/virtio/vhost-backend.h |  4 ++++
> > > >  3 files changed, 78 insertions(+), 6 deletions(-)
> > >
> > > Cc two developers working on these files right now.
> > 
> > I have to admit to not understanding the 'used_memslots' variable.
> > 
> > * It's a global in vhost.c
> > * but set by vhost_set_memory that's called from the listener associated
> >   with each individual vhost
> > * While they're probably always the same, the merging code calls
> >   the vhost_backend_can_merge method for each device, so the number
> >   of regions can be different.
> > 
> 
> Your mean for some devices the new added MemoryRegionSection can be merged,
> but for others it can not be merged?

That's my understanding, because of the call to vhost_backend_can_merge

> IIUC the vhost_mem for each vhost_dev
> is the same.
> 
> Meanwhile, I think it is more reasonable to add globals in vhost-backend.c
> and vhost-user.c respectively instead of 'used_memslots'. The reason
> is explained in patch 1. What do you think?

If we need globals I'm not sure it matters that much where they live.

Dave

> Regards,
> Jay
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK