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

On 07/24/2017 09:04 AM, Michal Privoznik wrote: > On 07/23/2017 11:33 PM, John Ferlan wrote: >> >> >> 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. > > Sure. I should have said that out loud - if this gets merged I can copy > it to other vir*ObjLists. Hopefully that doesn't cause problems on your > side. > Well I have no virdomainobjlist patches in my trees, but changes to virobject.{c,h} will have 'conflicts'. Of course I see you've already pushed so whatever - I'll have to deal with it now in my branches and will have to put together a new version of what I posted last month <sigh>. >> >> 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. > > Yep. I think we've discussed this (or maybe it was some different type, > like nwfilters? doesn't matter really). I recall me saying we should go > with the former. That is, vir*ObjListAdd should always return refed & > locked object. The reasoning is that *every* API which works with the > object should take a reference instead of relying on the one in the > list. As a nice result, every API can then just use vir*ObjEndAPI. > Yes - I think it was during the nwfilter review. >> 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. [1] >>> >>> 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 ;-). > > Yeah well. We don't have an API that works over multiple domains. But it > might sure be interesting :-) > >> >> 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). > > I think it's just a matter of syntax whether I write obj.method(); or > method(obj). The real OOP-ism lies in having compiler taking care of > ref/unref. > This was one of those points that was on the gray area between this series and the comments about the other series. Calling out usage of obj.FUNCTION() vs. FUNCTION(obj) was just a way to point out that we're really not OOP in the grand scheme of things. We use API names to represent more what a function does and try to group together code as best as possible with the eye towards similar functionality being together. [1] Since we've already gone down the path of having one module for virObjectClass from which virObjectLockable builds off it and is used by both *Obj and *ObjList, then why create a virobjectlist.c (or virobjecthash.c) in order to contain API's for list/hash mgmt. This (now pushed) patch series creates the precedence that Object *ObjList mgmt functions can stay in virobject.c, which works better for me.

>> 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. > > No, that's not what I'm calling for. > >> >> 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. > > Well, sure. ObjList is for us storing Objs. But I view ObjList as > unaware of what it's storing. Just like our hash tables. For them EXACTLY! This is why I started down this path... Of course I want far too generic for some people's preferences by going with primaryKey and secondaryKey type nomenclature, so I was forced by review to use UUID/Name which are far less generic, but "tie" together the various vir*obj.c consumers which is fine although would seemingly go against the premise that ObjList shouldn't be aware of what it's storing. It should only care that it's using a some Key rather than a named Key.

I also went with ObjHash since that's what it represents moreso than an ObjList. I suppose we could have both, but who would use a forward linked list for searching when hash table lookups are around? The one value for an ObjList I could see would be ordered operations - e.g. as a way to use as a queue of sorts to append on a tail operation while some other thread pulls off the head. But for storing persistent objects, hash is a better option. I still think the "RW" part should have been hidden by a new object that is to be primarily used for list/hash objects... What's to stop someone from thinking that virObjectRWLockable could be used for a vir*Obj object that goes into a list (or hash table)? I also have some specific comments for patch 2 which I'll leave there as a response... John > everything's void *. And it's only because we use ObjList to actually > store virObject that it can call 'virObjectRef(vm)' (where vm is say > virDomainObj) before actually returning it. Otherwise ObjList is blind > (or at least should be) to what it's storing. > >> >> >> 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. ;-) >> > > Yep. Same here. Thank God for navigation SW that collects data from > other users and re-route to avoid traffic jams (if possible). > > Michal >