From bbe3db55f26c2aef2f2ba27bf726bee1ec039b24 Mon Sep 17 00:00:00 2001 From: patacongo Date: Tue, 28 Aug 2012 14:40:12 +0000 Subject: Add some protection to the priority inheritance logic when sem_post() is called from an interrupt handler git-svn-id: https://nuttx.svn.sourceforge.net/svnroot/nuttx/trunk@5060 7fd9a85b-ad96-42d3-883c-3090e2eb8679 --- nuttx/sched/sem_holder.c | 296 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 215 insertions(+), 81 deletions(-) (limited to 'nuttx/sched/sem_holder.c') diff --git a/nuttx/sched/sem_holder.c b/nuttx/sched/sem_holder.c index 34f88185a..abe7e9e28 100644 --- a/nuttx/sched/sem_holder.c +++ b/nuttx/sched/sem_holder.c @@ -275,10 +275,10 @@ static int sem_boostholderprio(FAR struct semholder_s *pholder, FAR _TCB *htcb = (FAR _TCB *)pholder->htcb; FAR _TCB *rtcb = (FAR _TCB*)arg; - /* Make sure that the thread is still active. If it exited without releasing - * its counts, then that would be a bad thing. But we can take no real - * action because we don't know know that the program is doing. Perhaps its - * plan is to kill a thread, then destroy the semaphore. + /* Make sure that the holder thread is still active. If it exited without + * releasing its counts, then that would be a bad thing. But we can take no + * real action because we don't know know that the program is doing. Perhaps + * its plan is to kill a thread, then destroy the semaphore. */ if (!sched_verifytcb(htcb)) @@ -303,15 +303,16 @@ static int sem_boostholderprio(FAR struct semholder_s *pholder, if (rtcb->sched_priority > htcb->sched_priority) { - /* If the current priority has already been boosted, then add the - * boost priority to the list of restoration priorities. When the - * higher priority thread gets its count, then we need to revert - * to this saved priority, not to the base 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 */ + /* Save the current, boosted priority of the holder thread. */ if (htcb->npend_reprio < CONFIG_SEM_NNESTPRIO) { @@ -324,10 +325,10 @@ static int sem_boostholderprio(FAR struct semholder_s *pholder, } } - /* Raise the priority of the holder of the semaphore. This - * cannot cause a context switch because we have preemption - * disabled. The task will be marked "pending" and the switch - * will occur during up_block_task() processing. + /* 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. */ (void)sched_setpriority(htcb, rtcb->sched_priority); @@ -422,10 +423,10 @@ static int sem_restoreholderprio(FAR struct semholder_s *pholder, FAR sem_t *sem int j; #endif - /* Make sure that the thread is still active. If it exited without releasing - * its counts, then that would be a bad thing. But we can take no real - * action because we don't know know that the program is doing. Perhaps its - * plan is to kill a thread, then destroy the semaphore. + /* Make sure that the hdoler thread is still active. If it exited without + * releasing its counts, then that would be a bad thing. But we can take no + * real action because we don't know know that the program is doing. Perhaps + * its plan is to kill a thread, then destroy the semaphore. */ if (!sched_verifytcb(htcb)) @@ -434,8 +435,8 @@ static int sem_restoreholderprio(FAR struct semholder_s *pholder, FAR sem_t *sem sem_freeholder(sem, pholder); } - /* Was the priority of this thread boosted? If so, then drop its priority - * back to the correct level. + /* Was the priority of the holder thread boosted? If so, then drop its + * priority back to the correct level. What is the correct level? */ else if (htcb->sched_priority != htcb->base_priority) @@ -445,23 +446,23 @@ static int sem_restoreholderprio(FAR struct semholder_s *pholder, FAR sem_t *sem if (htcb->npend_reprio < 1) { - /* No... the thread has only been boosted once. Reset all priorities - * back to the base priority. + /* No... the holder thread has only been boosted once. Reset all + * priorities back to the base priority. */ DEBUGASSERT(htcb->sched_priority == stcb->sched_priority && htcb->npend_reprio == 0); sched_reprioritize(htcb, htcb->base_priority); } - /* There are multiple pending priority levels. The thread's "boosted" + /* 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 we boosted becuase it also holds another semaphore. + * greater if its priority we boosted becuase it also holds another semaphore). */ else if (htcb->sched_priority <= stcb->sched_priority) { - /* The thread has been boosted to the same priority as the task - * that just received the count. We will simply reprioritized + /* 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. */ @@ -492,9 +493,10 @@ static int sem_restoreholderprio(FAR struct semholder_s *pholder, FAR sem_t *sem } else { - /* The thread has been boosted to a higher priority than the task. The - * pending priority should be in he list (unless it was lost because of - * of list overflow). + /* 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 of list overflow or because the holder was + * reporioritize again unbeknownst to the priority inheritance logic). * * Search the list for the matching priority. */ @@ -522,7 +524,8 @@ static int sem_restoreholderprio(FAR struct semholder_s *pholder, FAR sem_t *sem } #else /* There is no alternative restore priorities, drop the priority - * all the way back to the threads "base" priority. + * of the holder thread all the way back to the threads "base" + * priority. */ sched_reprioritize(htcb, htcb->base_priority); @@ -533,10 +536,15 @@ static int sem_restoreholderprio(FAR struct semholder_s *pholder, FAR sem_t *sem } /**************************************************************************** - * Name: sem_restoreholderprioA & B + * Name: sem_restoreholderprioA + * + * Description: + * Reprioritize all holders except the currently executing task + * ****************************************************************************/ -static int sem_restoreholderprioA(FAR struct semholder_s *pholder, FAR sem_t *sem, FAR void *arg) +static int sem_restoreholderprioA(FAR struct semholder_s *pholder, + FAR sem_t *sem, FAR void *arg) { FAR _TCB *rtcb = (FAR _TCB*)g_readytorun.head; if (pholder->htcb != rtcb) @@ -547,7 +555,16 @@ static int sem_restoreholderprioA(FAR struct semholder_s *pholder, FAR sem_t *se return 0; } -static int sem_restoreholderprioB(FAR struct semholder_s *pholder, FAR sem_t *sem, FAR void *arg) +/**************************************************************************** + * Name: sem_restoreholderprioB + * + * Description: + * Reprioritize only the currently executing task + * + ****************************************************************************/ + +static int sem_restoreholderprioB(FAR struct semholder_s *pholder, + FAR sem_t *sem, FAR void *arg) { FAR _TCB *rtcb = (FAR _TCB*)g_readytorun.head; if (pholder->htcb == rtcb) @@ -559,6 +576,158 @@ static int sem_restoreholderprioB(FAR struct semholder_s *pholder, FAR sem_t *se return 0; } +/**************************************************************************** + * Name: sem_restorebaseprio_irq + * + * Description: + * This function is called after the an interrupt handler posts a count on + * the semaphore. It will check if we need to drop the priority of any + * threads holding a count on the semaphore. Their priority could have + * been boosted while they held the count. + * + * Parameters: + * stcb - The TCB of the task that was just started (if any). If the + * post action caused a count to be given to another thread, then stcb + * is the TCB that received the count. Note, just because stcb received + * the count, it does not mean that it it is higher priority than other + * threads. + * sem - A reference to the semaphore being posted. + * - If the semaphore count is <0 then there are still threads waiting + * for a count. stcb should be non-null and will be higher priority + * than all of the other threads still waiting. + * - If it is ==0 then stcb refers to the thread that got the last count; + * no other threads are waiting. + * - If it is >0 then there should be no threads waiting for counts and + * stcb should be null. + * + * Return Value: + * None + * + * Assumptions: + * The scheduler is locked. + * + ****************************************************************************/ + +static inline void sem_restorebaseprio_irq(FAR _TCB *stcb, FAR sem_t *sem) +{ + /* Perfom the following actions only if a new thread was given a count. + * The thread that received the count should be the highest priority + * of all threads waiting for a count from the semphore. So in that + * case, the priority of all holder threads should be dropped to the + * next highest pending priority. + */ + + if (stcb) + { + /* Drop the priority of all holder threads */ + + (void)sem_foreachholder(sem, sem_restoreholderprio, stcb); + } + + /* If there are no tasks waiting for available counts, then all holders + * should be at their base priority. + */ + +#ifdef CONFIG_DEBUG + else + { + (void)sem_foreachholder(sem, sem_verifyholder, NULL); + } +#endif +} + +/**************************************************************************** + * Name: sem_restorebaseprio_task + * + * Description: + * This function is called after the current running task releases a + * count on the semaphore. It will check if we need to drop the priority + * of any threads holding a count on the semaphore. Their priority could + * have been boosted while they held the count. + * + * Parameters: + * stcb - The TCB of the task that was just started (if any). If the + * post action caused a count to be given to another thread, then stcb + * is the TCB that received the count. Note, just because stcb received + * the count, it does not mean that it it is higher priority than other + * threads. + * sem - A reference to the semaphore being posted. + * - If the semaphore count is <0 then there are still threads waiting + * for a count. stcb should be non-null and will be higher priority + * than all of the other threads still waiting. + * - If it is ==0 then stcb refers to the thread that got the last count; + * no other threads are waiting. + * - If it is >0 then there should be no threads waiting for counts and + * stcb should be null. + * + * Return Value: + * None + * + * Assumptions: + * The scheduler is locked. + * + ****************************************************************************/ + +static inline void sem_restorebaseprio_task(FAR _TCB *stcb, FAR sem_t *sem) +{ + FAR _TCB *rtcb = (FAR _TCB*)g_readytorun.head; + FAR struct semholder_s *pholder; + + /* Perfom the following actions only if a new thread was given a count. + * The thread that received the count should be the highest priority + * of all threads waiting for a count from the semphore. So in that + * case, the priority of all holder threads should be dropped to the + * next highest pending priority. + */ + + if (stcb) + { + /* The currently executed thread should be the lower priority + * thread that just posted the count and caused this action. + * However, we cannot drop the priority of the currently running + * thread -- becuase that will cause it to be suspended. + * + * So, do this in two passes. First, reprioritizing all holders + * except for the running thread. + */ + + (void)sem_foreachholder(sem, sem_restoreholderprioA, stcb); + + /* Now, find an reprioritize only the ready to run task */ + + (void)sem_foreachholder(sem, sem_restoreholderprioB, stcb); + } + + /* If there are no tasks waiting for available counts, then all holders + * should be at their base priority. + */ + +#ifdef CONFIG_DEBUG + else + { + (void)sem_foreachholder(sem, sem_verifyholder, NULL); + } +#endif + + /* In any case, the currently executing task should have an entry in the + * list. Its counts were previously decremented; if it now holds no + * counts, then we need to remove it from the list of holders. + */ + + pholder = sem_findholder(sem, rtcb); + if (pholder) + { + /* When no more counts are held, remove the holder from the list. The + * count was decremented in sem_releaseholder. + */ + + if (pholder->counts <= 0) + { + sem_freeholder(sem, pholder); + } + } +} + /**************************************************************************** * Public Functions ****************************************************************************/ @@ -743,9 +912,10 @@ void sem_releaseholder(FAR sem_t *sem) * * Description: * This function is called after the current running task releases a - * count on the semaphore. It will check if we need to drop the priority - * of any threads holding a count on the semaphore. Their priority could - * have been boosted while they held the count. + * count on the semaphore or an interrupt handler posts a new count. It + * will check if we need to drop the priority of any threads holding a + * count on the semaphore. Their priority could have been boosted while + * they held the count. * * Parameters: * stcb - The TCB of the task that was just started (if any). If the @@ -763,7 +933,7 @@ void sem_releaseholder(FAR sem_t *sem) * stcb should be null. * * Return Value: - * 0 (OK) or -1 (ERROR) if unsuccessful + * None * * Assumptions: * The scheduler is locked. @@ -772,61 +942,25 @@ void sem_releaseholder(FAR sem_t *sem) void sem_restorebaseprio(FAR _TCB *stcb, FAR sem_t *sem) { - FAR _TCB *rtcb = (FAR _TCB*)g_readytorun.head; - FAR struct semholder_s *pholder; - /* Check our assumptions */ DEBUGASSERT((sem->semcount > 0 && stcb == NULL) || (sem->semcount <= 0 && stcb != NULL)); - /* Perfom the following actions only if a new thread was given a count. */ - - if (stcb) - { - /* The currently executed thread should be the low priority - * thread that just posted the count and caused this action. - * However, we cannot drop the priority of the currently running - * thread -- becuase that will cause it to be suspended. - * - * So, do this in two passes. First, reprioritizing all holders - * except for the running thread. - */ - - (void)sem_foreachholder(sem, sem_restoreholderprioA, stcb); - - /* Now, find an reprioritize only the ready to run task */ - - (void)sem_foreachholder(sem, sem_restoreholderprioB, stcb); - } - - /* If there are no tasks waiting for available counts, then all holders - * should be at their base priority. + /* Handler semaphore counts posed from an interrupt handler differently + * from interrupts posted from threads. The priority difference is that + * if the semaphore is posted from a thread, then the poster thread is + * a player in the priority inheritance scheme. The interrupt handler + * externally injects the new count without participated itself. */ -#ifdef CONFIG_DEBUG - else + if (up_interrupt_context()) { - (void)sem_foreachholder(sem, sem_verifyholder, NULL); + sem_restorebaseprio_irq(stcb, sem); } -#endif - - /* In any case, the currently execuing task should have an entry in the - * list. Its counts were previously decremented; if it now holds no - * counts, then we need to remove it from the list of holders. - */ - - pholder = sem_findholder(sem, rtcb); - if (pholder) + else { - /* When no more counts are held, remove the holder from the list. The - * count was decremented in sem_releaseholder. - */ - - if (pholder->counts <= 0) - { - sem_freeholder(sem, pholder); - } + sem_restorebaseprio_task(stcb, sem); } } -- cgit v1.2.3