Re: Move PinBuffer and UnpinBuffer to atomics

On 2016-03-28 15:46:43 +0300, Alexander Korotkov wrote:
> diff --git a/src/backend/storage/buffer/bufmnew file mode 100644
> index 6dd7c6e..fe6fb9c
> --- a/src/backend/storage/buffer/bufmgr.c
> +++ b/src/backend/storage/buffer/bufmgr.c
> @@ -52,7 +52,6 @@
>  #include "utils/resowner_private.h"
>  #include "utils/timestamp.h"
>  
> -
>  /* Note: these two macros only work on shared buffers, not local ones! */
>  #define BufHdrGetBlock(bufHdr)    ((Block) (BufferBlocks + ((Size) (bufHdr)->buf_id) * BLCKSZ))
>  #define BufferGetLSN(bufHdr)    (PageGetLSN(BufHdrGetBlock(bufHdr)))

spurious

> @@ -848,7 +852,7 @@ ReadBuffer_common(SMgrRelation smgr, cha
>       * it's not been recycled) but come right back here to try smgrextend
>       * again.
>       */
> -    Assert(!(bufHdr->flags & BM_VALID));        /* spinlock not needed */
> +    Assert(!(pg_atomic_read_u32(&bufHdr->state) & BM_VALID)); /* header lock not needed */
>  
>      bufBlock = isLocalBuf ? LocalBufHdrGetBlock(bufHdr) : BufHdrGetBlock(bufHdr);
>  
> @@ -932,8 +936,13 @@ ReadBuffer_common(SMgrRelation smgr, cha
>  
>      if (isLocalBuf)
>      {
> -        /* Only need to adjust flags */
> -        bufHdr->flags |= BM_VALID;
> +        /*
> +         * Only need to adjust flags.  Since it's local buffer, there is no
> +         * concurrency.  We assume read/write pair to be cheaper than atomic OR.
> +         */
> +        uint32 state = pg_atomic_read_u32(&bufHdr->state);
> +        state |= BM_VALID;
> +        pg_atomic_write_u32(&bufHdr->state, state);

I'm not a fan of repeating that comment in multiple places. Hm.


>      }
>      else
>      {
> @@ -987,10 +996,11 @@ BufferAlloc(SMgrRelation smgr, char relp
>      BufferTag    oldTag;            /* previous identity of selected buffer */
>      uint32        oldHash;        /* hash value for oldTag */
>      LWLock       *oldPartitionLock;        /* buffer partition lock for it */
> -    BufFlags    oldFlags;
> +    uint32        oldFlags;
>      int            buf_id;
>      BufferDesc *buf;
>      bool        valid;
> +    uint32        state;
>  
>      /* create a tag so we can lookup the buffer */
>      INIT_BUFFERTAG(newTag, smgr->smgr_rnode.node, forkNum, blockNum);
> @@ -1050,23 +1060,23 @@ BufferAlloc(SMgrRelation smgr, char relp
>      for (;;)
>      {
>          /*
> -         * Ensure, while the spinlock's not yet held, that there's a free
> +         * Ensure, while the header lock isn't yet held, that there's a free
>           * refcount entry.
>           */
>          ReservePrivateRefCountEntry();
>  
>          /*
>           * Select a victim buffer.  The buffer is returned with its header
> -         * spinlock still held!
> +         * lock still held!
>           */
> -        buf = StrategyGetBuffer(strategy);
> +        buf = StrategyGetBuffer(strategy, &state);

The new thing really still is a spinlock, not sure if it's worth
changing the comments that way.


> @@ -1319,7 +1330,7 @@ BufferAlloc(SMgrRelation smgr, char relp
>   * InvalidateBuffer -- mark a shared buffer invalid and return it to the
>   * freelist.
>   *
> - * The buffer header spinlock must be held at entry.  We drop it before
> + * The buffer header lock must be held at entry.  We drop it before
>   * returning.  (This is sane because the caller must have locked the
>   * buffer in order to be sure it should be dropped.)
>   *

Ok, by now I'm pretty sure that I don't want this to be changed
everywhere, just makes reviewing harder.



> @@ -1433,6 +1443,7 @@ void
>  MarkBufferDirty(Buffer buffer)
>  {
>      BufferDesc *bufHdr;
> +    uint32        state;
>  
>      if (!BufferIsValid(buffer))
>          elog(ERROR, "bad buffer ID: %d", buffer);
> @@ -1449,14 +1460,14 @@ MarkBufferDirty(Buffer buffer)
>      /* unfortunately we can't check if the lock is held exclusively */
>      Assert(LWLockHeldByMe(BufferDescriptorGetContentLock(bufHdr)));
>  
> -    LockBufHdr(bufHdr);
> +    state = LockBufHdr(bufHdr);
>  
> -    Assert(bufHdr->refcount > 0);
> +    Assert(BUF_STATE_GET_REFCOUNT(state) > 0);
>  
>      /*
>       * If the buffer was not dirty already, do vacuum accounting.
>       */
> -    if (!(bufHdr->flags & BM_DIRTY))
> +    if (!(state & BM_DIRTY))
>      {
>          VacuumPageDirty++;
>          pgBufferUsage.shared_blks_dirtied++;
> @@ -1464,9 +1475,9 @@ MarkBufferDirty(Buffer buffer)
>              VacuumCostBalance += VacuumCostPageDirty;
>      }
>  
> -    bufHdr->flags |= (BM_DIRTY | BM_JUST_DIRTIED);
> -
> -    UnlockBufHdr(bufHdr);
> +    state |= BM_DIRTY | BM_JUST_DIRTIED;
> +    state &= ~BM_LOCKED;
> +    pg_atomic_write_u32(&bufHdr->state, state);
>  }

Hm, this is a routine I think we should avoid taking the lock in; it's
often called quite frequently. Also doesn't look very hard.

>  static bool
>  PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy)
> @@ -1547,23 +1563,44 @@ PinBuffer(BufferDesc *buf, BufferAccessS
>  
>      if (ref == NULL)
>      {
> +        /* loop of CAS operations */
> +        uint32            state;
> +        uint32            oldstate;
> +        SpinDelayStatus    delayStatus;



>          ReservePrivateRefCountEntry();
>          ref = NewPrivateRefCountEntry(b);
>  
> -        LockBufHdr(buf);
> -        buf->refcount++;
> -        if (strategy == NULL)
> -        {
> -            if (buf->usage_count < BM_MAX_USAGE_COUNT)
> -                buf->usage_count++;
> -        }
> -        else
> +        state = pg_atomic_read_u32(&buf->state);
> +        oldstate = state;
> +
> +        init_spin_delay(&delayStatus, (Pointer)buf, __FILE__, __LINE__);

Hm. This way we're calling this on every iteration. That doesn't seem
like a good idea. How about making delayStatus a static, and
init_spin_delay a macro which returns a {struct, member, one, two} type
literal?


> +        while (true)
>          {
> -            if (buf->usage_count == 0)
> -                buf->usage_count = 1;
> +            /* spin-wait till lock is free */
> +            while (state & BM_LOCKED)
> +            {
> +                make_spin_delay(&delayStatus);
> +                state = pg_atomic_read_u32(&buf->state);
> +                oldstate = state;
> +            }

Maybe we should abstract that to pg_atomic_wait_bit_unset_u32()? It
seems quite likely we need this in other places (e.g. lwlock.c itself).


> +            /* increase refcount */
> +            state += BUF_REFCOUNT_ONE;
> +
> +            /* increase usagecount unless already max */
> +            if (BUF_STATE_GET_USAGECOUNT(state) != BM_MAX_USAGE_COUNT)
> +                state += BUF_USAGECOUNT_ONE;
> +
> +            /* try to do CAS, exit on success */

Seems like a somewhat obvious comment?


> @@ -1603,15 +1640,22 @@ PinBuffer_Locked(BufferDesc *buf)
>  {

>      Assert(GetPrivateRefCountEntry(BufferDescriptorGetBuffer(buf), false) == NULL);
>  
> -    buf->refcount++;
> -    UnlockBufHdr(buf);
> +    /*
> +     * Since we assume to held buffer header lock, we can update the buffer
> +     * state in a single write operation.
> +     */
> +    state = pg_atomic_read_u32(&buf->state);
> +    state += 1;
> +    state &= ~BM_LOCKED;
> +    pg_atomic_write_u32(&buf->state, state);

Te comment should probably mention that we're releasing the
spinlock. And the += 1 should be a BUF_REFCOUNT_ONE, otherwise it's hard
to understand.

> @@ -1646,30 +1690,66 @@ UnpinBuffer(BufferDesc *buf, bool fixOwn
>      ref->refcount--;
>      if (ref->refcount == 0)
>      {

> +
> +        /* Support LockBufferForCleanup() */
> +        if (state & BM_PIN_COUNT_WAITER)
> +        {
> +            state = LockBufHdr(buf);
> +
> +            if (state & BM_PIN_COUNT_WAITER && BUF_STATE_GET_REFCOUNT(state) == 1)
> +            {
> +                /* we just released the last pin other than the waiter's */
> +                int            wait_backend_pid = buf->wait_backend_pid;
>  
> +                state &= ~(BM_PIN_COUNT_WAITER | BM_LOCKED);
> +                pg_atomic_write_u32(&buf->state, state);
> +                ProcSendSignal(wait_backend_pid);
> +            }
> +            else
> +                UnlockBufHdr(buf);
> +        }

I think it's quite confusing to use UnlockBufHdr and direct bit
expressions in one branch.

Thinking about it I also don't think the pg_atomic_write_u32 variant is
correct without adding a write barrier; the other side might not see the
values yet.

I think we can just redefine UnlockBufHdr() to be a
pg_atomic_write_u32() and pg_write_memory_barrier()?



>       * BM_PERMANENT can't be changed while we hold a pin on the buffer, so we
> -     * need not bother with the buffer header spinlock.  Even if someone else
> +     * need not bother with the buffer header lock.  Even if someone else
>       * changes the buffer header flags while we're doing this, we assume that
>       * changing an aligned 2-byte BufFlags value is atomic, so we'll read the
>       * old value or the new value, but not random garbage.
>       */

The rest of the comment is outdated, BufFlags isn't a 2 byte value
anymore.

> @@ -3078,7 +3168,7 @@ FlushRelationBuffers(Relation rel)
>                            localpage,
>                            false);
>  
> -                bufHdr->flags &= ~(BM_DIRTY | BM_JUST_DIRTIED);
> +                pg_atomic_fetch_and_u32(&bufHdr->state, ~(BM_DIRTY | BM_JUST_DIRTIED));

Hm, in other places you replaced atomics on local buffers with plain
writes.

>              lsn = XLogSaveBufferForHint(buffer, buffer_std);
>          }
>  
> -        LockBufHdr(bufHdr);
> -        Assert(bufHdr->refcount > 0);
> -        if (!(bufHdr->flags & BM_DIRTY))
> +        state = LockBufHdr(bufHdr);
> +
> +        Assert(BUF_STATE_GET_REFCOUNT(state) > 0);
> +
> +        if (!(state & BM_DIRTY))
>          {
>              dirtied = true;        /* Means "will be dirtied by this action" */

It's again worthwhile to try make this work without taking the lock.


> -    buf->flags |= BM_IO_IN_PROGRESS;
> -
> -    UnlockBufHdr(buf);
> +    state |= BM_IO_IN_PROGRESS;
> +    state &= ~BM_LOCKED;
> +    pg_atomic_write_u32(&buf->state, state);

How about making UnlockBufHdr() take a new state parameter, and
internally unset BM_LOCKED?


>  /*
> + * Lock buffer header - set BM_LOCKED in buffer state.
> + */
> +uint32
> +LockBufHdr(volatile BufferDesc *desc)
> +{
> +    SpinDelayStatus    delayStatus;
> +    uint32            state;
> +
> +    init_spin_delay(&delayStatus, (Pointer)desc, __FILE__, __LINE__);
> +
> +    state = pg_atomic_read_u32(&desc->state);
> +
> +    for (;;)
> +    {
> +        /* wait till lock is free */
> +        while (state & BM_LOCKED)
> +        {
> +            make_spin_delay(&delayStatus);
> +            state = pg_atomic_read_u32(&desc->state);
> +            /* Add exponential backoff? Should seldomly be contended tho. */

Outdated comment.


> +/*
> + * Unlock buffer header - unset BM_LOCKED in buffer state.
> + */
> +void
> +UnlockBufHdr(volatile BufferDesc *desc)
> +{
> +    Assert(pg_atomic_read_u32(&desc->state) & BM_LOCKED);
> +
> +    pg_atomic_sub_fetch_u32(&desc->state, BM_LOCKED);
> +}

As suggested above, there's likely no need to use an actual atomic op
here.

Greetings,

Andres Freund