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

On 03/14/2016 02:00 PM, Andrea Bolognani wrote: I've rebased to top of tree after your recent pushes... Seems like a good place to reset focus! I think I need a virtual whiteboard. I understand/agree with the goal for actual devices in each list. One mental block is usage of virPCIDeviceListAdd for activeList and virPCIDeviceListAddCopy for inactiveList. The second mental block is whether these are for only managed devices (more on that later). Sure that's where you're headed, but with the current code this isn't necessarily true as a device could be on the inactiveList and activeList during patch 5, but that's the goal of patch 22. OK.. true... hopefully! right. sure, one would hope! Of course they need to be populated that way... This is where things start getting a bit fuzzy. I agree with the concept; however, the current implementation has a gotcha. Consider how VIR_APPEND_ELEMENT works. Initially 'pcidevs' is created as a new List, then for each 'hostdev' device, a new virPCIDevicePtr is created and added to the pcidevs list using virPCIDeviceListAdd. It's taken a bit to have VIR_APPEND_ELEMENT processing sink in for me ;-), but as I read the Prepare code it makes use of this processing model instead of creating a separate copy for the activeList. So here again, I'm trying to start fresh - go through each patch 1 by 1 so we can "agree" and move to the next one. Save a few electrons, too. Let's try to get to a point where patch22 can be further dissected. I think this is where the confusion for me lies. I seem to have also mistyped a bit - perhaps it's the blurred lines between current and future perfect (hah!) state. Initially an unmanaged device is not placed on the inactiveList (e.g. step2 in the PreparePCIDevices code), but it is placed on the activeList (step5). Later on, we can place an unmanaged device on the inactiveList either in virHostdevReattachPCIDevice using the "stolen" 'pcidevs' entry during virHostdevReAttachPCIDevices or virHostdevPCINodeDeviceDetach where virPCIDeviceDetach uses a copy of the device. Although patch22 removes the addition into the inactiveList if !managed (new step5 and deletion of lines in ReattachPCIDevice). Still leaves the NodeDeviceDetach path to understand from whence that request came. OK - it was a suggestion; however, as you'll see below I there's a caveat with this pcidevs list and the activeList... OK, understood - I agree. The extra lookups though can be expensive, but safety first. Right, but if I read patch22 correctly (jumping slightly ahead), only managed devices would be placed onto the activeList. Currently all pcidevs are moved to activeList, but your change there moves from inactiveList to activeList, where AFAICT the inactiveList only has managed devices. See my conundrum now? The good news is the fridge is stocked with cold ones to help me ;-) pcidevs is fine to me... it's just a list of PCI hostdevs. I've flushed patch 15 from local cache ;-) Yeah that makes perfect sense! No good deed goes unpunished ;-): As I read the current PreparePCIDevices code: Step1: Walk pcidevs, make some VFIO/ACS check... Make some PCI Node Device check... assume everything works as documented ;-) Step2: Walk pcidevs, if managed, call virPCIDeviceDetach to place *copy* of pci onto the inactive list, else VIR_DEBUG message Step3: Walk pcidevs, call virPCIDeviceReset on all devices (both those managed in inactiveList and those that are not managed) Step4: Walk hostdevs, SRIOV specific to replace network config (close my eyes, pray this works) Step5: Walk pcidevs, *adding* each device to activeList,. Since not copy, then device is on activeList *and* pcidevs at the same time, right? Step9 will "steal" from pcidevs to ensure virObjectUnref of pcidevs doesn't have virPCIDeviceListDispose free what's in activeList. (This is a key point...) Step6: Walk pcidevs, remove pci from inactiveList (free all memory of the copy initially placed on inactiveList). Step7: Walk pcidevs, if device found on activeList, call SetUsedBy Step8: Walk hostdevs, if device on pcidevs, then set unbind_from_stub, remove_slot, and reprobe (since everything on pcidevs is also in the activeList - magically the activeList data gets updated). Step9: Walk pcidevs, Stealing each element off of pcidevs (but doesn't virPCIDeviceFree the element so that virObjectUnref(pcidevs) won't find anything and do the free - because that element is on the activeList!). My notes (or what keeps bouncing around while working through this): 1. During the Prepare step, activeList add is not a copy, it uses VIR_APPEND_ELEMENT. IIUC, this means if 'dev' was dereferenced in virPCIDeviceListAdd after the APPEND, then there'd be a problem since dev would be set to NULL after the macro; however, since 'dev' is passed by value, that means the caller (e.g. the Prepare code) still can reference the element - it's just that the virPCIDevicePtr is listed in *two places*. You'll note other places in the code will Steal from one list and place on another, but this code doesn't do that until step9 where it steals all the pcidevs entries into apparently vaporware. "Theoretically" we really shouldn't reference activeList elements in pcidevs, but if we do reference them, then "magically" (so to speak) the activeList element would be updated. Personally it may be easier to use a *copy* for the activeList too and then let virObjectUnref handle the pcidevs entry deletion. 2. During Prepare, inactiveList add is done by copy (although one could argue that virPCIDeviceListAddCopy could be replaced by VIR_APPEND_ELEMENT_COPY I suppose). So now there are *two* copies of the same device - this is good and again, I think the model the activeList should use. 3. Current step9 just "steals" (or VIR_DELETE_ELEMENT) the element from pcidevs. This is "fine" since it's already/also on the activeList. You'll note other users of the macro would unref/free whatever memory was contained in the structure prior to removing from the list. Good thing we don't do that here ;-). If we don't steal the entry, then virObjectUnref(pcidevs) will call virPCIDeviceListDispose for us - your patch22 change to remove this may not be right... So given the current state of things, here's my view on the changes assuming my VIR_APPEND_ELEMENT viewpoint: Change #1: (what';s in the commit message) Alters step8 to use activeList instead of pcidevs. This part looks correct (perhaps worthy of it's own ACKable patch). Change #2: (not in the commit message) You jump into virHostdevReAttachPCIDevices and I then have to focus on a different algorithm and step process. Here's where I definitely believe you had patch22 in mind. But focusing on the code as it exists now... This code walks the hostdev's and will find anything on the pcidevs list which was on the former activeList and ensures the NetConfigRestore is run (e.g. the corollary of step4 in Prepare). Your change to ensure the device is on the inactiveList doesn't seem right at this point in time of the code. When you insert step2 in patch22, it would perhaps be more correct... Without peeking ahead too much, I think the insert unmanaged device onto inactiveList done in virHostdevReattachPCIDevice is incorrect at this point. Even if not incorrect, it's confusing. Thinking in terms of current code - step4 will take the everything that was formerly on the active list and: 1. Steal the pcidevs entry 2. Call virHostdevReattachPCIDevice with the stolen lookaside pcidevs entry... While it appears to be an actual, it isn't. Of course, I think you address that in patch 22, but trying not to get too far ahead. 2a. If the device is not managed, then add to the inactiveList and return (call virPCIDeviceFree if unsuccessfully add). What really confuses me here is why we place onto the inactiveList if not managed; however, you address that in patch22. Still it could be addressed earlier if that is a separate bug... 2b. If the device is managed, call virPCIDeviceReattach 2b1. Fail if on activeList (miserable) 2b2. Unbind stub 2b3. Call virPCIDeviceListDel -> Steal from inactiveList (search inactiveList for matching device by domain address, bus slot of the passed lookside pci device) -> call virPCIDeviceFree on the inactiveList entry 2c. Call virPCIDeviceFree on the lookaside list entry. 3. virObjectUnref(pcidevs) -> If I read the code right - each entry was stolen and properly removed so the virPCIDeviceListDispose has nothing to do. So it seems at this point, I would surmise virPCIDeviceReattach expects the pcidevs lookaside entry. The problem being insertion onto the inactiveList of an unmanaged device (something we avoided in Prepare). And yes, I'm still curious about virHostdevPCINodeDeviceDetach and how that plays into things. John