On Mon, Mar 09, 2015 at 09:24:32AM +0100, Michal Privoznik wrote:
> 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@xxxxxxxxxx>
> >>>> ---
> >>>> 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@xxxxxxxxxx>
> > 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?
That sounds like a reasonable approach - iirc that's the way I did it
for the domain object list too.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list