wangchdo commented on code in PR #17573:
URL: https://github.com/apache/nuttx/pull/17573#discussion_r2659532682
##########
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:
> 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** (the newer timer may has same or older `expire` than the older
timer), 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.
Can you tell the incorrect scenario using the APIs I provided? The new timer
has the same expired value? I think the only case is that the user changes the
callback, and the expired value is the same. it is easy to fix the issue you
are mentioning by adding a check whether the func is changed here. But i don't
think it is necessary,if you think it is necessary, i will add it here.
If users don't change the callback, it is meaningless to make the same timer
with a same expired value...
--
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]
