From: Stuart Henderson Subject: Re: patch: POSIX interval timers To: Simon Schwartz Cc: tech@openbsd.org Date: Sat, 23 Mar 2024 10:50:21 +0000 On 2024/03/21 13:41, Simon Schwartz wrote: > This patch probably isn't in an acceptable state as it stands now; I'm a total > newbie when it comes to operating systems and I've only recently been > learning how they work and getting familiar with the OpenBSD code. I've > been hacking on a barebones implementation of (POSIX interval timer > syscalls)[1] and I wanted to see what people thought so far. > > I'm pretty sure there's at least *something* I've completely screwed up, > so posting here to hopefully get a few pointers :-) > > If people here think this is a worthwhile thing I can keep hacking on it > and probably get it into a state where the existing getitimer/setitimer > code can be removed and reimplemented on top of this new API. > > [1]: https://pubs.opengroup.org/onlinepubs/9699919799/ > > --- > diff --git sys/kern/init_sysent.c sys/kern/init_sysent.c > index b64c4aa01..738c2048d 100644 > --- sys/kern/init_sysent.c > +++ sys/kern/init_sysent.c > @@ -534,14 +534,14 @@ const struct sysent sysent[] = { > sys_nosys }, /* 233 = obsolete t32_clock_settime */ This diff is mangled (at least tabs/spaces issues) and doesn't apply with patch. Lack of posix timers is a problem for a few things in ports, having an implementation would be very welcome. > { 0, 0, 0, > sys_nosys }, /* 234 = obsolete t32_clock_getres */ > - { 0, 0, 0, > - sys_nosys }, /* 235 = unimplemented timer_create */ > - { 0, 0, 0, > - sys_nosys }, /* 236 = unimplemented timer_delete */ > - { 0, 0, 0, > - sys_nosys }, /* 237 = unimplemented timer_settime */ > - { 0, 0, 0, > - sys_nosys }, /* 238 = unimplemented timer_gettime */ > + { 3, s(struct sys_timer_create_args), 0, > + sys_timer_create }, /* 235 = timer_create */ > + { 1, s(struct sys_timer_delete_args), 0, > + sys_timer_delete }, /* 236 = timer_delete */ > + { 4, s(struct sys_timer_settime_args), 0, > + sys_timer_settime }, /* 237 = timer_settime */ > + { 2, s(struct sys_timer_gettime_args), 0, > + sys_timer_gettime }, /* 238 = timer_gettime */ > { 0, 0, 0, > sys_nosys }, /* 239 = unimplemented timer_getoverrun */ > { 0, 0, 0, > diff --git sys/kern/kern_exit.c sys/kern/kern_exit.c > index ce6aea2aa..654193b44 100644 > --- sys/kern/kern_exit.c > +++ sys/kern/kern_exit.c > @@ -196,6 +196,7 @@ exit1(struct proc *p, int xexit, int xsig, int flags) > /* close open files and release open-file table */ > fdfree(p); > > + cancel_all_ptimers(); > cancel_all_itimers(); > > timeout_del(&pr->ps_rucheck_to); > diff --git sys/kern/kern_pledge.c sys/kern/kern_pledge.c > index 2099db42f..3d0bf8b47 100644 > --- sys/kern/kern_pledge.c > +++ sys/kern/kern_pledge.c > @@ -201,6 +201,11 @@ const uint64_t pledge_syscalls[SYS_MAXSYSCALL] = { > [SYS_sigpending] = PLEDGE_STDIO, > [SYS_getitimer] = PLEDGE_STDIO, > [SYS_setitimer] = PLEDGE_STDIO, > + /* Can also create threads; tfork is PLEDGE_STDIO too */ > + [SYS_timer_create] = PLEDGE_STDIO, > + [SYS_timer_delete] = PLEDGE_STDIO, > + [SYS_timer_settime] = PLEDGE_STDIO, > + [SYS_timer_gettime] = PLEDGE_STDIO, > > /* > * To support event driven programming. > diff --git sys/kern/kern_time.c sys/kern/kern_time.c > index 1687ee61d..064021bb9 100644 > --- sys/kern/kern_time.c > +++ sys/kern/kern_time.c > @@ -496,6 +496,310 @@ out: > } > > > +struct mutex ptimer_mtx = MUTEX_INITIALIZER(IPL_CLOCK); > + > +static void > +ptmr_timeout_func(void *arg) > +{ > + struct ptimer *ptmr = arg; > + struct timespec next; > + > + mtx_enter(&ptimer_mtx); > + > + switch (ptmr->ev.sigev_notify) { > + case SIGEV_NONE: > + /* No action */ > + break; > + case SIGEV_SIGNAL: > + // TODO: When a timer for which a signal is still pending expires, > + // no signal shall be queued, and a timer overrun shall occur. > + // > + // TODO: The sigev_signo member specifies the signal to be > + // generated. The sigev_value member is the application-defined > + // value to be passed to the signal-catching function at the time > + // of the signal delivery or to be returned at signal acceptance > + // as the si_value member of the siginfo_t structure. > + psignal(ptmr->proc, ptmr->ev.sigev_signo); > + break; > + case SIGEV_THREAD: > + /* TODO: SIGEV_THREAD */ > + break; > + } > + > + timespecclear(&ptmr->spec.it_value); > + if (timespecisset(&ptmr->spec.it_interval)) { > + nanouptime(&next); > + timespecadd(&next, &ptmr->spec.it_interval, &next); > + timeout_abs_ts(&ptmr->to, &next); > + } > + > + mtx_leave(&ptimer_mtx); > +} > + > +int > +sys_timer_create(struct proc *p, void *v, register_t *retval) > +{ > + struct sys_timer_create_args /* { > + syscallarg(clockid_t) clockid; > + syscallarg(struct sigevent *) evp; > + syscallarg(timer_t *) timerid; > + } */ *uap = v; > + struct sigevent *ev; > + timer_t chosenid; > + struct process *pr; > + struct ptimer *ptmr; > + int error; > + > + error = 0; > + pr = p->p_p; > + > + mtx_enter(&ptimer_mtx); > + > + /* allocate timer id */ > + for (chosenid = 0; pr->ps_ptimers[chosenid].occupied; chosenid++) { > + if (chosenid == nitems(pr->ps_ptimers) - 1) { > + error = EAGAIN; > + goto out_leave_mtx; > + } > + } > + ptmr = &pr->ps_ptimers[chosenid]; > + ptmr->occupied = 1; > + timespecclear(&ptmr->spec.it_value); > + timespecclear(&ptmr->spec.it_interval); > + /* TODO: validate clock */ > + ptmr->clock = SCARG(uap, clockid); > + ptmr->proc = p; > + ev = &ptmr->ev; > + timeout_set_flags(&ptmr->to, ptmr_timeout_func, ptmr, KCLOCK_UPTIME, 0); > + > + /* POSIX says: > + * > + * If the evp argument is NULL, the effect is > + * as if the evp argument pointed to a sigevent structure > + * with the sigev_notify member having the value SIGEV_SIGNAL, > + * the sigev_signo having a default signal number, and the > + * sigev_value member having the value of the timer ID. > + */ > + if (SCARG(uap, evp) == NULL) { > + ev->sigev_notify = SIGEV_SIGNAL; > + /* > + * XXX: POSIX specifies to use "a default" signal number. > + * > + * NetBSD defaults to SIGALRM for CLOCK_REALTIME/CLOCK_MONOTONIC, > + * SIGVTALRM for CLOCK_VIRTUAL, and SIGPROF for CLOCK_PROF. > + * > + * Linux's default is always SIGALRM. > + */ > + ev->sigev_signo = SIGALRM; > + ev->sigev_value.sival_int = chosenid; > + } else { > + if ((error = copyin(SCARG(uap, evp), ev, sizeof(*ev)))) > + goto err_release_timer; > + > + /* ensure valid notification type */ > + if (ev->sigev_notify < SIGEV_NONE || ev->sigev_notify > SIGEV_THREAD) { > + error = EINVAL; > + goto err_release_timer; > + } > + > + /* ensure signo is valid if SIGEV_SIGNAL */ > + if (ev->sigev_notify == SIGEV_SIGNAL && > + (ev->sigev_signo <= 0 || ev->sigev_signo >= NSIG)) { > + error = EINVAL; > + goto err_release_timer; > + } > + > + /* TODO: SIGEV_THREAD */ > + if (ev->sigev_notify == SIGEV_THREAD) { > + error = ENOTSUP; > + goto err_release_timer; > + } > + } > + > + if ((error = copyout(&chosenid, SCARG(uap, timerid), sizeof(chosenid)))) > + goto err_release_timer; > + > + > + goto out_leave_mtx; > +err_release_timer: > + ptmr->occupied = 0; > +out_leave_mtx: > + mtx_leave(&ptimer_mtx); > + return error; > +} > + > +/* > + * Caller must hold ptimer_mtx. > + * Set the POSIX interval timer specified by `timerid` with > + * the time given by `spec`. > + * If `oldspec` is not NULL, it is filled with the previous > + * current itimerspec before it is changed to the new spec. > + * If `spec` is NULL, don't change anything and just return > + * the current spec via `oldspec`. > + */ > +static int > +timer_settime(timer_t timerid, int flags, struct itimerspec *spec, > + struct itimerspec *oldspec) > +{ > + struct process *pr; > + struct ptimer *ptmr; > + struct timespec now; > + struct timespec next; > + > + MUTEX_ASSERT_LOCKED(&ptimer_mtx); > + pr = curproc->p_p; > + > + if (timerid >= nitems(pr->ps_ptimers) || !pr->ps_ptimers[timerid].occupied) > + return EINVAL; > + ptmr = &pr->ps_ptimers[timerid]; > + > + if (oldspec != NULL) > + memcpy(oldspec, &ptmr->spec, sizeof(*oldspec)); > + if (spec == NULL) > + return 0; > + > + if (timespecisset(&ptmr->spec.it_value) || > timespecisset(&ptmr->spec.it_interval)) > + timeout_del(&ptmr->to); > + > + memcpy(&ptmr->spec, spec, sizeof(ptmr->spec)); > + if (!timespecisset(&ptmr->spec.it_value) && > !timespecisset(&ptmr->spec.it_interval)) > + return 0; > + > + nanouptime(&now); > + if (!(flags & TIME_ABSTIME) && timespecisset(&spec->it_value)) > + timespecadd(&spec->it_value, &now, &spec->it_value); > + > + if (ptmr->clock == CLOCK_REALTIME) { > + if (timespecisset(&spec->it_value)) { > + timeout_abs_ts(&ptmr->to, &spec->it_value); > + } else { > + next = now; > + timespecadd(&next, &spec->it_interval, &next); > + timeout_abs_ts(&ptmr->to, &next); > + } > + } else { > + /* TODO: handle other clocks */ > + return ENOTSUP; > + } > + > + return 0; > +} > + > +int > +sys_timer_settime(struct proc *p, void *v, register_t *retval) > +{ > + struct sys_timer_settime_args /* { > + syscallarg(timer_t) timerid; > + syscallarg(int) flags; > + syscallarg(const struct itimerspec *) value; > + syscallarg(struct itimerspec *) ovalue; > + } */ *uap = v; > + timer_t timerid; > + int flags; > + struct itimerspec newspec; > + struct itimerspec oldspec; > + int error = 0; > + > + flags = SCARG(uap, flags); > + if ((error = copyin(SCARG(uap, value), &newspec, sizeof(newspec)))) > + return error; > + timerid = SCARG(uap, timerid); > + > + mtx_enter(&ptimer_mtx); > + if ((error = timer_settime(timerid, flags, &newspec, &oldspec))) > + return error; > + mtx_leave(&ptimer_mtx); > + > + if (SCARG(uap, ovalue) != NULL) { > + if ((error = copyout(&oldspec, SCARG(uap, ovalue), sizeof(oldspec)))) > + return error; > + } > + > + return error; > +} > + > +int > +sys_timer_gettime(struct proc *p, void *v, register_t *retval) > +{ > + struct sys_timer_gettime_args /* { > + syscallarg(timer_t) timerid; > + syscallarg(struct itimerspec *) value; > + } */ *uap = v; > + timer_t timerid; > + struct itimerspec curr_value; > + int error; > + > + timerid = SCARG(uap, timerid); > + mtx_enter(&ptimer_mtx); > + if ((error = timer_settime(timerid, 0, NULL, &curr_value))) > + return error; > + mtx_leave(&ptimer_mtx); > + > + if ((error = copyout(&curr_value, SCARG(uap, value), sizeof(curr_value)))) > + return error; > + > + return 0; > +} > + > +static int > +timer_delete(timer_t timerid) > +{ > + struct process *pr; > + struct itimerspec its; > + int error; > + > + MUTEX_ASSERT_LOCKED(&ptimer_mtx); > + pr = curproc->p_p; > + > + if (timerid >= nitems(pr->ps_ptimers) || !pr->ps_ptimers[timerid].occupied) > + return EINVAL; > + > + timespecclear(&its.it_value); > + timespecclear(&its.it_interval); > + > + if ((error = timer_settime(timerid, 0, &its, NULL))) > + return error; > + > + pr->ps_ptimers[timerid].occupied = 0; > + > + return 0; > +} > + > +int > +sys_timer_delete(struct proc *p, void *v, register_t *retval) > +{ > + struct sys_timer_delete_args /* { > + syscallarg(timer_t) timerid; > + } */ *uap = v; > + timer_t timerid; > + struct process *pr; > + int error = 0; > + > + timerid = SCARG(uap, timerid); > + pr = p->p_p; > + > + mtx_enter(&ptimer_mtx); > + error = timer_delete(timerid); > + mtx_leave(&ptimer_mtx); > + return error; > +} > + > +void > +cancel_all_ptimers(void) > +{ > + timer_t id; > + struct process *pr; > + > + pr = curproc->p_p; > + mtx_enter(&ptimer_mtx); > + > + for (id = 0; id < nitems(pr->ps_ptimers); id++) > + (void)timer_delete(id); > + > + mtx_leave(&ptimer_mtx); > +} > + > + > struct mutex itimer_mtx = MUTEX_INITIALIZER(IPL_CLOCK); > > /* > diff --git sys/kern/syscalls.c sys/kern/syscalls.c > index bb5009391..76b6eea84 100644 > --- sys/kern/syscalls.c > +++ sys/kern/syscalls.c > @@ -277,10 +277,10 @@ const char *const syscallnames[] = { > "#232 (obsolete t32_clock_gettime)", /* 232 = obsolete > t32_clock_gettime */ > "#233 (obsolete t32_clock_settime)", /* 233 = obsolete > t32_clock_settime */ > "#234 (obsolete t32_clock_getres)", /* 234 = obsolete > t32_clock_getres */ > - "#235 (unimplemented timer_create)", /* 235 = > unimplemented timer_create */ > - "#236 (unimplemented timer_delete)", /* 236 = > unimplemented timer_delete */ > - "#237 (unimplemented timer_settime)", /* 237 = > unimplemented timer_settime */ > - "#238 (unimplemented timer_gettime)", /* 238 = > unimplemented timer_gettime */ > + "timer_create", /* 235 = timer_create */ > + "timer_delete", /* 236 = timer_delete */ > + "timer_settime", /* 237 = timer_settime */ > + "timer_gettime", /* 238 = timer_gettime */ > "#239 (unimplemented timer_getoverrun)", /* 239 = > unimplemented timer_getoverrun */ > "#240 (obsolete t32_nanosleep)", /* 240 = obsolete t32_nanosleep */ > "#241 (unimplemented)", /* 241 = unimplemented */ > diff --git sys/kern/syscalls.master sys/kern/syscalls.master > index 2ed318508..d881f39db 100644 > --- sys/kern/syscalls.master > +++ sys/kern/syscalls.master > @@ -422,10 +422,12 @@ > 232 OBSOL t32_clock_gettime > 233 OBSOL t32_clock_settime > 234 OBSOL t32_clock_getres > -235 UNIMPL timer_create > -236 UNIMPL timer_delete > -237 UNIMPL timer_settime > -238 UNIMPL timer_gettime > +235 STD { int sys_timer_create(clockid_t clockid, struct > sigevent *evp, \ > + timer_t *timerid); } > +236 STD { int sys_timer_delete(timer_t timerid); } > +237 STD { int sys_timer_settime(timer_t timerid, int flags, \ > + const struct itimerspec *value, struct itimerspec *ovalue); } > +238 STD { int sys_timer_gettime(timer_t timerid, struct > itimerspec *value); } > 239 UNIMPL timer_getoverrun > ; > ; System calls 240-249 are reserved for other IEEE Std1003.1b syscalls > diff --git sys/sys/proc.h sys/sys/proc.h > index 6ed699d9b..cfaebdd19 100644 > --- sys/sys/proc.h > +++ sys/sys/proc.h > @@ -50,6 +50,7 @@ > #include /* For struct rusage */ > #include /* For struct rwlock */ > #include /* For struct sigio */ > +#include /* For struct sigevent */ > > #ifdef _KERNEL > #include > @@ -127,6 +128,7 @@ struct unveil; > * R rlimit_lock > * S scheduler lock > * T itimer_mtx > + * U ptimer_mtx > */ > struct process { > /* > @@ -197,6 +199,15 @@ struct process { > time_t ps_nextxcpu; /* when to send next SIGXCPU, */ > /* in seconds of process runtime */ > > + struct ptimer { /* [U] POSIX interval timers */ > + struct timeout to; /* timeout; in use if to_set == 1 */ > + struct sigevent ev; /* event notification specifier */ > + struct itimerspec spec; /* timeout specification */ > + struct proc *proc; /* pointer to this proc */ > + clockid_t clock; /* clock to timeout with */ > + int occupied:1; /* is there currently a timer in this slot? */ > + } ps_ptimers[32]; > + > u_int64_t ps_wxcounter; > > struct unveil *ps_uvpaths; /* unveil vnodes and names */ > diff --git sys/sys/signal.h sys/sys/signal.h > index dcb681044..8dba7bf1c 100644 > --- sys/sys/signal.h > +++ sys/sys/signal.h > @@ -140,6 +140,29 @@ struct sigaction { > #define SIG_BLOCK 1 /* block specified signal set */ > #define SIG_UNBLOCK 2 /* unblock specified signal set */ > #define SIG_SETMASK 3 /* set specified signal set */ > + > +/* > + * Notification event specifier for timer_create. > + */ > +struct sigevent { > + int sigev_notify; /* notification type */ > + int sigev_signo; /* signal number */ > + union sigval sigev_value; /* signal value */ > + void (*sigev_notify_function)(union sigval); /* notification function */ > + /* > + * XXX: not sure what to do here, pthread_attr_t is defined in > + * include/pthread.h, but it doesn't seem like thats supposed to > + * be included in any kernel headers. > + */ > + //pthread_attr_t *sigev_notify_attributes; /* notification attributes */ > +}; > + > +/* > + * Values for sigev_notify: > + */ > +#define SIGEV_NONE 0 /* no notification */ > +#define SIGEV_SIGNAL 1 /* notify via signal specified in sigev_signo */ > +#define SIGEV_THREAD 2 /* notify by calling sigev_notify_function */ > #endif /* __POSIX_VISIBLE || __XPG_VISIBLE */ > > #if __BSD_VISIBLE > diff --git sys/sys/syscall.h sys/sys/syscall.h > index 7e8396749..e5fa5bb88 100644 > --- sys/sys/syscall.h > +++ sys/sys/syscall.h > @@ -565,6 +565,18 @@ > /* 232 is obsolete t32_clock_gettime */ > /* 233 is obsolete t32_clock_settime */ > /* 234 is obsolete t32_clock_getres */ > +/* syscall: "timer_create" ret: "int" args: "clockid_t" "struct > sigevent *" "timer_t *" */ > +#define SYS_timer_create 235 > + > +/* syscall: "timer_delete" ret: "int" args: "timer_t" */ > +#define SYS_timer_delete 236 > + > +/* syscall: "timer_settime" ret: "int" args: "timer_t" "int" "const > struct itimerspec *" "struct itimerspec *" */ > +#define SYS_timer_settime 237 > + > +/* syscall: "timer_gettime" ret: "int" args: "timer_t" "struct itimerspec *" */ > +#define SYS_timer_gettime 238 > + > /* 240 is obsolete t32_nanosleep */ > /* syscall: "minherit" ret: "int" args: "void *" "size_t" "int" */ > #define SYS_minherit 250 > diff --git sys/sys/syscallargs.h sys/sys/syscallargs.h > index bdf2dac18..cca863ed7 100644 > --- sys/sys/syscallargs.h > +++ sys/sys/syscallargs.h > @@ -923,6 +923,28 @@ struct sys_shmdt_args { > syscallarg(const void *) shmaddr; > }; > > +struct sys_timer_create_args { > + syscallarg(clockid_t) clockid; > + syscallarg(struct sigevent *) evp; > + syscallarg(timer_t *) timerid; > +}; > + > +struct sys_timer_delete_args { > + syscallarg(timer_t) timerid; > +}; > + > +struct sys_timer_settime_args { > + syscallarg(timer_t) timerid; > + syscallarg(int) flags; > + syscallarg(const struct itimerspec *) value; > + syscallarg(struct itimerspec *) ovalue; > +}; > + > +struct sys_timer_gettime_args { > + syscallarg(timer_t) timerid; > + syscallarg(struct itimerspec *) value; > +}; > + > struct sys_minherit_args { > syscallarg(void *) addr; > syscallarg(size_t) len; > @@ -1350,6 +1372,10 @@ int sys_shmat(struct proc *, void *, register_t *); > int sys_shmdt(struct proc *, void *, register_t *); > #else > #endif > +int sys_timer_create(struct proc *, void *, register_t *); > +int sys_timer_delete(struct proc *, void *, register_t *); > +int sys_timer_settime(struct proc *, void *, register_t *); > +int sys_timer_gettime(struct proc *, void *, register_t *); > int sys_minherit(struct proc *, void *, register_t *); > int sys_poll(struct proc *, void *, register_t *); > int sys_issetugid(struct proc *, void *, register_t *); > diff --git sys/sys/time.h sys/sys/time.h > index cef9583c8..33a54e56c 100644 > --- sys/sys/time.h > +++ sys/sys/time.h > @@ -150,6 +150,9 @@ struct itimerval { > struct timeval it_value; /* current value */ > }; > > +/* Flags for timer_settime. */ > +#define TIME_ABSTIME 1 > + > #if __BSD_VISIBLE > /* > * clock information structure for sysctl({CTL_KERN, KERN_CLOCKRATE}) > @@ -333,6 +336,7 @@ int clock_gettime(struct proc *, clockid_t, > struct timespec *); > struct clockintr; > void itimer_update(struct clockintr *, void *, void *); > > +void cancel_all_ptimers(void); > void cancel_all_itimers(void); > int settime(const struct timespec *); > int ratecheck(struct timeval *, const struct timeval *); >