Re: Fix performance degradation of contended LWLock on NUMA

> On 2017-10-19 02:28, Andres Freund wrote:
>> On 2017-06-05 16:22:58 +0300, Sokolov Yura wrote:
>>> Algorithm for LWLockWaitForVar is also refactored. New version is:
>>> 1. If lock is not held by anyone, it immediately exit.
>>> 2. Otherwise it is checked for ability to take WaitList lock, because
>>> variable is protected with it. If so, CAS is performed, and if it
>>> is
>>> successful, loop breaked to step 4.
>>> 3. Otherwise spin_delay perfomed, and loop returns to step 1.
>>> 4. var is checked for change.
>>> If it were changed, we unlock wait list lock and exit.
>>> Note: it could not change in following steps because we are holding
>>> wait list lock.
>>> 5. Otherwise CAS on setting necessary flags is performed. If it
>>> succeed,
>>> then queue Proc to wait list and exit.
>>> 6. If CAS failed, then there is possibility for LWLock to be already
>>> released - if so then we should unlock wait list and exit.
>>> 7. Otherwise loop returns to step 5.
>>>
>>> So invariant is preserved:
>>> - either we found LWLock free,
>>> - or we found changed variable,
>>> - or we set flags and queue self while LWLock were held.
>>>
>>> Spin_delay is not performed at step 7, because we should release wait
>>> list lock as soon as possible.
>>
>> That seems unconvincing - by not delaying you're more likely to
>> *increase* the time till the current locker that holds the lock can
>> release the lock.
>
> But why? If our CAS wasn't satisfied, then other's one were satisfied.
> So there is always overall progress. And if it will sleep in this
> loop, then other waiters will spin in first loop in this functions.
> But given concrete usage of LWLockWaitForVar, probably it is not too
> bad to hold other waiters in first loop in this function (they loops
> until we release WaitListLock or lock released at whole).
>
> I'm in doubts what is better. May I keep it unchanged now?

In fact, if waiter will sleep here, lock holder will not be able to set
variable we are waiting for, and therefore will not release lock.
It is stated in the comment for the loop:

+ * Note: value could not change again cause we are holding WaitList
lock.

So delaying here we certainly will degrade.

>
> BTW, I found a small mistake in this place: I forgot to set
> LW_FLAG_LOCKED in a state before this CAS. Looks like it wasn't real
> error, because CAS always failed at first loop iteration (because real
> `lock->state` had LW_FLAG_LOCKED already set), and after failed CAS
> state adsorbs value from `lock->state`.
> I'll fix it.

Attach contains version with a fix.

--
Sokolov Yura aka funny_falcon
Postgres Professional: https://postgrespro.ru
The Russian Postgres Company