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

Michal Privoznik mprivozn at redhat.com
Fri Feb 9 06:48:19 UTC 2018


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 at redhat.com>
>>> ---
>>>   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.

Michal




More information about the libvir-list mailing list