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

[Eugenio Perez Martin]

On Wed, Feb 14, 2024 at 2:01 PM 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>

It seems to me you missed the cover letter and sent 01/02 as the first message?

> ---
>  hw/virtio/vhost.c | 49 +++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 2c9ac79..ef6d9b5 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,8 @@ static int vhost_set_backend_type(struct vhost_dev *dev,
>          r = -1;
>      }
>
> +    assert(dev->vhost_ops->backend_type == backend_type || r < 0);

Is this a debug leftover (at least the r<0)? This should never be
reached effectively, but then it does not make sense to leave the
default switch branch.

> +
>      return r;
>  }
>
> @@ -319,16 +321,23 @@ 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;
> +
> +    if (backend_type == VHOST_BACKEND_TYPE_NONE ||
> +        backend_type >= VHOST_BACKEND_TYPE_MAX)
> +        return NULL;

The callers (vhost_log_resize, etc) don't expect vhost_log_get to
return NULL. I think all of these should be an assertion, if any.

The rest looks good to me.

> +
> +    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 +349,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 +370,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 +394,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 +2056,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) {
> +            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
>
>