-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move to CLOCK_REALTIME introduce clock.h and lf_atomic.h #346
Conversation
…values due to Windows. Now split up into explicit 32bit and 64bit versions
system is too inflexible for that
@sberkun, I found the need to update the atomics implementations already now. For the 32bit platforms I think we have to do it by disabling interrupts. So I have moved |
@edwardalee @petervdonovan I am asking for your feedback on this. This got a little more involved than I had planned. I need a proper pass through the diff to add proper docs of the new functions etc. But would like to hear what you think about the proposed changes here. |
@edwardalee I mark this as ready for review. I think a quite important and subtle point here is that this PR moves the responsibility of translating back and forth between the physical clock and the synchronized physical clock introduced by the clock synchronization that we support. It is now moved to the platform API implementation. This means that whenever you do
This is a little subtle because some platforms do sleeping by calculating a sleep duration and then calling some lower-level API. In that case you should NOT subtract any clock sync offset since you will compute the duration relative There is a way to avoid this, which is to add a layer between the platform API and the user-facing code. For |
- Create clock.h, a layer between platform.h and the user/runtime - Move applying/removing clock_sync offsets to clock.h - Move monotonicity enforcement from tag.h to clock.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks excellent to me. I've added a few small suggestions.
…le with single-threaded also
Co-authored-by: Edward A. Lee <[email protected]>
…into clock-realtime
Thanks for the thorough review @edwardalee. I am sorry that I have made some additional substantial changes which I need some approval for. As I discussed in the meeting I introduced now a layer between the timing APIs of I think this is an improvement and makes thing more clear. We can definitely support dropping in your own clock sync protocol at some point. For now I had to hide it with macros because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This turned into a pretty big effort! It looks great to me. I've appended a few more small suggestions.
Co-authored-by: Edward A. Lee <[email protected]>
…into clock-realtime
I talked to Kevin Stanton, who recently retired from Intel after leading for many years their PTP efforts. He told me that some Intel platforms are already enforcing monotonicity for CLOCK_REALTIME, and that it is providing the PTP time. He says he thinks this is the trend and that all Linux systems will eventually do this. This reinforces that this PR is really the right approach for the long term. |
Great to hear! Would be quite interesting to hear more from him on how we can make the LF runtime as relevant as possible for the applications they were targeting at his group at Intel. Also what hardware we should target for our future PTP/TSN testbeds. |
The main purpose of this PR is to move to pure CLOCK_REALTIME on UNIX systems as discussed here #289. It gained a little weight and now it includes:
Older discussion:
After some consideration I decided to move the application of clock sync offset below the platform API. Previously this was done in tag.h, the layer above the platform API. The disadvantage was that we had an asymmetry. Because whenever you wanted to use lf_sleep_until or lf_cond_timedwait you had to remove the clock sync offset before calling it. Now the platform implementations has to apply the clock sync offset before handing off a timestamp, and it will also remove any clock_sync offset from absolute timestamps it receives for sleeping. It introduce some repetition but now the timestamps moving to/from the platform API are more consistent.
I also implemented a thread-safe and 32bit/64bit safe modification of the clock sync offset with a mutex. This is up for discussion. It makes it a lot costlier to read out the current time when we have clock-sync enabled.
There is still a FIXME with regards to how we best can make lf_time_physical monotonic and thread-safe on 32bit and 64bit systems. I think it is the same problem as the modification of the clock_sync offset, but more critical as it affects all systems.
Todos: