Fix-Point commented on PR #17675: URL: https://github.com/apache/nuttx/pull/17675#issuecomment-3707874701
> > 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. Since they have no different in performance (as I tested in https://github.com/apache/nuttx/pull/17675#issuecomment-3701557519), if I am the user, I prefered this one. **Table 1. Timer API Comparison** | Feature | `wdog` | `wqueue` | `hrtimer[#17675]` | `hrtimer[#17573]` | | :--- | :--- | :--- | :--- | :--- | | **Start Relative Timer** | `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 Timer** | `wd_start_absolute(wdog, func, arg, expected)` | - | `hrtimer_start_absolute(timer, func, arg, expected)` | `hrtimer_start(timer, delay, MODE_ABS)` | | **Start Periodic Timer** | `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 the Timer** | `wd_cancel(wdog)` | `work_cancel(queue, work)` | `hrtimer_cancel(timer)` | `hrtimer_cancel(timer)` | | **Cancel Timer 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 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 designed to running at low-memory embedded devices, so I decide to use the hazard-pointer. Table 2 is the comparison of the memory footprint, lower means better memory efficient. **Table 2. Memory Footprint Comparison (assuming clock_t is uint64_t)** | Data structure | `wdog` | `wqueue` | `hrtimer[#17675]` | `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 data to prove that your implementation is superior to mine in terms of functional correctness, performance, memory footprint or code reusability. I'm afraid I think your insistence is somewhat unconvincing. -- 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]
