On Tue, Sep 01, 2020 at 10:24:11AM -0500, Scott Cheloha wrote:
> On Mon, Aug 17, 2020 at 05:55:34PM -0500, Scott Cheloha wrote:
> >
> > [...]
>
> Two week bump.
>
> In summary:
>
> - Merge the critical sections so that "timer swap" with setitimer(2)
> is atomic.
>
> - To do this, move error-free operations into a common kernel
> subroutine, setitimer(). Now we have one critical section.
>
> - Leave error-prone operations in sys_getitimer() and sys_setitimer().
>
> In order to make the "timer swap" atomic we leave the timer installed
> if the copyout(9) fails. This isn't great, but it is unavoidable
> without permitting copyout(9) from within a mutex. FreeBSD and Linux
> went this direction, too. I would rather leave the timer running and
> have an atomic swap than race the hardclock(9) or the realitexpire()
> timeout.
>
> [...]
6 week bump. Diff attached again. Basically nothing has changed.
We're moving the critical sections from sys_getitimer() and
sys_setitimer() into a new subroutine, setitimer(), and merging those
critical sections.
I'm confident that merging the critical sections is the right
direction to take setitimer(2). It gets us closer to removing the
kernel lock and eliminates a race with hardclock(9).
In the near future we will need to call the setitimer() subroutine
during _exit(2) to safely cancel the ITIMER_REAL timeout so I think
putting the critical section into a subroutine we can call from
outside of kern_time.c is also the right thing to do.
I'm going to wait until after the tree is unlocked to commit this, as
the behavior change in the timer swap case *might* break some software
out there that isn't checking the setitimer(2) return code. It's
unlikely, but it's possible, so I'll wait.
CC: everyone I've ever talked to about setitimer(2).
Index: kern_time.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_time.c,v
retrieving revision 1.140
diff -u -p -r1.140 kern_time.c
--- kern_time.c 12 Aug 2020 15:31:27 -0000 1.140
+++ kern_time.c 25 Sep 2020 23:32:08 -0000
@@ -491,7 +491,7 @@ out:
struct mutex itimer_mtx = MUTEX_INITIALIZER(IPL_CLOCK);
/*
- * Get value of an interval timer. The process virtual and
+ * Get or set value of an interval timer. The process virtual and
* profiling virtual time timers are kept internally in the
* way they are specified externally: in time until they expire.
*
@@ -509,6 +509,62 @@ struct mutex itimer_mtx = MUTEX_INITIALI
* real time timers .it_interval. Rather, we compute the next time in
* absolute time the timer should go off.
*/
+void
+setitimer(int which, const struct itimerval *itv, struct itimerval *olditv)
+{
+ struct itimerspec its, oldits;
+ struct itimerspec *itimer;
+ struct process *pr;
+ int timo;
+
+ KASSERT(which >= ITIMER_REAL && which <= ITIMER_PROF);
+
+ pr = curproc->p_p;
+ itimer = &pr->ps_timer[which];
+
+ if (itv != NULL) {
+ TIMEVAL_TO_TIMESPEC(&itv->it_value, &its.it_value);
+ TIMEVAL_TO_TIMESPEC(&itv->it_interval, &its.it_interval);
+ }
+
+ if (which != ITIMER_REAL)
+ mtx_enter(&itimer_mtx);
+
+ if (olditv != NULL)
+ oldits = *itimer;
+ if (itv != NULL) {
+ if (which == ITIMER_REAL) {
+ struct timespec cts;
+ getnanouptime(&cts);
+ if (timespecisset(&its.it_value)) {
+ timo = tstohz(&its.it_value);
+ timeout_add(&pr->ps_realit_to, timo);
+ timespecadd(&its.it_value, &cts, &its.it_value);
+ } else
+ timeout_del(&pr->ps_realit_to);
+ }
+ *itimer = its;
+ }
+
+ if (which != ITIMER_REAL)
+ mtx_leave(&itimer_mtx);
+
+ if (olditv != NULL) {
+ if (which == ITIMER_REAL && timespecisset(&oldits.it_value)) {
+ struct timespec now;
+ getnanouptime(&now);
+ if (timespeccmp(&oldits.it_value, &now, <))
+ timespecclear(&oldits.it_value);
+ else {
+ timespecsub(&oldits.it_value, &now,
+ &oldits.it_value);
+ }
+ }
+ TIMESPEC_TO_TIMEVAL(&olditv->it_value, &oldits.it_value);
+ TIMESPEC_TO_TIMEVAL(&olditv->it_interval, &oldits.it_interval);
+ }
+}
+
int
sys_getitimer(struct proc *p, void *v, register_t *retval)
{
@@ -516,44 +572,16 @@ sys_getitimer(struct proc *p, void *v, r
syscallarg(int) which;
syscallarg(struct itimerval *) itv;
} */ *uap = v;
- struct itimerspec its;
struct itimerval aitv;
- struct itimerspec *itimer;
int which;
which = SCARG(uap, which);
if (which < ITIMER_REAL || which > ITIMER_PROF)
return (EINVAL);
- itimer = &p->p_p->ps_timer[which];
memset(&aitv, 0, sizeof(aitv));
- if (which != ITIMER_REAL)
- mtx_enter(&itimer_mtx);
- its = *itimer;
- if (which != ITIMER_REAL)
- mtx_leave(&itimer_mtx);
-
- if (which == ITIMER_REAL) {
- struct timespec now;
-
- getnanouptime(&now);
- /*
- * Convert from absolute to relative time in .it_value
- * part of real time timer. If time for real time timer
- * has passed return 0, else return difference between
- * current time and time for the timer to go off.
- */
- if (timespecisset(&its.it_value)) {
- if (timespeccmp(&its.it_value, &now, <))
- timespecclear(&its.it_value);
- else
- timespecsub(&its.it_value, &now,
- &its.it_value);
- }
- }
- TIMESPEC_TO_TIMEVAL(&aitv.it_value, &its.it_value);
- TIMESPEC_TO_TIMEVAL(&aitv.it_interval, &its.it_interval);
+ setitimer(which, NULL, &aitv);
return (copyout(&aitv, SCARG(uap, itv), sizeof (struct itimerval)));
}
@@ -566,14 +594,12 @@ sys_setitimer(struct proc *p, void *v, r
syscallarg(const struct itimerval *) itv;
syscallarg(struct itimerval *) oitv;
} */ *uap = v;
- struct sys_getitimer_args getargs;
- struct itimerspec aits;
struct itimerval aitv;
+ struct itimerval olditv;
const struct itimerval *itvp;
struct itimerval *oitv;
- struct process *pr = p->p_p;
+ struct itimerval *newitvp = NULL, *olditvp = NULL;
int error;
- int timo;
int which;
which = SCARG(uap, which);
@@ -588,36 +614,19 @@ sys_setitimer(struct proc *p, void *v, r
return (error);
if (itimerfix(&aitv.it_value) || itimerfix(&aitv.it_interval))
return (EINVAL);
- TIMEVAL_TO_TIMESPEC(&aitv.it_value, &aits.it_value);
- TIMEVAL_TO_TIMESPEC(&aitv.it_interval, &aits.it_interval);
+ newitvp = &aitv;
}
if (oitv != NULL) {
- SCARG(&getargs, which) = which;
- SCARG(&getargs, itv) = oitv;
- if ((error = sys_getitimer(p, &getargs, retval)))
- return (error);
+ memset(&olditv, 0, sizeof(olditv));
+ olditvp = &olditv;
}
- if (itvp == 0)
+ if (newitvp == NULL && olditvp == NULL)
return (0);
- if (which != ITIMER_REAL)
- mtx_enter(&itimer_mtx);
-
- if (which == ITIMER_REAL) {
- struct timespec cts;
+ setitimer(which, newitvp, olditvp);
- getnanouptime(&cts);
- if (timespecisset(&aits.it_value)) {
- timo = tstohz(&aits.it_value);
- timeout_add(&pr->ps_realit_to, timo);
- timespecadd(&aits.it_value, &cts, &aits.it_value);
- } else
- timeout_del(&pr->ps_realit_to);
- }
- pr->ps_timer[which] = aits;
-
- if (which != ITIMER_REAL)
- mtx_leave(&itimer_mtx);
+ if (SCARG(uap, oitv) != NULL)
+ return copyout(&olditv, SCARG(uap, oitv), sizeof(olditv));
return (0);
}
