From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH 1/2] libxl: Wait until QEMU removed the device before tearing it down Date: Fri, 14 Nov 2014 12:09:38 -0500 Message-ID: <54663752.5080200@oracle.com> References: <1415661410-5191-1-git-send-email-boris.ostrovsky@oracle.com> <1415661410-5191-2-git-send-email-boris.ostrovsky@oracle.com> <21606.11138.483109.931473@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21606.11138.483109.931473@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: xen-devel@lists.xen.org, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org 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.