sched: Refactor the functional incorrect HRTimer implementation.

[Fix-Point]

Summary

This PR is the part IV of #17556. The main changes in this PR are:

• Removed the functional incorrect hrtimer implementation.
• Introduced a reusable hrtimer_queue component, allowing users to freely compose it with any hardware timer to implement their own hrtimer instance.
• Introduced OS hrtimer submodule for OS scheduling and timing.

Background

High-resolution Timer (HRTimer) is a timer abstraction capable of achieving nanosecond-level timing precision, primarily used in scenarios requiring high-precision clock events. With the advancement of integrated circuit technology, modern high-precision timer hardware (such as the typical x86 HPET) can already meet sub-nanosecond timing requirements and offer femtosecond-level jitter control.

Although the current hardware timer abstraction (up_alarm/up_timer) in the NuttX kernel already supports nanosecond-level timing, its software timer abstraction, wdog, and the timer expiration interrupt handling process remain at microsecond-level (tick) precision, which falls short of high-precision timing demands. Therefore, it is necessary to implement a new timer abstraction—HRTimer, to address the precision limitations of wdog. HRTimer primarily provides the following functional interfaces:

• Set a timer in nanoseconds: Configure a software timer to trigger at a specified nanosecond time.
• Cancel a timer: Cancel the software timer.
• Handle timer expiration: Execute expiration processing after the timer event is triggered.

Design

The new NuttX HRTimer is designed to address the issues of insufficient precision in the current NuttX wdog. It draws on the strengths of the Linux HRTimer design while improving upon its weaknesses. As Figure 1 shows, the HRTimer design is divided into two parts: the HRTimer Queue and the HRTimer. The HRTimer Queue is a reusable component that allows users to freely customize their own HRTimer interface by pairing it with a private timer driver, without needing to modify the kernel code.

graph TD
    subgraph "Public Headers"
        A[hrtimer_queue_type.h<br/> Public Type Definition]
    end

    subgraph "Internal Implementation Headers"
        B1[hrtimer_type_list.h<br/>List Implementation]
        B2[hrtimer_type_rb.h<br/>RB-Tree Implementation]
        B3[...Others]
        
        C[hrtimer_queue.h<br/>Reusable Component<br/>HRTimer Queue Internal]
    end

    subgraph "Instances"
        D[hrtimer.h<br/>OS HRTimer API ]
        E[hrtimer.c<br/>OS HRTimer Implementation]

        F[myhrtimer.h<br/>Customed HRTimer API ]
        G[myhrtimer.c<br/>Customed HRTimer Implementation]
    end

    %% Dependent
    A --> B1
    A --> B2
    A --> B3
    
    B1 --> C
    B2 --> C
    B3 --> C

    A --> D
    C --> E
    D --> E

    A --> F
    C --> G
    F --> G

    %% Style
    style A fill:#e1f5fe
    style B1 fill:#f3e5f5
    style B2 fill:#f3e5f5
    style B3 fill:#f3e5f5
    style C fill:#fff3e0
    style D fill:#e8f5e8
    style E fill:#e8f5e8
    style F fill:#e8f5e8
    style G fill:#e8f5e8

Loading

Figure 1. The architecture of the HRTimer implementation

API Design

The HRTimer Queue is a zero-performance-overhead, composable, and customizable abstraction that provides only asynchronous-style interfaces:

• hrtimer_queue_start(queue, timer): Asynchronously sends an HRTimer to HRTimer queue.
• hrtimer_queue_async_cancel(queue, timer): Asynchronously cancels an HRTimer and returns the current reference count of the timer.
• hrtimer_queue_wait(queue, timer): Waits for the release of all references to the HRTimer to obtain ownership of the HRTimer data structure.

All other user interfaces can be composed based on these three interfaces.

On top of the HRTimer Queue, users only need to implement the following interfaces to customize their own HRTimer implementation:

• hrtimer_expiry(current): Handles timer expiration, typically called within the execution path of the corresponding timer hardware interrupt handler.
• hrtimer_reprogram(queue, next_expired): Sets the next timer event.
• hrtimer_wait_policy(): Waits if the timer callback is still executing.
• hrtimer_current(): Gets the current time to set relative timers.

After implementing the above three interfaces, users can include one of the hrtimer_type_xxx.h implementation to compose their own hrtimer implementation, which mainly includes the following interfaces:

• hrtimer_restart(timer, func, arg, time): Restarts a timer that has been asynchronously canceled (its callback function might still be executing). This interface is designed to explicitly remind users to be aware of concurrency issues, as concurrency problems are prone to occur in actual programming and are very difficult to locate. Providing such an interface facilitates quick identification of concurrency issues.
• hrtimer_start(timer, func, arg, time): Starts a stopped timer. The mode parameter indicates whether it is a relative or absolute timer.
• hrtimer_cancel(timer): Asynchronously cancels a timer. Note that the semantics of this interface are completely different from Linux's try_to_cancel. It ensures that the timer can definitely be canceled successfully, but may need to wait for its callback function to finish execution.
• hrtimer_cancel_sync(timer): Synchronously cancels a timer. If the timer's callback function is still executing, this function will spin-wait until the callback completes. It ensures that the user can always obtain ownership of the timer.

The design characteristics of HRTimer are as follows:

• Strict and Simplified HRTimer State Machine: In the old wdog design, wdog could be reset in any state, which introduced unnecessary complexity to certain function implementations. For example, wd_start had to account for the possibility of restarting. In the new HRTimer design, an HRTimer that has already been started and not canceled cannot be started again.

• Abstracted Sorting Queue: Since no single design can be optimal for all application scenarios, HRTimer abstracts interfaces for inserting and deleting nodes in the sorting queue. This allows for different data structure implementations to be configured for different application scenarios, as shown in Table 1.

  Table 1: Comparison of Several Sorting Queue Implementations

  Sorting Queue Implementation	Insert	Delete	Delete Head	Determinism	Suitable Scenarios
  Doubly Linked List	O(n)	O(1)	O(1)	Moderate	Embedded / Soft Real-time Systems
  Red-Black Tree	O(logn)	O(logn)	O(logn)	Slightly Poor	General Purpose

