On 3/17/22 16:30, Daniel P. Berrangé wrote: > The updateLock is a R/W lock held by anything which needs to read or > modify the rules associated with an NWFilter. > > APIs for defining/undefining NW filters rules hold a write lock on > updateLock. > > APIs for creating/deleting NW filter bindings hold a read lock on > updateLock, which prevents define/undefine taking place concurrently. > > The problems arise when we attempt to creating two NW filter bindings in > parallel. > > Thread 1 can acquire the mutex for filter A > > Thread 2 can acquire the mutex for filter B > > Consider if filters A and B both reference filters C and D, but in > different orders: > > Filter A > -> filter C > -> filter D > > Filter B > -> filter D > -> filter C > > Thread 1 will try to acquire locks in order A, C, D while thread 1 will > try to acquire in order A, D, C. Deadlock can still occur. > > Think we can sort the list of filters before acquiring locks on all of > them ? Nope, we allow arbitrary recursion: > > Filter A > -> filter C > -> filter E > -> filter F > -> filter H > -> filter K > -> filter D > -> filter G > -> filter I > > So we can't tell from looking at 'A' which filters we're going to > need to lock. We can only see the first level of filters references > and we need to lock those before we can see the second level of > filters, etc. > > We could probably come up with some cleverness to address this but > it isn't worth the time investment. It is simpler to just keep the > process of creating NW filter bindings totally serialized. > > Using two separate locks for this serialization though is pointless. > > Every code path which gets a read(updateLock) will go on to hold > updateMutex. It is simpler to just hold write(updateLock) and > get rid of updateMutex. At that point we don't need updateLock > to be a R/W lock, it can be a plain mutex. > > Thus this patch gets rid of the current updateLock and updateMutex > and introduces a new top level updateMutex. > > This has a secondary benefit of introducing fairness into the > locking. With a POSIX R/W lock, you get writer starvation if > you have lots of readers. IOW, if we call virNWFilterBIndingCreate > and virNWFilterBindingDelete in a tight loop from a couple of > threads, we can prevent virNWFilterDefine from ever acquiring > a write lock. > > Getting rid of the R/W lock gives us FIFO lock acquisition > preventing starvation of any API call servicing. > > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx> > --- > src/conf/nwfilter_conf.c | 33 +--------- > src/conf/nwfilter_conf.h | 9 --- > src/conf/virnwfilterobj.h | 3 + > src/libvirt_private.syms | 3 - > src/nwfilter/nwfilter_driver.c | 77 ++++++++++++------------ > src/nwfilter/nwfilter_gentech_driver.c | 83 +++----------------------- > 6 files changed, 52 insertions(+), 156 deletions(-) Reviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx> Michal
