322287 - Implement NSPR atomic routines in x86 assembly code for Intel Macs

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Wan-Teh Chang]

We should implement the NSPR atomic routines
(PR_AtomicXXX, declared in "pratom.h") in x86
assembly code for the upcoming Intel Macs.
(They are implemented in PowerPC assembly code
for the current Macs.  See
mozilla/nsprpub/pr/src/md/unix/os_Darwin_ppc.s)
We can use the x86 assembly code for Linux x86 in
mozilla/nsprpub/pr/src/md/unix/os_Linux_x86.s
as a starting point.

Any volunteer?

Comment 1

[Josh Aas]

I'll take a crack at it.

Comment 2

[Mark Mentovai]

Coincidentally, I glanced at this yesterday, and it looked like the Linux code would be a near drop-in.

Comment 3

[Josh Aas]

Attachment: work snapshot

Just putting this here to save my work and pass between machines. Not asking for review.

Comment 4

[Josh Aas]

Attachment: fix v1.0

This should work. It compiles and works fine on my Intel Mac.

Comment 5

[Josh Aas]

Requesting blocking since this is an Intel Mac fix that we'll need on the 1.5.0.x branch. Only affects Intel Mac builds - has no affect on PPC Mac builds.

Comment 6

[Mark Mentovai]

Comment on attachment 207701 [details] [diff] [review]
fix v1.0

Nice.  I've read the code and built it, and it's all OK.  I don't have the equipment to test it, but I'm sure you have.

Just a few minor comments:

>Index: pr/src/md/unix/os_Darwin_x86.s
>+#
>+# PRInt32 __PR_Darwinx86_AtomicSet(PRInt32 *val, PRInt32 newval);
>+#
>+    .text
>+    .globl __PR_Darwinx86_AtomicSet
>+    .align 4
>+__PR_Darwinx86_AtomicSet:
>+    movl 4(%esp), %ecx
>+    movl 8(%esp), %eax
>+    lock
>+    xchgl %eax, (%ecx)
>+    ret

I know it's present in the Linux source, but LOCK before XCHG isn't necessary.  XCHG implies LOCK.  Doesn't make a difference to me either way.

>Index: configure.in
>     if test "$CPU_ARCH" = "ppc"; then
>         PR_MD_ASFILES=os_Darwin_ppc.s
>     fi
>+    if test "$CPU_ARCH" = "i386"; then
>+        PR_MD_ASFILES=os_Darwin_x86.s
>+    fi

It would be better if we set PR_MD_ASFILES a few lines up, in the |case "${target_cpu}"| block where |$CPU_ARCH| is set, and avoid the extra conditionals.

>Index: pr/include/md/_darwin.h
> #endif /* __ppc__ */
> 
>+#if defined(__i386__)

Let's go with #elif defined(__i386__), and feel free to fix the #define of |_PR_SI_ARCHITECTURE| earlier in the file to be #ifdef __i386__/#elif defined(__ppc__)/#endif also.

>+#define _PR_HAVE_ATOMIC_OPS
>+#define _MD_INIT_ATOMIC()
>+extern PRInt32 _PR_Darwinx86_AtomicIncrement(PRInt32 *val);

Since the x isn't capitalized, maybe Darwinx86 is better as Darwin_x86?

Comment 7

[Josh Aas]