• Callback Execution Without Lock Held: HRTimer implements callback execution without lock held, ensuring that the system's blocking time is not limited by the user's callback function. However, this introduces additional states and waits, where waiting for reference release is primarily implemented using hazard pointers. This will be explained in detail in the subsequent state transition diagram.

• Clear HRTimer Object Ownership Transfer Path: In the wdog implementation, the wdog callback function could restart the current timer directly without regard to ownership, potentially causing concurrency issues. In the new implementation, the HRTimer callback function cannot restart itself. Instead, inspired by Linux's design, the callback function returns whether a restart is needed. If a restart is required, the thread executing the callback function re-enqueues it; otherwise, the thread releases ownership. This change ensures a clear ownership transfer path for the HRTimer object.

• Non-blocking Timer Restart: To address the issue in Linux where restarting a timer must wait for an already-started callback function to finish, which reduces the real-time performance, the new HRTimer implements a non-blocking timer restart mechanism. This mechanism reuses the last bit of the hazard pointer to mark whether the thread executing the callback has lost write ownership of the HRTimer object. After hrtimer_cancel is called, other threads executing callbacks will lose write ownership of the HRTimer (though their callback functions may still be executing). This means the HRTimer can be restarted and repurposed for other callbacks without waiting for the callback function to complete. However, note that the callback function might still be executing, requiring users to consider this concurrency and implement proper synchronization mechanisms within their callback functions. To explicitly remind users of this concurrency, an HRTimer whose callback function has not yet completed execution must be restarted using hrtimer_restart. This function relaxes the state checks on the HRTimer, allowing a timer with the callback running to be started.

• Deterministic Timer Cancellation: To address the starvation issue present in Linux's timer cancellation, the new HRTimer implementation sets a cancellation state via hrtimer_cancel. This cancellation state has a unique and deterministic state transition, eliminating starvation. Memory reclamation is performed through hazard pointer checking loops. Hazard pointer checking ensures that all threads finish executing the callback function and release read ownership (reference release) of the specified HRTimer object. The determinism of the timer cancellation discussion is presented in Table 2.

  Table 2: Timer Cancellation Function Semantic Comparison

Mode	Function	Determinism	Worst-Case Execution Time	Explanation
SMP Mode	hrtimer_cancel	Deterministic	O(1) (doubly-linked list removal) or O(log n) (red-black tree removal)	Asynchronously revokes the write ownership of the timer from the queue, but the callback function might be executing; waiting is not required.
SMP Mode	hrtimer_cancel_sync	Less-deterministic but bounded	O(cN), c: WCET of the timer callback function, N: number of cores.	Asynchronously revokes the timer ownership and waits for the timer callback to finish. This waiting time depends on the execution time of the callback function, is unavoidable, and thus has poorer determinism. However, this cancel function should only being called when the timer will not be used anymore.
non-SMP Mode	hrtimer_cancel	Deterministic	O(1) (doubly-linked list removal) or O(log n) (red-black tree removal)	In non-SMP mode, the execution of the hrtimer_expiry interrupt process cannot be interrupted. Therefore, the semantics of this implementation are essentially equivalent to hrtimer_cancel_sync.
non-SMP Mode	hrtimer_cancel_sync	Deterministic	O(1) (doubly-linked list removal) or O(log n) (red-black tree removal)	Similarly, in non-SMP mode, there is no scenario where the callback function is still executing during cancellation. Therefore, there is no need to wait for the callback function to complete, making it also deterministic.

%%{
  init: {
    'theme': 'base',
    'themeVariables': {
      'primaryColor': '#FFFFFF',
      'primaryTextColor' : '#000000',
      'mermiad-container': "#FFFFFF",
      'primaryBorderColor': '#000000',
      'lineColor': '#000000',
      'secondaryColor': '#FFFFFF',
      'tertiaryColor': '#000000'
    },
    'sequence': { 'mirrorActors': false }
  }
}%%

stateDiagram-v2
    HRTIMER_COMPLETED|private --> HRTIMER_PENDING|shared : [hrtimer_start]
    HRTIMER_PENDING|shared --> HRTIMER_RUNNING|shared: [hrtimer_expiry]
    HRTIMER_RUNNING|shared --> HRTIMER_CANCELED|half_shared : hrtimer callback return zero or htrimer_cancel
    HRTIMER_RUNNING|shared --> HRTIMER_PENDING|shared : hrtimer callback return non-zero in hrtimer_expiry
    HRTIMER_PENDING|shared --> HRTIMER_CANCELED|half_shared : [hrtimer_cancel]
    HRTIMER_CANCELED|half_shared --> HRTIMER_CANCELED|half_shared : [hrtimer_cancel]
    HRTIMER_CANCELED|half_shared --> HRTIMER_PENDING|shared : [hrtimer_restart]
    HRTIMER_CANCELED|half_shared --> HRTIMER_COMPLETED|private : [hrtimer_cancel_sync] waits all cores release the references to the timer.

Loading

Figure 2. HRTimer State Transition Diagram

The valid state transitions of an HRTimer object are shown in Figure 2. States are represented using a simplified notation of State|Ownership, such as HRTIMER_PENDING|shared. The meanings of the simplified ownership markers are as follows:

Ownership Markers

• |private indicates that the resource is exclusively owned by a specific thread t. Only the owning thread t can read from or write to this resource.
• |shared indicates that the resource is globally shared and can be read by any thread. However, only the thread t that holds the global lock l (t = Owned(l)) can obtain write ownership of this resource.
• |half_shared indicates that the resource may be accessed by multiple threads, but only the thread that called hrtimer_cancel holds write ownership of this resource. Modifications to it by threads executing callback functions are prevented.

The specific definitions of the states are as follows:

• HRTIMER_PENDING|shared: hrtimer->func != NULL. That is, the hrtimer has been inserted into the hrtimer_queue and is waiting to be executed.
• HRTIMER_COMPLETED|private: hrtimer->func == NULL ∧ ∀c ∈ [0, CONFIG_SMP_NCPUS), (hrtimer_queue->running[c] & ~(1u)) != hrtimer. That is, the hrtimer is not in a pending state, and no core is currently executing the hrtimer's callback function.
• HRTIMER_RUNNING|shared: hrtimer->func == NULL ∧ ∃c ∈ [0, CONFIG_SMP_NCPUS), hrtimer_queue->running[c] == hrtimer. That is, the hrtimer is not in a pending state, and there exists at least one core that is currently executing the hrtimer’s callback function.
• HRTIMER_CANCELED|half_shared: hrtimer->func == NULL ∧ ∀c ∈ [0, CONFIG_SMP_NCPUS), hrtimer_queue->running[c] != hrtimer. That is, the hrtimer is not in a pending state, and all cores have lost ownership of the hrtimer—meaning they can no longer read from or write to the hrtimer—though its callback function may still be in the process of being executed.

