On 02/08/2018 02:34 PM, John Ferlan 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.
>>
>
> I dunno - works in the tests I've run (libvirt-tck and avocado).
>
Oh, now I see why. The nwfilterDefineXML() API still grabs
nwfilterDriverLock() and hence there's nobody else wanting the write
lock since they are waiting on the driver lock. However, I think that
relying on updateLock (i.e. virNWFilterWriteLockFilterUpdates()) is enough.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
