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. +


---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

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

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

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

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

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. +

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

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. +

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

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

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

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

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

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. +

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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

---------------------------(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