> Date: Thu, 27 May 2021 18:29:04 -0500
> From: Scott Cheloha <scottchel...@gmail.com>
Sorry, but does is one of those areas where I'm not very aware how the
interfaces are used by applications. So my default position is:
"don't change it". Especially since these are "legacy" interfaces.
> On Wed, May 19, 2021 at 10:32:55AM -0500, Scott Cheloha wrote:
> > On Wed, May 12, 2021 at 01:15:05PM -0500, Scott Cheloha wrote:
> > >
> > > [...]
> > >
> > > Paul de Weerd mentioned off-list that the initial expiration for an
> > > ITIMER_REAL timer is always at least one tick. I looked into it and
> > > yes, this is the case, because the kernel rounds it_value up to one
> > > tick if it is non-zero.
> > >
> > > After thinking about it a bit I don't think we should do this
> > > rounding. At least, not for the initial expiration.
The manual page explicity says:
"Time values smaller than the resolution of the system clock are
rounded up to this reolution (typically 10 milliseconds)".
which has been there from revision 1.
Note that POSIX defines timer_gettime() and timer_settime(), which we
don't implement. We don't implement these, but the POSIX standard
says in the rationale:
"Practical clocks tick at a finite rate, with rates of 100 hertz and
1000 hertz being common. The inverse of this tick rate is the
clock resolution, also called the clock granularity, which in
either case is expressed as a time duration, being 10 milliseconds
and 1 millisecond respectively for these common rates. The
granularity of practical clocks implies that if one reads a given
clock twice in rapid succession, one may get the same time value
twice; and that timers must wait for the next clock tick after the
theoretical expiration time, to ensure that a timer never returns
too soon. Note also that the granularity of the clock may be
significantly coarser than the resolution of the data format used
to set and get time and interval values. Also note that some
implementations may choose to adjust time and/or interval values to
exactly match the ticks of the underlying clock."
which seems to imply that rounding up is what is desired here as well,
although I presume here the actual resolution of the clock is supposed
to be used. But for timers associated with the
CLOCK_PROCESS_CPUTIME_ID and CLOCK_THREAD_CPUTIME_ID that would be
realstathz, which is still tick-like...
In other words, I'm not convinced...
> > >
> > > [...]
> > >
> > > Currently the rounding is done in itimerfix(), which takes a timeval
> > > pointer as argument. Given that itimerfix() is used nowhere else in
> > > the kernel I think the easiest thing to do here is to rewrite
> > > itimerfix() to take an itimerval pointer as argument and have it do
> > > all input validation and normalization for setitimer(2) in one go:
> > >
> > > - Validate it_value, return EINVAL if not.
> > >
> > > - Validate it_interval, return EINVAL if not.
> > >
> > > - Clear it_interval if it_value is unset.
> > >
> > > - Round it_interval up if necessary.
> > >
> > > The 100 million second upper bound for it_value and it_interval is
> > > arbitrary and will probably change in the future, so I have isolated
> > > that check from the others.
> > >
> > > While we're changing the itimerfix() prototype we may as well pull it
> > > out of sys/time.h. As I said before, it isn't used anywhere else.
> > >
> > > OK?
> >
> > Ping.
>
> 2 week bump.
>
> Index: kern/kern_time.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_time.c,v
> retrieving revision 1.151
> diff -u -p -r1.151 kern_time.c
> --- kern/kern_time.c 23 Dec 2020 20:45:02 -0000 1.151
> +++ kern/kern_time.c 27 May 2021 23:28:20 -0000
> @@ -52,6 +52,8 @@
>
> #include <dev/clock_subr.h>
>
> +int itimerfix(struct itimerval *);
> +
> /*
> * Time of day and interval timer support.
> *
> @@ -628,10 +630,9 @@ sys_setitimer(struct proc *p, void *v, r
> error = copyin(SCARG(uap, itv), &aitv, sizeof(aitv));
> if (error)
> return error;
> - if (itimerfix(&aitv.it_value) || itimerfix(&aitv.it_interval))
> - return EINVAL;
> - if (!timerisset(&aitv.it_value))
> - timerclear(&aitv.it_interval);
> + error = itimerfix(&aitv);
> + if (error)
> + return error;
> newitvp = &aitv;
> }
> if (SCARG(uap, oitv) != NULL) {
> @@ -701,21 +702,34 @@ out:
> }
>
> /*
> - * Check that a proposed value to load into the .it_value or
> - * .it_interval part of an interval timer is acceptable.
> + * Check if the given setitimer(2) input is valid. Clear it_interval
> + * if it_value is unset. Round it_interval up to the minimum interval
> + * if necessary.
> */
> int
> -itimerfix(struct timeval *tv)
> +itimerfix(struct itimerval *itv)
> {
> + struct timeval min_interval = { .tv_sec = 0, .tv_usec = tick };
>
> - if (tv->tv_sec < 0 || tv->tv_sec > 100000000 ||
> - tv->tv_usec < 0 || tv->tv_usec >= 1000000)
> - return (EINVAL);
> + if (itv->it_value.tv_sec < 0 || !timerisvalid(&itv->it_value))
> + return EINVAL;
> + if (itv->it_value.tv_sec > 100000000)
> + return EINVAL;
>
> - if (tv->tv_sec == 0 && tv->tv_usec != 0 && tv->tv_usec < tick)
> - tv->tv_usec = tick;
> + if (itv->it_interval.tv_sec < 0 || !timerisvalid(&itv->it_interval))
> + return EINVAL;
> + if (itv->it_interval.tv_sec > 100000000)
> + return EINVAL;
>
> - return (0);
> + if (!timerisset(&itv->it_value))
> + timerclear(&itv->it_interval);
> +
> + if (timerisset(&itv->it_interval)) {
> + if (timercmp(&itv->it_interval, &min_interval, <))
> + itv->it_interval = min_interval;
> + }
> +
> + return 0;
> }
>
> /*
> Index: sys/time.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/time.h,v
> retrieving revision 1.58
> diff -u -p -r1.58 time.h
> --- sys/time.h 13 Jan 2021 16:28:50 -0000 1.58
> +++ sys/time.h 27 May 2021 23:28:20 -0000
> @@ -307,7 +307,6 @@ struct proc;
> int clock_gettime(struct proc *, clockid_t, struct timespec *);
>
> void cancel_all_itimers(void);
> -int itimerfix(struct timeval *);
> int itimerdecr(struct itimerspec *, long);
> int settime(const struct timespec *);
> int ratecheck(struct timeval *, const struct timeval *);
>
>
