Re: [libvirt] [PATCH v2 3/4] Add a mutex to serialize updates to firewall

On Tue, Jan 28, 2014 at 07:31:04AM -0500, Stefan Berger wrote: > On 01/28/2014 06:15 AM, Daniel P. Berrange wrote: >> On Mon, Jan 27, 2014 at 04:50:56PM -0500, Stefan Berger wrote: >>> On 01/27/2014 12:18 PM, Daniel P. Berrange wrote: >>>> The nwfilter conf update mutex previously serialized >>>> updates to the internal data structures for firewall >>>> rules, and updates to the firewall itself. Since the >>> Hm, wasn't aware (anymore) of this double-purpose. >> It wasn't entirely clear to me either, but I am right in >> believing that it isn't safe to have multiple threads all >> creating iptables rules for different VMs at the same >> time, aren't I ? > At least one thread shouldn't try to instantiate filters for one > (VM) interface while another one tears them down. So that would be > serialization per interface. I think instantiation of filters could > be done concurrently for different interfaces, but not the execution > of the iptables commands themselves, though. The latter is locking > that needs to be done by the ebtables/iptables driver and that > driver does serialize the execution of all scripts using the > execCLIMutex. The ebtables and iptables rules are created on a > per-interface basis, all hooking into some form of 'root' chains. > The critical part here is that the 'root' chains can be accessed > concurrently by different interfaces -- from what I can tell is that > all the scripts that are run by the eb/iptables driver only need to > be serialized and that is done with that execCLIMutex. So we should > be fine from that perspective. > > At least locking on a per-interface basis is already happening in > the 'gentech' driver: > > > nwfilter_gentech_driver.c::virNWFilterInstantiate > > [...] > if (virNWFilterLockIface(ifname) < 0) > goto err_exit; > > rc = techdriver->applyNewRules(ifname, nptrs, ptrs); > > if (teardownOld && rc == 0) > techdriver->tearOldRules(ifname); > > if (rc == 0 && (virNetDevValidateConfig(ifname, NULL, > ifindex) <= 0)) { > virResetLastError(); > /* interface changed/disppeared */ > techdriver->allTeardown(ifname); > rc = -1; > } > > virNWFilterUnlockIface(ifname); > [...] > > > > nwfilter_gentech_driver.c::_virNWFilterTeardownFilter > > [...] > if (virNWFilterLockIface(ifname) < 0) > return -1; > > techdriver->allTeardown(ifname); > > virNWFilterIPAddrMapDelIPAddr(ifname, NULL); > > virNWFilterUnlockIface(ifname); > [...] > > > (Besides the above calls to the 'techdriver', there are others that > call into the techdriver during the test phases of a filter updated. > They hold the writer lock to the filter updates and with this block > every concurrent thread then.) > > I may be missing something subtle, but I think there is already > enough serialization happening per interface. Ok, I think you might be right then, and we can just skip this patch entirely and rely in the ifname locks.

Daniel