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
--
|: 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