[libvirt] [PATCH] nwfilter: reorder locks
This patch reorders the locks for the nwfilter updates and the access the nwfilter objects. In the case that the IP address learning thread was instantiating filters while an update happened, the previous order lead to a deadlock. I am also adding a text file describing the locking order of the nwfilter subsystem. Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- src/conf/nwfilter_conf.c | 9 ++++---- src/nwfilter/locking.txt | 45 +++++++++++++++++++++++++++++++++++++++++ src/nwfilter/nwfilter_driver.c | 4 +++ 3 files changed, 54 insertions(+), 4 deletions(-) Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -2440,15 +2440,13 @@ virNWFilterTestUnassignDef(virConnectPtr { int rc = 0; - virNWFilterLockFilterUpdates(); - nwfilter->wantRemoved = 1; /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild(conn)) rc = 1; nwfilter->wantRemoved = 0; - virNWFilterUnlockFilterUpdates(); + return rc; } @@ -2480,8 +2478,9 @@ virNWFilterObjAssignDef(virConnectPtr co return NULL; } + virNWFilterLockFilterUpdates(); + if ((nwfilter = virNWFilterObjFindByName(nwfilters, def->name))) { - virNWFilterLockFilterUpdates(); nwfilter->newDef = def; /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild(conn)) { @@ -2498,6 +2497,8 @@ virNWFilterObjAssignDef(virConnectPtr co return nwfilter; } + virNWFilterUnlockFilterUpdates(); + if (VIR_ALLOC(nwfilter) < 0) { virReportOOMError(); return NULL; Index: libvirt-acl/src/nwfilter/nwfilter_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_driver.c @@ -372,6 +372,8 @@ nwfilterUndefine(virNWFilterPtr obj) { nwfilterDriverLock(driver); virNWFilterCallbackDriversLock(); + virNWFilterLockFilterUpdates(); + nwfilter = virNWFilterObjFindByUUID(&driver->nwfilters, obj->uuid); if (!nwfilter) { virNWFilterReportError(VIR_ERR_NO_NWFILTER, @@ -399,6 +401,8 @@ cleanup: if (nwfilter) virNWFilterObjUnlock(nwfilter); + virNWFilterUnlockFilterUpdates(); + virNWFilterCallbackDriversUnlock(); nwfilterDriverUnlock(driver); return ret; Index: libvirt-acl/src/nwfilter/locking.txt =================================================================== --- /dev/null +++ libvirt-acl/src/nwfilter/locking.txt @@ -0,0 +1,45 @@ +NWfilter locks: +--------------- + +The following NWFilter functions grab locks: + +- nwfilterDriverLock(driverState) + - protects driverState +- virNWFilterCallbackDriversLock() + - lock access to callback drivers +- virNWFilterLockFilterUpdates() + - prevent modification of any filter in use by any VM +- virNWFilterObjLock() + called by + - virNWFilterObjFindByUUID(), + - virNWFilterObjFindByName(), + - virNWFilterObjAssignDef() + and the functions return with the lock held + + +NWFilter-Lock ordering: +----------------------- + +Order in which the NWFilter lock functions have to be called: + +1. nwFilterDriverLock +2. virNWFilterCallbackDriversLock +3. virNWFilterLockFilterUpdates +4. virNWFilterObjLock + +Other locks: +------------ + +Other relevant locks to consider are related to hypervisor drivers. The +following locking orders are good with the qemu_driver as an example: + +qemu_driver - qemu_domain - filter object +qemu_driver - filter object - qemu_domain + +The qemu_driver serves as a general lock for all subsequent locks to a +qemu_domain. This works because the qemu_driver will always be locked +for a qemu_domain, thus only one lock-sequence of either (qemu_domain, +filter object) or (filter object, qemu_domain) can be active at any time. + +Reference: +https://www.redhat.com/archives/libvir-list/2010-October/msg00152.html
Any comments? Stefan On 05/11/2011 04:36 PM, Stefan Berger wrote:
This patch reorders the locks for the nwfilter updates and the access the nwfilter objects. In the case that the IP address learning thread was instantiating filters while an update happened, the previous order lead to a deadlock.
I am also adding a text file describing the locking order of the nwfilter subsystem.
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
--- src/conf/nwfilter_conf.c | 9 ++++---- src/nwfilter/locking.txt | 45 +++++++++++++++++++++++++++++++++++++++++ src/nwfilter/nwfilter_driver.c | 4 +++ 3 files changed, 54 insertions(+), 4 deletions(-)
Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -2440,15 +2440,13 @@ virNWFilterTestUnassignDef(virConnectPtr { int rc = 0;
- virNWFilterLockFilterUpdates(); - nwfilter->wantRemoved = 1; /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild(conn)) rc = 1;
nwfilter->wantRemoved = 0; - virNWFilterUnlockFilterUpdates(); + return rc; }
@@ -2480,8 +2478,9 @@ virNWFilterObjAssignDef(virConnectPtr co return NULL; }
+ virNWFilterLockFilterUpdates(); + if ((nwfilter = virNWFilterObjFindByName(nwfilters, def->name))) { - virNWFilterLockFilterUpdates(); nwfilter->newDef = def; /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild(conn)) { @@ -2498,6 +2497,8 @@ virNWFilterObjAssignDef(virConnectPtr co return nwfilter; }
+ virNWFilterUnlockFilterUpdates(); + if (VIR_ALLOC(nwfilter) < 0) { virReportOOMError(); return NULL; Index: libvirt-acl/src/nwfilter/nwfilter_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_driver.c @@ -372,6 +372,8 @@ nwfilterUndefine(virNWFilterPtr obj) { nwfilterDriverLock(driver); virNWFilterCallbackDriversLock();
+ virNWFilterLockFilterUpdates(); + nwfilter = virNWFilterObjFindByUUID(&driver->nwfilters, obj->uuid); if (!nwfilter) { virNWFilterReportError(VIR_ERR_NO_NWFILTER, @@ -399,6 +401,8 @@ cleanup: if (nwfilter) virNWFilterObjUnlock(nwfilter);
+ virNWFilterUnlockFilterUpdates(); + virNWFilterCallbackDriversUnlock(); nwfilterDriverUnlock(driver); return ret; Index: libvirt-acl/src/nwfilter/locking.txt =================================================================== --- /dev/null +++ libvirt-acl/src/nwfilter/locking.txt @@ -0,0 +1,45 @@ +NWfilter locks: +--------------- + +The following NWFilter functions grab locks: + +- nwfilterDriverLock(driverState) + - protects driverState +- virNWFilterCallbackDriversLock() + - lock access to callback drivers +- virNWFilterLockFilterUpdates() + - prevent modification of any filter in use by any VM +- virNWFilterObjLock() + called by + - virNWFilterObjFindByUUID(), + - virNWFilterObjFindByName(), + - virNWFilterObjAssignDef() + and the functions return with the lock held + + +NWFilter-Lock ordering: +----------------------- + +Order in which the NWFilter lock functions have to be called: + +1. nwFilterDriverLock +2. virNWFilterCallbackDriversLock +3. virNWFilterLockFilterUpdates +4. virNWFilterObjLock + +Other locks: +------------ + +Other relevant locks to consider are related to hypervisor drivers. The +following locking orders are good with the qemu_driver as an example: + +qemu_driver - qemu_domain - filter object +qemu_driver - filter object - qemu_domain + +The qemu_driver serves as a general lock for all subsequent locks to a +qemu_domain. This works because the qemu_driver will always be locked +for a qemu_domain, thus only one lock-sequence of either (qemu_domain, +filter object) or (filter object, qemu_domain) can be active at any time. + +Reference: +https://www.redhat.com/archives/libvir-list/2010-October/msg00152.html
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/11/2011 02:36 PM, Stefan Berger wrote:
This patch reorders the locks for the nwfilter updates and the access the nwfilter objects. In the case that the IP address learning thread was instantiating filters while an update happened, the previous order lead to a deadlock.
I am also adding a text file describing the locking order of the nwfilter subsystem.
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
--- src/conf/nwfilter_conf.c | 9 ++++---- src/nwfilter/locking.txt | 45
Hmm. We have daemon/THREADING.txt, src/qemu/THREADS.txt, and now src/nwfilter/locking.txt. Let's pick a single name for all 3 (THREADS.txt is my favorite - it's shortest, and by being ALL-CAPS, it sorts first in the directory for when you need it, and does not interfere with .c files for when you don't want to be bothered by it). I think you can be pre-approved to push an independent patch that trivially renames THREADING.txt to THREADS.txt, plus the Makefile.am fallout. Also, you need to list your new file in src/Makefile.am. ACK with those changes. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/26/2011 03:19 PM, Eric Blake wrote:
On 05/11/2011 02:36 PM, Stefan Berger wrote:
This patch reorders the locks for the nwfilter updates and the access the nwfilter objects. In the case that the IP address learning thread was instantiating filters while an update happened, the previous order lead to a deadlock.
I am also adding a text file describing the locking order of the nwfilter subsystem.
Signed-off-by: Stefan Berger<stefanb@us.ibm.com>
--- src/conf/nwfilter_conf.c | 9 ++++---- src/nwfilter/locking.txt | 45 Hmm. We have daemon/THREADING.txt, src/qemu/THREADS.txt, and now src/nwfilter/locking.txt.
Let's pick a single name for all 3 (THREADS.txt is my favorite - it's shortest, and by being ALL-CAPS, it sorts first in the directory for when you need it, and does not interfere with .c files for when you don't want to be bothered by it).
I think you can be pre-approved to push an independent patch that trivially renames THREADING.txt to THREADS.txt, plus the Makefile.am fallout.
Also, you need to list your new file in src/Makefile.am.
ACK with those changes. I pushed the code part now, will do the text later.
Stefan
participants (2)
-
Eric Blake -
Stefan Berger