Обсуждение: BUG #15844: MIPS: remove .set mips2 in s_lock.h to fix r6 build
The following bug has been logged on the website:
Bug reference: 15844
Logged by: Yunqiang Su
Email address: wzssyqa@gmail.com
PostgreSQL version: 12beta1
Operating system: Linux
Description:
MIPS r6 changes the encoding of LL/SC instruction,
while the .set mips2 will force assembler to generate
old encoding.
This patch can fix this problem.
In fact if we not willing to support MIPS I or any CPU without ll/sc
at all, we can just remove .set mips2 here.
Index: postgresql-11-11.2/src/include/storage/s_lock.h
===================================================================
--- postgresql-11-11.2.orig/src/include/storage/s_lock.h
+++ postgresql-11-11.2/src/include/storage/s_lock.h
@@ -606,6 +606,13 @@ typedef unsigned int slock_t;
#define TAS(lock) tas(lock)
+
+#if __mips_isa_rev >= 6
+# define MIPS_SET_VER " \n"
+#else
+# define MIPS_SET_VER ".set mips2 \n"
+#endif
+
static __inline__ int
tas(volatile slock_t *lock)
{
@@ -615,7 +622,7 @@ tas(volatile slock_t *lock)
__asm__ __volatile__(
" .set push \n"
- " .set mips2 \n"
+ MIPS_SET_VER
" .set noreorder \n"
" .set nomacro \n"
" ll %0, %2 \n"
@@ -637,7 +644,7 @@ do \
{ \
__asm__ __volatile__( \
" .set push \n" \
- " .set mips2 \n" \
+ MIPS_SET_VER \
" .set noreorder \n" \
" .set nomacro \n" \
" sync \n" \
On Tue, Jun 11, 2019 at 04:39:35AM +0000, PG Bug reporting form wrote: > MIPS r6 changes the encoding of LL/SC instruction, > while the .set mips2 will force assembler to generate > old encoding. This is quite architecture-specific. May I suggest to add this patch to the follow-up commit fest for awareness: https://commitfest.postgresql.org/23/ Do you have any references, like documentation, which summarize that? -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> 于2019年6月11日周二 下午12:57写道： > > On Tue, Jun 11, 2019 at 04:39:35AM +0000, PG Bug reporting form wrote: > > MIPS r6 changes the encoding of LL/SC instruction, > > while the .set mips2 will force assembler to generate > > old encoding. > > This is quite architecture-specific. May I suggest to add this patch > to the follow-up commit fest for awareness: > https://commitfest.postgresql.org/23/ > > Do you have any references, like documentation, which summarize that? https://s3-eu-west-1.amazonaws.com/downloads-mips/documents/MD00087-2B-MIPS64BIS-AFP-6.06.pdf The page: 307 > -- > Michael -- YunQiang Su
PG Bug reporting form <noreply@postgresql.org> writes:
> MIPS r6 changes the encoding of LL/SC instruction,
> while the .set mips2 will force assembler to generate
> old encoding.
> This patch can fix this problem.
So I have a bunch of questions about this patch.
1. What problem is this actually solving? We have what I think are
reasonably modern MIPS machines in the buildfarm (frogfish,
topminnow) and they aren't complaining.
2. What toolchains define __mips_isa_rev at all? Seems like we could
be trading one portability issue for another one.
3. If we actually need something here, don't we need a run-time probe
rather than a compile-time check? Distro packagers, for example,
can't assume that their packages will be run on exactly the hardware
they build on.
regards, tom lane
PG Bug reporting form <noreply@postgresql.org> writes:
> MIPS r6 changes the encoding of LL/SC instruction,
> while the .set mips2 will force assembler to generate
> old encoding.
> ...
> In fact if we not willing to support MIPS I or any CPU without ll/sc
> at all, we can just remove .set mips2 here.
After further digging around, I'm liking the alternative of just
removing the ".set mips2" lines. MIPS-I has been obsolete since 1989,
and the MIPS-II instruction set has a lot of other substantial advantages
over MIPS-I besides having LL/SC, so it's pretty hard to believe that
anyone is still using toolchains that default to assuming MIPS-I
instruction set.
Digging around in our archives, it looks like ".set mips2" was required
when it was added, but that was on the strength of testing with a
machine running Linux 2.4.27-r5k-cobalt. We need to research when/if
Linux changed their default configuration.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月13日周四 下午10:07写道： > > PG Bug reporting form <noreply@postgresql.org> writes: > > MIPS r6 changes the encoding of LL/SC instruction, > > while the .set mips2 will force assembler to generate > > old encoding. > > This patch can fix this problem. > > So I have a bunch of questions about this patch. > > 1. What problem is this actually solving? We have what I think are > reasonably modern MIPS machines in the buildfarm (frogfish, > topminnow) and they aren't complaining. > Yes. The MIPS r6 is quite new, and the machine that you have should be pre-r6. > 2. What toolchains define __mips_isa_rev at all? Seems like we could > be trading one portability issue for another one. > It is defined by gcc/llvm. For gcc: https://github.com/gcc-mirror/gcc/blob/41d6b10e96a1de98e90a7c0378437c3255814b16/gcc/config/mips/mips.h#L527 > 3. If we actually need something here, don't we need a run-time probe > rather than a compile-time check? Distro packagers, for example, > can't assume that their packages will be run on exactly the hardware > they build on. We don't think run-time probe is a good idea, since we treat r6 as a new architectures. It has it own port of OS, like Debian. Runtime probe make none sense here. > > regards, tom lane -- YunQiang Su
Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月14日周五 上午3:53写道： > > PG Bug reporting form <noreply@postgresql.org> writes: > > MIPS r6 changes the encoding of LL/SC instruction, > > while the .set mips2 will force assembler to generate > > old encoding. > > ... > > In fact if we not willing to support MIPS I or any CPU without ll/sc > > at all, we can just remove .set mips2 here. > > After further digging around, I'm liking the alternative of just > removing the ".set mips2" lines. MIPS-I has been obsolete since 1989, > and the MIPS-II instruction set has a lot of other substantial advantages > over MIPS-I besides having LL/SC, so it's pretty hard to believe that > anyone is still using toolchains that default to assuming MIPS-I > instruction set. > You are right. I have no idea anyone is using MIPS I. > Digging around in our archives, it looks like ".set mips2" was required > when it was added, but that was on the strength of testing with a > machine running Linux 2.4.27-r5k-cobalt. We need to research when/if > Linux changed their default configuration. It depends on the options to `as'. If we build `as' without any option, and call it directly, it still generate MIPS I binary now. While, as I know that we are using gcc to call `as', it won't be a problem anymore. Since no body set the default target of gcc as MIPS I nowdays. > > regards, tom lane -- YunQiang Su
YunQiang Su <wzssyqa@gmail.com> writes:
> Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月14日周五 上午3:53写道：
>> After further digging around, I'm liking the alternative of just
>> removing the ".set mips2" lines. MIPS-I has been obsolete since 1989,
>> and the MIPS-II instruction set has a lot of other substantial advantages
>> over MIPS-I besides having LL/SC, so it's pretty hard to believe that
>> anyone is still using toolchains that default to assuming MIPS-I
>> instruction set.
> You are right. I have no idea anyone is using MIPS I.
After further looking, it seems that isn't going to fly. I found from
the Debian release notes that they dropped MIPS-I support as of Stretch,
which means removing ".set mips2" would break both of our live MIPS
buildfarm machines (which run wheezy and jessie). I also found by
experimentation that NetBSD as of 7.0.2 doesn't default to assuming
MIPS2 either. (I didn't try anything newer, but a scan through the
port-mips mailing list found no suggestion that they've changed the
default since then.) So even though the hardware in use nowadays might
be fine with this, the toolchains are still behind the times.
So we'll have to go with the #if solution, I think. But I dislike
hardwiring "#if __mips_isa_rev >= 6" into s_lock.h. I'd suggest
modeling this hack on our rather-ancient hacks for similar problems
with PPC: put something like this into pg_config_manual.h
#if __mips_isa_rev >= 6
#define FORCE_MIPS2_ARCHITECTURE
#endif
(with a suitable comment) and then make s_lock.h do
#ifdef FORCE_MIPS2_ARCHITECTURE
" .set mips2 \n"
#endif
That'll make it a lot easier for people to tweak the condition
if they need to.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月16日周日 上午11:32写道： > > YunQiang Su <wzssyqa@gmail.com> writes: > > Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月14日周五 上午3:53写道： > >> After further digging around, I'm liking the alternative of just > >> removing the ".set mips2" lines. MIPS-I has been obsolete since 1989, > >> and the MIPS-II instruction set has a lot of other substantial advantages > >> over MIPS-I besides having LL/SC, so it's pretty hard to believe that > >> anyone is still using toolchains that default to assuming MIPS-I > >> instruction set. > > > You are right. I have no idea anyone is using MIPS I. > > After further looking, it seems that isn't going to fly. I found from > the Debian release notes that they dropped MIPS-I support as of Stretch, > which means removing ".set mips2" would break both of our live MIPS > buildfarm machines (which run wheezy and jessie). I also found by The default of wheezy and jessie is MIPS II. https://metadata.ftp-master.debian.org/changelogs//main/g/gcc-4.8/gcc-4.8_4.8.4-1_changelog Debian use MIPS II as default since gcc 4.5, (may be Squeeze) > experimentation that NetBSD as of 7.0.2 doesn't default to assuming I have no idea about NetBSD. Can you run gcc -v on it? > MIPS2 either. (I didn't try anything newer, but a scan through the > port-mips mailing list found no suggestion that they've changed the > default since then.) So even though the hardware in use nowadays might > be fine with this, the toolchains are still behind the times. > > So we'll have to go with the #if solution, I think. But I dislike > hardwiring "#if __mips_isa_rev >= 6" into s_lock.h. I'd suggest > modeling this hack on our rather-ancient hacks for similar problems > with PPC: put something like this into pg_config_manual.h > > #if __mips_isa_rev >= 6 > #define FORCE_MIPS2_ARCHITECTURE > #endif > If you'd like to do like this: #if __mips_isa_rev > 0 is enough. > (with a suitable comment) and then make s_lock.h do > > #ifdef FORCE_MIPS2_ARCHITECTURE > " .set mips2 \n" > #endif > > That'll make it a lot easier for people to tweak the condition > if they need to. > > regards, tom lane -- YunQiang Su
YunQiang Su <wzssyqa@gmail.com> writes: > Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月16日周日 上午11:32写道： >> experimentation that NetBSD as of 7.0.2 doesn't default to assuming > I have no idea about NetBSD. Can you run gcc -v on it? $ gcc -v Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/libexec/lto-wrapper Target: mipsel--netbsd Configured with: /usr/7/src/tools/gcc/../../external/gpl3/gcc/dist/configure --target=mipsel--netbsd --enable-long-long --enable-threads--with-bugurl=http://www.NetBSD.org/Misc/send-pr.html --with-pkgversion='NetBSD nb2 20150115' --with-system-zlib--enable-__cxa_atexit --enable-libstdcxx-threads --enable-libstdcxx-time=rt --enable-lto --with-mpc-lib=/var/obj/mknative/pmax-mipsel/usr/7/src/external/lgpl3/mpc/lib/libmpc --with-mpfr-lib=/var/obj/mknative/pmax-mipsel/usr/7/src/external/lgpl3/mpfr/lib/libmpfr --with-gmp-lib=/var/obj/mknative/pmax-mipsel/usr/7/src/external/lgpl3/gmp/lib/libgmp --with-mpc-include=/usr/7/src/external/lgpl3/mpc/dist/src--with-mpfr-include=/usr/7/src/external/lgpl3/mpfr/dist/src --with-gmp-include=/usr/7/src/external/lgpl3/gmp/lib/libgmp/arch/mipsel--enable-tls --disable-multilib --disable-symvers--disable-libstdcxx-pch --build=x86_64-unknown-netbsd6.0. --host=mipsel--netbsd --with-sysroot=/var/obj/mknative/pmax-mipsel/usr/7/src/destdir.pmax Thread model: posix gcc version 4.8.4 (nb2 20150115) I found that specifying -march=mips2 gets it to accept the s_lock.h code without ".set mips2". Given that we don't make any pretense of actually running on MIPS-I hardware, I wonder if some hack involving forcing -march would be sane? You'd get better code quality across the board, presumably. Also, checking the predefined symbols on this gcc, I don't see anything about __mips_isa_rev, but I do see that "__mips" is defined and -march=mips2 changes it from "1" to "2". So I'm wondering about some test along the lines of #if __mips_isa_rev > 0 || __mips == 1 #define FORCE_MIPS2_ARCHITECTURE #endif or alternatively, teach configure to force -march=mips2 if it sees that "__mips" is predefined as 1. (BTW, have you got any recommendations for booting recent Debian/MIPS under qemu? I can't get anything newer than wheezy to work.) regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月16日周日 下午12:28写道： > > YunQiang Su <wzssyqa@gmail.com> writes: > > Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月16日周日 上午11:32写道： > >> experimentation that NetBSD as of 7.0.2 doesn't default to assuming > > > I have no idea about NetBSD. Can you run gcc -v on it? > > $ gcc -v > Using built-in specs. > COLLECT_GCC=gcc > COLLECT_LTO_WRAPPER=/usr/libexec/lto-wrapper > Target: mipsel--netbsd > Configured with: /usr/7/src/tools/gcc/../../external/gpl3/gcc/dist/configure --target=mipsel--netbsd --enable-long-long--enable-threads --with-bugurl=http://www.NetBSD.org/Misc/send-pr.html --with-pkgversion='NetBSD nb2 20150115'--with-system-zlib --enable-__cxa_atexit --enable-libstdcxx-threads --enable-libstdcxx-time=rt --enable-lto --with-mpc-lib=/var/obj/mknative/pmax-mipsel/usr/7/src/external/lgpl3/mpc/lib/libmpc --with-mpfr-lib=/var/obj/mknative/pmax-mipsel/usr/7/src/external/lgpl3/mpfr/lib/libmpfr --with-gmp-lib=/var/obj/mknative/pmax-mipsel/usr/7/src/external/lgpl3/gmp/lib/libgmp --with-mpc-include=/usr/7/src/external/lgpl3/mpc/dist/src--with-mpfr-include=/usr/7/src/external/lgpl3/mpfr/dist/src --with-gmp-include=/usr/7/src/external/lgpl3/gmp/lib/libgmp/arch/mipsel--enable-tls --disable-multilib --disable-symvers--disable-libstdcxx-pch --build=x86_64-unknown-netbsd6.0. --host=mipsel--netbsd --with-sysroot=/var/obj/mknative/pmax-mipsel/usr/7/src/destdir.pmax > Thread model: posix > gcc version 4.8.4 (nb2 20150115) > > I found that specifying -march=mips2 gets it to accept the s_lock.h > code without ".set mips2". Given that we don't make any pretense of > actually running on MIPS-I hardware, I wonder if some hack involving > forcing -march would be sane? You'd get better code quality across > the board, presumably. > -march is not a good idea, since r6 cannot compatible with the previous version. If you use -march=mips2 or something else, it will failed to build for r6. Which -march to use should be determined by finial user or distribution vendor. If the vendor of NetBSD believe they have no need to support so old hardware, they should change the options that they build their gcc. For example build gcc with "--with-arch-32=mips32r2" or similar. > Also, checking the predefined symbols on this gcc, I don't see > anything about __mips_isa_rev, but I do see that "__mips" is defined > and -march=mips2 changes it from "1" to "2". So I'm wondering about > some test along the lines of > > #if __mips_isa_rev > 0 || __mips == 1 > #define FORCE_MIPS2_ARCHITECTURE > #endif > > or alternatively, teach configure to force -march=mips2 if it sees > that "__mips" is predefined as 1. > If you wish to use __mips, you can just use #if __mips > 1 > (BTW, have you got any recommendations for booting recent Debian/MIPS > under qemu? I can't get anything newer than wheezy to work.) I have an r6 one: http://mips64el.bfsu.edu.cn/debian-new/tarball/qemu/ It is buster. And I will figure out a r2 one for you. > > regards, tom lane -- YunQiang Su
YunQiang Su <wzssyqa@gmail.com> writes: > Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月16日周日 下午12:28写道： >> I found that specifying -march=mips2 gets it to accept the s_lock.h >> code without ".set mips2". Given that we don't make any pretense of >> actually running on MIPS-I hardware, I wonder if some hack involving >> forcing -march would be sane? You'd get better code quality across >> the board, presumably. > -march is not a good idea, since r6 cannot compatible with the previous version. > If you use -march=mips2 or something else, it will failed to build for r6. > Which -march to use should be determined by finial user or distribution vendor. Sure, the trick would be to not override a (default or specified) architecture setting that's higher than mips2. I was imagining using AC_EGREP_CPP() to see if __mips expands to exactly 1. But on reflection, probably a better idea is to just see if asm with ll/sc/sync compiles, and add -march=mips2 if not. (We should not do anything if --disable-spinlocks has been specified, btw, since then we don't need those asm commands to work. This is and would remain the workaround for building PG for MIPS-I, if some benighted soul insists on doing that.) >> (BTW, have you got any recommendations for booting recent Debian/MIPS >> under qemu? I can't get anything newer than wheezy to work.) > I have an r6 one: > http://mips64el.bfsu.edu.cn/debian-new/tarball/qemu/ > It is buster. Thanks for the pointer. regards, tom lane
I wrote:
> Sure, the trick would be to not override a (default or specified)
> architecture setting that's higher than mips2. I was imagining
> using AC_EGREP_CPP() to see if __mips expands to exactly 1. But
> on reflection, probably a better idea is to just see if asm with
> ll/sc/sync compiles, and add -march=mips2 if not.
Concretely, here's a patch that does it like that. I've verified
that this builds on my netbsd/gxemul installation.
regards, tom lane
diff --git a/configure b/configure
index fd61bf6..57ef478 100755
--- a/configure
+++ b/configure
@@ -6900,6 +6900,57 @@ CXXFLAGS="$CXXFLAGS $user_CXXFLAGS"
BITCODE_CFLAGS="$BITCODE_CFLAGS $user_BITCODE_CFLAGS"
BITCODE_CXXFLAGS="$BITCODE_CXXFLAGS $user_BITCODE_CXXFLAGS"
+case $host_cpu in
+ mips*)
+ # On MIPS, we must have ll/sc/sync instructions to do spinlocks, but those
+ # do not exist in the long-obsolete MIPS-I architecture. Annoyingly, many
+ # toolchains still default to emitting MIPS-I code, causing the assembler
+ # to reject these instructions. If using a gcc workalike, we can fix this
+ # by adding -march=mips2 to CFLAGS; otherwise, you're on your own to
+ # select the right compiler flag or use --disable-spinlocks.
+ if test "$enable_spinlocks" = yes -a x"$GCC" = x"yes"; then
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether assembler supports ll/sc/sync" >&5
+$as_echo_n "checking whether assembler supports ll/sc/sync... " >&6; }
+if ${pgac_cv_have_mips_spinlock_insts+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+
+int
+main ()
+{
+register volatile unsigned int *_l = NULL;
+ register int _res, _tmp;
+ __asm__ __volatile__(
+ " ll %0, %2 \n"
+ " sc %1, %2 \n"
+ " sync \n"
+ : "=&r" (_res), "=&r" (_tmp), "+R" (*_l) : : "memory");
+ return _res;
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+ pgac_cv_have_mips_spinlock_insts=yes
+else
+ pgac_cv_have_mips_spinlock_insts=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_have_mips_spinlock_insts" >&5
+$as_echo "$pgac_cv_have_mips_spinlock_insts" >&6; }
+ if test x"$pgac_cv_have_mips_spinlock_insts" = xno; then
+ CFLAGS="$CFLAGS -march=mips2"
+ CXXFLAGS="$CXXFLAGS -march=mips2"
+ BITCODE_CFLAGS="$BITCODE_CFLAGS -march=mips2"
+ BITCODE_CXXFLAGS="$BITCODE_CXXFLAGS -march=mips2"
+ fi
+ fi
+ ;;
+esac
+
BITCODE_CFLAGS=$BITCODE_CFLAGS
BITCODE_CXXFLAGS=$BITCODE_CXXFLAGS
diff --git a/configure.in b/configure.in
index 4586a17..e1097aa 100644
--- a/configure.in
+++ b/configure.in
@@ -607,6 +607,38 @@ CXXFLAGS="$CXXFLAGS $user_CXXFLAGS"
BITCODE_CFLAGS="$BITCODE_CFLAGS $user_BITCODE_CFLAGS"
BITCODE_CXXFLAGS="$BITCODE_CXXFLAGS $user_BITCODE_CXXFLAGS"
+case $host_cpu in
+ mips*)
+ # On MIPS, we must have ll/sc/sync instructions to do spinlocks, but those
+ # do not exist in the long-obsolete MIPS-I architecture. Annoyingly, many
+ # toolchains still default to emitting MIPS-I code, causing the assembler
+ # to reject these instructions. If using a gcc workalike, we can fix this
+ # by adding -march=mips2 to CFLAGS; otherwise, you're on your own to
+ # select the right compiler flag or use --disable-spinlocks.
+ if test "$enable_spinlocks" = yes -a x"$GCC" = x"yes"; then
+ AC_CACHE_CHECK([whether assembler supports ll/sc/sync],
+ [pgac_cv_have_mips_spinlock_insts],
+ [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],
+ [register volatile unsigned int *_l = NULL;
+ register int _res, _tmp;
+ __asm__ __volatile__(
+ " ll %0, %2 \n"
+ " sc %1, %2 \n"
+ " sync \n"
+ : "=&r" (_res), "=&r" (_tmp), "+R" (*_l) : : "memory");
+ return _res;])],
+ [pgac_cv_have_mips_spinlock_insts=yes],
+ [pgac_cv_have_mips_spinlock_insts=no])])
+ if test x"$pgac_cv_have_mips_spinlock_insts" = xno; then
+ CFLAGS="$CFLAGS -march=mips2"
+ CXXFLAGS="$CXXFLAGS -march=mips2"
+ BITCODE_CFLAGS="$BITCODE_CFLAGS -march=mips2"
+ BITCODE_CXXFLAGS="$BITCODE_CXXFLAGS -march=mips2"
+ fi
+ fi
+ ;;
+esac
+
AC_SUBST(BITCODE_CFLAGS, $BITCODE_CFLAGS)
AC_SUBST(BITCODE_CXXFLAGS, $BITCODE_CXXFLAGS)
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index a9a92de..3af033b 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -614,7 +614,6 @@ tas(volatile slock_t *lock)
__asm__ __volatile__(
" .set push \n"
- " .set mips2 \n"
" .set noreorder \n"
" .set nomacro \n"
" ll %0, %2 \n"
@@ -636,7 +635,6 @@ do \
{ \
__asm__ __volatile__( \
" .set push \n" \
- " .set mips2 \n" \
" .set noreorder \n" \
" .set nomacro \n" \
" sync \n" \
Hi, On 2019-06-16 12:16:53 -0400, Tom Lane wrote: > (We should not do anything if --disable-spinlocks has been specified, > btw, since then we don't need those asm commands to work. This is > and would remain the workaround for building PG for MIPS-I, if some > benighted soul insists on doing that.) I think we're pretty much at the point where we should just rip out all of our own spinlock implementations for non-common platforms, and solely rely on compiler intrinsics... The open coded ASM really doesn't age very well, and there's very little chance of us actually testing them properly. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > I think we're pretty much at the point where we should just rip out all > of our own spinlock implementations for non-common platforms, and solely > rely on compiler intrinsics... The open coded ASM really doesn't age > very well, and there's very little chance of us actually testing them > properly. It's completely not true that this code isn't tested; we have two different MIPS buildfarm machines that surely exercise spinlocks plenty. (There's a separate argument to be had about whether we should drop source-code support for platforms that aren't represented in the buildfarm. I'm not inclined to, but you could tenably hold that position.) As for compiler intrinsics, I dunno. I don't have very much faith in the quality of those for non-mainstream platforms, either --- see https://www.postgresql.org/message-id/flat/25414.1483076673%40sss.pgh.pa.us for a not-too-old example. And a lot of platforms like this are running pretty old compilers, so even if the problems have been fixed it'll be a long time before we can depend on that. regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月18日周二 上午12:50写道： > > Andres Freund <andres@anarazel.de> writes: > > I think we're pretty much at the point where we should just rip out all > > of our own spinlock implementations for non-common platforms, and solely > > rely on compiler intrinsics... The open coded ASM really doesn't age > > very well, and there's very little chance of us actually testing them > > properly. > > It's completely not true that this code isn't tested; we have two > different MIPS buildfarm machines that surely exercise spinlocks plenty. > Since NetBSD set the default ISA as MIPS I, and then we use ".set mips2", in fact a bug. I think that we should use some non-asm code for MIPS I ( our own spinlock implementations?) Do you have any NetBSD image that I can have a test of new patch? > (There's a separate argument to be had about whether we should drop > source-code support for platforms that aren't represented in the > buildfarm. I'm not inclined to, but you could tenably hold that > position.) > > As for compiler intrinsics, I dunno. I don't have very much faith in > the quality of those for non-mainstream platforms, either --- see > https://www.postgresql.org/message-id/flat/25414.1483076673%40sss.pgh.pa.us > for a not-too-old example. And a lot of platforms like this are running > pretty old compilers, so even if the problems have been fixed it'll be a > long time before we can depend on that. > > regards, tom lane -- YunQiang Su
YunQiang Su <wzssyqa@gmail.com> writes:
> Since NetBSD set the default ISA as MIPS I, and then we use ".set mips2",
> in fact a bug.
> I think that we should use some non-asm code for MIPS I ( our own
> spinlock implementations?)
You probably won't be surprised to hear that we keep having this
discussion ;-). Sure, we could essentially throw away all of s_lock.h in
favor of relying on __sync_lock_test_and_set() and __sync_lock_release().
But doing that would essentially abdicate having control over, or even
having much knowledge about, infrastructure that's absolutely critical
to the correctness and performance of Postgres. And we've seen a bunch
of cases where the compiler builtins are just not very well implemented
for non-mainstream architectures.
I got debian-mips-under-qemu working thanks to the files you sent a link
to, and I was curious to see what that gcc version would generate for
__sync_lock_test_and_set() and __sync_lock_release(), if I asked for
MIPS-I code. That's not the default of course, but "gcc -march=mips1
-mabi=32" seems to work. And what I get is
.set push
.set mips2
1:
ll $2,0($3)
li $1,1
sc $1,0($3)
beq $1,$0,1b
nop
sync
.set pop
and
.set push
.set mips2
sync
.set pop
sw $0,0($3)
So the gcc guys aren't any more wedded than we are to the rule that
one must not generate LL/SC on MIPS-I. They're probably assuming the
same thing we are, which is that if you're actually doing this on
MIPS-I then you'll get an instruction trap and the kernel will
emulate it for you. Any other choice would be completely disastrous
for performance on anything newer than MIPS-I ... and really, if you
are doing something that requires userland synchronization primitives,
you'd better not be using MIPS-I. It hasn't got them.
In short, moving to __sync_lock_test_and_set() would change basically
nothing on MIPS, but we'd no longer have any visibility into or control
over what it was doing. It would open us to other problems too -- for
example, if you try to apply __sync_lock_test_and_set() to a char
rather than an int, the code you get for that on MIPS is a whole lot
worse, but we'd have no visibility of that.
> Do you have any NetBSD image that I can have a test of new patch?
I've not been able to get NetBSD/MIPS to work under qemu. It does
mostly work under gxemul, but that has so many bugs as to be nigh
useless --- the float arithmetic emulation is what usually gets me
when I try to do anything with Postgres.
regards, tom lane
I wrote:
> So the gcc guys aren't any more wedded than we are to the rule that
> one must not generate LL/SC on MIPS-I. They're probably assuming the
> same thing we are, which is that if you're actually doing this on
> MIPS-I then you'll get an instruction trap and the kernel will
> emulate it for you.
Actually, after further contemplating that argument (which I was reminded
of by excavating in our commit log), I realize that having a .set mips2
in the ASM is actually the right way to do this, because that only results
in LL/SC getting emulated when we need them to be. Forcing -march=mips2
across the entire Postgres build would exceed our authority really ---
yeah, it might produce better code quality, but it's not our business to
override platform-level target architecture decisions.
So I now think your original proposal was about the best way to do this,
though I'd tweak it to make the test be "#if __mips_isa_rev < 2", as
attached. It doesn't seem like forcing the ISA level down is ever
something we want to do, even if we know there's no practical difference.
regards, tom lane
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index a9a92de..d2c707e 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -598,13 +598,31 @@ tas(volatile slock_t *lock)
#if defined(__mips__) && !defined(__sgi) /* non-SGI MIPS */
-/* Note: R10000 processors require a separate SYNC */
#define HAS_TEST_AND_SET
typedef unsigned int slock_t;
#define TAS(lock) tas(lock)
+/*
+ * Original MIPS-I processors lacked the LL/SC instructions, but if we are
+ * so unfortunate as to be running on one of those, we expect that the kernel
+ * will handle the illegal-instruction traps and emulate them for us. On
+ * anything newer (and really, MIPS-I is extinct) LL/SC is the only sane
+ * choice because any other synchronization method must involve a kernel
+ * call. Unfortunately, many toolchains still default to MIPS-I as the
+ * codegen target, so we have to be prepared to force the assembler to
+ * accept LL/SC.
+ *
+ * R10000 and up processors require a separate SYNC, which has the same
+ * issues as LL/SC.
+ */
+#if __mips_isa_rev < 2
+#define MIPS_SET_MIPS2 " .set mips2 \n"
+#else
+#define MIPS_SET_MIPS2
+#endif
+
static __inline__ int
tas(volatile slock_t *lock)
{
@@ -614,7 +632,7 @@ tas(volatile slock_t *lock)
__asm__ __volatile__(
" .set push \n"
- " .set mips2 \n"
+ MIPS_SET_MIPS2
" .set noreorder \n"
" .set nomacro \n"
" ll %0, %2 \n"
@@ -636,7 +654,7 @@ do \
{ \
__asm__ __volatile__( \
" .set push \n" \
- " .set mips2 \n" \
+ MIPS_SET_MIPS2 \
" .set noreorder \n" \
" .set nomacro \n" \
" sync \n" \
Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月23日周日 上午6:37写道： > > I wrote: > > So the gcc guys aren't any more wedded than we are to the rule that > > one must not generate LL/SC on MIPS-I. They're probably assuming the > > same thing we are, which is that if you're actually doing this on > > MIPS-I then you'll get an instruction trap and the kernel will > > emulate it for you. > > Actually, after further contemplating that argument (which I was reminded > of by excavating in our commit log), I realize that having a .set mips2 > in the ASM is actually the right way to do this, because that only results > in LL/SC getting emulated when we need them to be. Forcing -march=mips2 > across the entire Postgres build would exceed our authority really --- > yeah, it might produce better code quality, but it's not our business to > override platform-level target architecture decisions. you are correct, that we should follow the target architecture decisions of OS vendor, if possible. > > So I now think your original proposal was about the best way to do this, > though I'd tweak it to make the test be "#if __mips_isa_rev < 2", as here we should use #if __mips < 2 instead of __mips_isa_rev . The history of MIPS revisions: MIPS I : __mips=1 __mips_isa_rev = undef MIPS II : __mips=2 __mips_isa_rev = undef MIPS III : __mips=3 __mips_isa_rev = undef MIPS IV : __mips=4 __mips_isa_rev = undef MIPS 32/64(r1) : __mips=32/64 __mips_isa_rev = 1 MIPS 32/64 r2 : __mips=32/64 __mips_isa_rev = 2 MIPS 32/64 r3 : __mips=32/64 __mips_isa_rev = 3 MIPS 32/64 r5 : __mips=32/64 __mips_isa_rev = 5 MIPS 32/64 r6 : __mips=32/64 __mips_isa_rev = 6 > attached. It doesn't seem like forcing the ISA level down is ever > something we want to do, even if we know there's no practical difference. > > regards, tom lane > -- YunQiang Su
YunQiang Su <wzssyqa@gmail.com> writes:
> Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月23日周日 上午6:37写道：
>> So I now think your original proposal was about the best way to do this,
>> though I'd tweak it to make the test be "#if __mips_isa_rev < 2", as
> here we should use #if __mips < 2 instead of __mips_isa_rev .
OK, will do.
regards, tom lane