On Thu, Jan 23, 2014 at 04:13:57PM -0500, Stefan Berger wrote: > On 01/23/2014 07:37 AM, Daniel P. Berrange wrote: > > Makes sense... there are other paths as well: > > - SIGHUP or restart of libvirt that recreates all filters > - late instantiation of filters following detection of VM's IP address Ok, yes, I see those now. I was struggling to understand just what codepaths could result in __virNWFilterInstantiateFilter being invoked so I generated a callgraph for that function http://berrange.fedorapeople.org/nwfilter.ps tracing the calls, confirms there are the 6 entry points - filter define - filter undefine - state reload on sighup or dbus notify - vm startup / hotplug - vm shutdown / hotunplug - dhcp / ip snooping the first 3 will require write locks, while the last three will only require read locks. I'm squashing the conversion to read/write lock into this patch, since otherwise the intermediate state would be deadlock prone. > > src/conf/nwfilter_conf.c::virNWFilterObjLoad : I don't see this > function grabbing the lock. I think this is missing. virNWFilterObjLoad is called by StateInitialize or StateReload and only the reload function requires the lock. So I'm adding the lock to that function > >diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c > >index 89913cf8..f0e78ed 100644 > >--- a/src/nwfilter/nwfilter_gentech_driver.c > >+++ b/src/nwfilter/nwfilter_gentech_driver.c > >@@ -935,7 +935,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, > > int ifindex; > > int rc; > > > >- virNWFilterLockFilterUpdates(); > > > > /* after grabbing the filter update lock check for the interface; if > > it's not there anymore its filters will be or are being removed > >@@ -964,7 +963,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, > > foundNewFilter); > > > > cleanup: > >- virNWFilterUnlockFilterUpdates(); > > > > return rc; > > } > > This function is called by virNWFilterInstantiateFilter and > virNWFilterUpdateInstantiateFilter. So, in the former case this is > called when a VM is started, in the latter case if VMs' filters are > updated while the VM is running. I think you are covering the VM > creation case with the above calls for lxc and further below with > the changes to the qemu driver and the uml driver. > > There is at least one other part that needs to be covered: > > src/conf/nwfilter_conf.c::virNWFilterInstFiltersOnAllVMs :: kicked > off by nwfilterStateReload upon SIGHUP. We need a lock there now. > > src/conf/nwfilter_conf.c::virNWFilterTriggerVMFilterRebuild:: > - called by virNWFilterTestUnassignDef: > - called by src/nwfilter/nwfilter/nwfilterUndefine:: You > seem to just reorder some locks there; now we need a (writer) lock > there > - called by virNWFilterObjAssignDef: which must be called with > lock called following above reasoning Yep, I concur and have added lock calls to the StateReload function > >@@ -984,8 +982,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, > > int rc; > > bool foundNewFilter = false; > > > >- virNWFilterLockFilterUpdates(); > >- > > rc = __virNWFilterInstantiateFilter(driver, > > vmuuid, > > true, > >@@ -1009,8 +1005,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, > > } > > } > > > >- virNWFilterUnlockFilterUpdates(); > >- > > return rc; > > } > > This function here is called by > src/nwfilter/nwfilter_learnip.c::learnIPAddressThread > src/nwfilter/nwfilter_dhcpsnoop.c::virNWFilterSnoopIPLeaseInstallRule > > They instantiate the filters once a VM's IP address has been > detected. So this is where the *Late() comes from. > > If you remove the locking from here, you have to lock it there. > Considering what you do layer, I would keep the lock here and > convert this into a reader lock layer on. Yes, now I'm squashing the read/write lock conversion in, I'll keep the locking in this location. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list