Re: [libvirt] [PATCH v4 2/4] nwfilter: Convert _virNWFilterObj to use virObjectRWLockable

On 12/08/2017 09:01 AM, John Ferlan wrote: > Unlike it's counterparts, the nwfilter object code needs to be able > to support recursive read locks while processing and checking new > filters against the existing environment. Thus instead of using a > virObjectLockable which uses pure mutexes, use the virObjectRWLockable > and primarily use RWLockRead when obtaining the object lock since > RWLockRead locks can be recursively obtained (up to a limit) as long > as there's a corresponding unlock. > > Since all the object management is within the virnwfilterobj code, we > can limit the number of Write locks on the object to very small areas > of code to ensure we don't run into deadlock's with other threads and > domain code that will check/use the filters (it's a very delicate > balance). This limits the write locks to AssignDef and Remove processing. > > This patch will introduce a new API virNWFilterObjEndAPI to unlock > and deref the object. > > This patch introduces two helpers to promote/demote the read/write lock. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- >   src/conf/virnwfilterobj.c              | 191 > +++++++++++++++++++++++---------- >   src/conf/virnwfilterobj.h              |   9 +- >   src/libvirt_private.syms               |   3 +- >   src/nwfilter/nwfilter_driver.c         |  15 +-- >   src/nwfilter/nwfilter_gentech_driver.c |  11 +- >   5 files changed, 149 insertions(+), 80 deletions(-) > > diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c > index 87d7e7270..6b4758656 100644 > --- a/src/conf/virnwfilterobj.c > +++ b/src/conf/virnwfilterobj.c > @@ -34,7 +34,7 @@ >   VIR_LOG_INIT("conf.virnwfilterobj"); > >   struct _virNWFilterObj { > -    virMutex lock; > +    virObjectRWLockable parent; > >       bool wantRemoved; > > @@ -47,27 +47,69 @@ struct _virNWFilterObjList { >       virNWFilterObjPtr *objs; >   }; > > +static virClassPtr virNWFilterObjClass; > +static void virNWFilterObjDispose(void *opaque); > + > + > +static int > +virNWFilterObjOnceInit(void) > +{ > +    if (!(virNWFilterObjClass = > virClassNew(virClassForObjectRWLockable(), > +                                            "virNWFilterObj", > +                                            sizeof(virNWFilterObj), > +                                            virNWFilterObjDispose))) > +        return -1; > + > +    return 0; > +} > + > +VIR_ONCE_GLOBAL_INIT(virNWFilterObj) > + > >   static virNWFilterObjPtr >   virNWFilterObjNew(void) >   { >       virNWFilterObjPtr obj; > > -    if (VIR_ALLOC(obj) < 0) > +    if (virNWFilterObjInitialize() < 0) Is this function available at this point ?

>           return NULL; > > -    if (virMutexInitRecursive(&obj->lock) < 0) { > -        virReportError(VIR_ERR_INTERNAL_ERROR, > -                       "%s", _("cannot initialize mutex")); > -        VIR_FREE(obj); > +    if (!(obj = virObjectRWLockableNew(virNWFilterObjClass))) >           return NULL; > -    } > > -    virNWFilterObjLock(obj); > +    virObjectRWLockWrite(obj); Why do you create it with the write-lock held? I suppose if we do that we would always have to demote from writer. But I see the pattern of promoting *to* writer then demoting to reader in the rest of the patch.

>       return obj; >   } > > > +static void > +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj) > +{ > +    virObjectRWUnlock(obj); > +    virObjectRWLockWrite(obj); Do we need to unlock here or can we have it locked for reading 5 times and then lock for writing ? I suppose this isn't trying to decrease the read-locks to zero and then only we are able to grab a write lock, right ?

> +} > + > + > +static void > +virNWFilterObjDemoteFromWrite(virNWFilterObjPtr obj) > +{ > +    virObjectRWUnlock(obj); > +    virObjectRWLockRead(obj); > +} > + > + > +void > +virNWFilterObjEndAPI(virNWFilterObjPtr *obj) > +{ > +    if (!*obj) > +        return; > + > +    virObjectRWUnlock(*obj); > +    virObjectUnref(*obj); > +    *obj = NULL; > +} > + > + >   virNWFilterDefPtr >   virNWFilterObjGetDef(virNWFilterObjPtr obj) >   { > @@ -90,17 +132,15 @@ virNWFilterObjWantRemoved(virNWFilterObjPtr obj) > > >   static void > -virNWFilterObjFree(virNWFilterObjPtr obj) > +virNWFilterObjDispose(void *opaque) >   { > +    virNWFilterObjPtr obj = opaque; > + >       if (!obj) >           return; > >       virNWFilterDefFree(obj->def); >       virNWFilterDefFree(obj->newDef); > - > -    virMutexDestroy(&obj->lock); > - > -    VIR_FREE(obj); >   } > > > @@ -109,7 +149,7 @@ virNWFilterObjListFree(virNWFilterObjListPtr > nwfilters) >   { >       size_t i; >       for (i = 0; i < nwfilters->count; i++) > -        virNWFilterObjFree(nwfilters->objs[i]); > +        virObjectUnref(nwfilters->objs[i]); >       VIR_FREE(nwfilters->objs); >       VIR_FREE(nwfilters); >   } > @@ -132,22 +172,32 @@ virNWFilterObjListRemove(virNWFilterObjListPtr > nwfilters, >   { >       size_t i; > > -    virNWFilterObjUnlock(obj); > +    virObjectRWUnlock(obj); > >       for (i = 0; i < nwfilters->count; i++) { > -        virNWFilterObjLock(nwfilters->objs[i]); > +        virObjectRWLockWrite(nwfilters->objs[i]); The conversion is ok, but I am not sure why we are write-locking here ...?

>           if (nwfilters->objs[i] == obj) { > -            virNWFilterObjUnlock(nwfilters->objs[i]); > -            virNWFilterObjFree(nwfilters->objs[i]); > +            virObjectRWUnlock(nwfilters->objs[i]); > +            virObjectUnref(nwfilters->objs[i]); > >               VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count); The nwfilters->count and nwfilters->objs is actually protected by nwfilterDriverLock(). So all good here to mess with the list.

>               break; >           } > -        virNWFilterObjUnlock(nwfilters->objs[i]); > +        virObjectRWUnlock(nwfilters->objs[i]); >       } >   } > > > +/** > + * virNWFilterObjListFindByUUID > + * @nwfilters: Pointer to filter list > + * @uuid: UUID to use to lookup the object > + * > + * Search for the object by uuidstr in the hash table and return a read > + * locked copy of the object. > + * > + * Returns: A reffed and read locked object or NULL on error > + */ >   virNWFilterObjPtr >   virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, >                                const unsigned char *uuid) > @@ -158,17 +208,27 @@ > virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, > >       for (i = 0; i < nwfilters->count; i++) { >           obj = nwfilters->objs[i]; > -        virNWFilterObjLock(obj); > +        virObjectRWLockRead(obj); >           def = obj->def; >           if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN)) > -            return obj; > -        virNWFilterObjUnlock(obj); > +            return virObjectRef(obj); All callers to this function seem to properly Unref the obj now. Good.

