[libvirt] [PATCH 20/24] hostdev: Save netdev configuration of actual device

[Andrea Bolognani]

Word of warning, since this is very likely to get extremely
verbose: I fully expect to slip, here and there, and refer not
to the *current* situation as of this patch, but to the
*desired* situation at the end of the series.

Such *desired* situation is (not explicitly enough?) laid out
in patch 19, but I'll expand on it here for clarity:

  * the active list and inactive list contain the actual devices

  * each device is contained either in the active list, or in
    the inactive list. It can't be in both

  * a device that's neither in the active list nor in the
    inactive list is assigned to the host

  * for each PCI device, only the virPCIDevice instance that is
    part of one of the bookkeeping lists matter

  * when an operation is to be performed on a PCI device, the
    relevant virPCIDevice instance must be looked up in the
    appropriate bookkeeping list

  * virPCIDevice instances in 'pcidevs' are only used for
    looking up actual devices in the bookkeeping lists, contain
    no valuable data and are disposable at any time

  * exceptions to the above points are to be documented

Hopefully this makes it easier to see the meaning behind some
of the changes :)


On Fri, 2016-03-11 at 14:40 -0500, John Ferlan wrote:
> On 03/10/2016 06:02 PM, John Ferlan wrote:
> > On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
> So I went through this one again (new day, fresh start, partially clear
> mind).

That often helps :)

> First off - perhaps something for the previous documentation patch -
> PrepareDevices is called during the qemuProcessLaunch processing (eg
> domain startup) for all known host devices. It's also called during
> qemuDomainAttachHostPCIDevice or libxlDomainAttachHostPCIDevice
> (hotplug) for the *one* new device to be attached.
> 
> The purpose of the function is to take the passed hostdev list, move the
> devices to a managed active host device list for the guest. Devices that
> do not have the managed attribute are not processed.

That's not true, though: unmanaged devices *are* processed, even
if less operations are performed on them. The part about
detaching them from the host is skipped, because it's supposed
to have been performed by the user alread, but that's it;
everything else (resetting them, moving them to the appropriate
bookkeeping list, storing information about what driver and
domain is using them) is just the same as managed devices.

> > > @@ -650,7 +650,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
> > >  
> So after step 6, all the pcidevs are on the activePCIHostdevs list.
> Theoretically at least ;-).  None are on the inactivePCIHostdevs list.
> 
> Why in step 7 do we run through pcidevs, to find the device on the
> activePCIHostdev list in order to set the UsedBy of the actual device on
> the active list. Why not run the 'activePCIHostdev' list here too?
> 
> Since we really don't need pcidevs any more - can we delete it now?
> E.G. Step 9 it... Steps 7 & 8 don't really need pcidevs, right?  There
> is no error path for either.  I'm not asking for a patch - just making
> sure I'm reading things right!

I guess we *could* get rid of it early (even though I haven't
really checked to make sure it would work), but doing so wouldn't
give us any advantage IMHO.

As it is now (well, not as of patch 20, but as of patch 22 which
is basically what this whole series is building towards) 'pcidevs'
is used only for iteration, and iteration is performed only over
'pcidevs'. This is exactly the kind of straightforward knowledge
we need to hold on to if we are ever to get this code in a
manageable state.

Examples of stuff that is easier to reason about if we keep
'pcidevs' around and only ever iterate over it: to obtain an
actual device, we *always* need to look it up in the appropriate
list, no ambiguity; in the cleanup path, we can *always* just
destroy all of 'pcidevs', because no valuable data is stored in
there.

So, even if it might mean performing a couple more lookups here
and there, I think the separation between 'pcidevs' (a list of
instances we can use to look up the actual data) and the actual
bookkeeping lists (where the data we care about is stored) is
crucial and should be enforced.

> > >      /* Step 8: Now set the original states for hostdev def */
> > >      for (i = 0; i < nhostdevs; i++) {
> > > -        virPCIDevicePtr pci;
> > > +        virPCIDevicePtr actual;
> > >          virDomainHostdevDefPtr hostdev = hostdevs[i];
> > >          virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
> > >  
> > > @@ -659,24 +659,27 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
> > >          if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
> > >              continue;
> > >  
> > > -        pci = virPCIDeviceListFindByIDs(pcidevs,
> > > -                                        pcisrc->addr.domain,
> > > -                                        pcisrc->addr.bus,
> > > -                                        pcisrc->addr.slot,
> > > -                                        pcisrc->addr.function);
> > > +        /* We need to look up the actual device because it's the one
> > > +         * that contains the information we care about (unbind_from_stub,
> > > +         * remove_slot, reprobe) */
> >
> > When a device goes from the inactivePCIHostdevs list to the
> > activePCIHostdevs list "at some point in time" in the future - does it
> > do a similar save?
> 
> I'll answer my own question - because this is the Prepare function and
> we don't have to worry about it since everything is on the active list.

Exactly, all devices that go from the inactive list to the active
list do so via this function... We don't need to handle this
anywhere else. And by the time this specific lookup is performed,
all devices have already been moved to the active list (step 5).

> > That is this change only grabs devices from the active list for the
> > save; whereas, prior to this change it would pull from all pcidevs
> > 
> So, I believe this part is correct albeit a bit confusing...

There are *no* devices that are in 'pcidevs' but *not* in the
active list by this point - again, all objects in 'pcidevs' are
used (or supposed to be used) for lookup purposes only.

I wonder if a better name would help making this easier to keep
in mind even for someone who's looking at the code for the first
time, or even myself two months from now :)

