Re: Atomic operations within spinlocks

Hi,

On 2020-06-04 13:57:19 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2020-06-03 14:19:45 -0400, Tom Lane wrote:
> >> This seems to me to be very bad code.
>
> > I think straight out prohibiting use of atomics inside a spinlock will
> > lead to more complicated and slower code, rather than than improving
> > anything. I'm to blame for the ClockSweepTick() case, and I don't really
> > see how we could avoid the atomic-while-spinlocked without relying on
> > 64bit atomics, which are emulated on more platforms, and without having
> > a more complicated retry loop.
>
> Well, if you don't want to touch freelist.c then there is no point
> worrying about what pg_stat_get_wal_receiver is doing.  But having
> now studied that freelist.c code, I don't like it one bit.  It's
> overly complicated and not very accurately commented, making it
> *really* hard to convince oneself that it's not buggy.
>
> I think a good case could be made for ripping out what's there now
> and just redefining nextVictimBuffer as a pg_atomic_uint64 that we
> never reset (ie, make its comment actually true).  Then ClockSweepTick
> reduces to

Note that we can't do that in the older back branches, there wasn't any
64bit atomics fallback before

commit e8fdbd58fe564a29977f4331cd26f9697d76fc40
Author: Andres Freund <andres@anarazel.de>
Date:   2017-04-07 14:44:47 -0700

    Improve 64bit atomics support.


> pg_atomic_fetch_add_u64(&StrategyControl->nextVictimBuffer, 1) % NBuffers
>
> and when we want to know how many cycles have been completed, we just
> divide the counter by NBuffers.  Now admittedly, this is relying on both
> pg_atomic_fetch_add_u64 being fast and int64 division being fast ... but
> to throw your own argument back at you, do we really care anymore about
> performance on machines where those things aren't true?  Furthermore,
> since all this is happening in a code path that's going to lead to I/O,
> I'm not exactly convinced that a few nanoseconds matter anyway.

It's very easy to observe this code being a bottleneck. If we only
performed a single clock tick before IO, sure, then the cost would
obviously be swamped by the IO cost. But it's pretty common to end up
having to do that ~ NBuffers * 5 times for a single buffer.

I don't think it's realistic to rely on 64bit integer division being
fast in this path. The latency is pretty darn significant (64bit div is
35-88 cycles on skylake-x, 64bit idiv 42-95). And unless I
misunderstand, you'd have to do so (for % NBuffers) every single clock
tick, not just when we wrap around.

We could however avoid the spinlock if we were to use 64bit atomics, by
storing the number of passes in the upper 32bit, and the next victim
buffer in the lower. But that doesn't seem that simple either, and will
regress performance on 32bit platforms.


I don't the whole strategy logic at all, so I guess there's some
argument to improve things from that end. It's probably possible to
avoid the lock with 32bit atomics as well.


I'd still like to know which problem we're actually trying to solve
here. I don't understand the "error" issues you mentioned upthread.

Greetings,

Andres Freund