[v2,1/2] vhost: add used memslot number for vhost-user and vhost-kernel separately

Commit Message

Zhoujian (jay) Dec. 15, 2017, 8:45 a.m. UTC

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 <[e-mail]>
Signed-off-by: Zhe Liu <[e-mail]>
---
 hw/virtio/vhost-backend.c         | 14 ++++++++++++++
 hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++++++++
 hw/virtio/vhost.c                 | 16 +++++++++-------
 include/hw/virtio/vhost-backend.h |  4 ++++
 4 files changed, 58 insertions(+), 7 deletions(-)

Comments

Igor Mammedov Dec. 22, 2017, 4:25 p.m. UTC | #1

On Fri, 15 Dec 2017 16:45:54 +0800
Jay Zhou <[e-mail]> wrote:

> 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.

a bit of rant:
it seems vhost is chasing after a tail (1-2 regions that are not
actually RAM), while approach will allow to hotplug 1-2 extra dimms
vhost-user still will be limited to 8 regions max.


vhost-user protocol should be extended to allow qemu query
limit value from backend (which in turn should be tunable),
so that backend could be configured to meet user needs.

Jay,
would you like to look into it?

Otherwise it looks like patch will do its job,
see for a minor suggestion below.
 
> Signed-off-by: Jay Zhou <[e-mail]>
> Signed-off-by: Zhe Liu <[e-mail]>
> ---
>  hw/virtio/vhost-backend.c         | 14 ++++++++++++++
>  hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++++++++
>  hw/virtio/vhost.c                 | 16 +++++++++-------
>  include/hw/virtio/vhost-backend.h |  4 ++++
>  4 files changed, 58 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> index 7f09efa..866718c 100644
> --- a/hw/virtio/vhost-backend.c
> +++ b/hw/virtio/vhost-backend.c
> @@ -15,6 +15,8 @@
>  #include "hw/virtio/vhost-backend.h"
>  #include "qemu/error-report.h"
>  
> +static unsigned int vhost_kernel_used_memslots;
> +
>  static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int request,
>                               void *arg)
>  {
> @@ -233,6 +235,16 @@ static void vhost_kernel_set_iotlb_callback(struct vhost_dev *dev,
>          qemu_set_fd_handler((uintptr_t)dev->opaque, NULL, NULL, NULL);
>  }
>  
> +static void vhost_kernel_set_used_memslots(struct vhost_dev *dev)
> +{
> +    vhost_kernel_used_memslots = dev->mem->nregions;
> +}
> +
> +static unsigned int vhost_kernel_get_used_memslots(void)
> +{
> +    return vhost_kernel_used_memslots;
> +}
> +
>  static const VhostOps kernel_ops = {
>          .backend_type = VHOST_BACKEND_TYPE_KERNEL,
>          .vhost_backend_init = vhost_kernel_init,
> @@ -264,6 +276,8 @@ static const VhostOps kernel_ops = {
>  #endif /* CONFIG_VHOST_VSOCK */
>          .vhost_set_iotlb_callback = vhost_kernel_set_iotlb_callback,
>          .vhost_send_device_iotlb_msg = vhost_kernel_send_device_iotlb_msg,
> +        .vhost_set_used_memslots = vhost_kernel_set_used_memslots,
> +        .vhost_get_used_memslots = vhost_kernel_get_used_memslots,
>  };
>  
>  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 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_set_mem_table() already does the same counting,
there is no point in duplicating it, just drop vhost_set_used_memslots callback

> +    vhost_user_used_memslots = counter;
and update this value from vhost_user_set_mem_table()

> +}
> +
> +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..59a32e9 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -43,20 +43,21 @@
>  static struct vhost_log *vhost_log;
>  static struct vhost_log *vhost_log_shm;
>  
> -static unsigned int used_memslots;
>  static QLIST_HEAD(, vhost_dev) vhost_devices =
>      QLIST_HEAD_INITIALIZER(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() >=
> +            hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
> +            return false;
> +        }
>      }
> -    return slots_limit > used_memslots;
> +
> +    return true;
>  }
>  
>  static void vhost_dev_sync_region(struct vhost_dev *dev,
> @@ -606,7 +607,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;
> +    dev->vhost_ops->vhost_set_used_memslots(dev);
>  }
>  
>  static bool vhost_section(MemoryRegionSection *section)
> @@ -1251,7 +1252,8 @@ 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() >
> +        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;

