Bug 262215

Summary: Two imprecisions in cputime counters
Product: Base System Reporter: firk
Component: kernAssignee: freebsd-bugs (Nobody) <bugs>
Status: Closed FIXED    
Severity: Affects Only Me CC: arrowd, davidxu, eugen, phk
Priority: ---    
Version: 13.0-RELEASE   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
patch for 10.4 - 14-CURRENT
none
updated patch none

Description firk 2022-02-26 23:11:31 UTC 
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
Comment 1 Eugene Grosbein freebsd_committer freebsd_triage 2022-02-27 20:17:40 UTC 
Adding phk and davidxu to the CC list as they worked on the code in question and could have some opinion on the topic.
Comment 2 Poul-Henning Kamp freebsd_committer freebsd_triage 2022-02-28 07:50:36 UTC 
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 ?
Comment 3 firk 2022-03-01 07:41:20 UTC 
(In reply to Poul-Henning Kamp from comment #2)

May be, and the code will look cleaner.
Comment 4 firk 2022-03-01 07:41:49 UTC 
Created attachment 232178 [details]
updated patch
Comment 5 commit-hook freebsd_committer freebsd_triage 2022-03-21 13:34:45 UTC 
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(-)