> +        virObjectRWUnlock(obj); >       } > >       return NULL; >   } > > > +/** > + * virNWFilterObjListFindByName > + * @nwfilters: Pointer to filter list > + * @name: filter name to use to lookup the object > + * > + * Search for the object by name in the hash table and return a read > + * locked copy of the object. > + * > + * Returns: A reffed and read locked object or NULL on error > + */ >   virNWFilterObjPtr >   virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, >                                const char *name) > @@ -179,11 +239,11 @@ > virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, > >       for (i = 0; i < nwfilters->count; i++) { >           obj = nwfilters->objs[i]; > -        virNWFilterObjLock(obj); > +        virObjectRWLockRead(obj); >           def = obj->def; >           if (STREQ_NULLABLE(def->name, name)) > -            return obj; > -        virNWFilterObjUnlock(obj); > +            return virObjectRef(obj); Also good here... > +        virObjectRWUnlock(obj); >       } > >       return NULL; > @@ -205,8 +265,7 @@ > virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, >       if (virNWFilterObjWantRemoved(obj)) { >           virReportError(VIR_ERR_NO_NWFILTER, >                          _("Filter '%s' is in use."), filtername); > -        virNWFilterObjUnlock(obj); > -        return NULL; > +        virNWFilterObjEndAPI(&obj); I think we MUST still return NULL here. This would otherwise change the behaviour of the code and why unref only in this case?

>       } > >       return obj; > @@ -240,7 +299,7 @@ > _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, >               if (obj) { >                   rc = _virNWFilterObjListDefLoopDetect(nwfilters, > obj->def, >                                                         filtername); > -                virNWFilterObjUnlock(obj); > +                virNWFilterObjEndAPI(&obj); >                   if (rc < 0) >                       break; >               } > @@ -269,17 +328,36 @@ > virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, >   } > > > +/* virNWFilterObjTestUnassignDef > + * @obj: A read locked nwfilter object > + * > + * Cause the rebuild to occur because we're about to undefine the > nwfilter. > + * The rebuild code is designed to check if the filter is currently > in use > + * by a domain and thus disallow the unassign. > + * > + * NB: Although we enter with the UPDATE lock from UNDEFINE, let's still > + *     promote to a WRITE lock before changing *this* object's > wantRemoved > + *     value that will be used in the > virNWFilterObjListFindInstantiateFilter > + *     processing to determine whether we can really remove this > filter or not. > + * > + * Returns 0 if we can continue with the unassign, -1 if it's in use > + */ >   int >   virNWFilterObjTestUnassignDef(virNWFilterObjPtr obj) >   { >       int rc = 0; > > +    virNWFilterObjPromoteToWrite(obj); >       obj->wantRemoved = true; > +    virNWFilterObjDemoteFromWrite(obj); > + >       /* trigger the update on VMs referencing the filter */ >       if (virNWFilterTriggerVMFilterRebuild() < 0) >           rc = -1; > > +    virNWFilterObjPromoteToWrite(obj); >       obj->wantRemoved = false; > +    virNWFilterObjDemoteFromWrite(obj); > >       return rc; >   } > @@ -322,10 +400,10 @@ > virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, >                              _("filter with same UUID but different > name " >                                "('%s') already exists"), >                              objdef->name); > -            virNWFilterObjUnlock(obj); > +            virNWFilterObjEndAPI(&obj); >               return NULL; >           } > -        virNWFilterObjUnlock(obj); > +        virNWFilterObjEndAPI(&obj); >       } else { >           if ((obj = virNWFilterObjListFindByName(nwfilters, > def->name))) { >               char uuidstr[VIR_UUID_STRING_BUFLEN]; > @@ -335,7 +413,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr > nwfilters, >               virReportError(VIR_ERR_OPERATION_FAILED, >                              _("filter '%s' already exists with uuid > %s"), >                              def->name, uuidstr); > -            virNWFilterObjUnlock(obj); > +            virNWFilterObjEndAPI(&obj); >               return NULL; >           } >       } > @@ -347,7 +425,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr > nwfilters, >       } > > > +    /* Get a READ lock and immediately promote to WRITE while we adjust > +     * data within. */ >       if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { > +        virNWFilterObjPromoteToWrite(obj); > >           objdef = obj->def; >           if (virNWFilterDefEqual(def, objdef)) { Shouldn't we demote to read after this virNWFilterDefEqual() call and before the 'return obj;' that's not shown in the patch ?

> @@ -357,13 +438,20 @@ > virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, >           } > >           obj->newDef = def; > + > +        /* Demote while the trigger runs since it may need to grab a > read > +         * lock on this object and promote before returning. */ > +        virNWFilterObjDemoteFromWrite(obj); > + >           /* trigger the update on VMs referencing the filter */ >           if (virNWFilterTriggerVMFilterRebuild() < 0) { > +            virNWFilterObjPromoteToWrite(obj); >               obj->newDef = NULL; > -            virNWFilterObjUnlock(obj); > +            virNWFilterObjEndAPI(&obj); Not a bug, but to make this clearer I think we may want to call virNWFilterObjDemoteFromWrite(obj); virNWFilterObjEndAPI(&obj); I know it's redundant ...

>               return NULL; >           } > > +        virNWFilterObjPromoteToWrite(obj); >           virNWFilterDefFree(objdef); >           obj->def = def; >           obj->newDef = NULL; demote to read here ?

> @@ -375,13 +463,12 @@ > virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, > >       if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, >                                   nwfilters->count, obj) < 0) { > -        virNWFilterObjUnlock(obj); > -        virNWFilterObjFree(obj); > +        virNWFilterObjEndAPI(&obj); >           return NULL; >       } >       obj->def = def; > > -    return obj; > +    return virObjectRef(obj); This is correct but again , to make this clearer I think we should write: if (VIR_APPEND...) { [...] } else  {     virObjectRef(obj); }

