virtual_alarm: add unit test for quick alarm case and fix #3277
Conversation
|
FYI I updated the unit test to better reflect the scenario. We need another alarm in the future to make the test more accurate since that it way really triggers/exposes the issue. |
|
Can you explain the exact issue? The way that the virtualizer is currently implemented, this case should be handled by setting a low-level alarm in the very near future. I.e., suppose I have these three Alarms: A: reference: 100, dt: 50 At time 150, the low-level alarm should fire. This should trigger alarm A. Suppose that, when the loop across the alarms examines B, the current time t=153, so B does not fire. However, by the time the loop completes it is now t=155. This means that B should have fired but didn't. My understanding is that the statement starting here tock/capsules/src/virtual_alarm.rs Line 277 in b4ac85a Line 347 in b4ac85a tock/arch/rv32i/src/machine_timer.rs Line 48 in b4ac85a But it sounds like this is not happening? I see this approach (delays sometimes in alarms that are very close) as preferable to the potential of introducing an infinite loop. Delaying the alarms (to a subsequent interrupt) doesn't break concurrency in the system: the kernel will continue to handle interrupts, etc. In contrast, with this approach, if I repeatedly set an alarm with a tiny duration, I might enter an infinite loop. But you're seeing this problem happen in practice, so I must be missing something? Perhaps that line of code at the end lf the |
I think this is the problem. let next = self
.virtual_alarms
.iter()
.filter(|cur| cur.armed.get())
.min_by_key(|cur| cur.dt_reference.get().reference_plus_dt().wrapping_sub(now));is incorrect. It should not be a simple |
happens if a virtual alarm expires *after* the virtualizer loops over it but *before* the virtualizer calculates the next alarm. It is possible that there is an alarm that has not fired but is in the past. The bug is that when calculating the next alarm, the virtualizer performs a simple `wrapping_sub` of reference+dt to compare it with `now`. This calculation will make alarm in the past appear to be in the far future. Therefore, when calculating what the "next" alarm is, the code needs to consider if an alarm is in the past (but has not fired). Such an alarm should be one of the "newest" alarms, and so cause the virtualizer to schedule a near-immediate alarm to fire and trigger it. This PR also fixes a syntax error in the macro for a multi-alarm test.
|
@jettr Can you check whether this fixes the problem? |
Fix issue that occurs if a virtual alarm expires *after* the virtualizer loops over it but *before* the virtualizer calculates the next alarm. It is possible that there is an alarm that has not fired but is in the past. The bug is that when calculating the next alarm, the virtualizer performs a simple `wrapping_sub` of reference+dt to compare it with `now`. This calculation will make alarm in the past appear to be in the far future. Therefore, when calculating what the "next" alarm is, the code needs to consider if an alarm is in the past (but has not fired). Such an alarm should be one of the "newest" alarms, and so cause the virtualizer to schedule a near-immediate alarm to fire and trigger it. Also add unit test for verify this is fixed and prevent future regressions. Change-Id: Ib9e95e0c2d8791b0eb544881b604ede79b6fdb51
|
Yes, the code above does fix the issue and pass the unit test. I forced pushed this PR with your fix and the unit test (which I also updated to be a little more clear). |
|
bors r+ |
|
🔒 Permission denied Existing reviewers: click here to make phil-levis-google a reviewer |
|
bors r+ |
Pull Request Overview
Ensure we test the case when there are multiple alarms that do not get fired on the first pass of checking, but they would have fired by the time we do the calculation to set the next arm time. In that scenario the next arm time is way in the future (rollover) and the alarms do not get called when they are supposed to.
@phil-levis provided final fix for issue.
Formatting
make prepush.