[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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.

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.

> 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.

If they were combined and just the new API to perform the RW lock was
added, then for paths that want to use read locks:

   if (!virObjectLockRead(obj))
      error and fail


IOW: At this point in time - what is the purpose of a separate


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]