[libvirt] [PATCH 0/3] Introduce RW locks to virDomainObjList

[Pavel Hrdina]

On Sun, Jul 23, 2017 at 02:33:49PM +0200, Michal Privoznik wrote:
> On 07/23/2017 02:10 PM, John Ferlan wrote:
> > 
> > 
> > On 07/19/2017 10:31 AM, Michal Privoznik wrote:
> >> While this is not that critical (hash tables have O(1) time complexity for
> >> lookups), it lays down path towards making virDomainObj using RW locks instead
> >> of mutexes. Still, in my limited testing this showed slight improvement.
> >>
> >> Michal Privoznik (3):
> >>   virthread: Introduce virRWLockInitPreferWriter
> >>   virobject: Introduce virObjectRWLockable
> >>   virdomainobjlist: Use virObjectRWLockable
> >>
> >>  src/conf/virdomainobjlist.c |  24 ++++----
> >>  src/libvirt_private.syms    |   4 ++
> >>  src/util/virobject.c        | 144 ++++++++++++++++++++++++++++++++++----------
> >>  src/util/virobject.h        |  16 +++++
> >>  src/util/virthread.c        |  35 +++++++++++
> >>  src/util/virthread.h        |   1 +
> >>  6 files changed, 180 insertions(+), 44 deletions(-)
> >>
> > 
> > This could be a "next step" in the work I've been doing towards a common
> > object:
> 
> Sure. If we have just one common object the change can be done in one
> place instead of many. I don't care in what order are the changes merged.

I'm still not sure about the implementation that you are heading to,
I would actually prefer something similar to the current
virDomainObjList implementation, create a new module in utils called
virObjectList and make it somehow generic that it could be used by our
objects.  I personally don't like the fact that there will be two new
classes, one that enables using the other one.

> > 
> > https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html
> > 
> > which moves all those driver/vir*obj list API's for Lookup, Search,
> > ForEach, Add, Remove, etc. into object code since they're essentially
> > all following the same pattern.
> > 
> > Once there - altering the Lockable lock under the covers should be
> > relatively simple. I would be called a ListLock or HashLock instead of
> > an RWLock and the implementation is such that it's R or W depending on
> > API. Taking a quick refresher look at the series, for a new object I
> > call virObjectLookupHashClass - that could be the integration point to
> > use a local initialization for the class and then the appropriate lock
> > style depending on API.
> 
> I think I still prefer "RWLock" name over "ListLock" or "HashLock" since
> its name clearly suggests its purpose. I mean, ListLock doesn't say it's
> an RW lock. Moreover, as I say in the cover letter, I'd like to use RW
> locks for virDomainObj one day (for instance, there's no reason why two
> clients cannot fetch XML for the same domain at the same time).
> Therefore, it looks correct to me to derive virDomainObjClass from
> virObjectRWLockable instead of ListLock or HashLock or something.

I agree that RWLock is more descriptive about what it is.  And I also
agree that deriving virDomainObjClass from virObjectRWLockable is
better.  As I've already wrote above, the generic listing code should
work without a need to somehow modify the existing objects.

Just a note, I like the idea that there will be only one implementation
for listing object that will be reused, however the current proposed
implementation isn't that convincing to me.

> > 
> > Just thinking off the cuff and of course trying to keep stuff I've been
> > working on fresh ;-)
> 
> Sure, the more recent some patches are the more I understand them. Same
> here :-)

I think that this can be easily merged before the work for listing
object gets finished since it can be used for object that doesn't
require listing.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170723/88c79d76/attachment-0001.sig>