Re: [libvirt] [GSoC] Abstracting device address allocation

On 06/08/2016 02:04 PM, Tomasz Flendrich wrote: I know you took this sentence straight from the wiki page of GSoC ideas, but I'd be interested to see a few specific examples of the problems that you intend to solve (maybe Martin can help here, since I think he's the one who wrote that). I know of a few problems with PCI address allocation, but abstracting the data structure and functions used for allocation aren't going to solve them (for example, if you attach a device that uses PCI with "virsh attach-device .... --live", then then add another with "virsh attach-device --live --config", the new device will be assigned a different address in the persistent config than in the live domain (the result will be that the next time you shutdown and restart, the device will appear at a different address, which could confuse the guest OS). Another (which I need to fix *very* soon) is that even when a device is classified as connecting to PCIe slots rather than PCI (and there aren't enough of those right now, which is *yet another* problem I need to fix), if there aren't any empty PCIe slots available to assign, we'll fail rather than adding a new controller of the appropriate type and attached to the appropriate existing controller). But, as I said, a new data structure isn't going to solve any of those problems (and I'm not sure it will make the solution any easier to get to either). PCI addresses are likely much more complicated than you've planned for this. For starters, each PCI bus can have a different number of slots available (the 2nd dimension, which you list as {0..31}, has different start & end indexes depending on the type of controller that provides that bus. (e.g. pci-root, pcie-root, and pci-bridge have 31 slots from 1-31, dmi-to-pci-bridge and pcie-switch-upstream-port have 32 slots from 0-31, pcie-root-port and pcie-switch-downstream-port each have a single slot at 0,), and each bus has different rules about what devices can be plugged into them (e.g. a pcie-root-port can only be plugged into pcie-root or a pcie-switch-downstream-port, some devices can only be plugged into pci-root, pci-bridge, or dmi-to-pci-bridge, some can be plugged into pcie-root directly if specifically requested, but it's otherwise avoided, etc). So it's not just a case of a 4-dimensional vector with the same size at each index on on the same axis, and each address is not the same as any other address. Are you intending for this allocator to have any sort of "Give me the next free address" function? If so, doing this for PCI is likely to require you to add lots of extra complexity that will be completely unnecessary for the other address types (although... there are several different kinds of SCSI controller (including virtio-scsi as well as several emulations of real hardware) that all share the same address space, and we need to know what model is each controller so that the user can be sure devices are placed on the desired controller.) Of course maybe your intent is that your allocator will only implement an "allocate this specific address" function, and leave auto-assignment up to a higher layer; but then the higher layer would still need a way to efficiently scan the entire list of free addresses, and each address would need a bit of data describing the controllers at that address so they they could be matched. (Another thing - at any time a new bus of just about any type can be added, sometimes in response to a request to allocate an address for which no matching slot is available (e.g. - adding a new pcie-root-port, pcie-switch-upstream-port, and list of pcie-downstream-ports when there are no more hotpluggable PCIe slots available)). Again, this doesn't allow for some of the indexes on a particular dimension having a different start/end (or for other differences for a particular index, which possibly could be stored in some opaque pointer). Ah, there it is! This won't work for PCI (or for scsi) without much more information available for the ranges that are being allocated from. Disk device names are incredibly ugly beasts. Apparently once you get to sdz, you can start over at sdaa, sdab, etc. And the whole thing is pointless anyway, because that name isn't passed directly to the guest anyway (there isn't a way to do that - the guest OS determines device names on its own based on the bus and order in which the disks appear). ...which isn't all that much, since we aren't talking about microcontrollers with tiny amounts of RAM. Without looking at the implementation, I'm concerned that this may be adding lots of extra code complexity in order to save just a few bytes of RAM, which realistically is pointless - we only have one pool of PCI addresses per active domain, so the amount of memory used to store a bitmap of the availability of addresses is inconsequential in relation to the total memory requirement for a domain. I do see the usefulness of this in general, but in this particular case rather than reserving an entire slot at once, we may want to allocate a single function where the device is going to be, but mark the rest of that slot available for further functions to be allocated, then later mark it unavailable for further allocations *without actually allocating the rest of the functions in the slot*. The reason for this is that it would make it simpler to use the same code for detaching a "lone" device on a slot as we do for detaching the functions of a multifunction PCI device. (You're likely thinking "what's the difference between allocating a single function + mark the other functions on the slot as unavailable vs. just marking the entire slot as allocated; it all has to do with being able to easily notice when the slot is once again completely availaable - if you mark all the functions in the slot as used, then when you free it you have to free all the slots; if you're doing that, then you have to figure out whether the device you'r freeing was one function of a multifunction device, or if it was a lone device plugged into a slot by itself. Of course that may not fit in with your abstracted view of generic address allocation.) Right now we "ham fist" this issue by just marking the entire slot as allocated when someone asks for a single function, but really we should do allocations (and frees) one function at a time, but just with a knowledge of when it's appropriate to allocate a 2nd (and further) function on a slot that already has some functions allocated, and when it isn't. Again, my concern would be about making something more complex than necessary for the individual uses and their requirements. Eliminating nearly duplicate code is good, but you need to be careful that what you create doesn't require more "glue" to make each of the cases fit into the abstract model than would have been used for a separate simpler "from-scratch" implementation for each individual case. Yes. And we need to be aware that sometimes devices are added to the live domain but not the persistent config, sometimes only to the persistent config, and sometimes to both (and when they're added to both, they should use the same address in both live domain and persistent config, which isn't always the case today). You had said you wanted the qemu_* files to remain the same for now. Exactly what are you figuring on replacing? For example, for PCI addresses (since these are obviously the only ones I care about :-)) are you just planning to replace virDomainPCIAddressBus with your new virAllocator and a set of accessor functions? Even if you do nothing higher level than this, each bus will need to have a "connectFlags" that describes what type of devices can be connected to that bus (or range, or whatever you call it). Hopefully my comments haven't just made the situation more muddy :-)