Re: [libvirt] [PATCH v5 2/3] nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable

On 02/09/2018 01:48 AM, Michal Privoznik wrote: > On 02/08/2018 10:13 PM, Stefan Berger wrote: >> On 02/08/2018 08:13 AM, Michal Privoznik wrote: >>> On 02/06/2018 08:20 PM, John Ferlan wrote: >>>> Implement the self locking object list for nwfilter object lists >>>> that uses two hash tables to store the nwfilter object by UUID or >>>> by Name. >>>> >>>> As part of this alter the uuid argument to virNWFilterObjLookupByUUID >>>> to expect an already formatted uuidstr. >>>> >>>> Alter the existing list traversal code to implement the hash table >>>> find/lookup/search functionality. >>>> >>>> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >>>> --- >>>>    src/conf/virnwfilterobj.c      | 405 >>>> ++++++++++++++++++++++++++++------------- >>>>    src/conf/virnwfilterobj.h      |   2 +- >>>>    src/nwfilter/nwfilter_driver.c |   5 +- >>>>    3 files changed, 283 insertions(+), 129 deletions(-) >>>> @@ -425,24 +483,26 @@ >>>> virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, >>>>            return NULL; >>>>        } >>>>    - >>>> -    /* Get a READ lock and immediately promote to WRITE while we >>>> adjust >>>> -     * data within. */ >>>> -    if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { >>>> +    /* We're about to make some changes to objects on the list - so >>>> get the >>>> +     * list READ lock in order to Find the object and WRITE lock the >>>> object >>>> +     * since both paths would immediately promote it anyway */ >>>> +    virObjectRWLockRead(nwfilters); >>>> +    if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, >>>> def->name))) { >>>> +        virObjectRWLockWrite(obj); >>>> +        virObjectRWUnlock(nwfilters); >>>>              objdef = obj->def; >>>>            if (virNWFilterDefEqual(def, objdef)) { >>>> -            virNWFilterObjPromoteToWrite(obj); >>>>                virNWFilterDefFree(objdef); >>>>                obj->def = def; >>>>                virNWFilterObjDemoteFromWrite(obj); >>>>                return obj; >>>>            } >>>>    -        /* 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 = 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 */ >>>> @@ -462,39 +522,113 @@ >>>> virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, >>>>            return obj; >>>>        } >>>>    +    /* Promote the nwfilters to add a new object */ >>>> +    virObjectRWUnlock(nwfilters); >>>> +    virObjectRWLockWrite(nwfilters); >>> How can this work? What if there's another thread waiting for the write >>> lock (e.g. just to define an NWFilter)? The Unlock(nwfilters) wakes up >>> the other thread, it does its job, unlocks the list waking us in turn. >>> So we lock @nwfilters for writing and add something into the hash table >>> - without all the checks (e.g. duplicity check) done earlier. >> Could we keep the read lock and grab a 2nd lock as a write-lock? It >> wouldn't be a virObject call, though. > That is not possible because write & read lock must exclude each other > by definition. We can grab lock A and then lock B. Lock B would either be a read/write lock locked as a write lock or would be an exclusive lock. Why would that not work?