Fix-Point commented on PR #17573: URL: https://github.com/apache/nuttx/pull/17573#issuecomment-3701621485
> > @anchao @wangchdo I have serious doubts about your professionalism. > > > @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. Simple modifications like reference counting can not fix this implementation at all. > > > > > > Why are you allowing such a functional incorrect code to be merged into the master branch. Isn’t this simply a case of factional bias clouding judgment? Please check this, thanks. [#17675 (comment)](https://github.com/apache/nuttx/pull/17675#issuecomment-3698712790) > > @Fix-Point This PR addresses the SMP corner cases you mentioned. Could you please take a look? I propose this solution for the following reasons: > > 1. It allows separate optimization paths for SMP and non-SMP, leaving room for extreme performance and strong determinism in both cases. (per @xiaoxiang781216 's comments I will submit a separate PR to put the SMP specific logics in CONFIG_SMP ) > 2. It provides clean, easy-to-use and minimal user-facing APIs — only three core APIs, with no duplicated parameters: > `hrtimer_init()` > `hrtimer_start()` > `hrtimer_cancel() / hrtimer_cancel_sync()` > 3. It provides basement for queue abstraction if needed. > > @xiaoxiang781216 I resolved you comments, could you please take a look again @acassis @anchao please double check To behonest, you've only modified the code to prevent the test program from getting stuck there. You haven't solved the actual problem: **canceled periodic timer cannot overwrite new timer**. I've emphasized this many times: why make futile attempts again and again? >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. -- 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]
