Re: [PATCHES] [BUGS] BUG #2401: spinlocks not available on amd64
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/ ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [BUGS] BUG #2401: spinlocks not available on amd64
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 EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [BUGS] BUG #2401: spinlocks not available on amd64
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 !=== ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [BUGS] BUG #2401: spinlocks not available on amd64
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 <[EMAIL PROTECTED]> 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 EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [BUGS] BUG #2401: spinlocks not available on amd64
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 <[EMAIL PROTECTED]> 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? ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [BUGS] BUG #2401: spinlocks not available on amd64
Bruce Momjian 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 ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] [BUGS] BUG #2401: spinlocks not available on amd64
Tom Lane wrote: > Theo Schlossnagle <[EMAIL PROTECTED]> 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 EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [BUGS] BUG #2401: spinlocks not available on amd64
Theo Schlossnagle <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [BUGS] BUG #2401: spinlocks not available on amd64
Tom Lane wrote: Bruce Momjian 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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] [BUGS] BUG #2401: spinlocks not available on amd64
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 EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [BUGS] BUG #2401: spinlocks not available on amd64
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/solaris27 Apr 2006 22:28:42 - 1.26 --- src/template/solaris28 Apr 2006 04:20:02 - *** *** 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [BUGS] BUG #2401: spinlocks not available on amd64
Tom Lane wrote: > Bruce Momjian 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 EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] [BUGS] BUG #2401: spinlocks not available on amd64
Bruce Momjian 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [BUGS] BUG #2401: spinlocks not available on amd64
Bruce Momjian 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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] [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: [EMAIL PROTECTED] > >> 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 > movl4(%esp), %edx > movl8(%esp), %ecx > movl12(%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 EnterpriseDBhttp://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 - 1.2 --- src/backend/port/tas/solaris_sparc.s 27 Apr 2006 21:56:32 - *** *** 1,50 ! !! ! !! $PostgreSQL: pgsql/src/backend/port/tas/solaris_spa
