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

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(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? 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list