From d247e8d1d2668901abcdd767c1360f264c051774 Mon Sep 17 00:00:00 2001 From: Petro Karashchenko Date: Tue, 24 May 2022 00:06:10 +0200 Subject: [PATCH] sched/semaphore: fix priority boost restoration for priority inheritance Signed-off-by: Petro Karashchenko --- .../reference/user/05_counting_semaphore.rst | 17 -- include/nuttx/sched.h | 19 +- sched/Kconfig | 10 - sched/sched/sched_reprioritize.c | 6 +- sched/semaphore/sem_holder.c | 200 ++-------------- sched/semaphore/sem_post.c | 2 +- sched/task/task_restart.c | 4 +- sched/wqueue/kwork_inherit.c | 224 ++++-------------- 8 files changed, 87 insertions(+), 395 deletions(-) diff --git a/Documentation/reference/user/05_counting_semaphore.rst b/Documentation/reference/user/05_counting_semaphore.rst index 3f940e7a66..693ba40769 100644 --- a/Documentation/reference/user/05_counting_semaphore.rst +++ b/Documentation/reference/user/05_counting_semaphore.rst @@ -77,23 +77,6 @@ implementation: slightly increased code size and around 6-12 bytes times the value of ``CONFIG_SEM_PREALLOCHOLDERS``. - - ``CONFIG_SEM_NNESTPRIO``: In addition, there may be multiple - threads of various priorities that need to wait for a count from the - semaphore. These, the lower priority thread holding the semaphore may - have to be boosted numerous times and, to make things more complex, - will have to keep track of all of the boost priorities values in - order to correctly restore the priorities after a count has been - handed out to the higher priority thread. The - ``CONFIG_SEM_NNESTPRIO`` defines the size of an array, one array per - active thread. This setting is the maximum number of higher priority - threads (minus 1) than can be waiting for another thread to release a - count on a semaphore. This value may be set to zero if no more than - one thread is expected to wait for a semaphore. - - The cost associated with setting ``CONFIG_SEM_NNESTPRIO`` is slightly - increased code size and (``CONFIG_SEM_PREALLOCHOLDERS`` + 1) times - the maximum number of active threads. - - **Increased Susceptibility to Bad Thread Behavior**. These various structures tie the semaphore implementation more tightly to the behavior of the implementation. For examples, if a thread executes diff --git a/include/nuttx/sched.h b/include/nuttx/sched.h index 12bbcf04af..0811d08d86 100644 --- a/include/nuttx/sched.h +++ b/include/nuttx/sched.h @@ -559,10 +559,7 @@ struct tcb_s uint8_t task_state; /* Current state of the thread */ #ifdef CONFIG_PRIORITY_INHERITANCE -#if CONFIG_SEM_NNESTPRIO > 0 - uint8_t npend_reprio; /* Number of nested reprioritizations */ - uint8_t pend_reprios[CONFIG_SEM_NNESTPRIO]; -#endif + uint8_t boost_priority; /* "Boosted" priority of the thread */ uint8_t base_priority; /* "Normal" priority of the thread */ FAR struct semholder_s *holdsem; /* List of held semaphores */ #endif @@ -635,12 +632,12 @@ struct tcb_s /* Pre-emption monitor support ********************************************/ #ifdef CONFIG_SCHED_CRITMONITOR - uint32_t premp_start; /* Time when preemption disabled */ - uint32_t premp_max; /* Max time preemption disabled */ - uint32_t crit_start; /* Time critical section entered */ - uint32_t crit_max; /* Max time in critical section */ - uint32_t run_start; /* Time when thread begin run */ - uint32_t run_max; /* Max time thread run */ + uint32_t premp_start; /* Time when preemption disabled */ + uint32_t premp_max; /* Max time preemption disabled */ + uint32_t crit_start; /* Time critical section entered */ + uint32_t crit_max; /* Max time in critical section */ + uint32_t run_start; /* Time when thread begin run */ + uint32_t run_max; /* Max time thread run */ #endif /* State save areas *******************************************************/ @@ -670,7 +667,7 @@ struct task_tcb_s { /* Common TCB fields ******************************************************/ - struct tcb_s cmn; /* Common TCB fields */ + struct tcb_s cmn; /* Common TCB fields */ /* Task Management Fields *************************************************/ diff --git a/sched/Kconfig b/sched/Kconfig index 8319b4c5ca..0d0cd5f908 100644 --- a/sched/Kconfig +++ b/sched/Kconfig @@ -1172,16 +1172,6 @@ config SEM_PREALLOCHOLDERS are only using semaphores as mutexes (only one holder) OR if no more than two threads participate using a counting semaphore. -config SEM_NNESTPRIO - int "Maximum number of higher priority threads" - default 16 - ---help--- - If priority inheritance is enabled, then this setting is the - maximum number of higher priority threads (minus 1) than can be - waiting for another thread to release a count on a semaphore. - This value may be set to zero if no more than one thread is - expected to wait for a semaphore. - endif # PRIORITY_INHERITANCE menu "RTOS hooks" diff --git a/sched/sched/sched_reprioritize.c b/sched/sched/sched_reprioritize.c index b0d8fd2792..b504349f3a 100644 --- a/sched/sched/sched_reprioritize.c +++ b/sched/sched/sched_reprioritize.c @@ -79,11 +79,9 @@ int nxsched_reprioritize(FAR struct tcb_s *tcb, int sched_priority) tcb->base_priority = (uint8_t)sched_priority; - /* Discard any pending reprioritizations as well */ + /* Discard priority boost as well */ -#if CONFIG_SEM_NNESTPRIO > 0 - tcb->npend_reprio = 0; -#endif + tcb->boost_priority = 0; } return ret; diff --git a/sched/semaphore/sem_holder.c b/sched/semaphore/sem_holder.c index 43188f884d..e2cc379015 100644 --- a/sched/semaphore/sem_holder.c +++ b/sched/semaphore/sem_holder.c @@ -325,76 +325,6 @@ static int nxsem_boostholderprio(FAR struct semholder_s *pholder, FAR struct tcb_s *htcb = (FAR struct tcb_s *)pholder->htcb; FAR struct tcb_s *rtcb = (FAR struct tcb_s *)arg; -#if CONFIG_SEM_NNESTPRIO > 0 - /* If the priority of the thread that is waiting for a count is greater - * than the base priority of the thread holding a count, then we may need - * to adjust the holder's priority now or later to that priority. - */ - - if (rtcb->sched_priority > htcb->base_priority) - { - /* If the new priority is greater than the current, possibly already - * boosted priority of the holder thread, then we will have to raise - * the holder's priority now. - */ - - if (rtcb->sched_priority > htcb->sched_priority) - { - /* If the current priority of holder thread has already been - * boosted, then add the boost priority to the list of restoration - * priorities. When the higher priority waiter thread gets its - * count, then we need to revert the holder thread to this saved - * priority (not to its base priority). - */ - - if (htcb->sched_priority > htcb->base_priority) - { - /* Save the current, boosted priority of the holder thread. */ - - if (htcb->npend_reprio < CONFIG_SEM_NNESTPRIO) - { - htcb->pend_reprios[htcb->npend_reprio] = - htcb->sched_priority; - htcb->npend_reprio++; - } - else - { - serr("ERROR: CONFIG_SEM_NNESTPRIO exceeded\n"); - DEBUGASSERT(htcb->npend_reprio < CONFIG_SEM_NNESTPRIO); - } - } - - /* Raise the priority of the thread holding of the semaphore. - * This cannot cause a context switch because we have preemption - * disabled. The holder thread may be marked "pending" and the - * switch may occur during up_block_task() processing. - */ - - nxsched_set_priority(htcb, rtcb->sched_priority); - } - else - { - /* The new priority is above the base priority of the holder, - * but not as high as its current working priority. Just put it - * in the list of pending restoration priorities so that when the - * higher priority thread gets its count, we can revert to this - * saved priority and not to the base priority. - */ - - if (htcb->npend_reprio < CONFIG_SEM_NNESTPRIO) - { - htcb->pend_reprios[htcb->npend_reprio] = rtcb->sched_priority; - htcb->npend_reprio++; - } - else - { - serr("ERROR: CONFIG_SEM_NNESTPRIO exceeded\n"); - DEBUGASSERT(htcb->npend_reprio < CONFIG_SEM_NNESTPRIO); - } - } - } - -#else /* If the priority of the thread that is waiting for a count is less than * or equal to the priority of the thread holding a count, then do nothing * because the thread is already running at a sufficient priority. @@ -410,7 +340,6 @@ static int nxsem_boostholderprio(FAR struct semholder_s *pholder, nxsched_set_priority(htcb, rtcb->sched_priority); } -#endif return 0; } @@ -435,9 +364,6 @@ static int nxsem_verifyholder(FAR struct semholder_s *pholder, * In this case, the priority of the holder should not be boosted. */ -#if CONFIG_SEM_NNESTPRIO > 0 - DEBUGASSERT(htcb->npend_reprio == 0); -#endif DEBUGASSERT(htcb->sched_priority == htcb->base_priority); #endif @@ -473,12 +399,7 @@ static int nxsem_restoreholderprio(FAR struct semholder_s *pholder, FAR sem_t *sem, FAR void *arg) { FAR struct tcb_s *htcb = pholder->htcb; -#if CONFIG_SEM_NNESTPRIO > 0 - FAR struct tcb_s *stcb = (FAR struct tcb_s *)arg; - int rpriority; - int i; - int j; -#endif + int hpriority; /* Release the holder if all counts have been given up * before reprioritizing causes a context switch. @@ -489,117 +410,42 @@ static int nxsem_restoreholderprio(FAR struct semholder_s *pholder, nxsem_freeholder(sem, pholder); } + /* We attempt to restore thread priority to its base priority. If + * there is any thread with the higher priority waiting for the + * semaphore held by htcb then this value will be overwritten. + */ + + hpriority = htcb->boost_priority > htcb->base_priority ? + htcb->boost_priority : htcb->base_priority; + /* Was the priority of the holder thread boosted? If so, then drop its * priority back to the correct level. What is the correct level? */ - if (htcb->sched_priority != htcb->base_priority) + if (htcb->sched_priority != hpriority) { -#if CONFIG_SEM_NNESTPRIO > 0 - /* Are there other, pending priority levels to revert to? */ - - if (htcb->npend_reprio < 1) - { - /* No... the holder thread has only been boosted once. - * npend_reprio should be 0 and the boosted priority should be the - * priority of the task that just got the semaphore - * (stcb->sched_priority) - * - * That latter assumption may not be true if the stcb's priority - * was also boosted so that it no longer matches the htcb's - * sched_priority. Or if CONFIG_SEM_NNESTPRIO is too small (so - * that we do not have a proper record of the reprioritizations). - */ - - DEBUGASSERT(/* htcb->sched_priority == stcb->sched_priority && */ - htcb->npend_reprio == 0); - - /* Reset the holder's priority back to the base priority. */ - - nxsched_reprioritize(htcb, htcb->base_priority); - } - - /* There are multiple pending priority levels. The holder thread's - * "boosted" priority could greater than or equal to - * "stcb->sched_priority" (it could be greater if its priority was - * boosted because it also holds another semaphore). + /* Try to find the highest priority across all the threads that are + * waiting for any semaphore held by htcb. */ - else if (htcb->sched_priority <= stcb->sched_priority) + for (pholder = htcb->holdsem; pholder != NULL; + pholder = pholder->tlink) { - /* The holder thread has been boosted to the same priority as the - * waiter thread that just received the count. We will simply - * reprioritize to the next highest priority that we have in - * rpriority. - */ + FAR struct tcb_s *stcb; - /* Find the highest pending priority and remove it from the list */ + stcb = (FAR struct tcb_s *)dq_peek(SEM_WAITLIST(pholder->sem)); - for (i = 1, j = 0; i < htcb->npend_reprio; i++) + if (stcb != NULL && stcb->sched_priority > hpriority) { - if (htcb->pend_reprios[i] > htcb->pend_reprios[j]) - { - j = i; - } - } - - /* Remove the highest priority pending priority from the list */ - - rpriority = htcb->pend_reprios[j]; - i = htcb->npend_reprio - 1; - if (i > 0) - { - htcb->pend_reprios[j] = htcb->pend_reprios[i]; - } - - htcb->npend_reprio = i; - - /* And apply that priority to the thread (while retaining the - * base_priority) - */ - - nxsched_set_priority(htcb, rpriority); - } - else - { - /* The holder thread has been boosted to a higher priority than the - * waiter task. The pending priority should be in the list (unless - * it was lost because of list overflow or because the holder - * was reprioritized again unbeknownst to the priority inheritance - * logic). - * - * Search the list for the matching priority. - */ - - for (i = 0; i < htcb->npend_reprio; i++) - { - /* Does this pending priority match the priority of the thread - * that just received the count? - */ - - if (htcb->pend_reprios[i] == stcb->sched_priority) - { - /* Yes, remove it from the list */ - - j = htcb->npend_reprio - 1; - if (j > 0) - { - htcb->pend_reprios[i] = htcb->pend_reprios[j]; - } - - htcb->npend_reprio = j; - break; - } + hpriority = stcb->sched_priority; } } -#else - /* There is no alternative restore priorities, drop the priority - * of the holder thread all the way back to the threads "base" - * priority. + + /* Apply the selected priority to the thread (hopefully back to the + * threads base_priority). */ - nxsched_reprioritize(htcb, htcb->base_priority); -#endif + nxsched_set_priority(htcb, hpriority); } return 0; @@ -952,7 +798,7 @@ void nxsem_release_holder(FAR sem_t *sem) /* Find the container for this holder */ #if CONFIG_SEM_PREALLOCHOLDERS > 0 - for (pholder = sem->hhead; pholder; pholder = pholder->flink) + for (pholder = sem->hhead; pholder != NULL; pholder = pholder->flink) #else int i; diff --git a/sched/semaphore/sem_post.c b/sched/semaphore/sem_post.c index 21bb59fbbb..d5fd3842df 100644 --- a/sched/semaphore/sem_post.c +++ b/sched/semaphore/sem_post.c @@ -134,7 +134,7 @@ int nxsem_post(FAR sem_t *sem) if (sem_count <= 0) { /* Check if there are any tasks in the waiting for semaphore - * task list that are waiting for this semaphore. This is a + * task list that are waiting for this semaphore. This is a * prioritized list so the first one we encounter is the one * that we want. */ diff --git a/sched/task/task_restart.c b/sched/task/task_restart.c index 0a04c1481d..4f4c131031 100644 --- a/sched/task/task_restart.c +++ b/sched/task/task_restart.c @@ -165,9 +165,7 @@ int nxtask_restart(pid_t pid) #ifdef CONFIG_PRIORITY_INHERITANCE tcb->cmn.base_priority = tcb->cmn.init_priority; -# if CONFIG_SEM_NNESTPRIO > 0 - tcb->cmn.npend_reprio = 0; -# endif + tcb->cmn.boost_priority = 0; #endif /* Re-initialize the processor-specific portion of the TCB. This will diff --git a/sched/wqueue/kwork_inherit.c b/sched/wqueue/kwork_inherit.c index 2acfeb6575..a54fb790a1 100644 --- a/sched/wqueue/kwork_inherit.c +++ b/sched/wqueue/kwork_inherit.c @@ -66,7 +66,10 @@ static void lpwork_boostworker(pid_t wpid, uint8_t reqprio) wtcb = nxsched_get_tcb(wpid); DEBUGASSERT(wtcb); -#if CONFIG_SEM_NNESTPRIO > 0 + /* REVISIT: Priority multi-boost is not supported */ + + DEBUGASSERT(wtcb->boost_priority == 0); + /* If the priority of the client thread that is greater than the base * priority of the worker thread, then we may need to adjust the worker * thread's priority now or later to that priority. @@ -74,83 +77,29 @@ static void lpwork_boostworker(pid_t wpid, uint8_t reqprio) if (reqprio > wtcb->base_priority) { - /* If the new priority is greater than the current, possibly already - * boosted priority of the worker thread, then we will have to raise - * the worker thread's priority now. + /* Save boost priority value as it might be needed in case of multiple + * re-prioritisations happen, then the priority of the thread can't go + * below the boost priority value until priority boost is canceled. + */ + + wtcb->boost_priority = reqprio; + + /* If the priority of the client thread that is less than of equal to + * the priority of the worker thread, then do nothing because the + * thread is already running at a sufficient priority. */ if (reqprio > wtcb->sched_priority) { - /* If the current priority of worker thread has already been - * boosted, then add the boost priority to the list of restoration - * priorities. When the higher priority waiter thread gets its - * count, then we need to revert the worker thread to this saved - * priority (not to its base priority). - */ - - if (wtcb->sched_priority > wtcb->base_priority) - { - /* Save the current, boosted priority of the worker thread. */ - - if (wtcb->npend_reprio < CONFIG_SEM_NNESTPRIO) - { - wtcb->pend_reprios[wtcb->npend_reprio] = - wtcb->sched_priority; - wtcb->npend_reprio++; - } - else - { - serr("ERROR: CONFIG_SEM_NNESTPRIO exceeded\n"); - DEBUGASSERT(wtcb->npend_reprio < CONFIG_SEM_NNESTPRIO); - } - } - - /* Raise the priority of the worker. This cannot cause a context - * switch because we have preemption disabled. The worker thread - * may be marked "pending" and the switch may occur during - * sched_unblock() processing. + /* Raise the priority of the worker thread. This cannot cause + * context switch because we have preemption disabled. The task + * will be marked "pending" and the switch will occur during + * sched_unlock() processing. */ nxsched_set_priority(wtcb, reqprio); } - else - { - /* The new priority is above the base priority of the worker, - * but not as high as its current working priority. Just put it - * in the list of pending restoration priorities so that when the - * higher priority thread gets its count, we can revert to this - * saved priority and not to the base priority. - */ - - if (wtcb->npend_reprio < CONFIG_SEM_NNESTPRIO) - { - wtcb->pend_reprios[wtcb->npend_reprio] = reqprio; - wtcb->npend_reprio++; - } - else - { - serr("ERROR: CONFIG_SEM_NNESTPRIO exceeded\n"); - DEBUGASSERT(wtcb->npend_reprio < CONFIG_SEM_NNESTPRIO); - } - } } -#else - /* If the priority of the client thread that is less than of equal to the - * priority of the worker thread, then do nothing because the thread is - * already running at a sufficient priority. - */ - - if (reqprio > wtcb->sched_priority) - { - /* Raise the priority of the worker thread. This cannot cause - * context switch because we have preemption disabled. The task - * will be marked "pending" and the switch will occur during - * sched_unlock() processing. - */ - - nxsched_set_priority(wtcb, reqprio); - } -#endif } /**************************************************************************** @@ -174,127 +123,58 @@ static void lpwork_boostworker(pid_t wpid, uint8_t reqprio) static void lpwork_restoreworker(pid_t wpid, uint8_t reqprio) { FAR struct tcb_s *wtcb; -#if CONFIG_SEM_NNESTPRIO > 0 - uint8_t wpriority; - int index; - int selected; -#endif /* Get the TCB of the low priority worker thread from the process ID. */ wtcb = nxsched_get_tcb(wpid); DEBUGASSERT(wtcb); + /* REVISIT: Priority multi-boost is not supported. */ + + DEBUGASSERT(wtcb->boost_priority == reqprio); + + /* Clear the threat boost priority. */ + + wtcb->boost_priority = 0; + /* Was the priority of the worker thread boosted? If so, then drop its * priority back to the correct level. What is the correct level? */ if (wtcb->sched_priority != wtcb->base_priority) { -#if CONFIG_SEM_NNESTPRIO > 0 - /* Are there other, pending priority levels to revert to? */ + FAR struct semholder_s *pholder; + uint8_t wpriority; - if (wtcb->npend_reprio < 1) - { - /* No... the worker thread has only been boosted once. - * npend_reprio should be 0 and the boosted priority should be the - * priority of the client task (reqprio) - * - * That latter assumption may not be true if the client's priority - * was also boosted so that it no longer matches the wtcb's - * sched_priority. Or if CONFIG_SEM_NNESTPRIO is too small (so - * that we do not have a proper record of the reprioritizations). - */ - - DEBUGASSERT(/* wtcb->sched_priority == reqprio && */ - wtcb->npend_reprio == 0); - - /* Reset the worker's priority back to the base priority. */ - - nxsched_reprioritize(wtcb, wtcb->base_priority); - } - - /* There are multiple pending priority levels. The worker thread's - * "boosted" priority could greater than or equal to "reqprio" (it - * could be greater if its priority we boosted because it also holds - * some semaphore). + /* We attempt to restore task priority to its base priority. If there + * is any task with the higher priority waiting for the semaphore + * held by wtcb then this value will be overwritten. */ - else if (wtcb->sched_priority <= reqprio) - { - /* The worker thread has been boosted to the same priority as the - * waiter thread that just received the count. We will simply - * reprioritize to the next highest pending priority. - */ + wpriority = wtcb->base_priority; - /* Find the highest pending priority and remove it from the list */ - - for (index = 1, selected = 0; index < wtcb->npend_reprio; index++) - { - if (wtcb->pend_reprios[index] > wtcb->pend_reprios[selected]) - { - selected = index; - } - } - - /* Remove the highest priority pending priority from the list */ - - wpriority = wtcb->pend_reprios[selected]; - index = wtcb->npend_reprio - 1; - if (index > 0) - { - wtcb->pend_reprios[selected] = wtcb->pend_reprios[index]; - } - - wtcb->npend_reprio = index; - - /* And apply that priority to the thread (while retaining the - * base_priority) - */ - - nxsched_set_priority(wtcb, wpriority); - } - else - { - /* The worker thread has been boosted to a higher priority than the - * waiter task. The pending priority should be in the list (unless - * it was lost because of list overflow or because the worker was - * reprioritized again unbeknownst to the priority inheritance - * logic). - * - * Search the list for the matching priority. - */ - - for (index = 0; index < wtcb->npend_reprio; index++) - { - /* Does this pending priority match the priority of the thread - * that just received the count? - */ - - if (wtcb->pend_reprios[index] == reqprio) - { - /* Yes, remove it from the list */ - - selected = wtcb->npend_reprio - 1; - if (selected > 0) - { - wtcb->pend_reprios[index] = - wtcb->pend_reprios[selected]; - } - - wtcb->npend_reprio = selected; - break; - } - } - } -#else - /* There is no alternative restore priorities, drop the priority - * of the worker thread all the way back to the threads "base" - * priority. + /* Try to find the highest priority across all the tasks that are + * waiting for any semaphore held by wtcb. */ - nxsched_reprioritize(wtcb, wtcb->base_priority); -#endif + for (pholder = wtcb->holdsem; pholder != NULL; + pholder = pholder->tlink) + { + FAR struct tcb_s *stcb; + + stcb = (FAR struct tcb_s *)dq_peek(SEM_WAITLIST(pholder->sem)); + + if (stcb != NULL && stcb->sched_priority > wpriority) + { + wpriority = stcb->sched_priority; + } + } + + /* Apply the selected priority to the worker thread (hopefully back + * to the threads base_priority). + */ + + nxsched_set_priority(wtcb, wpriority); } }