Re: Wait free LW_SHARED acquisition - v0.2

On Mon, Jun 23, 2014 at 9:12 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:
> On 2014-06-23 19:59:10 +0530, Amit Kapila wrote:
> > On Tue, Jun 17, 2014 at 8:56 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
> > wrote:
> > 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.

I thought if we get the lock at first attempt, then result should be
true which seems to be clear, but for the case of second attempt you
are right that it's not clear. In such a case, I think we can go either
way and then later during tests or otherwise if any problem is discovered,
we can revert it.

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

In that case I might be missing some point here, un-patched code of
LWLockWaitForVar() never tries to acquire the lock, but the new code
does so. Basically I am not able to think what is the problem if we just
do below after queuing:
mustwait = pg_atomic_read_u32(&lock->lockcount) != 0;

Could you please explain what is the problem in just rechecking?

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

I just wanted to say that current behaviour of releaseOK seems to
be of use for some cases and if you want to change it, then would it
retain the current behaviour we get by releaseOK?

I understand that till now your patch has not changed anything specific
to releaseOK, but by above discussion I got the impression that you are
planing to change it, that's why I had asked above question.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com