Re: setitimer(2): don't round up it_value
On Thu, May 27, 2021 at 06:29:04PM -0500, Scott Cheloha wrote:
> 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.
> > >
> > > [...]
> > >
> > > 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.
A few comments below.
> 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 - 1.151
> +++ kern/kern_time.c 27 May 2021 23:28:20 -
> @@ -52,6 +52,8 @@
>
> #include
>
> +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), , sizeof(aitv));
> if (error)
> return error;
> - if (itimerfix(_value) || itimerfix(_interval))
> - return EINVAL;
> - if (!timerisset(_value))
> - timerclear(_interval);
> + error = itimerfix();
> + if (error)
> + return error;
> newitvp =
> }
> 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 > 1 ||
> - tv->tv_usec < 0 || tv->tv_usec >= 100)
> - return (EINVAL);
> + if (itv->it_value.tv_sec < 0 || !timerisvalid(>it_value))
> + return EINVAL;
> + if (itv->it_value.tv_sec > 1)
> + return EINVAL;
I would prefer this is written as:
if (itv->it_value.tv_sec < 0 || itv->it_value.tv_sec > 1)
return EINVAL;
if (!timerisvalid(>it_value))
return EINVAL;
At least comparing seconds together and then doing the usec check seems
more logical.
>
> - 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(>it_interval))
> + return EINVAL;
> + if (itv->it_interval.tv_sec > 1)
> + return EINVAL;
Same as above.
> - return (0);
> + if (!timerisset(>it_value))
> + timerclear(>it_interval);
> +
> + if (timerisset(>it_interval)) {
> + if (timercmp(>it_interval, _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.h13 Jan 2021 16:28:50 - 1.58
> +++ sys/time.h27 May 2021 23:28:20 -
> @@ -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
Re: setitimer(2): don't round up it_value
On Fri, May 28, 2021 at 08:15:20AM +0200, Mark Kettenis wrote: > > Date: Thu, 27 May 2021 18:29:04 -0500 > > From: Scott Cheloha > > 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... > I adjusted Paul de Weerds timer.c code to work with all three setitimer clocks. The result is this: Before: ITIMER_REAL timeout 80us min/avg/max/std-dev: 800208/808854.500/811085/3043.725 us timeout 40us min/avg/max/std-dev: 400210/409098.031/410645/2786.929 us timeout 20us min/avg/max/std-dev: 200035/209098.641/212912/2759.900 us timeout 10us min/avg/max/std-dev: 100025/108900.367/110360/3057.488 us timeout 5us min/avg/max/std-dev: 50025/59198.781/61234/2680.975 us timeout 25000us min/avg/max/std-dev: 28997/2.529/31023/200.045 us timeout 12500us min/avg/max/std-dev: 17508/20103.311/27802/896.630 us timeout6250us min/avg/max/std-dev: 10026/18596.189/23655/3354.573 us timeout3125us min/avg/max/std-dev: 10087/18304.020/21226/3679.233 us timeout1562us min/avg/max/std-dev: 10059/19395.580/20615/2335.428 us timeout 781us min/avg/max/std-dev: 10038/19299.439/28400/2792.181 us timeout 390us min/avg/max/std-dev: 10193/19502.061/21132/2122.942 us timeout 195us min/avg/max/std-dev: 10068/19297.430/21386/2517.788 us timeout 97us min/avg/max/std-dev: 10214/19799.789/20804/1372.321 us timeout 48us min/avg/max/std-dev: 10032/19399.801/28896/2668.078 us timeout 24us min/avg/max/std-dev: 10106/19599.900/20936/1930.546 us timeout 12us min/avg/max/std-dev: 10203/19799.570/20952/1379.133 us timeout 6us min/avg/max/std-dev: 10047/19799.711/21452/1406.507 us timeout 3us min/avg/max/std-dev: 10035/18699.750/20984/3237.439 us timeout 1us min/avg/max/std-dev: 10036/19499.779/20631/2148.104 us ITIMER_VIRTUAL timeout 80us min/avg/max/std-dev: 766035/802268.312/86/15187.871 us timeout 40us min/avg/max/std-dev: 372155/400825.844/497840/14646.898 us timeout 20us min/avg/max/std-dev: 182798/200070.953/217841/6382.939 us timeout 10us min/avg/max/std-dev: 85393/100599.352/118195/5079.172 us timeout 5us min/avg/max/std-dev: 32158/49660.000/62153/3548.212 us timeout 25000us min/avg/max/std-dev: 22158/30199.730/39995/2776.258 us timeout 12500us min/avg/max/std-dev: 13231/20539.410/33248/3425.671 us timeout6250us min/avg/max/std-dev: 2534/.680/20004/2506.703 us timeout3125us min/avg/max/std-dev: 3959/10081.800/20003/1933.720 us timeout1562us min/avg/max/std-dev: 2160/9845.760/14616/1819.296 us timeout 781us
Re: setitimer(2): don't round up it_value
On Fri, May 28, 2021 at 08:15:20AM +0200, Mark Kettenis wrote: > > Date: Thu, 27 May 2021 18:29:04 -0500 > > From: Scott Cheloha > > 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. This is a reasonable default stance. The typical usage pattern is to asynchronously do something periodically. Usually the SIGALRM handler sets a flag and then the application clears the flag and runs a subroutine in response. I have never seen an application in the wild, anywhere, use SIGVTALRM or SIGPROF. > > 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. Before that, even. This is documented this way because CSRG BSD on the DEC VAX could only drive a 100hz system clock: https://svnweb.freebsd.org/csrg/lib/libc/sys/getitimer.2?revision=20230=markup#l81 My impression is that we actually inherited the HZ=100 default from the CSRG BSD DEC VAX port because it was good enough for most new platforms and so eventually it became a defacto standard for all new BSD platforms. > Note that POSIX defines timer_gettime() and timer_settime(), which we > don't implement. I want these, but it would require implementing POSIX realtime signals. That implies: - Changing all of our kernel signal stuff. - Some of our signal stuff is MD, so changes on every architecture. - So now we have a pretty big kernel ABI break. - Then we need new stuff in libc. More than just syscall stubs. Probably a major revision bump and an ABI break. - And of course, new stuff in libpthread for the necessary multithreaded signal handling changes. Another major revision. I don't think guenther@ would do all of that work for such rarely used interfaces even if I threatened his life. Maybe for ports, though... > 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. For kclock timeouts the "theoretical resolution" is 1 nanosecond. setitimer(2) uses a timeval, though, so in this case you get 1 microsecond. Regardless, rounding in this case is still wrong. Let me show you with a practical example: 1. Suppose for sake of discussion you have a 100hz kernel with no error. That is, hardclock(9) runs *exactly* every 10ms on the primary CPU. Let's denote each hardclock tick on the kernel timeline with "tN". So The first tick happens at t0. And the second tick happens at t1 = t0 + 10ms. And so on. To start we then have a kernel timeline like this: t0 - - - - - - - - - t1 - - - - - - - - - t2 - - - - - - - - - t3 2. In my program I set a one-shot timer with setitimer(2) with a timeout of 1 microsecond. Signal handling code excluded for brevity: struct itimerval itv; itv.it_value.tv_sec = 0; itv.it_value.tv_usec = 1; timerclear(_interval); setitimer(ITIMER_REAL, , NULL); 3. Let's say the above setitimer(2) call "S" happens between t0 and t1: t0 - - - - S - - - - t1 - - - - - - - - - t2 - - - - - - - - - t3 The timeout is 1 microsecond. So the theoretical Expected expiration "E"
Re: setitimer(2): don't round up it_value
> Date: Thu, 27 May 2021 18:29:04 -0500
> From: Scott Cheloha
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 - 1.151
> +++ kern/kern_time.c 27 May 2021 23:28:20 -
> @@ -52,6 +52,8 @@
>
> #include
>
> +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), , sizeof(aitv));
> if (error)
> return error;
> - if (itimerfix(_value) || itimerfix(_interval))
> - return EINVAL;
> - if (!timerisset(_value))
> - timerclear(_interval);
> + error = itimerfix();
> + if (error)
> + return error;
> newitvp =
> }
> 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 <
Re: setitimer(2): don't round up it_value
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.
> >
> > [...]
> >
> > 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.c23 Dec 2020 20:45:02 - 1.151
+++ kern/kern_time.c27 May 2021 23:28:20 -
@@ -52,6 +52,8 @@
#include
+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), , sizeof(aitv));
if (error)
return error;
- if (itimerfix(_value) || itimerfix(_interval))
- return EINVAL;
- if (!timerisset(_value))
- timerclear(_interval);
+ error = itimerfix();
+ if (error)
+ return error;
newitvp =
}
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 > 1 ||
- tv->tv_usec < 0 || tv->tv_usec >= 100)
- return (EINVAL);
+ if (itv->it_value.tv_sec < 0 || !timerisvalid(>it_value))
+ return EINVAL;
+ if (itv->it_value.tv_sec > 1)
+ 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(>it_interval))
+ return EINVAL;
+ if (itv->it_interval.tv_sec > 1)
+ return EINVAL;
- return (0);
+ if (!timerisset(>it_value))
+ timerclear(>it_interval);
+
+ if (timerisset(>it_interval)) {
+ if (timercmp(>it_interval, _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 - 1.58
+++ sys/time.h 27 May 2021 23:28:20 -
@@ -307,7 +307,6 @@ struct proc;
intclock_gettime(struct proc *, clockid_t, struct timespec *);
void cancel_all_itimers(void);
-intitimerfix(struct timeval *);
intitimerdecr(struct itimerspec *, long);
intsettime(const struct timespec *);
intratecheck(struct timeval *, const struct timeval *);
Re: setitimer(2): don't round up it_value
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.
>
> [...]
>
> 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.
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.c23 Dec 2020 20:45:02 - 1.151
+++ kern/kern_time.c19 May 2021 15:32:26 -
@@ -52,6 +52,8 @@
#include
+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), , sizeof(aitv));
if (error)
return error;
- if (itimerfix(_value) || itimerfix(_interval))
- return EINVAL;
- if (!timerisset(_value))
- timerclear(_interval);
+ error = itimerfix();
+ if (error)
+ return error;
newitvp =
}
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 > 1 ||
- tv->tv_usec < 0 || tv->tv_usec >= 100)
- return (EINVAL);
+ if (itv->it_value.tv_sec < 0 || !timerisvalid(>it_value))
+ return EINVAL;
+ if (itv->it_value.tv_sec > 1)
+ 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(>it_interval))
+ return EINVAL;
+ if (itv->it_interval.tv_sec > 1)
+ return EINVAL;
- return (0);
+ if (!timerisset(>it_value))
+ timerclear(>it_interval);
+
+ if (timerisset(>it_interval)) {
+ if (timercmp(>it_interval, _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 - 1.58
+++ sys/time.h 19 May 2021 15:32:26 -
@@ -307,7 +307,6 @@ struct proc;
intclock_gettime(struct proc *, clockid_t, struct timespec *);
void cancel_all_itimers(void);
-intitimerfix(struct timeval *);
intitimerdecr(struct itimerspec *, long);
intsettime(const struct timespec *);
intratecheck(struct timeval *, const struct timeval *);
Re: setitimer(2): don't round up it_value
Hi Scott,
Thanks for this work! I see significant improvement with my test code
(see below; obviously focussing quite specificially on one thing).
Before your diff (snapshot from a few days ago; OpenBSD 6.9-current
(GENERIC.MP) #1: Mon May 3 11:04:25 MDT 2021) I got:
[weerd@pom] $ ./measure
Running for 100 loops
timeout 80us min/avg/max/std-dev: 800054/809727.250/810115/1609.611 us
timeout 40us min/avg/max/std-dev: 409668/41.406/410362/121.568 us
timeout 20us min/avg/max/std-dev: 209421/209997.953/210553/97.505 us
timeout 10us min/avg/max/std-dev: 109818/109997.078/110167/39.038 us
timeout 5us min/avg/max/std-dev: 59887/59995.539/60108/27.651 us
timeout 25000us min/avg/max/std-dev: 29648/29993.961/30316/50.629 us
timeout 12500us min/avg/max/std-dev: 19910/19994.100/20098/24.265 us
timeout6250us min/avg/max/std-dev: 19932/19994.020/20067/20.760 us
timeout3125us min/avg/max/std-dev: 19806/19993.980/20221/33.121 us
timeout1562us min/avg/max/std-dev: 19519/19993.971/20470/71.215 us
timeout 781us min/avg/max/std-dev: 19778/19993.980/20214/35.454 us
timeout 390us min/avg/max/std-dev: 19784/19994.029/20188/32.626 us
timeout 195us min/avg/max/std-dev: 19891/19994.230/20096/23.237 us
timeout 97us min/avg/max/std-dev: 19938/19994.170/20044/17.468 us
timeout 48us min/avg/max/std-dev: 19876/19994.010/20115/26.334 us
timeout 24us min/avg/max/std-dev: 19535/19994.199/20458/67.628 us
timeout 12us min/avg/max/std-dev: 19941/19994.240/20079/18.478 us
timeout 6us min/avg/max/std-dev: 19949/19994.150/20051/16.916 us
timeout 3us min/avg/max/std-dev: 19880/19994.221/20109/23.377 us
timeout 1us min/avg/max/std-dev: 19940/19994.289/20053/17.575 us
[I should add that these numbers are from an otherwise mostly idle
machine; they improved when my machine was busy building a new kernel]
After upgrading my local snap, cvs-up'ing, patching in your diff and
building + rebooting into a new kernel:
[weerd@pom] $ ./measure
Running for 100 loops
timeout 80us min/avg/max/std-dev: 800041/808010.312/810021/2992.553 us
timeout 40us min/avg/max/std-dev: 401639/402001.281/402373/-nan us
timeout 20us min/avg/max/std-dev: 200959/200997.922/201041/-nan us
timeout 10us min/avg/max/std-dev: 100456/100495.977/100533/37.185 us
timeout 5us min/avg/max/std-dev: 50141/50244.809/50351/30.089 us
timeout 25000us min/avg/max/std-dev: 28917/30144.590/31396/175.702 us
timeout 12500us min/avg/max/std-dev: 20048/20094.420/20143/10.583 us
timeout6250us min/avg/max/std-dev: 10016/10044.260/10084/9.683 us
timeout3125us min/avg/max/std-dev: 9974/10044.060/10140/14.810 us
timeout1562us min/avg/max/std-dev: 10016/10044.190/10078/6.827 us
timeout 781us min/avg/max/std-dev: 9964/10044.140/10143/14.516 us
timeout 390us min/avg/max/std-dev: 9578/10044.150/10507/66.234 us
timeout 195us min/avg/max/std-dev: 9981/10044.170/10122/16.570 us
timeout 97us min/avg/max/std-dev: 10010/10044.150/10117/10.721 us
timeout 48us min/avg/max/std-dev: 10004/10044.140/10083/10.331 us
timeout 24us min/avg/max/std-dev: 10016/10044.220/10078/6.487 us
timeout 12us min/avg/max/std-dev: 10014/10044.210/10079/6.802 us
timeout 6us min/avg/max/std-dev: 10013/10044.300/10085/9.871 us
timeout 3us min/avg/max/std-dev: 10007/10044.030/10090/8.477 us
timeout 1us min/avg/max/std-dev: 9980/10044.350/10135/13.301 us
(obviously some bug in the std-dev calculation there with the -nan)
Thanks again,
Paul
--- measure.c
#include
#include
#include
#include
#include
#include
#include
#define LOOPCOUNT 100
volatile sig_atomic_t trigger;
void
sighdlr(int signum) {
trigger = signum;
}
int
main() {
int long long count, d, sum, min, max, tsumsq;
longtimeout;
struct timeval str, end;
struct itimervalnew_timer;
double avg, dev;
new_timer.it_interval.tv_sec = 0;
new_timer.it_interval.tv_usec = 0;
new_timer.it_value.tv_sec = 0;
signal(SIGALRM, sighdlr);
printf("Running for %d loops\n\n", LOOPCOUNT);
for (timeout = 80; timeout != 0; timeout /= 2) {
new_timer.it_value.tv_usec = timeout;
min = 10;
max = 0;
sum = 0;
tsumsq = 0;
for (count = 0; count != LOOPCOUNT; count++) {
trigger = 0;
setitimer(ITIMER_REAL, _timer, NULL);
gettimeofday(, NULL);
while (trigger == 0)
pause();
gettimeofday(, NULL);
d = (end.tv_sec - str.tv_sec) * 100 + \
end.tv_usec - str.tv_usec;
sum +=
setitimer(2): don't round up it_value
Hi,
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.
Rounding the it_interval member of an itimerval value up to one tick
makes sense: if we don't round it up we might spin for a long time in
realitexpire() and itimerdecr() when we reload the timer.
However there is no such risk with the it_value member. We only use
it once and then it gets clobbered.
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?
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 - 1.58
+++ sys/time.h 12 May 2021 17:06:30 -
@@ -307,7 +307,6 @@ struct proc;
intclock_gettime(struct proc *, clockid_t, struct timespec *);
void cancel_all_itimers(void);
-intitimerfix(struct timeval *);
intitimerdecr(struct itimerspec *, long);
intsettime(const struct timespec *);
intratecheck(struct timeval *, const struct timeval *);
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.c23 Dec 2020 20:45:02 - 1.151
+++ kern/kern_time.c12 May 2021 17:06:30 -
@@ -52,6 +52,8 @@
#include
+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), , sizeof(aitv));
if (error)
return error;
- if (itimerfix(_value) || itimerfix(_interval))
- return EINVAL;
- if (!timerisset(_value))
- timerclear(_interval);
+ error = itimerfix();
+ if (error)
+ return error;
newitvp =
}
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) timer 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 > 1 ||
- tv->tv_usec < 0 || tv->tv_usec >= 100)
- return (EINVAL);
+ if (itv->it_value.tv_sec < 0 || !timerisvalid(>it_value))
+ return EINVAL;
+ if (itv->it_value.tv_sec > 1)
+ 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(>it_interval))
+ return EINVAL;
+ if (itv->it_interval.tv_sec > 1)
+ return EINVAL;
- return (0);
+ if (!timerisset(>it_value))
+ timerclear(>it_interval);
+
+ if (timerisset(>it_interval)) {
+ if (timercmp(>it_interval, _interval, <))
+ itv->it_interval = min_interval;
+ }
+
+ return 0;
}
/*
