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

[Si-Wei Liu]

On 3/14/2024 8:25 AM, Eugenio Perez Martin wrote:
> On Thu, Mar 14, 2024 at 9:38 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.
> >
> > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> > ---
> > 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 | 50 ++++++++++++++++++++++++++++++++++++++------------
> >   1 file changed, 38 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 2c9ac79..efe2f74 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);
> > +    }
> > +
> >       return r;
> >   }
> >
> > @@ -319,16 +323,22 @@ static struct vhost_log *vhost_log_alloc(uint64_t size, 
> > bool share)
> >       return log;
> >   }
> >
> > -static struct vhost_log *vhost_log_get(uint64_t size, bool share)
> > +static struct vhost_log *vhost_log_get(VhostBackendType backend_type,
> > +                                       uint64_t size, bool share)
> >   {
> > -    struct vhost_log *log = share ? vhost_log_shm : vhost_log;
> > +    struct vhost_log *log;
> > +
> > +    assert(backend_type > VHOST_BACKEND_TYPE_NONE);
> > +    assert(backend_type < VHOST_BACKEND_TYPE_MAX);
> > +
> > +    log = share ? vhost_log_shm[backend_type] : vhost_log[backend_type];
> >
> >       if (!log || log->size != size) {
> >           log = vhost_log_alloc(size, share);
> >           if (share) {
> > -            vhost_log_shm = log;
> > +            vhost_log_shm[backend_type] = log;
> >           } else {
> > -            vhost_log = log;
> > +            vhost_log[backend_type] = log;
> >           }
> >       } else {
> >           ++log->refcnt;
> > @@ -340,11 +350,20 @@ static struct vhost_log *vhost_log_get(uint64_t size, 
> > bool share)
> >   static void vhost_log_put(struct vhost_dev *dev, bool sync)
> >   {
> >       struct vhost_log *log = dev->log;
> > +    VhostBackendType backend_type;
> >
> >       if (!log) {
> >           return;
> >       }
> >
> > +    assert(dev->vhost_ops);
> > +    backend_type = dev->vhost_ops->backend_type;
> > +
> > +    if (backend_type == VHOST_BACKEND_TYPE_NONE ||
> > +        backend_type >= VHOST_BACKEND_TYPE_MAX) {
> > +        return;
> > +    }
> > +
> >       --log->refcnt;
> >       if (log->refcnt == 0) {
> >           /* Sync only the range covered by the old log */
> > @@ -352,13 +371,13 @@ static void vhost_log_put(struct vhost_dev *dev, bool 
> > sync)
> >               vhost_log_sync_range(dev, 0, dev->log_size * VHOST_LOG_CHUNK - 1);
> >           }
> >
> > -        if (vhost_log == log) {
> > +        if (vhost_log[backend_type] == log) {
> >               g_free(log->log);
> > -            vhost_log = NULL;
> > -        } else if (vhost_log_shm == log) {
> > +            vhost_log[backend_type] = NULL;
> > +        } else if (vhost_log_shm[backend_type] == log) {
> >               qemu_memfd_free(log->log, log->size * sizeof(*(log->log)),
> >                               log->fd);
> > -            vhost_log_shm = NULL;
> > +            vhost_log_shm[backend_type] = NULL;
> >           }
> >
> >           g_free(log);
> > @@ -376,7 +395,8 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
> >
> >   static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
> >   {
> > -    struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> > +    struct vhost_log *log = vhost_log_get(dev->vhost_ops->backend_type,
> > +                                          size, vhost_dev_log_is_shared(dev));
> >       uint64_t log_base = (uintptr_t)log->log;
> >       int r;
> >
> > @@ -2037,8 +2057,14 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice 
> > *vdev, bool vrings)
> >           uint64_t log_base;
> >
> >           hdev->log_size = vhost_get_log_size(hdev);
> > -        hdev->log = vhost_log_get(hdev->log_size,
> > +        hdev->log = vhost_log_get(hdev->vhost_ops->backend_type,
> > +                                  hdev->log_size,
> >                                     vhost_dev_log_is_shared(hdev));
> > +        if (!hdev->log) {
> I thought vhost_log_get couldn't return NULL :).
Sure, missed that. Will post a revised v4.

-Siwei
> Other than that,
>
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
>
> > +            VHOST_OPS_DEBUG(r, "vhost_log_get failed");
> > +            goto fail_vq;
> > +        }
> > +
> >           log_base = (uintptr_t)hdev->log->log;
> >           r = hdev->vhost_ops->vhost_set_log_base(hdev,
> >                                                   hdev->log_size ? log_base : 0,
> > --
> > 1.8.3.1