On Mon, Aug 5, 2013 at 11:40 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2013-08-05 10:49:08 -0500, Merlin Moncure wrote:
>> optimization 4: remove free list lock (via Jeff Janes). This is the
>> other optimization: one backend will no longer be able to shut down
>> buffer allocation
>
> I think splitting off the actual freelist checking into a spinlock makes
> quite a bit of sense. And I think a separate one should be used for the
> actual search for the clock sweep.
> I don't think the unlocked increment of nextVictimBuffer is a good idea
> though. nextVictimBuffer jumping over NBuffers under concurrency seems
> like a recipe for disaster to me. At the very, very least it will need a
> good wad of comments explaining what it means and how you're allowed to
> use it. The current way will lead to at least bgwriter accessing a
> nonexistant/out of bounds buffer via StrategySyncStart().
> Possibly it won't even save that much, it might just increase the
> contention on the buffer header spinlock's cacheline.
I agree; at least then it's not unambiguously better. if you (in
effect) swap all contention on allocation from a lwlock to a spinlock
it's not clear if you're improving things; it would have to be proven
and I'm trying to keep things simple.
Attached is a scaled down version of the patch that keeps the freelist
lock but still removes the spinlock during the clock sweep. This
still hits the major objectives of reducing the chance of scheduling
out while holding the BufFreelistLock and mitigating the worst case
impact of doing so if it does happen. An even more scaled down
version would keep the current logic exactly as is except for
replacing buffer lock in the clock sweep with a trylock (which is
IMNSHO a no-brainer).
merlin