[libvirt] [PATCH v4 2/4] nwfilter: Convert _virNWFilterObj to use virObjectRWLockable
Stefan Berger
stefanb at linux.vnet.ibm.com
Thu Feb 1 02:15:31 UTC 2018
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 at redhat.com>
> ---
> 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
More information about the libvir-list
mailing list