[libvirt] [PATCH v2 0/9] PCI hostdev refactoring

Changes in v2: * Internal functions operate on a single device, don't pass around device lists for no reason * Use virHostdev prefix for internal functions * Split off style adjustments and comments updated to separate patches See the cover letter for v1[1] for details about the series. Cheers. [1] https://www.redhat.com/archives/libvir-list/2016-January/msg00835.html Andrea Bolognani (9): hostdev: Add virHostdevOnlyReattachPCIDevice() hostdev: Use common reattach code to rollback prepare errors hostdev: Use common reattach code in virHostdevPCINodeDeviceReAttach() hostdev: Add virHostdevOnlyDetachPCIDevice() hostdev: Use common detach code in virHostdevPCINodeDeviceDetach() hostdev: Minor style adjustments hostdev: Update comments pci: Phase out virPCIDeviceReattachInit() pci: Add debug messages when unbinding from stub driver src/libvirt_private.syms | 1 - src/util/virhostdev.c | 385 ++++++++++++++++++++++++++++++----------------- src/util/virpci.c | 30 ++-- src/util/virpci.h | 1 - tests/virpcitest.c | 5 +- 5 files changed, 272 insertions(+), 150 deletions(-) -- 2.5.0

This function replaces virHostdevReattachPCIDevice() and, unlike it, does not perform list manipulation, leaving it to the calling function. This means virHostdevReAttachPCIDevices() had to be updated to cope with the updated requirements. --- src/util/virhostdev.c | 136 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 90 insertions(+), 46 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index f31ad41..66629b4 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -526,6 +526,74 @@ virHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev, return ret; } +/** + * virHostdevOnlyReattachPCIDevice: + * @mgr: hostdev manager + * @pci: PCI device to be reattached + * @skipUnmanaged: whether to skip unmanaged devices + * + * Reattach a PCI device to the host. + * + * This function only performs the base reattach steps that are required + * regardless of whether the device is being detached from a domain or + * had been simply detached from the host earlier. + * + * @pci must have already been marked as inactive, and the PCI related + * parts of @mgr (inactivePCIHostdevs, activePCIHostdevs) must have been + * locked beforehand using virObjectLock(). + * + * Returns: 0 on success, <0 on failure + */ +static int +virHostdevOnlyReattachPCIDevice(virHostdevManagerPtr mgr, + virPCIDevicePtr pci, + bool skipUnmanaged) +{ + virPCIDevicePtr actual; + int ret = -1; + + /* Retrieve the actual device from the inactive list */ + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) { + VIR_DEBUG("PCI device %s is not marked as inactive", + virPCIDeviceGetName(pci)); + goto out; + } + + /* Skip unmanaged devices if asked to do so */ + if (!virPCIDeviceGetManaged(actual) && skipUnmanaged) { + VIR_DEBUG("Not reattaching unmanaged PCI device %s", + virPCIDeviceGetName(actual)); + ret = 0; + goto out; + } + + /* Wait for device cleanup if it is qemu/kvm */ + if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) { + int retries = 100; + while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device") + && retries) { + usleep(100*1000); + retries--; + } + } + + VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual)); + if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to reattach PCI device %s: %s"), + virPCIDeviceGetName(actual), + err ? err->message : _("unknown error")); + virResetError(err); + goto out; + } + + ret = 0; + + out: + return ret; +} + int virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, const char *drv_name, @@ -753,45 +821,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, return ret; } -/* - * Pre-condition: inactivePCIHostdevs & activePCIHostdevs - * are locked - */ -static void -virHostdevReattachPCIDevice(virPCIDevicePtr dev, virHostdevManagerPtr mgr) -{ - /* If the device is not managed and was attached to guest - * successfully, it must have been inactive. - */ - if (!virPCIDeviceGetManaged(dev)) { - VIR_DEBUG("Adding unmanaged PCI device %s to inactive list", - virPCIDeviceGetName(dev)); - if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, dev) < 0) - virPCIDeviceFree(dev); - return; - } - - /* Wait for device cleanup if it is qemu/kvm */ - if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) { - int retries = 100; - while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device") - && retries) { - usleep(100*1000); - retries--; - } - } - - VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(dev)); - if (virPCIDeviceReattach(dev, mgr->activePCIHostdevs, - mgr->inactivePCIHostdevs) < 0) { - virErrorPtr err = virGetLastError(); - VIR_ERROR(_("Failed to re-attach PCI device: %s"), - err ? err->message : _("unknown error")); - virResetError(err); - } - virPCIDeviceFree(dev); -} - /* @oldStateDir: * For upgrade purpose: see virHostdevNetConfigRestore */ @@ -803,7 +832,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, int nhostdevs, const char *oldStateDir) { - virPCIDeviceListPtr pcidevs; + virPCIDeviceListPtr pcidevs = NULL; size_t i; if (!nhostdevs) @@ -848,11 +877,25 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, continue; } } + i++; + } + + /* Step 2: move all devices from the active list to the inactive list */ + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(dev)); - virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, dev); - i++; + if (!(actual = virPCIDeviceListSteal(hostdev_mgr->activePCIHostdevs, + dev))) + goto cleanup; + + VIR_DEBUG("Adding PCI device %s to inactive list", + virPCIDeviceGetName(actual)); + if (virPCIDeviceListAdd(hostdev_mgr->inactivePCIHostdevs, + actual) < 0) + goto cleanup; } /* At this point, any device that had been used by the guest is in @@ -900,13 +943,14 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, /* Loop 4: reattach devices to their host drivers (if managed) or place * them on the inactive list (if not managed) */ - while (virPCIDeviceListCount(pcidevs) > 0) { - virPCIDevicePtr dev = virPCIDeviceListStealIndex(pcidevs, 0); - virHostdevReattachPCIDevice(dev, hostdev_mgr); + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + + ignore_value(virHostdevOnlyReattachPCIDevice(hostdev_mgr, dev, true)); } - virObjectUnref(pcidevs); cleanup: + virObjectUnref(pcidevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); } -- 2.5.0

w/r/t: your [0/7] from initial series... As much as you don't want to keep living Groundhog Day - resolution of bugs like this are job security :-)... The subtleties of hostdev device Attach, Reattach, ReAttach, and Detach are such a tangle morass of shinola (ref: Steve Martin in "The Jerk")... w/r/t Suggestions for deamon restart issues... Seems like we need a way to save/restore the hostdev_mgr active/inactive lists using XML/JSON similar to how domains, storage, etc. handle it. Guess I just assumed that was already there ;-) since /var/run/libvirt/hostdevmgr exists. It seems that network stuff can be restored - virHostdevNetConfigRestore. Do you really think this series should be "held up" waiting to create some sort of status tracking? On 01/25/2016 11:20 AM, Andrea Bolognani wrote:
This function replaces virHostdevReattachPCIDevice() and, unlike it, does not perform list manipulation, leaving it to the calling function.
This means virHostdevReAttachPCIDevices() had to be updated to cope with the updated requirements. --- src/util/virhostdev.c | 136 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 90 insertions(+), 46 deletions(-)
Since I reviewed them all... I think the comment changes from 7/9 should just be inlined here and patch 4 instead of a separate patch
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index f31ad41..66629b4 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -526,6 +526,74 @@ virHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev, return ret; }
+/** + * virHostdevOnlyReattachPCIDevice:
Why not just reuse the function name you deleted? IOW: Is there a reason for "Only"? (not that I'm one that can complain about naming functions, but this just seems strange).
+ * @mgr: hostdev manager + * @pci: PCI device to be reattached
Interesting ... In 2 instances, this will be a pointer to the "copy" element, while in the third instance this will be the "actual" on inactive list element. For a copy element, we'd *have* to search inactive; however, for an 'actual' we don't "need" to.
+ * @skipUnmanaged: whether to skip unmanaged devices + * + * Reattach a PCI device to the host. + * + * This function only performs the base reattach steps that are required + * regardless of whether the device is being detached from a domain or + * had been simply detached from the host earlier. + * + * @pci must have already been marked as inactive, and the PCI related + * parts of @mgr (inactivePCIHostdevs, activePCIHostdevs) must have been + * locked beforehand using virObjectLock(). + * + * Returns: 0 on success, <0 on failure + */ +static int +virHostdevOnlyReattachPCIDevice(virHostdevManagerPtr mgr, + virPCIDevicePtr pci, + bool skipUnmanaged) +{ + virPCIDevicePtr actual; + int ret = -1; + + /* Retrieve the actual device from the inactive list */ + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) { + VIR_DEBUG("PCI device %s is not marked as inactive", + virPCIDeviceGetName(pci));
This is tricky - the only time we care about the return status (now) is when skipUnmanaged == false, a/k/a the path where we pass the onlist element.. In my first pass through the changes I thought - oh no if we return -1, then this would be a path that could get that generic libvirt failed for some reason message; however, closer inspection noted that we guaranteed it was on the inactive list before calling here.
+ goto out; + } + + /* Skip unmanaged devices if asked to do so */ + if (!virPCIDeviceGetManaged(actual) && skipUnmanaged) {
<sigh>, unrelated and existing - virPCIDeviceGetManaged probably should return bool instead of unsigned int
+ VIR_DEBUG("Not reattaching unmanaged PCI device %s", + virPCIDeviceGetName(actual)); + ret = 0; + goto out; + } + + /* Wait for device cleanup if it is qemu/kvm */ + if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) { + int retries = 100; + while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device") + && retries) { + usleep(100*1000); + retries--; + } + }
Existing, but if retries == 0, then cleanup never succeeded; however, perhaps one can assume that the subsequent call would fall over and play dead? Not that it really gets checked...
+ + VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual)); + if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to reattach PCI device %s: %s"), + virPCIDeviceGetName(actual), + err ? err->message : _("unknown error")); + virResetError(err); + goto out; + } + + ret = 0; + + out: + return ret; +} + int virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, const char *drv_name, @@ -753,45 +821,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, return ret; }
-/* - * Pre-condition: inactivePCIHostdevs & activePCIHostdevs - * are locked - */ -static void -virHostdevReattachPCIDevice(virPCIDevicePtr dev, virHostdevManagerPtr mgr) -{ - /* If the device is not managed and was attached to guest - * successfully, it must have been inactive. - */ - if (!virPCIDeviceGetManaged(dev)) { - VIR_DEBUG("Adding unmanaged PCI device %s to inactive list", - virPCIDeviceGetName(dev)); - if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, dev) < 0) - virPCIDeviceFree(dev); - return; - } - - /* Wait for device cleanup if it is qemu/kvm */ - if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) { - int retries = 100; - while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device") - && retries) { - usleep(100*1000); - retries--; - } - } - - VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(dev)); - if (virPCIDeviceReattach(dev, mgr->activePCIHostdevs, - mgr->inactivePCIHostdevs) < 0) { - virErrorPtr err = virGetLastError(); - VIR_ERROR(_("Failed to re-attach PCI device: %s"), - err ? err->message : _("unknown error")); - virResetError(err); - } - virPCIDeviceFree(dev); -} - /* @oldStateDir: * For upgrade purpose: see virHostdevNetConfigRestore */ @@ -803,7 +832,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, int nhostdevs, const char *oldStateDir) { - virPCIDeviceListPtr pcidevs; + virPCIDeviceListPtr pcidevs = NULL; size_t i;
if (!nhostdevs) @@ -848,11 +877,25 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, continue; } } + i++; + }
Curious why the decision for a second loop - if activeDev matches, then it almost seems a shame to loop again. Why not (within if (activeDev): actual = virPCIDeviceListSteal(hostdev_mgr->activePCIHostdevs, activeDev); /* !actual should never happen, but better safe than sorry */ if (actual && virPCIDeviceListAdd(hostdev_mgr->inactivePCIHostdevs, actual) < 0) virPCIDeviceFree(actual); /* You could also... */ virPCIDeviceListDel(pcidevs, dev); } NOTE: Whether there is one or two loops, the second loop would need to call virPCIDeviceFree(actual) since we'd leak it otherwise. You'll also note I didn't keep the goto cleanup. Previously the code would completely go through the pcidevs Loop 4 regardless of whether virHostdevReattachPCIDevice code had failures. The new code has the subtle difference of jumping to cleanup if a failure is found. That could leave things in an awkward state especially since virHostdevReAttachPCIDevices is a void. Since failure for DeviceListAdd is because 1. device is already there (which I would *hope* isn't the case) or 2. memory allocation failure (in which case not much else successful will probably happen anyway), then perhaps continuing on rather than jumping to cleanup isn't a bad idea... We could be returning some memory that someone may find useful. My concerns about jumping to cleanup are that this API is called in error recovery paths as well as part of the ominous comment "For upgrade purpose:..." (comment before function header). So it seems the "existing logic" is try to clean up as many as possible. By potentially erroring out too soon could lead to more problems. So the question becomes what havoc is introduced if we cannot add to the inactive list but decide to continue as before... It seems we'll end up "failing" in virHostdevOnlyReattachPCIDevice since it's not in the inactiveList, but our Loop 4 logic doesn't care. Of course we could delete 'dev' from 'pcidevs' too before then... Hopefully this makes sense... It's been an 'edit in process'...
+ + /* Step 2: move all devices from the active list to the inactive list */ + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual;
VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(dev)); - virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, dev);
This was a curious placement for *ListDel... If !activeDev, then calling *ListDel also won't find 'dev' on activelist...
- i++; + if (!(actual = virPCIDeviceListSteal(hostdev_mgr->activePCIHostdevs, + dev))) + goto cleanup;
If the choice is to use two loops (and perhaps keep the cleanups)... 1. If this Steal fails, then something is seriously wrong, but we don't even give it a VIR_DEBUG message. 2. If this Steal fails, then going to cleanup is again a subtle difference with the prior logic that said, well I couldn't do anything with this, but I'm going to keep processing. 3. If we keep processing, then something on 'pcidevs' won't be in 'inactivePCIHostdevs', causing virHostdevOnlyReattachPCIDevice to fail. But that does not matter since we ignore the return value in Loop 4. 4. If we do Steal and if the subsequent Add fails, then we leak 'actual', so prior to the goto cleanup call virPCIDeviceFree(actual); (or instead if the goto cleanup;'s are removed). John
+ + VIR_DEBUG("Adding PCI device %s to inactive list", + virPCIDeviceGetName(actual)); + if (virPCIDeviceListAdd(hostdev_mgr->inactivePCIHostdevs, + actual) < 0) + goto cleanup; }
/* At this point, any device that had been used by the guest is in @@ -900,13 +943,14 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, /* Loop 4: reattach devices to their host drivers (if managed) or place * them on the inactive list (if not managed) */ - while (virPCIDeviceListCount(pcidevs) > 0) { - virPCIDevicePtr dev = virPCIDeviceListStealIndex(pcidevs, 0); - virHostdevReattachPCIDevice(dev, hostdev_mgr); + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + + ignore_value(virHostdevOnlyReattachPCIDevice(hostdev_mgr, dev, true)); }
- virObjectUnref(pcidevs); cleanup: + virObjectUnref(pcidevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); }

