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

Re: [libvirt] [PATCH 0/7] Misc improvements



On Mon, Jul 31, 2017 at 10:26:30AM +0100, Daniel P. Berrange wrote:
On Mon, Jul 31, 2017 at 08:58:41AM +0200, Martin Kletzander wrote:
On Fri, Jul 28, 2017 at 04:58:57PM +0100, Daniel P. Berrange wrote:
> On Fri, Jul 28, 2017 at 11:47:56AM -0400, John Ferlan wrote:
> >
> >
> > On 07/28/2017 11:24 AM, Daniel P. Berrange wrote:
> > > On Fri, Jul 28, 2017 at 11:09:03AM -0400, John Ferlan wrote:
> > >>
> > >>
> > >> On 07/28/2017 10:32 AM, Martin Kletzander wrote:
> > >>> On Thu, Jul 27, 2017 at 01:47:20PM +0200, Michal Privoznik wrote:
> > >>>> As I started to turn more object into using RW locks, I've found
> > >>>> couple of
> > >>>> areas for improvement too.
> > >>>>
> > >>>> Michal Privoznik (7):
> > >>>>  virConnect: Update comment for @privateData
> > >>>>  Report error if virMutexInit fails
> > >>>>  virnetworkobj: Make virNetworkObjFindBy{UUID,Name}Locked() static
> > >>>>    again
> > >>>>  virNetworkObjList: Derive from virObjectRWLockable
> > >>>>  virNodeDeviceObjList: Derive from virObjectRWLockable
> > >>>>  virConnect: Derive from virObjectRWLockable
> > >>>>  storageDriver: Use RW locks
> > >>>>
> > >>>
> > >>> The patches I have not replied to look fine, but I think it would be
> > >>> easier to modify the common object after John's patches.  Are any of
> > >>> those non-conflicting with those series?  If yes, I can review those
> > >>> into more detail.
> > >>>
> > >>
> > >> I had contacted Michal via IRC about this when I saw these hit the list.
> > >> I'd prefer to see them handled via a common object set of patches.
> > >>
> > >> However, that said... I wish the RWLockable hadn't just gone in so
> > >> quickly, but what's done is done. I have a couple of other thoughts in
> > >> this area:
> > >>
> > >>  * I think virObjectLockableRead should return 0/-1 and have the caller
> > >> handle it.
> > >>  * I think there should be a virObjectLockableWrite w/ same return value
> > >> checking.
> > >
> > > I rather disagree with that - it just adds a massive amount more
> > > code to deal with failures from the lock apis that should never
> > > happen unless you've already screwed up somewhere else in your
> > > code. If the object you've passed into the methods has already
> > > been freed, then you're already doomed and trying to recover from
> > > that is never going to be reliable - in fact it could cause more
> > > trouble. The memory for the object passed in is either in the free
> > > pool (and so shouldn't be touched at all), or has been reused and
> > > allocated for some other object now (and so again touching it is
> > > a bad idea). Trying to detect & handle these situatuons is just
> > > doomed to be racy or dangerous or both
> > >
> >
> > I agree w/ the screw up part. Obviously for me it's the RW vs non-RW
> > usage that sent me down this path...
> >
> > Still, I'm not sure what you mean by massive amount of code to deal with
> > failures. I was considering only the RW lock mgmt.  Currently only
> > virdomainobjlist was modified to add virObjectLockRead and only done
> > within the last week. There's 9 virObjectLockRead calls and would be 4
> > virObjectLockWrite calls.
> >
> >     if (virObjectLock{Read|Write}(obj) < 0)
> >         {goto {cleanup|error}|return -1|return NULL};
>
> That's probably buggy if you use existing goto's, because many of
> those cleanup/error locations will call virObjectUnlock(obj), so
> you'll need to introduce another set of gotoo labels to optionally
> skip the unlock step. This is why I think it makes the code more
> complex for dubious benefit.
>
> > The only place this doesn't work properly is the vir*Remove() calls
> > which are void functions. We'd still be "stuck" with them.
>
> Yes that's another scenario I imagined - there are case where it simply
> isn't practical to do cleanup when locking fails.
>
> > Well I can propose the abort() on error if so desired. I agree w/r/t
> > some awful things that could happen...
>
> If we separate  virObjectLock vs virObjectRWLockWrite() then, we can
> just unconditionally reference the object in the virObjectLock method
> and just let the abort happen naturally, without needing explicit abort
>

I agree with most of it, but I can't wrap my head around what you meant
by this paragraph, could you explain it to someone whose brain is just
not working yet, please?

Currently we have:

void
virObjectLock(void *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)");
   }
}


What I'm suggesting is


void
virObjectLock(void *anyobj)
{
   virObjectLockablePtr obj = anyobj;
   virMutexLock(&obj->lock);
}

void
virObjectRWLock(void *anyobj)
{
   virObjectRWLockablePtr obj = anyobj;
   virRWLockWrite(&obj->lock);
}


eg just assume the caller has written code correctly and passing the
right type of object.


So no error checking, not aborts, nothing.  I liked the possibility of
gradual changes from Mutexes to RWLocks when Lock() handled both. I
understand we don't want to have any abort()s in our code, but I'm not
really sure for this one.  I also think we're missing lot of error
handling in virthread (merely due to multiple implementations in the
past?).

Anyway, there will always be room for improvement.


Regards,
Daniel
--
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Attachment: signature.asc
Description: Digital signature


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