| Message ID | 20220127114536.1121765-6-mlichvar@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Virtual PTP clock improvements and fix | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 3 of 3 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Thu, Jan 27, 2022 at 12:45:36PM +0100, Miroslav Lichvar wrote: > When a virtual clock is being created, initialize the timecounter to the > current system time instead of the Unix epoch to avoid very large steps > when the clock will be synchronized. I think we agreed that, going forward, new PHC drivers should start at zero (1970) instead of TAI - 37. Thanks, Richard
On Thu, Jan 27, 2022 at 02:01:16PM -0800, Richard Cochran wrote: > On Thu, Jan 27, 2022 at 12:45:36PM +0100, Miroslav Lichvar wrote: > > When a virtual clock is being created, initialize the timecounter to the > > current system time instead of the Unix epoch to avoid very large steps > > when the clock will be synchronized. > > I think we agreed that, going forward, new PHC drivers should start at > zero (1970) instead of TAI - 37. I tried to find the discussion around this decision, but failed. Do you have a link? To me, it seems very strange to start the PHC at 0. It makes the initial clock correction unnecessarily larger by ~7 orders of magnitude. The system clock is initialized from the RTC, which can have an error comparable to the TAI-UTC offset, especially if the machine was turned off for a longer period of time, so why not initialize the PHC from the system time? The error is much smaller than billions of seconds.
On Mon, Jan 31, 2022 at 11:21:08AM +0100, Miroslav Lichvar wrote: > I tried to find the discussion around this decision, but failed. Do > you have a link? I'll dig it up for you. > To me, it seems very strange to start the PHC at 0. It makes the > initial clock correction unnecessarily larger by ~7 orders of > magnitude. The system clock is initialized from the RTC, which can > have an error comparable to the TAI-UTC offset, especially if the > machine was turned off for a longer period of time, so why not > initialize the PHC from the system time? The error is much smaller > than billions of seconds. When the clock reads Jan 1, 1970, then that is clearly wrong, and so a user might suspect that it is uninititalized. When the clock is off by 37 seconds, the user will likely post a vague complaint to linuxptp-users asking why linuxptp doesn't handle leap seconds. I prefer the clarity of the first case. Thanks, Richard
On Mon, Jan 31, 2022 at 08:32:40AM -0800, Richard Cochran wrote: > On Mon, Jan 31, 2022 at 11:21:08AM +0100, Miroslav Lichvar wrote: > > To me, it seems very strange to start the PHC at 0. It makes the > > initial clock correction unnecessarily larger by ~7 orders of > > magnitude. The system clock is initialized from the RTC, which can > > have an error comparable to the TAI-UTC offset, especially if the > > machine was turned off for a longer period of time, so why not > > initialize the PHC from the system time? The error is much smaller > > than billions of seconds. > > When the clock reads Jan 1, 1970, then that is clearly wrong, and so a > user might suspect that it is uninititalized. FWIW, my first thought when I saw the huge offset in ptp4l was that something is horribly broken. > I prefer the clarity of the first case. I'd prefer smaller initial error and consistency. The vast majority of existing drivers seem to initialize the clock at current system time. Drivers starting at 0 now create confusion. If this is the right way, shouldn't be all existing drivers patched to follow that?
On Mon, Jan 31, 2022 at 11:21:08AM +0100, Miroslav Lichvar wrote: > On Thu, Jan 27, 2022 at 02:01:16PM -0800, Richard Cochran wrote: > > On Thu, Jan 27, 2022 at 12:45:36PM +0100, Miroslav Lichvar wrote: > > > When a virtual clock is being created, initialize the timecounter to the > > > current system time instead of the Unix epoch to avoid very large steps > > > when the clock will be synchronized. > > > > I think we agreed that, going forward, new PHC drivers should start at > > zero (1970) instead of TAI - 37. > > I tried to find the discussion around this decision, but failed. Do > you have a link? Here is one of the discussions. It didn't lay down a law or anything, but my arguments still stand. https://lore.kernel.org/all/20180815175040.3736548-1-arnd@arndb.de/ I've been pushing back on new drivers that use ktime_get isntead of zero, but some still sneak through, now and again. > To me, it seems very strange to start the PHC at 0. It makes the > initial clock correction unnecessarily larger by ~7 orders of > magnitude. The system clock is initialized from the RTC, which can > have an error comparable to the TAI-UTC offset, especially if the > machine was turned off for a longer period of time, so why not > initialize the PHC from the system time? The error is much smaller > than billions of seconds. Who cares? The size of the error is irrelevant, because the initial offset is wrong, and the action of the PTP will correct it, whether large or small. Thanks, Richard
On Tue, Feb 01, 2022 at 09:42:07AM +0100, Miroslav Lichvar wrote: > On Mon, Jan 31, 2022 at 08:32:40AM -0800, Richard Cochran wrote: > > On Mon, Jan 31, 2022 at 11:21:08AM +0100, Miroslav Lichvar wrote: > > > To me, it seems very strange to start the PHC at 0. It makes the > > > initial clock correction unnecessarily larger by ~7 orders of > > > magnitude. The system clock is initialized from the RTC, which can > > > have an error comparable to the TAI-UTC offset, especially if the > > > machine was turned off for a longer period of time, so why not > > > initialize the PHC from the system time? The error is much smaller > > > than billions of seconds. > > > > When the clock reads Jan 1, 1970, then that is clearly wrong, and so a > > user might suspect that it is uninititalized. > > FWIW, my first thought when I saw the huge offset in ptp4l was that > something is horribly broken. Yes, that is my point! Although you may have jumped to conclusions about the root cause, still the zero value got your attention. It is just too easy for people to see the correct date and time (down to the minute) and assume all is okay. > I'd prefer smaller initial error and consistency. The vast majority of > existing drivers seem to initialize the clock at current system time. > Drivers starting at 0 now create confusion. If this is the right way, > shouldn't be all existing drivers patched to follow that? I agree that consistency is good, and I would love to get rid of all that ktime_get usage, but maybe people will argue against it for their beloved driver. Going forward, I'm asking that new drivers start from zero for an "uninitialized" clock. Thanks, Richard
On Tue, Feb 01, 2022 at 11:03:51AM -0800, Richard Cochran wrote: > On Mon, Jan 31, 2022 at 11:21:08AM +0100, Miroslav Lichvar wrote: > > On Thu, Jan 27, 2022 at 02:01:16PM -0800, Richard Cochran wrote: > > > I think we agreed that, going forward, new PHC drivers should start at > > > zero (1970) instead of TAI - 37. > > > > I tried to find the discussion around this decision, but failed. Do > > you have a link? > > Here is one of the discussions. It didn't lay down a law or anything, > but my arguments still stand. > > https://lore.kernel.org/all/20180815175040.3736548-1-arnd@arndb.de/ I don't see strong arguments for either option (0, UTC, TAI) and would prefer consistency among drivers. It would be nice if the drivers called a common function to get the initial time, so it could be changed for all in one place if necessary. Anyway, I'll leave this for another time and drop the patch from the series. Thanks,
diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c index cb179a3ea508..5a24a5128013 100644 --- a/drivers/ptp/ptp_vclock.c +++ b/drivers/ptp/ptp_vclock.c @@ -187,7 +187,8 @@ struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock) return NULL; } - timecounter_init(&vclock->tc, &vclock->cc, 0); + timecounter_init(&vclock->tc, &vclock->cc, + ktime_to_ns(ktime_get_real())); ptp_schedule_worker(vclock->clock, PTP_VCLOCK_REFRESH_INTERVAL); return vclock;
When a virtual clock is being created, initialize the timecounter to the current system time instead of the Unix epoch to avoid very large steps when the clock will be synchronized. Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com> Cc: Yangbo Lu <yangbo.lu@nxp.com> Cc: Richard Cochran <richardcochran@gmail.com> --- drivers/ptp/ptp_vclock.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)