[libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable

[Michal Privoznik]

On 07/25/2017 05:47 PM, John Ferlan wrote:
> 
> 
> On 07/25/2017 10:25 AM, Michal Privoznik wrote:
>> On 07/25/2017 02:13 PM, John Ferlan wrote:
>>>
>>>
>>> On 07/25/2017 03:45 AM, Michal Privoznik wrote:
>>>> On 07/24/2017 05:22 PM, John Ferlan wrote:
>>>>> [...]
>>>>>
>>>>>>  /**
>>>>>>   * virObjectLock:
>>>>>> - * @anyobj: any instance of virObjectLockablePtr
>>>>>> + * @anyobj: any instance of virObjectLockable or virObjectRWLockable
>>>>>>   *
>>>>>> - * Acquire a lock on @anyobj. The lock must be
>>>>>> - * released by virObjectUnlock.
>>>>>> + * Acquire a lock on @anyobj. The lock must be released by
>>>>>> + * virObjectUnlock. In case the passed object is instance of
>>>>>> + * virObjectRWLockable a write lock is acquired.
>>>>>>   *
>>>>>>   * The caller is expected to have acquired a reference
>>>>>>   * on the object before locking it (eg virObjectRef).
>>>>>> @@ -340,31 +382,69 @@ virObjectGetLockableObj(void *anyobj)
>>>>>>  void
>>>>>>  virObjectLock(void *anyobj)
>>>>>>  {
>>>>>> -    virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
>>>>>> +    if (virObjectIsClass(anyobj, virObjectLockableClass)) {
>>>>>> +        virObjectLockablePtr obj = anyobj;
>>>>>> +        virMutexLock(&obj->lock);
>>>>>> +    } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
>>>>>> +        virObjectRWLockablePtr obj = anyobj;
>>>>>> +        virRWLockWrite(&obj->lock);
>>>>>> +    } else {
>>>>>> +        virObjectPtr obj = anyobj;
>>>>>> +        VIR_WARN("Object %p (%s) is not a virObjectLockable "
>>>>>> +                 "nor virObjectRWLockable instance",
>>>>>> +                 anyobj, obj ? obj->klass->name : "(unknown)");
>>>>>> +    }
>>>>>> +}
>>>>>>  
>>>>>> -    if (!obj)
>>>>>> -        return;
>>>>>>  
>>>>>> -    virMutexLock(&obj->lock);
>>>>>> +/**
>>>>>> + * virObjectLockRead:
>>>>>> + * @anyobj: any instance of virObjectRWLockablePtr
>>>>>> + *
>>>>>> + * Acquire a read lock on @anyobj. The lock must be
>>>>>> + * released by virObjectUnlock.
>>>>>> + *
>>>>>> + * The caller is expected to have acquired a reference
>>>>>> + * on the object before locking it (eg virObjectRef).
>>>>>> + * The object must be unlocked before releasing this
>>>>>> + * reference.
>>>>>> + */
>>>>>> +void
>>>>>> +virObjectLockRead(void *anyobj)
>>>>>> +{
>>>>>> +    if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
>>>>>> +        virObjectRWLockablePtr obj = anyobj;
>>>>>> +        virRWLockRead(&obj->lock);
>>>>>> +    } else {
>>>>>> +        virObjectPtr obj = anyobj;
>>>>>> +        VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
>>>>>> +                 anyobj, obj ? obj->klass->name : "(unknown)");
>>>>>> +    }
>>>>>>  }
>>>>>>  
>>>>>
>>>>> Hopefully no one "confuses" which object is which or no one starts using
>>>>> virObjectLockRead for a virObjectLockable and expects that read lock to
>>>>> be in place (or error out) or gasp actually wait for a write lock to
>>>>> release the lock as this does not error out.
>>>>
>>>> This could have already happened if one would pass bare virObject to
>>>> virObjectLock().
>>>>
>>>
>>> Consider passing a non virObject (such as what happened during an early
>>> change to the code for me - a virHashTablePtr) to virObjectLock...
>>
>> Well, that's what happens in C to functions accepting void pointer. In
>> this sense yes, we are not true OOP - compiler would catch that you're
>> calling a method that is not defined for the class. But since we're
>> implementing OOP in runtime, we are able to catch OOP related problem
>> only at runtime. I'm not sure it is something we can check for at
>> compile time. And I think other C libraries that re-implement OOP (like
>> glib) do just the same as us.
>>
> 
> Agreed - I cannot think of a way to capture this at compile time, but as
> part of that virObject series I did add some code to reduce the chance
> of some bad things happening during run time. They don't completely
> eliminate the possibility that someone calls virObject{Lock|Unlock}
> using an @obj that isn't a virObjectLockable.  But it does protect
> against someone using virObject{Ref|Unref} in order to avoid an
> virAtomicIntInc or a virAtomicIntDecAndTest on a random @obj field (plus
> some worse settings on random fields if for some reason @lastRef is true).
> 
> I hadn't come up with a good way to handle virObject{Lock|Unlock}, but
> this series jiggled the memory a bit and got me thinking...
> 
>>>
>>> In any case, we'll have to be more vigilant now to know that only
>>> ObjList @obj's for virdomainobjlist can use this new API.
>>
>> Why? Maybe I'm misunderstanding what you're saying, but I think that
>> network driver, and basically any other driver which uses vir*ObjList
>> can use it. And not just lists. Other objects too - e.g. qemuCaps,
>> virDomainCaps, etc (I really roughly skimmed through users of
>> virObjectLockable).
> 
> Ok - wasn't as clear as I should have been... True, as long as the
> _vir*ObjList uses virObjectRWLockable and the vir*ObjListNew() uses
> virObjectRWLockableNew. Similar for other objects that want to do the
> right thing.
> 
> I was considering someone that sees virObjectLockRead and tries to use
> it thinking - great, that's what I want - just a read lock since I'm not
> changing anything. But, they may not really get the Read lock if they
> pass a virObjectLockable... If not playing close attention during review
> to ensure that the call is done on an appropriate @obj (especially when
> both are used in some same function), then something may be missed.

