Re: [libvirt] [PATCH 22/24] hostdev: Streamline device ownership tracking

On Sun, 2016-03-13 at 10:06 -0400, John Ferlan wrote: I'll be honest: if we can agree that the code, with this patch applied, is correct, I see little value in splitting it further. It's not like we're really going to want to have just *some* of these changes but not the rest... They're very much connected. Fair enough :) But you'll see that those patches are absolutely trivial - this patch right here is the whole point of the series, and it's all smooth sailing from here on :) Yay :) This is in no way guaranteed to be the case. The inactive list keeps track of all inactive devices, including the ones that have manually detached from the host by the user and that we have no intention of ever attaching to a guest. Consider the case of an Ethernet adapter with 4 ports that are presented as 4 separate PCI devices, all in the same IOMMU group: if the user wants to pass only a single port to the guest, it will detach all of them from the host using 'virsh nodedev-detach' or similar, and then call 'virsh attach-device' a single time. The three remaining devices will stay in the inactive list until the user calls 'virsh nodedev-reattach' on them. Another example: the user might decide, for whatever reason, to detach a PCI device from the host, and then proceed to assign a completely unrelated host PCI device to a guest, leaving the previous one unassigned. Before the changes, objects from 'pcidevs' were *added* to the active list, so it was critical that 'pcidevs' was emptied here: if that didn't happen, the call to virObjectUnref() would have destroyed not only the device list, but the devices themselves, rendering the contents of the active list partially invalid. After the changes, the objects in 'pcidevs' are *always* disposable: when devices are detached from the host, they are *copied* instead of added to the inactive list, and later on those *copies* are *moved* to the active list. So we no longer need to be careful here: all the data we care about is either in the active list or the inactive list, and 'pcidevs', along with all its contents, can be released on a whim in the cleanup path. There's more on this below. The first one would not really be a failure, though - we might have processed only part of the devices before encountering an error, so it's totally possible that some of them have not been added to the active list yet. As for the second one... There really isn't much you can do if your memory allocation fails and you're already in the error path, is there? :( I'd rather keep it for clarity's sake. Right. virPCIDeviceReattach() only checks whether the device is on the active list for safety purposes; you still need to pass in an actual device (eg. instance taken from one of the bookkeeping lists, the inactive list in this case) because virPCIDeviceUnbindFromStub(), which performs the actual task of reattaching the device to the host, needs to have information such as unbind_from_stub, remove_slot and reprobe, which had been stored in the actual device by virPCIDeviceDetach(). This used to take care of the unmanaged devices by adding them to the inactive list and returning early to skip the call to virPCIDeviceReattach() later on. Now we take care of that in the caller, ReAttachPCIDevices(), by moving *all devices* from the active list to the inactive list, regardless of whether they were managed or not, and only calling ReattachPCIDevice() on managed devices. The end result is the same. My reply is below as well :) There is no such rule or assumptions: both managed and unmanaged devices belong to the active list. The only criteria for being on the active list is, well, being active, ie. assigned to a guest. After the changes, step 1 makes sure that 'pcidevs' (our working set) only contains (pointers to) devices that were both *active* and *used by the correct domain and driver* to begin with, which is what we expect to be dealing with in the rest of the function. Actually, this was already the case as of patch 15, and the logic was the same even before that. The only change here is splitting the code that changes / validates 'pcidevs' from the one that changes the contents of the active list. Yeah, thanks for spotting that. I'll add error checking. This might be a nice idea for further improvement, although it might not matter as much once the code is refactored as originally outlined in [1], which is going to be my next step. Yay :) For consistency, and because as explained above some of the devices in the inactive list might be completely unrelated to the task at hand. Not true, there can definitely be unmanaged devices. And we can't just loop over the inactive list because of the reasons explained above. virHostdevReattachPCIDevice() calls virPCIDeviceReattach(), which takes care of removing the device from the inactive list and releasing the associated memory. There is no memory leak: virObjectUnref() is now called unconditionally on 'pcidevs', and that takes care of releasing both the container itself *and* the instances that had been added to it. As explained above, the original code allowed the same instance to be part of two different lists at a time, 'pcidevs' and one of the bookkeeping lists. (Both of them, in some cases, but that was just for a limited period of time during state transitions.) What the new code does is regard all objects in 'pcidevs' as eminently disposable, and store the actual objects in the bookkeeping lists *every single time*. Each virPCIDevice instance, be it valuable or not, is only ever in a single list at a time. The downside is that we have to copy objects around slightly more frequently - even though some other changes might very well offset that overhead already - but the upside is that we can be way more ham-fisted when it comes to deleting stuff, which is exactly what we're doing here. It doesn't matter what's inside 'pcidevs', it's always going to be stuff we already have an authoritative copy of somewhere else, so let's just release it in the cleanup path and forget about it. Way easier to reason about, at least for my brain :) Thanks for getting this far - keeping a message this long understandable is pretty challenging, hopefully I've done a somewhat decent job at it. I believe there was some disconnect between the role I intended a variable to have and the role you understood they would have, and that caused lots of head scratching on your part... This is something I tried to address with patch 19, but I can see it was not as effective as I hoped it would be. Any suggestions on how to improve the situation? Cheers. [1] https://www.redhat.com/archives/libvir-list/2016-January/msg01066.html --  Andrea Bolognani Software Engineer - Virtualization Team