Page MenuHomeFreeBSD

Fix imprecisions and timestep in cputime counters
ClosedPublic

Authored by firk_cantconnect.ru on Mar 14 2022, 11:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 8, 4:59 AM
Unknown Object (File)
Fri, Dec 6, 2:19 PM
Unknown Object (File)
Nov 22 2024, 5:29 AM
Unknown Object (File)
Nov 17 2024, 8:54 PM
Unknown Object (File)
Nov 9 2024, 3:34 PM
Unknown Object (File)
Oct 14 2024, 8:53 PM
Unknown Object (File)
Oct 11 2024, 9:20 AM
Unknown Object (File)
Oct 11 2024, 7:28 AM
Subscribers

Details

Summary

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

Test Plan

cputick2usec() imprecision/timestep could be seen in KERN_PROC_PROC sysctl (struct kinfo_proc field ki_runtime) and in CLOCK_PROCESS_CPUTIME_ID/CLOCK_THREAD_CPUTIME_ID clocks

This program could show the timestep, but it is very time-consuming.
On 4GHz CPU, first timestep will be about 1ms at 76 minutes of consumed cputime, but 1ms step is hard to see because there is many other reasons for such lag. Second step of about 20 minutes will be at 53.4 days cputime.

#include <stdio.h>
#include <time.h>

static struct timespec ts, ts2;

static long tsdiff(void) {
  long r;
  r = ts2.tv_sec-ts.tv_sec;
  r *= 1000000000;
  r += ts2.tv_nsec;
  r -= ts.tv_nsec;
  return r;
}

int main(void) {
  long d, md;
  md = 0;
  clock_gettime(CLOCK_PROCESS_CPUTIME_ID,&ts);
  while(1) {
    clock_gettime(CLOCK_PROCESS_CPUTIME_ID,&ts2);
    d = tsdiff();
    if(md<d) md=d;
    printf("\r%u.%09us diff=%09luns maxdiff=%09luns",
      (unsigned)ts.tv_sec,(unsigned)ts.tv_nsec,d,md);
    fflush(stdout);
    ts = ts2;
  }
}

cputick2timespec() imprecision shown also by the above program: all values will have three trailing zeroes.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

gnn added a subscriber: gnn.

Is it possible to find out how this was tested?

Is it possible to find out how this was tested?

I added a test plan, but you have to wait for about 53 days of CPU time to see the result.

Multithreading can be used to reduce this period, 10 cpu-consuming threads (on 10 CPU cores) = 5 days, but it still too long.

I didn't done the test by myself, but this is trivial integer arithmetic.

For some reason phk@ can't currently log in, so he emailed me his comments. I paste them here, verbatim:

In terms of math, I think the proposed patch checks out.

The original math was shaped by the fact that 64/64 divides were
very expensive (SW-assisted), and because GCC 2.6 insisted on not
optimzing / and % in the same expression to a single call. 64/32
was considerably cheaper. (That could still be a concern on (tiny)
32 bit platforms, but hey...)

The "0ns->1000ns" kern_thread_cputime() detail:

I tried something like the proposed "+1" rounding, but several
utilities spotted the ns based API, converted from ns to
us (to avoid rewriting all their internal math?) and divided by zero.

A ports-build will not tell if that is still a concern, only running
the buggy programs will trigger it.

Poul-Henning

As for 64/64 divides on 32bit platform: in fact, (nearly) all 32-bit CPUs have a 32-bit tickrate (<4.2GHz), and modern compilers/libraries seems to use 64/32 math in runtime for such a case in 32-bit mode (__udivdi3 function).

The +1 rounding is in kern_clock_getres(), not in kern_thread_cputime(). I'm not sure how that value may be used as a denominator, rounded to us or not. Anyway, if the reason is to avoid zero when rounding to us, it should be if(tv_nsec<1000) tv_nsec=1000;.

Found same function in lib/libkvm/kvm_proc.c, patch it too.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 21 2022, 1:34 PM
This revision was automatically updated to reflect the committed changes.