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

[Laurent Vivier]

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