| Message ID | 20230208094253.702672-12-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Dynamycally switch to vhost shadow virtqueues at vdpa net migration | expand |
在 2023/2/8 17:42, Eugenio Pérez 写道: > Next patches enable devices to be migrated even if vdpa netdev has not > been started with x-svq. However, not all devices are migratable, so we > need to block migration if we detect that. > > Block vhost-vdpa device migration if it does not offer _F_SUSPEND and it > has not been started with x-svq. > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > hw/virtio/vhost-vdpa.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 84a6b9690b..9d30cf9b3c 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -442,6 +442,27 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) > return 0; > } > > + /* > + * If dev->shadow_vqs_enabled at initialization that means the device has > + * been started with x-svq=on, so don't block migration > + */ > + if (dev->migration_blocker == NULL && !v->shadow_vqs_enabled) { > + uint64_t backend_features; > + > + /* We don't have dev->backend_features yet */ > + ret = vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, > + &backend_features); > + if (unlikely(ret)) { > + error_setg_errno(errp, -ret, "Could not get backend features"); > + return ret; > + } > + > + if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) { > + error_setg(&dev->migration_blocker, > + "vhost-vdpa backend lacks VHOST_BACKEND_F_SUSPEND feature."); > + } I wonder why not let the device to decide? For networking device, we can live without suspend probably. Thanks > + } > + > /* > * Similar to VFIO, we end up pinning all guest memory and have to > * disable discarding of RAM.
On Wed, Feb 22, 2023 at 5:05 AM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2023/2/8 17:42, Eugenio Pérez 写道: > > Next patches enable devices to be migrated even if vdpa netdev has not > > been started with x-svq. However, not all devices are migratable, so we > > need to block migration if we detect that. > > > > Block vhost-vdpa device migration if it does not offer _F_SUSPEND and it > > has not been started with x-svq. > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > hw/virtio/vhost-vdpa.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > index 84a6b9690b..9d30cf9b3c 100644 > > --- a/hw/virtio/vhost-vdpa.c > > +++ b/hw/virtio/vhost-vdpa.c > > @@ -442,6 +442,27 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) > > return 0; > > } > > > > + /* > > + * If dev->shadow_vqs_enabled at initialization that means the device has > > + * been started with x-svq=on, so don't block migration > > + */ > > + if (dev->migration_blocker == NULL && !v->shadow_vqs_enabled) { > > + uint64_t backend_features; > > + > > + /* We don't have dev->backend_features yet */ > > + ret = vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, > > + &backend_features); > > + if (unlikely(ret)) { > > + error_setg_errno(errp, -ret, "Could not get backend features"); > > + return ret; > > + } > > + > > + if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) { > > + error_setg(&dev->migration_blocker, > > + "vhost-vdpa backend lacks VHOST_BACKEND_F_SUSPEND feature."); > > + } > > > I wonder why not let the device to decide? For networking device, we can > live without suspend probably. > Right, but how can we know if this is a net device in init? I don't think a switch (vhost_vdpa_get_device_id(dev)) is elegant. If the parent device does not need to be suspended i'd go with exposing a suspend ioctl but do nothing in the parent device. After that, it could even choose to return an error for GET_VRING_BASE. If we want to implement it as a fallback in qemu, I'd go for implementing it on top of this series. There are a few operations we could move to a device-kind specific ops. Would it make sense to you? Thanks! > Thanks > > > > + } > > + > > /* > > * Similar to VFIO, we end up pinning all guest memory and have to > > * disable discarding of RAM. >
在 2023/2/22 22:25, Eugenio Perez Martin 写道: > On Wed, Feb 22, 2023 at 5:05 AM Jason Wang <jasowang@redhat.com> wrote: >> >> 在 2023/2/8 17:42, Eugenio Pérez 写道: >>> Next patches enable devices to be migrated even if vdpa netdev has not >>> been started with x-svq. However, not all devices are migratable, so we >>> need to block migration if we detect that. >>> >>> Block vhost-vdpa device migration if it does not offer _F_SUSPEND and it >>> has not been started with x-svq. >>> >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> >>> --- >>> hw/virtio/vhost-vdpa.c | 21 +++++++++++++++++++++ >>> 1 file changed, 21 insertions(+) >>> >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c >>> index 84a6b9690b..9d30cf9b3c 100644 >>> --- a/hw/virtio/vhost-vdpa.c >>> +++ b/hw/virtio/vhost-vdpa.c >>> @@ -442,6 +442,27 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) >>> return 0; >>> } >>> >>> + /* >>> + * If dev->shadow_vqs_enabled at initialization that means the device has >>> + * been started with x-svq=on, so don't block migration >>> + */ >>> + if (dev->migration_blocker == NULL && !v->shadow_vqs_enabled) { >>> + uint64_t backend_features; >>> + >>> + /* We don't have dev->backend_features yet */ >>> + ret = vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, >>> + &backend_features); >>> + if (unlikely(ret)) { >>> + error_setg_errno(errp, -ret, "Could not get backend features"); >>> + return ret; >>> + } >>> + >>> + if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) { >>> + error_setg(&dev->migration_blocker, >>> + "vhost-vdpa backend lacks VHOST_BACKEND_F_SUSPEND feature."); >>> + } >> >> I wonder why not let the device to decide? For networking device, we can >> live without suspend probably. >> > Right, but how can we know if this is a net device in init? I don't > think a switch (vhost_vdpa_get_device_id(dev)) is elegant. I meant the caller of vhost_vdpa_init() which is net_init_vhost_vdpa(). Thanks > > If the parent device does not need to be suspended i'd go with > exposing a suspend ioctl but do nothing in the parent device. After > that, it could even choose to return an error for GET_VRING_BASE. > > If we want to implement it as a fallback in qemu, I'd go for > implementing it on top of this series. There are a few operations we > could move to a device-kind specific ops. > > Would it make sense to you? > > Thanks! > > >> Thanks >> >> >>> + } >>> + >>> /* >>> * Similar to VFIO, we end up pinning all guest memory and have to >>> * disable discarding of RAM.
On Thu, Feb 23, 2023 at 3:38 AM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2023/2/22 22:25, Eugenio Perez Martin 写道: > > On Wed, Feb 22, 2023 at 5:05 AM Jason Wang <jasowang@redhat.com> wrote: > >> > >> 在 2023/2/8 17:42, Eugenio Pérez 写道: > >>> Next patches enable devices to be migrated even if vdpa netdev has not > >>> been started with x-svq. However, not all devices are migratable, so we > >>> need to block migration if we detect that. > >>> > >>> Block vhost-vdpa device migration if it does not offer _F_SUSPEND and it > >>> has not been started with x-svq. > >>> > >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > >>> --- > >>> hw/virtio/vhost-vdpa.c | 21 +++++++++++++++++++++ > >>> 1 file changed, 21 insertions(+) > >>> > >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > >>> index 84a6b9690b..9d30cf9b3c 100644 > >>> --- a/hw/virtio/vhost-vdpa.c > >>> +++ b/hw/virtio/vhost-vdpa.c > >>> @@ -442,6 +442,27 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) > >>> return 0; > >>> } > >>> > >>> + /* > >>> + * If dev->shadow_vqs_enabled at initialization that means the device has > >>> + * been started with x-svq=on, so don't block migration > >>> + */ > >>> + if (dev->migration_blocker == NULL && !v->shadow_vqs_enabled) { > >>> + uint64_t backend_features; > >>> + > >>> + /* We don't have dev->backend_features yet */ > >>> + ret = vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, > >>> + &backend_features); > >>> + if (unlikely(ret)) { > >>> + error_setg_errno(errp, -ret, "Could not get backend features"); > >>> + return ret; > >>> + } > >>> + > >>> + if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) { > >>> + error_setg(&dev->migration_blocker, > >>> + "vhost-vdpa backend lacks VHOST_BACKEND_F_SUSPEND feature."); > >>> + } > >> > >> I wonder why not let the device to decide? For networking device, we can > >> live without suspend probably. > >> > > Right, but how can we know if this is a net device in init? I don't > > think a switch (vhost_vdpa_get_device_id(dev)) is elegant. > > > I meant the caller of vhost_vdpa_init() which is net_init_vhost_vdpa(). > That's doable but I'm not sure if it is convenient. Since we're always offering _F_LOG I thought of the lack of _F_SUSPEND as the default migration blocker for other kinds of devices like blk. If we move this code to net_init_vhost_vdpa, all other devices are in charge of block migration by themselves. I guess the right action is to use a variable similar to vhost_vdpa->f_log_all. It defaults to false, and the device can choose if it should export it or not. This way, the device does not migrate by default, and the equivalent of net_init_vhost_vdpa could choose whether to offer _F_LOG with SVQ or not. OTOH I guess other kinds of devices already must place blockers beyond _F_LOG, so maybe it makes sense to always offer _F_LOG even if _F_SUSPEND is not offered? Stefano G., would that break vhost-vdpa-blk support? Thanks! > Thanks > > > > > > If the parent device does not need to be suspended i'd go with > > exposing a suspend ioctl but do nothing in the parent device. After > > that, it could even choose to return an error for GET_VRING_BASE. > > > > If we want to implement it as a fallback in qemu, I'd go for > > implementing it on top of this series. There are a few operations we > > could move to a device-kind specific ops. > > > > Would it make sense to you? > > > > Thanks! > > > > > >> Thanks > >> > >> > >>> + } > >>> + > >>> /* > >>> * Similar to VFIO, we end up pinning all guest memory and have to > >>> * disable discarding of RAM. >
On Thu, Feb 23, 2023 at 7:07 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Thu, Feb 23, 2023 at 3:38 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > 在 2023/2/22 22:25, Eugenio Perez Martin 写道: > > > On Wed, Feb 22, 2023 at 5:05 AM Jason Wang <jasowang@redhat.com> wrote: > > >> > > >> 在 2023/2/8 17:42, Eugenio Pérez 写道: > > >>> Next patches enable devices to be migrated even if vdpa netdev has not > > >>> been started with x-svq. However, not all devices are migratable, so we > > >>> need to block migration if we detect that. > > >>> > > >>> Block vhost-vdpa device migration if it does not offer _F_SUSPEND and it > > >>> has not been started with x-svq. > > >>> > > >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > >>> --- > > >>> hw/virtio/vhost-vdpa.c | 21 +++++++++++++++++++++ > > >>> 1 file changed, 21 insertions(+) > > >>> > > >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > >>> index 84a6b9690b..9d30cf9b3c 100644 > > >>> --- a/hw/virtio/vhost-vdpa.c > > >>> +++ b/hw/virtio/vhost-vdpa.c > > >>> @@ -442,6 +442,27 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) > > >>> return 0; > > >>> } > > >>> > > >>> + /* > > >>> + * If dev->shadow_vqs_enabled at initialization that means the device has > > >>> + * been started with x-svq=on, so don't block migration > > >>> + */ > > >>> + if (dev->migration_blocker == NULL && !v->shadow_vqs_enabled) { > > >>> + uint64_t backend_features; > > >>> + > > >>> + /* We don't have dev->backend_features yet */ > > >>> + ret = vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, > > >>> + &backend_features); > > >>> + if (unlikely(ret)) { > > >>> + error_setg_errno(errp, -ret, "Could not get backend features"); > > >>> + return ret; > > >>> + } > > >>> + > > >>> + if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) { > > >>> + error_setg(&dev->migration_blocker, > > >>> + "vhost-vdpa backend lacks VHOST_BACKEND_F_SUSPEND feature."); > > >>> + } > > >> > > >> I wonder why not let the device to decide? For networking device, we can > > >> live without suspend probably. > > >> > > > Right, but how can we know if this is a net device in init? I don't > > > think a switch (vhost_vdpa_get_device_id(dev)) is elegant. > > > > > > I meant the caller of vhost_vdpa_init() which is net_init_vhost_vdpa(). > > > > That's doable but I'm not sure if it is convenient. So it's a question whether or not we try to let migration work without suspending. If we don't, there's no need to bother. Looking at the current vhost-net implementation, it tries to make migration work upon the error of get_vring_base() so maybe it's worth a try if it doesn't bother too much. But I'm fine to go either way. > > Since we're always offering _F_LOG I thought of the lack of _F_SUSPEND > as the default migration blocker for other kinds of devices like blk. Or we can have this by default and allow a specific type of device to clear? > If we move this code to net_init_vhost_vdpa, all other devices are in > charge of block migration by themselves. > > I guess the right action is to use a variable similar to > vhost_vdpa->f_log_all. It defaults to false, and the device can choose > if it should export it or not. This way, the device does not migrate > by default, and the equivalent of net_init_vhost_vdpa could choose > whether to offer _F_LOG with SVQ or not. Looks similar to what I think above. > > OTOH I guess other kinds of devices already must place blockers beyond > _F_LOG, so maybe it makes sense to always offer _F_LOG even if > _F_SUSPEND is not offered? I don't see any dependency between the two features. Technically, there could be devices that have neither _F_LOG nor _F_SUSPEND. Thanks > Stefano G., would that break vhost-vdpa-blk > support? > > Thanks! > > > Thanks > > > > > > > > > > If the parent device does not need to be suspended i'd go with > > > exposing a suspend ioctl but do nothing in the parent device. After > > > that, it could even choose to return an error for GET_VRING_BASE. > > > > > > If we want to implement it as a fallback in qemu, I'd go for > > > implementing it on top of this series. There are a few operations we > > > could move to a device-kind specific ops. > > > > > > Would it make sense to you? > > > > > > Thanks! > > > > > > > > >> Thanks > > >> > > >> > > >>> + } > > >>> + > > >>> /* > > >>> * Similar to VFIO, we end up pinning all guest memory and have to > > >>> * disable discarding of RAM. > > >
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 84a6b9690b..9d30cf9b3c 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -442,6 +442,27 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) return 0; } + /* + * If dev->shadow_vqs_enabled at initialization that means the device has + * been started with x-svq=on, so don't block migration + */ + if (dev->migration_blocker == NULL && !v->shadow_vqs_enabled) { + uint64_t backend_features; + + /* We don't have dev->backend_features yet */ + ret = vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, + &backend_features); + if (unlikely(ret)) { + error_setg_errno(errp, -ret, "Could not get backend features"); + return ret; + } + + if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_SUSPEND))) { + error_setg(&dev->migration_blocker, + "vhost-vdpa backend lacks VHOST_BACKEND_F_SUSPEND feature."); + } + } + /* * Similar to VFIO, we end up pinning all guest memory and have to * disable discarding of RAM.
Next patches enable devices to be migrated even if vdpa netdev has not been started with x-svq. However, not all devices are migratable, so we need to block migration if we detect that. Block vhost-vdpa device migration if it does not offer _F_SUSPEND and it has not been started with x-svq. Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- hw/virtio/vhost-vdpa.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)