Igor Mammedov Dec. 22, 2017, 7:03 p.m. UTC | #2

On Fri, 22 Dec 2017 17:25:13 +0100
Igor Mammedov <[e-mail]> wrote:

> On Fri, 15 Dec 2017 16:45:54 +0800
> Jay Zhou <[e-mail]> wrote:
[...]

> > +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_set_mem_table() already does the same counting,
> there is no point in duplicating it, just drop vhost_set_used_memslots callback
> 
> > +    vhost_user_used_memslots = counter;
> and update this value from vhost_user_set_mem_table()
never mind, updating it from vhost_user_set_mem_table() as it's called only when
device is started which is not the case when backend is initialized,
so the way you did it should work for both cases


> 
> > +}
> > +
[...]

Zhoujian (jay) Dec. 23, 2017, 6:01 a.m. UTC | #3

> -----Original Message-----
> From: Igor Mammedov [mailto:[e-mail]]
> Sent: Saturday, December 23, 2017 12:25 AM
> To: Zhoujian (jay) <[e-mail]>
> Cc: [e-mail]; [e-mail]; Huangweidong (C)
> <[e-mail]>; Gonglei (Arei) <[e-mail]>;
> wangxin (U) <[e-mail]>; Liuzhe (Cloud Open Labs, NFV)
> <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v2 1/2] vhost: add used memslot number for vhost-user
> and vhost-kernel separately
> 
> On Fri, 15 Dec 2017 16:45:54 +0800
> Jay Zhou <[e-mail]> wrote:
> 
> > 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.
> 
> a bit of rant:
> it seems vhost is chasing after a tail (1-2 regions that are not actually
> RAM), while approach will allow to hotplug 1-2 extra dimms vhost-user
> still will be limited to 8 regions max.
> 
> 
> vhost-user protocol should be extended to allow qemu query limit value
> from backend (which in turn should be tunable), so that backend could be
> configured to meet user needs.

Michael mentioned it can be extended with a protocol flag, here is the discussion
thread:
https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04656.html

> 
> Jay,
> would you like to look into it?

Yes, will look into it.

