30902 - True atomic implementation of NSPR atomic routines for Unix/Linux on x86

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P3)

Description

[Wan-Teh Chang]

We should implement the NSPR atomic routines (increment,
decrement, set, and add) in x86 assembly code for Unix/Linux
on x86 processors.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed x86 assembly implementation for Linux. Need the next patch.

Comment 2

[Wan-Teh Chang]

Attachment: Patch to use x86 assembly implementation of atomic routines on Linux.

Comment 3

[Wan-Teh Chang]

To test my code on Linux/x86, save the first
attachment (id=6225) as mozilla/nsprpub/pr/src/md/unix/os_Linux_x86.s,
then apply the second attachment (id=6226) as
a patch to your mozilla source tree.  You only
need to rebuild mozilla/nsprpub.

As I don't really know x86 assembly language,
I'd appreciate it if you could review the x86
assembly code (attachment 6225 [details]).  Hopefully the
comments in there make it self-explanatory.

Comment 4

[Warren Harris]

I'm not qualified to review the assembly, but just curious if you've done any 
profiling to see what the difference looks like. 

Cc'ing morse -- maybe he can review this.

Comment 5

[Wan-Teh Chang]

No, I haven't done any profiling to see what
the difference looks like.

Comment 6

[Mike Shaver (:shaver emeritus)]

I had some Linux kernel types look at it, and they thought it could be
improved.  In fact, they recommended that we ask to get the Linux kernel
versions (http://lxr.linux.no/source/include/asm-i386/atomic.h) licensed under
the MPL for our modification and use.

I've sent some mail, and I'll let you know what I hear back.

Comment 7

[srinivas]

You could use inline assembly in a C source file; the x86 implementation of 
these atomic ops are in ntmisc.c

Comment 8

[Wan-Teh Chang]

My assembly code (attachment 6225 [details]) is based on the
MSVC inline assembly code in ntmisc.c. I did not use
the gcc inline assembly code in ntmisc.c because it
hasn't been tested, not to mention that I can't
understand the strange syntax.  It doesn't seem to
be correct either. For example, the gcc inline
assembly codes of _PR_MD_ATOMIC_INCREMENT and
_PR_MD_ATOMIC_DECREMENT are identical.

Comment 9

[Wan-Teh Chang]

The inline assembly code for gcc on Windows
that Srinivas referred to is in the file
mozilla/nsprpub/pr/src/md/windows/ntmisc.c,
ifdef'd with __GNUC__.  Note that some of
that code may be incorrect, for example, the
the gcc inline assembly codes of
_PR_MD_ATOMIC_INCREMENT and
_PR_MD_ATOMIC_DECREMENT are identical.

Comment 10

[robey]

Attachment: gcc impl of atomic ops for linux, using __asm__()

Comment 11

[robey]

(added patch above)
wan-teh, after looking at the linux kernel impl, and the solaris/ultrasparc 
impl, and poking into intel man pages, i came up with my own impl using gcc's 
__asm__() syntax.  then i looked at your impl.

yours looks fine to me.  the only thing i would change is that you probably can 
use incl/decl instead of add/sub/mov.  i prefer to use gcc's asm stuff so that 
gcc will tackle the overhead, but that's a personal preference, and your asm 
code looks like it's roughly equivalent to (and a bit less redundant than) the 
gcc asm output.

in other words, "ship it". :)

Comment 12

[Wan-Teh Chang]

Robey, as we discussed in email, I can't use incl/decl
because the current spec. of PR_AtomicIncrement and
PR_AtomicDecrement says they return the result, not
just the sign of the result, of the increment/decrement.

Checked in my patch (attachments 6225, 6226) on the
main trunk of NSPR.  Note that SeaMonkey is *not*
pulling the tip of NSPR.

/cvsroot/mozilla/nsprpub/pr/include/md/_linux.h, revision 3.26
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_Linux_x86.s, initial revision: 3.1
/cvsroot/mozilla/nsprpub/pr/src/md/unix/Makefile, revision 3.29
/cvsroot/mozilla/nsprpub/pr/src/md/unix/Makefile.in, revision 1.14
/cvsroot/mozilla/nsprpub/pr/src/md/unix/objs.mk, revision 3.29

Comment 13

[Wan-Teh Chang]

Removed the 'nop' instructions that accidentally slipped in.
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_Linux_x86.s, revision 3.2

Comment 14

[Wan-Teh Chang]

I added an alternative implementation of PR_AtomicSet
(using cmpxchg instead of xchg) in the comments.
Also replaced a memory-to-register move by a
register-to-register move in PR_AtomicAdd.

/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_Linux_x86.s, revision 3.3

Comment 15

[Wan-Teh Chang]

Merged the patch onto the NSPRPUB_RELEASE_4_0_BRANCH.
/cvsroot/mozilla/nsprpub/pr/include/md/_linux.h, revision 3.23.10.3
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_Linux_x86.s, revision 3.3.2.1 (new 
file)
/cvsroot/mozilla/nsprpub/pr/src/md/unix/Makefile, revision 3.28.6.1
/cvsroot/mozilla/nsprpub/pr/src/md/unix/Makefile.in, revision 1.13.6.1
/cvsroot/mozilla/nsprpub/pr/src/md/unix/objs.mk, revision 3.28.8.1

Comment 16

[Wan-Teh Chang]

I merged the patch onto the NSPRPUB_CLIENT_BRANCH.
/cvsroot/mozilla/nsprpub/pr/include/md/_linux.h, revision 3.24.2.2
/cvsroot/mozilla/nsprpub/pr/src/md/unix/Makefile, revision 3.28.20.1
/cvsroot/mozilla/nsprpub/pr/src/md/unix/Makefile.in, revision 1.13.20.1
/cvsroot/mozilla/nsprpub/pr/src/md/unix/objs.mk, revision 3.28.22.1
/cvsroot/mozilla/nsprpub/pr/src/md/unix/os_Linux_x86.s, revision 3.3.4.1 (new 
file)