From 97bf82ee05036a26a15c1df6f27574ca60888df6 Mon Sep 17 00:00:00 2001 From: Gregory Nutt Date: Wed, 2 Nov 2016 18:21:46 -0600 Subject: [PATCH] Semaphores: Provide macros for sem_setprotobol() and sem_getprotocol() if priority inheritance is not enabled. More SEM_PRIO_* definitions to include/nuttx/semaphore.h --- TODO | 51 ++++++++++++++++++++++++++++++- include/nuttx/semaphore.h | 33 ++++++++++++++++++-- include/pthread.h | 27 ++++++++-------- include/semaphore.h | 22 +++++-------- sched/semaphore/sem_setprotocol.c | 19 ++++++++++++ 5 files changed, 120 insertions(+), 32 deletions(-) diff --git a/TODO b/TODO index 3c187e017e..97d9f00719 100644 --- a/TODO +++ b/TODO @@ -9,7 +9,7 @@ issues related to each board port. nuttx/: - (13) Task/Scheduler (sched/) + (14) Task/Scheduler (sched/) (1) Memory Management (mm/) (1) Power Management (drivers/pm) (3) Signals (sched/signal, arch/) @@ -216,6 +216,55 @@ o Task/Scheduler (sched/) Status: Open Priority: Medium-ish + Title: ISSUES WITH PRIORITY INHERITANCE WHEN SEMAPHORE USED AS IPC + Description: The semaphores have multiple uses. The typical usage is where + the semaphore is used as lock on one or more resources. In + this typical case, priority inheritance works perfectly: The + holder of a semaphore count must be remembered so that its + priority can be boosted if a higher priority task requires a + count from the semaphore. It remains the holder until is + calls sem_post() to release the count on the semaphore. + + But a different usage model for semaphores is for signalling + events. In this case, the semaphore count is initialized to + zero and the receiving task calls sem_wait() to wait for the + next event of interest. When an event of interest is + detected by another task (or even an interrupt handler), + sem_post() is called which increments the count to 1 and + wakes up the receiving task. + + For example, in the following TASK A waits for events and + TASK B (or perhaps an interrupt handler) signals task A of + the occurence of the events by posting the semaphore: + + TASK A TASK B + sem_init(sem, 0, 0); + sem_wait(sem); + sem_post(sem); + Awakens as holder + + These two usage models are really very different and priority + inheritance simply does not apply when the semaphore is used for + signalling rather than locking. In this signalling case + priority inheritance can interfere with the operation of the + semaphore. The problem is that when TASK A is awakened it is + a holder of the semaphore. Normally, a task is removed from + the holder list when it finally release the + + However, TASK A never calls sem_post(sem) so it becomes + *permanently* a holder of the semaphore and may have its + priority boosted when any other task tries to acquire the + semaphore. + + The fix is to call sem_setprotocol(SEM_PRIO_NONE) immediately + after the sem_init() call so that there will be no priority + inheritance operations on this semaphore used for signalling. + Status: Open + Priority: High. If you have priority inheritance enabled and you use + semaphores for signalling events, then you *must* call + sem_setprotocol(SEM_PRIO_NONE) immediately after initializing + the semaphore. + Title: SCALABILITY Description: Task control information is retained in simple lists. This is completely appropriate for small embedded systems where diff --git a/include/nuttx/semaphore.h b/include/nuttx/semaphore.h index 7fef2a91c5..04fde1bacb 100644 --- a/include/nuttx/semaphore.h +++ b/include/nuttx/semaphore.h @@ -51,6 +51,12 @@ * Pre-processor Definitions ****************************************************************************/ +/* Values for protocol attribute */ + +#define SEM_PRIO_NONE 0 +#define SEM_PRIO_INHERIT 1 +#define SEM_PRIO_PROTECT 2 + /**************************************************************************** * Public Type Definitions ****************************************************************************/ @@ -67,9 +73,7 @@ struct nsem_inode_s sem_t ns_sem; /* The contained semaphore */ - /* Inode payload unique to named semaphores. ns_inode must appear first - * in this structure in order to support casting between type sem_t and - * types of struct nsem_inode_s. */ + /* Inode payload unique to named semaphores. */ FAR struct inode *ns_inode; /* Containing inode */ }; @@ -158,6 +162,8 @@ int sem_reset(FAR sem_t *sem, int16_t count); #ifdef CONFIG_PRIORITY_INHERITANCE int sem_getprotocol(FAR sem_t *sem, FAR int *protocol); +#else +# define sem_getprotocol(s,p) do { *(p) == SEM_PRIO_NONE); } while (0) #endif /**************************************************************************** @@ -166,6 +172,25 @@ int sem_getprotocol(FAR sem_t *sem, FAR int *protocol); * Description: * Set semaphore protocol attribute. * + * One particularly important use of this function is when a semaphore + * is used for inter-task communication like: + * + * TASK A TASK B + * sem_init(sem, 0, 0); + * sem_wait(sem); + * sem_post(sem); + * Awakens as holder + * + * In this case priority inheritance can interfere with the operation of + * the semaphore. The problem is that when TASK A is restarted it is a + * holder of the semaphore. However, it never calls sem_post(sem) so it + * becomes *permanently* a holder of the semaphore and may have its + * priority boosted when any other task tries to acquire the semaphore. + * + * The fix is to call sem_setprotocol(SEM_PRIO_NONE) immediately after + * the sem_init() call so that there will be no priority inheritance + * operations on this semaphore. + * * Parameters: * sem - A pointer to the semaphore whose attributes are to be * modified @@ -179,6 +204,8 @@ int sem_getprotocol(FAR sem_t *sem, FAR int *protocol); #ifdef CONFIG_PRIORITY_INHERITANCE int sem_setprotocol(FAR sem_t *sem, int protocol); +#else +# define sem_setprotocol(s,p) DEBUGASSERT((p) == SEM_PRIO_NONE); #endif #undef EXTERN diff --git a/include/pthread.h b/include/pthread.h index e25e63d41f..1c368d737b 100644 --- a/include/pthread.h +++ b/include/pthread.h @@ -40,18 +40,19 @@ * Included Files ********************************************************************************/ -#include /* Default settings */ -#include /* Compiler settings, noreturn_function */ +#include /* Default settings */ +#include /* Compiler settings, noreturn_function */ -#include /* Needed for general types */ -#include /* Needed by pthread_[set|get]name_np */ +#include /* Needed for general types */ +#include /* Needed by pthread_[set|get]name_np */ -#include /* C99 fixed width integer types */ -#include /* C99 boolean types */ -#include /* For getpid */ -#include /* Needed for sem_t */ -#include /* Needed for sigset_t, includes this file */ -#include /* Needed for struct timespec */ +#include /* C99 fixed width integer types */ +#include /* C99 boolean types */ +#include /* For getpid */ +#include /* Needed for sigset_t, includes this file */ +#include /* Needed for struct timespec */ + +#include /* For sem_t and SEM_PRIO_* defines */ /******************************************************************************** * Pre-processor Definitions @@ -112,9 +113,7 @@ #define PTHREAD_INHERIT_SCHED 0 #define PTHREAD_EXPLICIT_SCHED 1 -#define PTHREAD_PRIO_NONE 0 -#define PTHREAD_PRIO_INHERIT 1 -#define PTHREAD_PRIO_PROTECT 2 +/* Default priority */ #define PTHREAD_DEFAULT_PRIORITY 100 @@ -135,7 +134,7 @@ #define PTHREAD_BARRIER_SERIAL_THREAD 0x1000 -/* Values for protocol attribute */ +/* Values for protocol mutex attribute */ #define PTHREAD_PRIO_NONE SEM_PRIO_NONE #define PTHREAD_PRIO_INHERIT SEM_PRIO_INHERIT diff --git a/include/semaphore.h b/include/semaphore.h index 964d753bd9..8821b129d0 100644 --- a/include/semaphore.h +++ b/include/semaphore.h @@ -45,24 +45,10 @@ #include #include -#ifdef __cplusplus -#define EXTERN extern "C" -extern "C" -{ -#else -#define EXTERN extern -#endif - /**************************************************************************** * Pre-processor Definitions ****************************************************************************/ -/* Values for protocol attribute */ - -#define SEM_PRIO_NONE 0 -#define SEM_PRIO_INHERIT 1 -#define SEM_PRIO_PROTECT 2 - /* Bit definitions for the struct sem_s flags field */ #define PRIOINHERIT_FLAGS_DISABLE (1 << 0) /* Bit 0: Priority inheritance @@ -130,6 +116,14 @@ typedef struct sem_s sem_t; * Public Data ****************************************************************************/ +#ifdef __cplusplus +#define EXTERN extern "C" +extern "C" +{ +#else +#define EXTERN extern +#endif + /**************************************************************************** * Public Function Prototypes ****************************************************************************/ diff --git a/sched/semaphore/sem_setprotocol.c b/sched/semaphore/sem_setprotocol.c index daacca9f30..1350206941 100644 --- a/sched/semaphore/sem_setprotocol.c +++ b/sched/semaphore/sem_setprotocol.c @@ -58,6 +58,25 @@ * Description: * Set semaphore protocol attribute. * + * One particularly important use of this furnction is when a semaphore + * is used for inter-task communication like: + * + * TASK A TASK B + * sem_init(sem, 0, 0); + * sem_wait(sem); + * sem_post(sem); + * Awakens as holder + * + * In this case priority inheritance can interfere with the operation of + * the semaphore. The problem is that when TASK A is restarted it is a + * holder of the semaphore. However, it never calls sem_post(sem) so it + * becomes *permanently* a holder of the semaphore and may have its + * priority boosted when any other task tries to acquire the semaphore. + * + * The fix is to call sem_setprotocol(SEM_PRIO_NONE) immediately after + * the sem_init() call so that there will be no priority inheritance + * operations on this semaphore. + * * Parameters: * sem - A pointer to the semaphore whose attributes are to be * modified