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

[Stefan Berger]

On 02/12/2018 04:38 AM, Michal Privoznik wrote:
> On 02/09/2018 12:47 PM, Stefan Berger wrote:
>> 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 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.
>> 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?
> Oh, I'm misunderstanding. What do you mean by locks A and B?
> virNWFilterObj and virNWFilterObjList locks or something else?

We could use the lock for recursive locking as we do it now. For 
'promoting' to write lock we would grab a 2nd lock that's exclusive.

    Stefan

>
> Michal
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list