Created attachment 232127 [details] patch for 10.4 - 14-CURRENT ====== kern_tc.c/cputick2usec() ====== > uint64_t cputick2usec(uint64_t tick) { > if (tick > 18446744073709551LL) /* floor(2^64 / 1000) */ > return (tick / (cpu_tickrate() / 1000000LL)); > else if (tick > 18446744073709LL) /* floor(2^64 / 1000000) */ > return ((tick * 1000LL) / (cpu_tickrate() / 1000LL)); > else > return ((tick * 1000000LL) / cpu_tickrate()); > } There is huge precision loss from dividing cpu_tickrate() by 1000 or 1000000 (up to 0.025% in the second case for 4GHz CPU). Worse, it will result in useconds step after reaching these two thresholds. Let's see: CPU 3999999999 Hz tick = 18446744073709 (real time 1:16:51.686019) usec = 18446744073709*1000000/3999999999 = 4611686019 = 1:16:51.686019 next usec = 18446744073710*1000/3999999 = 4611687171 = 1:16:51.687171 diff = 1152 usec (not so big, and such a step may be caused by a lot of sources anytime, but it is a fake step anyway) tick = 18446744073709551 (real time 53D 9:01:26.019580) usec = 18446744073709551*1000/3999999 = 4611687171349 = 53D 9:01:27.171349 (note already 1 sec imprecise) next usec = 18446744073709552/3999 = 4612839228234 = 53D 9:20:39.228234 diff = 19 minutes! ====== kern_time.c/cputick2timespec() ====== (it is used for clock_gettime() for querying process or thread consumed cpu time) Uses cputick2usec() and then needlessly converting usec to nsec, obviously losing precision even with fixed cputick2usec(). ====== kern_time.c/kern_clock_getres() ====== Uses some weird (anyway wrong) formula for getting cputick resolution. ------- patch attached the patch was done for 12.3, but may be applied succesfully to any version from 10.4 to CURRENT
Adding phk and davidxu to the CC list as they worked on the code in question and could have some opinion on the topic.
In terms of efficiency, I suspect the if() hurts more than the extra divides, so maybe just get rid of the if() and always take the "expensive" path ?
(In reply to Poul-Henning Kamp from comment #2) May be, and the code will look cleaner.
Created attachment 232178 [details] updated patch
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=bb53dd56c30c6360fc82be762ed98b0af6b9f69f commit bb53dd56c30c6360fc82be762ed98b0af6b9f69f Author: firk <firk@cantconnect.ru> AuthorDate: 2022-03-21 13:33:11 +0000 Commit: George V. Neville-Neil <gnn@FreeBSD.org> CommitDate: 2022-03-21 13:33:46 +0000 kern_tc.c/cputick2usec() (which is used to calculate cputime from cpu ticks) has some imprecision and, worse, huge timestep (about 20 minutes on 4GHz CPU) near 53.4 days of elapsed time. kern_time.c/cputick2timespec() (it is used for clock_gettime() for querying process or thread consumed cpu time) Uses cputick2usec() and then needlessly converting usec to nsec, obviously losing precision even with fixed cputick2usec(). kern_time.c/kern_clock_getres() uses some weird (anyway wrong) formula for getting cputick resolution. PR: 262215 Reviewed by: gnn Differential Revision: https://reviews.freebsd.org/D34558 lib/libkvm/kvm_proc.c | 9 ++------- sys/kern/kern_tc.c | 12 +++--------- sys/kern/kern_time.c | 12 +++++------- 3 files changed, 10 insertions(+), 23 deletions(-)