[libvirt] [PATCH v2 0/6] nwfilter adjustments for common object
jferlan at redhat.com
Wed Aug 2 11:27:15 UTC 2017
perhaps at least the first 3...
I'm now beginning to think/wonder if using the rwlock_rdlock would be
the solution at least for nwfilter objs. It seems from a quick scan of
the man page that they are designed to be recursive as long as the
consumer guarantees that there is an Unlock for every LockRead. A lot
better than rolling my own recursive lock algorithm that I tried in
patch 4. Would require some other adjustments (and concessions) along
the way, but seemingly possible.
On 07/18/2017 04:57 PM, John Ferlan wrote:
> v1: https://www.redhat.com/archives/libvir-list/2017-June/msg00079.html
> Changes since v1 (if I can recall all of them!):
> Patches 1, 4, 8-13 were pushed
> Patches 2, 3, 5-7 are dropped
> This this is a rework of patches 14-17
> Patch 1 (former patch14):
> * Requested adjustments made to ACK'd patch, but since this and the
> remaining ones were related I didn't yet push it.
> Patch 2 (new):
> * From review though... As it turns out, virNWFilterDefEqual doesn't
> require the @cmpUUIDs patch.
> Patch 3 (fromer patch 15):
> * Fixed the line as requested. Patch was ACK'd by like Patch 1 I held
> onto it since it was related.
> Patch 4 (former patch 16):
> * Let's call it a complete rewrite. Rather than rely solely on the
> refcnt of the object, I've added/implemented a 'trylock' mechanism
> which essentially will allow the subsequent patch to use the
> virObjectLockable (e.g. a non recursive lock). Of course as I got
> further into the code - I think I've come to the conclusion that
> there isn't a way for a @def to disappear between threads with a
> refcnt only mechanism because there's a few other serialized locks
> which would need to be hurdled before hand. Still as I found out
> while running the Avocado test 'nwfilter_update_vm_running.update_arp_rule'
> the recursion would occur because the AssignDef code would call the
> Instantiation with the lock from the def being updated and that's
> where all the awful magic needs to occur. Additionally, I found that
> one wouldn't want to attempt to lock the nwfilters list inside the
> virNWFilterObjListFindInstantiateFilter because AssignDef already
> had that lock. I debated needing a recursive lock there until I
> came to the conclusion that the list couldn't change because the
> DefineXML is protected by a driver level lock (as is the Undefine
> and Reload paths).
> Patch 5 (former patch 17):
> * No changes, it was ACK'd, but without 1-4 is useless
> Patch 6 (NEW):
> * Remove the need for the driver level lock for a few API's since
> we have self locking nwfilters list. Also left comments in the
> 3 places where that lock remained to hopefully cause someone great
> anxiety if they decided to attempt to remove the lock without
> first consulting a specialist.
> NB: Ran all of the changes through the 'nwfilter' tests found in
> the Avocado test suite.
> John Ferlan (6):
> nwfilter: Add @refs logic to __virNWFilterObj
> nwfilter: Remove unnecessary UUID comparison bypass
> nwfilter: Convert _virNWFilterObjList to be a virObjectLockable
> nwfilter: Remove recursive locking for nwfilter instantiation
> nwfilter: Convert virNWFilterObj to use virObjectLockable
> nwfilter: Remove need for nwfilterDriverLock in some API's
> src/conf/virnwfilterobj.c | 635 ++++++++++++++++++++++++---------
> src/conf/virnwfilterobj.h | 12 +-
> src/libvirt_private.syms | 6 +-
> src/nwfilter/nwfilter_driver.c | 66 ++--
> src/nwfilter/nwfilter_gentech_driver.c | 66 +++-
> src/util/virobject.c | 24 ++
> src/util/virobject.h | 4 +
> src/util/virthread.c | 5 +
> src/util/virthread.h | 1 +
> 9 files changed, 586 insertions(+), 233 deletions(-)
More information about the libvir-list