pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedAtomic operations within spinlocks
22+ messages / 5 participants
[nested] [flat]
Commits from this thread:
2020-06-18 cf1234a10e 3e69bf3b14 8bc13287ed c2a84bee12 825d89dda5 6cc2866c4c 5fffa8fce3 Fix deadlock danger when atomic ops are done under spinlock.
2020-06-18 e8302f107a 7e91f90a8e 3b37a6de02 59225dcefe 008c119928 e027219f21 3f66e1c5a5 Add basic spinlock tests to regression tests.
2020-06-17 4d4ca24efe 276bdc9392 b91cfaa34f 34d29222a0 b295f666b7 6606220534 dcf3cb60cb spinlock emulation: Fix bug when more than INT_MAX spinlocks are initialized.
* Atomic operations within spinlocks
@ 2020-06-03 18:19 Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 22+ messages in thread
From: Tom Lane @ 2020-06-03 18:19 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
In connection with the nearby thread about spinlock coding rule
violations, I noticed that we have several places that are doing
things like this:
SpinLockAcquire(&WalRcv->mutex);
...
written_lsn = pg_atomic_read_u64(&WalRcv->writtenUpto);
...
SpinLockRelease(&WalRcv->mutex);
That's from pg_stat_get_wal_receiver(); there is similar code in
freelist.c's ClockSweepTick() and StrategySyncStart().
This seems to me to be very bad code. In the first place, on machines
without the appropriate type of atomic operation, atomics.c is going
to be using a spinlock to emulate atomicity, which means this code
tries to take a spinlock while holding another one. That's not okay,
either from the standpoint of performance or error-safety. In the
second place, this coding seems to me to indicate serious confusion
about which lock is protecting what. In the above example, if
writtenUpto is only accessed through atomic operations then it seems
like we could just move the pg_atomic_read_u64 out of the spinlock
section; or if the spinlock is adequate protection then we could just
do a normal fetch. If we actually need both locks then this needs
significant re-thinking, IMO.
Comments?
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-06-03 20:45 ` Andres Freund <andres@anarazel.de>
2020-06-03 21:40 ` Re: Atomic operations within spinlocks Thomas Munro <thomas.munro@gmail.com>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 22+ messages in thread
From: Andres Freund @ 2020-06-03 20:45 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org
Hi,
On 2020-06-03 14:19:45 -0400, Tom Lane wrote:
> In connection with the nearby thread about spinlock coding rule
> violations, I noticed that we have several places that are doing
> things like this:
>
> SpinLockAcquire(&WalRcv->mutex);
> ...
> written_lsn = pg_atomic_read_u64(&WalRcv->writtenUpto);
> ...
> SpinLockRelease(&WalRcv->mutex);
>
> That's from pg_stat_get_wal_receiver(); there is similar code in
> freelist.c's ClockSweepTick() and StrategySyncStart().
>
> This seems to me to be very bad code. In the first place, on machines
> without the appropriate type of atomic operation, atomics.c is going
> to be using a spinlock to emulate atomicity, which means this code
> tries to take a spinlock while holding another one. That's not okay,
> either from the standpoint of performance or error-safety.
I'm honestly not particularly concerned about either of those in
general:
- WRT performance: Which platforms where we care about performance can't
do a tear-free read of a 64bit integer, and thus needs a spinlock for
this? And while the cases in freelist.c aren't just reads, they are
really cold paths (clock wrapping around).
- WRT error safety: What could happen here? The atomics codepaths is
no-fail code? And nothing should ever nest inside the atomic-emulation
spinlocks. What am I missing?
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.
> In the second place, this coding seems to me to indicate serious
> confusion about which lock is protecting what. In the above example,
> if writtenUpto is only accessed through atomic operations then it
> seems like we could just move the pg_atomic_read_u64 out of the
> spinlock section; or if the spinlock is adequate protection then we
> could just do a normal fetch. If we actually need both locks then
> this needs significant re-thinking, IMO.
Yea, it doesn't seem necessary at all that writtenUpto is read with the
spinlock held. That's very recent:
commit 2c8dd05d6cbc86b7ad21cfd7010e041bb4c3950b
Author: Michael Paquier <michael@paquier.xyz>
Date: 2020-05-17 09:22:07 +0900
Make pg_stat_wal_receiver consistent with the WAL receiver's shmem info
and I assume just was caused by mechanical replacement, rather than
intentionally doing so while holding the spinlock. As far as I can tell
none of the other writtenUpto accesses are under the spinlock.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
@ 2020-06-03 21:40 ` Thomas Munro <thomas.munro@gmail.com>
2020-06-04 07:03 ` Re: Atomic operations within spinlocks Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 22+ messages in thread
From: Thomas Munro @ 2020-06-03 21:40 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Jun 4, 2020 at 8:45 AM Andres Freund <andres@anarazel.de> wrote:
> On 2020-06-03 14:19:45 -0400, Tom Lane wrote:
> > In the second place, this coding seems to me to indicate serious
> > confusion about which lock is protecting what. In the above example,
> > if writtenUpto is only accessed through atomic operations then it
> > seems like we could just move the pg_atomic_read_u64 out of the
> > spinlock section; or if the spinlock is adequate protection then we
> > could just do a normal fetch. If we actually need both locks then
> > this needs significant re-thinking, IMO.
>
> Yea, it doesn't seem necessary at all that writtenUpto is read with the
> spinlock held. That's very recent:
>
> commit 2c8dd05d6cbc86b7ad21cfd7010e041bb4c3950b
> Author: Michael Paquier <michael@paquier.xyz>
> Date: 2020-05-17 09:22:07 +0900
>
> Make pg_stat_wal_receiver consistent with the WAL receiver's shmem info
>
> and I assume just was caused by mechanical replacement, rather than
> intentionally doing so while holding the spinlock. As far as I can tell
> none of the other writtenUpto accesses are under the spinlock.
Yeah. It'd be fine to move that after the spinlock release. Although
it's really just for informational purposes only, not for any data
integrity purpose, reading it before the spinlock acquisition would
theoretically allow it to appear to be (reportedly) behind
flushedUpto, which would be silly.
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-03 21:40 ` Re: Atomic operations within spinlocks Thomas Munro <thomas.munro@gmail.com>
@ 2020-06-04 07:03 ` Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 22+ messages in thread
From: Michael Paquier @ 2020-06-04 07:03 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Jun 04, 2020 at 09:40:31AM +1200, Thomas Munro wrote:
> Yeah. It'd be fine to move that after the spinlock release. Although
> it's really just for informational purposes only, not for any data
> integrity purpose, reading it before the spinlock acquisition would
> theoretically allow it to appear to be (reportedly) behind
> flushedUpto, which would be silly.
Indeed. This could just be done after the spinlock section. Sorry
about that.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
@ 2020-06-04 17:57 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:50 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:55 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
1 sibling, 2 replies; 22+ messages in thread
From: Tom Lane @ 2020-06-04 17:57 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: pgsql-hackers@lists.postgresql.org
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
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.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-06-04 18:50 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 19:06 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 22+ messages in thread
From: Tom Lane @ 2020-06-04 18:50 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: pgsql-hackers@lists.postgresql.org
I wrote:
> 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
> 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.
Actually ... we could probably use this design with a uint32 counter
as well, on machines where the 64-bit operations would be slow.
In that case, integer overflow of nextVictimBuffer would happen from
time to time, resulting in
1. The next actual victim buffer index would jump strangely. This
doesn't seem like it'd matter at all, as long as it was infrequent.
2. The computed completePasses value would go backwards. I bet
that wouldn't matter too much either, or at least we could teach
BgBufferSync to cope. (I notice the comments therein suggest that
it is already designed to cope with completePasses wrapping around,
so maybe nothing needs to be done.)
If NBuffers was large enough to be a significant fraction of UINT_MAX,
then these corner cases would happen often enough to perhaps be
problematic. But I seriously doubt that'd be possible on hardware
that wasn't capable of using the 64-bit code path.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:50 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-06-04 19:06 ` Andres Freund <andres@anarazel.de>
2020-06-04 19:13 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 22+ messages in thread
From: Andres Freund @ 2020-06-04 19:06 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org
Hi,
On 2020-06-04 14:50:40 -0400, Tom Lane wrote:
> Actually ... we could probably use this design with a uint32 counter
> as well, on machines where the 64-bit operations would be slow.
On skylake-x even a 32bit [i]div is still 26 cycles. That's more than an
atomic operation 18 cycles.
> 2. The computed completePasses value would go backwards. I bet
> that wouldn't matter too much either, or at least we could teach
> BgBufferSync to cope. (I notice the comments therein suggest that
> it is already designed to cope with completePasses wrapping around,
> so maybe nothing needs to be done.)
If we're not concerned about that, then we can remove the
atomic-inside-spinlock, I think. The only reason for that right now is
to avoid assuming a wrong pass number.
I don't think completePasses wrapping around is comparable in frequency
to wrapping around nextVictimBuffer. It's not really worth worrying
about bgwriter wrongly assuming it lapped the clock sweep once ever
UINT32_MAX * NBuffers ticks, but there being a risk every NBuffers seems
worth worrying about.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:50 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 19:06 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
@ 2020-06-04 19:13 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-06-05 03:03 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 22+ messages in thread
From: Tom Lane @ 2020-06-04 19:13 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: pgsql-hackers@lists.postgresql.org
Andres Freund <andres@anarazel.de> writes:
> On 2020-06-04 14:50:40 -0400, Tom Lane wrote:
>> 2. The computed completePasses value would go backwards. I bet
>> that wouldn't matter too much either, or at least we could teach
>> BgBufferSync to cope. (I notice the comments therein suggest that
>> it is already designed to cope with completePasses wrapping around,
>> so maybe nothing needs to be done.)
> If we're not concerned about that, then we can remove the
> atomic-inside-spinlock, I think. The only reason for that right now is
> to avoid assuming a wrong pass number.
Hmm. That might be a less-invasive path to a solution. I can take
a look, if you don't want to.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:50 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 19:06 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 19:13 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-06-05 03:03 ` Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 22+ messages in thread
From: Andres Freund @ 2020-06-05 03:03 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org
Hi,
On 2020-06-04 15:13:29 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2020-06-04 14:50:40 -0400, Tom Lane wrote:
> >> 2. The computed completePasses value would go backwards. I bet
> >> that wouldn't matter too much either, or at least we could teach
> >> BgBufferSync to cope. (I notice the comments therein suggest that
> >> it is already designed to cope with completePasses wrapping around,
> >> so maybe nothing needs to be done.)
>
> > If we're not concerned about that, then we can remove the
> > atomic-inside-spinlock, I think. The only reason for that right now is
> > to avoid assuming a wrong pass number.
>
> Hmm. That might be a less-invasive path to a solution. I can take
> a look, if you don't want to.
First, I think it would be problematic:
/*
* Find out where the freelist clock sweep currently is, and how many
* buffer allocations have happened since our last call.
*/
strategy_buf_id = StrategySyncStart(&strategy_passes, &recent_alloc);
...
/*
* Compute strategy_delta = how many buffers have been scanned by the
* clock sweep since last time. If first time through, assume none. Then
* see if we are still ahead of the clock sweep, and if so, how many
* buffers we could scan before we'd catch up with it and "lap" it. Note:
* weird-looking coding of xxx_passes comparisons are to avoid bogus
* behavior when the passes counts wrap around.
*/
if (saved_info_valid)
{
int32 passes_delta = strategy_passes - prev_strategy_passes;
strategy_delta = strategy_buf_id - prev_strategy_buf_id;
strategy_delta += (long) passes_delta * NBuffers;
Assert(strategy_delta >= 0);
ISTM that if we can get an out-of-sync strategy_passes and
strategy_buf_id we'll end up with a pretty wrong strategy_delta. Which,
I think, can cause to reset bgwriter's position:
else
{
/*
* We're behind, so skip forward to the strategy point and start
* cleaning from there.
*/
#ifdef BGW_DEBUG
elog(DEBUG2, "bgwriter behind: bgw %u-%u strategy %u-%u delta=%ld",
next_passes, next_to_clean,
strategy_passes, strategy_buf_id,
strategy_delta);
#endif
next_to_clean = strategy_buf_id;
next_passes = strategy_passes;
bufs_to_lap = NBuffers;
}
While I think that the whole logic in BgBufferSync doesn't make a whole
lot of sense, it does seem to me this has a fair potential to make it
worse. In a scenario with a decent cache hit ratio (leading to high
usagecounts) and a not that large NBuffers, we can end up up doing quite
a few passes (as in many a second), so it might not be that hard to hit
this.
I am not immediately coming up with a cheap solution that doesn't do the
atomics-within-spinlock thing. The best I can come up with is using a
64bit atomic, with the upper 32bit containing the number of passes, and
the lower 32bit containing the current buffer. Where the lower 32bit /
the buffer is handled like it currently is, i.e. we "try" to keep it
below NBuffers. So % is only used for the "cold" path. That'd just add a
64->32 bit cast in the hot path, which shouldn't be measurable. But it'd
regress platforms without 64bit atomics substantially.
We could obviously also just rewrite the BgBufferSync() logic, so it
doesn't care about things like "lapping", but that's not an easy change.
So the best I can really suggest, unless we were to agree on atomics
being ok inside spinlocks, is probably to just replace the spinlock with
an lwlock. That'd perhaps cause a small slowdown for a few cases, but
it'd make workload that e.g. use the freelist a lot (e.g. when tables
are dropped regularly) scale better.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-06-04 18:55 ` Andres Freund <andres@anarazel.de>
2020-06-04 19:07 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 22+ messages in thread
From: Andres Freund @ 2020-06-04 18:55 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:55 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
@ 2020-06-04 19:07 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-06-05 02:33 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 22+ messages in thread
From: Tom Lane @ 2020-06-04 19:07 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: pgsql-hackers@lists.postgresql.org
Andres Freund <andres@anarazel.de> writes:
> 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.
If you error out of getting the inner spinlock, the outer spinlock
is stuck, permanently, because there is no mechanism for spinlock
release during transaction abort. Admittedly it's not very likely
for the inner acquisition to fail, but it's possible. Aside from
timeout scenarios (e.g., process holding lock gets swapped out to
Timbuktu), it could be that both spinlocks are mapped onto a single
implementation lock by spin.c, which notes
* We map all spinlocks onto a set of NUM_SPINLOCK_SEMAPHORES semaphores.
* It's okay to map multiple spinlocks onto one semaphore because no process
* should ever hold more than one at a time.
You've falsified that argument ... and no, I don't want to upgrade
the spinlock infrastructure enough to make this OK. We shouldn't
ever be holding spinlocks long enough, or doing anything complicated
enough inside them, for such an upgrade to have merit.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:55 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 19:07 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-06-05 02:33 ` Andres Freund <andres@anarazel.de>
2020-06-06 00:19 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 22+ messages in thread
From: Andres Freund @ 2020-06-05 02:33 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org
Hi,
On 2020-06-04 15:07:34 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > 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.
>
> If you error out of getting the inner spinlock, the outer spinlock
> is stuck, permanently, because there is no mechanism for spinlock
> release during transaction abort. Admittedly it's not very likely
> for the inner acquisition to fail, but it's possible.
We PANIC on stuck spinlocks, so I don't think that's a problem.
> * We map all spinlocks onto a set of NUM_SPINLOCK_SEMAPHORES semaphores.
> * It's okay to map multiple spinlocks onto one semaphore because no process
> * should ever hold more than one at a time.
>
> You've falsified that argument ... and no, I don't want to upgrade
> the spinlock infrastructure enough to make this OK.
Well, theoretically we take care to avoid this problem. That's why we
have a separate define for spinlocks and atomics:
/*
* When we don't have native spinlocks, we use semaphores to simulate them.
* Decreasing this value reduces consumption of OS resources; increasing it
* may improve performance, but supplying a real spinlock implementation is
* probably far better.
*/
#define NUM_SPINLOCK_SEMAPHORES 128
/*
* When we have neither spinlocks nor atomic operations support we're
* implementing atomic operations on top of spinlock on top of semaphores. To
* be safe against atomic operations while holding a spinlock separate
* semaphores have to be used.
*/
#define NUM_ATOMICS_SEMAPHORES 64
and
#ifndef HAVE_SPINLOCKS
/*
* NB: If we're using semaphore based TAS emulation, be careful to use a
* separate set of semaphores. Otherwise we'd get in trouble if an atomic
* var would be manipulated while spinlock is held.
*/
s_init_lock_sema((slock_t *) &ptr->sema, true);
#else
SpinLockInit((slock_t *) &ptr->sema);
#endif
But it looks like that code is currently buggy (and looks like it always
has been), because we don't look at the nested argument when
initializing the semaphore. So we currently allocate too many
semaphores, without benefiting from them :(.
> We shouldn't ever be holding spinlocks long enough, or doing anything
> complicated enough inside them, for such an upgrade to have merit.
Well, I don't think atomic instructions are that complicated. And I
think prohibiting atomics-within-spinlock adds a problematic
restriction, without much in the way of benefits:
There's plenty things where it's somewhat easy to make the fast-path
lock-free, but the slow path still needs a lock (e.g. around a
freelist). And for those it's really useful to still be able to have a
coherent update to an atomic variable, to synchronize with the fast-path
that doesn't take the spinlock.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:55 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 19:07 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-05 02:33 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
@ 2020-06-06 00:19 ` Andres Freund <andres@anarazel.de>
2020-06-06 01:01 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-09 06:08 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-10 11:26 ` Re: Atomic operations within spinlocks Robert Haas <robertmhaas@gmail.com>
0 siblings, 3 replies; 22+ messages in thread
From: Andres Freund @ 2020-06-06 00:19 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org
Hi,
On 2020-06-04 19:33:02 -0700, Andres Freund wrote:
> But it looks like that code is currently buggy (and looks like it always
> has been), because we don't look at the nested argument when
> initializing the semaphore. So we currently allocate too many
> semaphores, without benefiting from them :(.
I wrote a patch for this, and when I got around to to testing it, I
found that our tests currently don't pass when using both
--disable-spinlocks and --disable-atomics. Turns out to not be related
to the issue above, but the global barrier support added in 13.
That *reads* two 64 bit atomics in a signal handler. Which is normally
fine, but not at all cool when atomics (or just 64 bit atomics) are
backed by spinlocks. Because we can "self interrupt" while already
holding the spinlock.
It looks to me that that's a danger whenever 64bit atomics are backed by
spinlocks, not just when both --disable-spinlocks and --disable-atomics
are used. But I suspect that it's really hard to hit the tiny window of
danger when those options aren't used. While we have buildfarm animals
testing each of those separately, we don't have one that tests both
together...
I'm not really sure what to do about that issue. The easisest thing
would probably be to change the barrier generation to 32bit (which
doesn't have to use locks for reads in any situation). I tested doing
that, and it fixes the hangs for me.
Randomly noticed while looking at the code:
uint64 flagbit = UINT64CONST(1) << (uint64) type;
that shouldn't be 64bit, right?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:55 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 19:07 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-05 02:33 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-06 00:19 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
@ 2020-06-06 01:01 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-06-06 02:31 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2 siblings, 1 reply; 22+ messages in thread
From: Tom Lane @ 2020-06-06 01:01 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Robert Haas <robertmhaas@gmail.com>; pgsql-hackers@lists.postgresql.org
Andres Freund <andres@anarazel.de> writes:
> I wrote a patch for this, and when I got around to to testing it, I
> found that our tests currently don't pass when using both
> --disable-spinlocks and --disable-atomics. Turns out to not be related
> to the issue above, but the global barrier support added in 13.
> That *reads* two 64 bit atomics in a signal handler. Which is normally
> fine, but not at all cool when atomics (or just 64 bit atomics) are
> backed by spinlocks. Because we can "self interrupt" while already
> holding the spinlock.
This is the sort of weird platform-specific problem that I'd prefer to
avoid by minimizing our expectations of what spinlocks can be used for.
> I'm not really sure what to do about that issue. The easisest thing
> would probably be to change the barrier generation to 32bit (which
> doesn't have to use locks for reads in any situation).
Yeah, I think we need a hard rule that you can't use a spinlock in
an interrupt handler --- which means no atomics that don't have
non-spinlock implementations on every platform.
At some point I think we'll have to give up --disable-spinlocks;
it's really of pretty marginal use (how often does anyone port PG
to a new CPU type?) and the number of weird interactions it adds
in this area seems like more than it's worth. But of course
requiring 64-bit atomics is still a step too far.
> Randomly noticed while looking at the code:
> uint64 flagbit = UINT64CONST(1) << (uint64) type;
I'm surprised we didn't get any compiler warnings about that.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:55 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 19:07 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-05 02:33 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-06 00:19 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-06 01:01 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-06-06 02:31 ` Andres Freund <andres@anarazel.de>
2020-06-06 02:52 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 22+ messages in thread
From: Andres Freund @ 2020-06-06 02:31 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; pgsql-hackers@lists.postgresql.org
Hi,
On 2020-06-05 21:01:56 -0400, Tom Lane wrote:
> > I'm not really sure what to do about that issue. The easisest thing
> > would probably be to change the barrier generation to 32bit (which
> > doesn't have to use locks for reads in any situation).
>
> Yeah, I think we need a hard rule that you can't use a spinlock in
> an interrupt handler --- which means no atomics that don't have
> non-spinlock implementations on every platform.
Yea, that might be the easiest thing to do. The only other thing I can
think of would be to mask all signals for the duration of the
atomic-using-spinlock operation. That'd make the fallback noticably more
expensive, but otoh, do we care enough?
I think a SIGNAL_HANDLER_BEGIN(); SIGNAL_HANDLER_END(); to back an
Assert(!InSignalHandler()); could be quite useful. Could also save
errno etc.
> At some point I think we'll have to give up --disable-spinlocks; it's
> really of pretty marginal use (how often does anyone port PG to a new
> CPU type?) and the number of weird interactions it adds in this area
> seems like more than it's worth.
Indeed. And any new architecture one would port PG to would have good
enough compiler intrinsics to make that trivial. I still think it'd make
sense to have a fallback implementation using compiler intrinsics...
And I think we should just require 32bit atomics at the same time. Would
probably kill gaur though.
I did just find a longstanding bug in the spinlock emulation code:
void
s_init_lock_sema(volatile slock_t *lock, bool nested)
{
static int counter = 0;
*lock = ((++counter) % NUM_SPINLOCK_SEMAPHORES) + 1;
}
void
s_unlock_sema(volatile slock_t *lock)
{
int lockndx = *lock;
if (lockndx <= 0 || lockndx > NUM_SPINLOCK_SEMAPHORES)
elog(ERROR, "invalid spinlock number: %d", lockndx);
PGSemaphoreUnlock(SpinlockSemaArray[lockndx - 1]);
}
I don't think it's ok that counter is a signed integer... While it maybe
used to be unlikely that we ever have that many spinlocks, I don't think
it's that hard anymore, because we dynamically allocate them for a lot
of parallel query stuff. A small regression test that initializes
enough spinlocks indeed errors out with
2020-06-05 18:08:29.110 PDT [734946][3/2:0] ERROR: invalid spinlock number: -126
2020-06-05 18:08:29.110 PDT [734946][3/2:0] STATEMENT: SELECT test_atomic_ops();
> > Randomly noticed while looking at the code:
> > uint64 flagbit = UINT64CONST(1) << (uint64) type;
>
> I'm surprised we didn't get any compiler warnings about that.
Unfortunately I don't think one can currently compile postgres with
warnings for "implicit casts" enabled :(.
> But of course requiring 64-bit atomics is still a step too far.
If we had a 32bit compare-exchange it ought to be possible to write a
signal-safe emulation of 64bit atomics. I think. Something *roughly*
like:
typedef struct pg_atomic_uint64
{
/*
* Meaning of state bits:
* 0-1: current valid
* 2-4: current proposed
* 5: in signal handler
* 6-31: pid of proposer
*/
pg_atomic_uint32 state;
/*
* One current value, two different proposed values.
*/
uint64 value[3];
} pg_atomic_uint64;
The update protocol would be something roughly like:
bool
pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 *expected, uint64 newval)
{
while (true)
{
uint32 old_state = pg_atomic_read_u32(&ptr->state);
uint32 updater_pid = PID_FROM_STATE(old_state);
uint32 new_state;
uint64 old_value;
int proposing;
/*
* Value changed, so fail. This is obviously racy, but we'll
* notice concurrent updates later.
*/
if (ptr->value[VALID_FIELD(old_state)] != *expected)
{
return false;
}
if (updater_pid == INVALID_PID)
{
new_state = old_state;
/* signal that current process is updating */
new_state |= MyProcPid >> PID_STATE_SHIFT;
if (InSignalHandler)
new_state |= PROPOSER_IN_SIGNAL_HANDLER_BIT;
/* set which index is being proposed */
new_state = (new_state & ~PROPOSER_BITS) |
NEXT_PROPOSED_FIELD(old_state, &proposing);
/*
* If we successfully can update state to contain our new
* value, we have a right to do so, and can only be
* interrupted by ourselves, in a signal handler.
*/
if (!pg_atomic_compare_exchange(&ptr->state, &old_state, new_state))
{
/* somebody else updated, restart */
continue;
}
old_state = new_state;
/*
* It's ok to compare the values now. If we are interrupted
* by a signal handler, we'll notice when updating
* state. There's no danger updating the same proposed value
* in two processes, because they they always would get
* offsets to propse into.
*/
ptr->value[proposing] = newval;
/* set the valid field to the one we just filled in */
new_state = (new_state & ~VALID_FIELD_BITS) | proposed;
/* remove ourselve as updater */
new_state &= UPDATER_BITS;
if (!pg_atomic_compare_exchange(&ptr->state, &old_state, new_state))
{
/*
* Should only happen when we were interrupted by this
* processes' handler.
*/
Assert(!InSignalHandler);
/*
* Signal handler must have cleaned out pid as updater.
*/
Assert(PID_FROM_STATE(old_state) != MyProcPid);
continue;
}
else
{
return true;
}
}
else if (PID_FROM_STATE(current_state) == MyProcPid)
{
/*
* This should only happen when in a signal handler. We don't
* currently allow nesting of signal handlers.
*/
Assert(!(current_state & PROPOSER_IN_SIGNAL_HANDLER_BIT));
/* interrupt our own non-signal-handler update */
new_state = old_state | PROPOSER_IN_SIGNAL_HANDLER_BIT;
/* set which index is being proposed */
new_state = (new_state & ~PROPOSER_BITS) |
NEXT_PROPOSED_FIELD(old_state, &proposing);
// FIXME: assert that previous value still was what we assumed
pg_atomic_exchange_u32(&ptr_state.state, new_state);
}
else
{
do
{
pg_spin_delay();
current_state = pg_atomic_read_u32(&ptr->state);
} while (PID_FROM_STATE(current_state) != INVALID_PID)
}
}
}
While that's not trivial, it'd not be that expensive. The happy path
would be two 32bit atomic operations to simulate a 64bit one.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:55 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 19:07 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-05 02:33 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-06 00:19 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-06 01:01 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-06 02:31 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
@ 2020-06-06 02:52 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-06-06 03:32 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 22+ messages in thread
From: Tom Lane @ 2020-06-06 02:52 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Robert Haas <robertmhaas@gmail.com>; pgsql-hackers@lists.postgresql.org
Andres Freund <andres@anarazel.de> writes:
> On 2020-06-05 21:01:56 -0400, Tom Lane wrote:
>> At some point I think we'll have to give up --disable-spinlocks; it's
>> really of pretty marginal use (how often does anyone port PG to a new
>> CPU type?) and the number of weird interactions it adds in this area
>> seems like more than it's worth.
> Indeed. And any new architecture one would port PG to would have good
> enough compiler intrinsics to make that trivial. I still think it'd make
> sense to have a fallback implementation using compiler intrinsics...
> And I think we should just require 32bit atomics at the same time. Would
> probably kill gaur though.
Not only gaur. A quick buildfarm survey finds these active members
reporting not having 32-bit atomics:
anole | 2020-06-05 11:20:17 | pgac_cv_gcc_atomic_int32_cas=no
chipmunk | 2020-05-29 22:27:56 | pgac_cv_gcc_atomic_int32_cas=no
curculio | 2020-06-05 22:30:06 | pgac_cv_gcc_atomic_int32_cas=no
frogfish | 2020-05-31 13:00:25 | pgac_cv_gcc_atomic_int32_cas=no
gaur | 2020-05-19 13:33:25 | pgac_cv_gcc_atomic_int32_cas=no
gharial | 2020-06-05 12:41:14 | pgac_cv_gcc_atomic_int32_cas=no
hornet | 2020-06-05 09:11:26 | pgac_cv_gcc_atomic_int32_cas=no
hoverfly | 2020-06-05 22:06:14 | pgac_cv_gcc_atomic_int32_cas=no
locust | 2020-06-05 10:14:29 | pgac_cv_gcc_atomic_int32_cas=no
mandrill | 2020-06-05 09:20:03 | pgac_cv_gcc_atomic_int32_cas=no
prairiedog | 2020-06-05 09:55:49 | pgac_cv_gcc_atomic_int32_cas=no
It looks to me like this is mostly about compiler support not the
hardware; that doesn't make it not a problem, though. (I also
remain skeptical about the quality of the compiler intrinsics
on non-mainstream hardware.)
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:55 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 19:07 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-05 02:33 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-06 00:19 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-06 01:01 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-06 02:31 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-06 02:52 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-06-06 03:32 ` Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 22+ messages in thread
From: Andres Freund @ 2020-06-06 03:32 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; pgsql-hackers@lists.postgresql.org
Hi,
On 2020-06-05 22:52:47 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2020-06-05 21:01:56 -0400, Tom Lane wrote:
> >> At some point I think we'll have to give up --disable-spinlocks; it's
> >> really of pretty marginal use (how often does anyone port PG to a new
> >> CPU type?) and the number of weird interactions it adds in this area
> >> seems like more than it's worth.
>
> > Indeed. And any new architecture one would port PG to would have good
> > enough compiler intrinsics to make that trivial. I still think it'd make
> > sense to have a fallback implementation using compiler intrinsics...
>
> > And I think we should just require 32bit atomics at the same time. Would
> > probably kill gaur though.
>
> Not only gaur. A quick buildfarm survey finds these active members
> reporting not having 32-bit atomics:
Hm, I don't think that's the right test. We have bespoke code to support
most of these, I think:
> anole | 2020-06-05 11:20:17 | pgac_cv_gcc_atomic_int32_cas=no
Has support via acc specific intrinsics.
> chipmunk | 2020-05-29 22:27:56 | pgac_cv_gcc_atomic_int32_cas=no
Doesn't have support for __atomic, but does have support for 32bit
__sync.
> gharial | 2020-06-05 12:41:14 | pgac_cv_gcc_atomic_int32_cas=no
__sync support for both 32 and 64 bit.
> curculio | 2020-06-05 22:30:06 | pgac_cv_gcc_atomic_int32_cas=no
> frogfish | 2020-05-31 13:00:25 | pgac_cv_gcc_atomic_int32_cas=no
__sync support for both 32 and 64 bit.
> mandrill | 2020-06-05 09:20:03 | pgac_cv_gcc_atomic_int32_cas=no
__sync support for 32, as well as as inline asm for 32bit atomics
(although we might be able to add 64 bit).
> hornet | 2020-06-05 09:11:26 | pgac_cv_gcc_atomic_int32_cas=no
> hoverfly | 2020-06-05 22:06:14 | pgac_cv_gcc_atomic_int32_cas=no
__sync support for both 32 and 64 bit, and we have open coded ppc asm.
> locust | 2020-06-05 10:14:29 | pgac_cv_gcc_atomic_int32_cas=no
> prairiedog | 2020-06-05 09:55:49 | pgac_cv_gcc_atomic_int32_cas=no
Wee, these don't have __sync? But I think it should be able to use the
asm ppc implementation for 32 bit atomics.
> gaur | 2020-05-19 13:33:25 | pgac_cv_gcc_atomic_int32_cas=no
As far as I understand pa-risc doesn't have any atomic instructions
except for TAS.
So I think gaur is really the only one that'd drop.
> It looks to me like this is mostly about compiler support not the
> hardware; that doesn't make it not a problem, though. (I also
> remain skeptical about the quality of the compiler intrinsics
> on non-mainstream hardware.)
I think that's fair enough for really old platforms, but at least for
gcc / clang I don't think it's a huge concern for newer ones. Even if
not mainstream. For gcc/clang the intrinsics basically back the
C11/C++11 "language level" atomics support. And those are extremely
widely used these days.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:55 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 19:07 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-05 02:33 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-06 00:19 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
@ 2020-06-09 06:08 ` Andres Freund <andres@anarazel.de>
2 siblings, 0 replies; 22+ messages in thread
From: Andres Freund @ 2020-06-09 06:08 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org
On 2020-06-05 17:19:26 -0700, Andres Freund wrote:
> Hi,
>
> On 2020-06-04 19:33:02 -0700, Andres Freund wrote:
> > But it looks like that code is currently buggy (and looks like it always
> > has been), because we don't look at the nested argument when
> > initializing the semaphore. So we currently allocate too many
> > semaphores, without benefiting from them :(.
>
> I wrote a patch for this, and when I got around to to testing it, I
> found that our tests currently don't pass when using both
> --disable-spinlocks and --disable-atomics. Turns out to not be related
> to the issue above, but the global barrier support added in 13.
>
> That *reads* two 64 bit atomics in a signal handler. Which is normally
> fine, but not at all cool when atomics (or just 64 bit atomics) are
> backed by spinlocks. Because we can "self interrupt" while already
> holding the spinlock.
>
> It looks to me that that's a danger whenever 64bit atomics are backed by
> spinlocks, not just when both --disable-spinlocks and --disable-atomics
> are used. But I suspect that it's really hard to hit the tiny window of
> danger when those options aren't used. While we have buildfarm animals
> testing each of those separately, we don't have one that tests both
> together...
>
> I'm not really sure what to do about that issue. The easisest thing
> would probably be to change the barrier generation to 32bit (which
> doesn't have to use locks for reads in any situation). I tested doing
> that, and it fixes the hangs for me.
>
>
> Randomly noticed while looking at the code:
> uint64 flagbit = UINT64CONST(1) << (uint64) type;
>
> that shouldn't be 64bit, right?
Attached is a series of patches addressing these issues, of varying
quality:
1) This fixes the above mentioned issue in the global barrier code by
using 32bit atomics. That might be fine, or it might not. I just
included it here because otherwise the tests cannot be run fully.
2) Fixes spinlock emulation when more than INT_MAX spinlocks are
initialized in the lifetime of a single backend
3) Add spinlock tests to normal regression tests.
- Currently as part of test_atomic_ops. Probably not worth having a
separate SQL function?
- Currently contains a test for 1) that's run when the spinlock
emulation is used. Probably too slow to actually indclude? Takes 15s
on my computer... OTOH, it's just with --disable-spinlocks...
- Could probably remove the current spinlock tests after this. The
only thing they additionally test is a stuck spinlock. Since
they're not run currently, they don't seem worth much?
4) Fix the potential for deadlocks when using atomics while holding a
spinlock, add tests for that.
Any comments?
Greetings,
Andres Freund
Attachments: download all (4 files, .tar.gz)
v1-0001-WORKAROUND-Avoid-spinlock-use-in-signal-handler-b.patch (text/x-diff, 7.2K) download
From 17a7730c282018d02e3d82990f4fd84e0d2f6ce0 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 8 Jun 2020 17:10:26 -0700
Subject: [PATCH v1 1/4] WORKAROUND: Avoid spinlock use in signal handler by
using 32bit generations.
As 64bit atomic reads aren't aren't guaranteed to be atomic on all
platforms we can end up with spinlocks being used inside signal
handlers. Which obviously is dangerous, as that can easily create self
deadlocks.
32bit atomics don't have that problem. But 32bit generations might not
be enough in all situations (?). So this possibly isn't the right
fix.
The problem can easily be reproduced by running make check in a
--disable-spinlocks --disable-atomics build.
Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
src/include/storage/procsignal.h | 4 +--
src/backend/storage/ipc/procsignal.c | 48 ++++++++++++++--------------
2 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index a0c0bc3ce55..c5a7e362dd9 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -65,8 +65,8 @@ extern void ProcSignalInit(int pss_idx);
extern int SendProcSignal(pid_t pid, ProcSignalReason reason,
BackendId backendId);
-extern uint64 EmitProcSignalBarrier(ProcSignalBarrierType type);
-extern void WaitForProcSignalBarrier(uint64 generation);
+extern uint32 EmitProcSignalBarrier(ProcSignalBarrierType type);
+extern void WaitForProcSignalBarrier(uint32 generation);
extern void ProcessProcSignalBarrier(void);
extern void procsignal_sigusr1_handler(SIGNAL_ARGS);
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index c809196d06a..eba424e15a7 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -60,7 +60,7 @@ typedef struct
{
pid_t pss_pid;
sig_atomic_t pss_signalFlags[NUM_PROCSIGNALS];
- pg_atomic_uint64 pss_barrierGeneration;
+ pg_atomic_uint32 pss_barrierGeneration;
pg_atomic_uint32 pss_barrierCheckMask;
} ProcSignalSlot;
@@ -72,7 +72,7 @@ typedef struct
*/
typedef struct
{
- pg_atomic_uint64 psh_barrierGeneration;
+ pg_atomic_uint32 psh_barrierGeneration;
ProcSignalSlot psh_slot[FLEXIBLE_ARRAY_MEMBER];
} ProcSignalHeader;
@@ -126,7 +126,7 @@ ProcSignalShmemInit(void)
{
int i;
- pg_atomic_init_u64(&ProcSignal->psh_barrierGeneration, 0);
+ pg_atomic_init_u32(&ProcSignal->psh_barrierGeneration, 0);
for (i = 0; i < NumProcSignalSlots; ++i)
{
@@ -134,7 +134,7 @@ ProcSignalShmemInit(void)
slot->pss_pid = 0;
MemSet(slot->pss_signalFlags, 0, sizeof(slot->pss_signalFlags));
- pg_atomic_init_u64(&slot->pss_barrierGeneration, PG_UINT64_MAX);
+ pg_atomic_init_u32(&slot->pss_barrierGeneration, PG_UINT32_MAX);
pg_atomic_init_u32(&slot->pss_barrierCheckMask, 0);
}
}
@@ -151,7 +151,7 @@ void
ProcSignalInit(int pss_idx)
{
volatile ProcSignalSlot *slot;
- uint64 barrier_generation;
+ uint32 barrier_generation;
Assert(pss_idx >= 1 && pss_idx <= NumProcSignalSlots);
@@ -178,8 +178,8 @@ ProcSignalInit(int pss_idx)
*/
pg_atomic_write_u32(&slot->pss_barrierCheckMask, 0);
barrier_generation =
- pg_atomic_read_u64(&ProcSignal->psh_barrierGeneration);
- pg_atomic_write_u64(&slot->pss_barrierGeneration, barrier_generation);
+ pg_atomic_read_u32(&ProcSignal->psh_barrierGeneration);
+ pg_atomic_write_u32(&slot->pss_barrierGeneration, barrier_generation);
pg_memory_barrier();
/* Mark slot with my PID */
@@ -230,7 +230,7 @@ CleanupProcSignalState(int status, Datum arg)
* Make this slot look like it's absorbed all possible barriers, so that
* no barrier waits block on it.
*/
- pg_atomic_write_u64(&slot->pss_barrierGeneration, PG_UINT64_MAX);
+ pg_atomic_write_u32(&slot->pss_barrierGeneration, PG_UINT32_MAX);
slot->pss_pid = 0;
}
@@ -317,11 +317,11 @@ SendProcSignal(pid_t pid, ProcSignalReason reason, BackendId backendId)
* Callers are entitled to assume that this function will not throw ERROR
* or FATAL.
*/
-uint64
+uint32
EmitProcSignalBarrier(ProcSignalBarrierType type)
{
- uint64 flagbit = UINT64CONST(1) << (uint64) type;
- uint64 generation;
+ uint32 flagbit = 1 << (uint32) type;
+ uint32 generation;
/*
* Set all the flags.
@@ -329,7 +329,7 @@ EmitProcSignalBarrier(ProcSignalBarrierType type)
* Note that pg_atomic_fetch_or_u32 has full barrier semantics, so this is
* totally ordered with respect to anything the caller did before, and
* anything that we do afterwards. (This is also true of the later call to
- * pg_atomic_add_fetch_u64.)
+ * pg_atomic_add_fetch_u32.)
*/
for (int i = 0; i < NumProcSignalSlots; i++)
{
@@ -342,7 +342,7 @@ EmitProcSignalBarrier(ProcSignalBarrierType type)
* Increment the generation counter.
*/
generation =
- pg_atomic_add_fetch_u64(&ProcSignal->psh_barrierGeneration, 1);
+ pg_atomic_add_fetch_u32(&ProcSignal->psh_barrierGeneration, 1);
/*
* Signal all the processes, so that they update their advertised barrier
@@ -379,16 +379,16 @@ EmitProcSignalBarrier(ProcSignalBarrierType type)
* 1 second.
*/
void
-WaitForProcSignalBarrier(uint64 generation)
+WaitForProcSignalBarrier(uint32 generation)
{
long timeout = 125L;
for (int i = NumProcSignalSlots - 1; i >= 0; i--)
{
volatile ProcSignalSlot *slot = &ProcSignal->psh_slot[i];
- uint64 oldval;
+ uint32 oldval;
- oldval = pg_atomic_read_u64(&slot->pss_barrierGeneration);
+ oldval = pg_atomic_read_u32(&slot->pss_barrierGeneration);
while (oldval < generation)
{
int events;
@@ -401,7 +401,7 @@ WaitForProcSignalBarrier(uint64 generation)
timeout, WAIT_EVENT_PROC_SIGNAL_BARRIER);
ResetLatch(MyLatch);
- oldval = pg_atomic_read_u64(&slot->pss_barrierGeneration);
+ oldval = pg_atomic_read_u32(&slot->pss_barrierGeneration);
if (events & WL_TIMEOUT)
timeout = Min(timeout * 2, 1000L);
}
@@ -428,7 +428,7 @@ WaitForProcSignalBarrier(uint64 generation)
void
ProcessProcSignalBarrier(void)
{
- uint64 generation;
+ uint32 generation;
uint32 flags;
/* Exit quickly if there's no work to do. */
@@ -443,7 +443,7 @@ ProcessProcSignalBarrier(void)
* happens before we atomically extract the flags, and that any subsequent
* state changes happen afterward.
*/
- generation = pg_atomic_read_u64(&ProcSignal->psh_barrierGeneration);
+ generation = pg_atomic_read_u32(&ProcSignal->psh_barrierGeneration);
flags = pg_atomic_exchange_u32(&MyProcSignalSlot->pss_barrierCheckMask, 0);
/*
@@ -466,7 +466,7 @@ ProcessProcSignalBarrier(void)
* things have changed further, it'll get fixed up when this function is
* next called.
*/
- pg_atomic_write_u64(&MyProcSignalSlot->pss_barrierGeneration, generation);
+ pg_atomic_write_u32(&MyProcSignalSlot->pss_barrierGeneration, generation);
}
static void
@@ -515,11 +515,11 @@ CheckProcSignalBarrier(void)
if (slot != NULL)
{
- uint64 mygen;
- uint64 curgen;
+ uint32 mygen;
+ uint32 curgen;
- mygen = pg_atomic_read_u64(&slot->pss_barrierGeneration);
- curgen = pg_atomic_read_u64(&ProcSignal->psh_barrierGeneration);
+ mygen = pg_atomic_read_u32(&slot->pss_barrierGeneration);
+ curgen = pg_atomic_read_u32(&ProcSignal->psh_barrierGeneration);
return (mygen != curgen);
}
--
2.25.0.114.g5b0ca878e0
v1-0002-spinlock-emulation-Fix-bug-when-more-than-INT_MAX.patch (text/x-diff, 1023B) download
From 23ded090b1ee9d91ea4455ddde30427c3756c78a Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 8 Jun 2020 15:25:49 -0700
Subject: [PATCH v1 2/4] spinlock emulation: Fix bug when more than INT_MAX
spinlocks are initialized.
Once the counter goes negative we ended up with spinlocks that errored
out on first use (due to check in tas_sema).
Author: Andres Freund
Discussion: https://postgr.es/m/20200606023103.avzrctgv7476xj7i@alap3.anarazel.de
Backpatch: 9.5-
---
src/backend/storage/lmgr/spin.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/storage/lmgr/spin.c b/src/backend/storage/lmgr/spin.c
index 4d2a4c6641a..753943e46d6 100644
--- a/src/backend/storage/lmgr/spin.c
+++ b/src/backend/storage/lmgr/spin.c
@@ -106,7 +106,7 @@ SpinlockSemaInit(void)
void
s_init_lock_sema(volatile slock_t *lock, bool nested)
{
- static int counter = 0;
+ static uint32 counter = 0;
*lock = ((++counter) % NUM_SPINLOCK_SEMAPHORES) + 1;
}
--
2.25.0.114.g5b0ca878e0
v1-0003-Add-basic-spinlock-tests-to-regression-tests.patch (text/x-diff, 3.9K) download
From 7eb9faf6b33792a22eccc4dd6f5fc6727c17fb41 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 8 Jun 2020 16:36:51 -0700
Subject: [PATCH v1 3/4] Add basic spinlock tests to regression tests.
As s_lock_test, the already existing test for spinlocks, isn't run in
an automated fashion (and doesn't test a normal backend environment),
adding tests that are run as part of a normal regression run is a good
idea.
Currently the new tests are run as part of the pre-existing
test_atomic_ops() test. That can probably be quibbled about.
The only operations that s_lock_test tests but the new tests don't are
the detection of a stuck spinlock and S_LOCK_FREE (which is otherwise
unused).
This currently contains a test for more than INT_MAX spinlocks (only
run with --disable-spinlocks), to ensure the previous commit fixing a
bug with more than INT_MAX spinlock initializations is correct, but
that might be too slow to enable generally.
It might be worth retiring s_lock_test after this. The added coverage
of a stuck spinlock probably isn't worth the added complexity?
Author: Andres Freund
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
src/test/regress/regress.c | 89 ++++++++++++++++++++++++++++++++++++++
1 file changed, 89 insertions(+)
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 960c155e5f2..a48f9de2532 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -794,6 +794,92 @@ test_atomic_uint64(void)
EXPECT_EQ_U64(pg_atomic_fetch_and_u64(&var, ~0), 0);
}
+static void
+test_spinlock(void)
+{
+ {
+ struct test_lock_struct
+ {
+ uint32 data_before;
+ slock_t lock;
+ uint32 data_after;
+ } struct_w_lock;
+
+ struct_w_lock.data_before = 0x44;
+ struct_w_lock.data_after = 0x17;
+
+ /* test basic operations via the SpinLock* API */
+ SpinLockInit(&struct_w_lock.lock);
+ SpinLockAcquire(&struct_w_lock.lock);
+ SpinLockRelease(&struct_w_lock.lock);
+
+ /* test basic operations via underlying S_* API */
+ S_INIT_LOCK(&struct_w_lock.lock);
+ S_LOCK(&struct_w_lock.lock);
+ S_UNLOCK(&struct_w_lock.lock);
+
+ /* and that "contended" acquisition works */
+ s_lock(&struct_w_lock.lock, "testfile", 17, "testfunc");
+ S_UNLOCK(&struct_w_lock.lock);
+
+ /*
+ * Check, using TAS directly, that a single spin cyle doesn't block
+ * when acquiring an already acquired lock.
+ */
+#ifdef TAS
+ S_LOCK(&struct_w_lock.lock);
+ if (!TAS(&struct_w_lock.lock))
+ elog(ERROR, "acquired already held spinlock");
+
+#ifdef TAS_SPIN
+ if (!TAS_SPIN(&struct_w_lock.lock))
+ elog(ERROR, "acquired already held spinlock");
+#endif /* defined(TAS_SPIN) */
+
+ S_UNLOCK(&struct_w_lock.lock);
+#endif /* defined(TAS) */
+
+ /*
+ * Verify that after all of this the non-lock contents are still
+ * correct.
+ */
+ EXPECT_EQ_U32(struct_w_lock.data_before, 0x44);
+ EXPECT_EQ_U32(struct_w_lock.data_after, 0x17);
+ }
+
+ /*
+ * Ensure that allocating more than INT32_MAX simulated spinlocks
+ * works. This is probably too expensive to run each regression test.
+ */
+#ifndef HAVE_SPINLOCKS
+ {
+ /*
+ * Initialize enough spinlocks to advance counter close to
+ * wraparound. It's too expensive to perform acquire/release for each,
+ * as those may be syscalls when the spinlock emulation is used (and
+ * even just atomic TAS would be expensive).
+ */
+ for (uint32 i = 0; i < INT32_MAX - 100000; i++)
+ {
+ slock_t lock;
+
+ SpinLockInit(&lock);
+ }
+
+ for (uint32 i = 0; i < 200000; i++)
+ {
+ slock_t lock;
+
+ SpinLockInit(&lock);
+
+ SpinLockAcquire(&lock);
+ SpinLockRelease(&lock);
+ SpinLockAcquire(&lock);
+ SpinLockRelease(&lock);
+ }
+ }
+#endif
+}
PG_FUNCTION_INFO_V1(test_atomic_ops);
Datum
@@ -805,6 +891,9 @@ test_atomic_ops(PG_FUNCTION_ARGS)
test_atomic_uint64();
+ /* XXX: Is there a better location for this? */
+ test_spinlock();
+
PG_RETURN_BOOL(true);
}
--
2.25.0.114.g5b0ca878e0
v1-0004-Fix-deadlock-danger-when-atomic-ops-are-done-unde.patch (text/x-diff, 7.1K) download
From 1d313fd848882c97651e3d7592e71496bbbcc339 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 8 Jun 2020 16:50:37 -0700
Subject: [PATCH v1 4/4] Fix deadlock danger when atomic ops are done under
spinlock.
This was a danger only for --disable-spinlocks in combination with
atomic operations unsupported by the current platform.
While atomics.c was careful to signal that a separate semaphore ought
to be used when spinlock emulation is active, spin.c didn't actually
implement that mechanism. That's my (Andres') fault, it seems to have
gotten lost during the development of the atomic operations support.
Fix that issue and add test for nesting atomic operations inside a
spinlock.
Author: Andres Freund
Discussion: https://postgr.es/m/20200605023302.g6v3ydozy5txifji@alap3.anarazel.de
Backpatch: 9.5-
---
src/backend/storage/lmgr/spin.c | 95 +++++++++++++++++++++++----------
src/test/regress/regress.c | 43 +++++++++++++++
2 files changed, 109 insertions(+), 29 deletions(-)
diff --git a/src/backend/storage/lmgr/spin.c b/src/backend/storage/lmgr/spin.c
index 753943e46d6..141606496eb 100644
--- a/src/backend/storage/lmgr/spin.c
+++ b/src/backend/storage/lmgr/spin.c
@@ -28,8 +28,24 @@
#ifndef HAVE_SPINLOCKS
+
+/*
+ * No TAS, so spinlocks are implemented as PGSemaphores.
+ */
+
+#ifndef HAVE_ATOMICS
+#define NUM_SIMULATION_SEMAPHORES (NUM_SPINLOCK_SEMAPHORES + NUM_ATOMICS_SEMAPHORES)
+#else
+#define NUM_SIMULATION_SEMAPHORES (NUM_SPINLOCK_SEMAPHORES)
+#endif /* DISABLE_ATOMICS */
+
PGSemaphore *SpinlockSemaArray;
-#endif
+
+#else /* !HAVE_SPINLOCKS */
+
+#define NUM_SIMULATION_SEMAPHORES 0
+
+#endif /* HAVE_SPINLOCKS */
/*
* Report the amount of shared memory needed to store semaphores for spinlock
@@ -38,34 +54,19 @@ PGSemaphore *SpinlockSemaArray;
Size
SpinlockSemaSize(void)
{
- return SpinlockSemas() * sizeof(PGSemaphore);
+ return NUM_SIMULATION_SEMAPHORES * sizeof(PGSemaphore);
}
-#ifdef HAVE_SPINLOCKS
-
/*
* Report number of semaphores needed to support spinlocks.
*/
int
SpinlockSemas(void)
{
- return 0;
+ return NUM_SIMULATION_SEMAPHORES;
}
-#else /* !HAVE_SPINLOCKS */
-/*
- * No TAS, so spinlocks are implemented as PGSemaphores.
- */
-
-
-/*
- * Report number of semaphores needed to support spinlocks.
- */
-int
-SpinlockSemas(void)
-{
- return NUM_SPINLOCK_SEMAPHORES + NUM_ATOMICS_SEMAPHORES;
-}
+#ifndef HAVE_SPINLOCKS
/*
* Initialize spinlock emulation.
@@ -92,23 +93,59 @@ SpinlockSemaInit(void)
/*
* s_lock.h hardware-spinlock emulation using semaphores
*
- * We map all spinlocks onto a set of NUM_SPINLOCK_SEMAPHORES semaphores.
- * It's okay to map multiple spinlocks onto one semaphore because no process
- * should ever hold more than one at a time. We just need enough semaphores
- * so that we aren't adding too much extra contention from that.
+ * We map all spinlocks onto NUM_SIMULATION_SEMAPHORES semaphores. It's okay to
+ * map multiple spinlocks onto one semaphore because no process should ever
+ * hold more than one at a time. We just need enough semaphores so that we
+ * aren't adding too much extra contention from that.
+ *
+ * There is one exception to the restriction of only holding one spinlock at a
+ * time, which is that it's ok if emulated atomic operations are nested inside
+ * spinlocks. To avoid the danger of spinlocks and atomic using the same sema,
+ * we make sure "normal" spinlocks and atomics backed by spinlocks use
+ * distinct semaphores (see the nested argument to s_init_lock_sema).
*
* slock_t is just an int for this implementation; it holds the spinlock
- * number from 1..NUM_SPINLOCK_SEMAPHORES. We intentionally ensure that 0
+ * number from 1..NUM_SIMULATION_SEMAPHORES. We intentionally ensure that 0
* is not a valid value, so that testing with this code can help find
* failures to initialize spinlocks.
*/
+static inline void
+s_check_valid(int lockndx)
+{
+ if (unlikely(lockndx <= 0 || lockndx > NUM_SIMULATION_SEMAPHORES))
+ elog(ERROR, "invalid spinlock number: %d", lockndx);
+}
+
void
s_init_lock_sema(volatile slock_t *lock, bool nested)
{
static uint32 counter = 0;
+ uint32 offset;
+ uint32 sema_total;
+ uint32 idx;
- *lock = ((++counter) % NUM_SPINLOCK_SEMAPHORES) + 1;
+ if (nested)
+ {
+ /*
+ * To allow nesting atomics inside spinlocked sections, use a
+ * different spinlock. See comment above.
+ */
+ offset = 1 + NUM_SPINLOCK_SEMAPHORES;
+ sema_total = NUM_ATOMICS_SEMAPHORES;
+ }
+ else
+ {
+ offset = 1;
+ sema_total = NUM_SPINLOCK_SEMAPHORES;
+ }
+
+ idx = (counter++ % sema_total) + offset;
+
+ /* double check we did things correctly */
+ s_check_valid(idx);
+
+ *lock = idx;
}
void
@@ -116,8 +153,8 @@ s_unlock_sema(volatile slock_t *lock)
{
int lockndx = *lock;
- if (lockndx <= 0 || lockndx > NUM_SPINLOCK_SEMAPHORES)
- elog(ERROR, "invalid spinlock number: %d", lockndx);
+ s_check_valid(lockndx);
+
PGSemaphoreUnlock(SpinlockSemaArray[lockndx - 1]);
}
@@ -134,8 +171,8 @@ tas_sema(volatile slock_t *lock)
{
int lockndx = *lock;
- if (lockndx <= 0 || lockndx > NUM_SPINLOCK_SEMAPHORES)
- elog(ERROR, "invalid spinlock number: %d", lockndx);
+ s_check_valid(lockndx);
+
/* Note that TAS macros return 0 if *success* */
return !PGSemaphoreTryLock(SpinlockSemaArray[lockndx - 1]);
}
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index a48f9de2532..231aab9d569 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -794,6 +794,47 @@ test_atomic_uint64(void)
EXPECT_EQ_U64(pg_atomic_fetch_and_u64(&var, ~0), 0);
}
+/*
+ * Verify that performing atomic ops inside a spinlock isn't a
+ * problem. Realistically that's only going to be a problem when both
+ * --disable-spinlocks and --disable-atomics are used, but it's cheap enough
+ * to just always test.
+ *
+ * The test works by initializing enough atomics that we'd conflict if there
+ * were an overlap between a spinlock and an atomic by holding a spinlock
+ * while manipulating more than NUM_SPINLOCK_SEMAPHORES atomics.
+ *
+ * NUM_TEST_ATOMICS doesn't really need to be more than
+ * NUM_SPINLOCK_SEMAPHORES, but it seems better to test a bit more
+ * extensively.
+ */
+static void
+test_atomic_spin_nest(void)
+{
+ slock_t lock;
+#define NUM_TEST_ATOMICS (NUM_SPINLOCK_SEMAPHORES + NUM_ATOMICS_SEMAPHORES + 27)
+ pg_atomic_uint32 atomics[NUM_TEST_ATOMICS];
+
+ SpinLockInit(&lock);
+
+ for (int i = 0; i < NUM_TEST_ATOMICS; i++)
+ pg_atomic_init_u32(&atomics[i], 0);
+
+ /* just so it's not all zeroes */
+ for (int i = 0; i < NUM_TEST_ATOMICS; i++)
+ EXPECT_EQ_U32(pg_atomic_fetch_add_u32(&atomics[i], i), 0);
+
+ /* test whether we can do atomic op with lock held */
+ SpinLockAcquire(&lock);
+ for (int i = 0; i < NUM_TEST_ATOMICS; i++)
+ {
+ EXPECT_EQ_U32(pg_atomic_fetch_sub_u32(&atomics[i], i), i);
+ EXPECT_EQ_U32(pg_atomic_read_u32(&atomics[i]), 0);
+ }
+ SpinLockRelease(&lock);
+}
+#undef NUM_TEST_ATOMICS
+
static void
test_spinlock(void)
{
@@ -891,6 +932,8 @@ test_atomic_ops(PG_FUNCTION_ARGS)
test_atomic_uint64();
+ test_atomic_spin_nest();
+
/* XXX: Is there a better location for this? */
test_spinlock();
--
2.25.0.114.g5b0ca878e0
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:55 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 19:07 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-05 02:33 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-06 00:19 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
@ 2020-06-10 11:26 ` Robert Haas <robertmhaas@gmail.com>
2020-06-10 13:51 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-11 17:26 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2 siblings, 2 replies; 22+ messages in thread
From: Robert Haas @ 2020-06-10 11:26 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Fri, Jun 5, 2020 at 8:19 PM Andres Freund <andres@anarazel.de> wrote:
> Randomly noticed while looking at the code:
> uint64 flagbit = UINT64CONST(1) << (uint64) type;
>
> that shouldn't be 64bit, right?
I'm going to admit ignorance here. What's the proper coding rule?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:55 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 19:07 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-05 02:33 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-06 00:19 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-10 11:26 ` Re: Atomic operations within spinlocks Robert Haas <robertmhaas@gmail.com>
@ 2020-06-10 13:51 ` Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 22+ messages in thread
From: Tom Lane @ 2020-06-10 13:51 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Jun 5, 2020 at 8:19 PM Andres Freund <andres@anarazel.de> wrote:
>> Randomly noticed while looking at the code:
>> uint64 flagbit = UINT64CONST(1) << (uint64) type;
>>
>> that shouldn't be 64bit, right?
> I'm going to admit ignorance here. What's the proper coding rule?
The shift distance can't exceed 64, so there's no need for it to be
wider than int. "type" is an enum, so explicitly casting it to an
integral type seems like good practice, but int is sufficient.
ISTR older compilers insisting that the shift distance not be
wider than int. But C99 doesn't seem to require that -- it only
restricts the value of the right operand.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:55 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 19:07 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-05 02:33 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-06 00:19 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-10 11:26 ` Re: Atomic operations within spinlocks Robert Haas <robertmhaas@gmail.com>
@ 2020-06-11 17:26 ` Andres Freund <andres@anarazel.de>
2020-06-11 19:50 ` Re: Atomic operations within spinlocks Robert Haas <robertmhaas@gmail.com>
1 sibling, 1 reply; 22+ messages in thread
From: Andres Freund @ 2020-06-11 17:26 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 2020-06-10 07:26:32 -0400, Robert Haas wrote:
> On Fri, Jun 5, 2020 at 8:19 PM Andres Freund <andres@anarazel.de> wrote:
> > Randomly noticed while looking at the code:
> > uint64 flagbit = UINT64CONST(1) << (uint64) type;
> >
> > that shouldn't be 64bit, right?
>
> I'm going to admit ignorance here. What's the proper coding rule?
Well, pss_barrierCheckMask member is just 32bit, so it seems odd to
declare the local variable 64bit?
uint64 flagbit = UINT64CONST(1) << (uint64) type;
...
pg_atomic_fetch_or_u32(&slot->pss_barrierCheckMask, flagbit);
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Atomic operations within spinlocks
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 17:57 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:55 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-04 19:07 ` Re: Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-05 02:33 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-06 00:19 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
2020-06-10 11:26 ` Re: Atomic operations within spinlocks Robert Haas <robertmhaas@gmail.com>
2020-06-11 17:26 ` Re: Atomic operations within spinlocks Andres Freund <andres@anarazel.de>
@ 2020-06-11 19:50 ` Robert Haas <robertmhaas@gmail.com>
0 siblings, 0 replies; 22+ messages in thread
From: Robert Haas @ 2020-06-11 19:50 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Jun 11, 2020 at 1:26 PM Andres Freund <andres@anarazel.de> wrote:
> Well, pss_barrierCheckMask member is just 32bit, so it seems odd to
> declare the local variable 64bit?
>
> uint64 flagbit = UINT64CONST(1) << (uint64) type;
> ...
> pg_atomic_fetch_or_u32(&slot->pss_barrierCheckMask, flagbit);
Oooooops.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 22+ messages in thread
end of thread, other threads:[~2020-06-11 19:50 UTC | newest]
Thread overview: 22+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-06-03 18:19 Atomic operations within spinlocks Tom Lane <tgl@sss.pgh.pa.us>
2020-06-03 20:45 ` Andres Freund <andres@anarazel.de>
2020-06-03 21:40 ` Thomas Munro <thomas.munro@gmail.com>
2020-06-04 07:03 ` Michael Paquier <michael@paquier.xyz>
2020-06-04 17:57 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 18:50 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-06-04 19:06 ` Andres Freund <andres@anarazel.de>
2020-06-04 19:13 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-06-05 03:03 ` Andres Freund <andres@anarazel.de>
2020-06-04 18:55 ` Andres Freund <andres@anarazel.de>
2020-06-04 19:07 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-06-05 02:33 ` Andres Freund <andres@anarazel.de>
2020-06-06 00:19 ` Andres Freund <andres@anarazel.de>
2020-06-06 01:01 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-06-06 02:31 ` Andres Freund <andres@anarazel.de>
2020-06-06 02:52 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-06-06 03:32 ` Andres Freund <andres@anarazel.de>
2020-06-09 06:08 ` Andres Freund <andres@anarazel.de>
2020-06-10 11:26 ` Robert Haas <robertmhaas@gmail.com>
2020-06-10 13:51 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-06-11 17:26 ` Andres Freund <andres@anarazel.de>
2020-06-11 19:50 ` Robert Haas <robertmhaas@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox