wangchdo commented on code in PR #17573:
URL: https://github.com/apache/nuttx/pull/17573#discussion_r2659540340
##########
sched/hrtimer/hrtimer_process.c:
##########
@@ -94,32 +106,36 @@ void hrtimer_process(uint64_t now)
RB_REMOVE(hrtimer_tree_s, &g_hrtimer_tree, &hrtimer->node);
- /* Ensure the timer callback is valid */
+ /* Increment running reference counter */
+
+ hrtimer->cpus++;
- DEBUGASSERT(hrtimer->func != NULL);
+ /* cpus is a running reference counter and must never wrap */
- hrtimer->state = HRTIMER_STATE_RUNNING;
+ DEBUGASSERT(hrtimer->cpus != 0);
- spin_unlock_irqrestore(&g_hrtimer_spinlock, flags);
+ /* Leave critical section before invoking the callback */
+
+ write_sequnlock_irqrestore(&g_hrtimer_spinlock, flags);
/* Invoke the timer callback */
- period = hrtimer->func(hrtimer);
+ period = func(arg);
- flags = spin_lock_irqsave(&g_hrtimer_spinlock);
+ /* Re-enter critical section to update timer state */
- if ((hrtimer->state == HRTIMER_STATE_CANCELED) || (period == 0))
- {
- /* Timer is canceled or one-shot; mark it inactive */
+ flags = write_seqlock_irqsave(&g_hrtimer_spinlock);
- hrtimer->state = HRTIMER_STATE_INACTIVE;
- }
- else
+ hrtimer->cpus--;
+
+ /* Re-arm periodic timer if not canceled or re-armed concurrently */
+
+ if (period > 0 && hrtimer->expired == expired)
Review Comment:
> I still insist what I stated before:
>
> > Designing correct concurrent algorithms requires systematic
consideration. As I stated in #17570, after carefully reviewing your
implementation, I could not really find a good solution that preserves version
information without introducing significant performance or memory overhead.
In my opinion, I think your new implementation is not proper due to three
reasons:
1. Your way of protecting timer from concurrency update is not efficient
2. You totally refactored the existing more user-friendly API design, this
is not correct
3. Your queue abstraction design should be based on the existing hrtimer
design.in fact, if you do not provide your way of queue abstraction, i already
planned to add mine.And, In my opinion, maybe I am wrong,Nuttx is a RTOS, not a
OS as linux, but I found your queue abstraction has a Linux style.
At the last, i also want to make this concurrency systematic consideration
illustrated in a more easy to understand in my opinion:
**If you want to fix concurrency issue, you only need to figure out all the
data that may be wrongly used in concurrency case, and then find a way to
protect them, this is what i think of as systematic thinking for concurrency
issues**
--
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]
