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 incorrect hrtimer implementation.
• Removed CONFIG_SCHED_TICKLESS_TICK_ARGUMENT to simplify kernel configuration and expiration handling.
• 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. 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.

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_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, mode): 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, mode): Starts a stopped timer. The mode parameter indicates whether it is a relative or absolute timer.
• hrtimer_async_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(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_async_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_async_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 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 async_cancel holds write ownership of this resource. Modifications to it by threads executing callback functions are prevented.

The resource ownership here uses a simplified notation. In actual static analysis or formal verification processes, more complex abstractions such as resource algebra might be employed.

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_async_cancel again, and the state remains unchanged.

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.

%%{
  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_COMPLETED|private : hrtimer callback return 0 in hrtimer_expiry
    HRTIMER_PENDING|shared --> HRTIMER_PENDING|shared : hrtimer callback return non-zero in hrtimer_expiry
    HRTIMER_PENDING|shared --> HRTIMER_CANCELED|half_shared : hrtimer_async_cancel
    HRTIMER_CANCELED|half_shared --> HRTIMER_CANCELED|private : hrtimer_cancel wait all cores release the references to the timer.
    HRTIMER_CANCELED|half_shared --> HRTIMER_PENDING|shared : hrtimer_restart
    HRTIMER_CANCELED|private --> HRTIMER_COMPLETED|private : Complete the cancel

Loading

Figure 2 HRTimer State Transition Diagram

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 & cancelcompared towd_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(&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, @wangchengdong 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          | [Lock]
      |                                      |
      | [Unlock]                             |--- hrtimer_cancel(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, arg, delay_ns) hrtimer_start(timer, cb, arg, delay_ns, HRTIMER_MODE_REL)
#define hrtimer_restart(timer, cb, arg, delay_ns) hrtimer_restart(timer, cb, arg, 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(void *arg, uint64_t expired_ns)
{
  FAR hrtimer_tparam_t *hrtimer_tparam = (FAR hrtimer_tparam_t *)arg;

  /* 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,
                       param, 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(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, param,
                           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(timer);
      DEBUGASSERT(timer->func == NULL);
    }

  hrtimer_cancel(timer);
}

static uint64_t hrtimer_test_rand_cancel_callback(void *arg, uint64_t expired_ns)
{
  FAR hrtimer_tparam_t *tparam = (FAR hrtimer_tparam_t *)arg;
  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->timer    = timer;
  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, param,
                             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);
            }

          while (HRTIMER_ISPENDING(timer))
            {
              hrtest_delay(0);
            }
        }
      else
        {
          hrtimer_async_cancel(timer);
        }
    }

  hrtimer_cancel(timer);
}

static uint64_t hrtimer_test_callback_period(void *arg, uint64_t expired_ns)
{
  FAR hrtimer_tparam_t *tparam   = (FAR hrtimer_tparam_t *)arg;
  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, param->interval) == OK);

  hrtest_delay(times * delay_ns);

  hrtimer_cancel(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(void *arg, uint64_t expired_ns)
{
  FAR hrtimer_tparam_t *hrtimer_tparam = (FAR hrtimer_tparam_t *)arg;

  /* 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(void *arg, uint64_t expired_ns)
{
  FAR hrtimer_tparam_t *hrtimer_tparam = (FAR hrtimer_tparam_t *)arg;

  /* 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(void *arg, uint64_t expired_ns)
{
  FAR hrtimer_tparam_t *hrtimer_tparam = (FAR hrtimer_tparam_t *)arg;
  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, param, 0);

      /* set param statue and start wdog */

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

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

      hrtimer_cancel(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, param, 0);

      spin_unlock_irqrestore(&lock, flags);

      up_ndelay(10000);

      flags = spin_lock_irqsave(&lock);

      hrtimer_async_cancel(timer);
      hrtimer_start(timer, hrtimer_test_callback_critdelay, param, 0);

      spin_unlock_irqrestore(&lock, flags);

      up_ndelay(10000);

      hrtimer_cancel(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(timer);

  param->lock = NULL;
}
#endif

static void hrtimer_test_run(FAR hrtimer_tparam_t *param)
{
  uint64_t             cnt;
  uint64_t             rest;
  hrtimer_t     test_hrtimer =
    {
      0
    };

  param->timer = &test_hrtimer;

  /* Wrong arguments of the hrtimer_start */

  ASSERT(hrtimer_start(&test_hrtimer, NULL, NULL, 0) != OK);
  ASSERT(hrtimer_start(&test_hrtimer, NULL, 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,
                       param, UINT64_MAX) == OK);

  /* Sleep for 1s */

  hrtest_delay(USEC_PER_SEC / 100);

  /* Testing hrtimer_cancel */

  // ASSERT(hrtimer_cancel(NULL) != 0);

  /* Ensure watchdog not alarmed */

  ASSERT(cnt == param->callback_cnt);

  rest = hrtimer_gettime(&test_hrtimer);

  ASSERT(rest < UINT64_MAX);

  ASSERT(hrtimer_cancel(&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");
}

[xiaoxiang781216]

move to separated pr

[Fix-Point]

Moved to #17680.

[xiaoxiang781216]

move to the separated pr too

[xiaoxiang781216]

merge to e72a975

[Fix-Point]

Merged.

[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.

[anchao]

why was the parameter prototype changed? If func/arg are determined in hrtimer_init or hrtimer_setup, it will yield better performance for the start process.

[Fix-Point]

Compared to previous implementation,

• when restarting the timer, writing the three fields—func, arg, and expired—does indeed involve two additional writes compared to writing only expired.
• However, within the critical section, this implementation does not require modifying the func of the hrtimer, which saves one write operation compared to the need to write the state field, resulting in an overall increase of one write outside the critical section.

The advantage of this interface design is that it reuses func to encode the state of the hrtimer, saving 4 bytes of space per hrtimer.

I believe the slight overhead outside the critical section will be hidden by the pipeline and should not have a significant impact on overall performance. I will attempt to write test cases to compare the performance of the two implementations.

[anchao]

add null pointer check or remove debug assert on line 102

[Fix-Point]

Thank you, null pointer check is added and debug assertion is removed.

[anchao]

cpu may changed without lock

[Fix-Point]

We can ensure the invariant that the core id will not change in the interrupt handling.

• NuttX currently does not support kernel preemption, thus interrupts cannot be nested within interrupt context. Even if kernel preemption and interrupt nesting are supported in the future, we can avoid nested calls to hrtimer_expiry by using an interrupt nesting counter.
• During interrupt handling, we use per-core ARCH_INTERRUPTSTACK, ensuring that the core ID remains unchanged while in interrupt context.

[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