Merge lp:~epics-core/epics-base/spinlockfix into lp:~epics-core/epics-base/3.15

[mdavidsaver]

To ensure proper OS independent behavior for spinlocks, disable task preemption on RTOS.

RTEMS/vxWorks implementations already disable interrupts.
However, if while holding a spinlock, a function call is made which wakes up a higher priority task,
a context switch will occur while the spinlock remains locked.
Thankfully the context switch will re-enable interrupts.
However, since epicsSpinLock for these OSs only disables interrupts,
the second task (or any other which happens to run) can successfuly
enter the critical seciton.

This branch changes epicsSpinLock() to also disable task preemption
on RTEMS/vxWorks. Further an assertion is added as a last line of
defence against silent violation of mutual exclusion.

Also, the POSIX implementation of spinlocks is vulnerable to priority inversion, so use the mutex based implementation with thread priorities are enabled.

Add epicsSpinMustCreate().

[Andrew Johnson (anj)]

Ping!

[Andrew Johnson (anj)]

In the RTEMS and VxWorks implementations of epicsSpinLock() and epicsSpinUnlock(), is it wise to make an assertion with both interrupts and thread dispatch disabled? Won't that result in a silent lock-up of the IOC if the assertion fails? Not helpful in the presence of a nasty bug that sprays memory, or of a bad memory chip.

The cantProceed() messages in the Posix implementation seem a little minimalistic, a little more detail might be helpful (put yourself in the position of an IOC developer who gets one of these, how useful are these to finding out what happened).

- Andrew

[Eric Norum (wenorum)]

I’m not a fan of assertions. Either a check is important or it isn’t. Not something to come and go between debugging and production versions.

On Jul 21, 2014, at 2:09 PM, Andrew Johnson <email address hidden> wr

> In the RTEMS and VxWorks implementations of epicsSpinLock() and epicsSpinUnlock(), is it wise to make an assertion with both interrupts and thread dispatch disabled? Won't that result in a silent lock-up of the IOC if the assertion fails? Not helpful in the presence of a nasty bug that sprays memory, or of a bad memory chip.
>
> The cantProceed() messages in the Posix implementation seem a little minimalistic, a little more detail might be helpful (put yourself in the position of an IOC developer who gets one of these, how useful are these to finding out what happened).
>
> - Andrew
> --
> https://code.launchpad.net/~epics-core/epics-base/spinlockfix/+merge/220845
> Your team EPICS Core Developers is requested to review the proposed merge of lp:~epics-core/epics-base/spinlockfix into lp:epics-base.

--
Eric Norum
<email address hidden>

[mdavidsaver (mdavidsaver)]

Would it be satisify you guys if "assert(X)" became "if(!X) cantProceed(Y)" where Y would be a message indicating that the some code slept while holding a spinlock?

> The cantProceed() messages in the Posix implementation ...

Do you mean the case where pthread_spin_lock() fails? The checkStatus() macro will already print strerror().

[Andrew Johnson (anj)]

As long as you're not going to be calling cantProceed() with interrupts and thread dispatch disabled that would be fine by me.

Thinking aloud, the error messages on a failure currently look something like this:

epicsSpin pthread_spin_lock failed: error <strerror>
epicsSpinLock pspin
Thread <name> (<id>) can't proceed, suspending.

epicsSpin pthread_mutex_lock failed: error <strerror>
epicsSpinLock pmutex
Thread <name> (<id>) can't proceed, suspending.

The middle line is actually redundant since the first line already tells us which implementation the code was built with. I would just pass NULL into the cantProceed() call instead.

[mdavidsaver (mdavidsaver)]

> As long as you're not going to be calling cantProceed() with interrupts and
> thread dispatch disabled that would be fine by me.

Hanging in an ISR isn't nice, but neither is entering the critical section w/o the lock. We (or rather our users) will lose either way.

Which way should we lose?

> ...I would just pass NULL into the cantProceed() call instead.

Ok.

[Andrew Johnson (anj)]

I don't think you need to lose, you're going to be suspending this thread if you detect a problem so the user's code will only enter the critical section if they manually resume the thread. You should just unlock everything before you call cantProceed().

Ah, but if we're in interrupt context then we do have a problem, it's illegal to call cantProceed() in that context, and we don't have a status return value that we can use to tell the caller that the lock operation failed. Maybe we shouldn't even bother with the locked member and trying to detect this condition at all?

[mdavidsaver (mdavidsaver)]

> ... Maybe we
> shouldn't even bother with the locked member and trying to detect this
> condition at all?

Something is required. Silently violating mutual exclusion is very difficult to find (ISRs are enough fun as it is).

For the present I'll change this to print a warning instead of cantProceed() in the ISR case.

[lp:~epics-core/epics-base/spinlockfix]

12483. By mdavidsaver on 2014-07-23

epicsSpin: better error messages when mis-use is detected

[mdavidsaver (mdavidsaver)]

I've replace the usage of assert() with explicit error messages. In the ISR case it returns, otherwise cantProceed() is called. In both cases interrupts and preemption are re-enabled first.

[lp:~epics-core/epics-base/spinlockfix]

12484. By mdavidsaver on 2014-07-24

epicsSpin: remove redundant cantProceed() messages

[mdavidsaver (mdavidsaver)]

> > ...I would just pass NULL into the cantProceed() call instead.
>
> Ok.

This is done.

review: Needs Resubmitting

[lp:~epics-core/epics-base/spinlockfix]

12485. By Andrew Johnson on 2014-07-24

Final spinlock tidying-up

