[libvirt PATCH v2] nwfilter: merge updateMutex and updateLock

[Michal Prívozník]

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 at redhat.com>
> ---
>  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 at redhat.com>

Michal