[libvirt] [PATCH] nwfilter: reorder locks

[Stefan Berger]

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 at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list