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 ?
> I also hadn't looked at this patch in the first round...
>
> >former is going to be turned into a read/write lock
> >instead of a mutex, a new lock is required to serialize
> >access to the firewall itself.
> >
> >With this new lock, the lock ordering rules will be
> >for virNWFilter{Define,Undefine}
> >
> > 1. nwfilter driver lock
> > 2. nwfilter update lock
>
>
> Insert: 3. nwfilter callback drivers lock
>
> This is then used in this order also by nwfilterStateReload
>
>
> > 3. virt driver lock
> > 4. domain object lock
> > 5. gentech driver lock
> >
> >and VM start
> >
> > 1. nwfilter update lock
> > 2. virt driver lock
> > 3. domain object lock
> > 4. gentech driver lock
> >
> >Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> >---
> > src/nwfilter/nwfilter_driver.c | 4 +++-
> > src/nwfilter/nwfilter_gentech_driver.c | 19 +++++++++++++++++--
> > src/nwfilter/nwfilter_gentech_driver.h | 2 +-
> > 3 files changed, 21 insertions(+), 4 deletions(-)
> >
> >diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c
> >index 89913cf8..d500963 100644
> >--- a/src/nwfilter/nwfilter_gentech_driver.c
> >+++ b/src/nwfilter/nwfilter_gentech_driver.c
> >@@ -936,6 +943,7 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
> > int rc;
> >
> > virNWFilterLockFilterUpdates();
> >+ virMutexLock(&updateMutex);
>
>
> Since the filter updates lock had the two purposes before, you are
> now introducing a separate lock to assign a purpose to each lock.
> Further below you are preventing concurrent teardowns to this here.
>
> I am wondering how much further down this lock here could actually
> be pushed. This and the other function
> (virNWFilterInstantiateFilterLate) where you place this lock are
> calling __virNWFilterInstantiateFilter and nothing else calls that
> function [and the filter read protection above the lock call will
> remain]. So I think this lock could be placed inside
> __virNWFilterInstantiateFilter(). Also looking at that function I am
> not sure whether there is anything worth protecting using this newly
> introduced lock then. It ends up calling virNWFilterInstantiate().
> Here I would be a bit careful with the threads being started to
> learn the IP addresses. So maybe this function could be the place
> where to serialize access. What's your take?
The callgraph showed the three entry points into this area of
code look like:
virNWFilterInstantiateFilterLate virNWFilterInstantiateFilter virNWFilterTeardownFilter
| | |
+-------------------------|-----+ |
| +------------+ | |
| | | |
V V V V
_virNWFilterInstantiateFilter _virNWFilterTeardownFilter
Looking at the code I don't see how it is safe to allow teardown to
happen in parallel with instantiate. The teardown code could confuse
and conflict with the instantiate code causing it to fail I believe.
In particular a VM could exit causing QEMU to request teardown, while
the DHCP snoop thread is in the middle of re-creating the iptables
ruleset for a VM, so we surely require serialization of modifications
to iptables.
I could, push the locking down one level, but it wouldn't change the
level of serialization, and the lock calls are clearer at the level
of the code I have them I believe.
Regards,
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