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>
>>> ---
>>> 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>
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?
Michal