> 
> Otherwise it looks like patch will do its job, see for a minor suggestion
> below.
> 
> > Signed-off-by: Jay Zhou <[e-mail]>
> > Signed-off-by: Zhe Liu <[e-mail]>
> > ---
> >  hw/virtio/vhost-backend.c         | 14 ++++++++++++++
> >  hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++++++++
> >  hw/virtio/vhost.c                 | 16 +++++++++-------
> >  include/hw/virtio/vhost-backend.h |  4 ++++
> >  4 files changed, 58 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> > index 7f09efa..866718c 100644
> > --- a/hw/virtio/vhost-backend.c
> > +++ b/hw/virtio/vhost-backend.c
> > @@ -15,6 +15,8 @@
> >  #include "hw/virtio/vhost-backend.h"
> >  #include "qemu/error-report.h"
> >
> > +static unsigned int vhost_kernel_used_memslots;
> > +
> >  static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int
> request,
> >                               void *arg)  { @@ -233,6 +235,16 @@
> > static void vhost_kernel_set_iotlb_callback(struct vhost_dev *dev,
> >          qemu_set_fd_handler((uintptr_t)dev->opaque, NULL, NULL,
> > NULL);  }
> >
> > +static void vhost_kernel_set_used_memslots(struct vhost_dev *dev) {
> > +    vhost_kernel_used_memslots = dev->mem->nregions; }
> > +
> > +static unsigned int vhost_kernel_get_used_memslots(void)
> > +{
> > +    return vhost_kernel_used_memslots; }
> > +
> >  static const VhostOps kernel_ops = {
> >          .backend_type = VHOST_BACKEND_TYPE_KERNEL,
> >          .vhost_backend_init = vhost_kernel_init, @@ -264,6 +276,8 @@
> > static const VhostOps kernel_ops = {  #endif /* CONFIG_VHOST_VSOCK */
> >          .vhost_set_iotlb_callback = vhost_kernel_set_iotlb_callback,
> >          .vhost_send_device_iotlb_msg =
> > vhost_kernel_send_device_iotlb_msg,
> > +        .vhost_set_used_memslots = vhost_kernel_set_used_memslots,
> > +        .vhost_get_used_memslots = vhost_kernel_get_used_memslots,
> >  };
> >
> >  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 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_set_mem_table() already does the same counting, there is no
> point in duplicating it, just drop vhost_set_used_memslots callback
> 
> > +    vhost_user_used_memslots = counter;
> and update this value from vhost_user_set_mem_table()
> 

Answered in another thread.

Regards,
Jay

> > +}
> > +
> > +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..59a32e9 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -43,20 +43,21 @@
> >  static struct vhost_log *vhost_log;
> >  static struct vhost_log *vhost_log_shm;
> >
> > -static unsigned int used_memslots;
> >  static QLIST_HEAD(, vhost_dev) vhost_devices =
> >      QLIST_HEAD_INITIALIZER(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() >=
> > +            hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
> > +            return false;
> > +        }
> >      }
> > -    return slots_limit > used_memslots;
> > +
> > +    return true;
> >  }
> >
> >  static void vhost_dev_sync_region(struct vhost_dev *dev, @@ -606,7
> > +607,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;
> > +    dev->vhost_ops->vhost_set_used_memslots(dev);
> >  }
> >
> >  static bool vhost_section(MemoryRegionSection *section) @@ -1251,7
> > +1252,8 @@ 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() >
> > +        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;

Zhoujian (jay) Dec. 23, 2017, 7:09 a.m. UTC | #4

Hi Igor,

> -----Original Message-----
> From: Igor Mammedov [mailto:[e-mail]]
> Sent: Saturday, December 23, 2017 3:03 AM
> To: Zhoujian (jay) <[e-mail]>
> Cc: Huangweidong (C) <[e-mail]>; [e-mail]; wangxin
> (U) <[e-mail]>; [e-mail]; Liuzhe (Cloud
> Open Labs, NFV) <[e-mail]>; [e-mail]; Gonglei
> (Arei) <[e-mail]>
> Subject: Re: [Qemu-devel] [PATCH v2 1/2] vhost: add used memslot number
> for vhost-user and vhost-kernel separately
> 
> On Fri, 22 Dec 2017 17:25:13 +0100
> Igor Mammedov <[e-mail]> wrote:
> 
> > On Fri, 15 Dec 2017 16:45:54 +0800
> > Jay Zhou <[e-mail]> wrote:
> [...]
> 
> > > +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_set_mem_table() already does the same counting, there is no
> > point in duplicating it, just drop vhost_set_used_memslots callback
> >
> > > +    vhost_user_used_memslots = counter;
> > and update this value from vhost_user_set_mem_table()
> never mind, updating it from vhost_user_set_mem_table() as it's called
> only when device is started which is not the case when backend is
> initialized, so the way you did it should work for both cases
> 

How about do it like this(not sure whether the best way, any idea?):

Define a new function, e.g.

static void vhost_user_set_used_memslots_and_msgs(struct vhost_dev *dev,
                            VhostUserMsg *msg, size_t *fd_num, int *fds)
{
    int num = 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) {
            if (msg && fd_num && fds) {
                msg->payload.memory.regions[num].userspace_addr
                                        = reg->userspace_addr;
                msg->payload.memory.regions[num].memory_size
                                        = reg->memory_size;
                msg->payload.memory.regions[num].guest_phys_addr
                                        = reg->guest_phys_addr;
                msg->payload.memory.regions[num].mmap_offset = offset;
                assert(num < VHOST_MEMORY_MAX_NREGIONS);
                fds[num++] = fd;
            }
        }
    }
    vhost_user_used_memslots = num;
    if (fd_num)
        *fd_num = num;
}

