On 07/29/2015 02:39 PM, Andres Freund wrote:
> On 2015-07-15 18:44:03 +0300, Heikki Linnakangas wrote:
>> Previously, LWLockAcquireWithVar set the variable associated with the lock
>> atomically with acquiring it. Before the lwlock-scalability changes, that
>> was straightforward because you held the spinlock anyway, but it's a lot
>> harder/expensive now. So I changed the way acquiring a lock with a variable
>> works. There is now a separate flag, LW_FLAG_VAR_SET, which indicates that
>> the current lock holder has updated the variable. The LWLockAcquireWithVar
>> function is gone - you now just use LWLockAcquire(), which always clears the
>> LW_FLAG_VAR_SET flag, and you can call LWLockUpdateVar() after that if you
>> want to set the variable immediately.
>>
>> This passes make check, but I haven't done any testing beyond that. Does
>> this look sane to you?
>
> The prime thing I dislike about the patch is how long it now holds the
> spinlock in WaitForVar. I don't understand why that's necessary? There's
> no need to hold a spinlock until after the
> mustwait = (pg_atomic_read_u32(&lock->state) & LW_VAL_EXCLUSIVE) != 0;
> unless I miss something?
>
> In an earlier email you say:
>> After the spinlock is released above, but before the LWLockQueueSelf() call,
>> it's possible that another backend comes in, acquires the lock, changes the
>> variable's value, and releases the lock again. In 9.4, the spinlock was not
>> released until the process was queued.
>
> But that's not a problem. The updater in that case will have queued a
> wakeup for all waiters, including WaitForVar()?
If you release the spinlock before LWLockQueueSelf(), then no. It's
possible that the backend you wanted to wait for updates the variable's
value before you've queued up. Or even releases the lock, and it gets
re-acquired by another backend, before you've queued up.
Or are you thinking of just moving the SpinLockRelease to after
LWLockQueueSelf(), i.e. to the other side of the "mustwait = ..." line?
That'd probably be ok.
- Heikki