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

Daniel P. Berrange berrange at redhat.com
Fri Mar 6 14:15:52 UTC 2015


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 at 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 at 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

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 :|




More information about the libvir-list mailing list