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

apps/ChangeLog.txt

@@ -285,4 +285,9 @@
 	  list (contributed by Kate).
 	* apps/nshlib/nsh_parse.c: CONFIG_NSH_DISABLE_WGET not CONFIG_NSH_DISABLE_GET
 	  in one location (found by Kate).
+	* apps/examples/ostest/prioinherit.c:  Limit the number of test
+	  threds to no more than 3 of each priority.  Bad things happen
+	  when the existing logic tried to created several hundred test
+	  treads!
+
 

apps/examples/ostest/prioinherit.c

@@ -59,20 +59,28 @@
 #  define CONFIG_SEM_PREALLOCHOLDERS 0
 #endif
 
+/* If resources were configured for lots of holders, then run 3 low priority
+ * threads.  Otherwise, just one.
+ */
+
 #if CONFIG_SEM_PREALLOCHOLDERS > 3
 #  define NLOWPRI_THREADS 3
 #else
-#  define NLOWPRI_THREADS (CONFIG_SEM_PREALLOCHOLDERS+1)
+#  define NLOWPRI_THREADS 1
 #endif
 
 #ifndef CONFIG_SEM_NNESTPRIO
 #  define CONFIG_SEM_NNESTPRIO 0
 #endif
 
+/* Where resources configured for lots of waiters?  If so then run 3 high
+ * priority threads.  Otherwise, just one.
+ */
+
 #if CONFIG_SEM_NNESTPRIO > 3
 #  define NHIGHPRI_THREADS 3
 #else
