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);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list