On Tue, 2016-01-26 at 18:55 -0500, John Ferlan wrote: > > w/r/t: your [0/7] from initial series... > > As much as you don't want to keep living Groundhog Day - resolution of > bugs like this are job security :-)... Groundhog Day is in less than a week, by the way! :) > w/r/t Suggestions for deamon restart issues... Seems like we need a way > to save/restore the hostdev_mgr active/inactive lists using XML/JSON > similar to how domains, storage, etc. handle it. Guess I just assumed > that was already there ;-) since /var/run/libvirt/hostdevmgr exists. It > seems that network stuff can be restored - virHostdevNetConfigRestore. > > Do you really think this series should be "held up" waiting to create > some sort of status tracking? I will look into your suggestion. I believe such save / restore functionality has to be in place by the time this series is merged if we don't want to break everything on daemon restart. > On 01/25/2016 11:20 AM, Andrea Bolognani wrote: > > This function replaces virHostdevReattachPCIDevice() and, unlike it, > > does not perform list manipulation, leaving it to the calling function. > > > > This means virHostdevReAttachPCIDevices() had to be updated to cope > > with the updated requirements. > > --- > > src/util/virhostdev.c | 136 +++++++++++++++++++++++++++++++++----------------- > > 1 file changed, 90 insertions(+), 46 deletions(-) > > Since I reviewed them all... I think the comment changes from 7/9 should > just be inlined here and patch 4 instead of a separate patch Will do - it was that way in v1 as well. > > diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c > > index f31ad41..66629b4 100644 > > --- a/src/util/virhostdev.c > > +++ b/src/util/virhostdev.c > > @@ -526,6 +526,74 @@ virHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev, > > return ret; > > } > > > > +/** > > + * virHostdevOnlyReattachPCIDevice: > > Why not just reuse the function name you deleted? IOW: Is there a reason > for "Only"? (not that I'm one that can complain about naming functions, > but this just seems strange). It's an attempt to make it stand out a bit from virHostdevPCINodeDeviceReAttach() virHostdevReAttachPCIDevices() in the same file. Mostly the latter. The reasoning behind "Only" is that the function performs "Only" the job of reattaching the device to the host, with the error checking, bookkeeping and additional steps left to the caller. Which is, strictly speaking, not true :) Maybe something like virHostdevReattachPCIDeviceCommon(), to express the fact that this basically contains as much functionality as it was possible to split off to a reusable routine? > > + * @mgr: hostdev manager > > + * @pci: PCI device to be reattached > > Interesting ... In 2 instances, this will be a pointer to the "copy" > element, while in the third instance this will be the "actual" on > inactive list element. For a copy element, we'd *have* to search > inactive; however, for an 'actual' we don't "need" to. Good point. I will try to find a solution that 1. avoids searching the list twice 2. avoids duplicating code 3. respects the Principle of Least Surprise I can't guarantee I'll be able to :) > > + * @skipUnmanaged: whether to skip unmanaged devices > > + * > > + * Reattach a PCI device to the host. > > + * > > + * This function only performs the base reattach steps that are required > > + * regardless of whether the device is being detached from a domain or > > + * had been simply detached from the host earlier. > > + * > > + * @pci must have already been marked as inactive, and the PCI related > > + * parts of @mgr (inactivePCIHostdevs, activePCIHostdevs) must have been > > + * locked beforehand using virObjectLock(). > > + * > > + * Returns: 0 on success, <0 on failure > > + */ > > +static int > > +virHostdevOnlyReattachPCIDevice(virHostdevManagerPtr mgr, > > + virPCIDevicePtr pci, > > + bool skipUnmanaged) > > +{ > > + virPCIDevicePtr actual; > > + int ret = -1; > > + > > + /* Retrieve the actual device from the inactive list */ > > + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) { > > + VIR_DEBUG("PCI device %s is not marked as inactive", > > + virPCIDeviceGetName(pci)); > > This is tricky - the only time we care about the return status (now) is > when skipUnmanaged == false, a/k/a the path where we pass the onlist > element.. > > In my first pass through the changes I thought - oh no if we return -1, > then this would be a path that could get that generic libvirt failed for > some reason message; however, closer inspection noted that we guaranteed > it was on the inactive list before calling here. So we should be good, right? :) > > + goto out; > > + } > > + > > + /* Skip unmanaged devices if asked to do so */ > > + if (!virPCIDeviceGetManaged(actual) && skipUnmanaged) { > > <sigh>, unrelated and existing - virPCIDeviceGetManaged probably should > return bool instead of unsigned int Yup, good catch. The same applies to virPCIDeviceGetUnbindFromStub() virPCIDeviceGetRemoveSlot() virPCIDeviceGetReprobe() as well. I'll fix them in a separate commit. > > + VIR_DEBUG("Not reattaching unmanaged PCI device %s", > > + virPCIDeviceGetName(actual)); > > + ret = 0; > > + goto out; > > + } > > + > > + /* Wait for device cleanup if it is qemu/kvm */ > > + if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) { > > + int retries = 100; > > + while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device") > > + && retries) { > > + usleep(100*1000); > > + retries--; > > + } > > + } > > Existing, but if retries == 0, then cleanup never succeeded; however, > perhaps one can assume that the subsequent call would fall over and play > dead? Not that it really gets checked... I recall raising the issue at some point in the past, but I don't remember the outcome of that discussion... Maybe this can be assessed again at a later time? > > + > > + VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual)); > > + if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, > > + mgr->inactivePCIHostdevs) < 0) { > > + virErrorPtr err = virGetLastError(); > > + VIR_ERROR(_("Failed to reattach PCI device %s: %s"), > > + virPCIDeviceGetName(actual), > > + err ? err->message : _("unknown error")); > > + virResetError(err); > > + goto out; > > + } > > + > > + ret = 0; > > + > > + out: > > + return ret; > > +} > > + > > int > > virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, > > const char *drv_name, > > @@ -753,45 +821,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, > > return ret; > > } > > > > -/* > > - * Pre-condition: inactivePCIHostdevs & activePCIHostdevs > > - * are locked > > - */ > > -static void > > -virHostdevReattachPCIDevice(virPCIDevicePtr dev, virHostdevManagerPtr mgr) > > -{ > > - /* If the device is not managed and was attached to guest > > - * successfully, it must have been inactive. > > - */ > > - if (!virPCIDeviceGetManaged(dev)) { > > - VIR_DEBUG("Adding unmanaged PCI device %s to inactive list", > > - virPCIDeviceGetName(dev)); > > - if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, dev) < 0) > > - virPCIDeviceFree(dev); > > - return; > > - } > > - > > - /* Wait for device cleanup if it is qemu/kvm */ > > - if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) { > > - int retries = 100; > > - while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device") > > - && retries) { > > - usleep(100*1000); > > - retries--; > > - } > > - } > > - > > - VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(dev)); > > - if (virPCIDeviceReattach(dev, mgr->activePCIHostdevs, > > - mgr->inactivePCIHostdevs) < 0) { > > - virErrorPtr err = virGetLastError(); > > - VIR_ERROR(_("Failed to re-attach PCI device: %s"), > > - err ? err->message : _("unknown error")); > > - virResetError(err); > > - } > > - virPCIDeviceFree(dev); > > -} > > - > > /* @oldStateDir: > > * For upgrade purpose: see virHostdevNetConfigRestore > > */ > > @@ -803,7 +832,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, > > int nhostdevs, > > const char *oldStateDir) > > { > > - virPCIDeviceListPtr pcidevs; > > + virPCIDeviceListPtr pcidevs = NULL; > > size_t i; > > > > if (!nhostdevs) > > @@ -848,11 +877,25 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, > > continue; > > } > > } > > + i++; > > + } > > Curious why the decision for a second loop - if activeDev matches, then > it almost seems a shame to loop again. Why not (within if (activeDev): > > actual = virPCIDeviceListSteal(hostdev_mgr->activePCIHostdevs, > activeDev); > /* !actual should never happen, but better safe than sorry */ > if (actual && > virPCIDeviceListAdd(hostdev_mgr->inactivePCIHostdevs, > actual) < 0) > virPCIDeviceFree(actual); > /* You could also... */ > virPCIDeviceListDel(pcidevs, dev); > } Mostly because I consider moving the devices from one list to another as a separate step. We could merge the two steps, and that would bring us down to 4 (or 5 if you count the one implicit in virHostdevGetActivePCIHostDeviceList()) loops, but I'm not sure whether that would be a significant gain in performance or whether it would just make the code a little more convoluted... > NOTE: Whether there is one or two loops, the second loop would need to > call virPCIDeviceFree(actual) since we'd leak it otherwise. You mean on error? Because otherwise I don't see the leak: the actual device is stolen from the active list and added (itself, not a copy) to the inactive list. > You'll also note I didn't keep the goto cleanup. Previously the code > would completely go through the pcidevs Loop 4 regardless of whether > virHostdevReattachPCIDevice code had failures. The new code has the > subtle difference of jumping to cleanup if a failure is found. That > could leave things in an awkward state especially since > virHostdevReAttachPCIDevices is a void. > > Since failure for DeviceListAdd is because 1. device is already there > (which I would *hope* isn't the case) or 2. memory allocation failure > (in which case not much else successful will probably happen anyway), > then perhaps continuing on rather than jumping to cleanup isn't a bad > idea... We could be returning some memory that someone may find useful. > > My concerns about jumping to cleanup are that this API is called in > error recovery paths as well as part of the ominous comment "For upgrade > purpose:..." (comment before function header). So it seems the "existing > logic" is try to clean up as many as possible. By potentially erroring > out too soon could lead to more problems. > > So the question becomes what havoc is introduced if we cannot add to the > inactive list but decide to continue as before... It seems we'll end up > "failing" in virHostdevOnlyReattachPCIDevice since it's not in the > inactiveList, but our Loop 4 logic doesn't care. Of course we could > delete 'dev' from 'pcidevs' too before then... > > Hopefully this makes sense... It's been an 'edit in process'... See the comment at the end of the message. > > + > > + /* Step 2: move all devices from the active list to the inactive list */ > > + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { > > + virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); > > + virPCIDevicePtr actual; > > > > VIR_DEBUG("Removing PCI device %s from active list", > > virPCIDeviceGetName(dev)); > > - virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, dev); > > This was a curious placement for *ListDel... If !activeDev, then calling > *ListDel also won't find 'dev' on activelist... If 'activeDev != NULL', then driver name and domain name are checked, which may cause the 'dev' to be removed from 'pcidev' and the loop to restart. If that does not happen 'dev' is removed from the active list, even thought it might not have been in that list in the first place. But the code is doing the right thing in all situations. > > - i++; > > + if (!(actual = virPCIDeviceListSteal(hostdev_mgr->activePCIHostdevs, > > + dev))) > > + goto cleanup; > > If the choice is to use two loops (and perhaps keep the cleanups)... > > 1. If this Steal fails, then something is seriously wrong, but we don't > even give it a VIR_DEBUG message. > > 2. If this Steal fails, then going to cleanup is again a subtle > difference with the prior logic that said, well I couldn't do anything > with this, but I'm going to keep processing. > > 3. If we keep processing, then something on 'pcidevs' won't be in > 'inactivePCIHostdevs', causing virHostdevOnlyReattachPCIDevice to fail. > But that does not matter since we ignore the return value in Loop 4. > > 4. If we do Steal and if the subsequent Add fails, then we leak > 'actual', so prior to the goto cleanup call virPCIDeviceFree(actual); > (or instead if the goto cleanup;'s are removed). Thanks for looking into this in such detail. I will go through the existing code again myself and either become confident that the code is doing the right thing or change it so that it does :) On the other hand, there's this patch I'm working on that changes the way bookkeeping is performed quite substantially... My idea was to propose it as a follow-up to this series, since it basically replaces some constructs with some other "equivalent" constructs without altering the overall control flow, but maybe at this point it could be worth it to merge everything together and hopefully avoid such pitfalls, and make the whole thing easier to reason about. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

On 01/27/2016 12:26 PM, Andrea Bolognani wrote:
On Tue, 2016-01-26 at 18:55 -0500, John Ferlan wrote:
w/r/t: your [0/7] from initial series...
As much as you don't want to keep living Groundhog Day - resolution of bugs like this are job security :-)...
Groundhog Day is in less than a week, by the way! :)
w/r/t Suggestions for deamon restart issues... Seems like we need a way to save/restore the hostdev_mgr active/inactive lists using XML/JSON similar to how domains, storage, etc. handle it. Guess I just assumed that was already there ;-) since /var/run/libvirt/hostdevmgr exists. It seems that network stuff can be restored - virHostdevNetConfigRestore.
Do you really think this series should be "held up" waiting to create some sort of status tracking?
I will look into your suggestion. I believe such save / restore functionality has to be in place by the time this series is merged if we don't want to break everything on daemon restart.
I assume that restart is already broken... This series does fix some issues in the "normal" flow though. Perhaps a chicken and egg type problem. If you fix restart first, what of this series would be beneficial to ensure restart doesn't have issues...
On 01/25/2016 11:20 AM, Andrea Bolognani wrote:
This function replaces virHostdevReattachPCIDevice() and, unlike it, does not perform list manipulation, leaving it to the calling function.
This means virHostdevReAttachPCIDevices() had to be updated to cope with the updated requirements. --- src/util/virhostdev.c | 136 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 90 insertions(+), 46 deletions(-)
Since I reviewed them all... I think the comment changes from 7/9 should just be inlined here and patch 4 instead of a separate patch
Will do - it was that way in v1 as well.
Right - I started looking at v2
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index f31ad41..66629b4 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -526,6 +526,74 @@ virHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev, return ret; }
+/** + * virHostdevOnlyReattachPCIDevice:
Why not just reuse the function name you deleted? IOW: Is there a reason for "Only"? (not that I'm one that can complain about naming functions, but this just seems strange).
It's an attempt to make it stand out a bit from
virHostdevPCINodeDeviceReAttach() virHostdevReAttachPCIDevices()
in the same file. Mostly the latter.
The reasoning behind "Only" is that the function performs "Only" the job of reattaching the device to the host, with the error checking, bookkeeping and additional steps left to the caller.
Which is, strictly speaking, not true :)
Maybe something like virHostdevReattachPCIDeviceCommon(), to express the fact that this basically contains as much functionality as it was possible to split off to a reusable routine?
virHostdevReattachPCIDeviceVeryCarefully :-) But since it's in the comment of the code: virHostdevReattachPCIDeviceToHost
+ * @mgr: hostdev manager + * @pci: PCI device to be reattached
Interesting ... In 2 instances, this will be a pointer to the "copy" element, while in the third instance this will be the "actual" on inactive list element. For a copy element, we'd *have* to search inactive; however, for an 'actual' we don't "need" to.
Good point.
I will try to find a solution that
1. avoids searching the list twice 2. avoids duplicating code 3. respects the Principle of Least Surprise
I can't guarantee I'll be able to :)
I kept losing focus on when something was on the inactive list or not. Then of course trying to reconcile 'pci' and 'dev' variable name usage.
+ * @skipUnmanaged: whether to skip unmanaged devices + * + * Reattach a PCI device to the host. + * + * This function only performs the base reattach steps that are required + * regardless of whether the device is being detached from a domain or + * had been simply detached from the host earlier. + * + * @pci must have already been marked as inactive, and the PCI related + * parts of @mgr (inactivePCIHostdevs, activePCIHostdevs) must have been + * locked beforehand using virObjectLock(). + * + * Returns: 0 on success, <0 on failure + */ +static int +virHostdevOnlyReattachPCIDevice(virHostdevManagerPtr mgr, + virPCIDevicePtr pci, + bool skipUnmanaged) +{ + virPCIDevicePtr actual; + int ret = -1; + + /* Retrieve the actual device from the inactive list */ + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) { + VIR_DEBUG("PCI device %s is not marked as inactive", + virPCIDeviceGetName(pci));
This is tricky - the only time we care about the return status (now) is when skipUnmanaged == false, a/k/a the path where we pass the onlist element..
In my first pass through the changes I thought - oh no if we return -1, then this would be a path that could get that generic libvirt failed for some reason message; however, closer inspection noted that we guaranteed it was on the inactive list before calling here.
So we should be good, right? :)
I think so - a lot of typing out loud which at least gives you my review context... I finally convinced myself there wasn't an issue even though it's strange to return something and in the end, no one really cares... Perhaps using a 4th parameter 'actual' (could be NULL) would make it more obvious that we knew on input that the device was already found on the inactive list. Determining whether we have a copy or an actual wasn't readily apparent. Consider some future "user" of this function - how easy is it to decide what to pass?
+ goto out; + } + + /* Skip unmanaged devices if asked to do so */ + if (!virPCIDeviceGetManaged(actual) && skipUnmanaged) {
<sigh>, unrelated and existing - virPCIDeviceGetManaged probably should return bool instead of unsigned int
Yup, good catch. The same applies to
virPCIDeviceGetUnbindFromStub() virPCIDeviceGetRemoveSlot() virPCIDeviceGetReprobe()
as well. I'll fix them in a separate commit.
I saw that today...
+ VIR_DEBUG("Not reattaching unmanaged PCI device %s", + virPCIDeviceGetName(actual)); + ret = 0; + goto out; + } + + /* Wait for device cleanup if it is qemu/kvm */ + if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) { + int retries = 100; + while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device") + && retries) { + usleep(100*1000); + retries--; + } + }
Existing, but if retries == 0, then cleanup never succeeded; however, perhaps one can assume that the subsequent call would fall over and play dead? Not that it really gets checked...
I recall raising the issue at some point in the past, but I don't remember the outcome of that discussion... Maybe this can be assessed again at a later time?
That's fine - it was just another of those typing out loud moments.
+ + VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual)); + if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to reattach PCI device %s: %s"), + virPCIDeviceGetName(actual), + err ? err->message : _("unknown error")); + virResetError(err); + goto out; + } + + ret = 0; + + out: + return ret; +} + int virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, const char *drv_name, @@ -753,45 +821,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, return ret; }
-/* - * Pre-condition: inactivePCIHostdevs & activePCIHostdevs - * are locked - */ -static void -virHostdevReattachPCIDevice(virPCIDevicePtr dev, virHostdevManagerPtr mgr) -{ - /* If the device is not managed and was attached to guest - * successfully, it must have been inactive. - */ - if (!virPCIDeviceGetManaged(dev)) { - VIR_DEBUG("Adding unmanaged PCI device %s to inactive list", - virPCIDeviceGetName(dev)); - if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, dev) < 0) - virPCIDeviceFree(dev); - return; - } - - /* Wait for device cleanup if it is qemu/kvm */ - if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) { - int retries = 100; - while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device") - && retries) { - usleep(100*1000); - retries--; - } - } - - VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(dev)); - if (virPCIDeviceReattach(dev, mgr->activePCIHostdevs, - mgr->inactivePCIHostdevs) < 0) { - virErrorPtr err = virGetLastError(); - VIR_ERROR(_("Failed to re-attach PCI device: %s"), - err ? err->message : _("unknown error")); - virResetError(err); - } - virPCIDeviceFree(dev); -} - /* @oldStateDir: * For upgrade purpose: see virHostdevNetConfigRestore */ @@ -803,7 +832,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, int nhostdevs, const char *oldStateDir) { - virPCIDeviceListPtr pcidevs; + virPCIDeviceListPtr pcidevs = NULL; size_t i;
if (!nhostdevs) @@ -848,11 +877,25 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, continue; } } + i++; + }
Curious why the decision for a second loop - if activeDev matches, then it almost seems a shame to loop again. Why not (within if (activeDev):
actual = virPCIDeviceListSteal(hostdev_mgr->activePCIHostdevs, activeDev); /* !actual should never happen, but better safe than sorry */ if (actual && virPCIDeviceListAdd(hostdev_mgr->inactivePCIHostdevs, actual) < 0) virPCIDeviceFree(actual); /* You could also... */ virPCIDeviceListDel(pcidevs, dev); }
Mostly because I consider moving the devices from one list to another as a separate step.
We could merge the two steps, and that would bring us down to 4 (or 5 if you count the one implicit in virHostdevGetActivePCIHostDeviceList()) loops, but I'm not sure whether that would be a significant gain in performance or whether it would just make the code a little more convoluted...
Your call - we go through the pcidevs list many times so it's not that big a deal...
NOTE: Whether there is one or two loops, the second loop would need to call virPCIDeviceFree(actual) since we'd leak it otherwise.
You mean on error? Because otherwise I don't see the leak: the actual device is stolen from the active list and added (itself, not a copy) to the inactive list.
Yes, the error path - if you fail to add actual to inactive, then it was dropped on the floor
You'll also note I didn't keep the goto cleanup. Previously the code would completely go through the pcidevs Loop 4 regardless of whether virHostdevReattachPCIDevice code had failures. The new code has the subtle difference of jumping to cleanup if a failure is found. That could leave things in an awkward state especially since virHostdevReAttachPCIDevices is a void.
Since failure for DeviceListAdd is because 1. device is already there (which I would *hope* isn't the case) or 2. memory allocation failure (in which case not much else successful will probably happen anyway), then perhaps continuing on rather than jumping to cleanup isn't a bad idea... We could be returning some memory that someone may find useful.
My concerns about jumping to cleanup are that this API is called in error recovery paths as well as part of the ominous comment "For upgrade purpose:..." (comment before function header). So it seems the "existing logic" is try to clean up as many as possible. By potentially erroring out too soon could lead to more problems.
So the question becomes what havoc is introduced if we cannot add to the inactive list but decide to continue as before... It seems we'll end up "failing" in virHostdevOnlyReattachPCIDevice since it's not in the inactiveList, but our Loop 4 logic doesn't care. Of course we could delete 'dev' from 'pcidevs' too before then...
Hopefully this makes sense... It's been an 'edit in process'...
See the comment at the end of the message.
+ + /* Step 2: move all devices from the active list to the inactive list */ + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual;
VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(dev)); - virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, dev);
This was a curious placement for *ListDel... If !activeDev, then calling *ListDel also won't find 'dev' on activelist...
If 'activeDev != NULL', then driver name and domain name are checked, which may cause the 'dev' to be removed from 'pcidev' and the loop to restart.
If that does not happen 'dev' is removed from the active list, even thought it might not have been in that list in the first place. But the code is doing the right thing in all situations.
Understood, but if !activeDev, then the virPCIDeviceListDel calls virPCIDeviceListSteal which calls virPCIDeviceListStealIndex using virPCIDeviceListFindIndex... You'll note activeList is sourced by calling virPCIDeviceListFind which calls virPCIDeviceListFindIndex So I was pointing out how pointless it would be to call virPCIDeviceListDel if activeDev == NULL (because it too would do nothing).
- i++; + if (!(actual = virPCIDeviceListSteal(hostdev_mgr->activePCIHostdevs, + dev))) + goto cleanup;
If the choice is to use two loops (and perhaps keep the cleanups)...
1. If this Steal fails, then something is seriously wrong, but we don't even give it a VIR_DEBUG message.
2. If this Steal fails, then going to cleanup is again a subtle difference with the prior logic that said, well I couldn't do anything with this, but I'm going to keep processing.
3. If we keep processing, then something on 'pcidevs' won't be in 'inactivePCIHostdevs', causing virHostdevOnlyReattachPCIDevice to fail. But that does not matter since we ignore the return value in Loop 4.
4. If we do Steal and if the subsequent Add fails, then we leak 'actual', so prior to the goto cleanup call virPCIDeviceFree(actual); (or instead if the goto cleanup;'s are removed).
Thanks for looking into this in such detail. I will go through the existing code again myself and either become confident that the code is doing the right thing or change it so that it does :)
On the other hand, there's this patch I'm working on that changes the way bookkeeping is performed quite substantially... My idea was to propose it as a follow-up to this series, since it basically replaces some constructs with some other "equivalent" constructs without altering the overall control flow, but maybe at this point it could be worth it to merge everything together and hopefully avoid such pitfalls, and make the whole thing easier to reason about.
Try to keep it under 100 patches please ;-) John
Cheers.
-- Andrea Bolognani Software Engineer - Virtualization Team

