[Qemu-devel] [PATCH for-2.4 v2 0/2] vhost: check if vhost has capacity for hotplugged memory

[Qemu-devel] [PATCH for-2.4 v2 0/2] vhost: check if vhost has capacity for hotplugged memory

[Igor Mammedov]

v1->v2:
  * replace probbing with checking for
    /sys/module/vhost/parameters/max_mem_regions and
    if it's missing has non wrong value return
    hardcoded legacy limit (64 slots).

it's defensive patchset which helps to avoid QEMU crashing
at memory hotplug time by checking that vhost has free capacity
for an additional memory slot.


Igor Mammedov (2):
  vhost: add vhost_has_free_slot() interface
  pc-dimm: add vhost slots limit check before commiting to hotplug

 hw/mem/pc-dimm.c                  |  7 +++++++
 hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
 hw/virtio/vhost-user.c            |  8 +++++++-
 hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
 include/hw/virtio/vhost-backend.h |  2 ++
 include/hw/virtio/vhost.h         |  1 +
 stubs/Makefile.objs               |  1 +
 stubs/vhost.c                     |  6 ++++++
 8 files changed, 65 insertions(+), 2 deletions(-)
 create mode 100644 stubs/vhost.c

-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.4 v2 1/2] vhost: add vhost_has_free_slot() interface

[Igor Mammedov]

it will allow for other parts of QEMU check if it's safe
to map memory region during hotplug/runtime.
That way hotplug path will have a chance to cancel
hotplug operation instead of crashing in vhost_commit().

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * replace probbing with checking for
    /sys/module/vhost/parameters/max_mem_regions and
    if it's missing has non wrong value return
    hardcoded legacy limit (64 slots).
---
 hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
 hw/virtio/vhost-user.c            |  8 +++++++-
 hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
 include/hw/virtio/vhost-backend.h |  2 ++
 include/hw/virtio/vhost.h         |  1 +
 stubs/Makefile.objs               |  1 +
 stubs/vhost.c                     |  6 ++++++
 7 files changed, 58 insertions(+), 2 deletions(-)
 create mode 100644 stubs/vhost.c

diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
index 4d68a27..11ec669 100644
--- a/hw/virtio/vhost-backend.c
+++ b/hw/virtio/vhost-backend.c
@@ -11,6 +11,7 @@
 #include "hw/virtio/vhost.h"
 #include "hw/virtio/vhost-backend.h"
 #include "qemu/error-report.h"
+#include "linux/vhost.h"
 
 #include <sys/ioctl.h>
 
@@ -42,11 +43,29 @@ static int vhost_kernel_cleanup(struct vhost_dev *dev)
     return close(fd);
 }
 
+static int vhost_kernel_memslots_limit(struct vhost_dev *dev)
+{
+    int limit = 64;
+    char *s;
+
+    if (g_file_get_contents("/sys/module/vhost/parameters/max_mem_regions",
+                            &s, NULL, NULL)) {
+        uint64_t val = g_ascii_strtoull(s, NULL, 10);
+        if (!((val == G_MAXUINT64 || !val) && errno)) {
+            return val;
+        }
+        error_report("ignoring invalid max_mem_regions value in vhost module:"
+                     " %s", s);
+    }
+    return limit;
+}
+
 static const VhostOps kernel_ops = {
         .backend_type = VHOST_BACKEND_TYPE_KERNEL,
         .vhost_call = vhost_kernel_call,
         .vhost_backend_init = vhost_kernel_init,
-        .vhost_backend_cleanup = vhost_kernel_cleanup
+        .vhost_backend_cleanup = vhost_kernel_cleanup,
+        .vhost_backend_memslots_limit = vhost_kernel_memslots_limit
 };
 
 int vhost_set_backend_type(struct vhost_dev *dev, VhostBackendType backend_type)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index e7ab829..acdfd04 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -343,9 +343,15 @@ static int vhost_user_cleanup(struct vhost_dev *dev)
     return 0;
 }
 
+static int vhost_user_memslots_limit(struct vhost_dev *dev)
+{
+    return VHOST_MEMORY_MAX_NREGIONS;
+}
+
 const VhostOps user_ops = {
         .backend_type = VHOST_BACKEND_TYPE_USER,
         .vhost_call = vhost_user_call,
         .vhost_backend_init = vhost_user_init,
-        .vhost_backend_cleanup = vhost_user_cleanup
+        .vhost_backend_cleanup = vhost_user_cleanup,
+        .vhost_backend_memslots_limit = vhost_user_memslots_limit
         };
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2712c6f..e964004 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -26,6 +26,18 @@
 
 static struct vhost_log *vhost_log;
 
+static int used_memslots;
+static int memslots_limit = -1;
+
+bool vhost_has_free_slot(void)
+{
+    if (memslots_limit >= 0) {
+        return memslots_limit > used_memslots;
+    }
+
+    return true;
+}
+
 static void vhost_dev_sync_region(struct vhost_dev *dev,
                                   MemoryRegionSection *section,
                                   uint64_t mfirst, uint64_t mlast,
@@ -457,6 +469,7 @@ static void vhost_set_memory(MemoryListener *listener,
     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);
     dev->memory_changed = true;
+    used_memslots = dev->mem->nregions;
 }
 
 static bool vhost_section(MemoryRegionSection *section)
@@ -1119,6 +1132,14 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
     if (r < 0) {
         goto fail_features;
     }
+
+    r = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
+    if (memslots_limit > 0) {
+        memslots_limit = MIN(memslots_limit, r);
+    } else {
+        memslots_limit = r;
+    }
+
     r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_MEM_TABLE, hdev->mem);
     if (r < 0) {
         r = -errno;
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index e472f29..28b6714 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -24,12 +24,14 @@ typedef int (*vhost_call)(struct vhost_dev *dev, unsigned long int request,
              void *arg);
 typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
 typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
+typedef int (*vhost_backend_memslots_limit)(struct vhost_dev *dev);
 
 typedef struct VhostOps {
     VhostBackendType backend_type;
     vhost_call vhost_call;
     vhost_backend_init vhost_backend_init;
     vhost_backend_cleanup vhost_backend_cleanup;
+    vhost_backend_memslots_limit vhost_backend_memslots_limit;
 } VhostOps;
 
 extern const VhostOps user_ops;
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index dd51050..17ff7b6 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -81,4 +81,5 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
                             uint64_t features);
 void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
                         uint64_t features);
+bool vhost_has_free_slot(void);
 #endif
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 9937a12..d2f1b21 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -38,3 +38,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o
 stub-obj-y += cpus.o
 stub-obj-y += kvm.o
 stub-obj-y += qmp_pc_dimm_device_list.o
+stub-obj-y += vhost.o
diff --git a/stubs/vhost.c b/stubs/vhost.c
new file mode 100644
index 0000000..d346b85
--- /dev/null
+++ b/stubs/vhost.c
@@ -0,0 +1,6 @@
+#include "hw/virtio/vhost.h"
+
+bool vhost_has_free_slot(void)
+{
+    return true;
+}
-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.4 v2 2/2] pc-dimm: add vhost slots limit check before commiting to hotplug

[Igor Mammedov]

it allows safely cancel memory hotplug if vhost backend
doesn't support necessary amount of memory slots and prevents
QEMU crashing in vhost due to hitting vhost limit on amount
of supported memory ranges.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/mem/pc-dimm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index bb04862..901bdbf 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -25,6 +25,7 @@
 #include "sysemu/numa.h"
 #include "sysemu/kvm.h"
 #include "trace.h"
+#include "hw/virtio/vhost.h"
 
 typedef struct pc_dimms_capacity {
      uint64_t size;
@@ -95,6 +96,12 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
         goto out;
     }
 
+    if (!vhost_has_free_slot()) {
+        error_setg(&local_err, "a used vhost backend has no free"
+                               " memory slots left");
+        goto out;
+    }
+
     memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
     vmstate_register_ram(mr, dev);
     numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.4 v2 0/2] vhost: check if vhost has capacity for hotplugged memory

[Michael S. Tsirkin]

On Wed, Jul 29, 2015 at 01:49:47PM +0200, Igor Mammedov wrote:
> v1->v2:
>   * replace probbing with checking for
>     /sys/module/vhost/parameters/max_mem_regions and
>     if it's missing has non wrong value return
>     hardcoded legacy limit (64 slots).
> 
> it's defensive patchset which helps to avoid QEMU crashing
> at memory hotplug time by checking that vhost has free capacity
> for an additional memory slot.

What if vhost is added after memory hotplug? Don't you need
to check that as well?


> 
> Igor Mammedov (2):
>   vhost: add vhost_has_free_slot() interface
>   pc-dimm: add vhost slots limit check before commiting to hotplug
> 
>  hw/mem/pc-dimm.c                  |  7 +++++++
>  hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
>  hw/virtio/vhost-user.c            |  8 +++++++-
>  hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
>  include/hw/virtio/vhost-backend.h |  2 ++
>  include/hw/virtio/vhost.h         |  1 +
>  stubs/Makefile.objs               |  1 +
>  stubs/vhost.c                     |  6 ++++++
>  8 files changed, 65 insertions(+), 2 deletions(-)
>  create mode 100644 stubs/vhost.c
> 
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH for-2.4 v2 1/2] vhost: add vhost_has_free_slot() interface

[Michael S. Tsirkin]

