[libvirt] [PATCH v5 1/3] nwfilter: Convert _virNWFilterObj to use virObjectRWLockable

[Stefan Berger]

On 02/08/2018 08:30 AM, Daniel P. Berrangé wrote:
> On Thu, Feb 08, 2018 at 02:13:58PM +0100, Michal Privoznik wrote:
>> On 02/06/2018 08:20 PM, John Ferlan wrote:
>>> Unlike it's counterparts, the nwfilter object code needs to be able
>>> to support recursive read locks while processing and checking new
>>> filters against the existing environment. Thus instead of using a
>>> virObjectLockable which uses pure mutexes, use the virObjectRWLockable
>>> and primarily use RWLockRead when obtaining the object lock since
>>> RWLockRead locks can be recursively obtained (up to a limit) as long
>>> as there's a corresponding unlock.
>>>
>>> Since all the object management is within the virnwfilterobj code, we
>>> can limit the number of Write locks on the object to very small areas
>>> of code to ensure we don't run into deadlock's with other threads and
>>> domain code that will check/use the filters (it's a very delicate
>>> balance). This limits the write locks to AssignDef and Remove processing.
>>>
>>> This patch will introduce a new API virNWFilterObjEndAPI to unlock
>>> and deref the object.
>>>
>>> This patch introduces two helpers to promote/demote the read/write lock.
>>>
>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>> ---
>>>   src/conf/virnwfilterobj.c              | 193 +++++++++++++++++++++++----------
>>>   src/conf/virnwfilterobj.h              |   9 +-
>>>   src/libvirt_private.syms               |   3 +-
>>>   src/nwfilter/nwfilter_driver.c         |  15 +--
>>>   src/nwfilter/nwfilter_gentech_driver.c |  11 +-
>>>   5 files changed, 153 insertions(+), 78 deletions(-)
>>>
>>> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
>>> index 87d7e7270..cd52706ec 100644
>>> --- a/src/conf/virnwfilterobj.c
>>> +++ b/src/conf/virnwfilterobj.c
>>> @@ -34,7 +34,7 @@
>>>   VIR_LOG_INIT("conf.virnwfilterobj");
>>>   
>>>   struct _virNWFilterObj {
>>> -    virMutex lock;
>>> +    virObjectRWLockable parent;
>>>   
>>>       bool wantRemoved;
>>>   
>>> @@ -47,27 +47,69 @@ struct _virNWFilterObjList {
>>>       virNWFilterObjPtr *objs;
>>>   };
>>>   
>>> +static virClassPtr virNWFilterObjClass;
>>> +static void virNWFilterObjDispose(void *opaque);
>>> +
>>> +
>>> +static int
>>> +virNWFilterObjOnceInit(void)
>>> +{
>>> +    if (!(virNWFilterObjClass = virClassNew(virClassForObjectRWLockable(),
>>> +                                            "virNWFilterObj",
>>> +                                            sizeof(virNWFilterObj),
>>> +                                            virNWFilterObjDispose)))
>>> +        return -1;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +VIR_ONCE_GLOBAL_INIT(virNWFilterObj)
>>> +
>>>   
>>>   static virNWFilterObjPtr
>>>   virNWFilterObjNew(void)
>>>   {
>>>       virNWFilterObjPtr obj;
>>>   
>>> -    if (VIR_ALLOC(obj) < 0)
>>> +    if (virNWFilterObjInitialize() < 0)
>>>           return NULL;
>>>   
>>> -    if (virMutexInitRecursive(&obj->lock) < 0) {
>>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>>> -                       "%s", _("cannot initialize mutex"));
>>> -        VIR_FREE(obj);
>>> +    if (!(obj = virObjectRWLockableNew(virNWFilterObjClass)))
>>>           return NULL;
>>> -    }
>>>   
>>> -    virNWFilterObjLock(obj);
>>> +    virObjectRWLockWrite(obj);
>>>       return obj;
>>>   }
>>>   
>>>   
>>> +static void
>>> +virNWFilterObjPromoteToWrite(virNWFilterObjPtr obj)
>>> +{
>>> +    virObjectRWUnlock(obj);
>>> +    virObjectRWLockWrite(obj);
>>> +}
>> How can this not deadlock? This will work only if @obj is locked exactly
>> once. But since we allow recursive locks any lock() that happens in the
>> 2nd level must deadlock with this. On the other hand, there's no such
>> case in the code currently. But that doesn't stop anybody from calling
>> PromoteWrite() without understanding its limitations.
>>
>> Maybe the fact that we need recursive lock for NWFilterObj means it's
>> not a good candidate for virObjectRWLocable? It is still a good
>> candidate for virObject though.
>>
>> Or if you want to spend extra time on this, maybe introducing
>> virObjectRecursiveLockable may be worth it (terrible name, I know).
> I can't remember exactly call trace scenarios that meant we required
> recursive locking. I vaguely recall is was related to fact that we
> have an alternative entry point into the nwfilter code that's triggered
> by the virt drivers. I'm thus vaguely hoping that when we split nwfilter
> off into a separate daemon from virt driver, the need for recursive
> lockingn would magically disappear. I could be completely wrong though :-)

The nwfilter code tries to detect circular references of filters, like 
a->b->c->a, and while testing for that needs to grab a read lock twice.

    Stefan

>
>
> Regards,
> Daniel