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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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.