On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
> After this patch, ownership of virPCIDevice instances is very easy
> to keep track of: for each host PCI device, the only instance that
> actually matters is the one inside one of the bookkeeping list.
>
> Whenever some operation needs to be performed on a PCI device, the
> actual device is looked up first; when this is not the case, a
> comment explains the reason.
> ---
> src/util/virhostdev.c | 118 +++++++++++++++++++++++++++-----------------------
> 1 file changed, 65 insertions(+), 53 deletions(-)
>
FWIW: from your cover - splitting this up... I think it's possible that
the changes for calling virHostdevReattachPCIDevice with an actual
device could be in a separate patch... But I certainly see how this and
that are "tied" together especially if you're considering the failure
path through reattachdevs where you've definitely put things on the
inactiveList.
There may also be a 4 patches overall in here since I see the Reattach
code could place an unmanaged device on the inactiveList (which you
modify in this patch) and I think the ReAttach code adjustments could be
separate too.
I think you're on the right track, with all these changes - it's some
minor details. I've been looking at the changes, walking away, looking
at them, so hopefully the following is followable!
BTW: I chose not to look at the last 2 patches as I think there are too
many questions left over
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index d1529c5..b2f54cd 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -618,28 +618,22 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
> last_processed_hostdev_vf = i;
> }
>
> - /* Step 5: Now mark all the devices as active */
> + /* Step 5: Move devices from the inactive list to the active list */
> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
> + virPCIDevicePtr actual;
>
> - VIR_DEBUG("Adding PCI device %s to active list",
> + VIR_DEBUG("Removing PCI device %s from inactive list",
> virPCIDeviceGetName(pci));
> - if (virPCIDeviceListAdd(mgr->activePCIHostdevs, pci) < 0)
> - goto inactivedevs;
> - }
> -
> - /* Step 6: Now remove the devices from inactive list. */
> - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> - virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
> + actual = virPCIDeviceListSteal(mgr->inactivePCIHostdevs, pci);
>
> - VIR_DEBUG("Removing PCI device %s from inactive list",
> + VIR_DEBUG("Adding PCI device %s to active list",
> virPCIDeviceGetName(pci));
> - virPCIDeviceListDel(mgr->inactivePCIHostdevs, pci);
> + if (!actual || virPCIDeviceListAdd(mgr->activePCIHostdevs, actual) < 0)
> + goto inactivedevs;
> }
This is so much more logical to me than the previous code. One question
though - why not use virPCIDeviceListStealIndex? Theoretically the
inactiveList and the pcidevs list is the same right? So for every
devices on pcidevs - steal index 0 of inactiveList and place it on
activeList. It's just a slight optimization.
>
> - /* Step 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 pci, actual;
>
> @@ -655,7 +649,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
> virPCIDeviceSetUsedBy(actual, drv_name, dom_name);
> }
>
> - /* Step 8: Now set the original states for hostdev def */
> + /* Step 7: Now set the original states for hostdev def */
> for (i = 0; i < nhostdevs; i++) {
> virPCIDevicePtr actual;
> virDomainHostdevDefPtr hostdev = hostdevs[i];
> @@ -690,23 +684,25 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
> }
> }
>
> - /* Step 9: Now steal all the devices from pcidevs */
> - while (virPCIDeviceListCount(pcidevs) > 0)
> - virPCIDeviceListStealIndex(pcidevs, 0);
> -
Help me out... Why were these lines removed? The inactiveDevs is
initially a copy of whatever is on 'pcidevs'... Thus activeDevs takes
from there and not pcidevs, right?
> ret = 0;
> goto cleanup;
>
> inactivedevs:
> - /* Only steal all the devices from activePCIHostdevs. We will
> - * free them in virObjectUnref().
> - */
> + /* Move devices back to the inactive list so that they can be
> + * processed properly below (reattachdevs label) */
> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
> + virPCIDevicePtr actual;
>
> VIR_DEBUG("Removing PCI device %s from active list",
> virPCIDeviceGetName(pci));
> - virPCIDeviceListSteal(mgr->activePCIHostdevs, pci);
> + if (!(actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, pci)))
> + continue;
> +
> + VIR_DEBUG("Adding PCI device %s to inactive list",
> + virPCIDeviceGetName(pci));
> + if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, actual) < 0)
> + continue;
Another option is to VIR_WARN (INFO, DEBUG, whatever) the failure. You
may also want to clear the error as well (virResetLastError). There are
two possible errors - it already exists on the list and memory
allocation errors.
The continue is superfluous since this is the end of the loop - it only
matters if someone adds more checks afterwards
> }
>
> resetvfnetconfig:
> @@ -716,16 +712,22 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
> reattachdevs:
> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
> + virPCIDevicePtr actual;
>
> - if (virPCIDeviceGetManaged(pci)) {
> + /* We need to look up the actual device because that's what
> + * virPCIDeviceReattach() exepects as its argument */
"expects"
As I read it virPCIDeviceReattach look for the device on the activeList,
but will "blindly" remove from the inactiveList *if* provided. That's
only true for xen_driver and virpcitest.
I believe this process is the more correct one.
> + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
> + continue;
> +
> + if (virPCIDeviceGetManaged(actual)) {
> VIR_DEBUG("Reattaching managed PCI device %s",
> - virPCIDeviceGetName(pci));
> - ignore_value(virPCIDeviceReattach(pci,
> + virPCIDeviceGetName(actual));
> + ignore_value(virPCIDeviceReattach(actual,
> mgr->activePCIHostdevs,
> mgr->inactivePCIHostdevs));
> } else {
> VIR_DEBUG("Not reattaching unmanaged PCI device %s",
> - virPCIDeviceGetName(pci));
> + virPCIDeviceGetName(actual));
> }
> }
>
> @@ -745,17 +747,6 @@ static void
> virHostdevReattachPCIDevice(virHostdevManagerPtr mgr,
> virPCIDevicePtr actual)
> {
> - /* If the device is not managed and was attached to guest
> - * successfully, it must have been inactive.
> - */
> - if (!virPCIDeviceGetManaged(actual)) {
> - VIR_DEBUG("Adding unmanaged PCI device %s to inactive list",
> - virPCIDeviceGetName(actual));
> - if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, actual) < 0)
> - virPCIDeviceFree(actual);
> - return;
> - }
> -
Is it just me getting really confused - this code used to add devices to
the inactiveList even though (at some point in time) the Prepare code
would "drop" them? E.G. step 2 before any of these changes.
Was this perhaps what caused you to add that PCIDeviceListFind code in
step 2 of patch 21 that I questioned?
Perhaps this alone deserves it's own patch and I think is it's own
problem since the lookaside lists are for devices that are managed, right?
> /* Wait for device cleanup if it is qemu/kvm */
> if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) {
> int retries = 100;
> @@ -774,7 +765,6 @@ virHostdevReattachPCIDevice(virHostdevManagerPtr mgr,
> err ? err->message : _("unknown error"));
> virResetError(err);
> }
> - virPCIDeviceFree(actual);
Hmm... mental note - what changes here... to cause us not to need to
free.... Answered my question below [1] during cleanup.
> }
>
> /* @oldStateDir:
> @@ -836,17 +826,29 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
> continue;
> }
>
So step1 now removes all pcidevs that are on the inactive list and that
are unmanaged since "by rule" only a managed device could be on the
activeList. I think you need to update the comment for step1 in any
case since there's no removal from the activeList.
> + i++;
> + }
> +
> + /* Step 2: Move devices from the active list to the inactive list */
> + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
> + virPCIDevicePtr actual;
> +
> VIR_DEBUG("Removing PCI device %s from active list",
> virPCIDeviceGetName(pci));
> - virPCIDeviceListDel(mgr->activePCIHostdevs, pci);
> - i++;
> + actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, pci);
> +
> + if (actual) {
> + VIR_DEBUG("Adding PCI device %s to inactive list",
> + virPCIDeviceGetName(pci));
> + virPCIDeviceListAdd(mgr->inactivePCIHostdevs, actual);
Coverity notes that there's no error checking here like there is in
other calls to virPCIDeviceListAdd. Perhaps similar to step 5 in
prepare, we could use VIR_WARN.
I also think it might be worthwhile to make a common routine to perform
this step - the only difference being perhaps a 'return/fail on error'
flag that would be set for Prepare and clear for ReAttach
> + }
> }
>
> - /* At this point, any device that had been used by the guest is in
> - * pcidevs, but has been removed from activePCIHostdevs.
> - */
> + /* At this point, any device that had been used by the guest has been
> + * moved to the inactive list */
So yes, step 3 makes perfect sense now.
>
> - /* Step 2: restore original network config of hostdevs that used
> + /* Step 3: restore original network config of hostdevs that used
> * <interface type='hostdev'>
> */
> for (i = 0; i < nhostdevs; i++) {
> @@ -871,7 +873,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
> }
> }
>
> - /* Step 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 pci = virPCIDeviceListGet(pcidevs, i);
>
> @@ -888,16 +890,26 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
> }
> }
>
> - /* Step 4: reattach devices to their host drivers (if managed) or place
> - * them on the inactive list (if not managed)
> - */
> - while (virPCIDeviceListCount(pcidevs) > 0) {
> - virPCIDevicePtr pci = virPCIDeviceListStealIndex(pcidevs, 0);
> - virHostdevReattachPCIDevice(mgr, pci);
> + /* Step 5: Reattach managed devices to their host drivers; unmanaged
> + * devices don't need to be processed further */
Why use pcidevs? Why not just walk inactivePCIHostdevs?
> + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
> + virPCIDevicePtr actual;
> +
> + /* We need to look up the actual device because that's what
> + * virHostdevReattachPCIDevice() expects as its argument */
> + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
> + continue;
> +
> + if (virPCIDeviceGetManaged(actual))
> + virHostdevReattachPCIDevice(mgr, actual);
> + else
> + VIR_DEBUG("Not reattaching unmanaged PCI device %s",
> + virPCIDeviceGetName(actual));
If I've read things right - there shouldn't be any unmanaged device in
pcidevs at this point as they should have been removed during step1. In
any case, if you walk inactiveList, then all this is moot.
Also now that we're no longer managing the device it should be removed
from inactive and free'd.
IOW: THis loop becomes a while inactivePCIHostdevs has elements, call
Reattach, then free the device.
> }
>
> - virObjectUnref(pcidevs);
> cleanup:
[1]
There could be a memory leak here - I think we need to walk the
remaining pcidevs entry and free each entry before unreffing the
containing object. I think that's what that the previous step4 would do
with the removed virPCIDeviceFree after the ListStealIndex.
> + virObjectUnref(pcidevs);
> virObjectUnlock(mgr->activePCIHostdevs);
> virObjectUnlock(mgr->inactivePCIHostdevs);
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list