* Abort epicsSpinTest() if epicsSpinCreate() returns NULL
* Adjust RELEASE_NOTES that describe the implementations.

[Andrew Johnson (anj)]

I just committed a couple of changes to epicsSpinTest.c that abort if epicsSpinCreate() returns NULL, and added one use of epicsSpinMustCreate(). I also adjusted the wording in the Release Notes describing the different implementations.

There doesn't seem to be any documentation for the epicsSpin API in the AppDevGuide at all. Not sure whose omission that is, possibly Ralph's?

Then I tried running epicsSpinTest on vxWorks-68040:

5.5.2> epicsSpinTest
1..18
ok 1 - epicsSpinTryLock(verify.spin) == 0
Deadlock in epicsSpinLock(). Either recursive lock, missing unlock, or locked by sleeping thread.Thread verifyTryLockThread (0x1ecda84) can't proceed, suspending.

Huh? Tried the same on an unmodified Base, I get one test failing:

5.5.2> epicsSpinTest
1..18
ok 1 - epicsSpinTryLock(verify.spin) == 0
not ok 2 - epicsSpinTryLock(pVerify->spin)
ok 3 - epicsEventWait(verify.done) == epicsEventWaitOK
# spinThread 0 starting
ok 4 - spinThread 0 epicsSpinLock taken
...
[Remaining tests all passed, although the performance test measured a time of 0.000000 microseconds]

The verifyTryLock() test as written doesn't seem to even be valid any more. It is creating and taking a spinLock in the first testOk1(), then with the lock still held it creates a new thread and tries to take the lock in that thread, and expects that try to fail. However you were probably expecting that the new thread shouldn't even be able to start since you called taskLock(), but the man-page for taskLock() says this:

    The task that calls this routine will be the only task that
    is allowed to execute, unless the task explicitly gives up
    the CPU by making itself no longer ready.

The main thread called epicsEventWait(verify.done) after creating that new thread, so it made itself no longer ready thus allowing the VxWorks kernel to start task switching again.

The new thread then calls epicsSpinTryLock() which in the UP implementations isn't expecting to be able to run while the lock is held so it doesn't even try to implement a proper tryLock() operation, and it always returned OK before. However now it notices that the lock had already been taken, so it suspends.

I modified the test code to create the new thread before taking the lock and wait on an epicsEvent before trying to take the lock, but it made no difference to the result; the taskLock() still gets broken as soon as the main thread suspends.

Is this just a problem in our test code, or do we need to change our implementation to allow for the taskLock() rule? Does RTEMS have an equivalent rule for tasks that have called _Thread_Disable_dispatch()?

review: Needs Fixing

[mdavidsaver (mdavidsaver)]

> The main thread called epicsEventWait(verify.done) after creating that new thread, so it made itself no longer ready thus allowing the VxWorks kernel to start task switching again.

This seems like a clear violation of the "don't sleep while holding a
spinlock" rule. Which is what the "Deadlock in epicsSpinLock()" test is
designed to catch.

[Andrew Johnson (anj)]

Agreed, so the current test code is invalid, but I still don't think the UP implementations of epicsSpinTryLock() should ever be able to call cantProceed() even in this circumstance — the point of providing the TryLock() versions is so the user can write code that they know will never block.

[mdavidsaver (mdavidsaver)]

> ... I still don't think the UP
> implementations of epicsSpinTryLock() should ever be able to call
> cantProceed() even in this circumstance ...

On UP RTOSs, the error in question can be triggered by three conditions, which can't be distinguished ATM.

1. Attempt at recursive locking
2. User code forgot epicsSpinUnlock()
3. User code slept while holding the spinlock

The only one which I imagine epicsSpinTryLock() handling sanely is #1, which would require that the epicsSpinId structure track the current thread ID with a special case for an ISR.