(In reply to comment #6)

> I know it's present in the Linux source, but LOCK before XCHG isn't necessary. 

From the IA-32 instruction manual:

"If a memory operand is referenced, the LOCK# signal is automatically asserted for the duration of the exchange operation, regardless of the presence or absence of the LOCK prefix or the value of the IOPL."

True. I will drop it from the Mac OS X and the Linux x86 impls.

> It would be better if we set PR_MD_ASFILES a few lines up, in the |case
> "${target_cpu}"| block where |$CPU_ARCH| is set, and avoid the extra
> conditionals.

Sure.

> Let's go with #elif defined(__i386__), and feel free to fix the #define of
> |_PR_SI_ARCHITECTURE| earlier in the file to be #ifdef __i386__/#elif
> defined(__ppc__)/#endif also.

Sure.

> >+#define _PR_HAVE_ATOMIC_OPS
> >+#define _MD_INIT_ATOMIC()
> >+extern PRInt32 _PR_Darwinx86_AtomicIncrement(PRInt32 *val);
> 
> Since the x isn't capitalized, maybe Darwinx86 is better as Darwin_x86?

Sure.

Comment 8

[Wan-Teh Chang]

I remember originally I didn't use the lock prefix, and
a Netscape Directory Server engineer demonstrated by testing
that it was necessary on multiprocessor systems.  I don't
remember whether the testing was done on Windows or Linux,
and whether the lock prefix was for the xchg or the xadd
instruction (or both).  If I have to guess, I'd guess that
the lock prefix was for the xadd instruction because the
testing was more likely to test incrementing and decrementing
a counter atomically than atomic set.

I didn't find any mention of "lock" in the CVS logs for
mozilla/nsprpub/pr/src/md/windows/ntmisc.c
(which has the inline x86 assembly) and
mozilla/nsprpub/pr/src/md/unix/os_Linux_x86.s, so I can't
provide more information than my vague memory.  It is
likely that the lock prefix was added before NSPR was
open sourced.

In Bugzilla, the only thing I can find is bug 5358 comment 8,
which also says the lock prefix is not necessary for xchg.

Comment 9

[Mark Mentovai]

XADD doesn't imply LOCK like XCHG does.

"The LOCK prefix can be prepended only to the following instructions and to those forms of the instructions that use a memory operand: ADD, ADC, AND, BTC, BTR, BTS, CMPXCHG, DEC, INC, NEG, NOT, OR, SBB, SUB, XOR, XADD, and XCHG...The XCHG instruction always asserts the LOCK# signal regardless of the presence or absence of the LOCK prefix."

ftp://download.intel.com/design/pentium/MANUALS/24319101.PDF page 303, see also pages 490, 492.

If there's any doubt, I suggest we test that XCHG with implicit LOCK functions as expected.

Comment 10

[Wan-Teh Chang]

Mark, I have no doubt.  It is much easier to write
a test program for PR_AtomicIncrement and PR_AtomicDecrement
than a test program for PR_AtomicSet, so most likely
it was the xadd instruction that I added the lock
prefix to, even though I can't find documentation
of that change in Bugzilla or the CVS repository.

Comment 11

[Josh Aas]

Attachment: fix v1.1

Addresses Mark's comments. The "lock" prefix for |xchg| instructions has been dropped on Linux x86, x86_64, and Darwin_x86.

Comment 12

[Mark Mentovai]

Comment on attachment 207747 [details] [diff] [review]
fix v1.1

You can also remove LOCK before XCHG from os_SunOS_x86 and os_SunOS_x86_64.

Comment 13

[Josh Aas]

Comment on attachment 207747 [details] [diff] [review]
fix v1.1

We can remove the "lock" prefix from SunOS xchg on checkin.

Comment 14

[Wan-Teh Chang]

Attachment: Remove the lock prefix for xchg in existing files

There are several other copies of the x86 assembly
code that also need to be fixed.

Mark, does the cmpxchg instruction need the lock prefix?
We have "lock cmpxchg" in the *comments* in some of those
files.  I don't want to comments to become out-of-sync.

Comment 15

[Mark Mentovai]

Comment on attachment 207785 [details] [diff] [review]
Remove the lock prefix for xchg in existing files

CMPXCHG doesn't imply LOCK, so you're correct not to change the commented alternate implementations.

Note: your patch and Josh's both remove the same LOCK from the Linux x86* implementations.

I was surprised to only see the gcc-style asm blocks modified for the Windows implementation, it turns out that the MSVC asm blocks already omit the explicit LOCK when using XCHG.

Comment 16

[Josh Aas]

It should be easy to reconcile the parts of the two patches that overlap. Wan-Teh, can you check this stuff in? I don't think I can do it on the trunk.

Comment 17

[Josh Aas]

Comment on attachment 207747 [details] [diff] [review]
fix v1.1

Requesting branch approvals. This is an Intel-Mac-only fix.

Comment 18

[Wan-Teh Chang]

Comment on attachment 207785 [details] [diff] [review]
Remove the lock prefix for xchg in existing files

Mark, thanks a lot for the thorough code review!

I checked in this patch on the NSPR trunk (NSPR 4.7):

Checking in pr/src/md/os2/os2emx.s;
/cvsroot/mozilla/nsprpub/pr/src/md/os2/os2emx.s,v  <--  os2emx.s
new revision: 1.3; previous revision: 1.2
done
Checking in pr/src/md/os2/os2vacpp.asm;
/cvsroot/mozilla/nsprpub/pr/src/md/os2/os2vacpp.asm,v  <--  os2vacpp.asm
new revision: 1.7; previous revision: 1.6
done
Checking in pr/src/md/unix/os_Linux_x86.s;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_Linux_x86.s,v  <--  os_Linux_x86.s
new revision: 3.8; previous revision: 3.7
done
Checking in pr/src/md/unix/os_Linux_x86_64.s;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_Linux_x86_64.s,v  <--  os_Linux_x86_6
4.s
new revision: 1.2; previous revision: 1.1
done
Checking in pr/src/md/unix/os_SunOS_x86.s;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_SunOS_x86.s,v  <--  os_SunOS_x86.s
new revision: 3.10; previous revision: 3.9
done
Checking in pr/src/md/unix/os_SunOS_x86_64.s;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_SunOS_x86_64.s,v  <--  os_SunOS_x86_6
4.s
new revision: 1.2; previous revision: 1.1
done
Checking in pr/src/md/windows/ntmisc.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/ntmisc.c,v  <--  ntmisc.c
new revision: 3.21; previous revision: 3.20
done

and on the NSPRPUB_PRE_4_2_CLIENT_BRANCH (Gecko 1.9 alpha):

Checking in pr/src/md/os2/os2emx.s;
/cvsroot/mozilla/nsprpub/pr/src/md/os2/os2emx.s,v  <--  os2emx.s
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in pr/src/md/os2/os2vacpp.asm;
/cvsroot/mozilla/nsprpub/pr/src/md/os2/os2vacpp.asm,v  <--  os2vacpp.asm
new revision: 1.1.4.5; previous revision: 1.1.4.4
done
Checking in pr/src/md/unix/os_Linux_x86.s;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_Linux_x86.s,v  <--  os_Linux_x86.s
new revision: 3.6.4.2; previous revision: 3.6.4.1
done
Checking in pr/src/md/unix/os_Linux_x86_64.s;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_Linux_x86_64.s,v  <--  os_Linux_x86_6
4.s
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in pr/src/md/unix/os_SunOS_x86.s;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_SunOS_x86.s,v  <--  os_SunOS_x86.s
new revision: 3.8.4.2; previous revision: 3.8.4.1
done
Checking in pr/src/md/unix/os_SunOS_x86_64.s;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_SunOS_x86_64.s,v  <--  os_SunOS_x86_6
4.s
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in pr/src/md/windows/ntmisc.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/ntmisc.c,v  <--  ntmisc.c
new revision: 3.15.4.6; previous revision: 3.15.4.5
done

Comment 19

[Wan-Teh Chang]

Attachment: fix v1.2

I removed the changes to os_Linux_x86.s and os_Linux_x86_64.s
from the patch because they don't need to be backported to the
MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.

I made a minor change to _darwin.h and added some comments to
os_Darwin_x86.s.

I checked in this patch on the NSPR trunk (NSPR 4.7):

Enter passphrase for key '/cygdrive/c/Documents and Settings/wtc/.ssh/id_dsa':
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.210; previous revision: 1.209
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.212; previous revision: 1.211
done
Checking in pr/include/md/_darwin.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_darwin.h,v  <--  _darwin.h
new revision: 3.19; previous revision: 3.18
done
RCS file: /cvsroot/mozilla/nsprpub/pr/src/md/unix/os_Darwin_x86.s,v
done
Checking in pr/src/md/unix/os_Darwin_x86.s;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_Darwin_x86.s,v  <--  os_Darwin_x86.s
initial revision: 1.1
done

and the NSPRPUB_PRE_4_2_CLIENT_BRANCH (Gecko 1.9 alpha):

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.78.2.128; previous revision: 1.78.2.127
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.83.2.126; previous revision: 1.83.2.125
done
Checking in pr/include/md/_darwin.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_darwin.h,v  <--  _darwin.h
new revision: 3.10.4.9; previous revision: 3.10.4.8
done
Checking in pr/src/md/unix/os_Darwin_x86.s;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_Darwin_x86.s,v  <--  os_Darwin_x86.s
new revision: 1.1.2.2; previous revision: 1.1.2.1
done

Comment 20

[Daniel Veditz [:dveditz]]

Assuming this won't make 1.8.0.1 at this point, but if you get a reviewed patch tested on the trunk, etc, you can request approval again.

Comment 21

[Josh Aas]

This patch (v1.2) is ready to go for Intel Macs on the 1.5.0.1 branch. The wtc review request is not really necessary since wtc posted the patch, already reviewed the exact same code above, and mark only requested sr so it would be more clear that it was reviewed to drivers.

I recommend approving fix v1.2 for the 1.5.0.1 branch.

Comment 22

[Mark Mentovai]

Comment on attachment 207998 [details] [diff] [review]
fix v1.2

Looks good.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 207998 [details] [diff] [review]
fix v1.2

a=dveditz for drivers

This is an intel-mac fix only, and trunk baking wouldn't help here.

Comment 24

[Josh Aas]

Landed fix v1.2 on MOZILLA_1_8_0_BRANCH and MOZILLA_1_8_BRANCH

Comment 25

[Wan-Teh Chang]

I checked in "fix v1.2" on the NSPR_4_6_BRANCH (NSPR 4.6.2).

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.197.2.2; previous revision: 1.197.2.1
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.199.2.2; previous revision: 1.199.2.1
done
Checking in pr/include/md/_darwin.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_darwin.h,v  <--  _darwin.h
new revision: 3.18.2.1; previous revision: 3.18
done
Checking in os_Darwin_x86.s;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_Darwin_x86.s,v  <--  os_Darwin_x86.s
new revision: 1.1.8.2; previous revision: 1.1.8.1
done