Yes, there is this concern and I understand it. But to me it's at the
same level as making sure that object passed to virObjectLock() is
virObjectLockable. Then again, warning is produced at runtime, so as a
part of developing/review process when testing the patches warning would
be produced.

> 
> TBH: We may find "other" uses of concurrent read locks even for @obj's.
> IIUC, if an @objlist has a read/RW lock, then an Add/Remove path cannot
> get the virMutexLock until that read lock is Unlocked. Likewise, there
> is @obj code that gets the virMutexLock even though it's not changing
> anything - it's just reading some field. Shouldn't it be safe to have
> multiple readers in this case too? I think of all those list traversal
> functions which grab the lock just to read/check/copy something. Because
> we have an RW lock on the @objlist, the contention for locks between
> threads now jumps to the @obj's.

Yes, it definitely looks like it. However, it's slightly more
complicated than that. At least in qemu we have these jobs which use
condition to wait until the job can be set. The code looks something
like this:

  pthread_cond_timedwait(vm->priv->job.cond, vm->parent.lock, then);

where @cond is a pthread condition, @lock is a pthread mutex and @then
is some timeout. Unfortunately, in pthread impl @lock really has to be a
mutex, not an RW lock (see man pthread_cond_timedwait). I have an idea
how to avoid this - move mutex to priv, right next to the condition, but
that's completely different discussion. For now it's sufficient to say
that we can't simply replace mutex for rw lock in virDomainObj. Some
additional work is needed. But it is still feasible, I think.

