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

[John Ferlan]

On 07/23/2017 04:46 PM, Michal Privoznik wrote:
> On 07/23/2017 07:21 PM, Pavel Hrdina wrote:
>> 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,
> 
> "you" as in John or as in me?
> 

I'm assuming me.  Still rather than discuss that here, respond to the
cover letter of the other series with you thoughts and concerns. Having
no feedback is far worse in my opinion. I really don't mind if someone
else picks up some other pieces - that's absolutely fine by me. The
consumers and higher counts of objects we have - the more stressed the
current algorithms will get.

This series starts down the path of altering the objlist locks to allow
multiple readers which is good, IMO. I'm OK with the name RW, I would
just prefer that it be lower in the stack and reused amongst all object
types rather than specific to domainobjlist.

As I've already determined, the domainobj code already has flaws with
the object that should be fixed first. In particular, callers to
virDomainObjListAdd have to decide whether to also virObjectRef the
returned object or not based on how they use it and whether they use the
virDomainObjEndAPI or (yuck) a direct virObjectUnlock. The ObjListAdd
only incremented the Ref count once even though it placed the object in
two tables. The corollary ObjListRemove will call virHashRemoveEntry
twice - each decrementing the Ref count.

>> 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.
> 
> Well I think we need two classes. A list of objects is something
> different than objects themselves. You can hardly assign some meaning to
> virDomainObj.lookupByName() or virDomainObjList.startCPUs(). It's
> important to differentiate "data an object is working with" and
> "operations an object can do". The fact that virDomainObjList works with
> virDomainObj doesn't mean it should be in any sense derived. In OOP, if
> class B is derived from class A, it means that B has all the
> attributes/methods that A has, plus something more, e.g. because B is
> more specialized. For instance, assume we have virCarClass. Then,
> virTruckClass or virV8TurboCharged3LClass can both be derived from
> virCarClass. But I can hardly imagine virParkingLotClass doing the same
> thing.
> 
> Michal
> 

Trying to consider the ramifications of virDomainObjList.startCPUs() -
for every (active) domain, start all the CPU's...  Wonder how far that
will get for a Host with 100 domains using 8 vCPU's each ;-).

We may want to think we're OOP, but we're not. If we were it'd be
obj.Ref(), obj.Unref(), obj.Lock(), obj.Unlock(), etc. Instead we have
virObjectRef(obj), virObjectUnref(obj), virObjectLock(obj),
virObjectUnlock(obj). Whether as I've in the other series the code is in
virobject.c or it's in some new virobjhashtable.c (or something, IDC)
doesn't really matter. I kept it in virobject because that was what was
working on the object currently. If we really want to become more OOP
like then that's a very different discussion.

I think you blur the lines with how ObjList and Obj's are used by us.
It's not like any of those virObject* API's are being created for some
external general libvirt provided API can use directly. They are
specific to the needs of the various drivers/objects. ObjList isn't
derived from Obj, but it consumes Obj's for the purposes of API's to be
able to add, query, remove.  There's a close, familial relationship
between the two.


John

On the highways around here, virParkingLogClass occurs every day from
330P to about 630P. It's even worse when virTruckClass collides with
virV8TurboCharged3LClass - that means your virCommuteObject.time() is
extended and your virDriverObject.bloodPressure() gets higher. ;-)