All state transitions not described in the diagram must return failure. For example, a timer in the HRTIMER_PENDING state cannot be started (start) again. Note that there is one exception: a thread that is already in the HRTIMER_CANCELED state can legally call hrtimer_cancel again, and the state remains unchanged. Besides, notice that the state transition HRTIMER_COMPLETED|private --[hrtimer_cancel/hrtimer_cancel_sync]--> HRTIMER_COMPLETED|private is also valid but not presented in the state diagram for better readability.

To avoid the overhead caused by threads waiting for callback functions to finish executing, HRTimer adds a restart interface. Under normal circumstances, the start interface cannot start a timer that is already in the canceled state. Only when the user uses this restart interface can a timer whose callback function has not yet completed be started normally. Using this interface serves to explicitly remind users to pay attention to concurrency within their callback functions. Furthermore, when concurrency issues arise with HRTimer, it helps in pinpointing the source of the problem—issues can only originate from callback functions where restart was used to restart the timer.

Performance Evaluation

We conducted 1 million interface calls on the intel64:nsh (Intel Core i7 12700) platform and measured their average execution CPU cycles, as shown in the Figure 3 below. It can be observed that the overhead for starting and asynchronously canceling timers is significantly reduced compared to wdog.

• The speedup of hrtimer_start compared to wd_start is 2.10x.
• The speedup of hrtimer_start & cancel compared to wd_start & cancel is 1.57x.
  Additionally, after enabling hrtimer, wdog processing is treated as an hrtimer timer, which lowers the overhead of the wdog interface.
• wd_start achieved a speedup of 1.73x when hrtimer is enabled.

Figure 3. HRtimer API Latency Test

Plan

The merge plan for this PR is as follows:

1. Introduce seqcount [Done]
2. Simplify the timer expiration processing flow in preparation for introducing HRtimer. [Done]
3. Modify the semantics of scheduling functions to allow immediate timer event triggering. [Done]
4. Fix buggy wdog module. [Done]
5. Introduce hrtimer_queue.
6. Introduce hrtimer.
7. [WIP] Introdude hrtimer_test.
8. [WIP] Add HRTimer documents.
9. [WIP] Enhance the oneshot arch_alarm to support non-tick arguments mode.

Impact

HRTimer currently is disabled by default, so it has no effect on system.

Testing

Tested on intel64:nsh, rv-virt:smp, qemu-armv8a:smp, ostest passed. The hrtimer SMP stress test ran for over 72 hours without errors. The parallel stress test cases is showed in Appendix.

Explanation

Here we need to provide some explanations to avoid misunderstandings with others' work:

Is this hrtimer an improvement based on @wangchdo work #17517?

