On 02/08/2018 02:32 PM, John Ferlan wrote:
>
>
> On 02/08/2018 08:13 AM, 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@xxxxxxxxxx>
>>> ---
>>> 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 dunno, worked for me. The helper is being called from a thread that
> has taken out a READ lock at most one time and needs to get the WRITE
> lock in order to change things. If something else has the READ lock that
> thread waits until the other thread releases the READ lock as far as I
> understand things.
Yes, I can see that. It's just that since the original lock is recursive
I expected things to get recursive and thus I've been thinking how would
this work there, nested in 2nd, 3rd, .. level. But as noted earlier, the
places where lock promoting is used are kind of safe since all the
locking is done at the first level.
>
> Back when I first did this I had quite a lot of debug code and I have a
> vague recollection of seeing output from this particular path and seeing
> a couple of unlocks before the WRITE was taken while running the avocado
> tests.
>
> I have zero interest in spending more time on this. That ship sailed. If
> the decision is to drop the patches, then fine. I tried. I disagree
> that NWFilterObj is not a candidate for RWLockable - in fact it's quite
> the opposite *because* of the recursive locks.
Well, there's a difference between recursive locks and rwlocks. And it's
exactly in what happens in recursion. With recursive locks we don't need
any lock promoting and thus it's safe to do read/write after lock().
With lock promoting (esp. done in unlock()+lockWrite() manner) we get
TOCTOU bugs.
>
> Additionally, because of the recursive locks we cannot use the
> virObjectLockable and quite frankly I see zero benefit from going down
> the path of just virObject. As they say, patches welcome.
>
> Somewhere along the line, I recall trying to post patches that were
> essentially virObjectRecursiveLockable and got NACK'd.
Why is that? IUUC the aim here is to unify all the vir*ObjectList
implementations so that we can drop code duplication. And so far what
I've seen only a couple of virObject* functions are needed
(virObjectRef, virObjectUnref, virObjectLock and virObjectUnlock). So if
we have virObjectRecursiveLockable class then we can still use those 4
functions safely and unify the code here. If you're not interested, I
can cook the patches.
>
>>> @@ -347,26 +426,39 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>>> }
>>>
>>>
>>> + /* Get a READ lock and immediately promote to WRITE while we adjust
>>> + * data within. */
>>> if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
>>>
>>> objdef = obj->def;
>>> if (virNWFilterDefEqual(def, objdef)) {
>>> + virNWFilterObjPromoteToWrite(obj);
>>> virNWFilterDefFree(objdef);
>>> obj->def = def;
>>> + virNWFilterObjDemoteFromWrite(obj);
>>> return obj;
>>> }
>>
>> What is the idea behind this if()? I don't understand it. There doesn't
>> seem to be any fields in @objdef or
>>
>
> Or you lost your train of thought? Happens with this code. The *DefEqual
> ensures that the new definition is the exact same as the old, but
> replaces the def for whatever reason - kind of the redefine type logic.
> If something is different, then we're stuck going through the
> FilterRebuild logic.
>
> That's about the extent of what I understand.
Yes, but the part that I'm not understanding is why we are replacing the
definition in the first place. I mean, if this were some integers
instead of pointers:
int x = 42;
if (struct.member == x) {
struct.member = x;
}
It makes exactly zero sense to me. But whatever, it's pre-existing.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
