Skip site navigation (1) Skip section navigation (2)

Re: BUG #2401: spinlocks not available on amd64

From: Theo Schlossnagle <jesus(at)omniti(dot)com>
To: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
Cc: Theo Schlossnagle <jesus(at)omniti(dot)com>, pgsql-bugs(at)postgresql(dot)org, josh(dot)berkus(at)sun(dot)com, robert(dot)lor(at)sun(dot)com
Subject: Re: BUG #2401: spinlocks not available on amd64
Date: 2006-04-20 03:33:11
Message-ID: 154BA792-BB51-4F85-86E3-0BBAEB732C33@omniti.com (view raw or flat)
Thread:
Lists: pgsql-bugspgsql-patches
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(at)omniti(dot)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.



In response to

Responses

pgsql-bugs by date

Next:From: Tomas ZeroloDate: 2006-04-20 04:40:58
Subject: Re: BUG #2400: 'Ã
Previous:From: Bruce MomjianDate: 2006-04-20 03:17:51
Subject: Re: BUG #2401: spinlocks not available on amd64

pgsql-patches by date

Next:From: Dave PageDate: 2006-04-20 07:07:17
Subject: Re: [PATCH] Reduce noise from tsort
Previous:From: Bruce MomjianDate: 2006-04-20 03:17:51
Subject: Re: BUG #2401: spinlocks not available on amd64

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group