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@xxxxxxxxxx>
>>>>> ---
>>>>> 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?
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
