Re: Move PinBuffer and UnpinBuffer to atomics

Hi,

> +/*
> + * The following two macros are aimed to simplify buffer state modification
> + * in CAS loop. It's assumed that variable "uint32 state" is defined outside
> + * of this loop. It should be used as following:
> + *
> + * BEGIN_BUFSTATE_CAS_LOOP(bufHdr);
> + * modifications of state variable;
> + * END_BUFSTATE_CAS_LOOP(bufHdr);
> + *
> + * For local buffers, these macros shouldn't be used.. Since there is
> + * no cuncurrency, local buffer state could be chaged directly by atomic
> + * read/write operations.
> + */
> +#define BEGIN_BUFSTATE_CAS_LOOP(bufHdr) \
> + do { \
> + SpinDelayStatus delayStatus = init_spin_delay((Pointer)(bufHdr)); \

> + uint32 oldstate; \
> + oldstate = pg_atomic_read_u32(&(bufHdr)->state); \
> + for (;;) { \
> + while (oldstate & BM_LOCKED) \
> + { \
> + make_spin_delay(&delayStatus); \
> + oldstate = pg_atomic_read_u32(&(bufHdr)->state); \
> + } \
> + state = oldstate
> +
> +#define END_BUFSTATE_CAS_LOOP(bufHdr) \
> + if (pg_atomic_compare_exchange_u32(&bufHdr->state, &oldstate, state)) \
> + break; \
> + } \
> + finish_spin_delay(&delayStatus); \
> + } while (0)

Hm. Not sure if that's not too much magic. Will think about it.

> /*
> + * Lock buffer header - set BM_LOCKED in buffer state.
> + */
> + uint32
> + LockBufHdr(BufferDesc *desc)
> + {
> + uint32 state;
> +
> + BEGIN_BUFSTATE_CAS_LOOP(desc);
> + state |= BM_LOCKED;
> + END_BUFSTATE_CAS_LOOP(desc);
> +
> + return state;
> + }
> +

Hm. It seems a bit over the top to do the full round here. How about

uint32 oldstate;
while (true)
{
oldstate = pg_atomic_fetch_or(..., BM_LOCKED);
if (!(oldstate & BM_LOCKED)
break;
perform_spin_delay();
}
return oldstate | BM_LOCKED;

Especially if we implement atomic xor on x86, that'd likely be a good
bit more efficient.

> typedef struct BufferDesc
> {
> BufferTag tag; /* ID of page contained in buffer */
> - BufFlags flags; /* see bit definitions above */
> - uint8 usage_count; /* usage counter for clock sweep code */
> - slock_t buf_hdr_lock; /* protects a subset of fields, see above */
> - unsigned refcount; /* # of backends holding pins on buffer */
> - int wait_backend_pid; /* backend PID of pin-count waiter */
>
> + /* state of the tag, containing flags, refcount and usagecount */
> + pg_atomic_uint32 state;
> +
> + int wait_backend_pid; /* backend PID of pin-count waiter */
> int buf_id; /* buffer's index number (from 0) */
> int freeNext; /* link in freelist chain */

Hm. Won't matter on most platforms, but it seems like a good idea to
move to an 8 byte aligned boundary. Move buf_id up?

> +/*
> + * Support for spin delay which could be useful in other places where
> + * spinlock-like procedures take place.
> + */
> +typedef struct
> +{
> + int spins;
> + int delays;
> + int cur_delay;
> + Pointer ptr;
> + const char *file;
> + int line;
> +} SpinDelayStatus;
> +
> +#define init_spin_delay(ptr) {0, 0, 0, (ptr), __FILE__, __LINE__}
> +
> +void make_spin_delay(SpinDelayStatus *status);
> +void finish_spin_delay(SpinDelayStatus *status);
> +
> #endif /* S_LOCK_H */

s/make_spin_delay/perform_spin_delay/? The former sounds like it's
allocating something or such.

Regards,

Andres