On Wed, Oct 06, 2010 at 05:56:36PM -0400, Stefan Berger wrote:
> V2:
> remove the locks from qemudVMFilterRebuild & umlVMFilterRebuild
[...]
> So, the following code paths exist towards
> qemu_driver:qemudVMFilterRebuild where we now want to put a
> qemu_driver lock in front of the filter lock.
>
> -> nwfilterUndefine() [ locks the filter ]
> -> virNWFilterTestUnassignDef()
> -> virNWFilterTriggerVMFilterRebuild()
> -> qemudVMFilterRebuild()
>
> -> nwfilterDefine()
> -> virNWFilterPoolAssignDef() [ locks the filter ]
> -> virNWFilterTriggerVMFilterRebuild()
> -> qemudVMFilterRebuild()
>
> -> nwfilterDriverReload()
> -> virNWFilterPoolLoadAllConfigs()
> ->virNWFilterPoolObjLoad()
> -> virNWFilterPoolAssignDef() [ locks the filter ]
> -> virNWFilterTriggerVMFilterRebuild()
> -> qemudVMFilterRebuild()
>
> -> nwfilterDriverStartup()
> -> virNWFilterPoolLoadAllConfigs()
> ->virNWFilterPoolObjLoad()
> -> virNWFilterPoolAssignDef() [ locks the filter ]
> -> virNWFilterTriggerVMFilterRebuild()
> -> qemudVMFilterRebuild()
>
> Qemu is not the only driver using the nwfilter driver, but also the
> UML driver calls into it. Therefore qemuVMFilterRebuild() can be
> exchanged with umlVMFilterRebuild() along with the driver lock of
> qemu_driver that can now be a uml_driver. Further, since UML and
> Qemu domains can be running on the same machine, the triggering of a
> rebuild of the filter can touch both types of drivers and their
> domains.
okay
> In the patch below I am now extending each nwfilter callback driver
> with functions for locking and unlocking the (VM) driver (UML, QEMU)
> and introduce new functions for locking all registered callback
> drivers and unlocking them. Then I am distributing the
> lock-all-cbdrivers/unlock-all-cbdrivers call into the above call
> paths. The last shown callpath starting with nwfilterDriverStart()
> is problematic since it is initialize before the Qemu and UML drives
> are and thus a lock in the path would result in a NULL pointer
> attempted to be locked -- the call to
> virNWFilterTriggerVMFilterRebuild() is never called, so we never
> lock either the qemu_driver or the uml_driver in that path.
> Therefore, only the first 3 paths now receive calls to lock and
> unlock all callback drivers. Now that the locks are distributed
> where it matters I can remove the qemu_driver and uml_driver lock
> from qemudVMFilterRebuild() and umlVMFilterRebuild() and not
> requiring the recursive locks.
okay,
> For now I want to put this out as an RFC patch. I have tested it by
> 'stretching' the critical section after the define/undefine
> functions each lock the filter so I can (easily) concurrently
> execute another VM operation (suspend,start). That code is in this
> patch and if you want you can de-activate it. It seems to work ok
> and operations are being blocked while the update is being done.
> I still also want to verify the other assumption above that locking
> filter and qemu_domain always has a preceding qemu_driver lock.
I looked at the patch and the only thing that need to be cleaned up
is the stretching blocki below, otherwise looks fine to me.
>
> +if (true) {
> + fprintf(stderr,"sleep in %s\n", __FUNCTION__);
> + sleep(CRITICAL_SECTION_STRETCH);
> + fprintf(stderr,"continue in %s\n", __FUNCTION__);
> +}
> +
It would be good to have some ways to exercise all code paths
in the locking (okay error handling specific paths aside because
it's really hard), before applying.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list