• This work is completely independent of @wangchdo contribution. The concept and design of the hrtimer began early when I implemented the ClockDevice abstraction (driver/timers: ClockDevice, a new timer driver abstraction for NuttX. #17276). The core concurrent state machine was completed as early as August this year. My plan was to support HRTimer after finishing the ClockDevice. During this period, I had no communication with @wangchdo .

Why are we removing the existing hrtimer implementation and replacing it with this one? Is this disrespectful to @wangchdo 's work?

• I do not disrespect @wangchdo work (sched/sched: Part 1: add high resolution timer (hrtimer) only (without os tick support with hrtimer) for NuttX #17517); on the contrary, I appreciate some of his implementations. However, @wangchdo 's HRTimer implementation has fundamental functional correctness issues. It violates the ownership invariant of hrtimer: that only one reader/writer can use the hrtimer object at a time. This violation makes it prone to issues in SMP scenarios, as I have consistently pointed out over and over again in PR (sched/hrtimr: Update hrtimer state-machine to allow RUNNING/ARMED hrtimer to be restarted #17570):

Simple modifications cannot fix @wangchdo implementation. Therefore, I believe the most effective approach is to remove the existing hrtimer implementation and replace it with this one.

For example, two key implementations severely violate the ownership invariant:

1. hrtimer expiration callback API design: The hrtimer pointer is explicitly exposed to users by being passed to the callback function. If the hrtimer is restarted while the callback is executing, the hrtimer has already been modified, posing risks and potential errors for the user-obtained parameters. For instance, in the following example, if the second test_callback is triggered before the first test_callback finishes executing, it may cause B to be updated twice.

uint32_t tmp;
static uint32_t test_callback(FAR struct hrtimer_s *hrtimer)
{
  .... // some heavy jobs
  uint32_t *count = hrtimer->arg; // Incorrect! Already updated by restarting the hrtimer.
  *count++;
  ....
  
}
...
static uint32_t A = 0;
static uint32_t B = 0;

timer.arg = &A;
hrtimer_start(&timer, 1000, HRTIMER_MODE_REL); // delay 1000ns to update A
...
hrtimer_cancel_sync(&timer);
timer.arg = &B; 
hrtimer_start(&timer, 0, HRTIMER_MODE_REL); // delay 0ns to update B;

ASSERT(A <= 1 && B <= 1) // ASSERT!!! B can be updated twice

2. hrtimer expiration handling process also violates the invariant, where multiple owners may operate an expired hrtimer. Even after I pointed out the issue, @wangchdo added a reference count to address memory reclamation during cancellation. However, it still suffers from concurrency serialization issues, which can cause a newly started periodic timer to be overwritten by an old periodic timer, as shown in the following thread interleaving:

Timeline:
CPU1 (Timer Callback)                     CPU2 (Set New Timer)
------|--------------------------------------|-------------------------
      |                                      |
      t1: timer triggers at t1               |
      |--- Callback starts                   |
      |   hrtimer->state <- running          |
      |                                      |
      | [Unlock]                             |--- hrtimer_cancel_sync(hrtimer)
      |                                      |--- hrtimer_start(hrtimer, t2)
      |                                      |    hrtimer->state <- armed
      |   ...                                | [Unlock]
      |                                      | ...
      |   Callback executes...               | [Lock]
      |                                      |--- New timer triggered
      |                                      |    hrtimer->state <- running
      |                                      | [Unlock]
      |                                      |    Calllback executes....
      |                                      |
      |   Returns next = UINT32_MAX          |
      | [Lock]                               |
      | if (hrtimer->state == running)       |
      |   hrtimer->expired                   | 
      |    <- t2 + UINT32_MAX (Incorrect! expected t1 + UINT32_MAX)
      |   hrtimer->state <- armed            | 
      | [Unlock]                             |
      |                                      |  Returns next = 1
      |                                      |  [Lock]
      |                                      |--- hrtimer->state != running
      |                                      |    failed to set next (Incorrect!)
      |                                      |    The previous cancelled timer
      |                                      |    callback restarted the new timer.
------|--------------------------------------|-------------------------

More similar concurrency issues could be cited here. As I have emphasized again and again, the fundamental problem is the violation of the ownership invariant of hrtimer: only one owner can modify the hrtimer object at a time.

Designing functionally correct concurrent algorithms is not easy at all. Relying solely on engineering experience is insufficient; theoretical methods are necessary to avoid errors, such as adapting resource invariants and using structured diagrams to clarify every possible concurrent state transition. @wangchdo's design failed to consider how to handle concurrency correctly, making it nearly impossible to improve upon his code base.

From an efficiency perspective, replacing @wangchdo 's implementation with this PR's implementation—which is functionally correct, offers better code reusability, and is more user-friendly—can save both of us time and allow us to focus on more meaningful optimizations.

Appendix

The hrtimer parallel stress test cases is showed as following, and they will be pushed to nuttx-apps after this PR is merged:

/****************************************************************************
 * apps/testing/ostest/hrtimer.c
 *
 * Licensed to the Apache Software Foundation (ASF) under one or more
 * contributor license agreements.  See the NOTICE file distributed with
 * this work for additional information regarding copyright ownership.  The
 * ASF licenses this file to you under the Apache License, Version 2.0 (the
 * "License"); you may not use this file except in compliance with the
 * License.  You may obtain a copy of the License at
 *
 *   http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
 * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
 * License for the specific language governing permissions and limitations
 * under the License.
 *
 ****************************************************************************/

/****************************************************************************
 * Included Files
 ****************************************************************************/

#include <nuttx/config.h>
#include <nuttx/arch.h>
#include <nuttx/sched.h>
#include <nuttx/spinlock.h>

#include <assert.h>
#include <pthread.h>
#include <stdio.h>
#include <syslog.h>
#include <unistd.h>

#include <nuttx/hrtimer.h>

/****************************************************************************
 * Pre-processor Definitions
 ****************************************************************************/

#define HRTIMER_TEST_RAND_ITER     (1024 * 2)
#define HRTIMER_TEST_THREAD_NR     (CONFIG_SMP_NCPUS * 8)
#define HRTIMER_TEST_TOLERENT_NS   (10 * NSEC_PER_TICK)
#define HRTIMER_TEST_CRITICAL_SECTION  1024

#define hrtest_printf(s, ...)      printf("[%d] " s, this_cpu(), __VA_ARGS__)

#define hrtest_delay(delay_ns)     usleep(delay_ns / 1000 + 1)

#define hrtimer_start(timer, cb, delay_ns) hrtimer_start(timer, cb, delay_ns, HRTIMER_MODE_REL)
#define hrtimer_restart(timer, cb, delay_ns) hrtimer_restart(timer, cb, delay_ns, HRTIMER_MODE_REL)

/****************************************************************************
 * Private Type
 ****************************************************************************/

typedef struct hrtimer_tparam_s
{
  FAR hrtimer_t     timer;
  FAR spinlock_t   *lock;
  uint64_t          interval;
  volatile uint64_t callback_cnt;
  volatile uint64_t triggered_ns;
  volatile uint8_t  current_cpu;
  volatile uint8_t  state;
} hrtimer_tparam_t;

/****************************************************************************
 * Private Functions
 ****************************************************************************/

static uint64_t hrtimer_test_callback(FAR const struct hrtimer_s *timer, uint64_t expired_ns)
{
  FAR hrtimer_tparam_t *hrtimer_tparam = (FAR hrtimer_tparam_t *)timer;

  /* Record the system tick at which the callback was triggered */

  // clock_systime_nsec(&hrtimer_tparam->triggered_ns);
  hrtimer_tparam->triggered_ns = clock_systime_nsec();

  /* Increment the callback count */

  hrtimer_tparam->callback_cnt   += 1;

  return 0;
}

static void hrtimer_test_checkdelay(int64_t diff, uint64_t delay_ns)
{
  /* Ensure the watchdog trigger time is not earlier than expected. */

  ASSERT(diff - delay_ns >= 0);

  /* If the timer latency exceeds the tolerance, print a warning. */

  if (diff - delay_ns > HRTIMER_TEST_TOLERENT_NS)
    {
      hrtest_printf("WARNING: hrtimer latency ns %" PRId64
                    "(> %u may indicate timing error)\n",
                    diff - delay_ns,
                    (unsigned)HRTIMER_TEST_TOLERENT_NS);
    }
}

static void hrtimer_test_once(FAR hrtimer_t *timer,
                              FAR hrtimer_tparam_t *param,
                              uint64_t delay_ns)
{
  uint64_t   cnt;
  int64_t    diff;
  uint64_t   timerset_ns;
  irqstate_t flags;

  hrtest_printf("hrtimer_test_once %" PRIu64 " ns\n", delay_ns);

  /* Save the current callback count. */

  cnt = param->callback_cnt;

  /* Enter a critical section to prevent interruptions. */

  flags = up_irq_save();
  sched_lock();

  /* Record the current system tick before setting the watchdog. */

  // clock_systime_nsec(&timerset_ns);
  timerset_ns = clock_systime_nsec();

  ASSERT(hrtimer_start(timer, hrtimer_test_callback, delay_ns) == OK);

  up_irq_restore(flags);
  sched_unlock();

  /* Wait until the callback is triggered exactly once. */

  while (cnt + 1 != param->callback_cnt)
    {
      hrtest_delay(delay_ns);
    }

  /* Check if the delay is within the acceptable tolerance. */

  diff = param->triggered_ns - timerset_ns;

  hrtimer_test_checkdelay(diff, delay_ns);

  hrtimer_cancel_sync(timer);
}

static void hrtimer_test_rand(FAR hrtimer_t *timer,
                              FAR hrtimer_tparam_t *param, uint64_t rand_ns)
{
  uint64_t   cnt;
  unsigned   idx;
  uint64_t   delay_ns;
  uint64_t   timer_setns;
  int64_t    diff;
  irqstate_t flags = 0;

  hrtest_printf("hrtimer_test_rand %" PRIu64 " ns\n", rand_ns);

  /* Perform multiple iterations with random delays. */

  for (idx = 0; idx < HRTIMER_TEST_RAND_ITER; idx++)
    {
      /* Generate a random delay within the specified range. */

      delay_ns = rand() % rand_ns;

      DEBUGASSERT(timer->func == NULL);

      /* Enter critical section if the callback count is odd. */

      cnt = param->callback_cnt;

      if (cnt % 2u)
        {
          flags = up_irq_save();
        }

      timer_setns = clock_systime_nsec();
      ASSERT(hrtimer_start(timer, hrtimer_test_callback,
                           delay_ns) == 0);
      if (cnt % 2u)
        {
          up_irq_restore(flags);
        }

      /* Decide to wait for the callback or cancel the watchdog. */

      if (delay_ns % 2u)
        {
          /* Wait for the callback. */

          while (cnt + 1u != param->callback_cnt)
            {
              hrtest_delay(delay_ns);
            }

          /* Check the delay if the callback count is odd. */

          if (cnt % 2u)
            {
              diff = (sclock_t)(param->triggered_ns - timer_setns);
              hrtimer_test_checkdelay(diff, delay_ns);
            }
        }

      hrtimer_cancel_sync(timer);
      DEBUGASSERT(timer->func == NULL);
    }

  hrtimer_cancel_sync(timer);
}

static uint64_t hrtimer_test_rand_cancel_callback(FAR const struct hrtimer_s *timer, uint64_t expired_ns)
{
  FAR hrtimer_tparam_t *tparam = (FAR hrtimer_tparam_t *)timer;
  FAR spinlock_t *lock = tparam->lock;
  uint64_t   delay_ns = 0;
  irqstate_t flags = spin_lock_irqsave(lock);

  /* Random sleep */

  delay_ns = tparam->triggered_ns % tparam->interval;

  /* Check if the version is same. */

  if (expired_ns == tparam->timer.expired)
    {
      tparam->triggered_ns = clock_systime_nsec();

      /* Increment the callback count */

      tparam->callback_cnt++;
    }

  spin_unlock_irqrestore(lock, flags);

  up_ndelay(delay_ns);

  return 0;
}

static void hrtimer_test_rand_cancel(FAR hrtimer_t *timer,
                                     FAR hrtimer_tparam_t *param,
                                     uint64_t rand_ns)
{
  uint64_t   cnt;
  unsigned   idx;
  uint64_t   delay_ns;
  irqstate_t flags;
  spinlock_t rand_cancel_lock = SP_UNLOCKED;

  hrtest_printf("hrtimer_test_rand cancel %" PRIu64 " ns\n", rand_ns);

  param->interval = rand_ns;
  param->lock     = &rand_cancel_lock;

  /* Perform multiple iterations with random delays. */

  for (idx = 0; idx < HRTIMER_TEST_RAND_ITER; idx++)
    {
      /* Generate a random delay within the specified range. */

      delay_ns = rand() % rand_ns;

      flags = spin_lock_irqsave(&rand_cancel_lock);

      cnt = param->callback_cnt;
      ASSERT(hrtimer_restart(timer, hrtimer_test_rand_cancel_callback,
                             delay_ns) == 0);

      spin_unlock_irqrestore(&rand_cancel_lock, flags);

      /* Decide to wait for the callback or cancel the watchdog. */

      if (delay_ns % 2u)
        {
          /* Wait for the callback finished. */

          while (param->callback_cnt != cnt + 1u)
            {
              hrtest_delay(delay_ns);
            }
          
          hrtimer_cancel(timer);
        }
      else
        {
          hrtimer_cancel(timer);
        }
    }

  hrtimer_cancel_sync(timer);
}

static uint64_t hrtimer_test_callback_period(FAR const struct hrtimer_s *timer, uint64_t expired_ns)
{
  FAR hrtimer_tparam_t *tparam   = (FAR hrtimer_tparam_t *)timer;
  sclock_t              interval = tparam->interval;

  tparam->callback_cnt++;
  tparam->triggered_ns = clock_systime_nsec();

  return interval;
}

static void hrtimer_test_period(FAR hrtimer_t *timer,
                                FAR hrtimer_tparam_t *param,
                                uint64_t delay_ns,
                                unsigned int times)
{
  uint64_t cnt;
  clock_t  timer_setns;
  hrtest_printf("hrtimer_test_period %" PRIu64 " ns\n", delay_ns);

  cnt = param->callback_cnt;

  param->interval = delay_ns;

  ASSERT(param->interval > 0);

  // clock_systime_nsec(&timer_setns);
  timer_setns = clock_systime_nsec();

  ASSERT(hrtimer_start(timer, hrtimer_test_callback_period, param->interval) == OK);

  hrtest_delay(times * delay_ns);

  hrtimer_cancel_sync(timer);

  DEBUGASSERT(timer->func == NULL);

  hrtest_printf("periodical hrtimer triggered %" PRIu64 " times, "
                 "elapsed nsec %" PRIu64 "\n", param->callback_cnt - cnt,
                 param->triggered_ns - timer_setns);

  if (param->callback_cnt - cnt < times)
    {
      hrtest_printf("WARNING: periodical hrtimer"
                    "triggered times < %u\n", times);
    }
}

#ifdef CONFIG_SMP
static uint64_t hrtimer_test_callback_crita(FAR const struct hrtimer_s *timer, uint64_t expired_ns)
{
  FAR hrtimer_tparam_t *hrtimer_tparam = (FAR hrtimer_tparam_t *)timer;

  /* change status */

  if (hrtimer_tparam->current_cpu != this_cpu())
    {
      hrtimer_tparam->current_cpu = this_cpu();
      hrtimer_tparam->callback_cnt++;
    }

  /* check whether parameter be changed by another critical section */

  ASSERT(hrtimer_tparam->state == 0);
  hrtimer_tparam->state = !hrtimer_tparam->state;

  return 0;
}

static uint64_t hrtimer_test_callback_critb(FAR const struct hrtimer_s *timer, uint64_t expired_ns)
{
  FAR hrtimer_tparam_t *hrtimer_tparam = (FAR hrtimer_tparam_t *)timer;

  /* change status */

  if (hrtimer_tparam->current_cpu != this_cpu())
    {
      hrtimer_tparam->current_cpu = this_cpu();
      hrtimer_tparam->callback_cnt++;
    }

  /* check whether parameter be changed by another critical section */

  ASSERT(hrtimer_tparam->state == 1);
  hrtimer_tparam->state = !hrtimer_tparam->state;

  return 0;
}

static uint64_t hrtimer_test_callback_critdelay(FAR const struct hrtimer_s *timer, uint64_t expired_ns)
{
  FAR hrtimer_tparam_t *hrtimer_tparam = (FAR hrtimer_tparam_t *)timer;
  spinlock_t *lock = hrtimer_tparam->lock;
  irqstate_t flags;

  flags = spin_lock_irqsave(lock);
  hrtimer_tparam->callback_cnt++;
  spin_unlock_irqrestore(lock, flags);

  up_ndelay(100 * NSEC_PER_USEC);

  return 0;
}

static void hrtimer_test_critical_section(FAR hrtimer_t *timer,
                                          FAR hrtimer_tparam_t *param)
{
  int      cnt = 0;
  uint64_t callback_cnt;
  spinlock_t lock  = SP_UNLOCKED;

  param->lock = &lock;

  DEBUGASSERT(!HRTIMER_ISPENDING(timer));

  while (cnt < HRTIMER_TEST_CRITICAL_SECTION)
    {
      /* set param statue and start wdog */

      param->state = 0;
      param->current_cpu = this_cpu();
      hrtimer_start(timer, hrtimer_test_callback_crita, 0);

      /* set param statue and start wdog */

      hrtimer_cancel_sync(timer);
      param->state = 1;
      param->current_cpu = this_cpu();
      hrtimer_start(timer, hrtimer_test_callback_critb, 0);

      if (++cnt % 256 == 0)
        {
          printf("hrtimer critical section test1 %d times.\n", cnt);
        }

      hrtimer_cancel_sync(timer);
    }

  cnt = 0;

  param->callback_cnt = 0;

  while (cnt < HRTIMER_TEST_CRITICAL_SECTION)
    {
      /* set param statue and start wdog */

      irqstate_t flags = spin_lock_irqsave(&lock);

      hrtimer_start(timer, hrtimer_test_callback_critdelay, 0);

      spin_unlock_irqrestore(&lock, flags);

      up_ndelay(10000);

      flags = spin_lock_irqsave(&lock);

      hrtimer_cancel(timer);
      hrtimer_start(timer, hrtimer_test_callback_critdelay, 0);

      spin_unlock_irqrestore(&lock, flags);

      up_ndelay(10000);

      hrtimer_cancel_sync(timer);
      callback_cnt = param->callback_cnt;

      hrtest_delay(10000);

      ASSERT(callback_cnt == param->callback_cnt);

      if (++cnt % 256 == 0)
        {
          printf("hrtimer critical section test2 %d times. count %llu\n", cnt,
                 (unsigned long long)param->callback_cnt);
        }
    }

  hrtimer_cancel_sync(timer);

  param->lock = NULL;
}
#endif

static void hrtimer_test_run(FAR hrtimer_tparam_t *param)
{
  uint64_t             cnt;
  uint64_t             rest;
  FAR hrtimer_t *test_hrtimer = &param->timer;


  /* Wrong arguments of the hrtimer_start */

  ASSERT(hrtimer_start(test_hrtimer, NULL, 0) != OK);
  ASSERT(hrtimer_start(test_hrtimer, NULL, -1) != OK);

  /* Delay = 0 */

  hrtimer_test_once(test_hrtimer, param, 0);

  /* Delay > 0, small */

  hrtimer_test_once(test_hrtimer, param, 1);
  hrtimer_test_once(test_hrtimer, param, 10);
  hrtimer_test_once(test_hrtimer, param, 100);
  hrtimer_test_once(test_hrtimer, param, 1000);
  hrtimer_test_once(test_hrtimer, param, 10000);

  /* Delay > 0, middle 100us */

  hrtimer_test_once(test_hrtimer, param, 100000);
  hrtimer_test_once(test_hrtimer, param, 1000000);
  hrtimer_test_once(test_hrtimer, param, 10000000);

#ifdef CONFIG_SMP

  /* Test wdog critical section */

  hrtimer_test_critical_section(test_hrtimer, param);

#endif

  /* Delay > 0, maximum */

  cnt = param->callback_cnt;

  /* Maximum */

  ASSERT(hrtimer_start(test_hrtimer, hrtimer_test_callback, UINT64_MAX) == OK);

  /* Sleep for 1s */

  hrtest_delay(USEC_PER_SEC / 100);

  /* Ensure watchdog not alarmed */

  ASSERT(cnt == param->callback_cnt);

  rest = hrtimer_gettime(test_hrtimer);

  ASSERT(rest < UINT64_MAX);

  ASSERT(hrtimer_cancel_sync(test_hrtimer) == OK);

  hrtest_printf("hrtimer_start with maximum delay, cancel OK, rest %" PRIu64 "\n",
                rest);

  /* period wdog delay from 1000us to 10000us */

  hrtimer_test_period(test_hrtimer, param, 1000000, 128);

  /* Random delay ~12us */

  hrtimer_test_rand(test_hrtimer, param, 12345);

  hrtimer_test_rand_cancel(test_hrtimer, param, 67890);
}

/* Multi threaded */

static FAR void *hrtimer_test_thread(FAR void *param)
{
  hrtimer_test_run(param);
  return NULL;
}

/****************************************************************************
 * Public Functions
 ****************************************************************************/

void hrtimer_test(void)
{
  unsigned int   thread_id;
  pthread_attr_t attr;
  pthread_t      pthreads[HRTIMER_TEST_THREAD_NR];
  hrtimer_tparam_t params[HRTIMER_TEST_THREAD_NR] =
    {
      0
    };

  printf("hrtimer_test start...\n");

  ASSERT(pthread_attr_init(&attr) == 0);

  /* Create wdog test thread */

  for (thread_id = 0; thread_id < HRTIMER_TEST_THREAD_NR; thread_id++)
    {
      ASSERT(pthread_create(&pthreads[thread_id], &attr,
                            hrtimer_test_thread, &params[thread_id]) == 0);
    }

  for (thread_id = 0; thread_id < HRTIMER_TEST_THREAD_NR; thread_id++)
    {
      pthread_join(pthreads[thread_id], NULL);
    }

  ASSERT(pthread_attr_destroy(&attr) == 0);

  printf("hrtimer_test end...\n");
}

[wangchdo]

@Fix-Point @xiaoxiang781216 @GUIDINGLI

I believe this hritmer feature is genuinely useful for Apache NuttX, which is why I decided to introduce it about three months ago. Since then, I have continued to refine and improve it whenever shortcomings were identified, with the goal of making it more robust and mature.

I really don’t want to get into a dispute again. If you want to merge this refactoring, I am not able to stop you

However, I would appreciate it if my implementation were not described as incorrect or even completely unusable, or described as having functional issues or being inferior in terms of performance, readability, or effectiveness.

I acknowledge that the first version did not fully account for SMP, but I have submitted a fix (#17642
) that addresses all SMP cases. This fix is straightforward, as it focuses solely on improving the Hrtimer state-machine.

Finally, before merging this, I would suggest performing a performance comparison with my PR (#17573
) to ensure the best outcome for NuttX.

Thank you!

Best Regards
Wang Chengdong

[anchao]

Setting aside the implementation details, there are far too many critical section issues that I haven’t pointed out them exhaustively. You are advised to fully resolve them before submitting the code.
Also, please watch your wording. What is "incorrect"? I’m genuinely curious about your professionalism and respectfulness — you are essentially belittling the work of individual developers.

[Fix-Point]

Setting aside the implementation details, there are far too many critical section issues that I haven’t pointed out them exhaustively. You are advised to fully resolve them before submitting the code. Also, please watch your wording. What is "incorrect"? I’m genuinely curious about your professionalism and respectfulness — you are essentially belittling the work of individual developers.

I am sorry for my mistake. The incorrect should be functional incorrect, which means it is inconsistent with the design specifications. (E.g, a cancelled and restarted periodic timer will not never be overwritten by an old periodic timer callback that has not yet completed in any cases.)

[wangchdo]

Setting aside the implementation details, there are far too many critical section issues that I haven’t pointed out them exhaustively. You are advised to fully resolve them before submitting the code. Also, please watch your wording. What is "incorrect"? I’m genuinely curious about your professionalism and respectfulness — you are essentially belittling the work of individual developers.

I am sorry for my mistake. The incorrect should be functional incorrect, which means it is inconsistent with the design specifications. (E.g, a cancelled and restarted periodic timer will not never be overwritten by an old periodic timer callback that has not yet completed in any cases.)

A minor fix can resolve the issue you mentioned, check #17642, you don't need to do such a big refactoring

[Fix-Point]

@GUIDINGLI suggested conducting performance tests on non-SMP, so I performed more detailed tests on intel64:nsh.

I actually measured the performance of the following two restart approaches:

This is our current implementation (inline functions have already been expanded):

// hrtimer_start_absolute(&hrtimer, (hrtimer_callback_t)clock_gettick_test, NULL, INT64_MAX - 1);
hrtimer.func = clock_gettick_test;
hrtimer.arg  = NULL;
hrtimer.expired = INT64_MAX - 1; // avoid reprogramming the timer
hrtimer_async_restart(&hrtimer); 

This is the implementation recommended by @wangchdo , who claimed it could improve performance.

// we have already initialized hrtimer.expired with INT64_MAX - 1 and hrtimer.arg with NULL.
hrtimer.func = clock_gettick_test; // Equivalent to modifying the state.
hrtimer_async_restart(&hrtimer); 

As mentioned earlier, the main time overhead of hrtimer_start comes from reprogramming the actual hardware timer (over 1000 CPU cycles). To avoid this overhead, we inserted a timer set to trigger in 200 years later for this test case. Both timer insertions do not actually program the hardware timer since they are not the earliest expired timer.

The test results showed that: the average overhead for both implementation tests running 1 million times is 48 CPU cycles (~23 ns). This demonstrates that CPU pipelining can really hide the extra read/write overhead. Therefore, @wangchdo claimed that his API design can improve restart performance lacks evidence.

Additionally, I measured that the time consumed by the synchronization (up_irq_save/up_irq_restore) is 35 cycles. In other words, excluding synchronization, the average overhead for restart is only about 13 cycles (queue insertion), while the overhead for cancel + restart (including the synchronization), when no hardware timer reprogramming is involved, is only 73 cycles (~35 ns).

Compared to @wangchdo implementation in non-SMP mode, without introducing reprogramming, the main time overhead for both implementations lies in queue insertion and deletion. Since both use the same RB-Tree implementation, their overhead is identical. However, given that restart in this implementation has fewer conditional branches, its actual runtime performance should theoretically be slightly better than @wangchdo 's implementation, as it involves fewer branch predictions and branch misprediction rollbacks. At the same time, we don't need to add state or reference count to hrtimer, and its memory overhead is also lower compared to his implementation.

[wangchdo]

Contributor

Hi @GUIDINGLI @Fix-Point

Thanks for your kindly comments, I am so happy that we are back to focus on friendly and kindly technical discussion.

I apologize for my previous comments which emphasize that your hrtimer implementation is similar to mine.

Also, I respect so much for your great solution in this PR of resolving concurrency issues of hritmer under SMP and your performance test. But I still want you take a look at my latest update of hrtimer in #17573, I updated it for performance and multicore concurrency protection (mainly by replacing the explicit state-machine operations)

I added an analysis from code-level in #17573 to illustrate the method which in my opinion is a better way for implementing hrtimer with the concurrency issues fixed. Please take a look.

My points are mainly on two aspects:

1. I believe my API design is more user-friendly (this is the most important)

My API design is as follows:

• hrtimer_init() — initializes the hrtimer object and assigns the callback and argument.
• hrtimer_start() — sets the expiration time for an hrtimer.
• hrtimer_set() — allows the user to update the callback and argument of an hrtimer.
• hrtimer_cancel() — cancels an hrtimer asynchronously.
• hrtimer_cancel_sync() — cancels an hrtimer synchronously.

2. I believe my latest method of fixing SMP concurrency issue or violation of ownership invariant under SMP is more effective.

3. I insist on improving the hrtimer in the future with the above two points not invalidated

[Fix-Point]

Contributor

Hi @GUIDINGLI @Fix-Point

Thanks for your kindly comments, I am so happy that we are back to focus on friendly and kindly technical discussion.

I apologize for my previous comments which emphasize that your hrtimer implementation is similar to mine.

Also, I respect so much for your great solution in this PR of resolving concurrency issues of hritmer under SMP and your performance test. But I still want you take a look at my latest update of hrtimer in #17573, I updated it for performance and multicore concurrency protection (mainly by replacing the explicit state-machine operations)

I added an analysis from code-level in #17573 to illustrate the method which in my opinion is a better way for implementing hrtimer with the concurrency issues fixed. Please take a look.

My points are mainly on two aspects:

1. I believe my API design is more user-friendly (this is the most important)

My API design is as follows:

• hrtimer_init() — initializes the hrtimer object and assigns the callback and argument.
• hrtimer_start() — sets the expiration time for an hrtimer.
• hrtimer_set() — allows the user to update the callback and argument of an hrtimer.
• hrtimer_cancel() — cancels an hrtimer asynchronously.
• hrtimer_cancel_sync() — cancels an hrtimer synchronously.

2. I believe my latest method of fixing SMP concurrency issue or violation of ownership invariant under SMP is more effective.

3. I insist on improving the hrtimer in the future with the above two points not invalidated

Response to 1

Here is the API comparison in Table 1. In this implementation, hrtimer API design in this implmentation is fully aligned with the wdog and wqueue. It means that users familiar with the wdog and wqueue API can quickly understand the hrtimer API. Since they have no different in performance (as I tested in #17675 (comment)), if I am the user, I prefered this one.

Table 1. Timer API Comparison

Feature	wdog	wqueue	hrtimer[#17675, this one]	hrtimer[#17573]
Start Relative	wd_start(wdog, func, arg, delay)	work_queue(queue, work, func, arg, delay)	hrtimer_start(timer, func, arg, delay)	hrtimer_start(timer, delay, MODE_REL)
Start Absolute	wd_start_absolute(wdog, func, arg, expected)	-	hrtimer_start_absolute(timer, func, arg, expected)	hrtimer_start(timer, expected, MODE_ABS)
Reset	-	-	-	hrtimer_set(timer, func, arg)
Periodic	wd_start_next(wdog, func, arg, delay)	work_queue_next(queue, work, func, arg, delay)	Return non-zero in hrtimer callback	Return non-zero in hrtimer callback
Cancel	wd_cancel(wdog)	work_cancel(queue, work)	hrtimer_cancel(timer)	hrtimer_cancel(timer)
Cancel Synchronously	-	work_cancel_sync(queue, work)	hrtimer_cancel_sync(timer)	hrtimer_cancel_sync(timer)

If you can unified the API to this one, I will update upon your code base instead of removing all of them.

Response to 2

Your latest attempt is almost same the versioning idea I tried before. Sadly, I found it still violated the ownership invariant. In fact, the expired field can not be used as version, since the expired is not monotonic, which is a fundamental assumption regarding the correctness of epoch-based memory reclamation. I believe it is very easy for you to make a test case to trigger the ownership invariant violation.

In my early implmentation, I added another monotonic version field for the hrtimer to do correct versioning (or Epoch-based memory reclamation). However, it will increase the memory footprint of the hrtimer. That's why I eventually gave up on the idea.

Sacrifies memory footprint for parallel scalability is not efficient for NuttX. Even you implement the correct Epoch-based memory reclamation, the memory footprint should always be larger than this implmentation, because you should keep tracking with the reference count and version in the hrtimer objects. I am sorry that I am still confused about what's your design balance. I think NuttX is target to run at low-memory embedded devices with limited cores, so I decide to use the hazard-pointer. Table 2 is the comparison of the memory footprint, lower means memory efficient.

Table 2. Memory Footprint Comparison (assuming clock_t is uint64_t)

Data structure	wdog	wqueue	hrtimer[#17675, this one]	hrtimer[#17573]
Size in 32-bit arch/bytes	24	24	24(list) or 32(rb-tree)	36
Size in 64-bit arch/bytes	40	40	40(list) or 56(rb-tree)	64

Response to 3

I still haven't seen any analysis or performance test data to prove that your implementation is superior to this implementation in terms of functional correctness, performance, memory footprint or code reusability. I'm afraid I think your insistence is somewhat unconvincing.

fixed

[xiaoxiang781216]

move after static

[Fix-Point]

Fixed.

[xiaoxiang781216]

unsigned int

[Fix-Point]

Fixed.

[xiaoxiang781216]

how about non-SMP case

[Fix-Point]

non-SMP case added.

[xiaoxiang781216]

what's differenct from hrtimer_queue_has_ownership

[Fix-Point]

hrtimer_queue_has_ownership should be called with the lock held.

[xiaoxiang781216]

remove the extra spaces

[Fix-Point]

Fixed.

[xiaoxiang781216]

why include here, but not hrtimer_type_xxx.h

[Fix-Point]

hrtimer_type_xxx.h is the queue implementation for internal usage.

[xiaoxiang781216]

do
  {
  }
while (0)

[Fix-Point]

Fixed.

[Fix-Point]

To improve code readability, I reorganized the PR commits. The composable hrtimer_queue will be a subsequent optimization; this part only includes the core implementation of the hrtimer. Please help with the review, thank you. @xiaoxiang781216 @GUIDINGLI @wangchdo