On 01/28/2014 07:38 AM, Daniel P. Berrange wrote:
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.
I definitely hope so.
Regards,
Stefan
Daniel
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list