On Wed, Jul 29, 2015 at 01:49:48PM +0200, Igor Mammedov wrote:
> it will allow for other parts of QEMU check if it's safe
> to map memory region during hotplug/runtime.
> That way hotplug path will have a chance to cancel
> hotplug operation instead of crashing in vhost_commit().
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>   * replace probbing with checking for
>     /sys/module/vhost/parameters/max_mem_regions and
>     if it's missing has non wrong value return
>     hardcoded legacy limit (64 slots).
> ---
>  hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
>  hw/virtio/vhost-user.c            |  8 +++++++-
>  hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
>  include/hw/virtio/vhost-backend.h |  2 ++
>  include/hw/virtio/vhost.h         |  1 +
>  stubs/Makefile.objs               |  1 +
>  stubs/vhost.c                     |  6 ++++++
>  7 files changed, 58 insertions(+), 2 deletions(-)
>  create mode 100644 stubs/vhost.c
> 
> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> index 4d68a27..11ec669 100644
> --- a/hw/virtio/vhost-backend.c
> +++ b/hw/virtio/vhost-backend.c
> @@ -11,6 +11,7 @@
>  #include "hw/virtio/vhost.h"
>  #include "hw/virtio/vhost-backend.h"
>  #include "qemu/error-report.h"
> +#include "linux/vhost.h"
>  
>  #include <sys/ioctl.h>
>  
> @@ -42,11 +43,29 @@ static int vhost_kernel_cleanup(struct vhost_dev *dev)
>      return close(fd);
>  }
>  
> +static int vhost_kernel_memslots_limit(struct vhost_dev *dev)
> +{
> +    int limit = 64;
> +    char *s;
> +
> +    if (g_file_get_contents("/sys/module/vhost/parameters/max_mem_regions",
> +                            &s, NULL, NULL)) {
> +        uint64_t val = g_ascii_strtoull(s, NULL, 10);
> +        if (!((val == G_MAXUINT64 || !val) && errno)) {
> +            return val;
> +        }
> +        error_report("ignoring invalid max_mem_regions value in vhost module:"
> +                     " %s", s);
> +    }
> +    return limit;
> +}
> +
>  static const VhostOps kernel_ops = {
>          .backend_type = VHOST_BACKEND_TYPE_KERNEL,
>          .vhost_call = vhost_kernel_call,
>          .vhost_backend_init = vhost_kernel_init,
> -        .vhost_backend_cleanup = vhost_kernel_cleanup
> +        .vhost_backend_cleanup = vhost_kernel_cleanup,
> +        .vhost_backend_memslots_limit = vhost_kernel_memslots_limit
>  };
>  
>  int vhost_set_backend_type(struct vhost_dev *dev, VhostBackendType backend_type)
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index e7ab829..acdfd04 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -343,9 +343,15 @@ static int vhost_user_cleanup(struct vhost_dev *dev)
>      return 0;
>  }
>  
> +static int vhost_user_memslots_limit(struct vhost_dev *dev)
> +{
> +    return VHOST_MEMORY_MAX_NREGIONS;
> +}
> +
>  const VhostOps user_ops = {
>          .backend_type = VHOST_BACKEND_TYPE_USER,
>          .vhost_call = vhost_user_call,
>          .vhost_backend_init = vhost_user_init,
> -        .vhost_backend_cleanup = vhost_user_cleanup
> +        .vhost_backend_cleanup = vhost_user_cleanup,
> +        .vhost_backend_memslots_limit = vhost_user_memslots_limit
>          };
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 2712c6f..e964004 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -26,6 +26,18 @@
>  
>  static struct vhost_log *vhost_log;
>  
> +static int used_memslots;
> +static int memslots_limit = -1;

I suspect this need to be in some structure, refcounted by vhost devices
using it. vhost_kernel_memslots_limit would then be called when 1st
vhost device is added.

> +
> +bool vhost_has_free_slot(void)
> +{
> +    if (memslots_limit >= 0) {
> +        return memslots_limit > used_memslots;
> +    }
> +
> +    return true;

Pls rewrite this using logic operators.
Something like
	return memslots_limit < 0 || memslots_limit > used_memslots; ?


> +}
> +
>  static void vhost_dev_sync_region(struct vhost_dev *dev,
>                                    MemoryRegionSection *section,
>                                    uint64_t mfirst, uint64_t mlast,
> @@ -457,6 +469,7 @@ static void vhost_set_memory(MemoryListener *listener,
>      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);
>      dev->memory_changed = true;
> +    used_memslots = dev->mem->nregions;
>  }
>  
>  static bool vhost_section(MemoryRegionSection *section)
> @@ -1119,6 +1132,14 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>      if (r < 0) {
>          goto fail_features;
>      }
> +
> +    r = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
> +    if (memslots_limit > 0) {
> +        memslots_limit = MIN(memslots_limit, r);
> +    } else {
> +        memslots_limit = r;
> +    }
> +
>      r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_MEM_TABLE, hdev->mem);
>      if (r < 0) {
>          r = -errno;

Doing stuff on start but not stop is almost always a mistake.
In this case, when all vhost devices go away, one can
add more memory again, right?

> diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> index e472f29..28b6714 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -24,12 +24,14 @@ typedef int (*vhost_call)(struct vhost_dev *dev, unsigned long int request,
>               void *arg);
>  typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
>  typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
> +typedef int (*vhost_backend_memslots_limit)(struct vhost_dev *dev);
>  
>  typedef struct VhostOps {
>      VhostBackendType backend_type;
>      vhost_call vhost_call;
>      vhost_backend_init vhost_backend_init;
>      vhost_backend_cleanup vhost_backend_cleanup;
> +    vhost_backend_memslots_limit vhost_backend_memslots_limit;
>  } VhostOps;
>  
>  extern const VhostOps user_ops;
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index dd51050..17ff7b6 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -81,4 +81,5 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
>                              uint64_t features);
>  void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
>                          uint64_t features);
> +bool vhost_has_free_slot(void);
>  #endif
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 9937a12..d2f1b21 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -38,3 +38,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o
>  stub-obj-y += cpus.o
>  stub-obj-y += kvm.o
>  stub-obj-y += qmp_pc_dimm_device_list.o
> +stub-obj-y += vhost.o
> diff --git a/stubs/vhost.c b/stubs/vhost.c
> new file mode 100644
> index 0000000..d346b85
> --- /dev/null
> +++ b/stubs/vhost.c
> @@ -0,0 +1,6 @@
> +#include "hw/virtio/vhost.h"
> +
> +bool vhost_has_free_slot(void)
> +{
> +    return true;
> +}
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH for-2.4 v2 1/2] vhost: add vhost_has_free_slot() interface

[Igor Mammedov]

On Wed, 29 Jul 2015 18:11:14 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 29, 2015 at 01:49:48PM +0200, Igor Mammedov wrote:
> > it will allow for other parts of QEMU check if it's safe
> > to map memory region during hotplug/runtime.
> > That way hotplug path will have a chance to cancel
> > hotplug operation instead of crashing in vhost_commit().
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v2:
> >   * replace probbing with checking for
> >     /sys/module/vhost/parameters/max_mem_regions and
> >     if it's missing has non wrong value return
> >     hardcoded legacy limit (64 slots).
> > ---
> >  hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
> >  hw/virtio/vhost-user.c            |  8 +++++++-
> >  hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
> >  include/hw/virtio/vhost-backend.h |  2 ++
> >  include/hw/virtio/vhost.h         |  1 +
> >  stubs/Makefile.objs               |  1 +
> >  stubs/vhost.c                     |  6 ++++++
> >  7 files changed, 58 insertions(+), 2 deletions(-)
> >  create mode 100644 stubs/vhost.c
> > 
> > diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> > index 4d68a27..11ec669 100644
> > --- a/hw/virtio/vhost-backend.c
> > +++ b/hw/virtio/vhost-backend.c
> > @@ -11,6 +11,7 @@
> >  #include "hw/virtio/vhost.h"
> >  #include "hw/virtio/vhost-backend.h"
> >  #include "qemu/error-report.h"
> > +#include "linux/vhost.h"
> >  
> >  #include <sys/ioctl.h>
> >  
> > @@ -42,11 +43,29 @@ static int vhost_kernel_cleanup(struct vhost_dev *dev)
> >      return close(fd);
> >  }
> >  
> > +static int vhost_kernel_memslots_limit(struct vhost_dev *dev)
> > +{
> > +    int limit = 64;
> > +    char *s;
> > +
> > +    if (g_file_get_contents("/sys/module/vhost/parameters/max_mem_regions",
> > +                            &s, NULL, NULL)) {
> > +        uint64_t val = g_ascii_strtoull(s, NULL, 10);
> > +        if (!((val == G_MAXUINT64 || !val) && errno)) {
> > +            return val;
> > +        }
> > +        error_report("ignoring invalid max_mem_regions value in vhost module:"
> > +                     " %s", s);
> > +    }
> > +    return limit;
> > +}
> > +
> >  static const VhostOps kernel_ops = {
> >          .backend_type = VHOST_BACKEND_TYPE_KERNEL,
> >          .vhost_call = vhost_kernel_call,
> >          .vhost_backend_init = vhost_kernel_init,
> > -        .vhost_backend_cleanup = vhost_kernel_cleanup
> > +        .vhost_backend_cleanup = vhost_kernel_cleanup,
> > +        .vhost_backend_memslots_limit = vhost_kernel_memslots_limit
> >  };
> >  
> >  int vhost_set_backend_type(struct vhost_dev *dev, VhostBackendType backend_type)
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index e7ab829..acdfd04 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -343,9 +343,15 @@ static int vhost_user_cleanup(struct vhost_dev *dev)
> >      return 0;
> >  }
> >  
> > +static int vhost_user_memslots_limit(struct vhost_dev *dev)
> > +{
> > +    return VHOST_MEMORY_MAX_NREGIONS;
> > +}
> > +
> >  const VhostOps user_ops = {
> >          .backend_type = VHOST_BACKEND_TYPE_USER,
> >          .vhost_call = vhost_user_call,
> >          .vhost_backend_init = vhost_user_init,
> > -        .vhost_backend_cleanup = vhost_user_cleanup
> > +        .vhost_backend_cleanup = vhost_user_cleanup,
> > +        .vhost_backend_memslots_limit = vhost_user_memslots_limit
> >          };
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 2712c6f..e964004 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -26,6 +26,18 @@
> >  
> >  static struct vhost_log *vhost_log;
> >  
> > +static int used_memslots;
> > +static int memslots_limit = -1;
> 
> I suspect this need to be in some structure, refcounted by vhost devices
> using it. vhost_kernel_memslots_limit would then be called when 1st
> vhost device is added.

vhost_kernel_memslots_limit() should be called for every added vhost device,
since mixed vhost backends may have different limit.

