Re: [PATCH v4 1/2] vhost: dirty log should be per backend type

[Si-Wei Liu]

On 3/14/2024 8:50 PM, Jason Wang wrote:
> On Fri, Mar 15, 2024 at 5:39 AM Si-Wei Liu <si-wei.liu@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.
> > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Si-Wei Liu <si-wei.liu@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.

>   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, vhost_load_backend_state, 
vhost_virtqueue_mask, vhost_config_mask, just to name a few. Why local 
assert a problem?

Thanks,
-Siwei

> Thanks