[libvirt] [PATCH v4 2/4] nwfilter: Convert _virNWFilterObj to use virObjectRWLockable
Stefan Berger
stefanb at linux.vnet.ibm.com
Fri Feb 2 21:53:28 UTC 2018
On 02/02/2018 02:31 PM, John Ferlan wrote:
>
> On 01/31/2018 09:15 PM, Stefan Berger wrote:
>> 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 ?
> It's "hidden" by that VIR_ONCE_GLOBAL_INIT macro... Fairly common when
> using referenced object classes. See src/util/virthread.h.
>
>>> 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.
>>
> Essentially a common way of handling a lockable object - if you search
> on vir.*ObjNew (where .* is domain, interface, network, nodedevice,
> secret, storagevol, and storagepool) you'll see a common theme.
>
> Also a bit of paranoia. Since the nwfilters list will eventually be
> write locked too during add, it shouldn't really matter.
>
> We only ever do this when adding a new nwfilter. The two existing
> callers of virNWFilterObjListAssignDef are virNWFilterObjListLoadConfig
> via virNWFilterObjListLoadAllConfigs and nwfilterDefineXML do not demote
> the lock from write to read. Both end up calling virNWFilterObjEndAPI to
> release the lock when they're done.
>
>>> 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 ?
>>
> That's my assumption on this too - trying to get read-locks refcnt to
> zero. If there were a bunch of readers, then obtaining the write lock
> will wait for all those readers to complete. If we were the only reader,
> then getting the write will happen. It would be really nice to have the
> dlm available - that code was generated using the OpenVMS lock manager
> which I actually got to know quite well in a previous job...
>
> In the long run, I believe this ends up being one of those
> pthread_rwlock_wrlock type "things" related to refcnt's and which thread
> we're talking about. The man page states:
>
> "The pthread_rwlock_wrlock() function applies a write lock to the
> read-write lock referenced by rwlock. The calling thread acquires the
> write lock if no other thread (reader or writer) holds the read-write
> lock rwlock. Otherwise, the thread blocks (that is, does not return from
> the pthread_rwlock_wrlock() call) until it can acquire the lock. Results
> are undefined if the calling thread holds the read-write lock (whether a
> read or write lock) at the time the call is made."
>
> So my "choice" is demote/promote mainly because of that "undefined" part
> and because of other 'research' which seemed to indicate that promoting
> to wr while holding rd doesn't work reliably.
>
>>> +}
>>> +
>>> +
>>> +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 ...?
>>
> This code essentially mirrors how other vir.*ObjListRemove API's handle
> removing an object from the List (or what eventually will be a Hash Table).
>
> I agree that getting the lock is superfluous, but to a degree this
> change is part of an overall larger set of changes (already complete)
> that have converted from linked lists to hash tables. The nwfilter code
> has lagged because no one really wants to look at it!
>
>>> 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.
>>
> True for now, but in two patches, that lock is removed. In the next
> patch the nwfilters gets rwlock capabilities too.
>
> Maybe once all this is done - I can go back and remove all the
> unnecessary Lock/Unlock once list is RWLock'd.
>
>>> 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.
>>
> Yes, this has been a painful exercise... The domain code still is messed
> up, but that's a different problem.
>
>>> + 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?
>>
> We do, see virNWFilterObjEndAPI, after it Unref and Unlock it:
>
> *obj = NULL;
>
> thus obj = NULL when we leave.
>
> Every time we call virNWFilterObjListFindByName we return with a locked
> and reffed @obj. Once we are done with that obj, we have to remove the
> lock and ref.
>
> It's a fairly common process with all the other driver code.
I didn't recognize it... and, hm, unless one peeks into that functions,
it may be difficult to follow. For clarity I would return NULL
explicitly here ...
>
>>> }
>>>
>>> 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 ?
>>
> True that could/should be done since we're no longer updating @obj. I
> can add that.
... could otherwise prevent others from grabbing the read lock...
>
>>> @@ -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 ...
>>
> This one I'm not sure I agree as what we're doing then is:
>
> Unlock(WR)
> Lock(RD)
> Unlock(RD)
>
> Instead of just
>
> Unlock(WR)
>
> I'm not 100% against it, but I'm not sure I see the reason. How about
> if I add before the EndAPI:
>
> /* NB: We're done, no need to Demote and End, just End */
Sounds good to me.
More information about the libvir-list
mailing list