On 01/28/2014 07:38 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
On 01/28/2014 06:15 AM, Daniel P. Berrange wrote: 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
On Tue, Jan 28, 2014 at 07:31:04AM -0500, Stefan Berger wrote: patch entirely and rely in the ifname locks.
I definitely hope so. Regards, Stefan
Daniel