--- src/util/virhostdev.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 66629b4..586937e 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -799,19 +799,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); - if (virPCIDeviceGetManaged(dev)) { - /* NB: This doesn't actually re-bind to original driver, just - * unbinds from the stub driver - */ - VIR_DEBUG("Reattaching managed PCI device %s", - virPCIDeviceGetName(dev)); - ignore_value(virPCIDeviceReattach(dev, - hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs)); - } else { - VIR_DEBUG("Not reattaching unmanaged PCI device %s", - virPCIDeviceGetName(dev)); - } + ignore_value(virHostdevOnlyReattachPCIDevice(hostdev_mgr, dev, true)); } cleanup: -- 2.5.0

Need something here... Perhaps to the effect of instead of inlining most of virHostdevOnlyReattachPCIDevice, call virHostdevOnlyReattachPCIDevice. One extra "call" or check being made by doing this is the virPCIDeviceGetStubDriver check and virPCIDeviceWaitForCleanup. Are there any "timing" considerations from this path? Now for every PCI hostdev found, we'll call *WaitForCleanup. That should perhaps be noted. This looks reasonable otherwise. John On 01/25/2016 11:20 AM, Andrea Bolognani wrote:
--- src/util/virhostdev.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 66629b4..586937e 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -799,19 +799,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
- if (virPCIDeviceGetManaged(dev)) { - /* NB: This doesn't actually re-bind to original driver, just - * unbinds from the stub driver - */ - VIR_DEBUG("Reattaching managed PCI device %s", - virPCIDeviceGetName(dev)); - ignore_value(virPCIDeviceReattach(dev, - hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs)); - } else { - VIR_DEBUG("Not reattaching unmanaged PCI device %s", - virPCIDeviceGetName(dev)); - } + ignore_value(virHostdevOnlyReattachPCIDevice(hostdev_mgr, dev, true)); }
cleanup:

