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