On 02/16/2018 04:47 AM, Michal Privoznik wrote: > On 02/16/2018 10:08 AM, Peter Krempa wrote: >> On Fri, Feb 16, 2018 at 09:52:53 +0100, Michal Privoznik wrote: >>> On 02/16/2018 09:34 AM, Pavel Hrdina wrote: >>>> On Mon, Feb 12, 2018 at 01:16:28PM +0100, Michal Privoznik wrote: >>>>> On 02/12/2018 01:10 PM, Peter Krempa wrote: >>>>>> On Mon, Feb 12, 2018 at 11:52:49 +0100, Michal Privoznik wrote: >>>>>>> Sometimes we need the lock in virObjectLockable to be recursive. >>>>>>> Because of the nature of pthreads we don't need a special class >>>>>>> for that - the pthread_* APIs don't distinguish between normal >>>>>>> and recursive locks. >>>>>>> >>>>>>> Based-on-work-of: John Ferlan <jferlan@xxxxxxxxxx> >>>>>>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> >>>>>>> --- >>>>>>> src/libvirt_private.syms | 1 + >>>>>>> src/util/virobject.c | 22 +++++++++++++++++++--- >>>>>>> src/util/virobject.h | 4 ++++ >>>>>>> 3 files changed, 24 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms >>>>>>> index 3b14d7d15..fcf378105 100644 >>>>>>> --- a/src/libvirt_private.syms >>>>>>> +++ b/src/libvirt_private.syms >> >> [...] >> >>> This can be viewed as rewrite of existing code, not completely new code. >>> >>>> >>>> I know that NWFilter code is complex and removing recursive locks is >>>> not an easy task, but for the long run I think it's worth it, it will >>>> make the code cleaner and easier to follow. >>> >>> Right, that the ideal goal. But as I said it's far from happening. I >>> think it was you who when trying to fix some issue in NWFilter drew call >>> graph in NWFilter driver and realized how complicated it is. That's why >>> I don't see it happening anywhere in near future. Also, if we really >>> have multiple entry points as Dan mentioned earlier can we really fix >>> this? I mean there are multiple locks that need to be acquired when >>> touching a virNWFilterObj. The advantage of reentrant mutex is that we >>> will not get a dead lock scenario if two functions fight over lock. >>> >>> Anyway, it's a pity that we are stuck on this patch while reworking the >>> vir*ObjList code. >> >> So and why can't we keep the NWfilter code as-is until the locking is >> sanitized first? It is working so I don't see a reason to try to rewrite >> it to objects if it is not trivially possible. >> That assumes some day locks will be sanitized or the nwfilter code will be rewritten. The chances of that happening are slim and nil AFAICT. Perhaps the "best" one could do is keep a cache of already locked objects to use for the lookup code rather than just using the "normal" find object in list logic. That way the code doesn't need to have recursive locks. Given that change in the code is feared - what chance does making any change at being successful? At least the changes we're discussing now allow the code to use existing mechanisms for object list and object mgmt. Keeping the nwfilter code as-is is an option, but to say it's working would seem to gloss over the fact that it's using recursive locks that can be used for write, but aren't "yet" unless someone removes the nwfilter driver, filter update, and callback locks in the define, undefine, and reload paths that keep any chance of an update in a single threaded path. > > Well, I find it somewhat disappointing. The patches that John and I > proposed make things better. But because they don't make it 100% better > they are NACKed. But I can live with having two different > implementations for vir*ObjList if that's what we want. Or if it's > better than having either John's or mines patches merged. I think > otherwise though. > At some point in time using virObjectLockable for managing refs, locks, and memory reclamation was deemed making things better. At some point in libvirt history virMutexInitRecursive was deemed to be OK, but now it's not for the purpose of using virObject* code for nwfilters. Eventually RWReadLocks came along which by nature allow the same thread to hold multiple concurrent read locks as long as the same number of matching unlocks is done. So it seemed reasonable to me to have virNWFilterObj use an RW lock, but in doing so the problem is we "know" it's a read lock, so in order to update the object getting a write lock is the "normal" thing to do. That meant promotion because it's undefined what happens if you have a read lock and you get a write lock. So maybe we "use" that knowledge that the define, undefine, and reload paths are single threaded and either avoid the promotion altogether or heavily document the NWFilterObj promotion to indicate the issue behind doing this promotion without the additional protection so that no one copies the logic. Or we do nothing because things "work". John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
