| Message ID | 20230224155438.112797-11-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Dynamically switch to vhost shadow virtqueues at vdpa net migration | expand |
在 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; > > } >
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; + } + 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; }
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(-)