refcounting might help with disabling limit check when all vhost devices
are hot-unplugged and user will be able to plug in more memory and won't be
able to add vhost device with lower limit.
but I see such usecase highly not probable, hence it's considered
adding refcounting as over-engineered way.

> 
> > +
> > +bool vhost_has_free_slot(void)
> > +{
> > +    if (memslots_limit >= 0) {
> > +        return memslots_limit > used_memslots;
> > +    }
> > +
> > +    return true;
> 
> Pls rewrite this using logic operators.
> Something like
> 	return memslots_limit < 0 || memslots_limit > used_memslots; ?
sure

> 
> > +}
> > +
> >  static void vhost_dev_sync_region(struct vhost_dev *dev,
> >                                    MemoryRegionSection *section,
> >                                    uint64_t mfirst, uint64_t mlast,
> > @@ -457,6 +469,7 @@ static void vhost_set_memory(MemoryListener *listener,
> >      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);
> >      dev->memory_changed = true;
> > +    used_memslots = dev->mem->nregions;
> >  }
> >  
> >  static bool vhost_section(MemoryRegionSection *section)
> > @@ -1119,6 +1132,14 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> >      if (r < 0) {
> >          goto fail_features;
> >      }
> > +
> > +    r = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
> > +    if (memslots_limit > 0) {
> > +        memslots_limit = MIN(memslots_limit, r);
> > +    } else {
> > +        memslots_limit = r;
> > +    }
> > +
> >      r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_MEM_TABLE, hdev->mem);
> >      if (r < 0) {
> >          r = -errno;
> 
> Doing stuff on start but not stop is almost always a mistake.
> In this case, when all vhost devices go away, one can
> add more memory again, right?
yep, it's possible provided vhost_call(VHOST_SET_MEM_TABLE) succeeds,
if it's not then device addition fails gracefully.
 
> 
> > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> > index e472f29..28b6714 100644
> > --- a/include/hw/virtio/vhost-backend.h
> > +++ b/include/hw/virtio/vhost-backend.h
> > @@ -24,12 +24,14 @@ typedef int (*vhost_call)(struct vhost_dev *dev, unsigned long int request,
> >               void *arg);
> >  typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
> >  typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
> > +typedef int (*vhost_backend_memslots_limit)(struct vhost_dev *dev);
> >  
> >  typedef struct VhostOps {
> >      VhostBackendType backend_type;
> >      vhost_call vhost_call;
> >      vhost_backend_init vhost_backend_init;
> >      vhost_backend_cleanup vhost_backend_cleanup;
> > +    vhost_backend_memslots_limit vhost_backend_memslots_limit;
> >  } VhostOps;
> >  
> >  extern const VhostOps user_ops;
> > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > index dd51050..17ff7b6 100644
> > --- a/include/hw/virtio/vhost.h
> > +++ b/include/hw/virtio/vhost.h
> > @@ -81,4 +81,5 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> >                              uint64_t features);
> >  void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
> >                          uint64_t features);
> > +bool vhost_has_free_slot(void);
> >  #endif
> > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> > index 9937a12..d2f1b21 100644
> > --- a/stubs/Makefile.objs
> > +++ b/stubs/Makefile.objs
> > @@ -38,3 +38,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o
> >  stub-obj-y += cpus.o
> >  stub-obj-y += kvm.o
> >  stub-obj-y += qmp_pc_dimm_device_list.o
> > +stub-obj-y += vhost.o
> > diff --git a/stubs/vhost.c b/stubs/vhost.c
> > new file mode 100644
> > index 0000000..d346b85
> > --- /dev/null
> > +++ b/stubs/vhost.c
> > @@ -0,0 +1,6 @@
> > +#include "hw/virtio/vhost.h"
> > +
> > +bool vhost_has_free_slot(void)
> > +{
> > +    return true;
> > +}
> > -- 
> > 1.8.3.1

Re: [Qemu-devel] [PATCH for-2.4 v2 0/2] vhost: check if vhost has capacity for hotplugged memory

[Igor Mammedov]

On Wed, 29 Jul 2015 18:03:36 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 29, 2015 at 01:49:47PM +0200, Igor Mammedov wrote:
> > v1->v2:
> >   * replace probbing with checking for
> >     /sys/module/vhost/parameters/max_mem_regions and
> >     if it's missing has non wrong value return
> >     hardcoded legacy limit (64 slots).
> > 
> > it's defensive patchset which helps to avoid QEMU crashing
> > at memory hotplug time by checking that vhost has free capacity
> > for an additional memory slot.
> 
> What if vhost is added after memory hotplug? Don't you need
> to check that as well?
vhost device can be hotplugged after memory hotplug as far as
current slots count doesn't exceed its limit,
if limit is exceeded device_add would fail or virtio device
would fallback to non vhost mode at its start-up (depends on
how particular device treats vhost_start failure).

> 
> 
> > 
> > Igor Mammedov (2):
> >   vhost: add vhost_has_free_slot() interface
> >   pc-dimm: add vhost slots limit check before commiting to hotplug
> > 
> >  hw/mem/pc-dimm.c                  |  7 +++++++
> >  hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
> >  hw/virtio/vhost-user.c            |  8 +++++++-
> >  hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
> >  include/hw/virtio/vhost-backend.h |  2 ++
> >  include/hw/virtio/vhost.h         |  1 +
> >  stubs/Makefile.objs               |  1 +
> >  stubs/vhost.c                     |  6 ++++++
> >  8 files changed, 65 insertions(+), 2 deletions(-)
> >  create mode 100644 stubs/vhost.c
> > 
> > -- 
> > 1.8.3.1

Re: [Qemu-devel] [PATCH for-2.4 v2 1/2] vhost: add vhost_has_free_slot() interface

[Michael S. Tsirkin]

On Thu, Jul 30, 2015 at 08:16:54AM +0200, Igor Mammedov wrote:
> On Wed, 29 Jul 2015 18:11:14 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 29, 2015 at 01:49:48PM +0200, Igor Mammedov wrote:
> > > it will allow for other parts of QEMU check if it's safe
> > > to map memory region during hotplug/runtime.
> > > That way hotplug path will have a chance to cancel
> > > hotplug operation instead of crashing in vhost_commit().
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > v2:
> > >   * replace probbing with checking for
> > >     /sys/module/vhost/parameters/max_mem_regions and
> > >     if it's missing has non wrong value return
> > >     hardcoded legacy limit (64 slots).
> > > ---
> > >  hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
> > >  hw/virtio/vhost-user.c            |  8 +++++++-
> > >  hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
> > >  include/hw/virtio/vhost-backend.h |  2 ++
> > >  include/hw/virtio/vhost.h         |  1 +
> > >  stubs/Makefile.objs               |  1 +
> > >  stubs/vhost.c                     |  6 ++++++
> > >  7 files changed, 58 insertions(+), 2 deletions(-)
> > >  create mode 100644 stubs/vhost.c
> > > 
> > > diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> > > index 4d68a27..11ec669 100644
> > > --- a/hw/virtio/vhost-backend.c
> > > +++ b/hw/virtio/vhost-backend.c
> > > @@ -11,6 +11,7 @@
> > >  #include "hw/virtio/vhost.h"
> > >  #include "hw/virtio/vhost-backend.h"
> > >  #include "qemu/error-report.h"
> > > +#include "linux/vhost.h"
> > >  
> > >  #include <sys/ioctl.h>
> > >  
> > > @@ -42,11 +43,29 @@ static int vhost_kernel_cleanup(struct vhost_dev *dev)
> > >      return close(fd);
> > >  }
> > >  
> > > +static int vhost_kernel_memslots_limit(struct vhost_dev *dev)
> > > +{
> > > +    int limit = 64;
> > > +    char *s;
> > > +
> > > +    if (g_file_get_contents("/sys/module/vhost/parameters/max_mem_regions",
> > > +                            &s, NULL, NULL)) {
> > > +        uint64_t val = g_ascii_strtoull(s, NULL, 10);
> > > +        if (!((val == G_MAXUINT64 || !val) && errno)) {
> > > +            return val;
> > > +        }
> > > +        error_report("ignoring invalid max_mem_regions value in vhost module:"
> > > +                     " %s", s);
> > > +    }
> > > +    return limit;
> > > +}
> > > +
> > >  static const VhostOps kernel_ops = {
> > >          .backend_type = VHOST_BACKEND_TYPE_KERNEL,
> > >          .vhost_call = vhost_kernel_call,
> > >          .vhost_backend_init = vhost_kernel_init,
> > > -        .vhost_backend_cleanup = vhost_kernel_cleanup
> > > +        .vhost_backend_cleanup = vhost_kernel_cleanup,
> > > +        .vhost_backend_memslots_limit = vhost_kernel_memslots_limit
> > >  };
> > >  
> > >  int vhost_set_backend_type(struct vhost_dev *dev, VhostBackendType backend_type)
> > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > index e7ab829..acdfd04 100644
> > > --- a/hw/virtio/vhost-user.c
> > > +++ b/hw/virtio/vhost-user.c
> > > @@ -343,9 +343,15 @@ static int vhost_user_cleanup(struct vhost_dev *dev)
> > >      return 0;
> > >  }
> > >  
> > > +static int vhost_user_memslots_limit(struct vhost_dev *dev)
> > > +{
> > > +    return VHOST_MEMORY_MAX_NREGIONS;
> > > +}
> > > +
> > >  const VhostOps user_ops = {
> > >          .backend_type = VHOST_BACKEND_TYPE_USER,
> > >          .vhost_call = vhost_user_call,
> > >          .vhost_backend_init = vhost_user_init,
> > > -        .vhost_backend_cleanup = vhost_user_cleanup
> > > +        .vhost_backend_cleanup = vhost_user_cleanup,
> > > +        .vhost_backend_memslots_limit = vhost_user_memslots_limit
> > >          };
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index 2712c6f..e964004 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -26,6 +26,18 @@
> > >  
> > >  static struct vhost_log *vhost_log;
> > >  
> > > +static int used_memslots;
> > > +static int memslots_limit = -1;
> > 
> > I suspect this need to be in some structure, refcounted by vhost devices
> > using it. vhost_kernel_memslots_limit would then be called when 1st
> > vhost device is added.
> 
> vhost_kernel_memslots_limit() should be called for every added vhost device,
> since mixed vhost backends may have different limit.
> 
> refcounting might help with disabling limit check when all vhost devices
> are hot-unplugged and user will be able to plug in more memory and won't be
> able to add vhost device with lower limit.
> but I see such usecase highly not probable, hence it's considered
> adding refcounting as over-engineered way.