On Tue, 2016-01-26 at 18:55 -0500, John Ferlan wrote:
Need something here... Perhaps to the effect of instead of inlining most of virHostdevOnlyReattachPCIDevice, call virHostdevOnlyReattachPCIDevice.
Sure.
One extra "call" or check being made by doing this is the virPCIDeviceGetStubDriver check and virPCIDeviceWaitForCleanup. Are there any "timing" considerations from this path? Now for every PCI hostdev found, we'll call *WaitForCleanup. That should perhaps be noted.
I think doing so should be fairly safe, possibly even desiderable. Let's see if anyone believes otherwise :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

This ensures the behavior for reattach is consistent, no matter how it was triggered (eg. 'virsh nodedev-reattach', 'virsh detach-device' or shutdown of a domain that is configured to use hostdevs). --- src/util/virhostdev.c | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 586937e..f40d636 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1637,10 +1637,24 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, return ret; } +/** + * virHostdevPCINodeDeviceReAttach: + * @hostdev_mgr: hostdev manager + * @pci: PCI device + * + * Reattach a specific PCI device to the host. + * + * This function makes sure the device is not in use before reattaching it + * to the host; if the device has already been reattached to the host, the + * operation is considered successful. + * + * Returns: 0 on success, <0 on failure + */ int virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr pci) { + virPCIDevicePtr actual; struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, NULL, false}; int ret = -1; @@ -1651,10 +1665,23 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) goto out; - virPCIDeviceReattachInit(pci); + /* We want this function to be idempotent, so if the device has already + * been removed from the inactive list (and is not active either, as + * per the check above) just return right away. We also need to retrieve + * the actual device from the inactive list because that's the one that + * contains state information such as the stub driver */ + if (!(actual = virPCIDeviceListFind(hostdev_mgr->inactivePCIHostdevs, + pci))) { + VIR_DEBUG("PCI device %s is already attached to the host", + virPCIDeviceGetName(pci)); + ret = 0; + goto out; + } - if (virPCIDeviceReattach(pci, hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) + /* Reattach the device. We don't want to skip unmanaged devices in + * this case, because the user explicitly asked for the device to + * be reattached to the host driver */ + if (virHostdevOnlyReattachPCIDevice(hostdev_mgr, actual, false) < 0) goto out; ret = 0; -- 2.5.0

On 01/25/2016 11:20 AM, Andrea Bolognani wrote:
This ensures the behavior for reattach is consistent, no matter how it was triggered (eg. 'virsh nodedev-reattach', 'virsh detach-device' or shutdown of a domain that is configured to use hostdevs).
Similar to 2/9 - using virHostdevOnlyReattachPCIDevice will result in a call to virPCIDeviceGetStubDriver and virPCIDeviceWaitForCleanup - for this path I assume this is thus desired.
--- src/util/virhostdev.c | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 586937e..f40d636 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1637,10 +1637,24 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, return ret; }
+/** + * virHostdevPCINodeDeviceReAttach:
^^ Oy ReAttach vs. Reattach is an eye test ;-)
+ * @hostdev_mgr: hostdev manager + * @pci: PCI device
Perhaps better to indicate a "new"ly generated PCI device that does not track the internal reattach states and other state information such as the stub driver. IOW: this is not a copy of an [in]activePCIHostdevs element
+ * + * Reattach a specific PCI device to the host. + * + * This function makes sure the device is not in use before reattaching it + * to the host; if the device has already been reattached to the host, the + * operation is considered successful. + * + * Returns: 0 on success, <0 on failure + */ int virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr pci) { + virPCIDevicePtr actual; struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, NULL, false}; int ret = -1; @@ -1651,10 +1665,23 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) goto out;
- virPCIDeviceReattachInit(pci); + /* We want this function to be idempotent, so if the device has already + * been removed from the inactive list (and is not active either, as + * per the check above) just return right away. We also need to retrieve + * the actual device from the inactive list because that's the one that + * contains state information such as the stub driver */ + if (!(actual = virPCIDeviceListFind(hostdev_mgr->inactivePCIHostdevs, + pci))) {
As noted in my 1/9 review - this code path passes the 'actual' dev onlist... The call we're about to make is going to repeat this search. Something we could avoid if performance of list searches were a concern. No other issues - John
+ VIR_DEBUG("PCI device %s is already attached to the host", + virPCIDeviceGetName(pci)); + ret = 0; + goto out; + }
- if (virPCIDeviceReattach(pci, hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) + /* Reattach the device. We don't want to skip unmanaged devices in + * this case, because the user explicitly asked for the device to + * be reattached to the host driver */ + if (virHostdevOnlyReattachPCIDevice(hostdev_mgr, actual, false) < 0) goto out;
ret = 0;

On Tue, 2016-01-26 at 18:56 -0500, John Ferlan wrote:
+/** + * virHostdevPCINodeDeviceReAttach: ^^ Oy ReAttach vs. Reattach is an eye test ;-)
Maybe we should standardize on either one or the other? I personally consider "ReAttach" to be quite an eyesore, but then again it's all over the public API so it's not going anywhere...
+ * @hostdev_mgr: hostdev manager + * @pci: PCI device Perhaps better to indicate a "new"ly generated PCI device that does not track the internal reattach states and other state information such as the stub driver. IOW: this is not a copy of an [in]activePCIHostdevs element
Great idea! Maybe even use a different name for the parameter, based on whether the virPCIDevicePtr is going to be used for something other than looking up the actual device? Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

On 01/27/2016 12:39 PM, Andrea Bolognani wrote:
On Tue, 2016-01-26 at 18:56 -0500, John Ferlan wrote:
+/** + * virHostdevPCINodeDeviceReAttach:
^^ Oy ReAttach vs. Reattach is an eye test ;-)
Maybe we should standardize on either one or the other? I personally consider "ReAttach" to be quite an eyesore, but then again it's all over the public API so it's not going anywhere...
Sadly, I think we're stuck with that CaMel case because it's a driver function...
+ * @hostdev_mgr: hostdev manager + * @pci: PCI device
Perhaps better to indicate a "new"ly generated PCI device that does not track the internal reattach states and other state information such as the stub driver.
IOW: this is not a copy of an [in]activePCIHostdevs element
Great idea! Maybe even use a different name for the parameter, based on whether the virPCIDevicePtr is going to be used for something other than looking up the actual device?
This is I believe where I went back to patch 1 and started thinking about what is passed in 'pci'... Anything to help make things more obvious could be beneficial, especially considering what this code ends up doing... John
Cheers.
-- Andrea Bolognani Software Engineer - Virtualization Team

This function mirrors virHostdevOnlyDetachPCIDevice(). The handling of active and inactive devices is updated and made more explicit, which means virHostdevPreparePCIDevices() has to be updated as well. --- src/util/virhostdev.c | 87 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 61 insertions(+), 26 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index f40d636..6f14574 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -594,6 +594,56 @@ virHostdevOnlyReattachPCIDevice(virHostdevManagerPtr mgr, return ret; } +/** + * virHostdevOnlyDetachPCIDevice: + * @mgr: hostdev manager + * @pci: PCI device to be detached + * @skipUnmanaged: whether to skip unmanaged devices + * + * Detach a PCI device from the host. + * + * This function only performs the base detach steps that are required + * regardless of whether the device is being attached to a domain or + * is simply being detached from the host for later use. + * + * The PCI related parts of @mgr (inactivePCIHostdevs, activePCIHostdevs) + * must have been locked beforehand using virObjectLock(). + * + * Returns: 0 on success, <0 on failure + */ +static int +virHostdevOnlyDetachPCIDevice(virHostdevManagerPtr mgr, + virPCIDevicePtr pci, + bool skipUnmanaged) +{ + int ret = -1; + + /* Skip unmanaged devices if asked to do so */ + if (!virPCIDeviceGetManaged(pci) && skipUnmanaged) { + VIR_DEBUG("Not detaching unmanaged PCI device %s", + virPCIDeviceGetName(pci)); + ret = 0; + goto out; + } + + VIR_DEBUG("Detaching PCI device %s", virPCIDeviceGetName(pci)); + if (virPCIDeviceDetach(pci, + mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to detach PCI device %s: %s"), + virPCIDeviceGetName(pci), + err ? err->message : _("unknown error")); + virResetError(err); + goto out; + } + + ret = 0; + + out: + return ret; +} + int virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, const char *drv_name, @@ -664,17 +714,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); - if (virPCIDeviceGetManaged(dev)) { - VIR_DEBUG("Detaching managed PCI device %s", - virPCIDeviceGetName(dev)); - if (virPCIDeviceDetach(dev, - hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) - goto reattachdevs; - } else { - VIR_DEBUG("Not detaching unmanaged PCI device %s", - virPCIDeviceGetName(dev)); - } + if (virHostdevOnlyDetachPCIDevice(hostdev_mgr, dev, true) < 0) + goto reattachdevs; } /* At this point, all devices are attached to the stub driver and have @@ -704,23 +745,21 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, last_processed_hostdev_vf = i; } - /* Loop 5: Now mark all the devices as active */ + /* Step 5: move all devices from the inactive list to the active list */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; - VIR_DEBUG("Adding PCI device %s to active list", + VIR_DEBUG("Removing PCI device %s from inactive list", virPCIDeviceGetName(dev)); - if (virPCIDeviceListAdd(hostdev_mgr->activePCIHostdevs, dev) < 0) + if (!(actual = virPCIDeviceListSteal(hostdev_mgr->inactivePCIHostdevs, + dev))) goto inactivedevs; - } - - /* Loop 6: Now remove the devices from inactive list. */ - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); - VIR_DEBUG("Removing PCI device %s from inactive list", - virPCIDeviceGetName(dev)); - virPCIDeviceListDel(hostdev_mgr->inactivePCIHostdevs, dev); + VIR_DEBUG("Adding PCI device %s to active list", + virPCIDeviceGetName(actual)); + if (virPCIDeviceListAdd(hostdev_mgr->activePCIHostdevs, actual) < 0) + goto inactivedevs; } /* Loop 7: Now set the used_by_domain of the device in @@ -771,10 +810,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDeviceFree(dev); } - /* Loop 9: Now steal all the devices from pcidevs */ - while (virPCIDeviceListCount(pcidevs) > 0) - virPCIDeviceListStealIndex(pcidevs, 0); - ret = 0; goto cleanup; -- 2.5.0

On 01/25/2016 11:20 AM, Andrea Bolognani wrote:
This function mirrors virHostdevOnlyDetachPCIDevice().
The handling of active and inactive devices is updated and made more explicit, which means virHostdevPreparePCIDevices() has to be updated as well. --- src/util/virhostdev.c | 87 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 61 insertions(+), 26 deletions(-)
I think parts of patch 7 to adjust comments should just be inlined here
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index f40d636..6f14574 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -594,6 +594,56 @@ virHostdevOnlyReattachPCIDevice(virHostdevManagerPtr mgr, return ret; }
+/** + * virHostdevOnlyDetachPCIDevice:
Similar to 1/9 comments, you could drop the "Only"
+ * @mgr: hostdev manager + * @pci: PCI device to be detached
... Copy of a 'hostdev' - expected to be added to inactivelist...
+ * @skipUnmanaged: whether to skip unmanaged devices + * + * Detach a PCI device from the host. + * + * This function only performs the base detach steps that are required + * regardless of whether the device is being attached to a domain or + * is simply being detached from the host for later use. + * + * The PCI related parts of @mgr (inactivePCIHostdevs, activePCIHostdevs) + * must have been locked beforehand using virObjectLock(). + * + * Returns: 0 on success, <0 on failure + */ +static int +virHostdevOnlyDetachPCIDevice(virHostdevManagerPtr mgr, + virPCIDevicePtr pci, + bool skipUnmanaged) +{ + int ret = -1; + + /* Skip unmanaged devices if asked to do so */ + if (!virPCIDeviceGetManaged(pci) && skipUnmanaged) { + VIR_DEBUG("Not detaching unmanaged PCI device %s", + virPCIDeviceGetName(pci)); + ret = 0; + goto out; + } + + VIR_DEBUG("Detaching PCI device %s", virPCIDeviceGetName(pci)); + if (virPCIDeviceDetach(pci, + mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to detach PCI device %s: %s"), + virPCIDeviceGetName(pci), + err ? err->message : _("unknown error")); + virResetError(err); + goto out; + } + + ret = 0; + + out: + return ret; +} + int virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, const char *drv_name, @@ -664,17 +714,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
- if (virPCIDeviceGetManaged(dev)) { - VIR_DEBUG("Detaching managed PCI device %s", - virPCIDeviceGetName(dev)); - if (virPCIDeviceDetach(dev, - hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) - goto reattachdevs; - } else { - VIR_DEBUG("Not detaching unmanaged PCI device %s", - virPCIDeviceGetName(dev)); - } + if (virHostdevOnlyDetachPCIDevice(hostdev_mgr, dev, true) < 0) + goto reattachdevs; }
/* At this point, all devices are attached to the stub driver and have @@ -704,23 +745,21 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, last_processed_hostdev_vf = i; }
- /* Loop 5: Now mark all the devices as active */ + /* Step 5: move all devices from the inactive list to the active list */
I know patch 7 adjusts other comments, but things are still described as loops in other comments - probably should follow... Or of course, make the comment changes now because [1a & 1b]
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual;
- VIR_DEBUG("Adding PCI device %s to active list", + VIR_DEBUG("Removing PCI device %s from inactive list", virPCIDeviceGetName(dev)); - if (virPCIDeviceListAdd(hostdev_mgr->activePCIHostdevs, dev) < 0) + if (!(actual = virPCIDeviceListSteal(hostdev_mgr->inactivePCIHostdevs, + dev))) goto inactivedevs; - } - - /* Loop 6: Now remove the devices from inactive list. */ - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
[1a] This removes Loop 6 (or Step 6), but does not renumber the following loops/steps nor does it address the comment much earlier "We have to use 9 loops here."
- virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
- VIR_DEBUG("Removing PCI device %s from inactive list", - virPCIDeviceGetName(dev)); - virPCIDeviceListDel(hostdev_mgr->inactivePCIHostdevs, dev); + VIR_DEBUG("Adding PCI device %s to active list", + virPCIDeviceGetName(actual)); + if (virPCIDeviceListAdd(hostdev_mgr->activePCIHostdevs, actual) < 0) + goto inactivedevs; }
This seems more efficient than the former code (and of course why I also suggest in 1/9 to not make a second pass).
/* Loop 7: Now set the used_by_domain of the device in @@ -771,10 +810,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDeviceFree(dev); }
- /* Loop 9: Now steal all the devices from pcidevs */
[1b] Delete another loop/step.
- while (virPCIDeviceListCount(pcidevs) > 0) - virPCIDeviceListStealIndex(pcidevs, 0); -
Hmmm.. interesting looks like this would have been a memory leak since the code ignoree the return.... Normal when *Steal is called from *ListDel, a *DeviceFree is called on the returned stolen device. John
ret = 0; goto cleanup;

On Tue, 2016-01-26 at 18:58 -0500, John Ferlan wrote:
- while (virPCIDeviceListCount(pcidevs) > 0) - virPCIDeviceListStealIndex(pcidevs, 0); - Hmmm.. interesting looks like this would have been a memory leak since
the code ignoree the return.... Normal when *Steal is called from *ListDel, a *DeviceFree is called on the returned stolen device.
I don't think it was, because earlier on the devices in 'pcidevs' are added, not copied, to the active list. So stealing all of them from 'pcidevs' here is a way to prevent 'virObjectUnref(pcidevs)' in the cleanup part to free the memory associated with them Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

This ensures the behavior for detach is consistent, no matter how it was triggered (eg. 'virsh nodedev-detach', 'virsh attach-device' or startup of a domain that is configured to use hostdevs). --- src/util/virhostdev.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 6f14574..5ded1e9 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1646,6 +1646,19 @@ virHostdevReAttachSCSIDevices(virHostdevManagerPtr hostdev_mgr, virObjectUnlock(hostdev_mgr->activeSCSIHostdevs); } +/** + * virHostdevPCINodeDeviceDetach: + * @hostdev_mgr: hostdev manager + * @pci: PCI device + * + * Detach a specific PCI device from the host. + * + * This function makes sure the device is not in use before detaching it + * from the host; if the device has already been detached from the host, + * the operation is considered successful. + * + * Returns: 0 on success, <0 on failure + */ int virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr pci) @@ -1660,11 +1673,22 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) goto out; - if (virPCIDeviceDetach(pci, hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) { + /* We want this function to be idempotent, so if the device has already + * been added to the inactive list (and is not active, as per the check + * above) just return right away */ + if (virPCIDeviceListFind(hostdev_mgr->inactivePCIHostdevs, pci)) { + VIR_DEBUG("PCI device %s is already detached from the host", + virPCIDeviceGetName(pci)); + ret = 0; goto out; } + /* Detach the device. We don't want to skip unmanaged devices in + * this case, because the user explicitly asked for the device to + * be detached from the host driver */ + if (virHostdevOnlyDetachPCIDevice(hostdev_mgr, pci, false) < 0) + goto out; + ret = 0; out: virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); -- 2.5.0

On 01/25/2016 11:21 AM, Andrea Bolognani wrote:
This ensures the behavior for detach is consistent, no matter how it was triggered (eg. 'virsh nodedev-detach', 'virsh attach-device' or startup of a domain that is configured to use hostdevs). --- src/util/virhostdev.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 6f14574..5ded1e9 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1646,6 +1646,19 @@ virHostdevReAttachSCSIDevices(virHostdevManagerPtr hostdev_mgr, virObjectUnlock(hostdev_mgr->activeSCSIHostdevs); }
+/** + * virHostdevPCINodeDeviceDetach: + * @hostdev_mgr: hostdev manager + * @pci: PCI device
A new PCI device to be detach from the host IOW: Not currently on some active/inactive list, right? Seems reasonable otherwise though. John
+ * + * Detach a specific PCI device from the host. + * + * This function makes sure the device is not in use before detaching it + * from the host; if the device has already been detached from the host, + * the operation is considered successful. + * + * Returns: 0 on success, <0 on failure + */ int virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr pci) @@ -1660,11 +1673,22 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) goto out;
- if (virPCIDeviceDetach(pci, hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) { + /* We want this function to be idempotent, so if the device has already + * been added to the inactive list (and is not active, as per the check + * above) just return right away */ + if (virPCIDeviceListFind(hostdev_mgr->inactivePCIHostdevs, pci)) { + VIR_DEBUG("PCI device %s is already detached from the host", + virPCIDeviceGetName(pci)); + ret = 0; goto out; }
+ /* Detach the device. We don't want to skip unmanaged devices in + * this case, because the user explicitly asked for the device to + * be detached from the host driver */ + if (virHostdevOnlyDetachPCIDevice(hostdev_mgr, pci, false) < 0) + goto out; + ret = 0; out: virObjectUnlock(hostdev_mgr->inactivePCIHostdevs);

Mostly labels names and whitespace. No functional changes. --- src/util/virhostdev.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 5ded1e9..ab17547 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -838,9 +838,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, } cleanup: + virObjectUnref(pcidevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); - virObjectUnref(pcidevs); + return ret; } @@ -1671,7 +1672,7 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, virObjectLock(hostdev_mgr->inactivePCIHostdevs); if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) - goto out; + goto cleanup; /* We want this function to be idempotent, so if the device has already * been added to the inactive list (and is not active, as per the check @@ -1680,19 +1681,21 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, VIR_DEBUG("PCI device %s is already detached from the host", virPCIDeviceGetName(pci)); ret = 0; - goto out; + goto cleanup; } /* Detach the device. We don't want to skip unmanaged devices in * this case, because the user explicitly asked for the device to * be detached from the host driver */ if (virHostdevOnlyDetachPCIDevice(hostdev_mgr, pci, false) < 0) - goto out; + goto cleanup; ret = 0; - out: + + cleanup: virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); + return ret; } @@ -1722,7 +1725,7 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, virObjectLock(hostdev_mgr->inactivePCIHostdevs); if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) - goto out; + goto cleanup; /* We want this function to be idempotent, so if the device has already * been removed from the inactive list (and is not active either, as @@ -1734,19 +1737,21 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, VIR_DEBUG("PCI device %s is already attached to the host", virPCIDeviceGetName(pci)); ret = 0; - goto out; + goto cleanup; } /* Reattach the device. We don't want to skip unmanaged devices in * this case, because the user explicitly asked for the device to * be reattached to the host driver */ if (virHostdevOnlyReattachPCIDevice(hostdev_mgr, actual, false) < 0) - goto out; + goto cleanup; ret = 0; - out: + + cleanup: virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); + return ret; } -- 2.5.0