What about 'tmpDevices'? 'lookupDevices'?

> > > +        actual = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs,
> > > +                                           pcisrc->addr.domain,
> > > +                                           pcisrc->addr.bus,
> > > +                                           pcisrc->addr.slot,
> > > +                                           pcisrc->addr.function);
> > >  
> > >          /* Appropriate values for the unbind_from_stub, remove_slot
> > >           * and reprobe properties of the device were set earlier
> > >           * by virPCIDeviceDetach() */
> > > -        if (pci) {
> > > +        if (actual) {
> > >              VIR_DEBUG("Saving network configuration of PCI device %s",
> > > -                      virPCIDeviceGetName(pci));
> > > +                      virPCIDeviceGetName(actual));
> > >              hostdev->origstates.states.pci.unbind_from_stub =
> > > -                virPCIDeviceGetUnbindFromStub(pci);
> > > +                virPCIDeviceGetUnbindFromStub(actual);
> > >              hostdev->origstates.states.pci.remove_slot =
> > > -                virPCIDeviceGetRemoveSlot(pci);
> > > +                virPCIDeviceGetRemoveSlot(actual);
> > >              hostdev->origstates.states.pci.reprobe =
> > > -                virPCIDeviceGetReprobe(pci);
> > > +                virPCIDeviceGetReprobe(actual);
> > >          }
> > >      }
> > >  
> > > @@ -844,17 +847,17 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
> More documentation thoughts... Because what exists now for the function
> doesn't really make sense - it's only describing one variable.
> 
> Anyway, the purpose of this function is to take the device(s) passed on
> the hostdev list and return them to the host (?if they are managed?).
> This function is called either from hotplug device remove, hotplug
> device attach failure path, or from qemuProcessStop during process
> shutdown.  When called from the host device remove or attach failure
> paths, there is only 1 hostdev in the list.

More documentation is definitely always a good thing, and this pretty
much sums up what the function does :)

> Prior to patch 15, the pcidev's was a copy of the "active list", now
> it's a list of all pcidevs found. Hopefully on either one of the two
> lists. That is I hope a found hostdev couldn't be on neither list.

See above: it's not a list of all PCI devices, just a list of PCI
devices we're going to detach from the guest (and possibly reattach
to the host) at this particular time.

> Prior to patch 15, step1 would either remove the device from pcidevs or
> from the activeList depending on the matching driver name. When done,
> the pcidevs would contain the list of devices just removed from the
> activeList. Note that virPCIDeviceListDel will repeat the active list
> virPCIDeviceListFind (essentially)...
> 
> After patch 15, the pcidevs gets reduced for devices not on the
> activeList or without a matching drv/dom name. Whether or not that is on
> the inactiveList isn't checked, but assumed.
> 
> Step 2 is I believe the antecedent of step 4 during the Prepare path.
> Step 4 would be called after Reset and before placing devices on active
> list replacing the net config for any PCINet device...
> 
> Prior to patch 15, step2 would walk the hostdev's list looking for
> devices on the pcidevs list (eg. the ones removed from the activeList).
> It would then take any PCINet device and restore it's config.
> 
> After patch 15 and with these changes, walk the inactiveList and perform
> the same call. This is not the devices we just removed from the
> activeList, so I don't think using the inactiveList in this case is right.

Yeah, this is totally bogus.

Looking up the device in the inactive list is the right thing
to do *only after* patch 22 has been applied and step 2 is "Move
devices from the active list to the inactive list".

With that in place we are guaranteed that the actual device will
be in the inactive list, and the code above will be correct.

I believe I actually wrote this code after patch 22 and then
squashed it in patch 20 on a misguided attempt to present changes
in a more linear fashion :(

> Before patch 15, step 3 would take any of the devices we removed from
> the activeList and perform a reset on them.
> 
> After patch 15, step 3 would do a similar function assuming that no
> device could not be on neither list.

NO device could NOT be on NEITHER list... My head is spinning :)

Actually, when we get here the devices are *guaranteed* to be in
neither list, because we removed them from the active list in
step 1. But that was the case even before patch 15, so it should
not be a problem.

> Hopefully this all makes sense - I keep reading it and going back and
> fort between old and new code.  The comment left in the code after step
> 1 is most helpful...

I know how you feel! Thanks for sticking with it, hopefully the
comments I've provided so far (especially the ones at the top of
this message) are helpful in understanding the thought process
behind the changes.

I'll get back to you with more comments tomorrow - bet you can't
wait for them! ;)

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team