From 76aa3aab0124b05e42ae5aa01ee27a78707d5d74 Mon Sep 17 00:00:00 2001 From: till straumann Date: Thu, 13 Aug 2020 17:16:28 +0200 Subject: [PATCH] Support run-time enabled priority-inheritance for epicsMutex, epicsEvent etc. --- configure/CONFIG_ENV | 3 + src/libCom/Makefile | 1 + src/libCom/env/envDefs.h | 1 + src/libCom/osi/os/Linux/osdThread.c | 4 +- src/libCom/osi/os/posix/osdEvent.c | 3 +- src/libCom/osi/os/posix/osdMutex.c | 226 +++++++++++++++++++++------- src/libCom/osi/os/posix/osdMutex.h | 24 ++- src/libCom/osi/os/posix/osdSpin.c | 4 +- src/libCom/osi/os/posix/osdThread.c | 4 +- 9 files changed, 211 insertions(+), 59 deletions(-) diff --git a/configure/CONFIG_ENV b/configure/CONFIG_ENV index 173dceaca..e0bcdf05c 100644 --- a/configure/CONFIG_ENV +++ b/configure/CONFIG_ENV @@ -54,3 +54,6 @@ EPICS_IOC_LOG_PORT=7004 EPICS_CMD_PROTO_PORT= EPICS_AR_PORT=7002 +# libCom +# Whether to enable priority inheritance -- must be set to Y[ES]/y[es]/T[RUE]/t[rue]/1 +EPICS_MUTEX_USE_PRIORITY_INHERITANCE="NO" diff --git a/src/libCom/Makefile b/src/libCom/Makefile index ed23af63c..8d89ff343 100644 --- a/src/libCom/Makefile +++ b/src/libCom/Makefile @@ -17,6 +17,7 @@ epicsReadline_INCLUDES += $(INCLUDES_$(COMMANDLINE_LIBRARY)) #POSIX thread priority scheduling flag THREAD_CPPFLAGS_NO += -DDONT_USE_POSIX_THREAD_PRIORITY_SCHEDULING +osdMutex_CPPFLAGS += $(THREAD_CPPFLAGS_$(USE_POSIX_THREAD_PRIORITY_SCHEDULING)) osdThread_CPPFLAGS += $(THREAD_CPPFLAGS_$(USE_POSIX_THREAD_PRIORITY_SCHEDULING)) osdSpin_CPPFLAGS += $(THREAD_CPPFLAGS_$(USE_POSIX_THREAD_PRIORITY_SCHEDULING)) diff --git a/src/libCom/env/envDefs.h b/src/libCom/env/envDefs.h index d47d4bbdd..781fa219f 100644 --- a/src/libCom/env/envDefs.h +++ b/src/libCom/env/envDefs.h @@ -69,6 +69,7 @@ epicsShareExtern const ENV_PARAM EPICS_CMD_PROTO_PORT; epicsShareExtern const ENV_PARAM EPICS_AR_PORT; epicsShareExtern const ENV_PARAM IOCSH_PS1; epicsShareExtern const ENV_PARAM IOCSH_HISTSIZE; +epicsShareExtern const ENV_PARAM EPICS_MUTEX_USE_PRIORITY_INHERITANCE; epicsShareExtern const ENV_PARAM *env_param_list[]; diff --git a/src/libCom/osi/os/Linux/osdThread.c b/src/libCom/osi/os/Linux/osdThread.c index bf7e6c7e3..24d4ec3ed 100644 --- a/src/libCom/osi/os/Linux/osdThread.c +++ b/src/libCom/osi/os/Linux/osdThread.c @@ -319,9 +319,9 @@ static void once(void) int status; pthread_key_create(&getpthreadInfo,0); - status = pthread_mutex_init(&onceLock,0); + status = epicsPosixMutexInit(&onceLock,posixMutexDefault); checkStatusQuit(status,"pthread_mutex_init","epicsThreadInit"); - status = pthread_mutex_init(&listLock,0); + status = epicsPosixMutexInit(&listLock,posixMutexDefault); checkStatusQuit(status,"pthread_mutex_init","epicsThreadInit"); pcommonAttr = calloc(1,sizeof(commonAttr)); if(!pcommonAttr) checkStatusOnceQuit(errno,"calloc","epicsThreadInit"); diff --git a/src/libCom/osi/os/posix/osdEvent.c b/src/libCom/osi/os/posix/osdEvent.c index dd3923c7d..0449daf87 100644 --- a/src/libCom/osi/os/posix/osdEvent.c +++ b/src/libCom/osi/os/posix/osdEvent.c @@ -25,6 +25,7 @@ #include "epicsTime.h" #include "errlog.h" #include "epicsAssert.h" +#include "osdMutex.h" /* Until these can be demonstrated to work leave them undefined*/ #undef _POSIX_THREAD_PROCESS_SHARED @@ -84,7 +85,7 @@ epicsShareFunc epicsEventId epicsShareAPI epicsEventCreate(epicsEventInitialStat int status; pevent = callocMustSucceed(1,sizeof(*pevent),"epicsEventCreate"); - status = pthread_mutex_init(&pevent->mutex,0); + status = epicsPosixMutexInit(&pevent->mutex,posixMutexDefault); checkStatusQuit(status,"pthread_mutex_init","epicsEventCreate"); status = pthread_cond_init(&pevent->cond,0); checkStatusQuit(status,"pthread_cond_init","epicsEventCreate"); diff --git a/src/libCom/osi/os/posix/osdMutex.c b/src/libCom/osi/os/posix/osdMutex.c index 064b82fe3..e3f5470dc 100644 --- a/src/libCom/osi/os/posix/osdMutex.c +++ b/src/libCom/osi/os/posix/osdMutex.c @@ -19,6 +19,7 @@ #include #include #include +#include #define epicsExportSharedSymbols #include "epicsMutex.h" @@ -26,6 +27,7 @@ #include "epicsTime.h" #include "errlog.h" #include "epicsAssert.h" +#include "envDefs.h" #define checkStatus(status,message) \ if((status)) { \ @@ -37,6 +39,99 @@ if(status) { \ cantProceed((method)); \ } +/* Until these can be demonstrated to work leave them undefined*/ +/* On solaris 8 _POSIX_THREAD_PRIO_INHERIT fails*/ +#if defined(DONT_USE_POSIX_THREAD_PRIORITY_SCHEDULING) +#undef _POSIX_THREAD_PRIO_INHERIT +#endif + +#if defined(_XOPEN_SOURCE) && (_XOPEN_SOURCE)>=500 +#define HAVE_RECURSIVE_MUTEX +#else +#undef HAVE_RECURSIVE_MUTEX +#endif + +/* Global var - pthread_once does not support passing args but it is more efficient + * then epicsThreadOnce which always acquires a mutex. + */ +static pthread_mutexattr_t globalAttrDefault; +#ifdef HAVE_RECURSIVE_MUTEX +static pthread_mutexattr_t globalAttrRecursive; +#endif +static pthread_once_t globalAttrInitOnce = PTHREAD_ONCE_INIT; + +static void setAttrDefaults(pthread_mutexattr_t *a) +{ + int status; + status = pthread_mutexattr_init(a); + checkStatusQuit(status,"pthread_mutexattr_init","setAttrDefaults"); + +#if defined _POSIX_THREAD_PRIO_INHERIT + { + const char *p = envGetConfigParamPtr(&EPICS_MUTEX_USE_PRIORITY_INHERITANCE); + char c = p ? toupper(p[0]) : 'N'; + if ( 'T' == c || 'Y' == c || '1' == c ) { + status = pthread_mutexattr_setprotocol(a, PTHREAD_PRIO_INHERIT); + if (errVerbose) checkStatus(status, "pthread_mutexattr_setprotocol(PTHREAD_PRIO_INHERIT)"); +#ifndef HAVE_RECURSIVE_MUTEX + /* The implementation based on a condition variable below does not support + * priority-inheritance! + */ + epicsPrintf("WARNING: PRIORITY-INHERITANCE UNAVAILABLE for epicsMutex\n"); +#endif + } + } +#endif + +} + +static void globalAttrInit() +{ + int status; + + setAttrDefaults( &globalAttrDefault ); + +#ifdef HAVE_RECURSIVE_MUTEX + setAttrDefaults( &globalAttrRecursive ); + status = pthread_mutexattr_settype(&globalAttrRecursive, PTHREAD_MUTEX_RECURSIVE); + checkStatusQuit(status, "pthread_mutexattr_settype(PTHREAD_MUTEX_RECURSIVE)", "globalAttrInit"); +#endif +} + +epicsShareFunc pthread_mutexattr_t * epicsShareAPI epicsPosixMutexAttrGet (EpicsPosixMutexProperty p) +{ + int status; + status = pthread_once( &globalAttrInitOnce, globalAttrInit ); + checkStatusQuit(status,"pthread_once","epicsPosixMutexAttrGet"); + switch ( p ) { + default: + case posixMutexDefault: + break; + case posixMutexRecursive: +#ifdef HAVE_RECURSIVE_MUTEX + return &globalAttrRecursive; +#else + return 0; +#endif + } + return &globalAttrDefault; +} + +epicsShareFunc int epicsShareAPI epicsPosixMutexInit (pthread_mutex_t *m, EpicsPosixMutexProperty p) +{ + pthread_mutexattr_t *atts = epicsPosixMutexAttrGet( p ); + if ( ! atts ) + return ENOTSUP; + return pthread_mutex_init(m, atts); +} + +epicsShareFunc void epicsShareAPI epicsPosixMutexMustInit (pthread_mutex_t *m, EpicsPosixMutexProperty p) +{ + int status; + status = epicsPosixMutexInit(m, p); + checkStatusQuit(status,"pthread_mutex_init","epicsMustInitPosixMutex"); +} + static int mutexLock(pthread_mutex_t *id) { int status; @@ -47,11 +142,6 @@ static int mutexLock(pthread_mutex_t *id) return status; } -/* Until these can be demonstrated to work leave them undefined*/ -/* On solaris 8 _POSIX_THREAD_PRIO_INHERIT fails*/ -#undef _POSIX_THREAD_PROCESS_SHARED -#undef _POSIX_THREAD_PRIO_INHERIT - /* Two completely different implementations are provided below * If support is available for PTHREAD_MUTEX_RECURSIVE then * only pthread_mutex is used. @@ -60,9 +150,8 @@ static int mutexLock(pthread_mutex_t *id) */ -#if defined(_XOPEN_SOURCE) && (_XOPEN_SOURCE)>=500 +#ifdef HAVE_RECURSIVE_MUTEX typedef struct epicsMutexOSD { - pthread_mutexattr_t mutexAttr; pthread_mutex_t lock; } epicsMutexOSD; @@ -71,20 +160,8 @@ epicsMutexOSD * epicsMutexOsdCreate(void) { int status; pmutex = callocMustSucceed(1, sizeof(*pmutex), "epicsMutexOsdCreate"); - status = pthread_mutexattr_init(&pmutex->mutexAttr); - checkStatusQuit(status,"pthread_mutexattr_init", "epicsMutexOsdCreate"); -#if defined _POSIX_THREAD_PRIO_INHERIT - status = pthread_mutexattr_setprotocol(&pmutex->mutexAttr, - PTHREAD_PRIO_INHERIT); - if (errVerbose) checkStatus(status, "pthread_mutexattr_setprotocal"); -#endif /*_POSIX_THREAD_PRIO_INHERIT*/ - - status = pthread_mutexattr_settype(&pmutex->mutexAttr, - PTHREAD_MUTEX_RECURSIVE); - if (errVerbose) checkStatus(status, "pthread_mutexattr_settype"); - - status = pthread_mutex_init(&pmutex->lock, &pmutex->mutexAttr); + status = epicsPosixMutexInit(&pmutex->lock, posixMutexRecursive); checkStatusQuit(status, "pthread_mutex_init", "epicsMutexOsdCreate"); return pmutex; } @@ -95,8 +172,6 @@ void epicsMutexOsdDestroy(struct epicsMutexOSD * pmutex) status = pthread_mutex_destroy(&pmutex->lock); checkStatus(status, "pthread_mutex_destroy"); - status = pthread_mutexattr_destroy(&pmutex->mutexAttr); - checkStatus(status, "pthread_mutexattr_destroy"); free(pmutex); } @@ -135,15 +210,85 @@ void epicsMutexOsdShow(struct epicsMutexOSD * pmutex, unsigned int level) { } -#else /*defined(_XOPEN_SOURCE) && (_XOPEN_SOURCE)>=500 */ +#else /* #ifdef HAVE_RECURSIVE_MUTEX */ + +/* The standard EPICS implementation of a recursive mutex (in absence of native support) + * does not allow for priority-inheritance: + * a low priority thread may hold the ('soft-') mutex, i.e., may be preempted in the + * critical section without the high-priority thread noticing (because the HP-thread is + * sleeping on the condvar and not waiting for the mutex). + * + * A better implementation could be: + * + * struct epicsMutexOSD { + * pthread_mutex_t mtx; + * atomic owner; + * unsigned count; + * }; + * + * void mutexLock(struct epicsMutexOSD *m) + * { + * pthread_t currentOwner = atomic_load(&m->owner, acquire); + * + * if ( pthread_equal(currentOwner, pthread_self()) ) { + * m->count++; + * return; + * } + * + * pthread_mutex_lock(&m->mtx); + * // ordering of this write to 'owner' is irrelevant since it doesn't matter + * // if another thread performing the test above sees the 'invalid' or already + * // 'our' TID. + * atomic_store(&m->owner, pthread_self(), relaxed); + * // mutex_lock has 'acquire' semantics, i.e., the + * // final count-- that happened in another thread is guaranteed + * // to have completed + * m->count = 1; + * } + * + * void mutexUnlock(struct epicsMutexOSD *o) + * { + * o->count--; + * if ( o->count == 0 ) { + * // acquire-release ordering between here and 'mutexLock' above'! + * // Theoretically (but extremely unlikely) the executing thread + * // may go away and a newly created thread with the same (recycled) + * // TID on a different CPU could still see the old TID in mutexLock + * // and believe it already owns the mutex... + * atomic_store(&m->owner, invalid_thread_id, release); + * pthread_mutex_unlock( &o->mtx ); + * } + * + * The 'invalid_thread_id' could be an ID of a permanently suspended dummy thread + * (pthread does not define a 'NULL' ID and you don't want to get into an 'ABA'-sort + * of situation where 'mutexLock' believes to be the current owner because the 'invalid' + * ID is a 'recycled' thread id). + * + * Without atomic operations we'd have to introduce a second mutex to protect the 'owner' + * member ('count' is only ever accessed with the mutex held). But that would then + * lead to two extra lock/unlock pairs in 'mutexLock'. A dirty version would ignore that + * and rely on pthread_t fitting in a CPU word and the acquire/release corner-case mentioned + * above to never happen. Plus, some CPUs (x86) are more strongly (acq/rel) ordered implicitly. + * + * Here the corner case again: + * + * CPU1 CPU2 + * owner = 1234 + * ... + * owner = invalid_tid + * mutex_unlock() + * + * thread 1234 dies new thread with recycled TID 1234 + * enters osdMutexLock + * 'owner=invalid_tid' assignment not yet visible on this CPU + * if ( pthread_equal( owner, pthread_self() ) ) { + * ==> ERRONEOUSLY ENTERS HERE + * } + */ typedef struct epicsMutexOSD { - pthread_mutexattr_t mutexAttr; pthread_mutex_t lock; pthread_cond_t waitToBeOwner; -#if defined _POSIX_THREAD_PROCESS_SHARED - pthread_condattr_t condAttr; -#endif /*_POSIX_THREAD_PROCESS_SHARED*/ int count; int owned; /* TRUE | FALSE */ pthread_t ownerTid; @@ -154,28 +299,10 @@ epicsMutexOSD * epicsMutexOsdCreate(void) { int status; pmutex = callocMustSucceed(1, sizeof(*pmutex), "epicsMutexOsdCreate"); - status = pthread_mutexattr_init(&pmutex->mutexAttr); - checkStatusQuit(status, "pthread_mutexattr_init", "epicsMutexOsdCreate"); -#if defined _POSIX_THREAD_PRIO_INHERIT - status = pthread_mutexattr_setprotocol( - &pmutex->mutexAttr,PTHREAD_PRIO_INHERIT); - if (errVerbose) checkStatus(status, "pthread_mutexattr_setprotocal"); -#endif /*_POSIX_THREAD_PRIO_INHERIT*/ + epicsPosixMutexMustInit(&pmutex->lock, posixMutexDefault); - status = pthread_mutex_init(&pmutex->lock, &pmutex->mutexAttr); - checkStatusQuit(status, "pthread_mutex_init", "epicsMutexOsdCreate"); - -#if defined _POSIX_THREAD_PROCESS_SHARED - status = pthread_condattr_init(&pmutex->condAttr); - checkStatus(status, "pthread_condattr_init"); - status = pthread_condattr_setpshared(&pmutex->condAttr, - PTHREAD_PROCESS_PRIVATE); - checkStatus(status, "pthread_condattr_setpshared"); - status = pthread_cond_init(&pmutex->waitToBeOwner, &pmutex->condAttr); -#else status = pthread_cond_init(&pmutex->waitToBeOwner, 0); -#endif /*_POSIX_THREAD_PROCESS_SHARED*/ checkStatusQuit(status, "pthread_cond_init", "epicsMutexOsdCreate"); return pmutex; } @@ -186,13 +313,8 @@ void epicsMutexOsdDestroy(struct epicsMutexOSD * pmutex) status = pthread_cond_destroy(&pmutex->waitToBeOwner); checkStatus(status, "pthread_cond_destroy"); -#if defined _POSIX_THREAD_PROCESS_SHARED - status = pthread_condattr_destroy(&pmutex->condAttr); -#endif /*_POSIX_THREAD_PROCESS_SHARED*/ status = pthread_mutex_destroy(&pmutex->lock); checkStatus(status, "pthread_mutex_destroy"); - status = pthread_mutexattr_destroy(&pmutex->mutexAttr); - checkStatus(status, "pthread_mutexattr_destroy"); free(pmutex); } @@ -281,4 +403,4 @@ void epicsMutexOsdShow(struct epicsMutexOSD *pmutex,unsigned int level) printf("ownerTid %p count %d owned %d\n", (void *)pmutex->ownerTid, pmutex->count, pmutex->owned); } -#endif /*defined(_XOPEN_SOURCE) && (_XOPEN_SOURCE)>=500 */ +#endif /* #ifdef HAVE_RECURSIVE_MUTEX */ diff --git a/src/libCom/osi/os/posix/osdMutex.h b/src/libCom/osi/os/posix/osdMutex.h index cc416384a..c0c9d7e21 100644 --- a/src/libCom/osi/os/posix/osdMutex.h +++ b/src/libCom/osi/os/posix/osdMutex.h @@ -7,4 +7,26 @@ * and higher are distributed subject to a Software License Agreement found * in file LICENSE that is included with this distribution. \*************************************************************************/ -/* for a pure posix implementation no osdMutex.h definitions are needed*/ +#ifndef osdMutexh +#define osdTMutexh + +#include + +#include "shareLib.h" + +#ifdef __cplusplus +extern "C" { +#endif + +typedef enum {posixMutexDefault = 0, posixMutexRecursive = 1} EpicsPosixMutexProperty; + +/* Returns NULL if requested set of properties is not supported */ +epicsShareFunc pthread_mutexattr_t * epicsShareAPI epicsPosixMutexAttrGet (EpicsPosixMutexProperty); +epicsShareFunc int epicsShareAPI epicsPosixMutexInit (pthread_mutex_t *,EpicsPosixMutexProperty); +epicsShareFunc void epicsShareAPI epicsPosixMutexMustInit(pthread_mutex_t *,EpicsPosixMutexProperty); + +#ifdef __cplusplus +} +#endif + +#endif /* osdMutexh */ diff --git a/src/libCom/osi/os/posix/osdSpin.c b/src/libCom/osi/os/posix/osdSpin.c index aa61a6c90..a5eaffc68 100644 --- a/src/libCom/osi/os/posix/osdSpin.c +++ b/src/libCom/osi/os/posix/osdSpin.c @@ -105,6 +105,8 @@ void epicsSpinUnlock(epicsSpinId spin) { #else /* USE_PSPIN */ +#include + /* * POSIX MUTEX IMPLEMENTATION */ @@ -121,7 +123,7 @@ epicsSpinId epicsSpinCreate(void) { if (!spin) goto fail; - status = pthread_mutex_init(&spin->lock, NULL); + status = epicsPosixMutexInit(&spin->lock, posixMutexDefault); checkStatus(status, "pthread_mutex_init"); if (status) goto fail; diff --git a/src/libCom/osi/os/posix/osdThread.c b/src/libCom/osi/os/posix/osdThread.c index 2dc4eda8a..4da3158ab 100644 --- a/src/libCom/osi/os/posix/osdThread.c +++ b/src/libCom/osi/os/posix/osdThread.c @@ -316,9 +316,9 @@ static void once(void) int status; pthread_key_create(&getpthreadInfo,0); - status = pthread_mutex_init(&onceLock,0); + status = epicsPosixMutexInit(&onceLock,posixMutexDefault); checkStatusQuit(status,"pthread_mutex_init","epicsThreadInit"); - status = pthread_mutex_init(&listLock,0); + status = epicsPosixMutexInit(&listLock,posixMutexDefault); checkStatusQuit(status,"pthread_mutex_init","epicsThreadInit"); pcommonAttr = calloc(1,sizeof(commonAttr)); if(!pcommonAttr) checkStatusOnceQuit(errno,"calloc","epicsThreadInit");