Re: Wait free LW_SHARED acquisition - v0.2

On 2014-10-22 13:32:07 +0530, Amit Kapila wrote:
> On Tue, Oct 21, 2014 at 7:56 PM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > On Wed, Oct 8, 2014 at 6:17 PM, Andres Freund <andres@2ndquadrant.com>
> wrote:
> > >
> > > On 2014-06-25 19:06:32 +0530, Amit Kapila wrote:
> 
> Today, I have verified all previous comments raised by
> me and looked at new code and below are my findings:
> 
> >>
> >> 4.
> >> LWLockAcquireCommon()
> >> {
> >> ..
> >> if (!LWLockDequeueSelf(l))
> >> {
> >>  for (;;)
> >> {
> >> PGSemaphoreLock(&proc->sem, false);
> >>  if (!proc->lwWaiting)
> >> break;
> >> extraWaits++;
> >>  }
> >> lock->releaseOK = true;
> >> ..
> >> }
> >>
> >> Setting releaseOK in above context might not be required  because if the
> >> control comes in this part of code, it will not retry to acquire another
> >> time.
> 
> > Hm. You're probably right.
> 
> You have agreed to fix this comment, but it seems you have forgot
> to change it.

After I've thought more about it, it's is actually required. This
essentially *is* a retry. Someobdy woke us up, which is where releaseOK
is supposed to be set.

> >> 11.
> >> LWLockRelease()
> >> {
> >> ..
> >> PRINT_LWDEBUG("LWLockRelease", lock, mode);
> >> }
> >>
> >> Shouldn't this be in begining of LWLockRelease function rather than
> >> after processing held_lwlocks array?
> 
> > Ok.
> 
> You have agreed to fix this comment, but it seems you have forgot
> to change it.



> Below comment doesn't seem to be adressed?
> 
> > LWLockAcquireOrWait()
> > {
> > ..
> > LOG_LWDEBUG("LWLockAcquireOrWait", lockid, mode, "succeeded");
> > ..
> > }
> 
> > a. such a log is not there in any other LWLock.. variants,
> >  if we want to introduce it, then shouldn't it be done at
> >  other places as well.

I think you're placing unneccessarily high consistency constraints on a
debugging feature here.

> Below point is yet to be resolved.
> 
> > > 12.
> > > #ifdef LWLOCK_DEBUG
> > > lock->owner = MyProc;
> > > #endif
> > >
> > > Shouldn't it be reset in LWLockRelease?
> >
> > That's actually intentional. It's quite useful to know the last owner
> > when debugging lwlock code.
> 
> Won't it cause any problem if the last owner process exits?

No. PGPROCs aren't deallocated or anything. And it's a debugging only
variable.

> Can you explain how pg_read_barrier() in below code makes this
> access safe?
> 
> LWLockWakeup()
> {
> ..
> + pg_read_barrier(); /* pairs with nwaiters-- */
> + if (!BOOL_ACCESS_ONCE(lock->releaseOK))
> ..
> }

What's the concern you have? Full memory barriers (the atomic
nwaiters--) pair with read memory barriers.

Greetings,

Andres Freund