And call it when the backend is initialized and the device is started
respectively,

static void vhost_user_set_used_memslots(struct vhost_dev *dev)
{
	vhost_user_set_used_memslots_and_msgs(dev, NULL, NULL, NULL);
}

static int vhost_user_set_mem_table(struct vhost_dev *dev,
						struct vhost_memory *mem)
{
	[...]

	vhost_user_set_used_memslots_and_msgs(dev, &msg, &fd_num, fds);

	[...]
}


Regards,
Jay

> 
> >
> > > +}
> > > +
> [...]

Igor Mammedov Dec. 28, 2017, 11:11 a.m. UTC | #5

On Sat, 23 Dec 2017 07:09:51 +0000
"Zhoujian (jay)" <[e-mail]> wrote:

> Hi Igor,
> 
> > -----Original Message-----
> > From: Igor Mammedov [mailto:[e-mail]]
> > Sent: Saturday, December 23, 2017 3:03 AM
> > To: Zhoujian (jay) <[e-mail]>
> > Cc: Huangweidong (C) <[e-mail]>; [e-mail]; wangxin
> > (U) <[e-mail]>; [e-mail]; Liuzhe (Cloud
> > Open Labs, NFV) <[e-mail]>; [e-mail]; Gonglei
> > (Arei) <[e-mail]>
> > Subject: Re: [Qemu-devel] [PATCH v2 1/2] vhost: add used memslot number
> > for vhost-user and vhost-kernel separately
> > 
> > On Fri, 22 Dec 2017 17:25:13 +0100
> > Igor Mammedov <[e-mail]> wrote:
> > 
> > > On Fri, 15 Dec 2017 16:45:54 +0800
> > > Jay Zhou <[e-mail]> wrote:
> > [...]
> > 
> > > > +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_set_mem_table() already does the same counting, there is no
> > > point in duplicating it, just drop vhost_set_used_memslots callback
> > >
> > > > +    vhost_user_used_memslots = counter;
> > > and update this value from vhost_user_set_mem_table()
> > never mind, updating it from vhost_user_set_mem_table() as it's called
> > only when device is started which is not the case when backend is
> > initialized, so the way you did it should work for both cases
> > 
> 
> How about do it like this(not sure whether the best way, any idea?):
> 
> Define a new function, e.g.
> 
> static void vhost_user_set_used_memslots_and_msgs(struct vhost_dev *dev,
>                             VhostUserMsg *msg, size_t *fd_num, int *fds)
s/vhost_user_set_used_memslots_and_msgs/vhost_user_prepare_msg/

> {
>     int num = 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) {
>             if (msg && fd_num && fds) {
>                 msg->payload.memory.regions[num].userspace_addr
>                                         = reg->userspace_addr;
>                 msg->payload.memory.regions[num].memory_size
>                                         = reg->memory_size;
>                 msg->payload.memory.regions[num].guest_phys_addr
>                                         = reg->guest_phys_addr;
>                 msg->payload.memory.regions[num].mmap_offset = offset;
>                 assert(num < VHOST_MEMORY_MAX_NREGIONS);
there is no need to assert here function can return error.
 
>                 fds[num++] = fd;
Is num counting in wrong place?

   if (msg && fd_num && fds) => false
is the case vhost_user_set_used_memslots(), so it won't count 'num'
which is later used to intialize vhost_user_used_memslots.

>             }
>         }
>     }
>     vhost_user_used_memslots = num;
add comment here as it won't be obvious to reader why vhost_user_used_memslots is here


>     if (fd_num)
>         *fd_num = num;
msg.payload.memory.nregions can be used directly instead of num amd fd_num
and it's 1 less argument to pass out of function.

> }
> 
> And call it when the backend is initialized and the device is started
> respectively,
> 
> static void vhost_user_set_used_memslots(struct vhost_dev *dev)
static int

for return value

