Re: [PATCH v2 7/9] hostdev: Update comments

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]