Re: BUG #2401: spinlocks not available on amd64

Theo Schlossnagle wrote:
> >> 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).

Great.

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

OK, this is a great help.  If you think it should be just one file we
can do that, but since the are separate instructions sets, separate
files I think still makes sense.  No one at Sun is helping us (after
repeated requests), so we do need someone to help clear up this mess and
improve it.

Let me merge in what you have done and I will let you know when you can
test another snapshot.

---------------------------------------------------------------------------


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