On Sat, Mar 16, 2024 at 2:33 AM Si-Wei Liu <si-wei....@oracle.com> wrote:
>
>
>
> On 3/14/2024 8:50 PM, Jason Wang wrote:
> > On Fri, Mar 15, 2024 at 5:39 AM Si-Wei Liu <si-wei....@oracle.com> wrote:
> >> There could be a mix of both vhost-user and vhost-kernel clients
> >> in the same QEMU process, where separate vhost loggers for the
> >> specific vhost type have to be used. Make the vhost logger per
> >> backend type, and have them properly reference counted.
> > It's better to describe what's the advantage of doing this.
> Yes, I can add that to the log. Although it's a niche use case, it was
> actually a long standing limitation / bug that vhost-user and
> vhost-kernel loggers can't co-exist per QEMU process, but today it's
> just silent failure that may be ended up with. This bug fix removes that
> implicit limitation in the code.
Ok.
> >
> >> Suggested-by: Michael S. Tsirkin <m...@redhat.com>
> >> Signed-off-by: Si-Wei Liu <si-wei....@oracle.com>
> >>
> >> ---
> >> v3->v4:
> >> - remove checking NULL return value from vhost_log_get
> >>
> >> v2->v3:
> >> - remove non-effective assertion that never be reached
> >> - do not return NULL from vhost_log_get()
> >> - add neccessary assertions to vhost_log_get()
> >> ---
> >> hw/virtio/vhost.c | 45 +++++++++++++++++++++++++++++++++------------
> >> 1 file changed, 33 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> index 2c9ac79..612f4db 100644
> >> --- a/hw/virtio/vhost.c
> >> +++ b/hw/virtio/vhost.c
> >> @@ -43,8 +43,8 @@
> >> do { } while (0)
> >> #endif
> >>
> >> -static struct vhost_log *vhost_log;
> >> -static struct vhost_log *vhost_log_shm;
> >> +static struct vhost_log *vhost_log[VHOST_BACKEND_TYPE_MAX];
> >> +static struct vhost_log *vhost_log_shm[VHOST_BACKEND_TYPE_MAX];
> >>
> >> /* Memslots used by backends that support private memslots (without an
> >> fd). */
> >> static unsigned int used_memslots;
> >> @@ -287,6 +287,10 @@ static int vhost_set_backend_type(struct vhost_dev
> >> *dev,
> >> r = -1;
> >> }
> >>
> >> + if (r == 0) {
> >> + assert(dev->vhost_ops->backend_type == backend_type);
> >> + }
> >> +
> > Under which condition could we hit this?
> Just in case some other function inadvertently corrupted this earlier,
> we have to capture discrepancy in the first place... On the other hand,
> it will be helpful for other vhost backend writers to diagnose day-one
> bug in the code. I feel just code comment here will not be
> sufficient/helpful.
See below.
>
> > It seems not good to assert a local logic.
> It seems to me quite a few local asserts are in the same file already,
> vhost_save_backend_state,
For example it has assert for
assert(!dev->started);
which is not the logic of the function itself but require
vhost_dev_start() not to be called before.
But it looks like this patch you assert the code just a few lines
above the assert itself?
dev->vhost_ops = &xxx_ops;
...
assert(dev->vhost_ops->backend_type == backend_type)
?
Thanks
> vhost_load_backend_state,
> vhost_virtqueue_mask, vhost_config_mask, just to name a few. Why local
> assert a problem?
>
> Thanks,
> -Siwei
>
> > Thanks
> >
>