I don't see what's not probable here. It's trivial to reproduce.
One reason to do this is if you want to tweak the limit.
vhost needs to be unloaded for this.


> > 
> > > +
> > > +bool vhost_has_free_slot(void)
> > > +{
> > > +    if (memslots_limit >= 0) {
> > > +        return memslots_limit > used_memslots;
> > > +    }
> > > +
> > > +    return true;
> > 
> > Pls rewrite this using logic operators.
> > Something like
> > 	return memslots_limit < 0 || memslots_limit > used_memslots; ?
> sure
> 
> > 
> > > +}
> > > +
> > >  static void vhost_dev_sync_region(struct vhost_dev *dev,
> > >                                    MemoryRegionSection *section,
> > >                                    uint64_t mfirst, uint64_t mlast,
> > > @@ -457,6 +469,7 @@ static void vhost_set_memory(MemoryListener *listener,
> > >      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);
> > >      dev->memory_changed = true;
> > > +    used_memslots = dev->mem->nregions;
> > >  }
> > >  
> > >  static bool vhost_section(MemoryRegionSection *section)
> > > @@ -1119,6 +1132,14 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> > >      if (r < 0) {
> > >          goto fail_features;
> > >      }
> > > +
> > > +    r = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
> > > +    if (memslots_limit > 0) {
> > > +        memslots_limit = MIN(memslots_limit, r);
> > > +    } else {
> > > +        memslots_limit = r;
> > > +    }
> > > +
> > >      r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_MEM_TABLE, hdev->mem);
> > >      if (r < 0) {
> > >          r = -errno;
> > 
> > Doing stuff on start but not stop is almost always a mistake.
> > In this case, when all vhost devices go away, one can
> > add more memory again, right?
> yep, it's possible provided vhost_call(VHOST_SET_MEM_TABLE) succeeds,
> if it's not then device addition fails gracefully.

VHOST_SET_MEM_TABLE on which device? All fds are gone.

> > 
> > > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> > > index e472f29..28b6714 100644
> > > --- a/include/hw/virtio/vhost-backend.h
> > > +++ b/include/hw/virtio/vhost-backend.h
> > > @@ -24,12 +24,14 @@ typedef int (*vhost_call)(struct vhost_dev *dev, unsigned long int request,
> > >               void *arg);
> > >  typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
> > >  typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
> > > +typedef int (*vhost_backend_memslots_limit)(struct vhost_dev *dev);
> > >  
> > >  typedef struct VhostOps {
> > >      VhostBackendType backend_type;
> > >      vhost_call vhost_call;
> > >      vhost_backend_init vhost_backend_init;
> > >      vhost_backend_cleanup vhost_backend_cleanup;
> > > +    vhost_backend_memslots_limit vhost_backend_memslots_limit;
> > >  } VhostOps;
> > >  
> > >  extern const VhostOps user_ops;
> > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > > index dd51050..17ff7b6 100644
> > > --- a/include/hw/virtio/vhost.h
> > > +++ b/include/hw/virtio/vhost.h
> > > @@ -81,4 +81,5 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> > >                              uint64_t features);
> > >  void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
> > >                          uint64_t features);
> > > +bool vhost_has_free_slot(void);
> > >  #endif
> > > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> > > index 9937a12..d2f1b21 100644
> > > --- a/stubs/Makefile.objs
> > > +++ b/stubs/Makefile.objs
> > > @@ -38,3 +38,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o
> > >  stub-obj-y += cpus.o
> > >  stub-obj-y += kvm.o
> > >  stub-obj-y += qmp_pc_dimm_device_list.o
> > > +stub-obj-y += vhost.o
> > > diff --git a/stubs/vhost.c b/stubs/vhost.c
> > > new file mode 100644
> > > index 0000000..d346b85
> > > --- /dev/null
> > > +++ b/stubs/vhost.c
> > > @@ -0,0 +1,6 @@
> > > +#include "hw/virtio/vhost.h"
> > > +
> > > +bool vhost_has_free_slot(void)
> > > +{
> > > +    return true;
> > > +}
> > > -- 
> > > 1.8.3.1

Re: [Qemu-devel] [PATCH for-2.4 v2 0/2] vhost: check if vhost has capacity for hotplugged memory

[Michael S. Tsirkin]

On Thu, Jul 30, 2015 at 08:22:18AM +0200, Igor Mammedov wrote:
> On Wed, 29 Jul 2015 18:03:36 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 29, 2015 at 01:49:47PM +0200, Igor Mammedov wrote:
> > > v1->v2:
> > >   * replace probbing with checking for
> > >     /sys/module/vhost/parameters/max_mem_regions and
> > >     if it's missing has non wrong value return
> > >     hardcoded legacy limit (64 slots).
> > > 
> > > it's defensive patchset which helps to avoid QEMU crashing
> > > at memory hotplug time by checking that vhost has free capacity
> > > for an additional memory slot.
> > 
> > What if vhost is added after memory hotplug? Don't you need
> > to check that as well?
> vhost device can be hotplugged after memory hotplug as far as
> current slots count doesn't exceed its limit,
> if limit is exceeded device_add would fail or virtio device
> would fallback to non vhost mode at its start-up (depends on
> how particular device treats vhost_start failure).

Where exactly does it fail?
memory_listener_register returns void so clearly it's not that ...

> > 
> > 
> > > 
> > > Igor Mammedov (2):
> > >   vhost: add vhost_has_free_slot() interface
> > >   pc-dimm: add vhost slots limit check before commiting to hotplug
> > > 
> > >  hw/mem/pc-dimm.c                  |  7 +++++++
> > >  hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
> > >  hw/virtio/vhost-user.c            |  8 +++++++-
> > >  hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
> > >  include/hw/virtio/vhost-backend.h |  2 ++
> > >  include/hw/virtio/vhost.h         |  1 +
> > >  stubs/Makefile.objs               |  1 +
> > >  stubs/vhost.c                     |  6 ++++++
> > >  8 files changed, 65 insertions(+), 2 deletions(-)
> > >  create mode 100644 stubs/vhost.c
> > > 
> > > -- 
> > > 1.8.3.1

Re: [Qemu-devel] [PATCH for-2.4 v2 0/2] vhost: check if vhost has capacity for hotplugged memory

[Michael S. Tsirkin]

On Thu, Jul 30, 2015 at 09:25:55AM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 30, 2015 at 08:22:18AM +0200, Igor Mammedov wrote:
> > On Wed, 29 Jul 2015 18:03:36 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Jul 29, 2015 at 01:49:47PM +0200, Igor Mammedov wrote:
> > > > v1->v2:
> > > >   * replace probbing with checking for
> > > >     /sys/module/vhost/parameters/max_mem_regions and
> > > >     if it's missing has non wrong value return
> > > >     hardcoded legacy limit (64 slots).
> > > > 
> > > > it's defensive patchset which helps to avoid QEMU crashing
> > > > at memory hotplug time by checking that vhost has free capacity
> > > > for an additional memory slot.
> > > 
> > > What if vhost is added after memory hotplug? Don't you need
> > > to check that as well?
> > vhost device can be hotplugged after memory hotplug as far as
> > current slots count doesn't exceed its limit,
> > if limit is exceeded device_add would fail or virtio device
> > would fallback to non vhost mode at its start-up (depends on
> > how particular device treats vhost_start failure).
> 
> Where exactly does it fail?
> memory_listener_register returns void so clearly it's not that ...

Oh, dev_start fails. But that's not called at device_add time.
And vhost-user can't fall back to anything.


> > > 
> > > 
> > > > 
> > > > Igor Mammedov (2):
> > > >   vhost: add vhost_has_free_slot() interface
> > > >   pc-dimm: add vhost slots limit check before commiting to hotplug
> > > > 
> > > >  hw/mem/pc-dimm.c                  |  7 +++++++
> > > >  hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
> > > >  hw/virtio/vhost-user.c            |  8 +++++++-
> > > >  hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
> > > >  include/hw/virtio/vhost-backend.h |  2 ++
> > > >  include/hw/virtio/vhost.h         |  1 +
> > > >  stubs/Makefile.objs               |  1 +
> > > >  stubs/vhost.c                     |  6 ++++++
> > > >  8 files changed, 65 insertions(+), 2 deletions(-)
> > > >  create mode 100644 stubs/vhost.c
> > > > 
> > > > -- 
> > > > 1.8.3.1

Re: [Qemu-devel] [PATCH for-2.4 v2 1/2] vhost: add vhost_has_free_slot() interface

[Igor Mammedov]