On 01/25/2016 11:21 AM, Andrea Bolognani wrote:
Mostly labels names and whitespace.
No functional changes. --- src/util/virhostdev.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
Seems reasonable, ACK John

On Tue, 2016-01-26 at 18:59 -0500, John Ferlan wrote:
On 01/25/2016 11:21 AM, Andrea Bolognani wrote:
Mostly labels names and whitespace. No functional changes. --- src/util/virhostdev.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) Seems reasonable, ACK
I'm picking up this series again; since this patch had already been ACKed and doesn't depend on other patches in the series, I've pushed it. More interesting stuff coming soon ;) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

Some of the comments are no longer accurate after the recent changes, others have been expanded and hopefully improved. The initial summary of the operations has been removed so that two separate edits are not needed when making changes. --- src/util/virhostdev.c | 68 +++++++++++++++++++++++---------------------------- 1 file changed, 30 insertions(+), 38 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index ab17547..0892dbf 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -675,11 +675,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, * must be reset before being marked as active. */ - /* Loop 1: validate that non-managed device isn't in use, eg - * by checking that device is either un-bound, or bound - * to pci-stub.ko - */ + /* Detaching devices from the host involves several steps; each of them + * is described at length below */ + /* Step 1: perform safety checks, eg. ensure the devices are assignable */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK); @@ -710,7 +709,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, } } - /* Loop 2: detach managed devices (i.e. bind to appropriate stub driver) */ + /* Step 2: detach managed devices (i.e. bind to appropriate stub driver). + * detachPCIDevices() will also mark devices as inactive */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); @@ -718,11 +718,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, goto reattachdevs; } - /* At this point, all devices are attached to the stub driver and have + /* At this point, devices are attached to the stub driver and have * been marked as inactive */ - /* Loop 3: Now that all the PCI hostdevs have been detached, we - * can safely reset them */ + /* Step 3: perform a PCI reset on all devices */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); @@ -732,8 +731,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, goto reattachdevs; } - /* Loop 4: For SRIOV network devices, Now that we have detached the - * the network device, set the netdev config */ + /* Step 4: set the netdev config for SRIOV network devices */ for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; if (!virHostdevIsPCINetDevice(hostdev)) @@ -762,9 +760,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, goto inactivedevs; } - /* Loop 7: Now set the used_by_domain of the device in - * activePCIHostdevs as domain name. - */ + /* Step 6: set driver and domain information */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev, activeDev; @@ -777,7 +773,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDeviceSetUsedBy(activeDev, drv_name, dom_name); } - /* Loop 8: Now set the original states for hostdev def */ + /* Step 7: set the original states for hostdev def */ for (i = 0; i < nhostdevs; i++) { virPCIDevicePtr dev; virPCIDevicePtr pcidev; @@ -792,10 +788,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, dev = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, pcisrc->addr.slot, pcisrc->addr.function); - /* original states "unbind_from_stub", "remove_slot", - * "reprobe" were already set by pciDettachDevice in - * loop 2. - */ + /* original states for "unbind_from_stub", "remove_slot" and + * "reprobe" (used when reattaching) were already set by + * detachPCIDevices() in a previous step */ VIR_DEBUG("Saving network configuration of PCI device %s", virPCIDeviceGetName(dev)); if ((pcidev = virPCIDeviceListFind(pcidevs, dev))) { @@ -875,16 +870,11 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, goto cleanup; } - /* Loop through the assigned devices 4 times: 1) delete them all from - * activePCIHostdevs, 2) restore network config of SRIOV netdevs, 3) Do a - * PCI reset on each device, 4) reattach the devices to their host drivers - * (managed) or add them to inactivePCIHostdevs (!managed). - */ + /* Reattaching devices to the host involves several steps; each of them + * is described at length below */ - /* - * Loop 1: verify that each device in the hostdevs list really was in use - * by this domain, and remove them all from the activePCIHostdevs list. - */ + /* Step 1: verify that each device in the hostdevs list really was in use + * by this domain */ i = 0; while (i < virPCIDeviceListCount(pcidevs)) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); @@ -922,14 +912,11 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, goto cleanup; } - /* At this point, any device that had been used by the guest is in - * pcidevs, but has been removed from activePCIHostdevs. - */ + /* At this point, devices are no longer marked as active and have been + * marked as inactive instead */ - /* - * Loop 2: restore original network config of hostdevs that used - * <interface type='hostdev'> - */ + /* Step 3: restore original network config of hostdevs that used + * <interface type='hostdev'> */ for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; @@ -950,7 +937,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, } } - /* Loop 3: perform a PCI Reset on all devices */ + /* Step 4: perform a PCI Reset on all devices */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); @@ -964,15 +951,20 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, } } - /* Loop 4: reattach devices to their host drivers (if managed) or place - * them on the inactive list (if not managed) - */ + /* Step 5: reattach managed devices to their host drivers; unmanaged + * devices need no further processing. reattachPCIDevices() will + * remove devices from the inactive list as they are reattached + * to the host */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); ignore_value(virHostdevOnlyReattachPCIDevice(hostdev_mgr, dev, true)); } + /* At this point, all devices are either marked as inactive (if they + * were unmanaged), or have been reattached to the host driver (if they + * were managed) and are no longer tracked by any of our device lists */ + cleanup: virObjectUnref(pcidevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); -- 2.5.0