-#  define NHIGHPRI_THREADS (CONFIG_SEM_NNESTPRIO+1)
+#  define NHIGHPRI_THREADS 1
 #endif
 
 /****************************************************************************

nuttx/Documentation/NuttxPortingGuide.html

@@ -4005,6 +4005,9 @@ build
     This may be set to zero if priority inheritance is disabled OR if you
     are only using semaphores as mutexes (only one holder) OR if no more
     than two threads participate using a counting semaphore.
+    If defined, then this should be a relatively large number because this
+    is the total number of counts on the total number of semaphores (like
+    64 or 100).
   </li>
   <li>
     <code>CONFIG_SEM_NNESTPRIO</code>: If priority inheritance is enabled,
@@ -4012,6 +4015,8 @@ build
     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.
+    If defined, then this should be a relatively small number because this the
+    number of maximumum of waiters on one semaphore (like 4 or 8).
   </li>
   <li>
     <code>CONFIG_FDCLONE_DISABLE</code>: Disable cloning of all file descriptors

nuttx/configs/README.txt

@@ -323,13 +323,17 @@ defconfig -- This is a configuration file similar to the Linux
       set to zero if priority inheritance is disabled OR if you
       are only using semaphores as mutexes (only one holder) OR
       if no more than two threads participate using a counting
-      semaphore.
+      semaphore.  If defined, then this should be a relatively
+      large number because this is the total number of counts on
+      the total number of semaphores (like 64 or 100).
     CONFIG_SEM_NNESTPRIO.  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.
+      a semaphore.    If defined, then this should be a relatively
+      small number because this the number of maximumum of waiters
+      on one semaphore (like 4 or 8).
     CONFIG_FDCLONE_DISABLE. Disable cloning of all file descriptors
       by task_create() when a new task is started.  If set, all
         files/drivers will appear to be closed in the new task.

nuttx/fs/nxffs/README.txt

@@ -154,7 +154,7 @@ Things to Do
 - The file name is always extracted and held in allocated, variable-length
   memory.  The file name is not used during reading and eliminating the
   file name in the entry structure would improve performance.
-- There is a big inefficient in reading.  On each read, the logic searches
+- There is a big inefficiency in reading.  On each read, the logic searches
   for the read position from the beginning of the file each time.  This
   may be necessary whenever an lseek() is done, but not in general.  Read
   performance could be improved by keeping FLASH offset and read positional
@@ -168,4 +168,13 @@ Things to Do
   FLASH.  As the file system becomes more filled with fixed files at the
   front of the device, the level of wear on the blocks at the end of the
   FLASH increases.
+- When the time comes to reorganization the FLASH, the system may be
+  inavailable for a long time.  That is a bad behavior.  What is needed,
+  I think, is a garbage collection task that runs periodically so that
+  when the big reorganizaiton event occurs, most of the work is already
+  done.  That garbarge collection should search for valid blocks that no
+  longer contain valid data.  It should pre-erase them, put them in
+  a good but empty state... all ready for file system re-organization.
+ 
+
 

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
+  /* Make sure that the holder thread is still active.  If it exited without
-   * its counts, then that would be a bad thing.  But we can take no real
+   * releasing its counts, then that would be a bad thing.  But we can take no
-   * action because we don't know know that the program is doing.  Perhaps its
+   * real action because we don't know know that the program is doing.  Perhaps
-   * plan is to kill a thread, then destroy the semaphore.
+   * 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
+          /* If the current priority of holder thread has already been
-           * boost priority to the list of restoration priorities.  When the
+           * boosted, then add the boost priority to the list of restoration
-           * higher priority thread gets its count, then we need to revert
+           * priorities.  When the higher priority waiter thread gets its
-           * to this saved priority, not to the base priority.
+           * 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
+          /* Raise the priority of the thread holding of the semaphore.
-           * cannot cause a context switch because we have preemption
+           * This cannot cause a context switch because we have preemption
-           * disabled.  The task will be marked "pending" and the switch
+           * disabled.  The holder thread may be marked "pending" and the
-           * will occur during up_block_task() processing.
+           * 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
+  /* Make sure that the hdoler thread is still active.  If it exited without
-   * its counts, then that would be a bad thing.  But we can take no real
+   * releasing its counts, then that would be a bad thing.  But we can take no
-   * action because we don't know know that the program is doing.  Perhaps its
+   * real action because we don't know know that the program is doing.  Perhaps
-   * plan is to kill a thread, then destroy the semaphore.
+   * 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
+  /* Was the priority of the holder thread boosted? If so, then drop its
-   * back to the correct level.
+   * 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
+          /* No... the holder thread has only been boosted once.  Reset all
-           * back to the base priority.
+           * 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
+          /* The holder thread has been boosted to the same priority as the waiter
-           * that just received the count.  We will simply reprioritized
+           * 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
+          /* The holder thread has been boosted to a higher priority than the
-           * pending priority should be in he list (unless it was lost because of
+           * waiter task.  The pending priority should be in the list (unless it
-           * of list overflow).
+           * 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
+ *   count on the semaphore or an interrupt handler posts a new count.  It
- *   of any threads holding a count on the semaphore.  Their priority could
+ *   will check if we need to drop the priority of any threads holding a
- *   have been boosted while they held the count.
+ *   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. */
+  /* Handler semaphore counts posed from an interrupt handler differently
-
+   * from interrupts posted from threads.  The priority difference is that
-  if (stcb)
+   * if the semaphore is posted from a thread, then the poster thread is
-    {
+   * a player in the priority inheritance scheme.  The interrupt handler
-      /* The currently executed thread should be the low priority
+   * externally injects the new count without participated itself.
-       * 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
+  if (up_interrupt_context())
+    {
+      sem_restorebaseprio_irq(stcb, sem);
+    }
   else
     {
-      (void)sem_foreachholder(sem, sem_verifyholder, NULL);
+      sem_restorebaseprio_task(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)
-    {
-      /* 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);
-        }
     }
 }
 

nuttx/sched/sem_post.c

@@ -123,17 +123,21 @@ int sem_post(FAR sem_t *sem)
       sem_releaseholder(sem);
       sem->semcount++;
 
-      /* If the result of of semaphore unlock is non-positive, then
-       * there must be some task waiting for the semaphore.
-       */
-
 #ifdef CONFIG_PRIORITY_INHERITANCE
-      /* Don't let it run until we complete the priority restoration
+      /* Don't let any unblocked tasks run until we complete any priority
-       * steps.
+       * restoration steps.  Interrupts are disabled, but we do not want
+       * the head of the read-to-run list to be modified yet.
+       *
+       * NOTE: If this sched_lock is called from an interrupt handler, it
+       * will do nothing.
        */
 
       sched_lock();
 #endif
+      /* If the result of of semaphore unlock is non-positive, then
+       * there must be some task waiting for the semaphore.
+       */
+
       if (sem->semcount <= 0)
         {
           /* Check if there are any tasks in the waiting for semaphore