[v4,10/15] vdpa: disable RAM block discard only for the first device

Although it does not make a big difference, its more correct and
simplifies the cleanup path in subsequent patches.

Move ram_block_discard_disable(false) call to the top of
vhost_vdpa_cleanup because:
* We cannot use vhost_vdpa_first_dev after dev->opaque = NULL
  assignment.
* Improve the stack order in cleanup: since it is the last action taken
  in init, it should be the first at cleanup.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

在 2023/2/24 23:54, Eugenio Pérez 写道:
> Although it does not make a big difference, its more correct and
> simplifies the cleanup path in subsequent patches.
>
> Move ram_block_discard_disable(false) call to the top of
> vhost_vdpa_cleanup because:
> * We cannot use vhost_vdpa_first_dev after dev->opaque = NULL
>    assignment.
> * Improve the stack order in cleanup: since it is the last action taken
>    in init, it should be the first at cleanup.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/virtio/vhost-vdpa.c | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 71e3dc21fe..27655e7582 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -431,16 +431,6 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp)
>       trace_vhost_vdpa_init(dev, opaque);
>       int ret;
>   
> -    /*
> -     * Similar to VFIO, we end up pinning all guest memory and have to
> -     * disable discarding of RAM.
> -     */
> -    ret = ram_block_discard_disable(true);
> -    if (ret) {
> -        error_report("Cannot set discarding of RAM broken");
> -        return ret;
> -    }
> -
>       v = opaque;
>       v->dev = dev;
>       dev->opaque =  opaque ;
> @@ -452,6 +442,16 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp)
>           return 0;
>       }
>   
> +    /*
> +     * Similar to VFIO, we end up pinning all guest memory and have to
> +     * disable discarding of RAM.
> +     */
> +    ret = ram_block_discard_disable(true);
> +    if (ret) {
> +        error_report("Cannot set discarding of RAM broken");
> +        return ret;
> +    }


We seems to lose the chance to free svq allocated by 
vhost_vdpa_init_svq() in this case?

Thanks


> +
>       vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>                                  VIRTIO_CONFIG_S_DRIVER);
>   
> @@ -577,12 +577,15 @@ static int vhost_vdpa_cleanup(struct vhost_dev *dev)
>       assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
>       v = dev->opaque;
>       trace_vhost_vdpa_cleanup(dev, v);
> +    if (vhost_vdpa_first_dev(dev)) {
> +        ram_block_discard_disable(false);
> +    }
> +
>       vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
>       memory_listener_unregister(&v->listener);
>       vhost_vdpa_svq_cleanup(dev);
>   
>       dev->opaque = NULL;
> -    ram_block_discard_disable(false);
>   
>       return 0;
>   }

On Mon, Feb 27, 2023 at 9:11 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2023/2/24 23:54, Eugenio Pérez 写道:
> > Although it does not make a big difference, its more correct and
> > simplifies the cleanup path in subsequent patches.
> >
> > Move ram_block_discard_disable(false) call to the top of
> > vhost_vdpa_cleanup because:
> > * We cannot use vhost_vdpa_first_dev after dev->opaque = NULL
> >    assignment.
> > * Improve the stack order in cleanup: since it is the last action taken
> >    in init, it should be the first at cleanup.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   hw/virtio/vhost-vdpa.c | 25 ++++++++++++++-----------
> >   1 file changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 71e3dc21fe..27655e7582 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -431,16 +431,6 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp)
> >       trace_vhost_vdpa_init(dev, opaque);
> >       int ret;
> >
> > -    /*
> > -     * Similar to VFIO, we end up pinning all guest memory and have to
> > -     * disable discarding of RAM.
> > -     */
> > -    ret = ram_block_discard_disable(true);
> > -    if (ret) {
> > -        error_report("Cannot set discarding of RAM broken");
> > -        return ret;
> > -    }
> > -
> >       v = opaque;
> >       v->dev = dev;
> >       dev->opaque =  opaque ;
> > @@ -452,6 +442,16 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp)
> >           return 0;
> >       }
> >
> > +    /*
> > +     * Similar to VFIO, we end up pinning all guest memory and have to
> > +     * disable discarding of RAM.
> > +     */
> > +    ret = ram_block_discard_disable(true);
> > +    if (ret) {
> > +        error_report("Cannot set discarding of RAM broken");
> > +        return ret;
> > +    }
>
>
> We seems to lose the chance to free svq allocated by
> vhost_vdpa_init_svq() in this case?
>

Right, I'll fix it in the next version.

Thanks!

> Thanks
>
>
> > +
> >       vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> >                                  VIRTIO_CONFIG_S_DRIVER);
> >
> > @@ -577,12 +577,15 @@ static int vhost_vdpa_cleanup(struct vhost_dev *dev)
> >       assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
> >       v = dev->opaque;
> >       trace_vhost_vdpa_cleanup(dev, v);
> > +    if (vhost_vdpa_first_dev(dev)) {
> > +        ram_block_discard_disable(false);
> > +    }
> > +
> >       vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
> >       memory_listener_unregister(&v->listener);
> >       vhost_vdpa_svq_cleanup(dev);
> >
> >       dev->opaque = NULL;
> > -    ram_block_discard_disable(false);
> >
> >       return 0;
> >   }
>