Re: Lockless StrategyGetBuffer() clock sweep

On Wed, Oct 29, 2014 at 3:09 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> But if it is, then how about
>> adding a flag that is 4 bytes wide or less alongside bgwriterLatch,
>> and just checking the flag instead of checking bgwriterLatch itself?
>
> Yea, that'd be nicer. I didn't do it because it made the patch slightly
> more invasive... The complexity really is only needed because we're not
> guaranteed that 64bit reads are atomic. Although we actually can be
> sure, because there's no platform with nonatomic intptr_t reads - so
> 64bit platforms actually *do* have atomic 64bit reads/writes.
>
> So if we just have a integer 'setBgwriterLatch' somewhere we're good. We
> don't even need to take a lock to set it. Afaics the worst that can
> happen is that several processes set the latch...

OK, that design is fine with me.

>> Actually, the same approach would work for INT_ACCESS_ONCE. So I
>> propose this approach instead: Add a new atomic flag to
>> StrategyControl, useSlowPath. If StrategyGetBuffer() finds
>> useSlowPath set, call StrategyGetBufferSlow(), which will acquire the
>> spinlock, check whether bgwriterLatch is set and/or the freelist is
>> non-empty and return a buffer ID if able to allocate from the
>> freelist; otherwise -1. If StrategyGetBufferSlow() returns -1, or we
>> decide not to call it in the first place because useSlowPath isn't
>> set, then fall through to your clock-sweep logic. We can set
>> useSlowPath at stratup and whenever we put buffers on the free list.
>
> I don't like the idea to to conflate the slowpath for the bgwriter latch
> with the freelist slowpath. And I don't really see what that'd buy us
> over what's in the patch right now?

Well, I was thinking that with a single flag check we could skip two
paths that, as of today, are both uncommonly taken. Moving all that
logic off into a separate function might help the compiler optimize
for the fast case, too.

> I wonder if we could make a trylock for spinlocks work - then we could
> look at the freelist if the lock is free and just go to the clock cycle
> otherwise. My guess is that that'd be quite performant. IIRC it's just
> the spinlock semaphore fallback that doesn't know how to do trylock...

I could go for that. I don't think the semaphore thing is really a
problem. If the spinlock semaphore implementation indeed can't
support that, then just have it fall back to waiting for the lock and
always returning true. Semaphores are so slow that it's not going to
make any difference to performance.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company