Experimental Physics and
Industrial Control System

mdavidsaver has proposed merging lp:~epics-core/epics-base/spinlockfix into lp:epics-base.

Requested reviews:
  EPICS Core Developers (epics-core)

For more details, see:
https://code.launchpad.net/~epics-core/epics-base/spinlockfix/+merge/220845

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().
-- 
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.

=== modified file 'src/libCom/osi/Makefile'
--- src/libCom/osi/Makefile	2012-10-29 19:25:23 +0000
+++ src/libCom/osi/Makefile	2014-05-23 19:40:11 +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/epicsSpin.h'
--- src/libCom/osi/epicsSpin.h	2012-10-29 19:25:23 +0000
+++ src/libCom/osi/epicsSpin.h	2014-05-23 19:40:11 +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-05-23 19:40:11 +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,37 +13,56 @@
 /*
  * Authors:  Ralph Lange <[email protected]>
  *           Andrew Johnson <[email protected]>
+ *           Michael Davidsaver <[email protected]>
  *
- * 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 <epicsAssert.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();
+    spin->level = level;
+    assert(!spin->locked);
+    spin->locked = 1;
 }
 
 int epicsSpinTryLock(epicsSpinId spin) {
@@ -50,5 +71,9 @@
 }
 
 void epicsSpinUnlock(epicsSpinId spin) {
-    rtems_interrupt_enable(spin->level);
+    rtems_interrupt_level level = spin->level;
+    assert(spin->locked);
+    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-05-23 19:40:11 +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/osdSpin.c'
--- src/libCom/osi/os/posix/osdSpin.c	2013-02-18 19:11:36 +0000
+++ src/libCom/osi/os/posix/osdSpin.c	2014-05-23 19:40:11 +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("epicsSpinLock pspin");
 }
 
 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("epicsSpinLock pmutex");
 }
 
 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/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-05-23 19:40:11 +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 <[email protected]>
+ *          Michael Davidsaver <[email protected]>
  *
- * 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,23 +27,38 @@
 
 #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();
+    assert(!spin->locked);
+    spin->key = key;
+    spin->locked = 1;
 }
 
 int epicsSpinTryLock(epicsSpinId spin) {
@@ -48,5 +67,10 @@
 }
 
 void epicsSpinUnlock(epicsSpinId spin) {
-    intUnlock(spin->key);
+    int key = spin->key;
+    assert(spin->locked);
+    spin->key = spin->locked = 0;
+    intUnlock(key);
+    if(!intContext())
+        taskUnlock();
 }

Navigate by Date:

Prev: RE: Status of devlib2 project? Williams Jr., Ernest L.

Next: Jenkins build became unstable: epics-base-3.15 #53 APS Jenkins

Index: 2002  2003  2004  2005  2006  2007  2008  2009  2010  2011  2012  2013  <2014>  2015  2016  2017  2018  2019  2020  2021  2022  2023  2024 

Navigate by Thread:

Prev: Re: Status of devlib2 project? Ralph Lange

Next: Jenkins build became unstable: epics-base-3.15 #53 APS Jenkins

Index: 2002  2003  2004  2005  2006  2007  2008  2009  2010  2011  2012  2013  <2014>  2015  2016  2017  2018  2019  2020  2021  2022  2023  2024