Fix-Point commented on PR #17570: URL: https://github.com/apache/nuttx/pull/17570#issuecomment-3677359372
> > Please pay more respect to parallel programming. It does not look like a simple change can fix this at all. > > **why this change is not simple? why fixing issue is not respecting you?** > > > I hope that before you claim you fix the issue, you can review your state-machine more carefully. I must say sorry that I have not too much time to review your commit. I am not your z3 SMT solver who can always automatically generate the counter-examples for you. I think it is impossible (or very hard) to allow restart the timer correctly unless refactoring your state-machine design. At least, I can not find a solution that can ensure functional correctness for your design. That is why I recommend refactoring your implementation with #17556. > > For example, When the newly-set timer (triggered at t2) while the old timer callback function (triggered at t1) is executing simultaneously. This is a highly likely SMP scenario of thread interleaving. The old timer callback returns next as `UINT32_MAX`, while the new request callback returns next as `1000`. However, since the old timer callback detects that the hrtimer is in the `running` state, so it sets the next timer to `(t2 + UINT32_MAX)` instead of `(t1 + UINT32_MAX)`. I suspect that the reason your patch fails to run `ostest` on `rv-virt:smp` is also due to this issue. > > **I already showed that ostest passed on rv-virt:smp64** > > > ```shell > > Timeline: > > > > CPU1 (Timer Callback) CPU2 (Set New Timer) > > > > ------|--------------------------------------|------------------------- > > > > | | > > > > t1: Old timer triggers at t1 | > > > > |--- Callback starts | > > > > | hrtimer->state <- running | [Lock] > > > > | [Unlock] t2: New timer being start(t2) > > > > | |--- hrtimer_start() > > > > | | 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) | > > > > | next expired | > > > > | = t2 + UINT32_MAX (wrong!) | > > > > | hrtimer->state <- armed | > > > > | [Unlock] | > > > > | | [Lock] > > > > | |--- hrtimer->state != running > > > > | | failed to set next (wrong!) > > > > | | > > > > ------|--------------------------------------|------------------------- > > ``` > > **I don’t think the case you pointed out is an issue, as allowing an hrtimer that has just been re-armed to be restarted again is purely a design choice. My initial design allowed re-arming an already armed hrtimer. After considering @xiaoxiang781216’s comments, I have revised the implementation to treat this case as an error.** > > > By the way, here is the another simple test-case that fails that I do not even know why. It stucked at `hrtimer_cancel_sync(&timer);`. Please fix this problem first. > > I must say that I am not a lab rat or your tester. Please ensure the functional correctness of your patches before merging it to upstream. I fully agree with @anchao, who pointed out my problem in #16342 before. > > > emm ... community is not a testing ground, please do not treating everyone like lab rats. > > > > > > After that, I am trying my best to ensure my PR is functional correctness. I hope you too. > > ```c > > /**************************************************************************** > > > > * apps/examples/hello/hello_main.c > > > > * > > > > * SPDX-License-Identifier: Apache-2.0 > > > > * > > > > * 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 <stdio.h> > > > > #include <unistd.h> > > > > > > > > #include <nuttx/hrtimer.h> > > > > > > > > #define HRTIMER_TEST_THREAD_NR (1) > > > > #define HRTIMER_TEST_NR (1000000) > > > > > > > > /**************************************************************************** > > > > * Public Functions > > > > ****************************************************************************/ > > > > > > > > static int volatile tcount = 0; > > > > static volatile uint32_t next = 0; > > > > static uint32_t test_callback(FAR struct hrtimer_s *hrtimer) > > > > { > > > > tcount++; > > > > up_ndelay(hrtimer->expired % (10 * NSEC_PER_USEC)); > > > > return 0; > > > > } > > > > > > > > static uint32_t test_callback_background(FAR struct hrtimer_s *hrtimer) > > > > { > > > > up_ndelay(hrtimer->expired % NSEC_PER_USEC); > > > > return 0; > > > > } > > > > > > > > > > > > static void test1(int tid) > > > > { > > > > hrtimer_t timer; > > > > int count = 0; > > > > irqstate_t flags; > > > > spinlock_t lock; > > > > > > > > if (tid == 0) > > > > { > > > > hrtimer_init(&timer, test_callback, NULL); > > > > } > > > > else > > > > { > > > > hrtimer_init(&timer, test_callback_background, NULL); > > > > } > > > > > > > > while (count++ < HRTIMER_TEST_NR) > > > > { > > > > int ret; > > > > if (tid == 0) > > > > { > > > > uint64_t delay = rand() % (10 * NSEC_PER_MSEC); > > > > > > > > /* Simulate the periodical hrtimer.. */ > > > > > > > > flags = spin_lock_irqsave(&lock); > > > > > > > > /* Use as periodical timer */ > > > > > > > > ret = hrtimer_cancel(&timer); > > > > ret = hrtimer_start(&timer, 1000, HRTIMER_MODE_REL); > > > > > > > > spin_unlock_irqrestore(&lock, flags); > > > > > > > > up_ndelay(NSEC_PER_MSEC); > > > > > > > > flags = spin_lock_irqsave(&lock); > > > > > > > > ret = hrtimer_cancel(&timer); > > > > ret = hrtimer_start(&timer, 1000, HRTIMER_MODE_REL); > > > > spin_unlock_irqrestore(&lock, flags); > > > > > > > > up_ndelay(NSEC_PER_MSEC); > > > > > > > > hrtimer_cancel_sync(&timer); // stucked here???? > > > > printf("???\n"); > > > > } > > > > else > > > > { > > > > /* Simulate the background hrtimer.. */ > > > > > > > > uint64_t delay = rand() % (10 * NSEC_PER_MSEC); > > > > > > > > ret = hrtimer_cancel(&timer); > > > > ret = hrtimer_start(&timer, delay, HRTIMER_MODE_REL); > > > > } > > > > > > > > UNUSED(ret); > > > > } > > > > } > > > > > > > > static void* test_thread(void *arg) > > > > { > > > > while (1) > > > > { > > > > test1((int)arg); > > > > } > > > > return NULL; > > > > } > > > > /**************************************************************************** > > > > * hello_main > > > > ****************************************************************************/ > > > > > > > > int main(int argc, FAR char *argv[]) > > > > { > > > > unsigned int thread_id; > > > > pthread_attr_t attr; > > > > pthread_t pthreads[HRTIMER_TEST_THREAD_NR]; > > > > > > > > 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, > > > > test_thread, (void *)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"); > > > > return 0; > > > > } > > ``` > > **this is a minor issue, i just forgot to set hritmer to inactive when period is 0 according to the state machine I designe, it is fixed now:** > > ``` > > case HRTIMER_STATE_RUNNING: > { > /* Timer callback completed normally */ > > if (period > 0) > { > hrtimer->expired += period; > hrtimer->state = HRTIMER_STATE_ARMED; > RB_INSERT(hrtimer_tree_s, &g_hrtimer_tree, > &hrtimer->node); > } > else > { > hrtimer->state = HRTIMER_STATE_INACTIVE; > } > > break; > } > ``` > > **and your new test passed on rv-virt:smp64 when this update is added:** > > ``` > NuttShell (NSH) > nsh> > nsh> > nsh> uname -a > NuttX 0.0.0 02057dc356-dirty Dec 20 2025 00:21:54 risc-v rv-virt > nsh> > nsh> hello > > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > ??? > > (...) > ``` > > **By the way, I have implemented hrtimer as an independent module, so even if it is not perfect, it does not impact existing NuttX functionality. I also want to emphasize that I respect you very much and have no intention of offending you. My concern is simply that spending a significant amount of time re-inventing the wheel—and claiming that a new implementation is superior in every aspect—may not be the most productive approach. In my opinion, building improvements on top of an existing implementation could be a more efficient and constructive direction.** I sincerely apologize for providing incorrect test cases earlier. Due to time constraints, I did not have the opportunity to review them promptly, which unfortunately wasted your time. To address this, I have now provided a reproducible test case that demonstrates the kind of thread interleaving I mentioned. ```c #include <nuttx/config.h> #include <stdio.h> #include <nuttx/hrtimer.h> #include <sys/resource.h> /**************************************************************************** * Name: hrtimer_process * * Description: * Called from the timer interrupt handler to process expired * high-resolution timers. If a timer has expired, its callback * function will be executed in the context of the timer interrupt. * * Input Parameters: * now - The current time (nsecs). * * Returned Value: * None ****************************************************************************/ void hrtimer_process(uint64_t now); /**************************************************************************** * Inline Functions ****************************************************************************/ /**************************************************************************** * Name: hrtimer_gettime * * Description: * Get the current high-resolution time in nanoseconds. * * Returned Value: * Current time in nanoseconds. ****************************************************************************/ static inline_function uint64_t hrtimer_gettime(void) { struct timespec ts; /* Get current time from platform-specific timer */ clock_systime_timespec(&ts); /* Convert timespec to nanoseconds */ return clock_time2nsec(&ts); } #define HRTIMER_TEST_THREAD_NR (4) /**************************************************************************** * Public Functions ****************************************************************************/ static volatile int count = 0; static hrtimer_t timer; static spinlock_t lock = SP_UNLOCKED; static uint32_t test_callback(FAR struct hrtimer_s *hrtimer) { irqstate_t flags; up_ndelay(10 * NSEC_PER_USEC); flags = spin_lock_irqsave(&lock); count++; spin_unlock_irqrestore(&lock, flags); return 0; } static uint32_t test_callback_back(FAR struct hrtimer_s *hrtimer) { return 0; } static void* test_thread(void *arg) { irqstate_t flags; int tid = (int)arg; int ret; if (tid == 0) { hrtimer_init(&timer, test_callback, NULL); } while (1) { if (tid == 0) { int tmp_count; flags = spin_lock_irqsave(&lock); ret = hrtimer_cancel(&timer); ret = hrtimer_start(&timer, 0, HRTIMER_MODE_REL); spin_unlock_irqrestore(&lock, flags); up_ndelay(1000); spin_lock_irqsave(&lock); ret = hrtimer_cancel(&timer); ret = hrtimer_start(&timer, 0, HRTIMER_MODE_REL); spin_unlock_irqrestore(&lock, flags); ret = hrtimer_cancel_sync(&timer); // timer should be fully-cancelled here. tmp_count = count; // Read the count printf("%d\n", tmp_count); ASSERT(tmp_count == count); // This should not assert since timer has been cancelled. } else { flags = irq_save(); hrtimer_process(hrtimer_gettime()); irq_restore(flags); } } UNUSED(ret); return NULL; } /**************************************************************************** * hello_main ****************************************************************************/ int main(int argc, FAR char *argv[]) { unsigned int thread_id; pthread_attr_t attr; pthread_t pthreads[HRTIMER_TEST_THREAD_NR]; 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, test_thread, (void *)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"); return 0; } ``` In this scenario, a timer that should have been canceled continues to execute. This could lead to very subtle concurrency bugs (such as use-after-free behavior). If such issues make their way into the production environment, they could result in hundreds or even thousands of man-hours being spent on debugging, causing significant losses. Therefore, I find your claim that this is not a problem to be unconvincing. I support @anchao’s viewpoint that we should design HRTimer from the user's perspective. In https://github.com/apache/nuttx/pull/17556, such issues are guaranteed not to occur. As I have mentioned before, **there are fundamental flaws in the underlying state machine design of your hrtimer. Minor modifications cannot fix these issues**. Therefore, I believe the **most efficient approach is to replace your implementation with https://github.com/apache/nuttx/pull/17556**. Honestly, your attempts, in my view, are futile. If you insist on finding a correct solution through trial and error, I can only respect your decision and wish you the best. Regarding the accusation of “reinventing the wheel,” I must counter your argument— - Strive for excellence. Today, there are numerous embedded RTOS options available. If NuttX aims to attract more users, it must excel in every aspect and prove itself superior to other RTOSes. Even a 1% optimization in CPU time overhead or a few bytes of memory savings can benefit tens of millions of IoT devices running NuttX, saving substantial energy and hardware costs. In my opinion, every byte and every CPU cycle should be optimized as much as possible. I hope you share this mindset. - As educated engineers, we should strive to deliver our best work. However, pursuing excellence demands significant time and effort—if you are familiar with the Pareto principle, you’ll understand that achieving 80% often requires only 1% of the effort, but perfecting the remaining 20% demands 99% of the effort. Please do not assume that those who pursue this level of refinement are wasting their time. This is extremely disrespectful. Furthermore, as @anchao emphasized, the community is not a testing ground. We should at least ensure basic functional correctness before merging code into the main branch. Please refrain from hastily integrating flawed code like this in the future. Finally, over the coming weeks, I will focus on merging https://github.com/apache/nuttx/pull/17556. I do not have the time to repeatedly point out the fundamental issues in your underlying state machine design. I hope you understand. If you have the time, I would appreciate it if you could review my design for any functional correctness issues or provide suggestions for improvement. Thank you. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
