1. Patchew
  2. QEMU
  3. View series

[RFC] vhost-vdpa: make notifiers _init()/_uninit() symmetric

Laurent Vivier posted 1 patch 3 years, 12 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220211161309.1385839-1-lvivier@redhat.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>
hw/virtio/vhost-vdpa.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
[RFC] vhost-vdpa: make notifiers _init()/_uninit() symmetric
Posted by Laurent Vivier 3 years, 12 months ago 
vhost_vdpa_host_notifiers_init() initializes queue notifiers
for queues "dev->vq_index" to queue "dev->vq_index + dev->nvqs",
whereas vhost_vdpa_host_notifiers_uninit() uninitializes the
same notifiers for queue "0" to queue "dev->nvqs".

This asymmetry seems buggy, fix that by using dev->vq_index
as the base for both.

Fixes: d0416d487bd5 ("vhost-vdpa: map virtqueue notification area if possible")
Cc: jasowang@redhat.com
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 04ea43704f5d..9be3dc66580c 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -395,15 +395,6 @@ static void vhost_vdpa_host_notifier_uninit(struct vhost_dev *dev,
     }
 }
 
-static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
-{
-    int i;
-
-    for (i = 0; i < n; i++) {
-        vhost_vdpa_host_notifier_uninit(dev, i);
-    }
-}
-
 static int vhost_vdpa_host_notifier_init(struct vhost_dev *dev, int queue_index)
 {
     size_t page_size = qemu_real_host_page_size;
@@ -442,6 +433,15 @@ err:
     return -1;
 }
 
+static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
+{
+    int i;
+
+    for (i = dev->vq_index; i < dev->vq_index + n; i++) {
+        vhost_vdpa_host_notifier_uninit(dev, i);
+    }
+}
+
 static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev)
 {
     int i;
@@ -455,7 +455,7 @@ static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev)
     return;
 
 err:
-    vhost_vdpa_host_notifiers_uninit(dev, i);
+    vhost_vdpa_host_notifiers_uninit(dev, i - dev->vq_index);
     return;
 }
 
-- 
2.34.1
Re: [RFC] vhost-vdpa: make notifiers _init()/_uninit() symmetric
Posted by Jason Wang 3 years, 11 months ago 
On Sat, Feb 12, 2022 at 12:13 AM Laurent Vivier <lvivier@redhat.com> wrote:
>
> vhost_vdpa_host_notifiers_init() initializes queue notifiers
> for queues "dev->vq_index" to queue "dev->vq_index + dev->nvqs",
> whereas vhost_vdpa_host_notifiers_uninit() uninitializes the
> same notifiers for queue "0" to queue "dev->nvqs".
>
> This asymmetry seems buggy, fix that by using dev->vq_index
> as the base for both.
>
> Fixes: d0416d487bd5 ("vhost-vdpa: map virtqueue notification area if possible")
> Cc: jasowang@redhat.com
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  hw/virtio/vhost-vdpa.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 04ea43704f5d..9be3dc66580c 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -395,15 +395,6 @@ static void vhost_vdpa_host_notifier_uninit(struct vhost_dev *dev,
>      }
>  }
>
> -static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
> -{
> -    int i;
> -
> -    for (i = 0; i < n; i++) {
> -        vhost_vdpa_host_notifier_uninit(dev, i);
> -    }
> -}
> -
>  static int vhost_vdpa_host_notifier_init(struct vhost_dev *dev, int queue_index)
>  {
>      size_t page_size = qemu_real_host_page_size;
> @@ -442,6 +433,15 @@ err:
>      return -1;
>  }
>
> +static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
> +{
> +    int i;
> +
> +    for (i = dev->vq_index; i < dev->vq_index + n; i++) {
> +        vhost_vdpa_host_notifier_uninit(dev, i);
> +    }
> +}

Patch looks good but I wonder why we need to move this function?

Thanks

> +
>  static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev)
>  {
>      int i;
> @@ -455,7 +455,7 @@ static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev)
>      return;
>
>  err:
> -    vhost_vdpa_host_notifiers_uninit(dev, i);
> +    vhost_vdpa_host_notifiers_uninit(dev, i - dev->vq_index);
>      return;
>  }
>
> --
> 2.34.1
>
Re: [RFC] vhost-vdpa: make notifiers _init()/_uninit() symmetric
Posted by Laurent Vivier 3 years, 11 months ago 
On 14/02/2022 04:20, Jason Wang wrote:
> On Sat, Feb 12, 2022 at 12:13 AM Laurent Vivier <lvivier@redhat.com> wrote:
>>
>> vhost_vdpa_host_notifiers_init() initializes queue notifiers
>> for queues "dev->vq_index" to queue "dev->vq_index + dev->nvqs",
>> whereas vhost_vdpa_host_notifiers_uninit() uninitializes the
>> same notifiers for queue "0" to queue "dev->nvqs".
>>
>> This asymmetry seems buggy, fix that by using dev->vq_index
>> as the base for both.
>>
>> Fixes: d0416d487bd5 ("vhost-vdpa: map virtqueue notification area if possible")
>> Cc: jasowang@redhat.com
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   hw/virtio/vhost-vdpa.c | 20 ++++++++++----------
>>   1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> index 04ea43704f5d..9be3dc66580c 100644
>> --- a/hw/virtio/vhost-vdpa.c
>> +++ b/hw/virtio/vhost-vdpa.c
>> @@ -395,15 +395,6 @@ static void vhost_vdpa_host_notifier_uninit(struct vhost_dev *dev,
>>       }
>>   }
>>
>> -static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
>> -{
>> -    int i;
>> -
>> -    for (i = 0; i < n; i++) {
>> -        vhost_vdpa_host_notifier_uninit(dev, i);
>> -    }
>> -}
>> -
>>   static int vhost_vdpa_host_notifier_init(struct vhost_dev *dev, int queue_index)
>>   {
>>       size_t page_size = qemu_real_host_page_size;
>> @@ -442,6 +433,15 @@ err:
>>       return -1;
>>   }
>>
>> +static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
>> +{
>> +    int i;
>> +
>> +    for (i = dev->vq_index; i < dev->vq_index + n; i++) {
>> +        vhost_vdpa_host_notifier_uninit(dev, i);
>> +    }
>> +}
> 
> Patch looks good but I wonder why we need to move this function?

