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

On Sat, Jun 28, 2014 at 4:25 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > 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.
>
> Why? Anything I can do to convince you of this? Note that other users of
> atomics (e.g. the linux kernel) widely use atomics inside spinlock
> protected regions.

The Linux kernel isn't a great example, because they can ensure that
they aren't preempted while holding the spinlock.  We can't, and
that's a principle part of my concern here.

More broadly, it doesn't seem consistent.  I think that other projects
also sometimes write code that acquires a spinlock while holding
another spinlock, and we don't do that; in fact, we've elevated that
to a principle, regardless of whether it performs well in practice.
In some cases, the CPU instruction that we issue to acquire that
spinlock might be the exact same instruction we'd issue to manipulate
an atomic variable.  I don't see how it can be right to say that a
spinlock-protected critical section is allowed to manipulate an atomic
variable with cmpxchg, but not allowed to acquire another spinlock
with cmpxchg.  Either acquiring the second spinlock is OK, in which
case our current coding rule is wrong, or acquiring the atomic
variable is not OK, either.  I don't see how we can have that both
ways.

>> 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.
>
> Well, I'm not saying it's something I suggest doing all the time. But if
> using an atomic op in the slow path allows you to remove the spinlock
> from 99% of the cases I don't see it having a significant impact.
> In most scenarios (where atomics aren't emulated, i.e. platforms we
> expect to used in heavily concurrent cases) the spinlock and the atomic
> will be on the same cacheline making stalls much less likely.

And this again is my point: why can't we make the same argument about
two spinlocks situated on the same cache line?  I don't have a bit of
trouble believing that doing the same thing with a couple of spinlocks
could sometimes work out well, too, but Tom is adamantly opposed to
that.  I don't want to just make an end run around that issue without
understanding whether he has a valid point.

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