Repository navigation
timer: Adjust calendar timers based on monotonic timer instead of realtime - #16428
Conversation
anitazha
left a comment
anitazha
left a comment
There was a problem hiding this comment.
Thanks! Love the new test and I also like that the manager timestamps will reflect realtime relative to the current clock value now.
There was a problem hiding this comment.
Maybe remove this comment about t->persistent since this also applies to non-persistent timers?
|
bionic "boot-and-services" are not doing so hot. Any clue if it's this PR or more widespread? |
yuwata
added
ci-fails/needs-rework 🔥
|
So I decided to make this PR smaller, by only fixing the issue with the OnCalendar= timers, but not touching the manager startup timestamp. My logic here is that if the realtime clock for some reason fluctuates and starts becoming unreliable, then the manager startup time might start to drift, which might just look odd... So I think we might need more discussion on that one before merging. This one to me is a clear fix, since scheduling timers based on monotonic time just makes sense. Yeah not sure why the bionic tests failed, I looked at the logs and didn't look related to my changes at all... Let's see how they do on this second run. Done removing the comment line you mentioned. Thanks for the review! |
…ltime
When the RTC time at boot is off in the future by a few days, OnCalendar=
timers will be scheduled based on the time at boot. But if the time has been
adjusted since boot, the timers will end up scheduled way in the future, which
may cause them not to fire as shortly or often as expected.
Update the logic so that the time will be adjusted based on monotonic time.
We do that by calculating the adjusted manager startup realtime from the
monotonic time stored at that time, by comparing that time with the realtime
and monotonic time of the current time.
Added a test case to validate this works as expected. The test case creates a
QEMU virtual machine with the clock 3 days in the future. Then we adjust the
clock back 3 days, and test creating a timer with an OnCalendar= for every 15
minutes. We also check the manager startup timestamp from both `systemd-analyze
dump` and from D-Bus.
Test output without the corresponding code changes that fix the issue:
Timer elapse outside of the expected 20 minute window.
next_elapsed=1594686119
now=1594426921
time_delta=259198
With the code changes in, the test passes as expected.
anitazha
added
good-to-merge/waiting-for-ci 👍
keszybz
added
ci-failure-appears-unrelated
and removed
good-to-merge/waiting-for-ci 👍
|
I dont get this. Why is this necessary? we watch wallclock time changes, and should reschedule everything in that case. Using the monotonic clock looks like a hack to me, we should schedule wallclock events by wallclock time, and watch wallclock time changes to deal with time jumps. There might be a bug with rescheduling things on time jumps, but I am pretty sure that should not be fixed like this... |
|
hmm, look at this again, i think I grok why you want this, i.e. it's not actually about rescheduling things when time changes but it's only about figuring out the timestamps for the initial elapsing, i.e. when cross connecting the boot time timestamps to the timer scheduler. I think this is more acceptable then. So ignore my comment above. I must say I am not too fond of the idea that we fill in a dual_timestamp here though, even though we just care about the realtime value... |
|
I prepped a fix for the dual_timestamp mapping stuff I complained about, please have a look at #16536 |
5 participants
When the RTC time at boot is off in the future by a few days,
OnCalendar=timers will be scheduled based on the time at boot. But if the time has been adjusted since boot, the timers will end up scheduled way in the future, which may cause them not to fire as shortly or often as expected.Update the logic so that the time will be adjusted based on monotonic time. We do that by calculating the adjusted manager startup realtime from the monotonic time stored at that time, by comparing that time with the realtime and monotonic time of the current time.
Added a test case to validate this works as expected. The test case creates a QEMU virtual machine with the clock 3 days in the future. Then we adjust the clock back 3 days, and test creating a timer with an
OnCalendar=for every 15minutes.
Test output without the corresponding code changes that fix the issue:
With the code changes in, the test passes as expected.
Fixes #16347. cc @anitazha.