On 01/25/2016 11:21 AM, Andrea Bolognani wrote:
Some of the comments are no longer accurate after the recent changes, others have been expanded and hopefully improved.
The initial summary of the operations has been removed so that two separate edits are not needed when making changes. --- src/util/virhostdev.c | 68 +++++++++++++++++++++++---------------------------- 1 file changed, 30 insertions(+), 38 deletions(-)
6 of one, half dozen of another, but I think these should have been done at the time of change... I'll make specific comments about changes here still though...
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index ab17547..0892dbf 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c
This comment block starts "We have to use 9 loops here." - that should be adjusted too as there are now 7 steps. (Is it any coincidence that there are also 7 stages of grief and 7 steps to great health? ;-))
@@ -675,11 +675,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, * must be reset before being marked as active. */
- /* Loop 1: validate that non-managed device isn't in use, eg - * by checking that device is either un-bound, or bound - * to pci-stub.ko - */ + /* Detaching devices from the host involves several steps; each of them + * is described at length below */
+ /* Step 1: perform safety checks, eg. ensure the devices are assignable */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK); @@ -710,7 +709,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, } }
- /* Loop 2: detach managed devices (i.e. bind to appropriate stub driver) */ + /* Step 2: detach managed devices (i.e. bind to appropriate stub driver). + * detachPCIDevices() will also mark devices as inactive */
s/detachPCIDevices/virHostdevOnlyDetachPCIDevice Or whatever it ends up being named. s/will also mark devices as inactive/will place device on inactive list */
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
@@ -718,11 +718,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, goto reattachdevs; }
- /* At this point, all devices are attached to the stub driver and have + /* At this point, devices are attached to the stub driver and have * been marked as inactive */
- /* Loop 3: Now that all the PCI hostdevs have been detached, we - * can safely reset them */ + /* Step 3: perform a PCI reset on all devices */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
@@ -732,8 +731,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, goto reattachdevs; }
- /* Loop 4: For SRIOV network devices, Now that we have detached the - * the network device, set the netdev config */ + /* Step 4: set the netdev config for SRIOV network devices */ for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; if (!virHostdevIsPCINetDevice(hostdev)) @@ -762,9 +760,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, goto inactivedevs; }
Made my Step 5 comment earlier
- /* Loop 7: Now set the used_by_domain of the device in - * activePCIHostdevs as domain name. - */ + /* Step 6: set driver and domain information */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev, activeDev;
@@ -777,7 +773,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDeviceSetUsedBy(activeDev, drv_name, dom_name); }
- /* Loop 8: Now set the original states for hostdev def */ + /* Step 7: set the original states for hostdev def */ for (i = 0; i < nhostdevs; i++) { virPCIDevicePtr dev; virPCIDevicePtr pcidev; @@ -792,10 +788,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, dev = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, pcisrc->addr.slot, pcisrc->addr.function);
- /* original states "unbind_from_stub", "remove_slot", - * "reprobe" were already set by pciDettachDevice in - * loop 2. - */ + /* original states for "unbind_from_stub", "remove_slot" and + * "reprobe" (used when reattaching) were already set by + * detachPCIDevices() in a previous step */ VIR_DEBUG("Saving network configuration of PCI device %s", virPCIDeviceGetName(dev)); if ((pcidev = virPCIDeviceListFind(pcidevs, dev))) { @@ -875,16 +870,11 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, goto cleanup; }
- /* Loop through the assigned devices 4 times: 1) delete them all from - * activePCIHostdevs, 2) restore network config of SRIOV netdevs, 3) Do a - * PCI reset on each device, 4) reattach the devices to their host drivers - * (managed) or add them to inactivePCIHostdevs (!managed). - */ + /* Reattaching devices to the host involves several steps; each of them + * is described at length below */
- /* - * Loop 1: verify that each device in the hostdevs list really was in use - * by this domain, and remove them all from the activePCIHostdevs list. - */ + /* Step 1: verify that each device in the hostdevs list really was in use + * by this domain */ i = 0; while (i < virPCIDeviceListCount(pcidevs)) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); @@ -922,14 +912,11 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, goto cleanup; }
- /* At this point, any device that had been used by the guest is in - * pcidevs, but has been removed from activePCIHostdevs. - */ + /* At this point, devices are no longer marked as active and have been + * marked as inactive instead */
- /* - * Loop 2: restore original network config of hostdevs that used - * <interface type='hostdev'> - */ + /* Step 3: restore original network config of hostdevs that used + * <interface type='hostdev'> */ for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i];
@@ -950,7 +937,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, } }
- /* Loop 3: perform a PCI Reset on all devices */ + /* Step 4: perform a PCI Reset on all devices */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
@@ -964,15 +951,20 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, } }
- /* Loop 4: reattach devices to their host drivers (if managed) or place - * them on the inactive list (if not managed) - */ + /* Step 5: reattach managed devices to their host drivers; unmanaged + * devices need no further processing. reattachPCIDevices() will
s/reattachPCIDevices/virHostdevOnlyReattachPCIDevice (or whatever the final name is)
+ * remove devices from the inactive list as they are reattached + * to the host */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
ignore_value(virHostdevOnlyReattachPCIDevice(hostdev_mgr, dev, true)); }
+ /* At this point, all devices are either marked as inactive (if they + * were unmanaged), or have been reattached to the host driver (if they + * were managed) and are no longer tracked by any of our device lists */ +
Ahh, but if you kept the goto cleanup code during what is step 2 here, then this wouldn't be necessarily true... Although I personally think your new Step 2 is just part of Step 1. John
cleanup: virObjectUnref(pcidevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs);

