Re: [BUGS] BUG #2401: spinlocks not available on amd64 - Mailing list pgsql-patches

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