Re: kern/57512: clock_gettime(CLOCK_THREAD_CPUTIME_ID) sometimes goes backwards

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

The following reply was made to PR kern/57512; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: conorh%sdf.org@localhost
Cc: gnats-bugs%NetBSD.org@localhost
Subject: Re: kern/57512: clock_gettime(CLOCK_THREAD_CPUTIME_ID) sometimes goes backwards
Date: Sat, 8 Jul 2023 01:31:03 +0000

 This is a multi-part message in MIME format.
 --=_17Wymd7lPaKOmlAiVMiz9N/EY5eA0fm1
 
 Can you please try the attached patch?
 
 --=_17Wymd7lPaKOmlAiVMiz9N/EY5eA0fm1
 Content-Type: text/plain; charset="ISO-8859-1"; name="clockcputimeid"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="clockcputimeid.patch"
 
 From 881ce53d36439f2dc9a4cc3be389095344d42501 Mon Sep 17 00:00:00 2001
 From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
 Date: Sat, 8 Jul 2023 01:29:28 +0000
 Subject: [PATCH] WIP: clock_gettime(2): Fix CLOCK_PROCESS/THREAD_CPUTIME_ID.
 
 Use same calculation as getrusage, not some ad-hoc arithmetic of
 internal scheduler parameters that are periodically rewound.
 
 XXX This creates copypasta of calcru for lwps -- should factor it out
 into a calcrulwp or something to avoid this.
 ---
  sys/kern/subr_time.c | 67 ++++++++++++++++++++++----------------------
  1 file changed, 33 insertions(+), 34 deletions(-)
 
 diff --git a/sys/kern/subr_time.c b/sys/kern/subr_time.c
 index eaa8312dd8fb..cad7fa26e998 100644
 --- a/sys/kern/subr_time.c
 +++ b/sys/kern/subr_time.c
 @@ -45,12 +45,6 @@ __KERNEL_RCSID(0, "$NetBSD: subr_time.c,v 1.37 2023/04/2=
 9 03:36:55 isaki Exp $")
  #include <sys/timetc.h>
  #include <sys/intr.h>
 =20
 -#ifdef DEBUG_STICKS
 -#define DPRINTF(a) uprintf a
 -#else
 -#define DPRINTF(a)=20
 -#endif
 -
  /*
   * Compute number of hz until specified time.  Used to compute second
   * argument to callout_reset() from an absolute time.
 @@ -248,32 +242,16 @@ clock_timeleft(clockid_t clockid, struct timespec *ts=
 , struct timespec *sleepts)
  	*sleepts =3D sleptts;
  }
 =20
 -static void
 -ticks2ts(uint64_t ticks, struct timespec *ts)
 -{
 -	ts->tv_sec =3D ticks / hz;
 -	uint64_t sticks =3D ticks - ts->tv_sec * hz;
 -	if (sticks > BINTIME_SCALE_MS)	/* floor(2^64 / 1000) */
 -		ts->tv_nsec =3D sticks / hz * 1000000000LL;
 -	else if (sticks > BINTIME_SCALE_US)	/* floor(2^64 / 1000000) */
 -		ts->tv_nsec =3D sticks * 1000LL / hz * 1000000LL;
 -	else
 -		ts->tv_nsec =3D sticks * 1000000000LL / hz;
 -	DPRINTF(("%s: %ju/%ju -> %ju.%ju\n", __func__,
 -	    (uintmax_t)ticks, (uintmax_t)sticks,
 -	    (uintmax_t)ts->tv_sec, (uintmax_t)ts->tv_nsec));
 -}
 -
  int
  clock_gettime1(clockid_t clock_id, struct timespec *ts)
  {
  	int error;
 -	uint64_t ticks;
  	struct proc *p;
 =20
  #define CPUCLOCK_ID_MASK (~(CLOCK_THREAD_CPUTIME_ID|CLOCK_PROCESS_CPUTIME_=
 ID))
  	if (clock_id & CLOCK_PROCESS_CPUTIME_ID) {
  		pid_t pid =3D clock_id & CPUCLOCK_ID_MASK;
 +		struct timeval cputime;
 =20
  		mutex_enter(&proc_lock);
  		p =3D pid =3D=3D 0 ? curproc : proc_find(pid);
 @@ -281,10 +259,10 @@ clock_gettime1(clockid_t clock_id, struct timespec *t=
 s)
  			mutex_exit(&proc_lock);
  			return ESRCH;
  		}
 -		ticks =3D p->p_uticks + p->p_sticks + p->p_iticks;
 -		DPRINTF(("%s: u=3D%ju, s=3D%ju, i=3D%ju\n", __func__,
 -		    (uintmax_t)p->p_uticks, (uintmax_t)p->p_sticks,
 -		    (uintmax_t)p->p_iticks));
 +		mutex_enter(p->p_lock);
 +		calcru(p, /*usertime*/NULL, /*systime*/NULL, /*intrtime*/NULL,
 +		    &cputime);
 +		mutex_exit(p->p_lock);
  		mutex_exit(&proc_lock);
 =20
  		// XXX: Perhaps create a special kauth type
 @@ -293,9 +271,14 @@ clock_gettime1(clockid_t clock_id, struct timespec *ts)
  		    KAUTH_ARG(KAUTH_REQ_PROCESS_CANSEE_ENTRY), NULL, NULL);
  		if (error)
  			return error;
 +
 +		TIMEVAL_TO_TIMESPEC(&cputime, ts);
 +		return 0;
  	} else if (clock_id & CLOCK_THREAD_CPUTIME_ID) {
  		struct lwp *l;
  		lwpid_t lid =3D clock_id & CPUCLOCK_ID_MASK;
 +		struct bintime tm =3D {0, 0};
 +
  		p =3D curproc;
  		mutex_enter(p->p_lock);
  		l =3D lid =3D=3D 0 ? curlwp : lwp_find(p, lid);
 @@ -303,15 +286,31 @@ clock_gettime1(clockid_t clock_id, struct timespec *t=
 s)
  			mutex_exit(p->p_lock);
  			return ESRCH;
  		}
 -		ticks =3D l->l_rticksum + l->l_slpticksum;
 -		DPRINTF(("%s: r=3D%ju, s=3D%ju\n", __func__,
 -		    (uintmax_t)l->l_rticksum, (uintmax_t)l->l_slpticksum));
 +
 +		/* XXX begin copypasta from calcru */
 +		lwp_lock(l);
 +		bintime_add(&tm, &l->l_rtime);
 +		if ((l->l_pflag & LP_RUNNING) !=3D 0 &&
 +		    (l->l_pflag & (LP_INTR | LP_TIMEINTR)) !=3D LP_INTR) {
 +			struct bintime diff;
 +			/*
 +			 * Adjust for the current time slice.  This is
 +			 * actually fairly important since the error
 +			 * here is on the order of a time quantum,
 +			 * which is much greater than the sampling
 +			 * error.
 +			 */
 +			binuptime(&diff);
 +			membar_consumer(); /* for softint_dispatch() */
 +			bintime_sub(&diff, &l->l_stime);
 +			bintime_add(&tm, &diff);
 +		}
 +		lwp_unlock(l);
 +		/* XXX end copypasta from calcru */
 +
  		mutex_exit(p->p_lock);
 -	} else
 -		ticks =3D (uint64_t)-1;
 =20
 -	if (ticks !=3D (uint64_t)-1) {
 -		ticks2ts(ticks, ts);
 +		bintime2timespec(&tm, ts);
  		return 0;
  	}
 =20
 
 --=_17Wymd7lPaKOmlAiVMiz9N/EY5eA0fm1--