Re: Wait free LW_SHARED acquisition - v0.2 - Mailing list pgsql-hackers

On 2014-06-23 19:59:10 +0530, Amit Kapila wrote:
> On Tue, Jun 17, 2014 at 8:56 PM, Andres Freund <andres@2ndquadrant.com>
> wrote:
> > On 2014-06-17 20:47:51 +0530, Amit Kapila wrote:
> > > On Tue, Jun 17, 2014 at 6:35 PM, Andres Freund <andres@2ndquadrant.com>
> > > wrote:
> > >
> > > You have followed it pretty well as far as I can understand from your
> > > replies, as there is no reproducible test (which I think is bit tricky
> to
> > > prepare), so it becomes difficult to explain by theory.
> >
> > I'm working an updated patch that moves the releaseOK into the
> > spinlocks. Maybe that's the problem already - it's certainly not correct
> > as is.
> 
> Sure, I will do the test/performance test with updated patch; you
> might want to include some more changes based on comments
> in mail below:

I'm nearly finished in cleaning up the atomics part of the patch which
also includes a bit of cleanup of the lwlocks code.

> Few more comments:
> 
> 1.
> LWLockAcquireCommon()
> {
> ..
> iterations++;
> }
> 
> In current logic, I could not see any use of these *iterations* variable.

It's useful for debugging. Should be gone in the final code.

> 2.
> LWLockAcquireCommon()
> {
> ..
> if (!LWLockDequeueSelf(l))
> {
> /*
> * Somebody else dequeued us and has or will..
>  ..
> */
> for (;;)
> {
>  PGSemaphoreLock(&proc->sem, false);
> if (!proc->lwWaiting)
> break;
>  extraWaits++;
> }
> lock->releaseOK = true;
> }
> 
> Do we want to set result = false; after waking in above code?
> The idea behind setting false is to indicate whether we get the lock
> immediately or not which previously was decided based on if it needs
> to queue itself?

Hm. I don't think it's clear which version is better.

> 3.
> LWLockAcquireCommon()
> {
> ..
> /*
>  * Ok, at this point we couldn't grab the lock on the first try. We
>  * cannot simply queue ourselves to the end of the list and wait to be
>  * woken up because by now the lock could long have been released.
>  * Instead add us to the queue and try to grab the lock again. If we
>  * suceed we need to revert the queuing and be happy, otherwise we
>  * recheck the lock. If we still couldn't grab it, we know that the
>  * other lock will see our queue entries when releasing since they
>  * existed before we checked for the lock.
>  */
> /* add to the queue */
> LWLockQueueSelf(l, mode);
> 
> /* we're now guaranteed to be woken up if necessary */
> mustwait = LWLockAttemptLock(l, mode, false, &potentially_spurious);
> ..
> }
> 
> a. By reading above code and comments, it is not quite clear why
> second attempt is important unless somebody thinks on it or refer
> your comments in *Notes* section at top of file.  I think it's better to
> indicate in some way so that code reader can refer to Notes section or
> whereever you are planing to keep those comments.

Ok.

> b. There is typo in above comment suceed/succeed.

Thanks, fixed.

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

> 5.
> LWLockWaitForVar()
> {
> ..
> else
> mustwait = false;
> 
> if (!mustwait)
> break;
> ..
> }
> 
> I think we can directly break in else part in above code.

Well, there's another case of mustwait=false above which is triggered
while the spinlock is held. Don't think it'd get simpler.

> 6.
> LWLockWaitForVar()
> {
> ..
> /*
>  * Quick test first to see if it the slot is free right now.
>  *
>  * XXX: the caller uses a spinlock before this,...
>  */
> if (pg_atomic_read_u32(&lock->lockcount) == 0)
>  return true;
> }
> 
> Does the part of comment that refers to spinlock is still relevant
> after using atomic ops?

Yes. pg_atomic_read_u32() isn't a memory barrier (and explicitly
documented not to be).

> 7.
> LWLockWaitForVar()
> {
> ..
> /*
>  * Add myself to wait queue. Note that this is racy, somebody else
>  * could wakeup before we're finished queuing.
>  * NB: We're using nearly the same twice-in-a-row lock acquisition
>  * protocol as LWLockAcquire(). Check its comments for details.
>  */
> LWLockQueueSelf(l, LW_WAIT_UNTIL_FREE);
> 
> /* we're now guaranteed to be woken up if necessary */
>  mustwait = LWLockAttemptLock(l, LW_EXCLUSIVE, false,
> &potentially_spurious);
> }
> 
> Why is it important to Attempt lock after queuing in this case, can't
> we just re-check exclusive lock as done before queuing?

Well, that's how Heikki designed LWLockWaitForVar().

> 8.
> LWLockWaitForVar()
> {
> ..
> PRINT_LWDEBUG("LWLockAcquire undo queue", lock, mode);
>  break;
> }
> else
> {
>  PRINT_LWDEBUG("LWLockAcquire waiting 4", lock, mode);
> }
> ..
> }
> 
> a. I think instead of LWLockAcquire, here we should use
>    LWLockWaitForVar

right.

> b. Isn't it better to use LOG_LWDEBUG instead of PRINT_LWDEBUG(),
>     as PRINT_LWDEBUG() is generally used in file at entry of functions to
>     log info about locks?

Fine with me.

> 9.
> LWLockUpdateVar()
> {
> /* Acquire mutex.  Time spent holding mutex should be short! */
>  SpinLockAcquire(&lock->mutex);
> ..
> }
> 
> Current code has an Assert for exclusive lock which is missing in
> patch, is there any reason for removing Assert?

That assert didn't use to be there on master... I'll add it again.

> 10.
> LWLockRelease(LWLock *l)
> {
> ..
> if (l == held_lwlocks[i].lock)
> {
> mode = held_lwlocks[i].mode;
> ..
> }
> 
> It seems that mode is added to held_lwlocks to use it in LWLockRelease().
> If yes, then can we deduce the same from lockcount?

No. It can be temporarily too high (the whole backout stuff). It's also
much cheaper to test a process local variable.

> 11.
> LWLockRelease()
> {
> ..
> PRINT_LWDEBUG("LWLockRelease", lock, mode);
> }
> 
> Shouldn't this be in begining of LWLockRelease function rather than
> after processing held_lwlocks array?

Ok.

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

> * 2) If somebody spuriously got blocked from acquiring the lock, they will
>  *    get queued in Phase 2 and we can wake them up if neccessary or they
> will
>  *    have gotten the lock in Phase 2.
> 
> In the above line, I think the second mention of *Phase 2* should be *Phase
> 3*.

Right, good catch.

> > I think both are actually critical for performance... Otherwise even a
> > only lightly contended lock would require scheduler activity when a
> > processes tries to lock something twice. Given the frequency we acquire
> > some locks with that'd be disastrous...
> 
> Do you have any suggestion how both behaviours can be retained?

Not sure what you mean. They currently *are* retained? Or do you mean
whether they could be retained while making lwlocks fai?

> Note - Still there is more to review in this patch, however I feel it is
> good idea to start some test/performance test of this patch, if you
> agree, then I will start the same with the updated patch (result
> of conclusion of current review comments).

I'll post a new version of this + the atomics patch tomorrow. Since the
whole atomics stuff has changed noticeably it probably makes sense to
wait till then.

Thanks for the look!

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services