On Tue, 2016-01-26 at 18:59 -0500, John Ferlan wrote:
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index ab17547..0892dbf 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c This comment block starts "We have to use 9 loops here." - that should be adjusted too as there are now 7 steps. (Is it any coincidence that
there are also 7 stages of grief and 7 steps to great health? ;-))
That comment is still there by mistake: the comment
+ /* Detaching devices from the host involves several steps; each of them + * is described at length below */
was supposed to replace it, the idea being that if we list and describe briefly the steps in a comment, as we currently do, we have to keep both the actual comments and the summary in sync with no real gain.
+ /* Step 1: perform safety checks, eg. ensure the devices are assignable */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK); @@ -710,7 +709,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, } } - /* Loop 2: detach managed devices (i.e. bind to appropriate stub driver) */ + /* Step 2: detach managed devices (i.e. bind to appropriate stub driver). + * detachPCIDevices() will also mark devices as inactive */ s/detachPCIDevices/virHostdevOnlyDetachPCIDevice Or whatever it ends up being named.
Missed when renaming the function between v1 and v2 :)
s/will also mark devices as inactive/will place device on inactive list */
Okay.
+ /* Step 5: reattach managed devices to their host drivers; unmanaged + * devices need no further processing. reattachPCIDevices() will s/reattachPCIDevices/virHostdevOnlyReattachPCIDevice (or whatever the final name is)
Same as above :)
+ * remove devices from the inactive list as they are reattached + * to the host */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); ignore_value(virHostdevOnlyReattachPCIDevice(hostdev_mgr, dev, true)); } + /* At this point, all devices are either marked as inactive (if they + * were unmanaged), or have been reattached to the host driver (if they + * were managed) and are no longer tracked by any of our device lists */ + Ahh, but if you kept the goto cleanup code during what is step 2 here, then this wouldn't be necessarily true...
Well, the comment was intended to refer to the situation after Step 5 has been performed, implying that no error that could have caused us to jump straight to cleanup had occurred. Maybe that can be made more obvious someway, I'll think about it. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