On Thu, 30 Jul 2015 09:22:25 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jul 30, 2015 at 08:16:54AM +0200, Igor Mammedov wrote:
> > On Wed, 29 Jul 2015 18:11:14 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Jul 29, 2015 at 01:49:48PM +0200, Igor Mammedov wrote:
> > > > it will allow for other parts of QEMU check if it's safe
> > > > to map memory region during hotplug/runtime.
> > > > That way hotplug path will have a chance to cancel
> > > > hotplug operation instead of crashing in vhost_commit().
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > > v2:
> > > >   * replace probbing with checking for
> > > >     /sys/module/vhost/parameters/max_mem_regions and
> > > >     if it's missing has non wrong value return
> > > >     hardcoded legacy limit (64 slots).
> > > > ---
> > > >  hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
> > > >  hw/virtio/vhost-user.c            |  8 +++++++-
> > > >  hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
> > > >  include/hw/virtio/vhost-backend.h |  2 ++
> > > >  include/hw/virtio/vhost.h         |  1 +
> > > >  stubs/Makefile.objs               |  1 +
> > > >  stubs/vhost.c                     |  6 ++++++
> > > >  7 files changed, 58 insertions(+), 2 deletions(-)
> > > >  create mode 100644 stubs/vhost.c
> > > > 
> > > > diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> > > > index 4d68a27..11ec669 100644
> > > > --- a/hw/virtio/vhost-backend.c
> > > > +++ b/hw/virtio/vhost-backend.c
> > > > @@ -11,6 +11,7 @@
> > > >  #include "hw/virtio/vhost.h"
> > > >  #include "hw/virtio/vhost-backend.h"
> > > >  #include "qemu/error-report.h"
> > > > +#include "linux/vhost.h"
> > > >  
> > > >  #include <sys/ioctl.h>
> > > >  
> > > > @@ -42,11 +43,29 @@ static int vhost_kernel_cleanup(struct vhost_dev *dev)
> > > >      return close(fd);
> > > >  }
> > > >  
> > > > +static int vhost_kernel_memslots_limit(struct vhost_dev *dev)
> > > > +{
> > > > +    int limit = 64;
> > > > +    char *s;
> > > > +
> > > > +    if (g_file_get_contents("/sys/module/vhost/parameters/max_mem_regions",
> > > > +                            &s, NULL, NULL)) {
> > > > +        uint64_t val = g_ascii_strtoull(s, NULL, 10);
> > > > +        if (!((val == G_MAXUINT64 || !val) && errno)) {
> > > > +            return val;
> > > > +        }
> > > > +        error_report("ignoring invalid max_mem_regions value in vhost module:"
> > > > +                     " %s", s);
> > > > +    }
> > > > +    return limit;
> > > > +}
> > > > +
> > > >  static const VhostOps kernel_ops = {
> > > >          .backend_type = VHOST_BACKEND_TYPE_KERNEL,
> > > >          .vhost_call = vhost_kernel_call,
> > > >          .vhost_backend_init = vhost_kernel_init,
> > > > -        .vhost_backend_cleanup = vhost_kernel_cleanup
> > > > +        .vhost_backend_cleanup = vhost_kernel_cleanup,
> > > > +        .vhost_backend_memslots_limit = vhost_kernel_memslots_limit
> > > >  };
> > > >  
> > > >  int vhost_set_backend_type(struct vhost_dev *dev, VhostBackendType backend_type)
> > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > index e7ab829..acdfd04 100644
> > > > --- a/hw/virtio/vhost-user.c
> > > > +++ b/hw/virtio/vhost-user.c
> > > > @@ -343,9 +343,15 @@ static int vhost_user_cleanup(struct vhost_dev *dev)
> > > >      return 0;
> > > >  }
> > > >  
> > > > +static int vhost_user_memslots_limit(struct vhost_dev *dev)
> > > > +{
> > > > +    return VHOST_MEMORY_MAX_NREGIONS;
> > > > +}
> > > > +
> > > >  const VhostOps user_ops = {
> > > >          .backend_type = VHOST_BACKEND_TYPE_USER,
> > > >          .vhost_call = vhost_user_call,
> > > >          .vhost_backend_init = vhost_user_init,
> > > > -        .vhost_backend_cleanup = vhost_user_cleanup
> > > > +        .vhost_backend_cleanup = vhost_user_cleanup,
> > > > +        .vhost_backend_memslots_limit = vhost_user_memslots_limit
> > > >          };
> > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > > index 2712c6f..e964004 100644
> > > > --- a/hw/virtio/vhost.c
> > > > +++ b/hw/virtio/vhost.c
> > > > @@ -26,6 +26,18 @@
> > > >  
> > > >  static struct vhost_log *vhost_log;
> > > >  
> > > > +static int used_memslots;
> > > > +static int memslots_limit = -1;
> > > 
> > > I suspect this need to be in some structure, refcounted by vhost devices
> > > using it. vhost_kernel_memslots_limit would then be called when 1st
> > > vhost device is added.
> > 
> > vhost_kernel_memslots_limit() should be called for every added vhost device,
> > since mixed vhost backends may have different limit.
> > 
> > refcounting might help with disabling limit check when all vhost devices
> > are hot-unplugged and user will be able to plug in more memory and won't be
> > able to add vhost device with lower limit.
> > but I see such usecase highly not probable, hence it's considered
> > adding refcounting as over-engineered way.
> 
> I don't see what's not probable here. It's trivial to reproduce.
> One reason to do this is if you want to tweak the limit.
> vhost needs to be unloaded for this.
would be something like this suitable:

static struct {
   int used_memslots;
   int memslots_limit;
   int refcount;
} slots_limit;

> 
> 
> > > 
> > > > +
> > > > +bool vhost_has_free_slot(void)
> > > > +{
> > > > +    if (memslots_limit >= 0) {
> > > > +        return memslots_limit > used_memslots;
> > > > +    }
> > > > +
> > > > +    return true;
> > > 
> > > Pls rewrite this using logic operators.
> > > Something like
> > > 	return memslots_limit < 0 || memslots_limit > used_memslots; ?
> > sure
> > 
> > > 
> > > > +}
> > > > +
> > > >  static void vhost_dev_sync_region(struct vhost_dev *dev,
> > > >                                    MemoryRegionSection *section,
> > > >                                    uint64_t mfirst, uint64_t mlast,
> > > > @@ -457,6 +469,7 @@ static void vhost_set_memory(MemoryListener *listener,
> > > >      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);
> > > >      dev->memory_changed = true;
> > > > +    used_memslots = dev->mem->nregions;
> > > >  }
> > > >  
> > > >  static bool vhost_section(MemoryRegionSection *section)
> > > > @@ -1119,6 +1132,14 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> > > >      if (r < 0) {
> > > >          goto fail_features;
> > > >      }
> > > > +
> > > > +    r = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
> > > > +    if (memslots_limit > 0) {
> > > > +        memslots_limit = MIN(memslots_limit, r);
> > > > +    } else {
> > > > +        memslots_limit = r;
> > > > +    }
> > > > +
> > > >      r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_MEM_TABLE, hdev->mem);
> > > >      if (r < 0) {
> > > >          r = -errno;
> > > 
> > > Doing stuff on start but not stop is almost always a mistake.
> > > In this case, when all vhost devices go away, one can
> > > add more memory again, right?
> > yep, it's possible provided vhost_call(VHOST_SET_MEM_TABLE) succeeds,
> > if it's not then device addition fails gracefully.
> 
> VHOST_SET_MEM_TABLE on which device? All fds are gone.
I've meant above in context of adding vhost devices after
memory hotplug.

As for original question, the lowest limit will stay even if
all vhost devices are gone with this impl.


> 
> > > 
> > > > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> > > > index e472f29..28b6714 100644
> > > > --- a/include/hw/virtio/vhost-backend.h
> > > > +++ b/include/hw/virtio/vhost-backend.h
> > > > @@ -24,12 +24,14 @@ typedef int (*vhost_call)(struct vhost_dev *dev, unsigned long int request,
> > > >               void *arg);
> > > >  typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
> > > >  typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
> > > > +typedef int (*vhost_backend_memslots_limit)(struct vhost_dev *dev);
> > > >  
> > > >  typedef struct VhostOps {
> > > >      VhostBackendType backend_type;
> > > >      vhost_call vhost_call;
> > > >      vhost_backend_init vhost_backend_init;
> > > >      vhost_backend_cleanup vhost_backend_cleanup;
> > > > +    vhost_backend_memslots_limit vhost_backend_memslots_limit;
> > > >  } VhostOps;
> > > >  
> > > >  extern const VhostOps user_ops;
> > > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > > > index dd51050..17ff7b6 100644
> > > > --- a/include/hw/virtio/vhost.h
> > > > +++ b/include/hw/virtio/vhost.h
> > > > @@ -81,4 +81,5 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> > > >                              uint64_t features);
> > > >  void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
> > > >                          uint64_t features);
> > > > +bool vhost_has_free_slot(void);
> > > >  #endif
> > > > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> > > > index 9937a12..d2f1b21 100644
> > > > --- a/stubs/Makefile.objs
> > > > +++ b/stubs/Makefile.objs
> > > > @@ -38,3 +38,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o
> > > >  stub-obj-y += cpus.o
> > > >  stub-obj-y += kvm.o
> > > >  stub-obj-y += qmp_pc_dimm_device_list.o
> > > > +stub-obj-y += vhost.o
> > > > diff --git a/stubs/vhost.c b/stubs/vhost.c
> > > > new file mode 100644
> > > > index 0000000..d346b85
> > > > --- /dev/null
> > > > +++ b/stubs/vhost.c
> > > > @@ -0,0 +1,6 @@
> > > > +#include "hw/virtio/vhost.h"
> > > > +
> > > > +bool vhost_has_free_slot(void)
> > > > +{
> > > > +    return true;
> > > > +}
> > > > -- 
> > > > 1.8.3.1

Re: [Qemu-devel] [PATCH for-2.4 v2 1/2] vhost: add vhost_has_free_slot() interface

[Michael S. Tsirkin]

