diff mbox series

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

Message ID 1710401215-25490-1-git-send-email-si-wei.liu@oracle.com (mailing list archive)
State New, archived
Headers show
Series [v3,1/2] vhost: dirty log should be per backend type | expand

Commit Message

Si-Wei Liu March 14, 2024, 7:26 a.m. UTC 
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(-)

Comments

Eugenio Perez Martin March 14, 2024, 3:25 p.m. UTC | #1 
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 :).

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
>
Si-Wei Liu March 14, 2024, 6:35 p.m. UTC | #2 
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
>>
diff mbox series

Patch

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) {
+            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,