Re: better atomics - v0.5 - Mailing list pgsql-hackers

On Fri, Jun 27, 2014 at 2:00 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-06-27 13:15:25 -0400, Robert Haas wrote:
>> On Thu, Jun 26, 2014 at 3:04 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > I don't really see usecases where it's not related data that's being
>> > touched, but: The point is that the fastpath (not using a spinlock) might
>> > touch the atomic variable, even while the slowpath has acquired the
>> > spinlock. So the slowpath can't just use non-atomic operations on the
>> > atomic variable.
>> > Imagine something like releasing a buffer pin while somebody else is
>> > doing something that requires holding the buffer header spinlock.
>>
>> If the atomic variable can be manipulated without the spinlock under
>> *any* circumstances, then how is it a good idea to ever manipulate it
>> with the spinlock?  That seems hard to reason about, and unnecessary.
>> Critical sections for spinlocks should be short and contain only the
>> instructions that need protection, and clearly the atomic op is not
>> one of those.
>
> Imagine the situation for the buffer header spinlock which is one of the
> bigger performance issues atm. We could aim to replace all usages of
> that with clever and complicated logic, but it's hard.
>
> The IMO reasonable (and prototyped) way to do it is to make the common
> paths lockless, but fall back to the spinlock for the more complicated
> situations. For the buffer header that means that pin/unpin and buffer
> lookup are lockless, but IO and changing the identity of a buffer still
> require the spinlock. My attempts to avoid the latter basically required
> a buffer header specific reimplementation of spinlocks.
>
> To make pin/unpin et al safe without acquiring the spinlock the pincount
> and the flags had to be moved into one variable so that when
> incrementing the pincount you automatically get the flagbits back. If
> it's currently valid all is good, otherwise the spinlock needs to be
> acquired. But for that to work you need to set flags while the spinlock
> is held.
>
> Possibly you can come up with ways to avoid that, but I am pretty damn
> sure that the code will be less robust by forbidding atomics inside a
> critical section, not the contrary. It's a good idea to avoid it, but
> not at all costs.

You might be right, but I'm not convinced.  Does the lwlock
scalability patch work this way, too?  I was hoping to look at that
RSN, so if there are roughly the same issues there it might shed some
light on this point also.

What I'm basically afraid of is that this will work fine in many cases
but have really ugly failure modes.  That's pretty much what happens
with spinlocks already - the overhead is insignificant at low levels
of contention but as the spinlock starts to become contended the CPUs
all fight over the cache line and performance goes to pot.  ISTM that
making the spinlock critical section significantly longer by putting
atomic ops in there could appear to win in some cases while being very
bad in others.

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