[PATCH] Native spinlock support on RISC-V
Hello,
The attached patch adds native spinlock support to PostgreSQL on RISC-V
systems. As suspected by Richard W.M. Jones of Red Hat back in 2016, the
__sync_lock_test_and_set() approach applied on arm and arm64 works here
as well.
Tested against PostgreSQL 13.3 on a physical rv64gc system (BeagleV
Starlight beta board) - builds and installs fine, all tests pass. From
what I can see in gcc documentation this should in theory work on rv32
(and possibly rv128) as well, therefore the patch as it stands covers
all RISC-V systems (i.e. doesn't check the value of __risc_xlen) - but I
haven't confirmed this experimentally.
--
MS
Attachments:
postgresql-riscv-spinlocks.patchtext/x-patch; charset=UTF-8; name=postgresql-riscv-spinlocks.patchDownload
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -315,12 +315,12 @@
#endif /* __ia64__ || __ia64 */
/*
- * On ARM and ARM64, we use __sync_lock_test_and_set(int *, int) if available.
+ * On ARM, ARM64 and RISC-V, we use __sync_lock_test_and_set(int *, int) if available.
*
* We use the int-width variant of the builtin because it works on more chips
* than other widths.
*/
-#if defined(__arm__) || defined(__arm) || defined(__aarch64__) || defined(__aarch64)
+#if defined(__arm__) || defined(__arm) || defined(__aarch64__) || defined(__aarch64) || defined(__riscv)
#ifdef HAVE_GCC__SYNC_INT32_TAS
#define HAS_TEST_AND_SET
@@ -337,7 +337,7 @@
#define S_UNLOCK(lock) __sync_lock_release(lock)
#endif /* HAVE_GCC__SYNC_INT32_TAS */
-#endif /* __arm__ || __arm || __aarch64__ || __aarch64 */
+#endif /* __arm__ || __arm || __aarch64__ || __aarch64 || __riscv */
/* S/390 and S/390x Linux (32- and 64-bit zSeries) */
Marek Szuba <marecki@gentoo.org> writes:
Tested against PostgreSQL 13.3 on a physical rv64gc system (BeagleV
Starlight beta board) - builds and installs fine, all tests pass.
Cool ... I had hoped to acquire one of those myself, but I didn't
make the cut.
regards, tom lane
Hi,
On 2021-08-13 11:09:04 -0400, Tom Lane wrote:
Marek Szuba <marecki@gentoo.org> writes:
Tested against PostgreSQL 13.3 on a physical rv64gc system (BeagleV
Starlight beta board) - builds and installs fine, all tests pass.
Seems like a good idea to me.
Cool ... I had hoped to acquire one of those myself, but I didn't
make the cut.
Should we backpatch this? It's not like we're going to break existing
risc-v systems by enabling spinlock support...
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2021-08-13 11:09:04 -0400, Tom Lane wrote:
Marek Szuba <marecki@gentoo.org> writes:
Tested against PostgreSQL 13.3 on a physical rv64gc system (BeagleV
Starlight beta board) - builds and installs fine, all tests pass.
Should we backpatch this? It's not like we're going to break existing
risc-v systems by enabling spinlock support...
Yeah, why not? If you were building with --disable-spinlocks before,
this shouldn't change anything for you.
(I haven't actually looked at the patch, mind you, but in principle
it shouldn't break anything that worked before.)
regards, tom lane
I wrote:
Andres Freund <andres@anarazel.de> writes:
Should we backpatch this? It's not like we're going to break existing
risc-v systems by enabling spinlock support...
Yeah, why not? If you were building with --disable-spinlocks before,
this shouldn't change anything for you.
(I haven't actually looked at the patch, mind you, but in principle
it shouldn't break anything that worked before.)
I now have looked at the patch, and it seems good as far as it goes,
but I wonder whether some effort ought to be expended in
src/include/port/atomics/.
regards, tom lane
Hi,
On Fri, Aug 13, 2021, at 19:25, Tom Lane wrote:
I wrote:
Andres Freund <andres@anarazel.de> writes:
Should we backpatch this? It's not like we're going to break existing
risc-v systems by enabling spinlock support...Yeah, why not? If you were building with --disable-spinlocks before,
this shouldn't change anything for you.
(I haven't actually looked at the patch, mind you, but in principle
it shouldn't break anything that worked before.)I now have looked at the patch, and it seems good as far as it goes,
but I wonder whether some effort ought to be expended in
src/include/port/atomics/.
That should automatically pick up the intrinsic. I think we should do the same on modern compilers for spinlocks, but that's a separate discussion I guess.
Address
"Andres Freund" <andres@anarazel.de> writes:
On Fri, Aug 13, 2021, at 19:25, Tom Lane wrote:
I now have looked at the patch, and it seems good as far as it goes,
but I wonder whether some effort ought to be expended in
src/include/port/atomics/.
That should automatically pick up the intrinsic. I think we should do the same on modern compilers for spinlocks, but that's a separate discussion I guess.
I was looking at the comment in atomics.h about
* Provide a full fallback of the pg_*_barrier(), pg_atomic**_flag and
* pg_atomic_* APIs for platforms without sufficient spinlock and/or atomics
* support. In the case of spinlock backed atomics the emulation is expected
* to be efficient, although less so than native atomics support.
so it seems like someday we might want to expend some effort on native
atomics. I agree that that day need not be today, though. This patch
seems sufficient until we get to the point of (at least) having some
RISC-V in the buildfarm.
regards, tom lane
Hi,
On 2021-08-13 13:37:02 -0400, Tom Lane wrote:
"Andres Freund" <andres@anarazel.de> writes:
On Fri, Aug 13, 2021, at 19:25, Tom Lane wrote:
I now have looked at the patch, and it seems good as far as it goes,
but I wonder whether some effort ought to be expended in
src/include/port/atomics/.That should automatically pick up the intrinsic. I think we should do the same on modern compilers for spinlocks, but that's a separate discussion I guess.
I was looking at the comment in atomics.h about
* Provide a full fallback of the pg_*_barrier(), pg_atomic**_flag and
* pg_atomic_* APIs for platforms without sufficient spinlock and/or atomics
* support. In the case of spinlock backed atomics the emulation is expected
* to be efficient, although less so than native atomics support.so it seems like someday we might want to expend some effort on native
atomics. I agree that that day need not be today, though. This patch
seems sufficient until we get to the point of (at least) having some
RISC-V in the buildfarm.
For gcc compatible compilers the relevant comments would be
* There exist generic, hardware independent, implementations for several
* compilers which might be sufficient, although possibly not optimal, for a
* new platform. If no such generic implementation is available spinlocks (or
* even OS provided semaphores) will be used to implement the API.
and
/*
* Compiler specific, but architecture independent implementations.
*
* Provide architecture independent implementations of the atomic
* facilities. At the very least compiler barriers should be provided, but a
* full implementation of
* * pg_compiler_barrier(), pg_write_barrier(), pg_read_barrier()
* * pg_atomic_compare_exchange_u32(), pg_atomic_fetch_add_u32()
* using compiler intrinsics are a good idea.
*/
Gcc's intriniscs are pretty good these days (and if not, a *lot* of
projects just outright break).
What's more, It turns out that using intrinsics with compilers of the
last ~5 years often generates *better* code than inline assembly
(e.g. because the compiler can utilize condition codes more effectively
and has more detailed information about clobbers). So for new platforms
that'll only have support by new compilers it doesn't really make sense
to add inline assembler imo.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2021-08-13 13:37:02 -0400, Tom Lane wrote:
so it seems like someday we might want to expend some effort on native
atomics. I agree that that day need not be today, though. This patch
seems sufficient until we get to the point of (at least) having some
RISC-V in the buildfarm.
Gcc's intriniscs are pretty good these days (and if not, a *lot* of
projects just outright break).
What's more, It turns out that using intrinsics with compilers of the
last ~5 years often generates *better* code than inline assembly
(e.g. because the compiler can utilize condition codes more effectively
and has more detailed information about clobbers). So for new platforms
that'll only have support by new compilers it doesn't really make sense
to add inline assembler imo.
I didn't say it had to be __asm blocks ;-). I was thinking more of the
sort of stuff we have in e.g. arch-arm.h and arch-ia64.h, where we know
some things about what is efficient or less efficient on a particular
architecture. gcc will do its best to provide implementations of
its builtins, but that doesn't mean that using a particular one of
them is always the most sane thing to do.
But anyway, that seems like minor optimization that maybe someday
somebody will be motivated to do. We're on the same page about this
being enough for today.
I did not like confusing the RISC-V case with the ARM case: duplicating
the code block seems better, in case there's ever reason to add
arch-specific stuff like SPIN_DELAY. So I split it off to its own
code block and pushed it.
regards, tom lane
On 13 Aug 2021, at 20:09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
..I split it off to its own code block and pushed it.
There didn’t seem to be anything left here (at least until there is hardware in
the buildfarm) so I took the liberty to close it as committed in the CF app.
--
Daniel Gustafsson https://vmware.com/
Daniel Gustafsson <daniel@yesql.se> writes:
There didn’t seem to be anything left here (at least until there is hardware in
the buildfarm) so I took the liberty to close it as committed in the CF app.
Ah, sorry, I did not realize there was a CF entry.
regards, tom lane
Re: Tom Lane
I did not like confusing the RISC-V case with the ARM case: duplicating
the code block seems better, in case there's ever reason to add
arch-specific stuff like SPIN_DELAY. So I split it off to its own
code block and pushed it.
Fwiw I can report success on Debian's riscv port with that commit
cherry-picked onto 13.4:
Christoph
On Wed, Sep 1, 2021 at 9:22 PM Christoph Berg <myon@debian.org> wrote:
Re: Tom Lane
I did not like confusing the RISC-V case with the ARM case: duplicating
the code block seems better, in case there's ever reason to add
arch-specific stuff like SPIN_DELAY. So I split it off to its own
code block and pushed it.Fwiw I can report success on Debian's riscv port with that commit
cherry-picked onto 13.4:
A couple of things I noticed on this architecture:
1. Even though we're using generic built-ins for atomics, I guess we
could still use a src/include/port/atomics/arch-riscv.h file so we
have a place to define PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY when
building for 64 bit. We'd need to find the chapter-and-verse to
justify that, of course, which I can try to do; if someone more
familiar with the ISA/spec/manual can point to it I'm all ears.
2. When configured with all options on FreeBSD 13, everything looks
good so far except LLVM JIT, which fails with various "Cannot select"
errors. Clang works fine for compiling PostgreSQL itself. Tested
with LLVM 12 (LLVM has supported RISCV since 9). Example:
+FATAL: fatal llvm error: Cannot select: 0x4f772068: ch = brcond
0x4f770f70, 0x4f772208, BasicBlock:ch< 0x4f76d600>
+ 0x4f772208: i64 = setcc 0x4f7723a8, Constant:i64<0>, setlt:ch
+ 0x4f7723a8: i64,ch = load<(load 4 from `i32* inttoptr (i64
1260491408 to i32*)`, align 16), sext from i32> 0x4fdee058,
Constant:i64<1260491408>, undef:i64
+ 0x4f770a90: i64 = Constant<1260491408>
+ 0x4f7703a8: i64 = undef
+ 0x4f7701a0: i64 = Constant<0>
+In function: evalexpr_0_0
Hi,
On November 2, 2021 3:55:58 PM PDT, Thomas Munro <thomas.munro@gmail.com> wrote:
2. When configured with all options on FreeBSD 13, everything looks
good so far except LLVM JIT, which fails with various "Cannot select"
errors. Clang works fine for compiling PostgreSQL itself. Tested
with LLVM 12 (LLVM has supported RISCV since 9). Example:+FATAL: fatal llvm error: Cannot select: 0x4f772068: ch = brcond 0x4f770f70, 0x4f772208, BasicBlock:ch< 0x4f76d600> + 0x4f772208: i64 = setcc 0x4f7723a8, Constant:i64<0>, setlt:ch + 0x4f7723a8: i64,ch = load<(load 4 from `i32* inttoptr (i64 1260491408 to i32*)`, align 16), sext from i32> 0x4fdee058, Constant:i64<1260491408>, undef:i64 + 0x4f770a90: i64 = Constant<1260491408> + 0x4f7703a8: i64 = undef + 0x4f7701a0: i64 = Constant<0> +In function: evalexpr_0_0
Any chance you could enable jit_dump_bitcode and manually try a failing query? That should dump. bc files in the data directory. That'd might allow debugging this outside the emulated environment.
I don't see where the undef is originating from, but I think it might suggest that something lead to that code being considered unreachable.
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Wed, Nov 3, 2021 at 5:13 PM Andres Freund <andres@anarazel.de> wrote:
Any chance you could enable jit_dump_bitcode and manually try a failing query? That should dump. bc files in the data directory. That'd might allow debugging this outside the emulated environment.
I don't see where the undef is originating from, but I think it might suggest that something lead to that code being considered unreachable.
postgres=# set jit_above_cost = 0;
SET
postgres=# select 1 + 1;
FATAL: fatal llvm error: Cannot select: 0x4b3ec1a0: i64,ch =
load<(load 8 from %ir.14)> 0x41ef6fe8, 0x4b3ec138, undef:i64
0x4b3ec138: i64 = add nuw 0x4b3eab60, Constant:i64<24>
0x4b3eab60: i64,ch = load<(load 8 from %ir.7)> 0x41ef6fe8,
0x4b3eaaf8, undef:i64
0x4b3eaaf8: i64 = add nuw 0x4b3ea068, Constant:i64<16>
0x4b3ea068: i64,ch = CopyFromReg 0x41ef6fe8, Register:i64 %4
0x4b3ea000: i64 = Register %4
0x4b3ea888: i64 = Constant<16>
0x4b3ea6e8: i64 = undef
0x4b3ea9c0: i64 = Constant<24>
0x4b3ea6e8: i64 = undef
In function: evalexpr_0_0
Ah, I hadn't noticed this in the log before:
'generic' is not a recognized processor for this target (ignoring processor)
Sounds kinda serious :-)
Resulting .bc files and .ll files (produced by llvm-dis) attached.
Attachments:
75392.0.bcapplication/octet-stream; name=75392.0.bcDownload
BC��5 b0$JY�fm��_Q�L ! ' ! �#�A�I29��%�b� EB�B28K
2��Hp�!#D��A�d�� CF� �2��*(*�1|�\� �� � 2"
d�$��$����L���M��(���2G � S
���@� (P��2P�I�Q��o�#(��������)���'�I����&