Thread: Re: [BUGS] BUG #2401: spinlocks not available on amd64
Great, changes attached and applied. I removed the solaris_i386 and solaris_x86_64.s files and made just one solaris_x86.s. I updated the build system to use the new file, updated the macros, and added some documentation on the approach. Thanks. Would you test current CVS to make sure it still works properly? Thanks. Shame, but this is too complex to backpatch. Seems it will just have to wait for 8.2. --------------------------------------------------------------------------- Theo Schlossnagle wrote: > > On Apr 19, 2006, at 11:17 PM, Bruce Momjian wrote: > > > Theo Schlossnagle wrote: > >> > >> The following bug has been logged online: > >> > >> Bug reference: 2401 > >> Logged by: Theo Schlossnagle > >> Email address: jesus@omniti.com > >> PostgreSQL version: 8.1.3 > >> Operating system: Solaris 10 > >> Description: spinlocks not available on amd64 > >> Details: > >> > >> Compiling 8.1.3 on solaris 10 x86 with Sun Studio 11 for amd64 target > >> architecture leads us to an error resulting from no available "tas" > >> assembly. > >> > >> The tas.s file doesn't look like valid assembly for the shipped Sun > >> assembler. > > > > Yes. We will have a fix for it in 8.2, but it was too risky for > > 8.1.X. > > You can try a snapshot tarball from our FTP server and see if that > > works. > > I reviewed the code there. I think it can be better. The issue is > that s_lock chooses to implement the lock in an unsigned char which > isn't optimal on sparc or x86. An over arching issue is that the tas > instruction is a less functional cas function, so it seems that the > tas should be cas and the spinlocks should be implemented that way. > By using cas, you can can actually know the locker by cas'ing the > lock to the process id instead of 1 -- we use that trick to see who > is holding the spinlock between threads (we obviously use thread ids > in that case). > > So... I changed the s_lock.h to merge the sparc and x86 sections thusly: > > ------------- > #if defined(__sun) && (defined(__i386) || defined(__amd64) || defined > (__sparc) | > | defined(__sparc__)) > /* > * Solaris/386 (we only get here for non-gcc case) > */ > #define HAS_TEST_AND_SET > typedef unsigned int slock_t; > > extern volatile slock_t > pg_atomic_cas(volatile slock_t *lock, slock_t with, slock_t cmp); > > #define TAS(a) (pg_atomic_cas((a), 1, 0) != 0) > > #endif > ------------- > > And then replaced solaris_sparc.s and solaris_i386.s (BTW there is no > reason to have these files seprately as you can #ifdef them correctly > in the assembly -- I didn't do that as I didn't want to disturb the > make system). > > solaris_sparc.s > ------------- > / > ======================================================================== > ===== > / tas.s -- compare and swap for solaris_sparc > / > ======================================================================== > ===== > > #if defined(__sparcv9) || defined(__sparc) > > .section ".text" > .align 8 > .skip 24 > .align 4 > > .global pg_atomic_cas > pg_atomic_cas: > cas [%o0],%o2,%o1 > mov %o1,%o0 > retl > nop > .type pg_atomic_cas,2 > .size pg_atomic_cas,(.-pg_atomic_cas) > #endif > ------------- > > solaris_i386.s > ------------- > / > ======================================================================== > ===== > / tas.s -- compare and swap for solaris_i386 > / > ======================================================================== > ===== > > .file "tas.s" > > #if defined(__amd64) > .code64 > #endif > > .globl pg_atomic_cas > .type pg_atomic_cas, @function > > .section .text, "ax" > .align 16 > pg_atomic_cas: > #if defined(__amd64) > movl %edx,%eax > lock > cmpxchgl %esi,(%rdi) > #else > movl 4(%esp), %edx > movl 8(%esp), %ecx > movl 12(%esp), %eax > lock > cmpxchgl %ecx, (%edx) > #endif > ret > .size pg_atomic_cas, . - pg_atomic_cas > ------------- > > > // Theo Schlossnagle > // CTO -- http://www.omniti.com/~jesus/ > // OmniTI Computer Consulting, Inc. -- http://www.omniti.com/ > // Ecelerity: Run with it. > > -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: src/backend/port/tas/solaris_sparc.s =================================================================== RCS file: /cvsroot/pgsql/src/backend/port/tas/solaris_sparc.s,v retrieving revision 1.2 diff -c -c -r1.2 solaris_sparc.s *** src/backend/port/tas/solaris_sparc.s 29 Nov 2003 19:51:54 -0000 1.2 --- src/backend/port/tas/solaris_sparc.s 27 Apr 2006 21:56:32 -0000 *************** *** 1,50 **** ! !! ! !! $PostgreSQL: pgsql/src/backend/port/tas/solaris_sparc.s,v 1.2 2003/11/29 19:51:54 pgsql Exp $ ! !! ! !! this would be a piece of inlined assembler but it appears ! !! to be easier to just write the assembler than to try to ! !! figure out how to make sure that in/out registers are kept ! !! straight in the asm's. ! !! ! .file "tas.c" ! .section ".text" ! .align 4 ! .global tas ! .type tas,#function ! .proc 04 ! tas: ! !! ! !! this is a leaf procedure - no need to save windows and ! !! diddle the CWP. ! !! ! !#PROLOGUE# 0 ! !#PROLOGUE# 1 ! ! !! ! !! write 0xFF into the lock address, saving the old value in %o0. ! !! this is an atomic action, even on multiprocessors. ! !! ! ldstub [%o0],%o0 ! ! !! ! !! if it was already set when we set it, somebody else already ! !! owned the lock -- return 1. ! !! ! cmp %o0,0 ! bne .LL2 ! mov 1,%o0 ! ! !! ! !! otherwise, it was clear and we now own the lock -- return 0. ! !! ! mov 0,%o0 ! .LL2: ! !! ! !! this is a leaf procedure - no need to restore windows and ! !! diddle the CWP. ! !! ! retl ! nop ! .LLfe1: ! .size tas,.LLfe1-tas ! .ident "GCC: (GNU) 2.5.8" --- 1,20 ---- ! /======================================================================= ! / solaris_sparc.s -- compare and swap for solaris_sparc ! /======================================================================= ! ! #if defined(__sparcv9) || defined(__sparc) ! ! .section ".text" ! .align 8 ! .skip 24 ! .align 4 ! ! .global pg_atomic_cas ! pg_atomic_cas: ! cas [%o0],%o2,%o1 ! mov %o1,%o0 ! retl ! nop ! .type pg_atomic_cas,2 ! .size pg_atomic_cas,(.-pg_atomic_cas) ! #endif Index: src/include/storage/s_lock.h =================================================================== RCS file: /cvsroot/pgsql/src/include/storage/s_lock.h,v retrieving revision 1.149 diff -c -c -r1.149 s_lock.h *** src/include/storage/s_lock.h 19 Apr 2006 23:11:15 -0000 1.149 --- src/include/storage/s_lock.h 27 Apr 2006 21:56:37 -0000 *************** *** 763,785 **** #endif ! #if defined(__sparc__) || defined(__sparc) #define HAS_TEST_AND_SET - typedef unsigned char slock_t; - #endif - - - /* out-of-line assembler from src/backend/port/tas/foo.s */ ! /* i386/X86_64 using Sun compiler */ ! #if defined(__sun) && (defined(__i386) || defined(__x86_64__)) ! /* ! * Solaris/386 (we only get here for non-gcc case) ! */ ! #define HAS_TEST_AND_SET ! typedef unsigned char slock_t; #endif --- 763,776 ---- #endif ! #if defined(__sun) && (defined(__i386) || defined(__x86_64__) || defined(__sparc__) || defined(__sparc)) #define HAS_TEST_AND_SET typedef unsigned char slock_t; ! extern volatile slock_t pg_atomic_cas(volatile slock_t *lock, slock_t with, ! slock_t cmp); ! #define TAS(a) (pg_atomic_cas((a), 1, 0) != 0) #endif Index: src/template/solaris =================================================================== RCS file: /cvsroot/pgsql/src/template/solaris,v retrieving revision 1.25 diff -c -c -r1.25 solaris *** src/template/solaris 2 Jan 2006 03:30:41 -0000 1.25 --- src/template/solaris 27 Apr 2006 21:56:38 -0000 *************** *** 4,32 **** if test "$enable_debug" != yes; then CFLAGS="$CFLAGS -O" # any optimization breaks debug fi ! fi ! ! # Pick right test-and-set (TAS) code. We need out-of-line assembler ! # when not using gcc. ! case $host in ! sparc-*-solaris*) ! if test "$GCC" != yes ; then ! need_tas=yes ! tas_file=solaris_sparc.s ! fi ;; ! i?86-*-solaris*) ! if test "$GCC" != yes ; then ! if isainfo | grep amd64 ! then ! need_tas=yes ! tas_file=solaris_x86_64.s ! else ! need_tas=yes ! tas_file=solaris_i386.s ! fi ! fi ;; ! esac # -D_POSIX_PTHREAD_SEMANTICS enables 5-arg getpwuid_r, among other things --- 4,24 ---- if test "$enable_debug" != yes; then CFLAGS="$CFLAGS -O" # any optimization breaks debug fi ! else ! # Pick the right test-and-set (TAS) code for the Sun compiler. ! # We would like to use in-line assembler, but the compiler ! # requires *.il files to be on every compile line, making ! # the build system too fragile. ! case $host in ! sparc-*-solaris*) ! need_tas=yes ! tas_file=solaris_sparc.s ;; ! i?86-*-solaris*) ! need_tas=yes ! tas_file=solaris_x86.s ;; ! esac ! fi # -D_POSIX_PTHREAD_SEMANTICS enables 5-arg getpwuid_r, among other things
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> ! extern volatile slock_t pg_atomic_cas(volatile slock_t *lock, slock_t with,
> ! slock_t cmp);
Surely it is not useful to mark the result of a function as volatile.
regards, tom lane
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Great, changes attached and applied. I removed the solaris_i386 and
> solaris_x86_64.s files and made just one solaris_x86.s. I updated the
> build system to use the new file, updated the macros, and added some
> documentation on the approach. Thanks.
BTW, on the replacement of ldstub with cas: according to what I find in
google, cas does not exist on pre-V9 SPARCs. Are we sure no one uses
sparc v8 chips anymore? Is there any actual advantage to making that
change?
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > > ! extern volatile slock_t pg_atomic_cas(volatile slock_t *lock, slock_t with, > > ! slock_t cmp); > > Surely it is not useful to mark the result of a function as volatile. "volatile" removed. -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote:
> Great, changes attached and applied. I removed the solaris_i386 and
> solaris_x86_64.s files and made just one solaris_x86.s. I updated the
> build system to use the new file, updated the macros, and added some
> documentation on the approach. Thanks.
>
> Would you test current CVS to make sure it still works properly? Thanks.
>
The patch adds an extra else in src/template/solaris that means the
assembly file is not properly picked up.
Also the claim that Sun's cc understands C preprocessor doesn't hold
true for me:
/usr/local/SUNWspro/bin/cc -Xa -v -DSUNOS4_CC -g -c tas.s
Assembler: tas.s
"tas.s", line 9 : Illegal mnemonic
"tas.s", line 9 : Syntax error
"tas.s", line 9 : Illegal mnemonic
"tas.s", line 9 : Illegal mnemonic
This is with cc -V
cc: Sun WorkShop 6 update 2 C 5.3 Patch 111680-09 2003/05/18
Index: src/template/solaris
===================================================================
RCS file: /projects/cvsroot/pgsql/src/template/solaris,v
retrieving revision 1.26
diff -c -r1.26 solaris
*** src/template/solaris 27 Apr 2006 22:28:42 -0000 1.26
--- src/template/solaris 28 Apr 2006 04:20:02 -0000
***************
*** 4,10 ****
if test "$enable_debug" != yes; then
CFLAGS="$CFLAGS -O" # any optimization breaks debug
fi
! else
# Pick the right test-and-set (TAS) code for the Sun compiler.
# We would like to use in-line assembler, but the compiler
# requires *.il files to be on every compile line, making
--- 4,10 ----
if test "$enable_debug" != yes; then
CFLAGS="$CFLAGS -O" # any optimization breaks debug
fi
!
# Pick the right test-and-set (TAS) code for the Sun compiler.
# We would like to use in-line assembler, but the compiler
# requires *.il files to be on every compile line, making
Kris Jurka wrote: > Bruce Momjian wrote: > > Great, changes attached and applied. I removed the solaris_i386 and > > solaris_x86_64.s files and made just one solaris_x86.s. I updated the > > build system to use the new file, updated the macros, and added some > > documentation on the approach. Thanks. > > > > Would you test current CVS to make sure it still works properly? Thanks. > > > > The patch adds an extra else in src/template/solaris that means the > assembly file is not properly picked up. Sorry, removed. > Also the claim that Sun's cc understands C preprocessor doesn't hold > true for me: > > /usr/local/SUNWspro/bin/cc -Xa -v -DSUNOS4_CC -g -c tas.s > Assembler: tas.s > "tas.s", line 9 : Illegal mnemonic > "tas.s", line 9 : Syntax error > "tas.s", line 9 : Illegal mnemonic > "tas.s", line 9 : Illegal mnemonic > > This is with cc -V > cc: Sun WorkShop 6 update 2 C 5.3 Patch 111680-09 2003/05/18 Interesting. It is probably possible to just run the *.s file through cpp and compile the result, or just go back to having two files. -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Tom Lane wrote: >Bruce Momjian <pgman@candle.pha.pa.us> writes: > > >>Great, changes attached and applied. I removed the solaris_i386 and >>solaris_x86_64.s files and made just one solaris_x86.s. I updated the >>build system to use the new file, updated the macros, and added some >>documentation on the approach. Thanks. >> >> > >BTW, on the replacement of ldstub with cas: according to what I find in >google, cas does not exist on pre-V9 SPARCs. Are we sure no one uses >sparc v8 chips anymore? Is there any actual advantage to making that >change? > > This is true, and can be addressed with #defines to pull in the old old code. Just to note, you can't run Solaris 10 or any future version of Solaris on sparcv8 either. Sparcv8plus is 3bit sparc code and it does support the cas operation. The instruction cas that operates on a word is more much efficient than the code that it replaced. I'm don't own or have access to any sparc machines old enough to have that issue (even on my guest accounts at my alma mater). If this is a concern, it seems more than reasonable to #elif the case in for that architecture in the sparc assembly file -- being open source, you'll no doubt alienate some dude who thinks its cool to run Postgres on his Sparc ELC. I'd remind everyone that the spinlock stuff is entirely optional at build time. I think it be more elegant approach to discontinue spinlock support on sparcv8 and older sparc architectures and just force them to use heavier weight locking mechanisms. I "accidentally" did this on my Sol10 amd64 box before I realized what the build system did. I also think it immensely useful to replace all of the tas subsystem with cas so that one could reliabily lock these atomics with the process id of the locker. This would allow extremely good diagnostics in the event that a backend process is abruptly teminated while holding a log on that shm segment. If the process ID was in there, it would then be a contition that is both diagnosable and recoverable. I could likely provide the cas bits and pieces to do this stuff on linux/freebsd/mac os x and windows on x86/64 sparcv8plus/sparcv9 and PPC if you think that would be useful. Best regards, Theo
Theo Schlossnagle <jesus@omniti.com> writes:
> I'd remind everyone that the spinlock stuff is entirely optional at
> build time.
Not really. The performance hit for not having hardware spinlocks is
so severe that it's not considered a reasonable fallback.
> I also think it immensely useful to replace all of the tas subsystem
> with cas so that one could reliabily lock these atomics with the process
> id of the locker.
I cannot, ever once in my years working on Postgres, remember having
wanted such a thing. I am strongly against mucking with the spinlock
code for mere aesthetics --- it's too fragile and hard to test,
especially on platforms you don't have ready access to.
In short, it ain't broken and we don't need to fix it.
regards, tom lane
Tom Lane wrote: > Theo Schlossnagle <jesus@omniti.com> writes: > > I'd remind everyone that the spinlock stuff is entirely optional at > > build time. > > Not really. The performance hit for not having hardware spinlocks is > so severe that it's not considered a reasonable fallback. > > > I also think it immensely useful to replace all of the tas subsystem > > with cas so that one could reliabily lock these atomics with the process > > id of the locker. > > I cannot, ever once in my years working on Postgres, remember having > wanted such a thing. I am strongly against mucking with the spinlock > code for mere aesthetics --- it's too fragile and hard to test, > especially on platforms you don't have ready access to. > > In short, it ain't broken and we don't need to fix it. Agreed. Should the new Solaris ASM code be modified? -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> In short, it ain't broken and we don't need to fix it.
> Agreed. Should the new Solaris ASM code be modified?
No, I have no objection to the new code for Solaris. I just don't want
to go off changing TAS macros for the sake of change.
regards, tom lane
Hi Bruce, The new SPARC assembly file src/backend/port/tas/solaris_sparc.s uses / instead of ! for comments, and as a result the compile fails with Sun Studio 11. Please modify the first 3 lines to look like the following. > !======================================================================= > ! solaris_sparc.s -- compare and swap for solaris_sparc > !======================================================================= Thanks! -Robert Bruce Momjian wrote On 04/29/06 17:16,: >Tom Lane wrote: > > >>Theo Schlossnagle <jesus@omniti.com> writes: >> >> >>>I'd remind everyone that the spinlock stuff is entirely optional at >>>build time. >>> >>> >>Not really. The performance hit for not having hardware spinlocks is >>so severe that it's not considered a reasonable fallback. >> >> >> >>>I also think it immensely useful to replace all of the tas subsystem >>>with cas so that one could reliabily lock these atomics with the process >>>id of the locker. >>> >>> >>I cannot, ever once in my years working on Postgres, remember having >>wanted such a thing. I am strongly against mucking with the spinlock >>code for mere aesthetics --- it's too fragile and hard to test, >>especially on platforms you don't have ready access to. >> >>In short, it ain't broken and we don't need to fix it. >> >> > >Agreed. Should the new Solaris ASM code be modified? > > >
Done, for Solaris and x86. --------------------------------------------------------------------------- Robert Lor wrote: > > Hi Bruce, > > The new SPARC assembly file src/backend/port/tas/solaris_sparc.s uses / > instead of ! for comments, and as a result the compile fails with Sun > Studio 11. Please modify the first 3 lines to look like the following. > > > !======================================================================= > > ! solaris_sparc.s -- compare and swap for solaris_sparc > > !======================================================================= > > > > Thanks! > > -Robert > > > Bruce Momjian wrote On 04/29/06 17:16,: > > >Tom Lane wrote: > > > > > >>Theo Schlossnagle <jesus@omniti.com> writes: > >> > >> > >>>I'd remind everyone that the spinlock stuff is entirely optional at > >>>build time. > >>> > >>> > >>Not really. The performance hit for not having hardware spinlocks is > >>so severe that it's not considered a reasonable fallback. > >> > >> > >> > >>>I also think it immensely useful to replace all of the tas subsystem > >>>with cas so that one could reliabily lock these atomics with the process > >>>id of the locker. > >>> > >>> > >>I cannot, ever once in my years working on Postgres, remember having > >>wanted such a thing. I am strongly against mucking with the spinlock > >>code for mere aesthetics --- it's too fragile and hard to test, > >>especially on platforms you don't have ready access to. > >> > >>In short, it ain't broken and we don't need to fix it. > >> > >> > > > >Agreed. Should the new Solaris ASM code be modified? > > > > > > > -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce, the change was only needed for the SPARC version only. The x86 file worked just fine before and needs to be reverted back. Yes, they are different. Apologies if the message was not clear the first time! Thanks, Robert Bruce Momjian wrote: >Done, for Solaris and x86. > >--------------------------------------------------------------------------- > >Robert Lor wrote: > > >>Hi Bruce, >> >>The new SPARC assembly file src/backend/port/tas/solaris_sparc.s uses / >>instead of ! for comments, and as a result the compile fails with Sun >>Studio 11. Please modify the first 3 lines to look like the following. >> >> >> >>>!======================================================================= >>>! solaris_sparc.s -- compare and swap for solaris_sparc >>>!======================================================================= >>> >>> >> >>
Robert Lor wrote: > Bruce, the change was only needed for the SPARC version only. The x86 > file worked just fine before and needs to be reverted back. Yes, they > are different. OK, fixed, thanks. > Apologies if the message was not clear the first time! Yes, you were clear, but it was so illogical I figured it must be both platforms. :-0 I added a comment to clarify. This was the way the comments were in 8.1.X. -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote:
> Robert Lor wrote:
>> Bruce, the change was only needed for the SPARC version only. The x86
>> file worked just fine before and needs to be reverted back. Yes, they
>> are different.
>
> OK, fixed, thanks.
>
>> Apologies if the message was not clear the first time!
>
> Yes, you were clear, but it was so illogical I figured it must be both
> platforms. :-0
Don't blame x86 for SPARCs sillyness, that just isn't fair ;)
Joshua D. Drake
>
> I added a comment to clarify. This was the way the comments were in
> 8.1.X.
>
--
=== The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240
Providing the most comprehensive PostgreSQL solutions since 1997
http://www.commandprompt.com/