Re: [libvirt] [PATCH v2 23/24] virNetworkObjUnsetDefTransient: Lock object list if needed

On 06.03.2015 15:15, Daniel P. Berrange wrote: > On Fri, Mar 06, 2015 at 02:42:34PM +0100, Michal Privoznik wrote: >> On 06.03.2015 14:31, Peter Krempa wrote: >>> On Thu, Mar 05, 2015 at 12:05:24 +0100, Michal Privoznik wrote: >>>> This patch alone does not make much sense, I know. But it >>>> prepares ground for next patch which when looking up a network in >>>> the object list will not lock each network separately when >>>> accessing its definition. Therefore we must have all the places >>>> changing network definition lock the list. >>>> >>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >>>> --- >>>> src/conf/network_conf.c | 9 ++++++++- >>>> src/conf/network_conf.h | 3 ++- >>>> src/network/bridge_driver.c | 4 ++-- >>>> 3 files changed, 12 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c >>>> index 3d318ce..007cebb 100644 >>>> --- a/src/conf/network_conf.c >>>> +++ b/src/conf/network_conf.c >>>> @@ -537,12 +537,19 @@ virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live) >>>> * This *undoes* what virNetworkObjSetDefTransient did. >>>> */ >>>> void >>> >>> I've looked through the next patch and you are basically trying to make >>> the name and UUID pointers for domain immutable or at leas write locked >>> ... >>> >>>> -virNetworkObjUnsetDefTransient(virNetworkObjPtr network) >>>> +virNetworkObjUnsetDefTransient(virNetworkObjListPtr nets, >>>> + virNetworkObjPtr network) >>>> { >>>> if (network->newDef) { >>>> + virObjectRef(network); >>>> + virObjectUnlock(network); >>>> + virObjectLock(nets); >>>> + virObjectLock(network); >>>> + virObjectUnref(network); >>> >>> But I don't really like pulling in the complexity into this helper. >>> >>> >>>> virNetworkDefFree(network->def); >>>> network->def = network->newDef; >>>> network->newDef = NULL; >>>> + virObjectUnlock(nets); >>>> } >>>> } >>> >>> While I like the idea, I'd rather see a conversion to R/W locks or >>> making of the name and UUID pointers immutable than this hack. >> >> Well: >> >> 1) We don't have an virObjectRWLockable or something similar. I can add >> it, but that would postpone merging this patchset for yet another version. >> >> 2) Nor UUID nor name can be made immutable, as we are storing just a >> pointers to network objects in the array. Not UUID or name. It's not a >> hash table like in virDomainObjList* [1]. And when looking up an object, >> we access each object's definition directly. Therefore all other places >> changing definition must lock the object list. > > This is why I changed the virDomainObjList to use a hash instead of a > list when I introduced lockless access for domain objects. > > commit 37abd471656957c76eac687ce2ef94d79c8e2731 > Author: Daniel P. Berrange <berrange(a)redhat.com&gt; > Date: Fri Jan 11 13:54:15 2013 +0000 > > Turn virDomainObjList into an opaque virObject > > As a step towards making virDomainObjList thread-safe turn it > into an opaque virObject, preventing any direct access to its > internals. > > As part of this a new method virDomainObjListForEach is > introduced to replace all existing usage of virHashForEach > > >> 1: Yes, one day we can turn the array into hash table too. There's >> plenty of work to be done. I agree. But I prefer it to be divided into >> smaller pieces instead of this one big patchset of hundreds of patches :-P > > I'd rather expect to see virNetworkObjList turned into an opaque > struct using a virHashTable internally as the very first patch in > the series. Keeping a list which requires linear scans is incompatible > with doing fast lockless code IMHO Yes, this could work. Although, I'm inclined to push patches from beginning till 09/24 and introduce patch turning the array into a hash table right after that. My rationale is that at point of 09/24 whole code uses accessors to the network object list so turning array into hash table could end up being small patch. Objections?