> {
> 	vhost_user_set_used_memslots_and_msgs(dev, NULL, NULL, NULL);
we can do here
        vhost_user_set_used_memslots_and_msgs(dev, &msg, &fd_num, fds);
and discard/ignore results, that way it will be less conditionals inside of called function

> }
> 
> static int vhost_user_set_mem_table(struct vhost_dev *dev,
> 						struct vhost_memory *mem)
> {
> 	[...]
> 
> 	vhost_user_set_used_memslots_and_msgs(dev, &msg, &fd_num, fds);
   rc = vhost_user_set_used_memslots_and_msgs(dev, &msg, &fd_num, fds);

and return error from vhost_user_set_mem_table(), caller should be able
to coupe with error.

> 
> 	[...]
> }
> 
> 
> Regards,
> Jay
> 
> > 
> > >
> > > > +}
> > > > +
> > [...]
>

Zhoujian (jay) Dec. 28, 2017, 1:42 p.m. UTC | #6

> -----Original Message-----
> From: Igor Mammedov [mailto:[e-mail]]
> Sent: Thursday, December 28, 2017 7:12 PM
> To: Zhoujian (jay) <[e-mail]>
> Cc: Huangweidong (C) <[e-mail]>; [e-mail]; wangxin
> (U) <[e-mail]>; [e-mail]; Liuzhe (Cloud Open
> Labs, NFV) <[e-mail]>; [e-mail]; Gonglei (Arei)
> <[e-mail]>
> Subject: Re: [Qemu-devel] [PATCH v2 1/2] vhost: add used memslot number
> for vhost-user and vhost-kernel separately
> 
> On Sat, 23 Dec 2017 07:09:51 +0000
> "Zhoujian (jay)" <[e-mail]> wrote:
> 
> > Hi Igor,
> >
> > > -----Original Message-----
> > > From: Igor Mammedov [mailto:[e-mail]]
> > > Sent: Saturday, December 23, 2017 3:03 AM
> > > To: Zhoujian (jay) <[e-mail]>
> > > Cc: Huangweidong (C) <[e-mail]>; [e-mail];
> > > wangxin
> > > (U) <[e-mail]>; [e-mail]; Liuzhe
> > > (Cloud Open Labs, NFV) <[e-mail]>;
> > > [e-mail]; Gonglei
> > > (Arei) <[e-mail]>
> > > Subject: Re: [Qemu-devel] [PATCH v2 1/2] vhost: add used memslot
> > > number for vhost-user and vhost-kernel separately
> > >
> > > On Fri, 22 Dec 2017 17:25:13 +0100
> > > Igor Mammedov <[e-mail]> wrote:
> > >
> > > > On Fri, 15 Dec 2017 16:45:54 +0800 Jay Zhou
> > > > <[e-mail]> wrote:
> > > [...]
> > >
> > > > > +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_set_mem_table() already does the same counting, there
> > > > is no point in duplicating it, just drop vhost_set_used_memslots
> > > > callback
> > > >
> > > > > +    vhost_user_used_memslots = counter;
> > > > and update this value from vhost_user_set_mem_table()
> > > never mind, updating it from vhost_user_set_mem_table() as it's
> > > called only when device is started which is not the case when
> > > backend is initialized, so the way you did it should work for both
> > > cases
> > >
> >
> > How about do it like this(not sure whether the best way, any idea?):
> >
> > Define a new function, e.g.
> >
> > static void vhost_user_set_used_memslots_and_msgs(struct vhost_dev *dev,
> >                             VhostUserMsg *msg, size_t *fd_num, int
> > *fds)
> s/vhost_user_set_used_memslots_and_msgs/vhost_user_prepare_msg/

Ok

> 
> > {
> >     int num = 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) {
> >             if (msg && fd_num && fds) {
> >                 msg->payload.memory.regions[num].userspace_addr
> >                                         = reg->userspace_addr;
> >                 msg->payload.memory.regions[num].memory_size
> >                                         = reg->memory_size;
> >                 msg->payload.memory.regions[num].guest_phys_addr
> >                                         = reg->guest_phys_addr;
> >                 msg->payload.memory.regions[num].mmap_offset = offset;
> >                 assert(num < VHOST_MEMORY_MAX_NREGIONS);
> there is no need to assert here function can return error.

Ok

> 
> >                 fds[num++] = fd;
> Is num counting in wrong place?
> 
>    if (msg && fd_num && fds) => false
> is the case vhost_user_set_used_memslots(), so it won't count 'num'
> which is later used to intialize vhost_user_used_memslots.

My bad, will fix.

> >             }
> >         }
> >     }
> >     vhost_user_used_memslots = num;
> add comment here as it won't be obvious to reader why
> vhost_user_used_memslots is here

