Closed Bug 429387 Opened 12 years ago Closed 12 years ago

use kuser helpers for js_CompareAndSwap

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dougt, Assigned: vlad)

References

Details

(Keywords: mobile)

Attachments

(1 file, 2 obsolete files)

on linux arm, __kernel_cmpxchg is provided which may prove to be a faster atomic compare and swap routine.  we have seen noticeable gains when using this function to implement nspr locking (bug 405929).

A few comments about the patch:

1) we are using a configure variable because it is not possible to test for this function's existence as we are building in a "scratchbox" and not on the actual hardware.

2) there is concern about using __kernel_cmpxchg on older ARM processors due to the possibility of spurious false negatives.  (see 405929 for the description of the problem).  We are not sure it continues to exists on the devices we currently care about.
Attached patch patch v.1 (obsolete) — Splinter Review
(In reply to comment #0)
> function to implement nspr locking (bug 405929).

I assume you meant bug 405992
Is __kernel_cmpxchg a compiler intrinsic, library function, or kernel system call?

/be
kernel function mapped as:

   #define __kernel_cmpxchg (*(__kernel_cmpxchg_t *)0xffff0fc0)
Why
  +       (defined(LINUX) && defined(__arm__) && defined(USE_ARM_KUSER)) ||
instead of just defined(USE_ARM_KUSER)?

Brendan, __kernel_cmpxchg is a new 'user helper' in linux, where certain functions are mapped at fixed addresses in each userspace process to allow them to do quickly do things that might otherwise have required kernel interaction.  It's done that way to allow the kernel to change the actual instructions executed based on cpu type and kernel internals.

Another kernel function that might be of interest is __kernel_get_tls, if TLS was set via the set_tls syscal: http://svn.dd-wrt.com:8000/dd-wrt/browser/src/linux/xscale/linux-2.6.24/arch/arm/kernel/entry-armv.S?rev=8887#L844
Attached patch we'll need this as well (obsolete) — Splinter Review
This is related, and will need to be merged -- we need to pass the right configure flag to the in-tree nspr, as well as adding the kuser helpers to JS.
vlad: right, USE_ARM_KUSER implies ARM and LINUX.
nspr has a similar support for its locking code.  They changed --enable-arm-kuser to --with-arm-kuser.  
Updated and merged the two patches; this passes --with-arm-kuser down to NSPR is --with-arm-kuser is specified on the gecko configure line; it also adds the same kuser helper code for js_CompareAndSwap.  Seems to work fine; I don't have perf numbers from an actual device, though.
Attachment #316096 - Attachment is obsolete: true
Attachment #318073 - Attachment is obsolete: true
Attachment #320396 - Flags: superreview?(brendan)
Attachment #320396 - Flags: review?(shaver)
vlad, I am worried about spurious false negatives.  My  patch didn't worry about it, but i was worried about it (see comment #1). In NSPR, we do a loop over __kernel_cmpxchg until it returns success:

+    do {
+        ov = *vp;
+    } while (__kernel_cmpxchg(ov, nv, vp));
+

Do we have to do something similar?
Assignee: doug.turner → vladimir
No; the NSPR methods are lock and unlock, JS just needs compare-and-swap which returns true if the swap occurred, or false if it didn't.  JS layers its own lock/unlock on top of that.  (The kuser helper returns the opposite of what js_CompareAndSwap is supposed to do, so the code that's there now is correct.)
Comment on attachment 320396 [details] [diff] [review]
updated/merged patch

r=shaver (js doesn't need sr).  Wanna get a quick eye on the build part, vacuous comment and all?
Attachment #320396 - Flags: superreview?(brendan)
Attachment #320396 - Flags: review?(shaver)
Attachment #320396 - Flags: review+
Flags: wanted1.9.0.x+
Keywords: mobile
Comment on attachment 320396 [details] [diff] [review]
updated/merged patch

r=me on the configure bits
Attachment #320396 - Flags: review+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 446169
Flags: in-testsuite-
Flags: in-litmus-
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.