On Thu, Jul 30, 2015 at 09:04:54AM +0200, Igor Mammedov wrote:
> On Thu, 30 Jul 2015 09:22:25 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Jul 30, 2015 at 08:16:54AM +0200, Igor Mammedov wrote:
> > > On Wed, 29 Jul 2015 18:11:14 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Wed, Jul 29, 2015 at 01:49:48PM +0200, Igor Mammedov wrote:
> > > > > it will allow for other parts of QEMU check if it's safe
> > > > > to map memory region during hotplug/runtime.
> > > > > That way hotplug path will have a chance to cancel
> > > > > hotplug operation instead of crashing in vhost_commit().
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > ---
> > > > > v2:
> > > > >   * replace probbing with checking for
> > > > >     /sys/module/vhost/parameters/max_mem_regions and
> > > > >     if it's missing has non wrong value return
> > > > >     hardcoded legacy limit (64 slots).
> > > > > ---
> > > > >  hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
> > > > >  hw/virtio/vhost-user.c            |  8 +++++++-
> > > > >  hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
> > > > >  include/hw/virtio/vhost-backend.h |  2 ++
> > > > >  include/hw/virtio/vhost.h         |  1 +
> > > > >  stubs/Makefile.objs               |  1 +
> > > > >  stubs/vhost.c                     |  6 ++++++
> > > > >  7 files changed, 58 insertions(+), 2 deletions(-)
> > > > >  create mode 100644 stubs/vhost.c
> > > > > 
> > > > > diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> > > > > index 4d68a27..11ec669 100644
> > > > > --- a/hw/virtio/vhost-backend.c
> > > > > +++ b/hw/virtio/vhost-backend.c
> > > > > @@ -11,6 +11,7 @@
> > > > >  #include "hw/virtio/vhost.h"
> > > > >  #include "hw/virtio/vhost-backend.h"
> > > > >  #include "qemu/error-report.h"
> > > > > +#include "linux/vhost.h"
> > > > >  
> > > > >  #include <sys/ioctl.h>
> > > > >  
> > > > > @@ -42,11 +43,29 @@ static int vhost_kernel_cleanup(struct vhost_dev *dev)
> > > > >      return close(fd);
> > > > >  }
> > > > >  
> > > > > +static int vhost_kernel_memslots_limit(struct vhost_dev *dev)
> > > > > +{
> > > > > +    int limit = 64;
> > > > > +    char *s;
> > > > > +
> > > > > +    if (g_file_get_contents("/sys/module/vhost/parameters/max_mem_regions",
> > > > > +                            &s, NULL, NULL)) {
> > > > > +        uint64_t val = g_ascii_strtoull(s, NULL, 10);
> > > > > +        if (!((val == G_MAXUINT64 || !val) && errno)) {
> > > > > +            return val;
> > > > > +        }
> > > > > +        error_report("ignoring invalid max_mem_regions value in vhost module:"
> > > > > +                     " %s", s);
> > > > > +    }
> > > > > +    return limit;
> > > > > +}
> > > > > +
> > > > >  static const VhostOps kernel_ops = {
> > > > >          .backend_type = VHOST_BACKEND_TYPE_KERNEL,
> > > > >          .vhost_call = vhost_kernel_call,
> > > > >          .vhost_backend_init = vhost_kernel_init,
> > > > > -        .vhost_backend_cleanup = vhost_kernel_cleanup
> > > > > +        .vhost_backend_cleanup = vhost_kernel_cleanup,
> > > > > +        .vhost_backend_memslots_limit = vhost_kernel_memslots_limit
> > > > >  };
> > > > >  
> > > > >  int vhost_set_backend_type(struct vhost_dev *dev, VhostBackendType backend_type)
> > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > > index e7ab829..acdfd04 100644
> > > > > --- a/hw/virtio/vhost-user.c
> > > > > +++ b/hw/virtio/vhost-user.c
> > > > > @@ -343,9 +343,15 @@ static int vhost_user_cleanup(struct vhost_dev *dev)
> > > > >      return 0;
> > > > >  }
> > > > >  
> > > > > +static int vhost_user_memslots_limit(struct vhost_dev *dev)
> > > > > +{
> > > > > +    return VHOST_MEMORY_MAX_NREGIONS;
> > > > > +}
> > > > > +
> > > > >  const VhostOps user_ops = {
> > > > >          .backend_type = VHOST_BACKEND_TYPE_USER,
> > > > >          .vhost_call = vhost_user_call,
> > > > >          .vhost_backend_init = vhost_user_init,
> > > > > -        .vhost_backend_cleanup = vhost_user_cleanup
> > > > > +        .vhost_backend_cleanup = vhost_user_cleanup,
> > > > > +        .vhost_backend_memslots_limit = vhost_user_memslots_limit
> > > > >          };
> > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > > > index 2712c6f..e964004 100644
> > > > > --- a/hw/virtio/vhost.c
> > > > > +++ b/hw/virtio/vhost.c
> > > > > @@ -26,6 +26,18 @@
> > > > >  
> > > > >  static struct vhost_log *vhost_log;
> > > > >  
> > > > > +static int used_memslots;
> > > > > +static int memslots_limit = -1;
> > > > 
> > > > I suspect this need to be in some structure, refcounted by vhost devices
> > > > using it. vhost_kernel_memslots_limit would then be called when 1st
> > > > vhost device is added.
> > > 
> > > vhost_kernel_memslots_limit() should be called for every added vhost device,
> > > since mixed vhost backends may have different limit.
> > > 
> > > refcounting might help with disabling limit check when all vhost devices
> > > are hot-unplugged and user will be able to plug in more memory and won't be
> > > able to add vhost device with lower limit.
> > > but I see such usecase highly not probable, hence it's considered
> > > adding refcounting as over-engineered way.
> > 
> > I don't see what's not probable here. It's trivial to reproduce.
> > One reason to do this is if you want to tweak the limit.
> > vhost needs to be unloaded for this.
> would be something like this suitable:
> 
> static struct {
>    int used_memslots;
>    int memslots_limit;
>    int refcount;
> } slots_limit;

I don't mind.

> > 
> > 
> > > > 
> > > > > +
> > > > > +bool vhost_has_free_slot(void)
> > > > > +{
> > > > > +    if (memslots_limit >= 0) {
> > > > > +        return memslots_limit > used_memslots;
> > > > > +    }
> > > > > +
> > > > > +    return true;
> > > > 
> > > > Pls rewrite this using logic operators.
> > > > Something like
> > > > 	return memslots_limit < 0 || memslots_limit > used_memslots; ?
> > > sure
> > > 
> > > > 
> > > > > +}
> > > > > +
> > > > >  static void vhost_dev_sync_region(struct vhost_dev *dev,
> > > > >                                    MemoryRegionSection *section,
> > > > >                                    uint64_t mfirst, uint64_t mlast,
> > > > > @@ -457,6 +469,7 @@ static void vhost_set_memory(MemoryListener *listener,
> > > > >      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);
> > > > >      dev->memory_changed = true;
> > > > > +    used_memslots = dev->mem->nregions;
> > > > >  }
> > > > >  
> > > > >  static bool vhost_section(MemoryRegionSection *section)
> > > > > @@ -1119,6 +1132,14 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> > > > >      if (r < 0) {
> > > > >          goto fail_features;
> > > > >      }
> > > > > +
> > > > > +    r = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
> > > > > +    if (memslots_limit > 0) {
> > > > > +        memslots_limit = MIN(memslots_limit, r);
> > > > > +    } else {
> > > > > +        memslots_limit = r;
> > > > > +    }
> > > > > +
> > > > >      r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_MEM_TABLE, hdev->mem);
> > > > >      if (r < 0) {
> > > > >          r = -errno;
> > > > 
> > > > Doing stuff on start but not stop is almost always a mistake.
> > > > In this case, when all vhost devices go away, one can
> > > > add more memory again, right?
> > > yep, it's possible provided vhost_call(VHOST_SET_MEM_TABLE) succeeds,
> > > if it's not then device addition fails gracefully.
> > 
> > VHOST_SET_MEM_TABLE on which device? All fds are gone.
> I've meant above in context of adding vhost devices after
> memory hotplug.
> 
> As for original question, the lowest limit will stay even if
> all vhost devices are gone with this impl.
> 

Right. Need to fix that.

> > 
> > > > 
> > > > > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> > > > > index e472f29..28b6714 100644
> > > > > --- a/include/hw/virtio/vhost-backend.h
> > > > > +++ b/include/hw/virtio/vhost-backend.h
> > > > > @@ -24,12 +24,14 @@ typedef int (*vhost_call)(struct vhost_dev *dev, unsigned long int request,
> > > > >               void *arg);
> > > > >  typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
> > > > >  typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
> > > > > +typedef int (*vhost_backend_memslots_limit)(struct vhost_dev *dev);
> > > > >  
> > > > >  typedef struct VhostOps {
> > > > >      VhostBackendType backend_type;
> > > > >      vhost_call vhost_call;
> > > > >      vhost_backend_init vhost_backend_init;
> > > > >      vhost_backend_cleanup vhost_backend_cleanup;
> > > > > +    vhost_backend_memslots_limit vhost_backend_memslots_limit;
> > > > >  } VhostOps;
> > > > >  
> > > > >  extern const VhostOps user_ops;
> > > > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > > > > index dd51050..17ff7b6 100644
> > > > > --- a/include/hw/virtio/vhost.h
> > > > > +++ b/include/hw/virtio/vhost.h
> > > > > @@ -81,4 +81,5 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> > > > >                              uint64_t features);
> > > > >  void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
> > > > >                          uint64_t features);
> > > > > +bool vhost_has_free_slot(void);
> > > > >  #endif
> > > > > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> > > > > index 9937a12..d2f1b21 100644
> > > > > --- a/stubs/Makefile.objs
> > > > > +++ b/stubs/Makefile.objs
> > > > > @@ -38,3 +38,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o
> > > > >  stub-obj-y += cpus.o
> > > > >  stub-obj-y += kvm.o
> > > > >  stub-obj-y += qmp_pc_dimm_device_list.o
> > > > > +stub-obj-y += vhost.o
> > > > > diff --git a/stubs/vhost.c b/stubs/vhost.c
> > > > > new file mode 100644
> > > > > index 0000000..d346b85
> > > > > --- /dev/null
> > > > > +++ b/stubs/vhost.c
> > > > > @@ -0,0 +1,6 @@
> > > > > +#include "hw/virtio/vhost.h"
> > > > > +
> > > > > +bool vhost_has_free_slot(void)
> > > > > +{
> > > > > +    return true;
> > > > > +}
> > > > > -- 
> > > > > 1.8.3.1

Re: [Qemu-devel] [PATCH for-2.4 v2 0/2] vhost: check if vhost has capacity for hotplugged memory

[Igor Mammedov]