I moved the _uninit function close to the _init one to be able to compare them easier.
I think it will help reviewers in the future if code is side-by-side.

But we can let it at its original place.

Thanks,
Laurent
Re: [RFC] vhost-vdpa: make notifiers _init()/_uninit() symmetric
Posted by Jason Wang 3 years, 11 months ago 
On Mon, Feb 14, 2022 at 3:33 PM Laurent Vivier <lvivier@redhat.com> wrote:
>
> On 14/02/2022 04:20, Jason Wang wrote:
> > On Sat, Feb 12, 2022 at 12:13 AM Laurent Vivier <lvivier@redhat.com> wrote:
> >>
> >> vhost_vdpa_host_notifiers_init() initializes queue notifiers
> >> for queues "dev->vq_index" to queue "dev->vq_index + dev->nvqs",
> >> whereas vhost_vdpa_host_notifiers_uninit() uninitializes the
> >> same notifiers for queue "0" to queue "dev->nvqs".
> >>
> >> This asymmetry seems buggy, fix that by using dev->vq_index
> >> as the base for both.
> >>
> >> Fixes: d0416d487bd5 ("vhost-vdpa: map virtqueue notification area if possible")
> >> Cc: jasowang@redhat.com
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> ---
> >>   hw/virtio/vhost-vdpa.c | 20 ++++++++++----------
> >>   1 file changed, 10 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >> index 04ea43704f5d..9be3dc66580c 100644
> >> --- a/hw/virtio/vhost-vdpa.c
> >> +++ b/hw/virtio/vhost-vdpa.c
> >> @@ -395,15 +395,6 @@ static void vhost_vdpa_host_notifier_uninit(struct vhost_dev *dev,
> >>       }
> >>   }
> >>
> >> -static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
> >> -{
> >> -    int i;
> >> -
> >> -    for (i = 0; i < n; i++) {
> >> -        vhost_vdpa_host_notifier_uninit(dev, i);
> >> -    }
> >> -}
> >> -
> >>   static int vhost_vdpa_host_notifier_init(struct vhost_dev *dev, int queue_index)
> >>   {
> >>       size_t page_size = qemu_real_host_page_size;
> >> @@ -442,6 +433,15 @@ err:
> >>       return -1;
> >>   }
> >>
> >> +static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
> >> +{
> >> +    int i;
> >> +
> >> +    for (i = dev->vq_index; i < dev->vq_index + n; i++) {
> >> +        vhost_vdpa_host_notifier_uninit(dev, i);
> >> +    }
> >> +}
> >
> > Patch looks good but I wonder why we need to move this function?
>
> I moved the _uninit function close to the _init one to be able to compare them easier.
> I think it will help reviewers in the future if code is side-by-side.

Fine.

So

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

>
> But we can let it at its original place.
>
> Thanks,
> Laurent
>
Re: [RFC] vhost-vdpa: make notifiers _init()/_uninit() symmetric
Posted by Stefano Garzarella 3 years, 11 months ago 
On Fri, Feb 11, 2022 at 05:13:09PM +0100, Laurent Vivier wrote:
>vhost_vdpa_host_notifiers_init() initializes queue notifiers
>for queues "dev->vq_index" to queue "dev->vq_index + dev->nvqs",
>whereas vhost_vdpa_host_notifiers_uninit() uninitializes the
>same notifiers for queue "0" to queue "dev->nvqs".
>
>This asymmetry seems buggy, fix that by using dev->vq_index
>as the base for both.
>
>Fixes: d0416d487bd5 ("vhost-vdpa: map virtqueue notification area if possible")
>Cc: jasowang@redhat.com
>Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>---
> hw/virtio/vhost-vdpa.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>index 04ea43704f5d..9be3dc66580c 100644
>--- a/hw/virtio/vhost-vdpa.c
>+++ b/hw/virtio/vhost-vdpa.c
>@@ -395,15 +395,6 @@ static void vhost_vdpa_host_notifier_uninit(struct vhost_dev *dev,
>     }
> }
>
>-static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
>-{
>-    int i;
>-
>-    for (i = 0; i < n; i++) {
>-        vhost_vdpa_host_notifier_uninit(dev, i);
>-    }
>-}
>-
> static int vhost_vdpa_host_notifier_init(struct vhost_dev *dev, int queue_index)
> {
>     size_t page_size = qemu_real_host_page_size;
>@@ -442,6 +433,15 @@ err:
>     return -1;
> }
>
>+static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
>+{
>+    int i;
>+
>+    for (i = dev->vq_index; i < dev->vq_index + n; i++) {
>+        vhost_vdpa_host_notifier_uninit(dev, i);
>+    }
>+}
>+
> static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev)
> {
>     int i;
>@@ -455,7 +455,7 @@ static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev)
>     return;
>
> err:
>-    vhost_vdpa_host_notifiers_uninit(dev, i);
>+    vhost_vdpa_host_notifiers_uninit(dev, i - dev->vq_index);
>     return;
> }
>
>-- 
>2.34.1
>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.