Will do.

> 
> >     if (fd_num)
> >         *fd_num = num;
> msg.payload.memory.nregions can be used directly instead of num amd fd_num
> and it's 1 less argument to pass out of function.
> 
> > }
> >
> > And call it when the backend is initialized and the device is started
> > respectively,
> >
> > static void vhost_user_set_used_memslots(struct vhost_dev *dev)
> static int
> 
> for return value
> 
> > {
> > 	vhost_user_set_used_memslots_and_msgs(dev, NULL, NULL, NULL);
> we can do here
>         vhost_user_set_used_memslots_and_msgs(dev, &msg, &fd_num, fds);
> and discard/ignore results, that way it will be less conditionals inside
> of called function
> 
> > }
> >
> > static int vhost_user_set_mem_table(struct vhost_dev *dev,
> > 						struct vhost_memory *mem)
> > {
> > 	[...]
> >
> > 	vhost_user_set_used_memslots_and_msgs(dev, &msg, &fd_num, fds);
>    rc = vhost_user_set_used_memslots_and_msgs(dev, &msg, &fd_num, fds);
> 
> and return error from vhost_user_set_mem_table(), caller should be able to
> coupe with error.
> 

Will fix all of them in the next version.

Regards,
Jay

Patch

diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
index 7f09efa..866718c 100644
--- a/hw/virtio/vhost-backend.c
+++ b/hw/virtio/vhost-backend.c
@@ -15,6 +15,8 @@ 
 #include "hw/virtio/vhost-backend.h"
 #include "qemu/error-report.h"
 
+static unsigned int vhost_kernel_used_memslots;
+
 static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int request,
                              void *arg)
 {
@@ -233,6 +235,16 @@  static void vhost_kernel_set_iotlb_callback(struct vhost_dev *dev,
         qemu_set_fd_handler((uintptr_t)dev->opaque, NULL, NULL, NULL);
 }
 
+static void vhost_kernel_set_used_memslots(struct vhost_dev *dev)
+{
+    vhost_kernel_used_memslots = dev->mem->nregions;
+}
+
+static unsigned int vhost_kernel_get_used_memslots(void)
+{
+    return vhost_kernel_used_memslots;
+}
+
 static const VhostOps kernel_ops = {
         .backend_type = VHOST_BACKEND_TYPE_KERNEL,
         .vhost_backend_init = vhost_kernel_init,
@@ -264,6 +276,8 @@  static const VhostOps kernel_ops = {
 #endif /* CONFIG_VHOST_VSOCK */
         .vhost_set_iotlb_callback = vhost_kernel_set_iotlb_callback,
         .vhost_send_device_iotlb_msg = vhost_kernel_send_device_iotlb_msg,
+        .vhost_set_used_memslots = vhost_kernel_set_used_memslots,
+        .vhost_get_used_memslots = vhost_kernel_get_used_memslots,
 };
 
 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 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..59a32e9 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -43,20 +43,21 @@ 
 static struct vhost_log *vhost_log;
 static struct vhost_log *vhost_log_shm;
 
-static unsigned int used_memslots;
 static QLIST_HEAD(, vhost_dev) vhost_devices =
     QLIST_HEAD_INITIALIZER(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() >=
+            hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
+            return false;
+        }
     }
-    return slots_limit > used_memslots;
+
+    return true;
 }
 
 static void vhost_dev_sync_region(struct vhost_dev *dev,
@@ -606,7 +607,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;
+    dev->vhost_ops->vhost_set_used_memslots(dev);
 }
 
 static bool vhost_section(MemoryRegionSection *section)
@@ -1251,7 +1252,8 @@  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() >
+        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;