On Thu, 30 Jul 2015 09:29:56 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jul 30, 2015 at 09:25:55AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jul 30, 2015 at 08:22:18AM +0200, Igor Mammedov wrote:
> > > On Wed, 29 Jul 2015 18:03:36 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Wed, Jul 29, 2015 at 01:49:47PM +0200, Igor Mammedov wrote:
> > > > > v1->v2:
> > > > >   * replace probbing with checking for
> > > > >     /sys/module/vhost/parameters/max_mem_regions and
> > > > >     if it's missing has non wrong value return
> > > > >     hardcoded legacy limit (64 slots).
> > > > > 
> > > > > it's defensive patchset which helps to avoid QEMU crashing
> > > > > at memory hotplug time by checking that vhost has free capacity
> > > > > for an additional memory slot.
> > > > 
> > > > What if vhost is added after memory hotplug? Don't you need
> > > > to check that as well?
> > > vhost device can be hotplugged after memory hotplug as far as
> > > current slots count doesn't exceed its limit,
> > > if limit is exceeded device_add would fail or virtio device
> > > would fallback to non vhost mode at its start-up (depends on
> > > how particular device treats vhost_start failure).
> > 
> > Where exactly does it fail?
> > memory_listener_register returns void so clearly it's not that ...
> 
> Oh, dev_start fails. But that's not called at device_add time.
> And vhost-user can't fall back to anything.
Yes, looks like it would lead to non functional vhost-user backed device
since there isn't any error handling at that stage.

But it's would be the same without memory hotplug also, one just has to
start QEMU with several -name memdev=xxx options to cause that condition.

Probably the best place to add this check is at vhost_net_init()
so that backend creation fails when one tries to add it on monitor/CLI

> 
> 
> > > > 
> > > > 
> > > > > 
> > > > > Igor Mammedov (2):
> > > > >   vhost: add vhost_has_free_slot() interface
> > > > >   pc-dimm: add vhost slots limit check before commiting to hotplug
> > > > > 
> > > > >  hw/mem/pc-dimm.c                  |  7 +++++++
> > > > >  hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
> > > > >  hw/virtio/vhost-user.c            |  8 +++++++-
> > > > >  hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
> > > > >  include/hw/virtio/vhost-backend.h |  2 ++
> > > > >  include/hw/virtio/vhost.h         |  1 +
> > > > >  stubs/Makefile.objs               |  1 +
> > > > >  stubs/vhost.c                     |  6 ++++++
> > > > >  8 files changed, 65 insertions(+), 2 deletions(-)
> > > > >  create mode 100644 stubs/vhost.c
> > > > > 
> > > > > -- 
> > > > > 1.8.3.1
> 

Re: [Qemu-devel] [PATCH for-2.4 v2 0/2] vhost: check if vhost has capacity for hotplugged memory

[Michael S. Tsirkin]

On Thu, Jul 30, 2015 at 09:31:34AM +0200, Igor Mammedov wrote:
> On Thu, 30 Jul 2015 09:29:56 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Jul 30, 2015 at 09:25:55AM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Jul 30, 2015 at 08:22:18AM +0200, Igor Mammedov wrote:
> > > > On Wed, 29 Jul 2015 18:03:36 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Wed, Jul 29, 2015 at 01:49:47PM +0200, Igor Mammedov wrote:
> > > > > > v1->v2:
> > > > > >   * replace probbing with checking for
> > > > > >     /sys/module/vhost/parameters/max_mem_regions and
> > > > > >     if it's missing has non wrong value return
> > > > > >     hardcoded legacy limit (64 slots).
> > > > > > 
> > > > > > it's defensive patchset which helps to avoid QEMU crashing
> > > > > > at memory hotplug time by checking that vhost has free capacity
> > > > > > for an additional memory slot.
> > > > > 
> > > > > What if vhost is added after memory hotplug? Don't you need
> > > > > to check that as well?
> > > > vhost device can be hotplugged after memory hotplug as far as
> > > > current slots count doesn't exceed its limit,
> > > > if limit is exceeded device_add would fail or virtio device
> > > > would fallback to non vhost mode at its start-up (depends on
> > > > how particular device treats vhost_start failure).
> > > 
> > > Where exactly does it fail?
> > > memory_listener_register returns void so clearly it's not that ...
> > 
> > Oh, dev_start fails. But that's not called at device_add time.
> > And vhost-user can't fall back to anything.
> Yes, looks like it would lead to non functional vhost-user backed device
> since there isn't any error handling at that stage.
> 
> But it's would be the same without memory hotplug also, one just has to
> start QEMU with several -name memdev=xxx options to cause that condition.

Absolutely. And kvm has this problem too if using kernels before 2014.

But I have a question: do we have to figure the number of
chunks exactly? How about being blunt, and just limiting the
number of memory devices?

How about this:
	- teach memory listeners about a new "max mem devices" field
	- when registering a listener, check that # of mem devices
	  does not exceed this limit, if not - fail registering
	  listener
	- when adding mem device, check no existing listener
	  has a limit that conflicts with it

Of course we could add a separate linked list+ register API with just this
field instead of adding it to a memory listener, if that seems
more appropriate.


> Probably the best place to add this check is at vhost_net_init()
> so that backend creation fails when one tries to add it on monitor/CLI

I'd say vhost_dev_init - it's not network specific at all.

> > 
> > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > Igor Mammedov (2):
> > > > > >   vhost: add vhost_has_free_slot() interface
> > > > > >   pc-dimm: add vhost slots limit check before commiting to hotplug
> > > > > > 
> > > > > >  hw/mem/pc-dimm.c                  |  7 +++++++
> > > > > >  hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
> > > > > >  hw/virtio/vhost-user.c            |  8 +++++++-
> > > > > >  hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
> > > > > >  include/hw/virtio/vhost-backend.h |  2 ++
> > > > > >  include/hw/virtio/vhost.h         |  1 +
> > > > > >  stubs/Makefile.objs               |  1 +
> > > > > >  stubs/vhost.c                     |  6 ++++++
> > > > > >  8 files changed, 65 insertions(+), 2 deletions(-)
> > > > > >  create mode 100644 stubs/vhost.c
> > > > > > 
> > > > > > -- 
> > > > > > 1.8.3.1
> > 

Re: [Qemu-devel] [PATCH for-2.4 v2 0/2] vhost: check if vhost has capacity for hotplugged memory

[Igor Mammedov]

On Thu, 30 Jul 2015 10:58:23 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jul 30, 2015 at 09:31:34AM +0200, Igor Mammedov wrote:
> > On Thu, 30 Jul 2015 09:29:56 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Thu, Jul 30, 2015 at 09:25:55AM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Jul 30, 2015 at 08:22:18AM +0200, Igor Mammedov wrote:
> > > > > On Wed, 29 Jul 2015 18:03:36 +0300
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > 
> > > > > > On Wed, Jul 29, 2015 at 01:49:47PM +0200, Igor Mammedov wrote:
> > > > > > > v1->v2:
> > > > > > >   * replace probbing with checking for
> > > > > > >     /sys/module/vhost/parameters/max_mem_regions and
> > > > > > >     if it's missing has non wrong value return
> > > > > > >     hardcoded legacy limit (64 slots).
> > > > > > > 
> > > > > > > it's defensive patchset which helps to avoid QEMU crashing
> > > > > > > at memory hotplug time by checking that vhost has free capacity
> > > > > > > for an additional memory slot.
> > > > > > 
> > > > > > What if vhost is added after memory hotplug? Don't you need
> > > > > > to check that as well?
> > > > > vhost device can be hotplugged after memory hotplug as far as
> > > > > current slots count doesn't exceed its limit,
> > > > > if limit is exceeded device_add would fail or virtio device
> > > > > would fallback to non vhost mode at its start-up (depends on
> > > > > how particular device treats vhost_start failure).
> > > > 
> > > > Where exactly does it fail?
> > > > memory_listener_register returns void so clearly it's not that ...
> > > 
> > > Oh, dev_start fails. But that's not called at device_add time.
> > > And vhost-user can't fall back to anything.
> > Yes, looks like it would lead to non functional vhost-user backed device
> > since there isn't any error handling at that stage.
> > 
> > But it's would be the same without memory hotplug also, one just has to
> > start QEMU with several -name memdev=xxx options to cause that condition.
> 
> Absolutely. And kvm has this problem too if using kernels before 2014.
> 
> But I have a question: do we have to figure the number of
> chunks exactly? How about being blunt, and just limiting the
> number of memory devices?
it would be guess work, number of chunks is not static and
changes during guest runtime as it configures devices so
chunks # at startup != chunks # at any other time


> 
> How about this:
> 	- teach memory listeners about a new "max mem devices" field
# of mem devices != # of memory ranges

> 	- when registering a listener, check that # of mem devices
> 	  does not exceed this limit, if not - fail registering
> 	  listener
> 	- when adding mem device, check no existing listener
> 	  has a limit that conflicts with it
> 
> Of course we could add a separate linked list+ register API with just this
> field instead of adding it to a memory listener, if that seems
> more appropriate.
> 
> 
> > Probably the best place to add this check is at vhost_net_init()
> > so that backend creation fails when one tries to add it on monitor/CLI
> 
> I'd say vhost_dev_init - it's not network specific at all.
> 
> > > 
> > > 
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > Igor Mammedov (2):
> > > > > > >   vhost: add vhost_has_free_slot() interface
> > > > > > >   pc-dimm: add vhost slots limit check before commiting to hotplug
> > > > > > > 
> > > > > > >  hw/mem/pc-dimm.c                  |  7 +++++++
> > > > > > >  hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
> > > > > > >  hw/virtio/vhost-user.c            |  8 +++++++-
> > > > > > >  hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
> > > > > > >  include/hw/virtio/vhost-backend.h |  2 ++
> > > > > > >  include/hw/virtio/vhost.h         |  1 +
> > > > > > >  stubs/Makefile.objs               |  1 +
> > > > > > >  stubs/vhost.c                     |  6 ++++++
> > > > > > >  8 files changed, 65 insertions(+), 2 deletions(-)
> > > > > > >  create mode 100644 stubs/vhost.c
> > > > > > > 
> > > > > > > -- 
> > > > > > > 1.8.3.1
> > > 
> 

Re: [Qemu-devel] [PATCH for-2.4 v2 0/2] vhost: check if vhost has capacity for hotplugged memory

