From nobody Wed May 14 18:59:39 2025 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of redhat.com designates 209.132.183.28 as permitted sender) client-ip=209.132.183.28; envelope-from=libvir-list-bounces@redhat.com; helo=mx1.redhat.com; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of redhat.com designates 209.132.183.28 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass(p=none dis=none) header.from=redhat.com Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by mx.zohomail.com with SMTPS id 1517944860242785.3087303229236; Tue, 6 Feb 2018 11:21:00 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2CD6D883D2; Tue, 6 Feb 2018 19:20:59 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0391D5C25F; Tue, 6 Feb 2018 19:20:59 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by colo-mx.corp.redhat.com (Postfix) with ESMTP id C01734A46E; Tue, 6 Feb 2018 19:20:58 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id w16JKSY2013046 for ; Tue, 6 Feb 2018 14:20:28 -0500 Received: by smtp.corp.redhat.com (Postfix) id CA7565C556; Tue, 6 Feb 2018 19:20:28 +0000 (UTC) Received: from unknown54ee7586bd10.attlocal.net.com (ovpn-116-232.phx2.redhat.com [10.3.116.232]) by smtp.corp.redhat.com (Postfix) with ESMTP id 752195C66F for ; Tue, 6 Feb 2018 19:20:28 +0000 (UTC) From: John Ferlan To: libvir-list@redhat.com Date: Tue, 6 Feb 2018 14:20:10 -0500 Message-Id: <20180206192012.16861-2-jferlan@redhat.com> In-Reply-To: <20180206192012.16861-1-jferlan@redhat.com> References: <20180206192012.16861-1-jferlan@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-loop: libvir-list@redhat.com Subject: [libvirt] [PATCH v5 1/3] nwfilter: Convert _virNWFilterObj to use virObjectRWLockable X-BeenThere: libvir-list@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Development discussions about the libvirt library & tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Sender: libvir-list-bounces@redhat.com Errors-To: libvir-list-bounces@redhat.com X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Tue, 06 Feb 2018 19:20:59 +0000 (UTC) X-ZohoMail: RSF_0 Z_629925259 SPT_0 Content-Type: text/plain; charset="utf-8" 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 --- src/conf/virnwfilterobj.c | 193 +++++++++++++++++++++++------= ---- 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, 153 insertions(+), 78 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 87d7e7270..cd52706ec 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -34,7 +34,7 @@ VIR_LOG_INIT("conf.virnwfilterobj"); =20 struct _virNWFilterObj { - virMutex lock; + virObjectRWLockable parent; =20 bool wantRemoved; =20 @@ -47,27 +47,69 @@ struct _virNWFilterObjList { virNWFilterObjPtr *objs; }; =20 +static virClassPtr virNWFilterObjClass; +static void virNWFilterObjDispose(void *opaque); + + +static int +virNWFilterObjOnceInit(void) +{ + if (!(virNWFilterObjClass =3D virClassNew(virClassForObjectRWLockable(= ), + "virNWFilterObj", + sizeof(virNWFilterObj), + virNWFilterObjDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virNWFilterObj) + =20 static virNWFilterObjPtr virNWFilterObjNew(void) { virNWFilterObjPtr obj; =20 - if (VIR_ALLOC(obj) < 0) + if (virNWFilterObjInitialize() < 0) return NULL; =20 - if (virMutexInitRecursive(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(obj); + if (!(obj =3D virObjectRWLockableNew(virNWFilterObjClass))) return NULL; - } =20 - virNWFilterObjLock(obj); + virObjectRWLockWrite(obj); return obj; } =20 =20 +static void +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj) +{ + virObjectRWUnlock(obj); + virObjectRWLockWrite(obj); +} + + +static void +virNWFilterObjDemoteFromWrite(virNWFilterObjPtr obj) +{ + virObjectRWUnlock(obj); + virObjectRWLockRead(obj); +} + + +void +virNWFilterObjEndAPI(virNWFilterObjPtr *obj) +{ + if (!*obj) + return; + + virObjectRWUnlock(*obj); + virObjectUnref(*obj); + *obj =3D NULL; +} + + virNWFilterDefPtr virNWFilterObjGetDef(virNWFilterObjPtr obj) { @@ -90,17 +132,15 @@ virNWFilterObjWantRemoved(virNWFilterObjPtr obj) =20 =20 static void -virNWFilterObjFree(virNWFilterObjPtr obj) +virNWFilterObjDispose(void *opaque) { + virNWFilterObjPtr obj =3D opaque; + if (!obj) return; =20 virNWFilterDefFree(obj->def); virNWFilterDefFree(obj->newDef); - - virMutexDestroy(&obj->lock); - - VIR_FREE(obj); } =20 =20 @@ -109,7 +149,7 @@ virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) { size_t i; for (i =3D 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 nwfilt= ers, { size_t i; =20 - virNWFilterObjUnlock(obj); + virObjectRWUnlock(obj); =20 for (i =3D 0; i < nwfilters->count; i++) { - virNWFilterObjLock(nwfilters->objs[i]); + virObjectRWLockWrite(nwfilters->objs[i]); if (nwfilters->objs[i] =3D=3D obj) { - virNWFilterObjUnlock(nwfilters->objs[i]); - virNWFilterObjFree(nwfilters->objs[i]); + virObjectRWUnlock(nwfilters->objs[i]); + virObjectUnref(nwfilters->objs[i]); =20 VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count); break; } - virNWFilterObjUnlock(nwfilters->objs[i]); + virObjectRWUnlock(nwfilters->objs[i]); } } =20 =20 +/** + * 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 nw= filters, =20 for (i =3D 0; i < nwfilters->count; i++) { obj =3D nwfilters->objs[i]; - virNWFilterObjLock(obj); + virObjectRWLockRead(obj); def =3D obj->def; if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN)) - return obj; - virNWFilterObjUnlock(obj); + return virObjectRef(obj); + virObjectRWUnlock(obj); } =20 return NULL; } =20 =20 +/** + * 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 nw= filters, =20 for (i =3D 0; i < nwfilters->count; i++) { obj =3D nwfilters->objs[i]; - virNWFilterObjLock(obj); + virObjectRWLockRead(obj); def =3D obj->def; if (STREQ_NULLABLE(def->name, name)) - return obj; - virNWFilterObjUnlock(obj); + return virObjectRef(obj); + virObjectRWUnlock(obj); } =20 return NULL; @@ -205,7 +265,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjL= istPtr nwfilters, if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } =20 @@ -240,7 +300,7 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr = nwfilters, if (obj) { rc =3D _virNWFilterObjListDefLoopDetect(nwfilters, obj->de= f, filtername); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); if (rc < 0) break; } @@ -269,17 +329,36 @@ virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr= nwfilters, } =20 =20 +/* 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 virNWFilterObjListFindInstantiateFil= ter + * 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 =3D 0; =20 + virNWFilterObjPromoteToWrite(obj); obj->wantRemoved =3D true; + virNWFilterObjDemoteFromWrite(obj); + /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild() < 0) rc =3D -1; =20 + virNWFilterObjPromoteToWrite(obj); obj->wantRemoved =3D false; + virNWFilterObjDemoteFromWrite(obj); =20 return rc; } @@ -322,10 +401,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwf= ilters, _("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 =3D virNWFilterObjListFindByName(nwfilters, def->name))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -335,7 +414,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfil= ters, virReportError(VIR_ERR_OPERATION_FAILED, _("filter '%s' already exists with uuid %s"), def->name, uuidstr); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } } @@ -347,26 +426,39 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwf= ilters, } =20 =20 + /* Get a READ lock and immediately promote to WRITE while we adjust + * data within. */ if ((obj =3D virNWFilterObjListFindByName(nwfilters, def->name))) { =20 objdef =3D obj->def; if (virNWFilterDefEqual(def, objdef)) { + virNWFilterObjPromoteToWrite(obj); virNWFilterDefFree(objdef); obj->def =3D def; + virNWFilterObjDemoteFromWrite(obj); return obj; } =20 + /* Set newDef and run the trigger with a demoted lock since it may= need + * to grab a read lock on this object and promote before returning= . */ + virNWFilterObjPromoteToWrite(obj); obj->newDef =3D def; + virNWFilterObjDemoteFromWrite(obj); + /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild() < 0) { + virNWFilterObjPromoteToWrite(obj); obj->newDef =3D NULL; - virNWFilterObjUnlock(obj); + /* NB: We're done, no need to Demote and End, just End */ + virNWFilterObjEndAPI(&obj); return NULL; } =20 + virNWFilterObjPromoteToWrite(obj); virNWFilterDefFree(objdef); obj->def =3D def; obj->newDef =3D NULL; + virNWFilterObjDemoteFromWrite(obj); return obj; } =20 @@ -375,11 +467,12 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwf= ilters, =20 if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, nwfilters->count, obj) < 0) { - virNWFilterObjUnlock(obj); - virNWFilterObjFree(obj); + virNWFilterObjEndAPI(&obj); return NULL; } + virObjectRef(obj); obj->def =3D def; + virNWFilterObjDemoteFromWrite(obj); =20 return obj; } @@ -395,10 +488,10 @@ virNWFilterObjListNumOfNWFilters(virNWFilterObjListPt= r nwfilters, =20 for (i =3D 0; i < nwfilters->count; i++) { virNWFilterObjPtr obj =3D nwfilters->objs[i]; - virNWFilterObjLock(obj); + virObjectRWLockRead(obj); if (!filter || filter(conn, obj->def)) nfilters++; - virNWFilterObjUnlock(obj); + virObjectRWUnlock(obj); } =20 return nfilters; @@ -418,16 +511,16 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr nwfi= lters, =20 for (i =3D 0; i < nwfilters->count && nnames < maxnames; i++) { virNWFilterObjPtr obj =3D nwfilters->objs[i]; - virNWFilterObjLock(obj); + virObjectRWLockRead(obj); def =3D 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); } =20 return nnames; @@ -464,16 +557,16 @@ virNWFilterObjListExport(virConnectPtr conn, =20 for (i =3D 0; i < nwfilters->count; i++) { obj =3D nwfilters->objs[i]; - virNWFilterObjLock(obj); + virObjectRWLockRead(obj); def =3D obj->def; if (!filter || filter(conn, def)) { if (!(nwfilter =3D virGetNWFilter(conn, def->name, def->uuid))= ) { - virNWFilterObjUnlock(obj); + virObjectRWUnlock(obj); goto cleanup; } tmp_filters[nfilters++] =3D nwfilter; } - virNWFilterObjUnlock(obj); + virObjectRWUnlock(obj); } =20 *filters =3D tmp_filters; @@ -551,24 +644,10 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPt= r nwfilters, continue; =20 obj =3D virNWFilterObjListLoadConfig(nwfilters, configDir, entry->= d_name); - if (obj) - virNWFilterObjUnlock(obj); + + virNWFilterObjEndAPI(&obj); } =20 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; }; =20 +void +virNWFilterObjEndAPI(virNWFilterObjPtr *obj); + virNWFilterDefPtr virNWFilterObjGetDef(virNWFilterObjPtr obj); =20 @@ -105,10 +108,4 @@ int virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, const char *configDir); =20 -void -virNWFilterObjLock(virNWFilterObjPtr obj); - -void -virNWFilterObjUnlock(virNWFilterObjPtr obj); - #endif /* VIRNWFILTEROBJ_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0bce0bbfb..2f7fc550b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1029,6 +1029,7 @@ virNodeDeviceObjListRemove; =20 =20 # conf/virnwfilterobj.h +virNWFilterObjEndAPI; virNWFilterObjGetDef; virNWFilterObjGetNewDef; virNWFilterObjListAssignDef; @@ -1042,9 +1043,7 @@ virNWFilterObjListLoadAllConfigs; virNWFilterObjListNew; virNWFilterObjListNumOfNWFilters; virNWFilterObjListRemove; -virNWFilterObjLock; virNWFilterObjTestUnassignDef; -virNWFilterObjUnlock; virNWFilterObjWantRemoved; =20 =20 diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 717bce269..3a0447627 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -466,7 +466,7 @@ nwfilterLookupByUUID(virConnectPtr conn, nwfilter =3D virGetNWFilter(conn, def->name, def->uuid); =20 cleanup: - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return nwfilter; } =20 @@ -496,7 +496,7 @@ nwfilterLookupByName(virConnectPtr conn, nwfilter =3D virGetNWFilter(conn, def->name, def->uuid); =20 cleanup: - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return nwfilter; } =20 @@ -583,6 +583,8 @@ nwfilterDefineXML(virConnectPtr conn, =20 if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) { virNWFilterObjListRemove(driver->nwfilters, obj); + virObjectUnref(obj); + obj =3D NULL; goto cleanup; } =20 @@ -590,8 +592,7 @@ nwfilterDefineXML(virConnectPtr conn, =20 cleanup: virNWFilterDefFree(def); - if (obj) - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); =20 virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); @@ -629,12 +630,12 @@ nwfilterUndefine(virNWFilterPtr nwfilter) goto cleanup; =20 virNWFilterObjListRemove(driver->nwfilters, obj); + virObjectUnref(obj); obj =3D NULL; ret =3D 0; =20 cleanup: - if (obj) - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); =20 virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); @@ -667,7 +668,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, ret =3D virNWFilterDefFormat(def); =20 cleanup: - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return ret; } =20 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; =20 for (i =3D 0; i < inst->nfilters; i++) - virNWFilterObjUnlock(inst->filters[i]); + virNWFilterObjEndAPI(&inst->filters[i]); VIR_FREE(inst->filters); inst->nfilters =3D 0; =20 @@ -426,8 +426,7 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStateP= tr driver, if (ret < 0) virNWFilterInstReset(inst); virNWFilterHashTableFree(tmpvars); - if (obj) - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return ret; } =20 @@ -541,7 +540,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr fi= lter, =20 /* create a temporary hashmap for depth-first tree traversal */ if (!(tmpvars =3D virNWFilterCreateVarsFrom(inc->params, vars)= )) { - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return -1; } =20 @@ -565,7 +564,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr fi= lter, =20 virNWFilterHashTableFree(tmpvars); =20 - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); if (rc < 0) return -1; } @@ -839,7 +838,7 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverSta= tePtr driver, virNWFilterHashTableFree(vars1); =20 err_exit: - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); =20 VIR_FREE(str_ipaddr); VIR_FREE(str_macaddr); --=20 2.13.6 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list