[EXPERIMENTAL]sched/hrtimr: Allow running/armed hrtimer to be restarted

Allow running/armed hrtimer to be restarted to
  fix hrtimer bug: #17567

Co-authored-by: ouyangxiangzhen <ouyangxiangzhen@xiaomi.com>
Signed-off-by: Chengdong Wang <wangchengdong@lixiang.com>

include/nuttx/hrtimer.h

@@ -51,20 +51,6 @@ enum hrtimer_mode_e
   HRTIMER_MODE_REL       /* Relative delay from now */
 };
 
-/* High-resolution timer states
- *
- * State transitions are managed internally by the hrtimer framework.
- * Callers must not modify the state directly.
- */
-
-enum hrtimer_state_e
-{
-  HRTIMER_STATE_INACTIVE = 0, /* Timer is inactive and not queued */
-  HRTIMER_STATE_ARMED,        /* Timer is armed and waiting for expiry */
-  HRTIMER_STATE_RUNNING,      /* Timer callback is currently executing */
-  HRTIMER_STATE_CANCELED      /* Timer canceled (callback may be running) */
-};
-
 /* Forward declarations */
 
 struct hrtimer_s;
@@ -79,7 +65,8 @@ typedef struct hrtimer_node_s hrtimer_node_t;
  * timer context and must not block.
  */
 
-typedef uint32_t (*hrtimer_cb)(FAR struct hrtimer_s *hrtimer);
+typedef CODE uint64_t
+(*hrtimer_cb)(FAR hrtimer_t *hrtimer, uint64_t expired);
 
 /* Red-black tree node used to order hrtimers by expiration time */
 
@@ -97,11 +84,9 @@ struct hrtimer_node_s
 
 struct hrtimer_s
 {
-  hrtimer_node_t          node;    /* RB-tree node for sorted insertion */
-  enum hrtimer_state_e    state;   /* Current timer state */
-  hrtimer_cb              func;    /* Expiration callback function */
-  FAR void               *arg;     /* Argument passed to callback */
-  uint64_t                expired; /* Absolute expiration time (ns) */
+  hrtimer_node_t node;   /* RB-tree node for sorted insertion */
+  hrtimer_cb func;       /* Expiration callback function */
+  uint64_t expired;      /* Absolute expiration time (ns) */
 };
 
 /****************************************************************************
@@ -134,13 +119,10 @@ extern "C"
  ****************************************************************************/
 
 static inline_function
-void hrtimer_init(FAR hrtimer_t *hrtimer,
-                  hrtimer_cb func,
-                  FAR void *arg)
+void hrtimer_init(FAR hrtimer_t *hrtimer, hrtimer_cb func)
 {
-  hrtimer->state = HRTIMER_STATE_INACTIVE;
-  hrtimer->func  = func;
-  hrtimer->arg   = arg;
+  memset(hrtimer, 0, sizeof(hrtimer_t));
+  hrtimer->func = func;
 }
 
 /****************************************************************************
@@ -202,7 +184,7 @@ int hrtimer_cancel_sync(FAR hrtimer_t *hrtimer);
  *
  * Input Parameters:
  *   hrtimer - Timer instance to start
- *   ns      - Expiration time in nanoseconds
+ *   expired - Expiration time in nanoseconds
  *   mode    - HRTIMER_MODE_ABS or HRTIMER_MODE_REL
  *
  * Returned Value:
@@ -210,7 +192,7 @@ int hrtimer_cancel_sync(FAR hrtimer_t *hrtimer);
  ****************************************************************************/
 
 int hrtimer_start(FAR hrtimer_t *hrtimer,
-                  uint64_t ns,
+                  uint64_t expired,
                   enum hrtimer_mode_e mode);
 
 #undef EXTERN

sched/hrtimer/hrtimer.h

@@ -32,6 +32,20 @@
 #include <nuttx/clock.h>
 #include <nuttx/hrtimer.h>
 
+#ifdef CONFIG_HRTIMER
+
+/****************************************************************************
+ * Public Types
+ ****************************************************************************/
+
+/* Red-black tree head for managing active high-resolution timers.
+ *
+ * Timers are ordered by expiration time, the earliest expiring timer
+ * being the left-most (minimum) node in the tree.
+ */
+
+RB_HEAD(hrtimer_tree_s, hrtimer_node_s);
+
 /****************************************************************************
  * Public Data
  ****************************************************************************/