[Michael S. Tsirkin]

On Thu, Jul 30, 2015 at 11:17:41AM +0200, Igor Mammedov wrote:
> On Thu, 30 Jul 2015 10:58:23 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Jul 30, 2015 at 09:31:34AM +0200, Igor Mammedov wrote:
> > > On Thu, 30 Jul 2015 09:29:56 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Thu, Jul 30, 2015 at 09:25:55AM +0300, Michael S. Tsirkin wrote:
> > > > > On Thu, Jul 30, 2015 at 08:22:18AM +0200, Igor Mammedov wrote:
> > > > > > On Wed, 29 Jul 2015 18:03:36 +0300
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > 
> > > > > > > On Wed, Jul 29, 2015 at 01:49:47PM +0200, Igor Mammedov wrote:
> > > > > > > > v1->v2:
> > > > > > > >   * replace probbing with checking for
> > > > > > > >     /sys/module/vhost/parameters/max_mem_regions and
> > > > > > > >     if it's missing has non wrong value return
> > > > > > > >     hardcoded legacy limit (64 slots).
> > > > > > > > 
> > > > > > > > it's defensive patchset which helps to avoid QEMU crashing
> > > > > > > > at memory hotplug time by checking that vhost has free capacity
> > > > > > > > for an additional memory slot.
> > > > > > > 
> > > > > > > What if vhost is added after memory hotplug? Don't you need
> > > > > > > to check that as well?
> > > > > > vhost device can be hotplugged after memory hotplug as far as
> > > > > > current slots count doesn't exceed its limit,
> > > > > > if limit is exceeded device_add would fail or virtio device
> > > > > > would fallback to non vhost mode at its start-up (depends on
> > > > > > how particular device treats vhost_start failure).
> > > > > 
> > > > > Where exactly does it fail?
> > > > > memory_listener_register returns void so clearly it's not that ...
> > > > 
> > > > Oh, dev_start fails. But that's not called at device_add time.
> > > > And vhost-user can't fall back to anything.
> > > Yes, looks like it would lead to non functional vhost-user backed device
> > > since there isn't any error handling at that stage.
> > > 
> > > But it's would be the same without memory hotplug also, one just has to
> > > start QEMU with several -name memdev=xxx options to cause that condition.
> > 
> > Absolutely. And kvm has this problem too if using kernels before 2014.
> > 
> > But I have a question: do we have to figure the number of
> > chunks exactly? How about being blunt, and just limiting the
> > number of memory devices?
> it would be guess work, number of chunks is not static and
> changes during guest runtime as it configures devices so
> chunks # at startup != chunks # at any other time
> 

But #chunks < # memory devices, correct?



> > 
> > How about this:
> > 	- teach memory listeners about a new "max mem devices" field
> # of mem devices != # of memory ranges

but # of memory ranges <= # of mem devices.
If we can support them all separately, we are fine.


And the big advantage is it's easy to document.
Just tell users "we support at most X mem devices" and that's all.


> > 	- when registering a listener, check that # of mem devices
> > 	  does not exceed this limit, if not - fail registering
> > 	  listener
> > 	- when adding mem device, check no existing listener
> > 	  has a limit that conflicts with it
> > 
> > Of course we could add a separate linked list+ register API with just this
> > field instead of adding it to a memory listener, if that seems
> > more appropriate.
> > 
> > 
> > > Probably the best place to add this check is at vhost_net_init()
> > > so that backend creation fails when one tries to add it on monitor/CLI
> > 
> > I'd say vhost_dev_init - it's not network specific at all.
> > 
> > > > 
> > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > Igor Mammedov (2):
> > > > > > > >   vhost: add vhost_has_free_slot() interface
> > > > > > > >   pc-dimm: add vhost slots limit check before commiting to hotplug
> > > > > > > > 
> > > > > > > >  hw/mem/pc-dimm.c                  |  7 +++++++
> > > > > > > >  hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
> > > > > > > >  hw/virtio/vhost-user.c            |  8 +++++++-
> > > > > > > >  hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
> > > > > > > >  include/hw/virtio/vhost-backend.h |  2 ++
> > > > > > > >  include/hw/virtio/vhost.h         |  1 +
> > > > > > > >  stubs/Makefile.objs               |  1 +
> > > > > > > >  stubs/vhost.c                     |  6 ++++++
> > > > > > > >  8 files changed, 65 insertions(+), 2 deletions(-)
> > > > > > > >  create mode 100644 stubs/vhost.c
> > > > > > > > 
> > > > > > > > -- 
> > > > > > > > 1.8.3.1
> > > > 
> > 

Re: [Qemu-devel] [PATCH for-2.4 v2 0/2] vhost: check if vhost has capacity for hotplugged memory

[Igor Mammedov]

On Thu, 30 Jul 2015 18:54:40 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jul 30, 2015 at 11:17:41AM +0200, Igor Mammedov wrote:
> > On Thu, 30 Jul 2015 10:58:23 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Thu, Jul 30, 2015 at 09:31:34AM +0200, Igor Mammedov wrote:
> > > > On Thu, 30 Jul 2015 09:29:56 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Thu, Jul 30, 2015 at 09:25:55AM +0300, Michael S. Tsirkin wrote:
> > > > > > On Thu, Jul 30, 2015 at 08:22:18AM +0200, Igor Mammedov wrote:
> > > > > > > On Wed, 29 Jul 2015 18:03:36 +0300
> > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > 
> > > > > > > > On Wed, Jul 29, 2015 at 01:49:47PM +0200, Igor Mammedov wrote:
> > > > > > > > > v1->v2:
> > > > > > > > >   * replace probbing with checking for
> > > > > > > > >     /sys/module/vhost/parameters/max_mem_regions and
> > > > > > > > >     if it's missing has non wrong value return
> > > > > > > > >     hardcoded legacy limit (64 slots).
> > > > > > > > > 
> > > > > > > > > it's defensive patchset which helps to avoid QEMU crashing
> > > > > > > > > at memory hotplug time by checking that vhost has free capacity
> > > > > > > > > for an additional memory slot.
> > > > > > > > 
> > > > > > > > What if vhost is added after memory hotplug? Don't you need
> > > > > > > > to check that as well?
> > > > > > > vhost device can be hotplugged after memory hotplug as far as
> > > > > > > current slots count doesn't exceed its limit,
> > > > > > > if limit is exceeded device_add would fail or virtio device
> > > > > > > would fallback to non vhost mode at its start-up (depends on
> > > > > > > how particular device treats vhost_start failure).
> > > > > > 
> > > > > > Where exactly does it fail?
> > > > > > memory_listener_register returns void so clearly it's not that ...
> > > > > 
> > > > > Oh, dev_start fails. But that's not called at device_add time.
> > > > > And vhost-user can't fall back to anything.
> > > > Yes, looks like it would lead to non functional vhost-user backed device
> > > > since there isn't any error handling at that stage.
> > > > 
> > > > But it's would be the same without memory hotplug also, one just has to
> > > > start QEMU with several -name memdev=xxx options to cause that condition.
> > > 
> > > Absolutely. And kvm has this problem too if using kernels before 2014.
> > > 
> > > But I have a question: do we have to figure the number of
> > > chunks exactly? How about being blunt, and just limiting the
> > > number of memory devices?
> > it would be guess work, number of chunks is not static and
> > changes during guest runtime as it configures devices so
> > chunks # at startup != chunks # at any other time
> > 
> 
> But #chunks < # memory devices, correct?
nope, it's opposite way around 1 memdev = 1MR and that
1MR could be fragmented in multiple chunks in flat view
if something overlay-ed over it.

> 
> 
> > > 
> > > How about this:
> > > 	- teach memory listeners about a new "max mem devices" field
> > # of mem devices != # of memory ranges
> 
> but # of memory ranges <= # of mem devices.
> If we can support them all separately, we are fine.
that should be # of memory ranges >= # of mem devices


> And the big advantage is it's easy to document.
> Just tell users "we support at most X mem devices" and that's all.
That won't work with #ranges >= #devices, I've already tried.

How about other way around to teach vhost handle upto 509 like it's
been done with KVM.
That should cover close future where we have max 256 hotplugged memory
devices and the rest would be free for using with "-numa memdev" or
for other purposes.


> > > 	- when registering a listener, check that # of mem devices
> > > 	  does not exceed this limit, if not - fail registering
> > > 	  listener
> > > 	- when adding mem device, check no existing listener
> > > 	  has a limit that conflicts with it
> > > 
> > > Of course we could add a separate linked list+ register API with just this
> > > field instead of adding it to a memory listener, if that seems
> > > more appropriate.
> > > 
> > > 
> > > > Probably the best place to add this check is at vhost_net_init()
> > > > so that backend creation fails when one tries to add it on monitor/CLI
> > > 
> > > I'd say vhost_dev_init - it's not network specific at all.
> > > 
> > > > > 
> > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Igor Mammedov (2):
> > > > > > > > >   vhost: add vhost_has_free_slot() interface
> > > > > > > > >   pc-dimm: add vhost slots limit check before commiting to hotplug
> > > > > > > > > 
> > > > > > > > >  hw/mem/pc-dimm.c                  |  7 +++++++
> > > > > > > > >  hw/virtio/vhost-backend.c         | 21 ++++++++++++++++++++-
> > > > > > > > >  hw/virtio/vhost-user.c            |  8 +++++++-
> > > > > > > > >  hw/virtio/vhost.c                 | 21 +++++++++++++++++++++
> > > > > > > > >  include/hw/virtio/vhost-backend.h |  2 ++
> > > > > > > > >  include/hw/virtio/vhost.h         |  1 +
> > > > > > > > >  stubs/Makefile.objs               |  1 +
> > > > > > > > >  stubs/vhost.c                     |  6 ++++++
> > > > > > > > >  8 files changed, 65 insertions(+), 2 deletions(-)
> > > > > > > > >  create mode 100644 stubs/vhost.c
> > > > > > > > > 
> > > > > > > > > -- 
> > > > > > > > > 1.8.3.1
> > > > > 
> > >