On 02/10/2014 03:46 PM, Andres Freund wrote:
Hi,During the lwlock scalability work I noticed a longstanding issue with the lwlock code. LWLockRelease() and the other mentioned locations do the following to wake up any waiters, without holding the lock's spinlock: /* * Awaken any waiters I removed from the queue. */ while (head != NULL) { LOG_LWDEBUG("LWLockRelease", T_NAME(l), T_ID(l), "release waiter"); proc = head; head = proc->lwWaitLink; proc->lwWaitLink = NULL; proc->lwWaiting = false; PGSemaphoreUnlock(&proc->sem); } which means they manipulate the lwWaitLink queue without protection. That's done intentionally. The code tries to protect against corruption of the list to do a woken up backend acquiring a lock (this or an independent one) by only continuing when the lwWaiting flag is set to false. Unfortunately there's absolutely no guarantee that a) the assignment to lwWaitLink and lwWaiting are done in that order b) that the stores are done in-order from the POV of other backends. So what we need to do is to acquire a write barrier between the assignments to lwWaitLink and lwWaiting, i.e. proc->lwWaitLink = NULL; pg_write_barrier(); proc->lwWaiting = false; the reader side already uses an implicit barrier by using spinlocks. I've fixed this as part 1 of the lwlock scalability work in [1], but Heikki rightfully suggested that a) this should be backpatched b) done in a separate commit. There is the question what to do about the branches without barriers? I guess a SpinLockAcquire()/Release() would do?
The other alternative we discussed on IM is to unlink the waiters from the linked list, while still holding the spinlock. We can't do the PGSemaphoreUnlock() call to actually wake up the waiters while holding the spinlock, but all the shared memory manipulations we could. It would move all the modifications of the shared structures under the spinlock, which feels comforting.
It would require using some-sort of a backend-private data structure to hold the list of processes to wake up. We don't want to palloc() in LWLockRelease(), but we could malloc() a large-enough array once at process initialization, and use that on all LWLockRelease() calls.
- Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
