Re: [Xen-devel] [PATCH 1/2] libxl: Wait until QEMU removed the device before tearing it down

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.
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

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 ?  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.


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.

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel