Re: Tesing of / bugs in new timerfd API
On Mon, 17 Dec 2007, Michael Kerrisk wrote:
> > Can you try the two patches below? I tried them on my 32 bit box (one of
> > the rare beasts still lingering around here) and it seems to be working
> > fine (those go on top of the previous ones).
>
> Against 2.6.24-rc5, I applied first your earlier patches ("v3") and
> then the newest patch. My tests confirm that:
>
> > This fixed the 32 bit tick-count truncation, and makes the time returned
> > to be the remaining time till the next expiration.
>
> Are you going to resubmit a new patch set that includes these latest changes?
Yes, I'll send them out to Andrew today.
- Davide
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Tesing of / bugs in new timerfd API
Hi Davide,
On 12/16/07, Davide Libenzi <[EMAIL PROTECTED]> wrote:
> On Fri, 14 Dec 2007, Michael Kerrisk wrote:
>
> > You snipped my example that demonstrated the problem. Both of the
> > following runs create a timer that expires 10 seconds from "now", but
> > observe the difference in the value returned by timerfd_gettime():
> >
> > $ ./timerfd_test 10 # does not use TFD_TIMER_ABSTIME
> > Initial setting for settime: value=10.000, interval=0.000
> > ./timerfd_test> g
> > (elapsed time= 1)
> > Current value: value=346.448, interval=0.000
> >
> > $ ./timerfd_test -a 10 # uses TFD_TIMER_ABSTIME
> > Initial setting for settime: value=1197630855.254, interval=0.000
> > ./timerfd_test> g
> > (elapsed time= 1)
> > Current value: value=1197630855.254, interval=0.000
> >
> > Either there's an inconsistency here depending on the use of
> > TFD_TIMER_ABSTIME, or there is a bug in my understanding or my test
> program
> > (but so far I haven't spotted that bug ;-).).
>
> Can you try the two patches below? I tried them on my 32 bit box (one of
> the rare beasts still lingering around here) and it seems to be working
> fine (those go on top of the previous ones).
Against 2.6.24-rc5, I applied first your earlier patches ("v3") and
then the newest patch. My tests confirm that:
> This fixed the 32 bit tick-count truncation, and makes the time returned
> to be the remaining time till the next expiration.
Are you going to resubmit a new patch set that includes these latest changes?
Michael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Tesing of / bugs in new timerfd API
On Fri, 14 Dec 2007, Michael Kerrisk wrote:
> You snipped my example that demonstrated the problem. Both of the
> following runs create a timer that expires 10 seconds from "now", but
> observe the difference in the value returned by timerfd_gettime():
>
> $ ./timerfd_test 10 # does not use TFD_TIMER_ABSTIME
> Initial setting for settime: value=10.000, interval=0.000
> ./timerfd_test> g
> (elapsed time= 1)
> Current value: value=346.448, interval=0.000
>
> $ ./timerfd_test -a 10 # uses TFD_TIMER_ABSTIME
> Initial setting for settime: value=1197630855.254, interval=0.000
> ./timerfd_test> g
> (elapsed time= 1)
> Current value: value=1197630855.254, interval=0.000
>
> Either there's an inconsistency here depending on the use of
> TFD_TIMER_ABSTIME, or there is a bug in my understanding or my test program
> (but so far I haven't spotted that bug ;-).).
Can you try the two patches below? I tried them on my 32 bit box (one of
the rare beasts still lingering around here) and it seems to be working
fine (those go on top of the previous ones).
This fixed the 32 bit tick-count truncation, and makes the time returned
to be the remaining time till the next expiration.
- Davide
---
fs/timerfd.c|6 +++---
include/linux/hrtimer.h | 10 +-
kernel/hrtimer.c|9 -
kernel/posix-timers.c |9 +
4 files changed, 17 insertions(+), 17 deletions(-)
Index: linux-2.6.mod/include/linux/hrtimer.h
===
--- linux-2.6.mod.orig/include/linux/hrtimer.h 2007-12-13 16:37:36.0
-0800
+++ linux-2.6.mod/include/linux/hrtimer.h 2007-12-14 16:05:23.0
-0800
@@ -295,12 +295,12 @@
}
/* Forward a hrtimer so it expires after now: */
-extern unsigned long
+extern u64
hrtimer_forward(struct hrtimer *timer, ktime_t now, ktime_t interval);
/* Forward a hrtimer so it expires after the hrtimer's current now */
-static inline unsigned long hrtimer_forward_now(struct hrtimer *timer,
- ktime_t interval)
+static inline u64 hrtimer_forward_now(struct hrtimer *timer,
+ ktime_t interval)
{
return hrtimer_forward(timer, timer->base->get_time(), interval);
}
@@ -322,9 +322,9 @@
extern void __init hrtimers_init(void);
#if BITS_PER_LONG < 64
-extern unsigned long ktime_divns(const ktime_t kt, s64 div);
+extern u64 ktime_divns(const ktime_t kt, s64 div);
#else /* BITS_PER_LONG < 64 */
-# define ktime_divns(kt, div) (unsigned long)((kt).tv64 / (div))
+# define ktime_divns(kt, div) (u64)((kt).tv64 / (div))
#endif
/* Show pending timers: */
Index: linux-2.6.mod/kernel/hrtimer.c
===
--- linux-2.6.mod.orig/kernel/hrtimer.c 2007-12-13 16:37:53.0 -0800
+++ linux-2.6.mod/kernel/hrtimer.c 2007-12-13 16:41:42.0 -0800
@@ -306,7 +306,7 @@
/*
* Divide a ktime value by a nanosecond value
*/
-unsigned long ktime_divns(const ktime_t kt, s64 div)
+u64 ktime_divns(const ktime_t kt, s64 div)
{
u64 dclc, inc, dns;
int sft = 0;
@@ -321,7 +321,7 @@
dclc >>= sft;
do_div(dclc, (unsigned long) div);
- return (unsigned long) dclc;
+ return dclc;
}
#endif /* BITS_PER_LONG >= 64 */
@@ -655,10 +655,9 @@
* Forward the timer expiry so it will expire in the future.
* Returns the number of overruns.
*/
-unsigned long
-hrtimer_forward(struct hrtimer *timer, ktime_t now, ktime_t interval)
+u64 hrtimer_forward(struct hrtimer *timer, ktime_t now, ktime_t interval)
{
- unsigned long orun = 1;
+ u64 orun = 1;
ktime_t delta;
delta = ktime_sub(now, timer->expires);
Index: linux-2.6.mod/kernel/posix-timers.c
===
--- linux-2.6.mod.orig/kernel/posix-timers.c2007-12-13 16:38:15.0
-0800
+++ linux-2.6.mod/kernel/posix-timers.c 2007-12-13 16:45:20.0 -0800
@@ -256,8 +256,9 @@
if (timr->it.real.interval.tv64 == 0)
return;
- timr->it_overrun += hrtimer_forward(timer, timer->base->get_time(),
- timr->it.real.interval);
+ timr->it_overrun += (unsigned int) hrtimer_forward(timer,
+ timer->base->get_time(),
+ timr->it.real.interval);
timr->it_overrun_last = timr->it_overrun;
timr->it_overrun = -1;
@@ -386,7 +387,7 @@
now = ktime_add(now, kj);
}
#endif
- timr->it_overrun +=
+ timr->it_overrun += (unsigned int)
hrtimer_forward(timer, now,
timr->it.real.interv
Re: Tesing of / bugs in new timerfd API
Hi Davide,
Davide Libenzi wrote:
> On Thu, 13 Dec 2007, Michael Kerrisk wrote:
>
BUG 2:
The last sentence does not match the implementation.
(Nor is it consistent with the behavior of POSIX timers.
And I *think* things did work correctly in the original
timerfd() implementation, but I have not gone back to check.)
Suppose that we set an absolute timer to expire 100 seconds
in the future. Then according to this sentence of the man
page then each subsequent call to timerfd_gettime() should
retrun an itimerspec structure whose it_value steadily
decreases from 100 to 0 (when the timer expires). (This
is the behavior in the analogous situation with POSIX timers
and with setitimer()/getitimer().)
However, the implementation of timerfd_gettime() always
returns the "time when the timer would next expire", and
this value depends on whether TFD_TIMER_ABSTIME was specified
when setting the timer.
>>> This is been like that from the beginning of the new API. So no, the
>>> previous was behaving exactly the same WRT this feature.
>>> Is this something really needed?
>> Three reasons that I think of off the top of my head (and there might
>> well be more reasosn) why this should change:
>>
>> a) consistency with the other two timer APIs (POSIX timers
>> (timer_create(), etc.), and setitimer()/getitimer()).
You made no comment on this, which is perhaps the most important of the
reasons to make the change.
>> b) Returning the amount of time until the next expiration is more
>> useful to userland: I'd say the most common case is for userland to
>> want to know how long until the next expiration occurs, or to adjust
>> that time by adding/subtracting some value to the existing setting.
>> That is difficult to with the current implementation: the userland app
>> must use timer_gettime(), call clock_gettime(), and calculate the
>> difference between the two, in order to know how much time remains
>> until the next timer expiration.
>
> Bah, I don't want to argue with that because otherwise it starts going
> towards the "typical" use cases listing, that can be found the same exact
> reasons to have one or the other way. And we end up wasting lots of time.
> We'd just have to move another thing that userspace could do, inside the
> kernel (subtract the current value returned by hrtimer_forward() in
> ->expires with "now").
Okay -- I still think this reason matters, but let's leave it for the
moment. I think the other reasons are strong enough anyway...
>> c) Currently, the information returned differs depending on whether
>> TFD_TIMER_ABSTIME is specified -- this is not the case for the
>> analogous situation for POSIX timers. For POSIX timers, the returned
>> setting is always the amount of time until the timer next expires.
>> This inconsistency is messy for applications -- the application may
>> not (be able to) know whether or not the timer it is examining was set
>> using TFD_TIMER_ABSTIME (the timerfd might have been created by a
>> library, for example).
>
> Hmm... the time returned is always the next absolute time when the next
> timer tick will go off. It does not depend on TFD_TIMER_ABSTIME. I return
> the ->expires field of the timer, and hrtimer_forward() sets it to the
> next absolute time.
You snipped my example that demonstrated the problem. Both of the
following runs create a timer that expires 10 seconds from "now", but
observe the difference in the value returned by timerfd_gettime():
$ ./timerfd_test 10 # does not use TFD_TIMER_ABSTIME
Initial setting for settime: value=10.000, interval=0.000
./timerfd_test> g
(elapsed time= 1)
Current value: value=346.448, interval=0.000
$ ./timerfd_test -a 10 # uses TFD_TIMER_ABSTIME
Initial setting for settime: value=1197630855.254, interval=0.000
./timerfd_test> g
(elapsed time= 1)
Current value: value=1197630855.254, interval=0.000
Either there's an inconsistency here depending on the use of
TFD_TIMER_ABSTIME, or there is a bug in my understanding or my test program
(but so far I haven't spotted that bug ;-).).
Cheers,
Michael
PS There was a little bug in my test program, do do with the treatment of
nanosecond command-line arguments. It didn't affect any of the tests, but
for completeness, the revised version of the program is below.
/* timerfd_test.c */
/* Link with -lrt */
#define _GNU_SOURCE
#include
#include
#include
#if defined(__i386__)
#define __NR_timerfd_create 322
#define __NR_timerfd_settime 325
#define __NR_timerfd_gettime 326
#endif
static int
timerfd_create(int clockid, int flags)
{
return syscall(__NR_timerfd_create, clockid, flags);
}
static int
timerfd_settime(int fd, int flags, struct itimerspec *new_value,
struct itimerspec *curr_value)
{
return syscall(__NR_timerfd_settime, fd, flags,
new_value, curr_value);
}
static int
timerfd_gettime(int fd, str
Re: Tesing of / bugs in new timerfd API
On Thu, 13 Dec 2007, Michael Kerrisk wrote: > > > BUG 2: > > > The last sentence does not match the implementation. > > > (Nor is it consistent with the behavior of POSIX timers. > > > And I *think* things did work correctly in the original > > > timerfd() implementation, but I have not gone back to check.) > > > > > > Suppose that we set an absolute timer to expire 100 seconds > > > in the future. Then according to this sentence of the man > > > page then each subsequent call to timerfd_gettime() should > > > retrun an itimerspec structure whose it_value steadily > > > decreases from 100 to 0 (when the timer expires). (This > > > is the behavior in the analogous situation with POSIX timers > > > and with setitimer()/getitimer().) > > > > > > However, the implementation of timerfd_gettime() always > > > returns the "time when the timer would next expire", and > > > this value depends on whether TFD_TIMER_ABSTIME was specified > > > when setting the timer. > > > > This is been like that from the beginning of the new API. So no, the > > previous was behaving exactly the same WRT this feature. > > Is this something really needed? > > Three reasons that I think of off the top of my head (and there might > well be more reasosn) why this should change: > > a) consistency with the other two timer APIs (POSIX timers > (timer_create(), etc.), and setitimer()/getitimer()). > > b) Returning the amount of time until the next expiration is more > useful to userland: I'd say the most common case is for userland to > want to know how long until the next expiration occurs, or to adjust > that time by adding/subtracting some value to the existing setting. > That is difficult to with the current implementation: the userland app > must use timer_gettime(), call clock_gettime(), and calculate the > difference between the two, in order to know how much time remains > until the next timer expiration. Bah, I don't want to argue with that because otherwise it starts going towards the "typical" use cases listing, that can be found the same exact reasons to have one or the other way. And we end up wasting lots of time. We'd just have to move another thing that userspace could do, inside the kernel (subtract the current value returned by hrtimer_forward() in ->expires with "now"). > c) Currently, the information returned differs depending on whether > TFD_TIMER_ABSTIME is specified -- this is not the case for the > analogous situation for POSIX timers. For POSIX timers, the returned > setting is always the amount of time until the timer next expires. > This inconsistency is messy for applications -- the application may > not (be able to) know whether or not the timer it is examining was set > using TFD_TIMER_ABSTIME (the timerfd might have been created by a > library, for example). Hmm... the time returned is always the next absolute time when the next timer tick will go off. It does not depend on TFD_TIMER_ABSTIME. I return the ->expires field of the timer, and hrtimer_forward() sets it to the next absolute time. - Davide -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Tesing of / bugs in new timerfd API
Hi Davide, On Dec 13, 2007 10:49 PM, Davide Libenzi <[EMAIL PROTECTED]> wrote: > On Thu, 13 Dec 2007, Michael Kerrisk wrote: > > > Davide, Andrew, > > > > I applied Davide's v3 patchset (sent into LKML on 25 Nov) against > > 2.4.24-rc3, and did various tests (all on x86). Several tests > > were done using the program at the foot of this mail. Various others > > were done by cobbling together bits of code that I haven't included > > here. > > Thanks for such a thorough test Michael. You're welcome! [...] > > BUG 2: > > The last sentence does not match the implementation. > > (Nor is it consistent with the behavior of POSIX timers. > > And I *think* things did work correctly in the original > > timerfd() implementation, but I have not gone back to check.) > > > > Suppose that we set an absolute timer to expire 100 seconds > > in the future. Then according to this sentence of the man > > page then each subsequent call to timerfd_gettime() should > > retrun an itimerspec structure whose it_value steadily > > decreases from 100 to 0 (when the timer expires). (This > > is the behavior in the analogous situation with POSIX timers > > and with setitimer()/getitimer().) > > > > However, the implementation of timerfd_gettime() always > > returns the "time when the timer would next expire", and > > this value depends on whether TFD_TIMER_ABSTIME was specified > > when setting the timer. > > This is been like that from the beginning of the new API. So no, the > previous was behaving exactly the same WRT this feature. > Is this something really needed? Three reasons that I think of off the top of my head (and there might well be more reasosn) why this should change: a) consistency with the other two timer APIs (POSIX timers (timer_create(), etc.), and setitimer()/getitimer()). b) Returning the amount of time until the next expiration is more useful to userland: I'd say the most common case is for userland to want to know how long until the next expiration occurs, or to adjust that time by adding/subtracting some value to the existing setting. That is difficult to with the current implementation: the userland app must use timer_gettime(), call clock_gettime(), and calculate the difference between the two, in order to know how much time remains until the next timer expiration. c) Currently, the information returned differs depending on whether TFD_TIMER_ABSTIME is specified -- this is not the case for the analogous situation for POSIX timers. For POSIX timers, the returned setting is always the amount of time until the timer next expires. This inconsistency is messy for applications -- the application may not (be able to) know whether or not the timer it is examining was set using TFD_TIMER_ABSTIME (the timerfd might have been created by a library, for example). Cheers, Michael -- Michael Kerrisk Maintainer of the Linux man-pages project http://www.kernel.org/doc/man-pages/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Tesing of / bugs in new timerfd API
On Thu, 13 Dec 2007, Michael Kerrisk wrote: > Davide, Andrew, > > I applied Davide's v3 patchset (sent into LKML on 25 Nov) against > 2.4.24-rc3, and did various tests (all on x86). Several tests > were done using the program at the foot of this mail. Various others > were done by cobbling together bits of code that I haven't included > here. Thanks for such a thorough test Michael. > Tested: after setting a CLOCK_REALTIME timer with the > TFD_TIMER_ABSTIME flag to expire at some time in the past with > a non-zero interval (e.g., setting 100 seconds in the past, with > a 5 second interval), read() from the file descriptor returns > the correct number of expirations (e.g., 20). > > This seems a reasonable thing to do, I suppose. However, while > playing around to test this, I found what looks like a bug (see > below). > > BUG 1: > However, this test exposed what looks like a bug: if I set a > CLOCK_REALTIME clock to expire in the past, with a very small > interval, then the maximum number of expirations that can be > returned via read seems to be limited to 32 bits, even though > we have a 64-bit value for returning this information. > I haven't checked the kernel source to determine where this > bug is. Yes, true. Right now hrtimer_forward() (that we use to get the "missed" ticks) returns an unsigned long, that on 32 bit is (duh!) 32 bit :) But hrtimer_forward() has all in place to just return an u64. So, Thomas, would it be OK to have hrtimer_forward() (and the new hrtimer_forward_now()) to return a u64? > BUG 2: > The last sentence does not match the implementation. > (Nor is it consistent with the behavior of POSIX timers. > And I *think* things did work correctly in the original > timerfd() implementation, but I have not gone back to check.) > > Suppose that we set an absolute timer to expire 100 seconds > in the future. Then according to this sentence of the man > page then each subsequent call to timerfd_gettime() should > retrun an itimerspec structure whose it_value steadily > decreases from 100 to 0 (when the timer expires). (This > is the behavior in the analogous situation with POSIX timers > and with setitimer()/getitimer().) > > However, the implementation of timerfd_gettime() always > returns the "time when the timer would next expire", and > this value depends on whether TFD_TIMER_ABSTIME was specified > when setting the timer. This is been like that from the beginning of the new API. So no, the previous was behaving exactly the same WRT this feature. Is this something really needed? - Davide -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Tesing of / bugs in new timerfd API
Davide, Andrew,
I applied Davide's v3 patchset (sent into LKML on 25 Nov) against
2.4.24-rc3, and did various tests (all on x86). Several tests
were done using the program at the foot of this mail. Various others
were done by cobbling together bits of code that I haven't included
here.
In covering the tests I ran, and as a kind of proof of concept,
I'll treat the draft man page as the test spec.
I think I've found two bugs (look for the string "BUG" below), one
of which is a significant deviation from expected behavior. See
the comments below.
Cheers,
Michael
> SYNOPSIS
> #include
>
> int timerfd_create(int clockid, int flags);
>
> int timerfd_settime(int fd, int flags,
> const struct itimerspec *new_value,
> struct itimerspec *curr_value);
>
> int timerfd_gettime(int fd,
> struct itimerspec *curr_value);
>
> DESCRIPTION
> These system calls create and operate on a timer that
> delivers timer expiration notifications via a file
> descriptor. They provide an alternative to the use of
> setitimer(2) or timer_create(3), with the advantage that
> the file descriptor may be monitored by select(2),
> poll(2), and epoll(7).
>
> The use of these three system calls is analogous to the
> use of timer_create(3), timer_settime(3), and timer_get-
> time(3). (There is no analog of timer_gettoverrun(3),
> since that functionality is provided by read(2), as
> described below.)
>
> timerfd_create()
> timerfd_create() creates a new timer object, and returns
> a file descriptor that refers to that timer. The
> clockid argument specifies the clock that is used to
> mark the progress of the timer, and must be either
> CLOCK_REALTIME or CLOCK_MONOTONIC.
Verified. I've tried creating various combinations of
CLOCK_REALTIME and CLOCK_MONOTONIC timers, both one shot
(it_interval is zero) and repeating (it_interval is non-zero),
and everything works as I would expect.
> CLOCK_REALTIME is a
> settable system-wide clock. CLOCK_MONOTONIC is a non-
> settable clock that is not affected by discontinuous
> changes in the system clock (e.g., manual changes to
> system time). The current value of each of these clocks
> can be retrieved using clock_gettime(3).
>
> The flags argument is reserved for future use. As at
> Linux 2.6.25, this argument must be specified as zero.
(Verified: see the ERRORS section of the man page, below.)
> timerfd_settime()
> timerfd_settime() arms (starts) or disarms (stops) the
> timer referred to by the file descriptor fd.
Disarming a timer works fine in my tests.
> The new_value argument specifies the initial expiration
> and interval for the timer. The itimer structure used
> for this argument contains two fields, each of which is
> in turn a structure of type timespec:
>
> struct timespec {
> time_t tv_sec;/* Seconds */
> long tv_nsec; /* Nanoseconds */
> };
>
> struct itimerspec {
> struct timespec it_interval; /* Interval for
>periodic timer */
> struct timespec it_value; /* Initial
>expiration */
> };
>
> new_value.it_value specifies the initial expiration of
> the timer, in seconds and nanoseconds. Setting either
> field of new_value.it_value to a non-zero value arms the
> timer. Setting both fields of new_value.it_value to
> zero disarms the timer.
Verified: setting both fields of new_value.it_value to
zero disarms the timer.
> Setting one or both fields of new_value.it_interval to
> non-zero values specifies the period, in seconds and
> nanoseconds, for repeated timer expirations after the
> initialexpiration.
I've tested intervals down to 1 nanosec, which seem to seems to
work as I would expect. (But see the BUG reported below.)
> If both fields of
> new_value.it_interval are zero, the timer expires just
> once, at the time specified by new_value.it_value.
Verified.
> The flags argument is either 0, to start a relative
> timer (new_value.it_interval specifies a time relative
> to the current value of the clock specified by clockid),
> or TFD_TIMER_ABSTIME, to start an absolute timer
> (new_value.it_interval specifies an absolute time for
> the clock specified by clockid; that is, the timer will
> expire when the value of that clock reaches the value
> specified in new_value.it_interval).
Tested: after setting a CLOCK_REALTIME timer with the
TFD_TIMER_ABSTIME flag to expire at some time in the past with
a non-zero interval (e.g., setting 100 seconds in the past, with
a 5 secon