@@ -44,13 +58,11 @@ extern spinlock_t g_hrtimer_spinlock;
 
 extern struct hrtimer_tree_s g_hrtimer_tree;
 
-/****************************************************************************
- * Public Types
- ****************************************************************************/
+/* Array of pointers to currently running high-resolution timers
+ * for each CPU in SMP configurations. Index corresponds to CPU ID.
+ */
 
-/* Red-black tree head for managing active hrtimers */
-
-RB_HEAD(hrtimer_tree_s, hrtimer_node_s);
+extern FAR hrtimer_t *g_hrtimer_running[CONFIG_SMP_NCPUS];
 
 /****************************************************************************
  * Public Function Prototypes
@@ -147,14 +159,13 @@ int hrtimer_starttimer(uint64_t ns)
  *   b - Pointer to the second hrtimer node.
  *
  * Returned Value:
- *   >0 if b expires before a
- *    0 if a and b expire at the same time
- *   <0 if b expires after a
+ *   <0 if a expires before b
+ *   >=0 if a expires after b
  ****************************************************************************/
 
 static inline_function
 int hrtimer_compare(FAR const hrtimer_node_t *a,
-                FAR const hrtimer_node_t *b)
+                    FAR const hrtimer_node_t *b)
 {
   FAR const hrtimer_t *atimer = (FAR const hrtimer_t *)a;
   FAR const hrtimer_t *btimer = (FAR const hrtimer_t *)b;
@@ -173,4 +184,90 @@ int hrtimer_compare(FAR const hrtimer_node_t *a,
 
 RB_PROTOTYPE(hrtimer_tree_s, hrtimer_node_s, entry, hrtimer_compare);
 
+/****************************************************************************
+ * Name: hrtimer_is_armed
+ *
+ * Description:
+ *   Test whether a timer is currently armed (inserted into the RB-tree).
+ *
+ * Returned Value:
+ *   true if armed, false otherwise.
+ ****************************************************************************/
+
+static inline_function bool hrtimer_is_armed(FAR hrtimer_t *hrtimer)
+{
+  /* RB-tree root has NULL parent, so root must be checked explicitly */
+
+  return RB_PARENT(&hrtimer->node, entry) != NULL ||
+         RB_ROOT(&g_hrtimer_tree) == &hrtimer->node;
+}
+
+/****************************************************************************
+ * Name: hrtimer_remove
+ *
+ * Description:
+ *   Remove a timer from the RB-tree and mark it as unarmed.
+ ****************************************************************************/
+
+static inline_function void hrtimer_remove(FAR hrtimer_t *hrtimer)
+{
+  RB_REMOVE(hrtimer_tree_s, &g_hrtimer_tree, &hrtimer->node);
+
+  /* Explicitly clear parent to mark the timer as unarmed */
+
+  RB_PARENT(&hrtimer->node, entry) = NULL;
+}
+
+/****************************************************************************
+ * Name: hrtimer_insert
+ *
+ * Description:
+ *   Insert a timer into the RB-tree according to its expiration time.
+ ****************************************************************************/
+
+static inline_function void hrtimer_insert(FAR hrtimer_t *hrtimer)
+{
+  RB_INSERT(hrtimer_tree_s, &g_hrtimer_tree, &hrtimer->node);
+}
+
+/****************************************************************************
+ * Name: hrtimer_get_first
+ *
+ * Description:
+ *   Return the earliest expiring armed timer.
+ *
+ * Returned Value:
+ *   Pointer to the earliest timer, or NULL if none are armed.
+ ****************************************************************************/
+
+static inline_function FAR hrtimer_t *hrtimer_get_first(void)
+{
+  return (FAR hrtimer_t *)RB_MIN(hrtimer_tree_s, &g_hrtimer_tree);
+}
+
+/****************************************************************************
+ * Name: hrtimer_is_first
+ *
+ * Description:
+ *   Test whether the given high-resolution timer is the earliest
+ *   expiring timer in the RB-tree.
+ *
+ *   In a red-black tree ordered by expiration time, the earliest timer
+ *   is represented by the left-most node. Therefore, a timer is the
+ *   earliest one if it has no left child.
+ *
+ * Input Parameters:
+ *   hrtimer - Pointer to the high-resolution timer to be tested.
+ *
+ * Returned Value:
+ *   true  - The timer is the earliest expiring armed timer.
+ *   false - The timer is not the earliest timer.
+ ****************************************************************************/
+
+static inline_function bool hrtimer_is_first(FAR hrtimer_t *hrtimer)
+{
+  return RB_LEFT(&hrtimer->node, entry) == NULL;
+}
+
+#endif /* CONFIG_HRTIMER */
 #endif /* __SCHED_HRTIMER_HRTIMER_H */

sched/hrtimer/hrtimer_cancel.c

@@ -27,9 +27,9 @@
 #include <nuttx/config.h>
 #include <nuttx/arch.h>
 #include <nuttx/clock.h>
-
 #include <errno.h>
 
+#include "sched/sched.h"
 #include "hrtimer/hrtimer.h"
 
 /****************************************************************************
@@ -40,6 +40,40 @@
 
 #define HRTIMER_CANCEL_SYNC_DELAY_MS  5
 
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: hrtimer_is_active
+ *
+ * Description:
+ *   Check whether a high-resolution timer is currently running on any CPU.
+ *
+ * Input Parameters:
+ *   hrtimer - Pointer to the high-resolution timer to check.
+ *
+ * Returned Value:
+ *   true  - The timer is active on at least one CPU.
+ *   false - The timer is not active.
+ ****************************************************************************/
+
+static inline_function bool hrtimer_is_active(FAR hrtimer_t *hrtimer)
+{
+  bool is_active = false;
+
+  for (int i = 0; i < CONFIG_SMP_NCPUS; i++)
+    {
+      if (g_hrtimer_running[i] == hrtimer)
+        {
+          is_active = true;
+          break;
+        }
+    }
+
+  return is_active;
+}
+
 /****************************************************************************
  * Public Functions
  ****************************************************************************/
@@ -83,8 +117,8 @@
 int hrtimer_cancel(FAR hrtimer_t *hrtimer)
 {
   FAR hrtimer_t *first;
-  irqstate_t     flags;
-  int            ret = OK;
+  irqstate_t flags;
+  int ret = OK;
 
   DEBUGASSERT(hrtimer != NULL);
 
@@ -92,50 +126,20 @@ int hrtimer_cancel(FAR hrtimer_t *hrtimer)
 
   flags = spin_lock_irqsave(&g_hrtimer_spinlock);
 
-  /* Capture the current earliest timer before any modification */
-
-  first = (FAR hrtimer_t *)RB_MIN(hrtimer_tree_s, &g_hrtimer_tree);
-
-  switch (hrtimer->state)
+  if (hrtimer_is_armed(hrtimer))
     {
-      case HRTIMER_STATE_ARMED:
-        {
-          /* Remove the timer from the active tree */
-
-          RB_REMOVE(hrtimer_tree_s, &g_hrtimer_tree, &hrtimer->node);
-          hrtimer->state = HRTIMER_STATE_INACTIVE;
-          break;
-        }
-
-      case HRTIMER_STATE_RUNNING:
-        {
-          /* The callback is currently executing.
-           *
-           * Mark the timer as canceled so it will not be re-armed or
-           * executed again.  The running callback is allowed to complete.
-           *
-           * NOTE: The timer node is expected to have already been removed
-           *       from the tree when the callback started executing.
-           */
-
-          hrtimer->state = HRTIMER_STATE_CANCELED;
-          break;
-        }
-
-      case HRTIMER_STATE_INACTIVE:
-      case HRTIMER_STATE_CANCELED:
-      default:
-        {
-          ret = -EINVAL;
-          break;
-        }
+      hrtimer_remove(hrtimer);
     }
 
+  /* Mark timer as cancelled */
+
+  hrtimer->expired = UINT64_MAX;
+
   /* If the canceled timer was the earliest one, update the hardware timer */
 
-  if ((ret == OK) && (first == hrtimer))
+  if (hrtimer_is_first(hrtimer))
     {
-      first = (FAR hrtimer_t *)RB_MIN(hrtimer_tree_s, &g_hrtimer_tree);
+      first = hrtimer_get_first();
       if (first != NULL)
         {
           ret = hrtimer_starttimer(first->expired);
@@ -196,7 +200,7 @@ int hrtimer_cancel_sync(FAR hrtimer_t *hrtimer)
    * and the state becomes inactive.
    */
 
-  while (hrtimer->state != HRTIMER_STATE_INACTIVE)
+  while (hrtimer_is_active(hrtimer))
     {
       if (cansleep)
         {

sched/hrtimer/hrtimer_initialize.c

@@ -32,6 +32,12 @@
  * Public Data
  ****************************************************************************/
 
+/* Array of pointers to currently running high-resolution timers
+ * for each CPU in SMP configurations. Index corresponds to CPU ID.
+ */
+
+FAR hrtimer_t *g_hrtimer_running[CONFIG_SMP_NCPUS];
+
 /* Global spinlock protecting the high-resolution timer subsystem.
  *
  * This spinlock serializes access to the hrtimer red-black tree and
@@ -49,8 +55,7 @@ spinlock_t g_hrtimer_spinlock = SP_UNLOCKED;
  * The tree is ordered by absolute expiration time.
  */
 
-struct hrtimer_tree_s g_hrtimer_tree =
-  RB_INITIALIZER(g_hrtimer_tree);
+struct hrtimer_tree_s g_hrtimer_tree = RB_INITIALIZER(g_hrtimer_tree);
 
 /****************************************************************************
  * Public Functions

sched/hrtimer/hrtimer_process.c

@@ -28,6 +28,7 @@
 #include <assert.h>
 #include <nuttx/arch.h>
 #include <nuttx/clock.h>
+
 #include "hrtimer/hrtimer.h"
 
 /****************************************************************************
@@ -54,7 +55,7 @@
  *       if no timers remain.
  *
  * Input Parameters:
- *   ts - Pointer to the current high-resolution timestamp.
+ *   now - Current high-resolution timestamp.
  *
  * Returned Value:
  *   None.
@@ -69,9 +70,11 @@
 void hrtimer_process(uint64_t now)
 {
   FAR hrtimer_t *hrtimer;
-  uint64_t expired;
-  uint32_t period = 0;
   irqstate_t flags;
+  hrtimer_cb func;
+  uint64_t expired;
+  uint64_t period;
+  int cpu = this_cpu();
 
   /* Lock the hrtimer RB-tree to protect access */
 
@@ -79,53 +82,64 @@ void hrtimer_process(uint64_t now)
 
   /* Fetch the earliest active timer */
 
-  hrtimer = (FAR hrtimer_t *)RB_MIN(hrtimer_tree_s, &g_hrtimer_tree);
+  hrtimer = hrtimer_get_first();
 
   while (hrtimer != NULL)
     {
+      func = hrtimer->func;
+
+      /* Ensure the timer callback is valid */
+
+      DEBUGASSERT(func != NULL);
+
+      expired = hrtimer->expired;
+
       /* Check if the timer has expired */
 
-      if (!clock_compare(hrtimer->expired, now))
+      if (!clock_compare(expired, now))
         {
           break;
         }
 
       /* Remove the expired timer from the active tree */
 
-      RB_REMOVE(hrtimer_tree_s, &g_hrtimer_tree, &hrtimer->node);
+      hrtimer_remove(hrtimer);
 
-      /* Ensure the timer callback is valid */
+      g_hrtimer_running[cpu] = hrtimer;
 
-      DEBUGASSERT(hrtimer->func != NULL);
-
-      hrtimer->state = HRTIMER_STATE_RUNNING;
+      /* Leave critical section before invoking the callback */
 
       spin_unlock_irqrestore(&g_hrtimer_spinlock, flags);
 
       /* Invoke the timer callback */
 
-      period = hrtimer->func(hrtimer);
+      period = func(hrtimer, expired);
+
+      /* Re-enter critical section to update timer state */
 
       flags = spin_lock_irqsave(&g_hrtimer_spinlock);
 
-      if ((hrtimer->state == HRTIMER_STATE_CANCELED) || (period == 0))
-        {
-          /* Timer is canceled or one-shot; mark it inactive */
+      g_hrtimer_running[cpu] = NULL;
 
-          hrtimer->state = HRTIMER_STATE_INACTIVE;
-        }
-      else
-        {
-          /* Restart the periodic timer */
+      /* If the timer is periodic and has not been rearmed or
+       * cancelled concurrently,
+       * compute next expiration and reinsert into RB-tree
+       */
 
+      if (period > 0 && hrtimer->expired == expired)
+        {
           hrtimer->expired += period;
-          hrtimer->state = HRTIMER_STATE_ARMED;
-          RB_INSERT(hrtimer_tree_s, &g_hrtimer_tree, &hrtimer->node);
+
+          /* Ensure no overflow occurs */
+
+          DEBUGASSERT(hrtimer->expired > period);
+
+          hrtimer_insert(hrtimer);
         }
 
       /* Fetch the next earliest timer */
 
-      hrtimer = (FAR hrtimer_t *)RB_MIN(hrtimer_tree_s, &g_hrtimer_tree);
+      hrtimer = hrtimer_get_first();
     }
 
   /* Schedule the next timer expiration */

sched/hrtimer/hrtimer_start.c

@@ -31,53 +31,6 @@
 
 #include "hrtimer/hrtimer.h"
 
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
-
-/****************************************************************************
- * Name: hrtimer_insert
- *
- * Description:
- *   Insert the given high-resolution timer into the active timer RB-tree.
- *   If the timer is already in the tree, it will be replaced.
- *   If the inserted timer becomes the earliest timer in the tree, the
- *   hardware timer will be configured to fire at its expiration.
- *
- * Input Parameters:
- *   hrtimer - Pointer to the hrtimer structure to be inserted.
- *
- * Returned Value:
- *   OK (0) on success; negated errno value on failure.
- *
- * Assumptions/Notes:
- *   - This function should be called with interrupts disabled or under
- *     spinlock protection to ensure RB-tree integrity.
- *   - If the timer is currently running, insertion is rejected with -EBUSY.
- ****************************************************************************/
-
-static inline int hrtimer_insert(FAR hrtimer_t *hrtimer)
-{
-  DEBUGASSERT(hrtimer != NULL);
-
-  /* Insert (or replace) the timer into the RB-tree ordered by expiration */
-
-  RB_INSERT(hrtimer_tree_s, &g_hrtimer_tree, &hrtimer->node);
-
-  /* Mark the timer as armed */
-
-  hrtimer->state = HRTIMER_STATE_ARMED;
-
-  /* If the inserted timer is now the earliest, start hardware timer */
-
-  if (&hrtimer->node == RB_MIN(hrtimer_tree_s, &g_hrtimer_tree))
-    {
-      return hrtimer_starttimer(hrtimer->expired);
-    }
-
-  return OK;
-}
-
 /****************************************************************************
  * Public Functions
  ****************************************************************************/
@@ -91,7 +44,7 @@ static inline int hrtimer_insert(FAR hrtimer_t *hrtimer)
  *
  * Input Parameters:
  *   hrtimer - Pointer to the hrtimer structure.
- *   ns      - Expiration time in nanoseconds. Interpretation
+ *   expired - Expiration time in nanoseconds. Interpretation
  *             depends on mode.
  *   mode    - Timer mode (HRTIMER_MODE_ABS or HRTIMER_MODE_REL).
  *
@@ -107,7 +60,7 @@ static inline int hrtimer_insert(FAR hrtimer_t *hrtimer)
  ****************************************************************************/
 
 int hrtimer_start(FAR hrtimer_t *hrtimer,
-                  uint64_t ns,
+                  uint64_t expired,
                   enum hrtimer_mode_e mode)
 {
   irqstate_t flags;
@@ -119,30 +72,40 @@ int hrtimer_start(FAR hrtimer_t *hrtimer,
 
   flags = spin_lock_irqsave(&g_hrtimer_spinlock);
 
-  /* Reject start if the timer is already running or armed */
-
-  if ((hrtimer->state == HRTIMER_STATE_RUNNING) ||
-      (hrtimer->state == HRTIMER_STATE_ARMED))
+  if (hrtimer_is_armed(hrtimer))
     {
-      spin_unlock_irqrestore(&g_hrtimer_spinlock, flags);
-      return -EBUSY;
+      hrtimer_remove(hrtimer);
     }
 
   /* Compute absolute expiration time */
 
   if (mode == HRTIMER_MODE_ABS)
     {
-      hrtimer->expired = ns;
+      hrtimer->expired = expired;
     }
   else
     {
-      hrtimer->expired = hrtimer_gettime() + ns;
+      hrtimer->expired = hrtimer_gettime() + expired;
     }
 
+  /* Ensure expiration time does not overflow */
+
+  DEBUGASSERT(hrtimer->expired >= expired);
+
   /* Insert the timer into the RB-tree */
 
-  ret = hrtimer_insert(hrtimer);
+  hrtimer_insert(hrtimer);
+
+  /* If the inserted timer is now the earliest, start hardware timer */
+
+  if (hrtimer_is_first(hrtimer))
+    {
+      ret = hrtimer_starttimer(hrtimer->expired);
+    }
+
+  /* Release spinlock and restore interrupts */
 
   spin_unlock_irqrestore(&g_hrtimer_spinlock, flags);
+
   return ret;
 }