Last Comment Bug 322287 - Implement NSPR atomic routines in x86 assembly code for Intel Macs
: Implement NSPR atomic routines in x86 assembly code for Intel Macs
Status: RESOLVED FIXED
: fixed1.8.0.1, fixed1.8.1
Product: NSPR
Classification: Components
Component: NSPR (show other bugs)
: 4.6
: PowerPC Mac OS X
: -- normal (vote)
: 4.6.2
Assigned To: Josh Aas
: Wan-Teh Chang
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-01-03 17:42 PST by Wan-Teh Chang
Modified: 2006-01-10 16:58 PST (History)
3 users (show)
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
work snapshot (4.65 KB, patch)
2006-01-04 10:12 PST, Josh Aas
no flags Details | Diff | Review
fix v1.0 (4.66 KB, patch)
2006-01-05 23:28 PST, Josh Aas
mark: review+
Details | Diff | Review
fix v1.1 (6.45 KB, patch)
2006-01-06 11:53 PST, Josh Aas
mark: review+
wtc: superreview+
Details | Diff | Review
Remove the lock prefix for xchg in existing files (3.79 KB, patch)
2006-01-06 17:30 PST, Wan-Teh Chang
mark: review+
Details | Diff | Review
fix v1.2 (5.99 KB, patch)
2006-01-09 10:52 PST, Wan-Teh Chang
jaas: review+
mark: superreview+
dveditz: approval1.8.0.1+
dveditz: approval1.8.1+
Details | Diff | Review

Description Wan-Teh Chang 2006-01-03 17:42:49 PST
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 2006-01-03 17:50:20 PST
I'll take a crack at it.
Comment 2 Mark Mentovai 2006-01-03 19:18:11 PST
Coincidentally, I glanced at this yesterday, and it looked like the Linux code would be a near drop-in.
Comment 3 Josh Aas 2006-01-04 10:12:09 PST
Created attachment 207513 [details] [diff] [review]
work snapshot

Just putting this here to save my work and pass between machines. Not asking for review.
Comment 4 Josh Aas 2006-01-05 23:28:24 PST
Created attachment 207701 [details] [diff] [review]
fix v1.0

This should work. It compiles and works fine on my Intel Mac.
Comment 5 Josh Aas 2006-01-05 23:30:51 PST
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 2006-01-06 08:21:14 PST
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 2006-01-06 10:39:42 PST
(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 2006-01-06 11:01:14 PST
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 2006-01-06 11:09:18 PST
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 2006-01-06 11:30:13 PST
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 2006-01-06 11:53:28 PST
Created attachment 207747 [details] [diff] [review]
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 2006-01-06 11:56:58 PST
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 2006-01-06 12:47:43 PST
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 2006-01-06 17:30:55 PST
Created attachment 207785 [details] [diff] [review]
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 2006-01-06 19:40:44 PST
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 2006-01-07 13:14:45 PST
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 2006-01-07 13:15:30 PST
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 2006-01-09 10:00:56 PST
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 2006-01-09 10:52:51 PST
Created attachment 207998 [details] [diff] [review]
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] 2006-01-09 12:23:21 PST
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 2006-01-09 12:29:09 PST
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 2006-01-09 12:35:10 PST
Comment on attachment 207998 [details] [diff] [review]
fix v1.2

Looks good.
Comment 23 Daniel Veditz [:dveditz] 2006-01-10 14:04:37 PST
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 2006-01-10 15:20:44 PST
Landed fix v1.2 on MOZILLA_1_8_0_BRANCH and MOZILLA_1_8_BRANCH
Comment 25 Wan-Teh Chang 2006-01-10 16:58:26 PST
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

Note You need to log in before you can comment on or make changes to this bug.