On 11/14/2014 11:19 AM, Ian Jackson wrote:
Boris Ostrovsky writes ("[PATCH 1/2] libxl: Wait until QEMU removed the device
before tearing it down"):
When a device is hot-unplugged libxl sends QEMU a "device-del" message
(via QMP). This call returns after QEMU has initiated device removal by
sending an interrupt to the guest. At some point later QEMU is expected
to clean up after the device (such as unbind/unmap MSIs), which will
occur when the guest signals that the device has been ejected.
As discussed, I agree that this is a problem. There is a race between
qemu/libxl and the guest. If libxl gets there first, you see the
symptoms you report. Having libxl not progress with the rest of the
removal is the right approach to fixing it.
However, unfortunately, the approach you have taken has some problems.
The most seriouis is this: you may not call usleep() anywhere inside
libxl. If you want to wait, you must use the callback mechanisms.
There are already instances of usleep in libxl (e.g. qmp_open(), which
is where I got the ~5 seconds number, btw). But I agree that callbacks
are better here, I didn't think of non-xl users.
This is because the process running libxl may be a daemon handling
many domains, and we must not hang waiting for any particular domain.
I think that this means:
* Making libxl__device_pci_remove_common asynchronous (ie, so
that it makes a callback when done, rather than simply returning;
* Then, making do_pci_remove asynchronous, too. This will involve
unfolding the loop in libxl__device_pci_remove_common into a
callback chain.
* Then, adjusting the new asynchronous do_pci_remove so that it
becomes two (or perhaps more) chained callback functions which
use a libxl__ev_time to manage the polling loop
OK, I'll try to see what needs to be done (I am not familiar with how
libxl does asynchronous calls)
Secondly, I'm not particularly keen on polling. Is there not a QMP
function that can be used to get notified when the device is really
removed ?
I didn't see any.
Sadly I guess that if there is, restructuring the qmp
handling in libxl_qmp.c (qmp_next et al) to be able to use it would be
way out of scope for a bugfix during the freeze.
This would probably require changes in QEMU as well since I don't
believe it notifies anyone on the other end of QMP connection that it is
done cleaning up.
Finally, some notes about error handling:
+ else {
+ unsigned total_us = 0, wait_us = 100000;
+
+ rc = -ETIMEDOUT;
rc must contain only libxl error values. Most libxl functions return
libxl error values, not errno values.
rc is overwritten later:
if (rc) {
rc = ERROR_FAIL;
goto out_fail;
}
So this whole rc assignment business in the patch was somewhat pointless
anyway.
One question though: should we fail on the timeout and not clean up
xenstore and reset the device (which is what this version of the patch
does)? Because we may end up with device finally removed and cleanup up
by QEMU/guest but not reset and removed from xenstore.
Thanks.
-boris
I'm sorry that the rest of this file is so confused about this. I
think you should use `ret' to contain responses which are `0 (for
success) or -1 (setting errno)', and probably something like
`errnoval' if you need actual errno values.
You should definitely never write -EFOOBAR in userland code.
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
