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(-)
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(-) -- 2.13.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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: 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. Just thinking off the cuff and of course trying to keep stuff I've been working on fresh ;-) John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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. > > 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. > > 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 :-) Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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. ;-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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. > > 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. > 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 ;-). 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. > 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 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jul 24, 2017 at 11:12:01AM -0400, John Ferlan wrote:
>
>
> 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.
No, there is a difference, the virObject and virObjectLockable and
virObjectRWLockable are generic objects that are newer used directly and
are used only as parents for other child objects like virDomainObj.
The virHashTable isn't even derived from virObject, it only operates
with virObject. The same way virObjectList should be implemented.
>
> >> 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.
You are saying exactly and yet you are introducing new
virObjectLookupKeys which is used instead of virObjectLockable just to
make the new listing virObjectLookupHash to work and that's wrong. It
should work even with virObject.
Pavel
> 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
> >
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/24/2017 02:33 PM, Pavel Hrdina wrote: >> 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. > > You are saying exactly and yet you are introducing new > virObjectLookupKeys which is used instead of virObjectLockable just to > make the new listing virObjectLookupHash to work and that's wrong. It > should work even with virObject. > > Pavel > This really isn't the right place for a discussion about the other series - the merits of that solution should be discussed there... Still, I must be missing something. Why is it wrong to create a new object that would have a specific use? virObjectLockable was created at some point in history and then used as a way to provide locking for a virObject that only had a @ref. Someone could still create a virObject as well and just get @refs. With the new objects based on those two objects you get a few more features that allow for add, search, lookup, remove. But you call that wrong, which is confusing to me. It doesn't make much sense to have a hash table with objects that have no way of being looked up does it? How do you add the object? Even the virnode* uses a hash table that has objects that have UUID and Name used for lookup. A virObjectLockable has a @ref and @lock - there's *nothing* in there that can be used for a key for a hash table. So what in your opinion would be the use of an object in a table that cannot be fetched. So "something" has to create an object that uses the existing virObjectLockable as a base and allows for it to be build upon in order to be more useful. A new object class needs to be created and used. If something still wants to use virObjectLockable and build their own who knows what in order to manage the virObjectLockable's they still can. The virObjectLockable isn't going away, it's being extended. After initial series way back in February: https://www.redhat.com/archives/libvir-list/2017-February/msg00519.html I was encouraged to take a more object oriented view, thus the follow up RFC in April: https://www.redhat.com/archives/libvir-list/2017-April/msg00321.html which got no feedback, so in late May it's: https://www.redhat.com/archives/libvir-list/2017-May/msg01178.html which got some feedback and a quick v2 in early June: https://www.redhat.com/archives/libvir-list/2017-June/msg00070.html that got more feedback, but mainly that the generic primaryKey and secondaryKey were not favored. So, v3 was generated in mid June that was less abstract: https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html which again gets zero feedback, but apparently has been read. Still no where in any of this work has it been said using these types of objects was wrong. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Sun, Jul 23, 2017 at 10:46:15PM +0200, 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? Oh right, I should be exact, John. > > > 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 Well, I know how all this work. What I meant is that virDomainObj would be still derived from virObjectLockable or virObjectRWLockable and there would be virObjectList class which would implement the lookup functions, addObj, removeObj, and so on. You would create a new instance of virObjectList class and fill that instance with domain objects that you need to list. The domain object itself doesn't have to know anything about the virObjectList class. To use the similar explanation, you have virObjectClass (virObjectLockableClass) and you have virCarClass (virDomainObjClass) and virTruckClass (virStoragePoolClass) and there would be virParkingClass (virObjectListClass). The virCarClass is not derived from virVehicleParkableClass (virObjectLookupKeys) in order to have virParkingClass (virObjectLookupHash) to allow virCarClass to park. That's what I don't like about the current implementation. Pavel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.