Re: LWLock contention: I think I understand the problem - Mailing list pgsql-hackers

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> What you want to do is to wake up the sleeper but not give them the lock
> until they are actually running and can aquire it themselves.

Yeah.  Essentially this is a partial reversion to the idea of a
spinlock.  But it's more efficient than our old implementation with
timed waits between retries, because (a) a process will not be awoken
unless it has a chance at getting the lock, and (b) when a contended-for
lock is freed, a waiting process will be made ready immediately, rather
than waiting for a time tick to elapse.  So, if the lock-releasing
process does block before the end of its quantum, the released process
is available to run immediately.  Under the old scheme, a process that
had failed to get a spinlock couldn't run until its select wait timed
out, even if the lock were now available.  So I think it's still a net
win to have the LWLock mechanism in there, rather than just changing
them back to spinlocks.

> If you code up a patch, I will test it on my SMP machine using pgbench.
> Hopefully this will help Tatsuo's 4-way AIX machine too, and Linux.

Attached is a proposed patch (against the current-CVS version of
lwlock.c).  I haven't committed this yet, but it seems to be a win on
a single CPU.  Can people try it on multi CPUs?

            regards, tom lane

*** src/backend/storage/lmgr/lwlock.c.orig    Fri Dec 28 18:26:04 2001
--- src/backend/storage/lmgr/lwlock.c    Sat Dec 29 15:20:08 2001
***************
*** 195,201 ****
  LWLockAcquire(LWLockId lockid, LWLockMode mode)
  {
      volatile LWLock *lock = LWLockArray + lockid;
!     bool        mustwait;

      PRINT_LWDEBUG("LWLockAcquire", lockid, lock);

--- 195,202 ----
  LWLockAcquire(LWLockId lockid, LWLockMode mode)
  {
      volatile LWLock *lock = LWLockArray + lockid;
!     PROC       *proc = MyProc;
!     int            extraWaits = 0;

      PRINT_LWDEBUG("LWLockAcquire", lockid, lock);

***************
*** 206,248 ****
       */
      HOLD_INTERRUPTS();

!     /* Acquire mutex.  Time spent holding mutex should be short! */
!     SpinLockAcquire_NoHoldoff(&lock->mutex);
!
!     /* If I can get the lock, do so quickly. */
!     if (mode == LW_EXCLUSIVE)
      {
!         if (lock->exclusive == 0 && lock->shared == 0)
          {
!             lock->exclusive++;
!             mustwait = false;
          }
          else
-             mustwait = true;
-     }
-     else
-     {
-         /*
-          * If there is someone waiting (presumably for exclusive access),
-          * queue up behind him even though I could get the lock.  This
-          * prevents a stream of read locks from starving a writer.
-          */
-         if (lock->exclusive == 0 && lock->head == NULL)
          {
!             lock->shared++;
!             mustwait = false;
          }
-         else
-             mustwait = true;
-     }

!     if (mustwait)
!     {
!         /* Add myself to wait queue */
!         PROC       *proc = MyProc;
!         int            extraWaits = 0;

          /*
           * If we don't have a PROC structure, there's no way to wait. This
           * should never occur, since MyProc should only be null during
           * shared memory initialization.
--- 207,263 ----
       */
      HOLD_INTERRUPTS();

!     /*
!      * Loop here to try to acquire lock after each time we are signaled
!      * by LWLockRelease.
!      *
!      * NOTE: it might seem better to have LWLockRelease actually grant us
!      * the lock, rather than retrying and possibly having to go back to
!      * sleep.  But in practice that is no good because it means a process
!      * swap for every lock acquisition when two or more processes are
!      * contending for the same lock.  Since LWLocks are normally used to
!      * protect not-very-long sections of computation, a process needs to
!      * be able to acquire and release the same lock many times during a
!      * single process dispatch cycle, even in the presence of contention.
!      * The efficiency of being able to do that outweighs the inefficiency of
!      * sometimes wasting a dispatch cycle because the lock is not free when a
!      * released waiter gets to run.  See pgsql-hackers archives for 29-Dec-01.
!      */
!     for (;;)
      {
!         bool        mustwait;
!
!         /* Acquire mutex.  Time spent holding mutex should be short! */
!         SpinLockAcquire_NoHoldoff(&lock->mutex);
!
!         /* If I can get the lock, do so quickly. */
!         if (mode == LW_EXCLUSIVE)
          {
!             if (lock->exclusive == 0 && lock->shared == 0)
!             {
!                 lock->exclusive++;
!                 mustwait = false;
!             }
!             else
!                 mustwait = true;
          }
          else
          {
!             if (lock->exclusive == 0)
!             {
!                 lock->shared++;
!                 mustwait = false;
!             }
!             else
!                 mustwait = true;
          }

!         if (!mustwait)
!             break;                /* got the lock */

          /*
+          * Add myself to wait queue.
+          *
           * If we don't have a PROC structure, there's no way to wait. This
           * should never occur, since MyProc should only be null during
           * shared memory initialization.
***************
*** 267,275 ****
           *
           * Since we share the process wait semaphore with the regular lock
           * manager and ProcWaitForSignal, and we may need to acquire an
!          * LWLock while one of those is pending, it is possible that we
!          * get awakened for a reason other than being granted the LWLock.
!          * If so, loop back and wait again.  Once we've gotten the lock,
           * re-increment the sema by the number of additional signals
           * received, so that the lock manager or signal manager will see
           * the received signal when it next waits.
--- 282,290 ----
           *
           * Since we share the process wait semaphore with the regular lock
           * manager and ProcWaitForSignal, and we may need to acquire an
!          * LWLock while one of those is pending, it is possible that we get
!          * awakened for a reason other than being signaled by LWLockRelease.
!          * If so, loop back and wait again.  Once we've gotten the LWLock,
           * re-increment the sema by the number of additional signals
           * received, so that the lock manager or signal manager will see
           * the received signal when it next waits.
***************
*** 287,309 ****

          LOG_LWDEBUG("LWLockAcquire", lockid, "awakened");

!         /*
!          * The awakener already updated the lock struct's state, so we
!          * don't need to do anything more to it.  Just need to fix the
!          * semaphore count.
!          */
!         while (extraWaits-- > 0)
!             IpcSemaphoreUnlock(proc->sem.semId, proc->sem.semNum);
!     }
!     else
!     {
!         /* Got the lock without waiting */
!         SpinLockRelease_NoHoldoff(&lock->mutex);
      }

      /* Add lock to list of locks held by this backend */
      Assert(num_held_lwlocks < MAX_SIMUL_LWLOCKS);
      held_lwlocks[num_held_lwlocks++] = lockid;
  }

  /*
--- 302,322 ----

          LOG_LWDEBUG("LWLockAcquire", lockid, "awakened");

!         /* Now loop back and try to acquire lock again. */
      }

+     /* We are done updating shared state of the lock itself. */
+     SpinLockRelease_NoHoldoff(&lock->mutex);
+
      /* Add lock to list of locks held by this backend */
      Assert(num_held_lwlocks < MAX_SIMUL_LWLOCKS);
      held_lwlocks[num_held_lwlocks++] = lockid;
+
+     /*
+      * Fix the process wait semaphore's count for any absorbed wakeups.
+      */
+     while (extraWaits-- > 0)
+         IpcSemaphoreUnlock(proc->sem.semId, proc->sem.semNum);
  }

  /*
***************
*** 344,355 ****
      }
      else
      {
!         /*
!          * If there is someone waiting (presumably for exclusive access),
!          * queue up behind him even though I could get the lock.  This
!          * prevents a stream of read locks from starving a writer.
!          */
!         if (lock->exclusive == 0 && lock->head == NULL)
          {
              lock->shared++;
              mustwait = false;
--- 357,363 ----
      }
      else
      {
!         if (lock->exclusive == 0)
          {
              lock->shared++;
              mustwait = false;
***************
*** 427,446 ****
          if (lock->exclusive == 0 && lock->shared == 0)
          {
              /*
!              * Remove the to-be-awakened PROCs from the queue, and update
!              * the lock state to show them as holding the lock.
               */
              proc = head;
!             if (proc->lwExclusive)
!                 lock->exclusive++;
!             else
              {
-                 lock->shared++;
                  while (proc->lwWaitLink != NULL &&
                         !proc->lwWaitLink->lwExclusive)
                  {
                      proc = proc->lwWaitLink;
-                     lock->shared++;
                  }
              }
              /* proc is now the last PROC to be released */
--- 435,451 ----
          if (lock->exclusive == 0 && lock->shared == 0)
          {
              /*
!              * Remove the to-be-awakened PROCs from the queue.  If the
!              * front waiter wants exclusive lock, awaken him only.
!              * Otherwise awaken as many waiters as want shared access.
               */
              proc = head;
!             if (!proc->lwExclusive)
              {
                  while (proc->lwWaitLink != NULL &&
                         !proc->lwWaitLink->lwExclusive)
                  {
                      proc = proc->lwWaitLink;
                  }
              }
              /* proc is now the last PROC to be released */