> 
>>
>> And @obj's are untouched with this change, aren't they? I feel like
>> we're not on the same page here. My change just allowed two threads to
>> look up domains at the same time. It doesn't affect domains in any way.
>>
> 
> I understand what the change did...  Hopefully the above helps explain
> my thoughts better.
> 
>>> Longer term
>>> I'll adjust to use them.
>>>
>>>>>
>>>>> This is a danger in the long term assumption of having a void function
>>>>> that has callers that can make the assumption that upon return the Lock
>>>>> really was taken.
>>>>
>>>> Well, I agree that this is one more thing to be cautious about, but no
>>>> more than making sure object passed to virObjectLock() is virObjectLockable.
>>>>
>>>
>>> There are some ugly ways to handle this including using some kind of
>>> macro for virObjectLock that would force an abort of some sort. We have
>>> been "fortunate" to have well behaved and reviewed code, but with two
>>> lock API's now it's just one of those things to keep track of for reviews.
>>
>> I don't find this change that frightening, but I agree that bare
>> VIR_WARN() we have might not be enough. There are plenty ways where
>> malicious pointers can be passed to virObject* APIs, and especially
>> virObjectLock(). Worst case scenario we might pass some "random" address
>> in memory, make the caller think the object is locked while in fact it
>> is not. We are strongly against abort(), but maybe we can revisit that
>> decision in this case because it can lead to data loss/corruption. This
>> error is different to OOM error or 'unable to start a domain' one.
>>
> 
> Currently way too many virObjectLock's and virObjectRef's to make it
> possible to perform checking reasonably. The abort() is a possibility,
> but yes, ugly albeit perhaps safer than corruption.
> 
> Still new code could have added error checking...
> 
>>>
>>>>>
>>>>> Perhaps the better way to attack this would have been to create a
>>>>> virObjectLockWrite() function called only from vir*ObjListAdd and
>>>>> vir*ObjListRemove leaving the other virObjectLock()'s in tact and having
>>>>> the virObjectLock() code make the decision over whether to use the
>>>>> virRWLockRead or virMutexLock depending on the object class.
>>>>
>>>> What's the difference? If virObjectLock() does what you're suggesting
>>>> (what indeed is implemented too), what's the point in having
>>>> virObjectLockWrite()?
>>>>
>>>> Michal
>>>>
>>>
>>> 1. Less code and a lock that could check status... I understand not
>>> wanting to modify 10 callers to check status, but modifying two and thus
>>> making it clearer to the caller that these are "different" might not be
>>> a bad thing.
>>
>> Hold on. If we are going to make virObjectLock() return an error - why
>> do it just for virObjectRWLockable? Doing the following is no less
>> worse:
>>
>> char a[] = "Some random string";
>> virObjectLock(a);
>>
> 
> char a[] = "Some random string";
> 
> if (!virObjectLock{Read|Write}(a))
>     virReportError("you ain't got it");
>     return -1>
> 
>>>
>>> 2. The "default" action being more consumers probably will use the Read
>>> locks (as I believe is the premise/impetus of the series). The
>>> Add/Remove would seemingly be used less and thus the exception. So why
>>> not have the default for ObjList/virObjectRWLockableClass be to have the
>>> virObjectLock use a virRWLockRead instead of virRWLockWrite?
>>
>> I beg to disagree. Whenever I see 'virObjectLock' I am sure that no
>> other thread is changing the object that is guarded by the mutex. If,
>> however, virObjectLock was grabbing read lock, it would be very
>> confusing and I would have to check every time whether the object I'm
>> passing is virObjectLockable or virObjectRWLockable because the
>> virObjectLock() function would behave differently depending on class of
>> the object.
> 
> OK - fair argument for the opposing viewpoint. Makes sense.
> 
> Still virObject{Lock|Unlock} is overloaded to do different things based
> on the object type which I recall being something I've been dinged on in
> the past.
> 
> Considering the argument about checking the return value, it may
> {be|have been} a good opportunity to add checking to getting the lock.
> 
>>
>> Moreover, now I can do the following and the code still works:
>>
>> diff --git i/src/conf/virnetworkobj.c w/src/conf/virnetworkobj.c
>> index ccde72e72..4fe13fc40 100644
>> --- i/src/conf/virnetworkobj.c
>> +++ w/src/conf/virnetworkobj.c
>> @@ -60 +60 @@ virNetworkObjOnceInit(void)
>> -    if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(),
>> +    if (!(virNetworkObjListClass = virClassNew(virClassForObjectRWLockable(),
>> diff --git i/src/conf/virnetworkobj.h w/src/conf/virnetworkobj.h
>> index 8090c2e24..ee4a939f2 100644
>> --- i/src/conf/virnetworkobj.h
>> +++ w/src/conf/virnetworkobj.h
>> @@ -30 +30 @@ struct _virNetworkObj {
>> -    virObjectLockable parent;
>> +    virObjectRWLockable parent;
>>
>>
>> With your suggestion, I'd have to change all virObjectLock() in
>> src/conf/virnetworkobj.c to virObjectLockWrite() (all that lock the hash
>> table, not network object itself).
>>
> 
> Obviously I hadn't completely through everything...
> 
> But perhaps this also proves that under the covers we could have just
> converted virObjectLockable to be a virObjectRWLockable without creating
> the new class. Especially since the former patch 1 has been reverted and
> there's no difference between virObjectLockableNew and
> virObjectRWLockableNew other than which class was used to initialize.

We can't do that because of pthread condition variables.

Michal