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

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.