Re: [libvirt PATCH v3 00/21] Add support for persistent mediated devices

On Thu, Dec 24, 2020 at 08:14:24AM -0600, Jonathon Jongsma wrote: > This patch series follows the previously-merged series which added support for > transient mediated devices. This series expands mdev support to include > persistent device definitions. Again, it relies on mdevctl as the backend. > > It follows the common libvirt pattern of APIs by adding the following new APIs > for node devices: > - virNodeDeviceDefineXML() - defines a persistent device > - virNodeDeviceUndefine() - undefines a persistent device > - virNodeDeviceCreate() - starts a previously-defined device > > It also adds virsh commands mapping to these new APIs: nodedev-define, > nodedev-undefine, and nodedev-start. > > Since we rely on mdevctl for the definition of ediated devices, we need a way > to stay up-to-date with devices that are defined by mdevctl (outside of > libvirt). The method for staying up-to-date is currently a little bit crude > due to the fact that mdevctl does not emit any events when new devices are > added or removed. As a workaround, we create a file monitor for the mdevctl > config directory and re-query mdevctl when we detect changes within that > directory. In the future, mdevctl may introduce a more elegant solution. > > changes in v3: > - streamlined tests -- removed some unnecessary duplication > - split out patch to factor out node device name generation function > - split nodeDeviceParseMdevctlChildDevice() into a separate function > - added follow-up patch to remove space-padded alignment in header > - refactored the mdevctl update handling significantly: > - no longer a separate persistent thread that gets signaled by a timer > - now piggybacks onto the existing udev thread and signals the thread in t= > he > same way that the udev event does. > - Daniel suggested spawning a throw-away thread to handle mdevctl updates, > but that introduces the complexity of possibly serializing multiple > throw-away threads (e.g. if we get an 'created' event followed immediate= > ly > by a 'deleted' event, two threads may be spawned and we'd need to ensure > they are properly ordered) > - added virNodeDeviceObjListForEach() and virNodeDeviceObjListRemoveLocked() > to simplify removing devices that are removed from mdevctl. > - coding style fixes > - NOTE: per Erik's request, I experimented with changing the way that mdevctl > commands were generated and tested (e.g. introducing something like > virMdevctlGetCommand(def, MDEVCTL_COMMAND_<SUBCOMMAND>, ...)), but it was > too invasive and awkward and didn't seem worthwhile Thanks for giving ^this a try :) I think we're converging nicely with the only remaining bit to agree on being the mdevctl monitoring thread. I think we can target 7.1.0 with this series.