I see how this fixes vhost_has_free_slot() to correctly determine whether or not regions can be added, but why are the used_memslots_exceeded variable and the used_memslots_is_exceeded() API needed?
On Mon, Aug 10, 2020 at 9:44 PM Jiajun Chen <chenjiaj...@huawei.com> 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. > > Signed-off-by: Jiajun Chen <chenjiaj...@huawei.com> > Signed-off-by: Jianjay Zhou <jianjay.z...@huawei.com> > --- > hw/virtio/vhost-backend.c | 14 ++++++++ > hw/virtio/vhost-user.c | 28 ++++++++++++++++ > hw/virtio/vhost.c | 54 +++++++++++++++++++++++++------ > include/hw/virtio/vhost-backend.h | 5 +++ > include/hw/virtio/vhost.h | 1 + > net/vhost-user.c | 7 ++++ > 6 files changed, 100 insertions(+), 9 deletions(-) > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 31231218dc..04d20fc3ee 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -2354,6 +2355,31 @@ void vhost_user_cleanup(VhostUserState *user) > user->chr = NULL; > } > > +static void vhost_user_set_used_memslots(struct vhost_dev *dev) > +{ > + unsigned 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; > + > + assert((uintptr_t)reg->userspace_addr == reg->userspace_addr); > + mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr, > + &offset); Why not use the vhost_user_get_mr_data helper? It would simplify the code a bit. > + if (mr && memory_region_get_fd(mr) > 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_backend_init, > @@ -2387,4 +2413,6 @@ const VhostOps user_ops = { > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 1a1384e7a6..7f36d7af25 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1346,9 +1373,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void > *opaque, > memory_listener_register(&hdev->memory_listener, &address_space_memory); > QLIST_INSERT_HEAD(&vhost_devices, hdev, entry); > > - if (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"); > + /* > + * If we started a VM without any vhost device, > + * vhost_dev_used_memslots_is_exceeded will always return false for the > + * first time vhost device hot-plug(vhost_get_used_memslots is always 0), > + * so it needs to double check here > + */ > + if (vhost_dev_used_memslots_is_exceeded(hdev)) { Why can't we just check if hdev->vhost_ops->vhost_get_used_memslots() > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)? > r = -1; > if (busyloop_timeout) { > goto fail_busyloop; > @@ -1773,3 +1804,8 @@ int vhost_net_set_backend(struct vhost_dev *hdev, > > return -1; > } > + > +bool used_memslots_is_exceeded(void) > +{ > + return used_memslots_exceeded; > +} > diff --git a/net/vhost-user.c b/net/vhost-user.c > index 17532daaf3..2f0216b518 100644 > --- a/net/vhost-user.c > +++ b/net/vhost-user.c > @@ -347,6 +348,12 @@ static int net_vhost_user_init(NetClientState *peer, > const char *device, > qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, > net_vhost_user_event, NULL, nc0->name, NULL, > true); > + > + if (used_memslots_is_exceeded()) { Why can't you use vhost_has_free_slot() instead here? > + error_report("used memslots exceeded the backend limit, quit " > + "loop"); > + goto err; > + } > } while (!s->started); > > assert(s->vhost_net); > -- > 2.27.0.dirty > >