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 concurrently, but the expired value is the same. Here would be a
design choice, if you think this is a violation of ownership invariant, it can
be fixed by adding a check whether the func is changed here. If you think it is
not, the check is not needed here as the current implementation.
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]
