| Message ID | 20220211161309.1385839-1-lvivier@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] vhost-vdpa: make notifiers _init()/_uninit() symmetric | expand |
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 >
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
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 >
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 > >
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; }
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(-)