src/libvirt_private.syms | 5 ++ src/qemu/qemu_hostdev.c | 16 +++++ src/qemu/qemu_hostdev.h | 4 ++ src/qemu/qemu_hotplug.c | 56 ++++++++++++++---- src/util/virfile.c | 52 +++++++++++++++++ src/util/virfile.h | 1 + src/util/virhostdev.c | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++----- src/util/virhostdev.h | 16 +++++ src/util/virpci.c | 69 ++++++++++++++++++---- src/util/virpci.h | 4 ++ 10 files changed, 360 insertions(+), 36 deletions(-)
Hi everyone. Hoping to get some feedback on this approach, or some
alternatives proposed below, to the following issue:
Currently libvirt immediately attempts to rebind a managed device back to the
host driver when it receives a DEVICE_DELETED event from QEMU. This is
problematic for 2 reasons:
1) If multiple devices from a group are attached to a guest, this can move
the group into a "non-viable" state where some devices are assigned to
the host and some to the guest.
2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase
where additional cleanup occurs. In most cases libvirt can ignore this
cleanup, but in the case of VFIO devices this is where closing of a VFIO
group FD occurs, and failing to wait before rebinding the device to the
host driver can result in unexpected behavior. In the case of powernv
hosts at least, this can lead to a host driver crashing due to the default
DMA windows not having been fully-restored yet. The window between this is
and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've
seen host dumps with Mellanox CX4 VFs being rebound to host driver during
this period (on powernv hosts).
Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver
until all the devices in the group have been detached, at which point all
the hostdevs are rebound as a group. Until that point, the devices are traced
by the drvManager's inactiveList in a similar manner to hostdevs that are
assigned to VFIO via the nodedev-detach interface.
Patch 5 addresses 2) by adding an additional check that, when the last device
from a group is detached, polls /proc for open FDs referencing the VFIO group
path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we
time out, we abandon rebinding the hostdevs back to the host.
There are a couple alternatives to Patch 5 that might be worth considering:
a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of
DEVICE_DELETED. Paired with patches 1-4 this would let us drop patch 5 in
favor of minimal changes to libvirt's event handlers.
The downsides are:
- that we'd incur some latency for all device-detach calls, but it's not
apparent to me whether this delay is significant for anything outside
of VFIO.
- there may be cases where finalization after DEVICE_DELETE/unparent are
is not guaranteed, and I'm not sure QOM would encourage such
expectations even if that's currently the case.
b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most
direct solution. With this we could completely separate out the handling
of rebinding to host driver based on receival of this event.
The downsides are:
- this would only work for newer versions of QEMU, though we could use
the poll-wait in patch 5 as a fallback.
- synchronizing sync/async device-detach threads with sync/async
handlers for this would be a bit hairy, but I have a WIP in progress
that seems *fairly reasonable*
c) Take the approach in Patch 5, either as a precursor to implementing b) or
something else, or just sticking with that for now.
d) ???
Personally I'm leaning toward c), but I'm wondering if that's "good enough"
for now, or if we should pursue something more robust from the start, or
perhaps something else entirely?
Any feedback is greatly appreciated!
src/libvirt_private.syms | 5 ++
src/qemu/qemu_hostdev.c | 16 +++++
src/qemu/qemu_hostdev.h | 4 ++
src/qemu/qemu_hotplug.c | 56 ++++++++++++++----
src/util/virfile.c | 52 +++++++++++++++++
src/util/virfile.h | 1 +
src/util/virhostdev.c | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
src/util/virhostdev.h | 16 +++++
src/util/virpci.c | 69 ++++++++++++++++++----
src/util/virpci.h | 4 ++
10 files changed, 360 insertions(+), 36 deletions(-)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jun 28, 2017 at 07:24:55PM -0500, Michael Roth wrote: > Hi everyone. Hoping to get some feedback on this approach, or some > alternatives proposed below, to the following issue: > > Currently libvirt immediately attempts to rebind a managed device back to the > host driver when it receives a DEVICE_DELETED event from QEMU. This is > problematic for 2 reasons: > > 1) If multiple devices from a group are attached to a guest, this can move > the group into a "non-viable" state where some devices are assigned to > the host and some to the guest. > > 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase > where additional cleanup occurs. In most cases libvirt can ignore this > cleanup, but in the case of VFIO devices this is where closing of a VFIO > group FD occurs, and failing to wait before rebinding the device to the > host driver can result in unexpected behavior. In the case of powernv > hosts at least, this can lead to a host driver crashing due to the default > DMA windows not having been fully-restored yet. The window between this is > and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've > seen host dumps with Mellanox CX4 VFs being rebound to host driver during > this period (on powernv hosts). Why on earth does QEMU's device finalization take 6 seconds to complete. That feels very broken to me, unless QEMU is not being schedled due to host being overcomitted. If that's not the case, then we have a bug to investigate in QEMU to find out why cleanup is delayed so long. From libvirt's POV, we consider 'DEVICE_DELETED' as meaning both that the frontend has gone *and* the corresponding backend has gone. Aside from cleaning the VFIO group, we use this as a trigger for all other device related cleanup like SELinux labelling, cgroup device ACLs, etc. If the backend is not guaranteed to be closed in QEMU when this emit is emitted then either QEMU needs to delay the event until it is really cleaned up, or QEMU needs to add a further event to emit when the backend is clean. > Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver > until all the devices in the group have been detached, at which point all > the hostdevs are rebound as a group. Until that point, the devices are traced > by the drvManager's inactiveList in a similar manner to hostdevs that are > assigned to VFIO via the nodedev-detach interface. > > Patch 5 addresses 2) by adding an additional check that, when the last device > from a group is detached, polls /proc for open FDs referencing the VFIO group > path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we > time out, we abandon rebinding the hostdevs back to the host. That is just gross - it is tieing libvirt to details of the QEMU internal implementation. I really don't think we should be doing that. So NACK to this from my POV. > There are a couple alternatives to Patch 5 that might be worth considering: > > a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of > DEVICE_DELETED. Paired with patches 1-4 this would let us drop patch 5 in > favor of minimal changes to libvirt's event handlers. > > The downsides are: > - that we'd incur some latency for all device-detach calls, but it's not > apparent to me whether this delay is significant for anything outside > of VFIO. > - there may be cases where finalization after DEVICE_DELETE/unparent are > is not guaranteed, and I'm not sure QOM would encourage such > expectations even if that's currently the case. > > b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most > direct solution. With this we could completely separate out the handling > of rebinding to host driver based on receival of this event. > > The downsides are: > - this would only work for newer versions of QEMU, though we could use > the poll-wait in patch 5 as a fallback. > - synchronizing sync/async device-detach threads with sync/async > handlers for this would be a bit hairy, but I have a WIP in progress > that seems *fairly reasonable* > > c) Take the approach in Patch 5, either as a precursor to implementing b) or > something else, or just sticking with that for now. > > d) ??? Fix DEVICE_DELETE so its only emitted when the backend associated with the device is fully cleaned up. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Quoting Daniel P. Berrange (2017-06-29 03:33:19)
> On Wed, Jun 28, 2017 at 07:24:55PM -0500, Michael Roth wrote:
> > Hi everyone. Hoping to get some feedback on this approach, or some
> > alternatives proposed below, to the following issue:
> >
> > Currently libvirt immediately attempts to rebind a managed device back to the
> > host driver when it receives a DEVICE_DELETED event from QEMU. This is
> > problematic for 2 reasons:
> >
> > 1) If multiple devices from a group are attached to a guest, this can move
> > the group into a "non-viable" state where some devices are assigned to
> > the host and some to the guest.
> >
> > 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase
> > where additional cleanup occurs. In most cases libvirt can ignore this
> > cleanup, but in the case of VFIO devices this is where closing of a VFIO
> > group FD occurs, and failing to wait before rebinding the device to the
> > host driver can result in unexpected behavior. In the case of powernv
> > hosts at least, this can lead to a host driver crashing due to the default
> > DMA windows not having been fully-restored yet. The window between this is
> > and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've
> > seen host dumps with Mellanox CX4 VFs being rebound to host driver during
> > this period (on powernv hosts).
>
> Why on earth does QEMU's device finalization take 6 seconds to complete.
> That feels very broken to me, unless QEMU is not being schedled due to
> host being overcomitted. If that's not the case, then we have a bug to
> investigate in QEMU to find out why cleanup is delayed so long.
In this particular case QEMU starts finalization almost immediately
after the DEVICE_DELETED, but it looks like most of the time between that and
closing of the group FD is spent in the kernel. Here's what it looks
like from QEMU with vfio*/monitor* traces enabled (in this case
unplugging a Mellanox CX3 PF):
# device_del called
...
# vfio device's device_unparent() called:
# unrealize->exit->vfio_exitfn called:
61948@1498759308.951038:vfio_intx_disable (0002:01:00.0)
61948@1498759308.953657:vfio_region_exit Device 0002:01:00.0, region 0
61948@1498759308.954532:vfio_region_exit Device 0002:01:00.0, region 2
# unrealize->exit->vfio_exitfn returns, DEVICE_DELETED sent:
61948@1498759308.954633:monitor_protocol_event_queue event=9 data=0x3fff6c508690 rate=0
61948@1498759308.954669:monitor_protocol_event_emit event=9 data=0x3fff6c508690
# last unref of vfio device, vfio_instance_finalize() called:
61948@1498759308.955660:vfio_region_finalize Device 0002:01:00.0, region 0
61948@1498759308.955742:vfio_region_finalize Device 0002:01:00.0, region 2
61948@1498759308.955751:vfio_put_base_device close vdev->fd=30
# close(VFIODevice.fd) <- 5 SECOND DELAY
61948@1498759313.140515:vfio_put_base_device_completed close completed vdev->fd=30
61948@1498759313.181124:vfio_disconnect_container close container->fd=102
61948@1498759313.181152:vfio_put_group close group->fd=101
# close(VFIOGroup.fd)
61948@1498759313.181157:vfio_put_group_completed close completed group->fd=101
# vfio_instance_finalize() returns
# vfio device's device_unparent() returns
I poked around in the VFIO group close path and figured restoring ownership
of IOMMU to the host via vfio_iommu_driver_ops.release() (via
close(groupfd) was where all the time was spent, but didn't expect it to
be the close(VFIODevice.fd). Maybe Alexey/Alex have a better idea. I'll
look into it more as well.
But suffice to say there's not much QEMU can do about the delay other
than moving deferring the DEVICE_DELETED event or adding a later-stage
event.
>
>
> From libvirt's POV, we consider 'DEVICE_DELETED' as meaning both that the
> frontend has gone *and* the corresponding backend has gone. Aside from
> cleaning the VFIO group, we use this as a trigger for all other device
> related cleanup like SELinux labelling, cgroup device ACLs, etc. If the
> backend is not guaranteed to be closed in QEMU when this emit is emitted
> then either QEMU needs to delay the event until it is really cleaned up,
> or QEMU needs to add a further event to emit when the backend is clean.
>
>
> > Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver
> > until all the devices in the group have been detached, at which point all
> > the hostdevs are rebound as a group. Until that point, the devices are traced
> > by the drvManager's inactiveList in a similar manner to hostdevs that are
> > assigned to VFIO via the nodedev-detach interface.
> >
> > Patch 5 addresses 2) by adding an additional check that, when the last device
> > from a group is detached, polls /proc for open FDs referencing the VFIO group
> > path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we
> > time out, we abandon rebinding the hostdevs back to the host.
>
> That is just gross - it is tieing libvirt to details of the QEMU internal
> implementation. I really don't think we should be doing that. So NACK to
> this from my POV.
>
> > There are a couple alternatives to Patch 5 that might be worth considering:
> >
> > a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of
> > DEVICE_DELETED. Paired with patches 1-4 this would let us drop patch 5 in
> > favor of minimal changes to libvirt's event handlers.
> >
> > The downsides are:
> > - that we'd incur some latency for all device-detach calls, but it's not
> > apparent to me whether this delay is significant for anything outside
> > of VFIO.
> > - there may be cases where finalization after DEVICE_DELETE/unparent are
> > is not guaranteed, and I'm not sure QOM would encourage such
> > expectations even if that's currently the case.
> >
> > b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most
> > direct solution. With this we could completely separate out the handling
> > of rebinding to host driver based on receival of this event.
> >
> > The downsides are:
> > - this would only work for newer versions of QEMU, though we could use
> > the poll-wait in patch 5 as a fallback.
> > - synchronizing sync/async device-detach threads with sync/async
> > handlers for this would be a bit hairy, but I have a WIP in progress
> > that seems *fairly reasonable*
> >
> > c) Take the approach in Patch 5, either as a precursor to implementing b) or
> > something else, or just sticking with that for now.
> >
> > d) ???
>
> Fix DEVICE_DELETE so its only emitted when the backend associated with
> the device is fully cleaned up.
Need to explore same concerns I had WRT to DEVICE_FINALIZED above, but
assuming those aren't an issue this would indeed makes things even
simpler. Will look into this more.
Would patch 5 be out of the question even as a fallback for downlevel
QEMUs? Or is that scenario too unlikely to warrant it?
Thanks!
>
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
On Thu, 29 Jun 2017 13:22:44 -0500 Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > Quoting Daniel P. Berrange (2017-06-29 03:33:19) > > On Wed, Jun 28, 2017 at 07:24:55PM -0500, Michael Roth wrote: > > > Hi everyone. Hoping to get some feedback on this approach, or some > > > alternatives proposed below, to the following issue: > > > > > > Currently libvirt immediately attempts to rebind a managed device back to the > > > host driver when it receives a DEVICE_DELETED event from QEMU. This is > > > problematic for 2 reasons: > > > > > > 1) If multiple devices from a group are attached to a guest, this can move > > > the group into a "non-viable" state where some devices are assigned to > > > the host and some to the guest. > > > > > > 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase > > > where additional cleanup occurs. In most cases libvirt can ignore this > > > cleanup, but in the case of VFIO devices this is where closing of a VFIO > > > group FD occurs, and failing to wait before rebinding the device to the > > > host driver can result in unexpected behavior. In the case of powernv > > > hosts at least, this can lead to a host driver crashing due to the default > > > DMA windows not having been fully-restored yet. The window between this is > > > and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've > > > seen host dumps with Mellanox CX4 VFs being rebound to host driver during > > > this period (on powernv hosts). > > > > Why on earth does QEMU's device finalization take 6 seconds to complete. > > That feels very broken to me, unless QEMU is not being schedled due to > > host being overcomitted. If that's not the case, then we have a bug to > > investigate in QEMU to find out why cleanup is delayed so long. > > In this particular case QEMU starts finalization almost immediately > after the DEVICE_DELETED, but it looks like most of the time between that and > closing of the group FD is spent in the kernel. Here's what it looks > like from QEMU with vfio*/monitor* traces enabled (in this case > unplugging a Mellanox CX3 PF): > > # device_del called > ... > # vfio device's device_unparent() called: > # unrealize->exit->vfio_exitfn called: > 61948@1498759308.951038:vfio_intx_disable (0002:01:00.0) > 61948@1498759308.953657:vfio_region_exit Device 0002:01:00.0, region 0 > 61948@1498759308.954532:vfio_region_exit Device 0002:01:00.0, region 2 > # unrealize->exit->vfio_exitfn returns, DEVICE_DELETED sent: > 61948@1498759308.954633:monitor_protocol_event_queue event=9 data=0x3fff6c508690 rate=0 > 61948@1498759308.954669:monitor_protocol_event_emit event=9 data=0x3fff6c508690 > # last unref of vfio device, vfio_instance_finalize() called: > 61948@1498759308.955660:vfio_region_finalize Device 0002:01:00.0, region 0 > 61948@1498759308.955742:vfio_region_finalize Device 0002:01:00.0, region 2 > 61948@1498759308.955751:vfio_put_base_device close vdev->fd=30 > > # close(VFIODevice.fd) <- 5 SECOND DELAY > > 61948@1498759313.140515:vfio_put_base_device_completed close completed vdev->fd=30 > 61948@1498759313.181124:vfio_disconnect_container close container->fd=102 > 61948@1498759313.181152:vfio_put_group close group->fd=101 > # close(VFIOGroup.fd) > 61948@1498759313.181157:vfio_put_group_completed close completed group->fd=101 > # vfio_instance_finalize() returns > # vfio device's device_unparent() returns > > I poked around in the VFIO group close path and figured restoring ownership > of IOMMU to the host via vfio_iommu_driver_ops.release() (via > close(groupfd) was where all the time was spent, but didn't expect it to > be the close(VFIODevice.fd). Maybe Alexey/Alex have a better idea. I'll > look into it more as well. > > But suffice to say there's not much QEMU can do about the delay other > than moving deferring the DEVICE_DELETED event or adding a later-stage > event. The device close needs to do a device reset and attempt to restore the state of the device, plus whatever that EEH code is doing. I would have expected the group close to teardown the IOMMU, but maybe I'm forgetting some subtlety, in either case it's going to happen as part of the finalize and it's going to take some time. Thanks, Alex > > From libvirt's POV, we consider 'DEVICE_DELETED' as meaning both that the > > frontend has gone *and* the corresponding backend has gone. Aside from > > cleaning the VFIO group, we use this as a trigger for all other device > > related cleanup like SELinux labelling, cgroup device ACLs, etc. If the > > backend is not guaranteed to be closed in QEMU when this emit is emitted > > then either QEMU needs to delay the event until it is really cleaned up, > > or QEMU needs to add a further event to emit when the backend is clean. > > > > > > > Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver > > > until all the devices in the group have been detached, at which point all > > > the hostdevs are rebound as a group. Until that point, the devices are traced > > > by the drvManager's inactiveList in a similar manner to hostdevs that are > > > assigned to VFIO via the nodedev-detach interface. > > > > > > Patch 5 addresses 2) by adding an additional check that, when the last device > > > from a group is detached, polls /proc for open FDs referencing the VFIO group > > > path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we > > > time out, we abandon rebinding the hostdevs back to the host. > > > > That is just gross - it is tieing libvirt to details of the QEMU internal > > implementation. I really don't think we should be doing that. So NACK to > > this from my POV. > > > > > There are a couple alternatives to Patch 5 that might be worth considering: > > > > > > a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of > > > DEVICE_DELETED. Paired with patches 1-4 this would let us drop patch 5 in > > > favor of minimal changes to libvirt's event handlers. > > > > > > The downsides are: > > > - that we'd incur some latency for all device-detach calls, but it's not > > > apparent to me whether this delay is significant for anything outside > > > of VFIO. > > > - there may be cases where finalization after DEVICE_DELETE/unparent are > > > is not guaranteed, and I'm not sure QOM would encourage such > > > expectations even if that's currently the case. > > > > > > b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most > > > direct solution. With this we could completely separate out the handling > > > of rebinding to host driver based on receival of this event. > > > > > > The downsides are: > > > - this would only work for newer versions of QEMU, though we could use > > > the poll-wait in patch 5 as a fallback. > > > - synchronizing sync/async device-detach threads with sync/async > > > handlers for this would be a bit hairy, but I have a WIP in progress > > > that seems *fairly reasonable* > > > > > > c) Take the approach in Patch 5, either as a precursor to implementing b) or > > > something else, or just sticking with that for now. > > > > > > d) ??? > > > > Fix DEVICE_DELETE so its only emitted when the backend associated with > > the device is fully cleaned up. > > Need to explore same concerns I had WRT to DEVICE_FINALIZED above, but > assuming those aren't an issue this would indeed makes things even > simpler. Will look into this more. > > Would patch 5 be out of the question even as a fallback for downlevel > QEMUs? Or is that scenario too unlikely to warrant it? > > Thanks! > > > > > > > Regards, > > Daniel > > -- > > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > > |: https://libvirt.org -o- https://fstop138.berrange.com :| > > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| > > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 29 Jun 2017 09:33:19 +0100 "Daniel P. Berrange" <berrange@redhat.com> wrote: > On Wed, Jun 28, 2017 at 07:24:55PM -0500, Michael Roth wrote: > > Hi everyone. Hoping to get some feedback on this approach, or some > > alternatives proposed below, to the following issue: > > > > Currently libvirt immediately attempts to rebind a managed device back to the > > host driver when it receives a DEVICE_DELETED event from QEMU. This is > > problematic for 2 reasons: > > > > 1) If multiple devices from a group are attached to a guest, this can move > > the group into a "non-viable" state where some devices are assigned to > > the host and some to the guest. > > > > 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase > > where additional cleanup occurs. In most cases libvirt can ignore this > > cleanup, but in the case of VFIO devices this is where closing of a VFIO > > group FD occurs, and failing to wait before rebinding the device to the > > host driver can result in unexpected behavior. In the case of powernv > > hosts at least, this can lead to a host driver crashing due to the default > > DMA windows not having been fully-restored yet. The window between this is > > and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've > > seen host dumps with Mellanox CX4 VFs being rebound to host driver during > > this period (on powernv hosts). I've been trying to tackle this from the kernel side too, currently Linux's driver model really neither allows vfio bus drivers to nak unbinding a device from an in-use group nor nak binding a device from an in-use group to an incompatible driver. The issue you identify in QEMU/libvirt exacerbates the problem as QEMU has not yet finalized the device/group references before libvirt tries to unbind the device from the vfio bus driver and attach it to a host driver. I'd love to solve this from both sides by allowing the kernel to prevent driver binds that we'd consider compromising and also introduce a bit of patience in the QEMU/libvirt path to avoid the kernel needing to impose that driver blocking. > Why on earth does QEMU's device finalization take 6 seconds to complete. > That feels very broken to me, unless QEMU is not being schedled due to > host being overcomitted. If that's not the case, then we have a bug to > investigate in QEMU to find out why cleanup is delayed so long. I wouldn't necessarily jump to the conclusion that this is a bug, if it's relating to tearing down the IOMMU mappings for the device, gigs of mappings can take non-trivial time. Is that the scenario here? Is that 6s somehow proportional to guest memory size? > From libvirt's POV, we consider 'DEVICE_DELETED' as meaning both that the > frontend has gone *and* the corresponding backend has gone. Aside from > cleaning the VFIO group, we use this as a trigger for all other device > related cleanup like SELinux labelling, cgroup device ACLs, etc. If the > backend is not guaranteed to be closed in QEMU when this emit is emitted > then either QEMU needs to delay the event until it is really cleaned up, > or QEMU needs to add a further event to emit when the backend is clean. Clearly libvirt and QEMU's idea of what DEVICE_DELETED means don't align. > > Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver > > until all the devices in the group have been detached, at which point all > > the hostdevs are rebound as a group. Until that point, the devices are traced > > by the drvManager's inactiveList in a similar manner to hostdevs that are > > assigned to VFIO via the nodedev-detach interface. There are certainly some benefits to group-awareness here, currently an admin user like libvirt can trigger a BUG_ON by trying to bind a device back to a host driver when a group is still in use, at best we might improve that to rejecting the compromising bind. > > Patch 5 addresses 2) by adding an additional check that, when the last device > > from a group is detached, polls /proc for open FDs referencing the VFIO group > > path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we > > time out, we abandon rebinding the hostdevs back to the host. > > That is just gross - it is tieing libvirt to details of the QEMU internal > implementation. I really don't think we should be doing that. So NACK to > this from my POV. It seems a little silly for QEMU to emit the event while it's still in use, clearly emitting the event at the right point would negate any need for snooping around in proc. > > There are a couple alternatives to Patch 5 that might be worth considering: > > > > a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of > > DEVICE_DELETED. Paired with patches 1-4 this would let us drop patch 5 in > > favor of minimal changes to libvirt's event handlers. > > > > The downsides are: > > - that we'd incur some latency for all device-detach calls, but it's not > > apparent to me whether this delay is significant for anything outside > > of VFIO. > > - there may be cases where finalization after DEVICE_DELETE/unparent are > > is not guaranteed, and I'm not sure QOM would encourage such > > expectations even if that's currently the case. > > > > b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most > > direct solution. With this we could completely separate out the handling > > of rebinding to host driver based on receival of this event. > > > > The downsides are: > > - this would only work for newer versions of QEMU, though we could use > > the poll-wait in patch 5 as a fallback. > > - synchronizing sync/async device-detach threads with sync/async > > handlers for this would be a bit hairy, but I have a WIP in progress > > that seems *fairly reasonable* > > > > c) Take the approach in Patch 5, either as a precursor to implementing b) or > > something else, or just sticking with that for now. > > > > d) ??? > > Fix DEVICE_DELETE so its only emitted when the backend associated with > the device is fully cleaned up. Adding a FINALIZE seems to require a two-step fix, fix QEMU then fix libvirt, whereas moving DELETE to the correct location automatically fixes the behavior with existing libvirt. I don't know that a GROUP_DELETED makes much sense, libvirt can know about groups on its own and it just leads to a vfio specific path. Thanks, Alex -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Quoting Alex Williamson (2017-06-29 14:28:11) > On Thu, 29 Jun 2017 09:33:19 +0100 > "Daniel P. Berrange" <berrange@redhat.com> wrote: > > > On Wed, Jun 28, 2017 at 07:24:55PM -0500, Michael Roth wrote: > > > Hi everyone. Hoping to get some feedback on this approach, or some > > > alternatives proposed below, to the following issue: > > > > > > Currently libvirt immediately attempts to rebind a managed device back to the > > > host driver when it receives a DEVICE_DELETED event from QEMU. This is > > > problematic for 2 reasons: > > > > > > 1) If multiple devices from a group are attached to a guest, this can move > > > the group into a "non-viable" state where some devices are assigned to > > > the host and some to the guest. > > > > > > 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase > > > where additional cleanup occurs. In most cases libvirt can ignore this > > > cleanup, but in the case of VFIO devices this is where closing of a VFIO > > > group FD occurs, and failing to wait before rebinding the device to the > > > host driver can result in unexpected behavior. In the case of powernv > > > hosts at least, this can lead to a host driver crashing due to the default > > > DMA windows not having been fully-restored yet. The window between this is > > > and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've > > > seen host dumps with Mellanox CX4 VFs being rebound to host driver during > > > this period (on powernv hosts). > > I've been trying to tackle this from the kernel side too, currently > Linux's driver model really neither allows vfio bus drivers to nak > unbinding a device from an in-use group nor nak binding a device > from an in-use group to an incompatible driver. The issue you identify > in QEMU/libvirt exacerbates the problem as QEMU has not yet finalized > the device/group references before libvirt tries to unbind the device > from the vfio bus driver and attach it to a host driver. I'd love to > solve this from both sides by allowing the kernel to prevent driver > binds that we'd consider compromising and also introduce a bit of > patience in the QEMU/libvirt path to avoid the kernel needing to impose > that driver blocking. Perfect, I'd meant to ask about this and it skipped my mind. I had some concerns about the fact that even with these qemu/libvirt changes in place we could still trigger these sorts of crashes by issuing such a rebind outside of libvirt, so good to know that aspect is being looked at as well and that the 2 aren't in conflict. > > > Why on earth does QEMU's device finalization take 6 seconds to complete. > > That feels very broken to me, unless QEMU is not being schedled due to > > host being overcomitted. If that's not the case, then we have a bug to > > investigate in QEMU to find out why cleanup is delayed so long. > > I wouldn't necessarily jump to the conclusion that this is a bug, if > it's relating to tearing down the IOMMU mappings for the device, gigs > of mappings can take non-trivial time. Is that the scenario here? Is > that 6s somehow proportional to guest memory size? I think you're right that the teardown is via group close. Since the traces seem to pin it on the vfio device FD close, the device reset you mentioned is probably what's going on here, but I'll try to confirm. > > > From libvirt's POV, we consider 'DEVICE_DELETED' as meaning both that the > > frontend has gone *and* the corresponding backend has gone. Aside from > > cleaning the VFIO group, we use this as a trigger for all other device > > related cleanup like SELinux labelling, cgroup device ACLs, etc. If the > > backend is not guaranteed to be closed in QEMU when this emit is emitted > > then either QEMU needs to delay the event until it is really cleaned up, > > or QEMU needs to add a further event to emit when the backend is clean. > > Clearly libvirt and QEMU's idea of what DEVICE_DELETED means don't > align. > > > > Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver > > > until all the devices in the group have been detached, at which point all > > > the hostdevs are rebound as a group. Until that point, the devices are traced > > > by the drvManager's inactiveList in a similar manner to hostdevs that are > > > assigned to VFIO via the nodedev-detach interface. > > There are certainly some benefits to group-awareness here, currently > an admin user like libvirt can trigger a BUG_ON by trying to bind a > device back to a host driver when a group is still in use, at best we > might improve that to rejecting the compromising bind. > > > > Patch 5 addresses 2) by adding an additional check that, when the last device > > > from a group is detached, polls /proc for open FDs referencing the VFIO group > > > path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we > > > time out, we abandon rebinding the hostdevs back to the host. > > > > That is just gross - it is tieing libvirt to details of the QEMU internal > > implementation. I really don't think we should be doing that. So NACK to > > this from my POV. > > It seems a little silly for QEMU to emit the event while it's still in > use, clearly emitting the event at the right point would negate any > need for snooping around in proc. > > > > There are a couple alternatives to Patch 5 that might be worth considering: > > > > > > a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of > > > DEVICE_DELETED. Paired with patches 1-4 this would let us drop patch 5 in > > > favor of minimal changes to libvirt's event handlers. > > > > > > The downsides are: > > > - that we'd incur some latency for all device-detach calls, but it's not > > > apparent to me whether this delay is significant for anything outside > > > of VFIO. > > > - there may be cases where finalization after DEVICE_DELETE/unparent are > > > is not guaranteed, and I'm not sure QOM would encourage such > > > expectations even if that's currently the case. > > > > > > b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most > > > direct solution. With this we could completely separate out the handling > > > of rebinding to host driver based on receival of this event. > > > > > > The downsides are: > > > - this would only work for newer versions of QEMU, though we could use > > > the poll-wait in patch 5 as a fallback. > > > - synchronizing sync/async device-detach threads with sync/async > > > handlers for this would be a bit hairy, but I have a WIP in progress > > > that seems *fairly reasonable* > > > > > > c) Take the approach in Patch 5, either as a precursor to implementing b) or > > > something else, or just sticking with that for now. > > > > > > d) ??? > > > > Fix DEVICE_DELETE so its only emitted when the backend associated with > > the device is fully cleaned up. > > Adding a FINALIZE seems to require a two-step fix, fix QEMU then fix > libvirt, whereas moving DELETE to the correct location automatically > fixes the behavior with existing libvirt. I don't know that a > GROUP_DELETED makes much sense, libvirt can know about groups on its > own and it just leads to a vfio specific path. Thanks, Ok, seems like there's a consensus there. I'll report back if there's any obvious showstopper with this, but otherwise I'll work on a patch to defer the DEVICE_DELETED, and drop patch 5 from the libvirt side of things. Thanks! > > Alex > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 06/28/2017 08:24 PM, Michael Roth wrote: > Hi everyone. Hoping to get some feedback on this approach, or some > alternatives proposed below, to the following issue: > > Currently libvirt immediately attempts to rebind a managed device back to the > host driver when it receives a DEVICE_DELETED event from QEMU. This is > problematic for 2 reasons: > > 1) If multiple devices from a group are attached to a guest, this can move > the group into a "non-viable" state where some devices are assigned to > the host and some to the guest. Since we don't support hotplug with managed='yes' of individual (or multiple) functions of a multifunction host device, I don't know that it's very useful to support hot *un*plug of it - it would only be useful if the multi-function device were present in the guest when it was started, and then was hot-unplugged later. And this is all a lot of extra complexity, though, so it would be useful to know what are the scenarios where it would actually be used (i.e. is this a legitimate need, or just an interesting exercise?) > > 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase > where additional cleanup occurs. In most cases libvirt can ignore this > cleanup, but in the case of VFIO devices this is where closing of a VFIO > group FD occurs, and failing to wait before rebinding the device to the > host driver can result in unexpected behavior. In the case of powernv > hosts at least, this can lead to a host driver crashing due to the default > DMA windows not having been fully-restored yet. The window between this is > and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've > seen host dumps with Mellanox CX4 VFs being rebound to host driver during > this period (on powernv hosts). I agree with Dan that the situation described here should be considered a qemu bug - according to my understanding (from back at the time DEVICE_DELETED was added to qemu (I think at libvirt's request) qemu should never emit the DEVICE_DELETED event until *everything* related to the device is finished - that was the whole point of adding the event in the first palce. Covering up this bug with a bunch of extra libvirt complexity is just creating the potential for even more bugs in the more complex code. > > Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver > until all the devices in the group have been detached, at which point all > the hostdevs are rebound as a group. Until that point, the devices are traced > by the drvManager's inactiveList in a similar manner to hostdevs that are > assigned to VFIO via the nodedev-detach interface. What happens if libvirtd is restarted during this period? Is the inactiveList rebuilt with all the info necessary to assure that the nodedev-reattach does/doesn't happen (as appropriate) for all devices? > > Patch 5 addresses 2) by adding an additional check that, when the last device > from a group is detached, polls /proc for open FDs referencing the VFIO group > path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we > time out, we abandon rebinding the hostdevs back to the host. > > There are a couple alternatives to Patch 5 that might be worth considering: > > a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of > DEVICE_DELETED. Is the "device is *almost* completely deleted" event (current DEVIE_DELETED) really something that anyone wants/needs to know about? Or is the only useful event just the one that you're calling DEVICE_FINALIZED? If the latter, then I think it would be better to just change when DEVICE_DELETED is emitted. Paired with patches 1-4 this would let us drop patch 5 in > favor of minimal changes to libvirt's event handlers. > > The downsides are: > - that we'd incur some latency for all device-detach calls, but it's not > apparent to me whether this delay is significant for anything outside > of VFIO. > - there may be cases where finalization after DEVICE_DELETE/unparent are > is not guaranteed, and I'm not sure QOM would encourage such > expectations even if that's currently the case. > > b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most > direct solution. With this we could completely separate out the handling > of rebinding to host driver based on receival of this event. > > The downsides are: > - this would only work for newer versions of QEMU, though we could use > the poll-wait in patch 5 as a fallback. I don't think we should add such a complex patch as a fallback to support older versions of qemu that don't have the bug fixed. Instead, just tell people to upgrade qemu (after all, they're going to need to update *something* (either libvirt or qemu); no need to update libvirt just in order to avoid updating qemu). > - synchronizing sync/async device-detach threads with sync/async > handlers for this would be a bit hairy, but I have a WIP in progress > that seems *fairly reasonable* > > c) Take the approach in Patch 5, either as a precursor to implementing b) or > something else, or just sticking with that for now. > > d) ??? > > Personally I'm leaning toward c), but I'm wondering if that's "good enough" > for now, or if we should pursue something more robust from the start, or > perhaps something else entirely? > > Any feedback is greatly appreciated! > > src/libvirt_private.syms | 5 ++ > src/qemu/qemu_hostdev.c | 16 +++++ > src/qemu/qemu_hostdev.h | 4 ++ > src/qemu/qemu_hotplug.c | 56 ++++++++++++++---- > src/util/virfile.c | 52 +++++++++++++++++ > src/util/virfile.h | 1 + > src/util/virhostdev.c | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++----- > src/util/virhostdev.h | 16 +++++ > src/util/virpci.c | 69 ++++++++++++++++++---- > src/util/virpci.h | 4 ++ > 10 files changed, 360 insertions(+), 36 deletions(-) > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 29 Jun 2017 14:50:15 -0400 Laine Stump <laine@redhat.com> wrote: > On 06/28/2017 08:24 PM, Michael Roth wrote: > > Hi everyone. Hoping to get some feedback on this approach, or some > > alternatives proposed below, to the following issue: > > > > Currently libvirt immediately attempts to rebind a managed device back to the > > host driver when it receives a DEVICE_DELETED event from QEMU. This is > > problematic for 2 reasons: > > > > 1) If multiple devices from a group are attached to a guest, this can move > > the group into a "non-viable" state where some devices are assigned to > > the host and some to the guest. > > Since we don't support hotplug with managed='yes' of individual (or > multiple) functions of a multifunction host device, I don't know that > it's very useful to support hot *un*plug of it - it would only be useful > if the multi-function device were present in the guest when it was > started, and then was hot-unplugged later. And this is all a lot of > extra complexity, though, so it would be useful to know what are the > scenarios where it would actually be used (i.e. is this a legitimate > need, or just an interesting exercise?) This doesn't make sense to me, since when do we not support hotplug with managed='yes' and how is it prevented? Also, let's just not talk about multifunction, a multifunction device does not imply a shared group, nor does a shared group imply multifunction. So is it hotplug of a device which is in a shared group that is not supported, and if so how? I think libvirt tries to do the hot-add, but it hits the non-viable group when it gives it to QEMU. On hot-remove, I'm pretty sure libvirt just lets the host crash into the ground by re-binding the device to the host driver. If we don't want to support it, that's one thing, but the current model is more just neglectful than unsupported. > > 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase > > where additional cleanup occurs. In most cases libvirt can ignore this > > cleanup, but in the case of VFIO devices this is where closing of a VFIO > > group FD occurs, and failing to wait before rebinding the device to the > > host driver can result in unexpected behavior. In the case of powernv > > hosts at least, this can lead to a host driver crashing due to the default > > DMA windows not having been fully-restored yet. The window between this is > > and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've > > seen host dumps with Mellanox CX4 VFs being rebound to host driver during > > this period (on powernv hosts). > > I agree with Dan that the situation described here should be considered > a qemu bug - according to my understanding (from back at the time > DEVICE_DELETED was added to qemu (I think at libvirt's request) qemu > should never emit the DEVICE_DELETED event until *everything* related to > the device is finished - that was the whole point of adding the event in > the first palce. Covering up this bug with a bunch of extra libvirt > complexity is just creating the potential for even more bugs in the more > complex code. Agree, but ISTR not everyone thinks that way. I don't remember the opposing viewpoint though. Thanks, Alex -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 06/29/2017 03:44 PM, Alex Williamson wrote: > On Thu, 29 Jun 2017 14:50:15 -0400 > Laine Stump <laine@redhat.com> wrote: > >> On 06/28/2017 08:24 PM, Michael Roth wrote: >>> Hi everyone. Hoping to get some feedback on this approach, or some >>> alternatives proposed below, to the following issue: >>> >>> Currently libvirt immediately attempts to rebind a managed device back to the >>> host driver when it receives a DEVICE_DELETED event from QEMU. This is >>> problematic for 2 reasons: >>> >>> 1) If multiple devices from a group are attached to a guest, this can move >>> the group into a "non-viable" state where some devices are assigned to >>> the host and some to the guest. >> >> Since we don't support hotplug with managed='yes' of individual (or >> multiple) functions of a multifunction host device, I don't know that >> it's very useful to support hot *un*plug of it - it would only be useful >> if the multi-function device were present in the guest when it was >> started, and then was hot-unplugged later. And this is all a lot of >> extra complexity, though, so it would be useful to know what are the >> scenarios where it would actually be used (i.e. is this a legitimate >> need, or just an interesting exercise?) > > This doesn't make sense to me, since when do we not support hotplug > with managed='yes' and how is it prevented? Wait a minute. I wasn't thinking straight, got sidetracked into a different problem, and completely screwed up what I was trying to say. So basically forget everything in that paragraph :-/ What I started out to say was simply that we didn't support managed='yes' hotplugging of a single device that's in an iommu group containing multiple devices (and that led me to wander into the topic of multifunction devices, which as you say is really unrelated). We don't do anything specifically to prevent it, but as you say, it will fail when it gets to qemu. At one time someone talked about having libvirt automatically detach the other devices in the same group at times like this, but that was rightfully detected as ridiculous (it may have been me talking to myself, and I realized the stupidity of the idea before I managed to repeat it to anyone else). > Also, let's just not talk > about multifunction, a multifunction device does not imply a shared > group, nor does a shared group imply multifunction. Yeah, that was me being distracted while thinking about the situation and mistakenly conflating two different problems. > So is it hotplug > of a device which is in a shared group that is not supported, and if so > how? "not supported" != "actively prevented", it just means "there's no way to successfully do that" > I think libvirt tries to do the hot-add, but it hits the > non-viable group when it gives it to QEMU. Correct. > On hot-remove, I'm pretty > sure libvirt just lets the host crash into the ground by re-binding the > device to the host driver. I haven't tried, but you're likely right. > If we don't want to support it, that's one > thing, but the current model is more just neglectful than unsupported. Semantics. Potato, Potahto. The end result is "it doesn't work". If we're going to try to fix one, we should try to fix the other too. > > >>> 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase >>> where additional cleanup occurs. In most cases libvirt can ignore this >>> cleanup, but in the case of VFIO devices this is where closing of a VFIO >>> group FD occurs, and failing to wait before rebinding the device to the >>> host driver can result in unexpected behavior. In the case of powernv >>> hosts at least, this can lead to a host driver crashing due to the default >>> DMA windows not having been fully-restored yet. The window between this is >>> and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've >>> seen host dumps with Mellanox CX4 VFs being rebound to host driver during >>> this period (on powernv hosts). >> >> I agree with Dan that the situation described here should be considered >> a qemu bug - according to my understanding (from back at the time >> DEVICE_DELETED was added to qemu (I think at libvirt's request) qemu >> should never emit the DEVICE_DELETED event until *everything* related to >> the device is finished - that was the whole point of adding the event in >> the first palce. Covering up this bug with a bunch of extra libvirt >> complexity is just creating the potential for even more bugs in the more >> complex code. > > Agree, but ISTR not everyone thinks that way. I don't remember the > opposing viewpoint though. Possible. I only retain about 1% of what goes by the screen, and even most of that is gone within 6 months. I thought I recalled that there was no DEVICE_DELETED event before libvirt requested it, so it would make sense for it to behave according to what libvirt needs (assuming nobody else is using it). That may be a narcissistic and incorrect view though.
Quoting Laine Stump (2017-06-29 13:50:15) > On 06/28/2017 08:24 PM, Michael Roth wrote: > > Hi everyone. Hoping to get some feedback on this approach, or some > > alternatives proposed below, to the following issue: > > > > Currently libvirt immediately attempts to rebind a managed device back to the > > host driver when it receives a DEVICE_DELETED event from QEMU. This is > > problematic for 2 reasons: > > > > 1) If multiple devices from a group are attached to a guest, this can move > > the group into a "non-viable" state where some devices are assigned to > > the host and some to the guest. > > Since we don't support hotplug with managed='yes' of individual (or > multiple) functions of a multifunction host device, I don't know that > it's very useful to support hot *un*plug of it - it would only be useful > if the multi-function device were present in the guest when it was > started, and then was hot-unplugged later. And this is all a lot of > extra complexity, though, so it would be useful to know what are the > scenarios where it would actually be used (i.e. is this a legitimate > need, or just an interesting exercise?) > > > > > 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase > > where additional cleanup occurs. In most cases libvirt can ignore this > > cleanup, but in the case of VFIO devices this is where closing of a VFIO > > group FD occurs, and failing to wait before rebinding the device to the > > host driver can result in unexpected behavior. In the case of powernv > > hosts at least, this can lead to a host driver crashing due to the default > > DMA windows not having been fully-restored yet. The window between this is > > and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've > > seen host dumps with Mellanox CX4 VFs being rebound to host driver during > > this period (on powernv hosts). > > I agree with Dan that the situation described here should be considered > a qemu bug - according to my understanding (from back at the time > DEVICE_DELETED was added to qemu (I think at libvirt's request) qemu > should never emit the DEVICE_DELETED event until *everything* related to > the device is finished - that was the whole point of adding the event in > the first palce. Covering up this bug with a bunch of extra libvirt > complexity is just creating the potential for even more bugs in the more > complex code. > > > > > Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver > > until all the devices in the group have been detached, at which point all > > the hostdevs are rebound as a group. Until that point, the devices are traced > > by the drvManager's inactiveList in a similar manner to hostdevs that are > > assigned to VFIO via the nodedev-detach interface. > > What happens if libvirtd is restarted during this period? Is the > inactiveList rebuilt with all the info necessary to assure that the > nodedev-reattach does/doesn't happen (as appropriate) for all devices? Hmm, good question. The Unbindable() check only needs to know that nothing in the activeList belongs to the group we're checking, and that list at least seems to get rebuilt appropriately on restart. But the Unbind() relies on inactiveList and the behavior there is what nodedev-detach currently does, which is to add it to inactive list while libvirtd is running, and then just forget about it when libvirtd restarts. For nodedev-detach it's fine since virHostdevPreparePCIDevices() re-adds it as needed in the device-attach path. But yah, for this purpose it ends up losing track of hostdevs that are still pending rebind to the host, and that means some devices may not get rebound at the appropriate time if there was a libvirtd restart. Unlike with device-attach, we can't just re-add it on-demand because we actually need to know whether or not it was previously in the list. So I think we'd need to add some persistent state to track this. Will look into adding handling for that. > > > > > > Patch 5 addresses 2) by adding an additional check that, when the last device > > from a group is detached, polls /proc for open FDs referencing the VFIO group > > path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we > > time out, we abandon rebinding the hostdevs back to the host. > > > > There are a couple alternatives to Patch 5 that might be worth considering: > > > > a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of > > DEVICE_DELETED. > > Is the "device is *almost* completely deleted" event (current > DEVIE_DELETED) really something that anyone wants/needs to know about? > Or is the only useful event just the one that you're calling > DEVICE_FINALIZED? If the latter, then I think it would be better to just > change when DEVICE_DELETED is emitted. > > Paired with patches 1-4 this would let us drop patch 5 in > > favor of minimal changes to libvirt's event handlers. > > > > The downsides are: > > - that we'd incur some latency for all device-detach calls, but it's not > > apparent to me whether this delay is significant for anything outside > > of VFIO. > > - there may be cases where finalization after DEVICE_DELETE/unparent are > > is not guaranteed, and I'm not sure QOM would encourage such > > expectations even if that's currently the case. > > > > b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most > > direct solution. With this we could completely separate out the handling > > of rebinding to host driver based on receival of this event. > > > > The downsides are: > > - this would only work for newer versions of QEMU, though we could use > > the poll-wait in patch 5 as a fallback. > > I don't think we should add such a complex patch as a fallback to > support older versions of qemu that don't have the bug fixed. Instead, > just tell people to upgrade qemu (after all, they're going to need to > update *something* (either libvirt or qemu); no need to update libvirt > just in order to avoid updating qemu). Ok, makes sense. Thanks! > > > > - synchronizing sync/async device-detach threads with sync/async > > handlers for this would be a bit hairy, but I have a WIP in progress > > that seems *fairly reasonable* > > > > c) Take the approach in Patch 5, either as a precursor to implementing b) or > > something else, or just sticking with that for now. > > > > d) ??? > > > > Personally I'm leaning toward c), but I'm wondering if that's "good enough" > > for now, or if we should pursue something more robust from the start, or > > perhaps something else entirely? > > > > Any feedback is greatly appreciated! > > > > src/libvirt_private.syms | 5 ++ > > src/qemu/qemu_hostdev.c | 16 +++++ > > src/qemu/qemu_hostdev.h | 4 ++ > > src/qemu/qemu_hotplug.c | 56 ++++++++++++++---- > > src/util/virfile.c | 52 +++++++++++++++++ > > src/util/virfile.h | 1 + > > src/util/virhostdev.c | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++----- > > src/util/virhostdev.h | 16 +++++ > > src/util/virpci.c | 69 ++++++++++++++++++---- > > src/util/virpci.h | 4 ++ > > 10 files changed, 360 insertions(+), 36 deletions(-) > > > > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2017-06-29 at 15:59 -0500, Michael Roth wrote: > > > Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver > > > until all the devices in the group have been detached, at which point all > > > the hostdevs are rebound as a group. Until that point, the devices are traced > > > by the drvManager's inactiveList in a similar manner to hostdevs that are > > > assigned to VFIO via the nodedev-detach interface. > > > > What happens if libvirtd is restarted during this period? Is the > > inactiveList rebuilt with all the info necessary to assure that the > > nodedev-reattach does/doesn't happen (as appropriate) for all devices? > > Hmm, good question. > > The Unbindable() check only needs to know that nothing in the activeList > belongs to the group we're checking, and that list at least seems to get > rebuilt appropriately on restart. > > But the Unbind() relies on inactiveList and the behavior there is what > nodedev-detach currently does, which is to add it to inactive list while > libvirtd is running, and then just forget about it when libvirtd restarts. > For nodedev-detach it's fine since virHostdevPreparePCIDevices() re-adds > it as needed in the device-attach path. But yah, for this purpose it ends > up losing track of hostdevs that are still pending rebind to the host, and > that means some devices may not get rebound at the appropriate time if > there was a libvirtd restart. > > Unlike with device-attach, we can't just re-add it on-demand because we > actually need to know whether or not it was previously in the list. So > I think we'd need to add some persistent state to track this. Will look > into adding handling for that. FWIW last time a tried to attack this issue[1] I got pretty much as far as this, eg. the code worked as intended but restarting libvirtd would result in an inconsistent state which prevented you from performing some operations. Unfortunately I got sidetracked by other work and stopped just short of implementing a way to persist the relevant information on disk :( [1] ~1.5 years ago, according to git log -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.