The other two are clear mis-uses of the API which are unrecoverable (#2 is a deadlock, #3 violates mutual exclusion).

So if good behavior is out the window, I'd prefer to fail early and visibly.

[mdavidsaver (mdavidsaver)]

> ... I still don't think the UP
> implementations of epicsSpinTryLock() should ever be able to call
> cantProceed() even in this circumstance ...

Ok, so I was thinking about this from the context of epicsSpinLock(). The try lock does have an error path which can be used. I would argue that we shouldn't call this defined behavior as I don't think we want the overhead of detecting recursive calls in other implementations.

[Andrew Johnson (anj)]

If epicsSpinLock() would block [for any reason], epicsSpinTryLock() should immediately return a non-zero value in the same situation (EBUSY/EWOULDBLOCK and maybe even EDEADLOCK if we can detect that, I don't particularly like our current 1 return value for "busy").

We don't have to promise to detect recursion and deadlocks (although we should if we can easily do so), but I do maintain that the TryLock() call should never wait or block.

[lp:~epics-core/epics-base/spinlockfix]

12486. By mdavidsaver on 2014-07-25

epicsSpinTest: fix verifyTryLock()

avoid sleeping with a spinlock held.
Now test only works on SMP systems.

12487. By mdavidsaver on 2014-07-25

epicsSpin: try lock return non-blocking

Avoid cantProceed() in try lock, even for undefined behavior.

[mdavidsaver (mdavidsaver)]

> If epicsSpinLock() would block [for any reason], epicsSpinTryLock() should
> immediately return a non-zero value

Done.

I also fixed the trylock test for SMP systesm, and skip it for UP systems.

[lp:~epics-core/epics-base/spinlockfix]

12488. By Andrew Johnson on 2014-07-25

Fix epicsAtomic headers when used from C code

Several C++ and C99-isms crept in.

12489. By Andrew Johnson on 2014-07-25

Fix epicsSpinTest.c spinThread tests

Runs many more rounds, without blocking with the lock held.

[Andrew Johnson (anj)]

Did you try building for RTEMS? My epicsSpinTest.c build failed on both
VxWorks and RTEMS due to problems with the epicsAtomic.h headers not
having been tested from C code before; they had a couple of C++isms that
I just fixed and committed.

However we still have a problem. Here's the output from epicsSpinTest on
VxWorks 5.5.2 from a 68040:

5.5.2> epicsSpinTest
1..16
ok 1 # SKIP verifyTryLock() only for SMP systems
# spinThread 0 starting
ok 2 - spinThread 0 epicsSpinLock taken
# spinThread 1 starting
# spinThread 2 starting
Deadlock in epicsSpinLock(). Either recursive lock, missing unlock, or
locked by sleeping thread.Thread task1 (0x1eccc48) can't proceed,
suspending.
Deadlock in epicsSpinLock(). Either recursive lock, missing unlock, or
locked by sleeping thread.Thread task2 (0x1ecba4c) can't proceed,
suspending.
ok 3 - spinThread 0 epicsSpinLock taken
ok 4 - spinThread 0 epicsSpinLock taken
ok 5 - spinThread 0 epicsSpinLock taken
ok 6 - spinThread 0 epicsSpinLock taken
# spinThread 0 exiting
# lock()*1/unlock()*1 takes 4.416667 microseconds

Planned 16 tests but only ran 6

    Results
    =======
       Tests: 6
      Passed: 6 = 100.00%
     Skipped: 1 = 16.67%
value = 2 = 0x2

Both task1 and task2 still exist and are suspended at the end of the
test. Same results on VxWorks 6.9 from a PowerPC (but with better lock
performance, 0.116667 microseconds).

Looks at the code...

void spinThread(void *arg)
{
    info *pinfo = (info *) arg;
    testDiag("spinThread %d starting", pinfo->threadnum);
    while (pinfo->quit--) {
        epicsSpinLock(pinfo->spin);
        testPass("spinThread %d epicsSpinLock taken", pinfo->threadnum);
        epicsThreadSleep(.1);

Lookey-there, not one but two blocking calls made while owning the
spinLock...

I reworked that spinThread test to add a shared counter variable
protected by the lock; to not output on every operation, increased the
number of rounds, and to signal the main thread when finished so it
doesn't have to guess (which it gets wrong). Oh, and it releases memory
as well just to be nice.

Please check my committed result.

- Andrew
--
Advertising may be described as the science of arresting the human
intelligence long enough to get money from it. -- Stephen Leacock

[lp:~epics-core/epics-base/spinlockfix]

12490. By mdavidsaver on 2014-07-25

epicsSpinTest: plug some leaks

12491. By mdavidsaver on 2014-07-25

epicsSpinTest: add to libCom test harness

[mdavidsaver (mdavidsaver)]

Now passes on RTEMS 4.9. Fixed remaining memory leaks in epicsSpinTest and added it to the libCom test harness (was in the Makefile, but not being run)

> ***** epicsSpinTest *****
> 1..2
> ok 1 # SKIP verifyTryLock() only for SMP systems
> # spinThread 0 starting
> # spinThread 1 starting
> # spinThread 2 starting
> # spinThread 0 exiting
> # spinThread 1 exiting
> # spinThread 2 exiting
> ok 2 - Loops run = 1500 (expecting 1500)
> # lock()*1/unlock()*1 takes 0.240219 microseconds
>
> Results
> =======
> Tests: 2
> Passed: 2 = 100.00%
> Skipped: 1 = 50.00%

[Andrew Johnson (anj)]

Passes on VxWorks, merging.

review: Approve

=== modified file 'documentation/RELEASE_NOTES.html'
--- documentation/RELEASE_NOTES.html	2014-06-12 19:47:42 +0000
+++ documentation/RELEASE_NOTES.html	2014-07-25 22:18:29 +0000
@@ -136,12 +136,13 @@
 <p>The new header file epicsSpin.h adds a portable spin-locks API which is
 intended for locking very short sections of code (typically one or two lines of
 C or C++) to provide a critical section that protects against race conditions.
-On Posix platforms this uses the pthread_spinlock_t type if it's available but
-falls back to a pthread_mutex_t if not; on the UP VxWorks and RTEMS platforms
-the implementations lock out the CPU interrupts; the default implementation
-(used where no better implementation is available for the platform) uses an
-epicsMutex. Spin-locks may not be taken recursively, and the code inside the
-critical section should always be short and deterministic.</p>
+On Posix platforms this uses the pthread_spinlock_t type if it's available and
+the build is not configured to use Posix thread priorities, but otherwise it
+falls back to a pthread_mutex_t. On the UP VxWorks and RTEMS platforms the
+implementations lock out CPU interrupts and disable task preemption while a
+spin-lock is held. The default implementation (used when no other implementation
+is provided) uses an epicsMutex. Spin-locks may not be taken recursively, and
+the code inside the critical section should be short and deterministic.</p>
 
 <h3>Improvements to aToIPAddr()</h3>
 

=== modified file 'src/libCom/osi/Makefile'
--- src/libCom/osi/Makefile	2012-10-29 19:25:23 +0000
+++ src/libCom/osi/Makefile	2014-07-25 22:18:29 +0000
@@ -103,6 +103,7 @@
 #POSIX thread priority scheduling flag
 THREAD_CPPFLAGS_NO += -DDONT_USE_POSIX_THREAD_PRIORITY_SCHEDULING
 osdThread_CPPFLAGS += $(THREAD_CPPFLAGS_$(USE_POSIX_THREAD_PRIORITY_SCHEDULING))
+osdSpin_CPPFLAGS += $(THREAD_CPPFLAGS_$(USE_POSIX_THREAD_PRIORITY_SCHEDULING))
 
 Com_SRCS += osdThread.c
 Com_SRCS += osdThreadExtra.c

=== modified file 'src/libCom/osi/epicsAtomicDefault.h'
--- src/libCom/osi/epicsAtomicDefault.h	2011-09-01 17:25:53 +0000
+++ src/libCom/osi/epicsAtomicDefault.h	2014-07-25 22:18:29 +0000
@@ -34,9 +34,11 @@
 #ifndef EPICS_ATOMIC_INCR_INTT
 EPICS_ATOMIC_INLINE int epicsAtomicIncrIntT ( int * pTarget )
 {
-    EpicsAtomicLockKey key; 
+    EpicsAtomicLockKey key;
+    int result;
+
     epicsAtomicLock ( & key );
-    const int result = ++(*pTarget);
+    result = ++(*pTarget);
     epicsAtomicUnlock ( & key );
     return result;
 }
@@ -46,8 +48,10 @@
 EPICS_ATOMIC_INLINE size_t epicsAtomicIncrSizeT ( size_t * pTarget )
 {
     EpicsAtomicLockKey key;
+    size_t result;
+
     epicsAtomicLock ( & key );
-    const size_t result = ++(*pTarget);
+    result = ++(*pTarget);
     epicsAtomicUnlock ( & key );
     return result;
 }
@@ -60,8 +64,10 @@
 EPICS_ATOMIC_INLINE int epicsAtomicDecrIntT ( int * pTarget )
 {
     EpicsAtomicLockKey key;
+    int result;
+
     epicsAtomicLock ( & key );
-    const int result = --(*pTarget);
+    result = --(*pTarget);
     epicsAtomicUnlock ( & key );
     return result;
 }
@@ -71,8 +77,10 @@
 EPICS_ATOMIC_INLINE size_t epicsAtomicDecrSizeT ( size_t * pTarget )
 {
     EpicsAtomicLockKey key;
+    size_t result;
+
     epicsAtomicLock ( & key );
-    const size_t result = --(*pTarget);
+    result = --(*pTarget);
     epicsAtomicUnlock ( & key );
     return result;
 }
@@ -85,8 +93,10 @@
 EPICS_ATOMIC_INLINE int epicsAtomicAddIntT ( int * pTarget, int delta )
 {
     EpicsAtomicLockKey key;
+    int result;
+
     epicsAtomicLock ( & key );
-    const int result = *pTarget += delta;
+    result = *pTarget += delta;
     epicsAtomicUnlock ( & key );
     return result;
 }
@@ -96,8 +106,10 @@
 EPICS_ATOMIC_INLINE size_t epicsAtomicAddSizeT ( size_t * pTarget, size_t delta )
 {
     EpicsAtomicLockKey key;
+    size_t result;
+
     epicsAtomicLock ( & key );
-    const size_t result = *pTarget += delta;
+    result = *pTarget += delta;
     epicsAtomicUnlock ( & key );
     return result;
 }
@@ -107,8 +119,10 @@
 EPICS_ATOMIC_INLINE size_t epicsAtomicSubSizeT ( size_t * pTarget, size_t delta )
 {
     EpicsAtomicLockKey key;
+    size_t result;
+
     epicsAtomicLock ( & key );
-    const size_t result = *pTarget -= delta;
+    result = *pTarget -= delta;
     epicsAtomicUnlock ( & key );
     return result;
 }
@@ -176,9 +190,11 @@
 #ifndef EPICS_ATOMIC_CAS_INTT
 EPICS_ATOMIC_INLINE int epicsAtomicCmpAndSwapIntT ( int * pTarget, int oldval, int newval )
 {
-    EpicsAtomicLockKey key; 
+    EpicsAtomicLockKey key;
+    int cur;
+
     epicsAtomicLock ( & key );
-    const int cur = *pTarget;
+    cur = *pTarget;
     if ( cur == oldval ) {
         *pTarget = newval;
     }
@@ -191,9 +207,11 @@
 EPICS_ATOMIC_INLINE size_t epicsAtomicCmpAndSwapSizeT ( size_t * pTarget, 
 				size_t oldval, size_t newval )
 {
-    EpicsAtomicLockKey key; 
+    EpicsAtomicLockKey key;
+    size_t cur;
+
     epicsAtomicLock ( & key );
-    const size_t cur = *pTarget;
+    cur = *pTarget;
     if ( cur == oldval ) {
         *pTarget = newval;
     }
@@ -208,8 +226,10 @@
                             EpicsAtomicPtrT oldval, EpicsAtomicPtrT newval )
 {
     EpicsAtomicLockKey key;
+    EpicsAtomicPtrT cur;
+
     epicsAtomicLock ( & key );
-    const EpicsAtomicPtrT cur = *pTarget;
+    cur = *pTarget;
     if ( cur == oldval ) {
         *pTarget = newval;
     }

=== modified file 'src/libCom/osi/epicsSpin.h'
--- src/libCom/osi/epicsSpin.h	2012-10-29 19:25:23 +0000
+++ src/libCom/osi/epicsSpin.h	2014-07-25 22:18:29 +0000
@@ -17,7 +17,8 @@
 
 typedef struct epicsSpin *epicsSpinId;
 
-epicsShareFunc epicsSpinId epicsSpinCreate();
+epicsShareFunc epicsSpinId epicsSpinCreate(void);
+epicsShareFunc epicsSpinId epicsSpinMustCreate(void);
 epicsShareFunc void epicsSpinDestroy(epicsSpinId);
 
 epicsShareFunc void epicsSpinLock(epicsSpinId);

=== modified file 'src/libCom/osi/os/RTEMS/osdSpin.c'
--- src/libCom/osi/os/RTEMS/osdSpin.c	2013-02-12 18:13:01 +0000
+++ src/libCom/osi/os/RTEMS/osdSpin.c	2014-07-25 22:18:29 +0000
@@ -4,6 +4,8 @@
 * Copyright (c) 2012 ITER Organization.
 * Copyright (c) 2013 UChicago Argonne LLC, as Operator of Argonne
 *     National Laboratory.
+* Copyright (c) 2013 Brookhaven Science Assoc. as Operator of Brookhaven
+*     National Laboratory.
 * EPICS BASE is distributed subject to a Software License Agreement found
 * in file LICENSE that is included with this distribution.
 \*************************************************************************/
@@ -11,44 +13,88 @@
 /*
  * Authors:  Ralph Lange <[e-mail]>
  *           Andrew Johnson <[e-mail]>
+ *           Michael Davidsaver <[e-mail]>
  *
- * Based on epicsInterrupt.c (RTEMS implementation) by Eric Norum
+ * Inspired by Linux UP spinlocks implemention
+ *   include/linux/spinlock_api_up.h
  */
 
 /*
  * RTEMS (single CPU): LOCK INTERRUPT
  *
  * CAVEAT:
- * This implementation is for UP architectures only.
- *
+ * This implementation is intended for UP architectures only.
  */
 
+#define __RTEMS_VIOLATE_KERNEL_VISIBILITY__ 1
+
 #include <stdlib.h>
 #include <rtems.h>
+#include <cantProceed.h>
 
 #include "epicsSpin.h"
 
 typedef struct epicsSpin {
     rtems_interrupt_level level;
+    unsigned int locked;
 } epicsSpin;
 
-epicsSpinId epicsSpinCreate() {
+epicsSpinId epicsSpinCreate(void) {
     return calloc(1, sizeof(epicsSpin));
 }
 
+epicsSpinId epicsSpinMustCreate(void)
+{
+    epicsSpinId ret = epicsSpinCreate();
+    if(!ret)
+        cantProceed("epicsSpinMustCreate fails");
+    return ret;
+}
+
 void epicsSpinDestroy(epicsSpinId spin) {
     free(spin);
 }
 
 void epicsSpinLock(epicsSpinId spin) {
-    rtems_interrupt_disable(spin->level);
+    rtems_interrupt_level level;
+    rtems_interrupt_disable (level);
+    _Thread_Disable_dispatch();
+    if(spin->locked) {
+        rtems_interrupt_enable (level);
+        _Thread_Enable_dispatch();
+        if(!rtems_interrupt_is_in_progress()) {
+            printk("Deadlock in epicsSpinLock(%p).  Either recursive lock, missing unlock, or locked by sleeping thread.", spin);
+            cantProceed(NULL);
+        } else {
+            printk("epicsSpinLock(%p) failure in ISR.  Either recursive lock, missing unlock, or locked by sleeping thread.\n", spin);
+        }
+        return;
+    }
+    spin->level = level;
+    spin->locked = 1;
 }
 
 int epicsSpinTryLock(epicsSpinId spin) {
-    epicsSpinLock(spin);
+    rtems_interrupt_level level;
+    rtems_interrupt_disable (level);
+    _Thread_Disable_dispatch();
+    if(spin->locked) {
+        rtems_interrupt_enable (level);
+        _Thread_Enable_dispatch();
+        return 1;
+    }
+    spin->level = level;
+    spin->locked = 1;
     return 0;
 }
 
 void epicsSpinUnlock(epicsSpinId spin) {
-    rtems_interrupt_enable(spin->level);
+    rtems_interrupt_level level = spin->level;
+    if(!spin->locked) {
+        printk("epicsSpinUnlock(%p) failure.  lock not taken\n", spin);
+        return;
+    }
+    spin->level = spin->locked = 0;
+    rtems_interrupt_enable (level);
+    _Thread_Enable_dispatch();
 }

=== modified file 'src/libCom/osi/os/default/osdSpin.c'
--- src/libCom/osi/os/default/osdSpin.c	2012-11-20 23:27:38 +0000
+++ src/libCom/osi/os/default/osdSpin.c	2014-07-25 22:18:29 +0000
@@ -25,7 +25,7 @@
     epicsMutexId lock;
 } epicsSpin;
 
-epicsSpinId epicsSpinCreate() {
+epicsSpinId epicsSpinCreate(void) {
     epicsSpin *spin;
 
     spin = calloc(1, sizeof(*spin));
@@ -43,6 +43,14 @@
     return NULL;
 }
 
+epicsSpinId epicsSpinMustCreate(void)
+{
+    epicsSpinId ret = epicsSpinCreate();
+    if(!ret)
+        cantProceed("epicsSpinMustCreate fails");
+    return ret;
+}
+
 void epicsSpinDestroy(epicsSpinId spin) {
     epicsMutexDestroy(spin->lock);
     free(spin);

=== modified file 'src/libCom/osi/os/posix/epicsAtomicOSD.h'
--- src/libCom/osi/os/posix/epicsAtomicOSD.h	2011-09-02 15:59:03 +0000
+++ src/libCom/osi/os/posix/epicsAtomicOSD.h	2014-07-25 22:18:29 +0000
@@ -16,7 +16,7 @@
 #ifndef epicsAtomicOSD_h
 #define epicsAtomicOSD_h
 
-struct EpicsAtomicLockKey {};
+typedef struct EpicsAtomicLockKey {} EpicsAtomicLockKey;
 
 #ifdef __cplusplus
 extern "C" {

=== modified file 'src/libCom/osi/os/posix/osdSpin.c'
--- src/libCom/osi/os/posix/osdSpin.c	2013-02-18 19:11:36 +0000
+++ src/libCom/osi/os/posix/osdSpin.c	2014-07-25 22:18:29 +0000
@@ -18,15 +18,27 @@
 
 #define epicsExportSharedSymbols
 #include "errlog.h"
+#include "cantProceed.h"
 #include "epicsSpin.h"
 
+/* POSIX spinlocks may be subject to priority inversion
+ * and so can't be guaranteed safe in situations where
+ * threads have different priorities, and thread
+ * preemption can't be disabled.
+ */
+#if defined(DONT_USE_POSIX_THREAD_PRIORITY_SCHEDULING)
+#if defined(_POSIX_SPIN_LOCKS) && (_POSIX_SPIN_LOCKS > 1)
+#  define USE_PSPIN
+#endif
+#endif
+
 #define checkStatus(status,message) \
     if ((status)) { \
         errlogPrintf("epicsSpin %s failed: error %s\n", \
             (message), strerror((status))); \
     }
 
-#if defined(_POSIX_SPIN_LOCKS) && (_POSIX_SPIN_LOCKS > 1)
+#ifdef USE_PSPIN
 
 /*
  *  POSIX SPIN LOCKS IMPLEMENTATION
@@ -36,7 +48,7 @@
     pthread_spinlock_t lock;
 } epicsSpin;
 
-epicsSpinId epicsSpinCreate() {
+epicsSpinId epicsSpinCreate(void) {
     epicsSpin *spin;
     int status;
 
@@ -70,6 +82,8 @@
 
     status = pthread_spin_lock(&spin->lock);
     checkStatus(status, "pthread_spin_lock");
+    if (status)
+        cantProceed(NULL);
 }
 
 int epicsSpinTryLock(epicsSpinId spin) {
@@ -79,7 +93,7 @@
     if (status == EBUSY)
         return 1;
     checkStatus(status, "pthread_spin_trylock");
-    return 0;
+    return status;
 }
 
 void epicsSpinUnlock(epicsSpinId spin) {
@@ -89,7 +103,7 @@
     checkStatus(status, "pthread_spin_unlock");
 }
 
-#else /* defined(_POSIX_SPIN_LOCKS) && (_POSIX_SPIN_LOCKS > 1) */
+#else /* USE_PSPIN */
 
 /*
  *  POSIX MUTEX IMPLEMENTATION
@@ -99,7 +113,7 @@
     pthread_mutex_t lock;
 } epicsSpin;
 
-epicsSpinId epicsSpinCreate() {
+epicsSpinId epicsSpinCreate(void) {
     epicsSpin *spin;
     int status;
 
@@ -133,6 +147,8 @@
 
     status = pthread_mutex_lock(&spin->lock);
     checkStatus(status, "pthread_mutex_lock");
+    if (status)
+        cantProceed(NULL);
 }
 
 int epicsSpinTryLock(epicsSpinId spin) {
@@ -142,7 +158,7 @@
     if (status == EBUSY)
         return 1;
     checkStatus(status, "pthread_mutex_trylock");
-    return 0;
+    return status;
 }
 
 void epicsSpinUnlock(epicsSpinId spin) {
@@ -152,4 +168,13 @@
     checkStatus(status, "pthread_mutex_unlock");
 }
 
-#endif /* defined(_POSIX_SPIN_LOCKS) && (_POSIX_SPIN_LOCKS > 1) */
+#endif /* USE_PSPIN */
+
+
+epicsSpinId epicsSpinMustCreate(void)
+{
+    epicsSpinId ret = epicsSpinCreate();
+    if(!ret)
+        cantProceed("epicsSpinMustCreate fails");
+    return ret;
+}

=== modified file 'src/libCom/osi/os/solaris/epicsAtomicOSD.h'
--- src/libCom/osi/os/solaris/epicsAtomicOSD.h	2011-09-07 00:51:04 +0000
+++ src/libCom/osi/os/solaris/epicsAtomicOSD.h	2014-07-25 22:18:29 +0000
@@ -159,7 +159,7 @@
 
 #endif /* ifdef __SunOS_5_10 */
 
-struct EpicsAtomicLockKey {};
+typedef struct EpicsAtomicLockKey {} EpicsAtomicLockKey;
 
 #ifdef __cplusplus
 extern "C" {

=== modified file 'src/libCom/osi/os/vxWorks/osdSpin.c'
--- src/libCom/osi/os/vxWorks/osdSpin.c	2012-11-20 21:04:28 +0000
+++ src/libCom/osi/os/vxWorks/osdSpin.c	2014-07-25 22:18:29 +0000
@@ -2,18 +2,22 @@
 * Copyright (c) 2012 Helmholtz-Zentrum Berlin
 *     fuer Materialien und Energie GmbH.
 * Copyright (c) 2012 ITER Organization.
+* Copyright (c) 2013 Brookhaven Science Assoc. as Operator of Brookhaven
+*     National Laboratory.
 * EPICS BASE is distributed subject to a Software License Agreement found
 * in file LICENSE that is included with this distribution.
 \*************************************************************************/
 
 /*
  * Author:  Ralph Lange <[e-mail]>
+ *          Michael Davidsaver <[e-mail]>
  *
- * based on epicsInterrupt.c (vxWorks implementation) by Marty Kraimer
+ * Inspired by Linux UP spinlocks implemention
+ *   include/linux/spinlock_api_up.h
  */
 
 /*
- * vxWorks (single CPU): LOCK INTERRUPT
+ * vxWorks (single CPU): LOCK INTERRUPT and DISABLE PREEMPTION
  *
  * CAVEAT:
  * This implementation will not compile on vxWorks SMP architectures.
@@ -23,30 +27,71 @@
 
 #include <stdlib.h>
 #include <intLib.h>
+#include <taskLib.h>
 
 #include "epicsSpin.h"
 
 typedef struct epicsSpin {
     int key;
+    unsigned int locked;
 } epicsSpin;
 
-epicsSpinId epicsSpinCreate() {
+epicsSpinId epicsSpinCreate(void) {
     return calloc(1, sizeof(epicsSpin));
 }
 
+epicsSpinId epicsSpinMustCreate(void)
+{
+    epicsSpinId ret = epicsSpinCreate();
+    if(!ret)
+        cantProceed("epicsSpinMustCreate fails");
+    return ret;
+}
+
 void epicsSpinDestroy(epicsSpinId spin) {
     free(spin);
 }
 
 void epicsSpinLock(epicsSpinId spin) {
-    spin->key = intLock();
+    int key = intLock();
+    if(!intContext())
+        taskLock();
+    if(spin->locked) {
+        intUnlock(key);
+        if(!intContext()) {
+            taskUnlock();
+            cantProceed("Deadlock in epicsSpinLock().  Either recursive lock, missing unlock, or locked by sleeping thread.");
+        } else {
+            epicsInterruptContextMessage("epicsSpinLock() failure in ISR.  Either recursive lock, missing unlock, or locked by sleeping thread.\n");
+        }
+        return;
+    }
+    spin->key = key;
+    spin->locked = 1;
 }
 
 int epicsSpinTryLock(epicsSpinId spin) {
-    epicsSpinLock(spin);
+    int key = intLock();
+    if(!intContext())
+        taskLock();
+    if(spin->locked) {
+        intUnlock(key);
+        if(!intContext())
+            taskUnlock();
+        return 1;
+    }
+    spin->key = key;
+    spin->locked = 1;
     return 0;
 }
 
 void epicsSpinUnlock(epicsSpinId spin) {
-    intUnlock(spin->key);
+    int key = spin->key;
+    if(!spin->locked) {
+        epicsInterruptContextMessage("epicsSpinUnlock() failure.  lock not taken\n");
+    }
+    spin->key = spin->locked = 0;
+    intUnlock(key);
+    if(!intContext())
+        taskUnlock();
 }

=== modified file 'src/libCom/test/epicsRunLibComTests.c'
--- src/libCom/test/epicsRunLibComTests.c	2013-12-17 18:54:04 +0000
+++ src/libCom/test/epicsRunLibComTests.c	2014-07-25 22:18:29 +0000
@@ -18,6 +18,7 @@
 
 int epicsThreadTest(void);
 int epicsTimerTest(void);
+int epicsSpinTest(void);
 int epicsAlgorithm(void);
 int epicsEllTest(void);
 int epicsEnvTest(void);
@@ -60,6 +61,8 @@
      */
     runTest(epicsTimerTest);
 
+    runTest(epicsSpinTest);
+
     runTest(epicsAlgorithm);
 
     runTest(epicsEllTest);

=== modified file 'src/libCom/test/epicsSpinTest.c'
--- src/libCom/test/epicsSpinTest.c	2012-11-20 21:55:53 +0000
+++ src/libCom/test/epicsSpinTest.c	2014-07-25 22:18:29 +0000
@@ -22,6 +22,7 @@
 
 #include "epicsTime.h"
 #include "epicsThread.h"
+#include "epicsAtomic.h"
 #include "epicsSpin.h"
 #include "epicsEvent.h"
 #include "errlog.h"
@@ -31,22 +32,26 @@
 typedef struct info {
     int         threadnum;
     epicsSpinId spin;
-    int         quit;
+    int         *counter;
+    int         rounds;
+    epicsEventId done;
 } info;
 
+#define spinDelay 0.016667
+
 void spinThread(void *arg)
 {
     info *pinfo = (info *) arg;
     testDiag("spinThread %d starting", pinfo->threadnum);
-    while (pinfo->quit--) {
+    epicsThreadSleep(0.1);  /* Try to align threads */
+    while (pinfo->rounds--) {
+        epicsThreadSleep(spinDelay);
         epicsSpinLock(pinfo->spin);
-        testPass("spinThread %d epicsSpinLock taken", pinfo->threadnum);
-        epicsThreadSleep(.1);
+        pinfo->counter[0]++;
         epicsSpinUnlock(pinfo->spin);
-        epicsThreadSleep(.9);
     }
     testDiag("spinThread %d exiting", pinfo->threadnum);
-    return;
+    epicsEventSignal(pinfo->done);
 }
 
 static void lockPair(struct epicsSpin *spin)
@@ -94,6 +99,8 @@
 
     /* Initialize spinlock */
     spin = epicsSpinCreate();
+    if (!spin)
+        testAbort("epicsSpinCreate() returned NULL");
 
     /* test a single lock pair */
     epicsTimeGetCurrent(&begin);
@@ -106,78 +113,133 @@
     delay /= N * 100u;  /* convert to delay per lock pair */
     delay *= 1e6;       /* convert to micro seconds */
     testDiag("lock()*1/unlock()*1 takes %f microseconds", delay);
+    epicsSpinDestroy(spin);
 }
 
+struct verifyTryLock;
+
+struct verifyTryLockEnt {
+    epicsEventId done;
+    struct verifyTryLock *main;
+};
+
 struct verifyTryLock {
     epicsSpinId  spin;
-    epicsEventId done;
+    int flag;
+    struct verifyTryLockEnt *ents;
 };
 
 static void verifyTryLockThread(void *pArg)
 {
-    struct verifyTryLock *pVerify =
-        (struct verifyTryLock *) pArg;
-
-    testOk1(epicsSpinTryLock(pVerify->spin));
+    struct verifyTryLockEnt *pVerify =
+        (struct verifyTryLockEnt *) pArg;
+
+    while(epicsAtomicGetIntT(&pVerify->main->flag)==0) {
+        int ret = epicsSpinTryLock(pVerify->main->spin);
+        if(ret!=0) {
+            epicsAtomicCmpAndSwapIntT(&pVerify->main->flag, 0, ret);
+            break;
+        } else
+            epicsSpinUnlock(pVerify->main->spin);
+    }
+
     epicsEventSignal(pVerify->done);
 }
 
+/* Start one thread per CPU which will all try lock
+ * the same spinlock.  They break as soon as one
+ * fails to take the lock.
+ */
 static void verifyTryLock()
 {
+    int N, i;
     struct verifyTryLock verify;
 
-    verify.spin = epicsSpinCreate();
-    verify.done = epicsEventMustCreate(epicsEventEmpty);
-
-    testOk1(epicsSpinTryLock(verify.spin) == 0);
-
-    epicsThreadCreate("verifyTryLockThread", 40,
-        epicsThreadGetStackSize(epicsThreadStackSmall),
-        verifyTryLockThread, &verify);
-
-    testOk1(epicsEventWait(verify.done) == epicsEventWaitOK);
-
-    epicsSpinUnlock(verify.spin);
+    N = epicsThreadGetCPUs();
+    if(N==1) {
+        testSkip(1, "verifyTryLock() only for SMP systems");
+        return;
+    }
+
+    verify.flag = 0;
+    verify.spin = epicsSpinMustCreate();
+
+    testDiag("Starting %d spinners", N);
+
+    verify.ents = calloc(N, sizeof(*verify.ents));
+    for(i=0; i<N; i++) {
+        verify.ents[i].main = &verify;
+        verify.ents[i].done = epicsEventMustCreate(epicsEventEmpty);
+        epicsThreadMustCreate("verifyTryLockThread", 40,
+            epicsThreadGetStackSize(epicsThreadStackSmall),
+                              verifyTryLockThread, &verify.ents[i]);
+    }
+
+    testDiag("All started");
+
+    for(i=0; i<N; i++) {
+        epicsEventMustWait(verify.ents[i].done);
+        epicsEventDestroy(verify.ents[i].done);
+    }
+
+    testDiag("All done");
+
+    testOk(verify.flag==1, "epicsTryLock returns %d (expect 1)", verify.flag);
+
     epicsSpinDestroy(verify.spin);
-    epicsEventDestroy(verify.done);
+    free(verify.ents);
 }
 
 MAIN(epicsSpinTest)
 {
     const int nthreads = 3;
-    const int nrounds = 5;
+    const int nrounds = 500;
     unsigned int stackSize;
     epicsThreadId *id;
-    int i;
+    int i, counter;
     char **name;
-    void **arg;
     info **pinfo;
     epicsSpinId spin;
 
-    testPlan(3 + nthreads * nrounds);
+    testPlan(2);
 
     verifyTryLock();
 
     spin = epicsSpinCreate();
+    if (!spin)
+        testAbort("epicsSpinCreate() returned NULL");
 
     id = (epicsThreadId *) calloc(nthreads, sizeof(epicsThreadId));
     name = (char **) calloc(nthreads, sizeof(char *));
-    arg = (void **) calloc(nthreads, sizeof(void *));
     pinfo = (info **) calloc(nthreads, sizeof(info *));
     stackSize = epicsThreadGetStackSize(epicsThreadStackSmall);
+    counter = 0;
     for (i = 0; i < nthreads; i++) {
         name[i] = (char *) calloc(10, sizeof(char));
         sprintf(name[i],"task%d",i);
         pinfo[i] = (info *) calloc(1, sizeof(info));
         pinfo[i]->threadnum = i;
         pinfo[i]->spin = spin;
-        pinfo[i]->quit = nrounds;
-        arg[i] = pinfo[i];
+        pinfo[i]->counter = &counter;
+        pinfo[i]->rounds = nrounds;
+        pinfo[i]->done = epicsEventMustCreate(epicsEventEmpty);
         id[i] = epicsThreadCreate(name[i], 40, stackSize,
                                   spinThread,
-                                  arg[i]);
-    }
-    epicsThreadSleep(2.0 + nrounds);
+                                  pinfo[i]);
+    }
+    for (i = 0; i < nthreads; i++) {
+        epicsEventMustWait(pinfo[i]->done);
+        epicsEventDestroy(pinfo[i]->done);
+        free(name[i]);
+        free(pinfo[i]);
+    }
+    testOk(counter == nthreads * nrounds, "Loops run = %d (expecting %d)",
+        counter, nthreads * nrounds);
+
+    free(pinfo);
+    free(name);
+    free(id);
+    epicsSpinDestroy(spin);
 
     epicsSpinPerformance();