obj->def = def; return obj; We take the additional reference because we just successfully put it onto the list.

>   } > > > @@ -395,10 +482,10 @@ > virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters, > >       for (i = 0; i < nwfilters->count; i++) { >           virNWFilterObjPtr obj = nwfilters->objs[i]; > -        virNWFilterObjLock(obj); > +        virObjectRWLockRead(obj); >           if (!filter || filter(conn, obj->def)) >               nfilters++; > -        virNWFilterObjUnlock(obj); > +        virObjectRWUnlock(obj); >       } > >       return nfilters; > @@ -418,16 +505,16 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr > nwfilters, > >       for (i = 0; i < nwfilters->count && nnames < maxnames; i++) { >           virNWFilterObjPtr obj = nwfilters->objs[i]; > -        virNWFilterObjLock(obj); > +        virObjectRWLockRead(obj); >           def = obj->def; >           if (!filter || filter(conn, def)) { >               if (VIR_STRDUP(names[nnames], def->name) < 0) { > -                virNWFilterObjUnlock(obj); > +                virObjectRWUnlock(obj); >                   goto failure; >               } >               nnames++; >           } > -        virNWFilterObjUnlock(obj); > +        virObjectRWUnlock(obj); >       } > >       return nnames; > @@ -464,16 +551,16 @@ virNWFilterObjListExport(virConnectPtr conn, > >       for (i = 0; i < nwfilters->count; i++) { >           obj = nwfilters->objs[i]; > -        virNWFilterObjLock(obj); > +        virObjectRWLockRead(obj); >           def = obj->def; >           if (!filter || filter(conn, def)) { >               if (!(nwfilter = virGetNWFilter(conn, def->name, > def->uuid))) { > -                virNWFilterObjUnlock(obj); > +                virObjectRWUnlock(obj); >                   goto cleanup; >               } >               tmp_filters[nfilters++] = nwfilter; >           } > -        virNWFilterObjUnlock(obj); > +        virObjectRWUnlock(obj); >       } > >       *filters = tmp_filters; > @@ -551,24 +638,10 @@ > virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, >               continue; > >           obj = virNWFilterObjListLoadConfig(nwfilters, configDir, > entry->d_name); > -        if (obj) > -            virNWFilterObjUnlock(obj); > + > +        virNWFilterObjEndAPI(&obj); >       } > >       VIR_DIR_CLOSE(dir); >       return ret; >   } > - > - > -void > -virNWFilterObjLock(virNWFilterObjPtr obj) > -{ > -    virMutexLock(&obj->lock); > -} > - > - > -void > -virNWFilterObjUnlock(virNWFilterObjPtr obj) > -{ > -    virMutexUnlock(&obj->lock); > -} > diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h > index 8e79518ed..0281bc5f5 100644 > --- a/src/conf/virnwfilterobj.h > +++ b/src/conf/virnwfilterobj.h > @@ -41,6 +41,9 @@ struct _virNWFilterDriverState { >       bool watchingFirewallD; >   }; > > +void > +virNWFilterObjEndAPI(virNWFilterObjPtr *obj); > + >   virNWFilterDefPtr >   virNWFilterObjGetDef(virNWFilterObjPtr obj); > > @@ -105,10 +108,4 @@ int >   virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, >                                    const char *configDir); > > -void > -virNWFilterObjLock(virNWFilterObjPtr obj); > - > -void > -virNWFilterObjUnlock(virNWFilterObjPtr obj); > - >   #endif /* VIRNWFILTEROBJ_H */ > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index de4ec4d44..132244878 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1022,6 +1022,7 @@ virNodeDeviceObjListRemove; > > >   # conf/virnwfilterobj.h > +virNWFilterObjEndAPI; >   virNWFilterObjGetDef; >   virNWFilterObjGetNewDef; >   virNWFilterObjListAssignDef; > @@ -1035,9 +1036,7 @@ virNWFilterObjListLoadAllConfigs; >   virNWFilterObjListNew; >   virNWFilterObjListNumOfNWFilters; >   virNWFilterObjListRemove; > -virNWFilterObjLock; >   virNWFilterObjTestUnassignDef; > -virNWFilterObjUnlock; >   virNWFilterObjWantRemoved; > > > diff --git a/src/nwfilter/nwfilter_driver.c > b/src/nwfilter/nwfilter_driver.c > index 885dbcc28..b38740b15 100644 > --- a/src/nwfilter/nwfilter_driver.c > +++ b/src/nwfilter/nwfilter_driver.c > @@ -400,7 +400,7 @@ nwfilterLookupByUUID(virConnectPtr conn, >       nwfilter = virGetNWFilter(conn, def->name, def->uuid); > >    cleanup: > -    virNWFilterObjUnlock(obj); > +    virNWFilterObjEndAPI(&obj); >       return nwfilter; >   } > > @@ -430,7 +430,7 @@ nwfilterLookupByName(virConnectPtr conn, >       nwfilter = virGetNWFilter(conn, def->name, def->uuid); > >    cleanup: > -    virNWFilterObjUnlock(obj); > +    virNWFilterObjEndAPI(&obj); >       return nwfilter; >   } > > @@ -517,6 +517,8 @@ nwfilterDefineXML(virConnectPtr conn, > >       if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) { >           virNWFilterObjListRemove(driver->nwfilters, obj); > +        virObjectUnref(obj); > +        obj = NULL; >           goto cleanup; >       } > > @@ -524,8 +526,7 @@ nwfilterDefineXML(virConnectPtr conn, > >    cleanup: >       virNWFilterDefFree(def); > -    if (obj) > -        virNWFilterObjUnlock(obj); > +    virNWFilterObjEndAPI(&obj); > >       virNWFilterCallbackDriversUnlock(); >       virNWFilterUnlockFilterUpdates(); > @@ -563,12 +564,12 @@ nwfilterUndefine(virNWFilterPtr nwfilter) >           goto cleanup; > >       virNWFilterObjListRemove(driver->nwfilters, obj); > +    virObjectUnref(obj); >       obj = NULL; >       ret = 0; > >    cleanup: > -    if (obj) > -        virNWFilterObjUnlock(obj); > +    virNWFilterObjEndAPI(&obj); > >       virNWFilterCallbackDriversUnlock(); >       virNWFilterUnlockFilterUpdates(); > @@ -601,7 +602,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, >       ret = virNWFilterDefFormat(def); > >    cleanup: > -    virNWFilterObjUnlock(obj); > +    virNWFilterObjEndAPI(&obj); >       return ret; >   } > > diff --git a/src/nwfilter/nwfilter_gentech_driver.c > b/src/nwfilter/nwfilter_gentech_driver.c > index 840d419bb..48d0e1769 100644 > --- a/src/nwfilter/nwfilter_gentech_driver.c > +++ b/src/nwfilter/nwfilter_gentech_driver.c > @@ -316,7 +316,7 @@ virNWFilterInstReset(virNWFilterInstPtr inst) >       size_t i; > >       for (i = 0; i < inst->nfilters; i++) > -        virNWFilterObjUnlock(inst->filters[i]); > +        virNWFilterObjEndAPI(&inst->filters[i]); >       VIR_FREE(inst->filters); >       inst->nfilters = 0; > > @@ -426,8 +426,7 @@ > virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, >       if (ret < 0) >           virNWFilterInstReset(inst); >       virNWFilterHashTableFree(tmpvars); > -    if (obj) > -        virNWFilterObjUnlock(obj); > +    virNWFilterObjEndAPI(&obj); >       return ret; >   } > > @@ -541,7 +540,7 @@ > virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, > >               /* create a temporary hashmap for depth-first tree > traversal */ >               if (!(tmpvars = virNWFilterCreateVarsFrom(inc->params, > vars))) { > -                virNWFilterObjUnlock(obj); > +                virNWFilterObjEndAPI(&obj); >                   return -1; >               } > > @@ -565,7 +564,7 @@ > virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, > >               virNWFilterHashTableFree(tmpvars); > > -            virNWFilterObjUnlock(obj); > +            virNWFilterObjEndAPI(&obj); >               if (rc < 0) >                   return -1; >           } > @@ -839,7 +838,7 @@ > virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, >       virNWFilterHashTableFree(vars1); > >    err_exit: > -    virNWFilterObjUnlock(obj); > +    virNWFilterObjEndAPI(&obj); > >       VIR_FREE(str_ipaddr); >       VIR_FREE(str_macaddr); The call to virNWFilterObjListFindInstantiateFilter() now returns an additional reference, so these last 4 calls to virNWFilterObjEndApi() are unreferencing that -- good.    Stefan