Messages in this thread | | | Date | Fri, 14 Dec 2007 12:25:49 +0100 | Subject | Re: Tesing of / bugs in new timerfd API | From | Michael Kerrisk <> |
| |
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 <sys/syscall.h> #include <unistd.h> #include <time.h> #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, struct itimerspec *curr_value) { return syscall(__NR_timerfd_gettime, fd, curr_value); }
#define TFD_TIMER_ABSTIME (1 << 0)
#define handle_error(msg) \ do { perror(msg); exit(EXIT_FAILURE); } while (0)
// #include <sys/timerfd.h> #include <time.h> #include <sys/times.h> #include <unistd.h> #include <stdlib.h> #include <stdio.h> #include <string.h> #include <stdint.h> /* Definition of uint64_t */
static void usage(const char *pname, const char *msg) { if (msg != NULL) printf("%s", msg); fprintf(stderr, "Usage: %s [options] value-sec [value-nsec " "[intvl-sec [intvl-nsec]]]\n", pname); fprintf(stderr, "Options are:\n"); fprintf(stderr, "\t-a Use absolute timer\n"); fprintf(stderr, "\t-m Use CLOCK_MONOTONIC " "(instead of CLOCK_REALTIME)\n");
exit(EXIT_FAILURE); } /* usage */
static void display_help(void) { printf("n val-sec val-nsec intvl-sec intvl-nsec\n" " Reset timer value & interval\n"); printf("g Get timer value\n"); printf("r Read from file descriptor\n"); printf("t Print elapsed time\n"); } /* display_help */
#define MAX_LINE 1024
static void print_itimerspec(struct itimerspec *its) { printf("value=%ld.%03ld, interval=%ld.%03ld", (long) its->it_value.tv_sec, (long) its->it_value.tv_nsec / 1000000, (long) its->it_interval.tv_sec, (long) its->it_interval.tv_nsec / 1000000); } /* print_itimerspec */
int main(int argc, char *argv[]) { struct itimerspec new_value, curr_value; int fd, flags; struct timespec now; int s, opt, use_abs_timer, use_monotonic; long arg1, arg2, arg3, arg4; uint64_t exp; time_t start; char line[MAX_LINE]; int num_read, clockid; char cmd;
use_abs_timer = 0; use_monotonic = 0;
while ((opt = getopt(argc, argv, "am")) != -1) { switch (opt) { case 'a': use_abs_timer = 1; break;
case 'm': use_monotonic = 1; break;
default: usage(argv[1], NULL); break; } /* switch */ } /* while */
clockid = (use_monotonic ? CLOCK_MONOTONIC : CLOCK_REALTIME);
if (optind + 1 > argc) usage(argv[0], NULL);
if (clock_gettime(clockid, &now) == -1) handle_error("clock_gettime");
flags = use_abs_timer ? TFD_TIMER_ABSTIME : 0;
/* Create a timer with initial expiration and interval as specified in command line */
if (use_abs_timer) { new_value.it_value.tv_sec = now.tv_sec + atoi(argv[optind]); new_value.it_value.tv_nsec = (argc > optind + 1) ? atol(argv[optind + 1]) + now.tv_nsec : now.tv_nsec; if (new_value.it_value.tv_nsec > 1000000000) { new_value.it_value.tv_sec += new_value.it_value.tv_nsec / 1000000000; new_value.it_value.tv_nsec %= 1000000000; } /* if */ } else { new_value.it_value.tv_sec = atoi(argv[optind]); new_value.it_value.tv_nsec = (argc > optind + 1) ? atol(argv[optind + 1]) : 0; }
new_value.it_interval.tv_sec = (argc > optind + 2) ? atol(argv[optind + 2]) : 0; new_value.it_interval.tv_nsec = (argc > optind + 3) ? atol(argv[optind + 3]) : 0;
fd = timerfd_create(clockid, 0); if (fd == -1) handle_error("timerfd_create");
printf("Initial setting for settime: "); print_itimerspec(&new_value); printf("\n");
s = timerfd_settime(fd, flags, &new_value, &curr_value); if (s == -1) handle_error("timerfd_settime");
start = time(NULL);
for ( ; ; ) { printf("%s> ", argv[0]); fflush(stdout);
if (fgets(line, MAX_LINE, stdin) == NULL) /* EOF */ exit(EXIT_SUCCESS); line[strlen(line) - 1] = '\0'; /* Remove trailing '\n' */
if (*line == '\0') continue; /* Skip blank lines */
if (line[0] == '?') display_help();
num_read = sscanf(line, " %c %ld %ld %ld %ld", &cmd, &arg1, &arg2, &arg3, &arg4);
switch (cmd) { case 'n': if (num_read != 5) { printf("Wrong number of arguments\n"); continue; }
if (use_abs_timer) { printf("This is an absolute timer\n"); if (clock_gettime(clockid, &now) == -1) handle_error("clock_gettime"); printf("Now: "); printf("value=%ld.%03ld", (long) now.tv_sec, (long) now.tv_nsec / 1000000); printf("\n");
new_value.it_value.tv_sec = now.tv_sec + arg1; new_value.it_value.tv_nsec = now.tv_nsec + arg2;
} else { printf("This is a relative timer\n"); new_value.it_value.tv_sec = arg1; new_value.it_value.tv_nsec = arg2; }
new_value.it_interval.tv_sec = arg3; new_value.it_interval.tv_nsec = arg4;
printf("New setting for settime: "); print_itimerspec(&new_value); printf("\n");
s = timerfd_settime(fd, flags, &new_value, &curr_value); if (s == -1) { perror("timerfd_settime"); break; } printf("Previous setting from settime: "); print_itimerspec(&curr_value); printf("\n");
break;
case 't': printf("%ld\n", (long) (time(NULL) - start)); break;
case 'g': s = timerfd_gettime(fd, &curr_value); if (s == -1) handle_error("timerfd_gettime"); printf("(elapsed time=%3ld)\n", (long) (time(NULL) - start)); printf("Current value: "); print_itimerspec(&curr_value); printf("\n"); break;
case 'r': s = read(fd, &exp, sizeof(uint64_t)); if (s != sizeof(uint64_t)) handle_error("read"); printf("Read: %lld\n", exp); break; } /* switch */ } /* for */
exit(EXIT_SUCCESS); }
| |