Re: [PATCH] vhost-user: add separate memslot counter for vhost-user

[Raphael Norwitz]

On Mon, Oct 12, 2020 at 7:12 AM chenjiajun <chenjiajun8@huawei.com> wrote:
>
>
>
> On 2020/10/2 10:05, Raphael Norwitz wrote:
> > On Mon, Sep 28, 2020 at 9:17 AM Jiajun Chen <chenjiajun8@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.
> >>
> >> --
> >> ChangeList:
> >> v3:
> >> -make used_memslots a member of struct vhost_dev instead of a global 
> >> static value
> >>
> >> v2:
> >> -eliminating useless used_memslots_exceeded variable and 
> >> used_memslots_is_exceeded() API
> >>
> >> v1:
> >> -vhost-user: add separate memslot counter for vhost-user
> >>
> >> Signed-off-by: Jiajun Chen <chenjiajun8@huawei.com>
> >> Signed-off-by: Jianjay Zhou <jianjay.zhou@huawei.com>
> >
> > I'm happy with this from a vhost/vhost-user perspective. vhost-backend
> > change looks good too. I'm a little confused by what's going on with
> > net/vhost-user.c.
> >
> >> ---
> >>  hw/virtio/vhost-backend.c         | 12 ++++++++++
> >>  hw/virtio/vhost-user.c            | 25 +++++++++++++++++++++
> >>  hw/virtio/vhost.c                 | 37 +++++++++++++++++++++++--------
> >>  include/hw/virtio/vhost-backend.h |  5 +++++
> >>  include/hw/virtio/vhost.h         |  1 +
> >>  net/vhost-user.c                  |  7 ++++++
> >>  6 files changed, 78 insertions(+), 9 deletions(-)
> >>
> >
> >> diff --git a/net/vhost-user.c b/net/vhost-user.c
> >> index 17532daaf3..7e93955537 100644
> >> --- a/net/vhost-user.c
> >> +++ b/net/vhost-user.c
> >> @@ -20,6 +20,7 @@
> >>  #include "qemu/error-report.h"
> >>  #include "qemu/option.h"
> >>  #include "trace.h"
> >> +#include "include/hw/virtio/vhost.h"
> >>
> >>  typedef struct NetVhostUserState {
> >>      NetClientState nc;
> >> @@ -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);
> >
> > Can you elaborate on this check here? What does it have to do with
> > fixing memslots accounting? Maybe it should be in a separate change?
> >
> When the number of virtual machine memslots exceeds the upper limit of the 
> back-end support,
> QEMU main thread may enters an endless loop and cannot process other requests.
> And number of memslots will not automatically decrease, so add a check here 
> to exit from loop
> in this scenario. For the newly started virtual machine, boot fails; for the 
> hot plug network card,
> hot plug fails.

I don't understand what you mean by "number of memslots will not
automatically decrease". Where did this happen before and what changes
when the new memslots counter is introduced?

> >> +
> >> +        if (!vhost_has_free_slot()) {
> >> +            error_report("used memslots exceeded the backend limit, quit "
> >> +                         "loop");
> >> +            goto err;
> >> +        }
> >>      } while (!s->started);
> >>
> >>      assert(s->vhost_net);
> >> --
> >> 2.27.0.dirty
> >>
> > .
> >