The name is confusing, and the only use left is in a test case. --- src/libvirt_private.syms | 1 - src/util/virpci.c | 8 -------- src/util/virpci.h | 1 - tests/virpcitest.c | 5 ++++- 4 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6d221de..3f3dbe9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1991,7 +1991,6 @@ virPCIDeviceListSteal; virPCIDeviceListStealIndex; virPCIDeviceNew; virPCIDeviceReattach; -virPCIDeviceReattachInit; virPCIDeviceReset; virPCIDeviceSetManaged; virPCIDeviceSetRemoveSlot; diff --git a/src/util/virpci.c b/src/util/virpci.c index 505c1f3..f56dbe6 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1771,14 +1771,6 @@ virPCIDeviceGetUsedBy(virPCIDevicePtr dev, *dom_name = dev->used_by_domname; } -void virPCIDeviceReattachInit(virPCIDevicePtr pci) -{ - pci->unbind_from_stub = true; - pci->remove_slot = true; - pci->reprobe = true; -} - - virPCIDeviceListPtr virPCIDeviceListNew(void) { diff --git a/src/util/virpci.h b/src/util/virpci.h index d1ac942..0debd7b 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -119,7 +119,6 @@ void virPCIDeviceSetRemoveSlot(virPCIDevice *dev, unsigned int virPCIDeviceGetReprobe(virPCIDevicePtr dev); void virPCIDeviceSetReprobe(virPCIDevice *dev, bool reprobe); -void virPCIDeviceReattachInit(virPCIDevice *dev); virPCIDeviceListPtr virPCIDeviceListNew(void); diff --git a/tests/virpcitest.c b/tests/virpcitest.c index fb0c970..6dceef2 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -301,7 +301,10 @@ testVirPCIDeviceReattachSingle(const void *opaque) if (!dev) goto cleanup; - virPCIDeviceReattachInit(dev); + virPCIDeviceSetUnbindFromStub(dev, true); + virPCIDeviceSetRemoveSlot(dev, true); + virPCIDeviceSetReprobe(dev, true); + if (virPCIDeviceReattach(dev, NULL, NULL) < 0) goto cleanup; -- 2.5.0

On 01/25/2016 11:21 AM, Andrea Bolognani wrote:
The name is confusing, and the only use left is in a test case. --- src/libvirt_private.syms | 1 - src/util/virpci.c | 8 -------- src/util/virpci.h | 1 - tests/virpcitest.c | 5 ++++- 4 files changed, 4 insertions(+), 11 deletions(-)
Seems reasonable - ACK John

On Tue, 2016-01-26 at 18:59 -0500, John Ferlan wrote:
On 01/25/2016 11:21 AM, Andrea Bolognani wrote:
The name is confusing, and the only use left is in a test case. --- src/libvirt_private.syms | 1 - src/util/virpci.c | 8 -------- src/util/virpci.h | 1 - tests/virpcitest.c | 5 ++++- 4 files changed, 4 insertions(+), 11 deletions(-)
Seems reasonable - ACK
This has been pushed. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

Unbinding a PCI device from the stub driver can require several steps, and it can be useful for debugging to be able to trace which of these steps are performed and which are skipped for each device. --- src/util/virpci.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index f56dbe6..51a3f88 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1106,26 +1106,37 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) if (!driver) { /* The device is not bound to any driver and we are almost done. */ + VIR_DEBUG("PCI device %s is not bound to any driver", dev->name); goto reprobe; } - if (!dev->unbind_from_stub) + if (!dev->unbind_from_stub) { + VIR_DEBUG("Unbind from stub skipped for PCI device %s", dev->name); goto remove_slot; + } /* If the device isn't bound to a known stub, skip the unbind. */ if (virPCIStubDriverTypeFromString(driver) < 0 || - virPCIStubDriverTypeFromString(driver) == VIR_PCI_STUB_DRIVER_NONE) + virPCIStubDriverTypeFromString(driver) == VIR_PCI_STUB_DRIVER_NONE) { + VIR_DEBUG("Unbind from stub skipped for PCI device %s because of " + "unknown stub driver", dev->name); goto remove_slot; + } - VIR_DEBUG("Found stub driver %s", driver); + VIR_DEBUG("Found stub driver %s for PCI device %s", driver, dev->name); + VIR_DEBUG("Unbinding PCI device %s", dev->name); if (virPCIDeviceUnbind(dev) < 0) goto cleanup; dev->unbind_from_stub = false; remove_slot: - if (!dev->remove_slot) + if (!dev->remove_slot) { + VIR_DEBUG("Slot removal skipped for PCI device %s", dev->name); goto reprobe; + } + + VIR_DEBUG("Removing slot for PCI device %s", dev->name); /* Xen's pciback.ko wants you to use remove_slot on the specific device */ if (!(path = virPCIDriverFile(driver, "remove_slot"))) @@ -1141,10 +1152,13 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) reprobe: if (!dev->reprobe) { + VIR_DEBUG("Reprobe skipped for PCI device %s", dev->name); result = 0; goto cleanup; } + VIR_DEBUG("Reprobing for PCI device %s", dev->name); + /* Trigger a re-probe of the device is not in the stub's dynamic * ID table. If the stub is available, but 'remove_id' isn't * available, then re-probing would just cause the device to be -- 2.5.0

On 01/25/2016 11:21 AM, Andrea Bolognani wrote:
Unbinding a PCI device from the stub driver can require several steps, and it can be useful for debugging to be able to trace which of these steps are performed and which are skipped for each device. --- src/util/virpci.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index f56dbe6..51a3f88 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1106,26 +1106,37 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
if (!driver) { /* The device is not bound to any driver and we are almost done. */ + VIR_DEBUG("PCI device %s is not bound to any driver", dev->name); goto reprobe; }
- if (!dev->unbind_from_stub) + if (!dev->unbind_from_stub) { + VIR_DEBUG("Unbind from stub skipped for PCI device %s", dev->name); goto remove_slot; + }
/* If the device isn't bound to a known stub, skip the unbind. */ if (virPCIStubDriverTypeFromString(driver) < 0 || - virPCIStubDriverTypeFromString(driver) == VIR_PCI_STUB_DRIVER_NONE) + virPCIStubDriverTypeFromString(driver) == VIR_PCI_STUB_DRIVER_NONE) { + VIR_DEBUG("Unbind from stub skipped for PCI device %s because of " + "unknown stub driver", dev->name); goto remove_slot; + }
- VIR_DEBUG("Found stub driver %s", driver); + VIR_DEBUG("Found stub driver %s for PCI device %s", driver, dev->name); + VIR_DEBUG("Unbinding PCI device %s", dev->name);
Redundant - How about "Found stub driver %s to unbind PCI device %s" or "Unbinding PCI device %s for stub driver %s" (don't forget to change order of args ;-)) IOW: No need to have two messages. Hope they help some day! ACK - John
if (virPCIDeviceUnbind(dev) < 0) goto cleanup; dev->unbind_from_stub = false;
remove_slot: - if (!dev->remove_slot) + if (!dev->remove_slot) { + VIR_DEBUG("Slot removal skipped for PCI device %s", dev->name); goto reprobe; + } + + VIR_DEBUG("Removing slot for PCI device %s", dev->name);
/* Xen's pciback.ko wants you to use remove_slot on the specific device */ if (!(path = virPCIDriverFile(driver, "remove_slot"))) @@ -1141,10 +1152,13 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
reprobe: if (!dev->reprobe) { + VIR_DEBUG("Reprobe skipped for PCI device %s", dev->name); result = 0; goto cleanup; }
+ VIR_DEBUG("Reprobing for PCI device %s", dev->name); + /* Trigger a re-probe of the device is not in the stub's dynamic * ID table. If the stub is available, but 'remove_id' isn't * available, then re-probing would just cause the device to be

On Tue, 2016-01-26 at 19:00 -0500, John Ferlan wrote:
- VIR_DEBUG("Found stub driver %s", driver); + VIR_DEBUG("Found stub driver %s for PCI device %s", driver, dev->name); + VIR_DEBUG("Unbinding PCI device %s", dev->name); Redundant - How about "Found stub driver %s to unbind PCI device %s" or "Unbinding PCI device %s for stub driver %s" (don't forget to change order of args ;-)) IOW: No need to have two messages.
What about "Unbinding PCI device %s (stub driver %s)"?
Hope they help some day!
They have certainly helped me while working on the rest of the series ;) I've tried to reply to each of your comments exactly once, please let me know if I've missed any in the process. Thanks again for taking the time to look at this. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

On Tue, 2016-01-26 at 19:00 -0500, John Ferlan wrote:
- VIR_DEBUG("Found stub driver %s", driver); + VIR_DEBUG("Found stub driver %s for PCI device %s", driver, dev->name); + VIR_DEBUG("Unbinding PCI device %s", dev->name);
Redundant - How about "Found stub driver %s to unbind PCI device %s" or "Unbinding PCI device %s for stub driver %s" (don't forget to change order of args ;-))
IOW: No need to have two messages.
Hope they help some day!
ACK -
I ended up using "Unbinding PCI device %s from stub driver %s" after all. Pushed. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
participants (2)
-
Andrea Bolognani
-
John Ferlan