[patch 2/2] timerfd extend clockid support

[patch 2/2] timerfd extend clockid support

[Davide Libenzi]

The following patch extends timerfd clockid support to cover the ones 
supported by timer_create(). 
It exports the invalid_clockid() function outside posix-timers.c to allow 
timerfd to properly check input parameters.
Andrew, this is (eventually) .30 material, and do not take the patch 
until you have Thomas sign off.
Thomas, timerfd uses core hrtimer functions for its tasks. By extending 
the clockid support, I assume that the clockids other than CLOCK_MONOTONIC 
and CLOCK_REALTIME, behaves the same from a hrtimer caller POV. Right?



Signed-off-by: Davide Libenzi <davidel@xmailserver.org>


- Davide


---
 fs/timerfd.c                 |    3 +--
 include/linux/posix-timers.h |    1 +
 kernel/posix-timers.c        |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6.mod/fs/timerfd.c
===================================================================
--- linux-2.6.mod.orig/fs/timerfd.c	2009-02-08 15:02:06.000000000 -0800
+++ linux-2.6.mod/fs/timerfd.c	2009-02-08 15:03:43.000000000 -0800
@@ -187,8 +187,7 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
 	BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
 
 	if ((flags & ~TFD_CREATE_FLAGS) ||
-	    (clockid != CLOCK_MONOTONIC &&
-	     clockid != CLOCK_REALTIME))
+	    invalid_clockid(clockid))
 		return -EINVAL;
 
 	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
Index: linux-2.6.mod/include/linux/posix-timers.h
===================================================================
--- linux-2.6.mod.orig/include/linux/posix-timers.h	2009-02-08 15:02:06.000000000 -0800
+++ linux-2.6.mod/include/linux/posix-timers.h	2009-02-08 15:02:53.000000000 -0800
@@ -84,6 +84,7 @@ struct k_clock {
 			   struct itimerspec * cur_setting);
 };
 
+int invalid_clockid(const clockid_t which_clock);
 void register_posix_clock(const clockid_t clock_id, struct k_clock *new_clock);
 
 /* error handlers for timer_create, nanosleep and settime */
Index: linux-2.6.mod/kernel/posix-timers.c
===================================================================
--- linux-2.6.mod.orig/kernel/posix-timers.c	2009-02-08 15:02:06.000000000 -0800
+++ linux-2.6.mod/kernel/posix-timers.c	2009-02-08 15:02:53.000000000 -0800
@@ -205,7 +205,7 @@ static int no_timer_create(struct k_itim
 /*
  * Return nonzero if we know a priori this clockid_t value is bogus.
  */
-static inline int invalid_clockid(const clockid_t which_clock)
+int invalid_clockid(const clockid_t which_clock)
 {
 	if (which_clock < 0)	/* CPU clock, posix_cpu_* will check it */
 		return 0;

Re: [patch 2/2] timerfd extend clockid support

[Michael Kerrisk]

On Mon, Feb 9, 2009 at 12:23 PM, Davide Libenzi <davidel@xmailserver.org> wrote:
> The following patch extends timerfd clockid support to cover the ones
> supported by timer_create().
> It exports the invalid_clockid() function outside posix-timers.c to allow
> timerfd to properly check input parameters.
> Andrew, this is (eventually) .30 material, and do not take the patch
> until you have Thomas sign off.
> Thomas, timerfd uses core hrtimer functions for its tasks. By extending
> the clockid support, I assume that the clockids other than CLOCK_MONOTONIC
> and CLOCK_REALTIME, behaves the same from a hrtimer caller POV. Right?

Hi Davide,

Have you done any testing of this patch?  My attempts at testing with
clocks other than REALTIME and MONOTONIC all don't work so far.  In
some cases, my test programs causes the system to hang.  Try
experimenting with the (not yet well tested) test program below, to
check the behavior with other clocks.

Cheers,

Michael

/* timerfd_demo.c

   Compile with -lrt
*/
#include <stdlib.h>
#include <unistd.h>
#include <stdio.h>
#include <signal.h>
#include <errno.h>
#include <time.h>
#include <sys/timerfd.h>
#include <pthread.h>
#include <stdint.h>               /* Definition of uint64_t */

#define SIG SIGUSR1

#define errExit(msg)    do { perror(msg); exit(EXIT_FAILURE); \
                        } while (0)

static void *
thread_func(void *arg)
{
    printf("Thread burning CPU\n");
    for (;;)
        ;
}

int
main(int argc, char *argv[])
{
    timer_t timer_id;
    struct itimerspec its;
    clockid_t clock_id;
    int flags, s, fd;
    struct timespec ts;
    uint64_t numExp;

    if (argc < 4) {
        fprintf(stderr, "Usage: %s <clockid> <flags> <num-secs> "
                "[<clock-adj-secs>]\n", argv[0]);
#define fpe(str) fprintf(stderr, str);
        fpe("<clockid> is one of:\n");
        fpe("\tm  CLOCK_MONOTONIC\n");
        fpe("\tr  CLOCK_REALTIME\n");
        fpe("\tp  CLOCK_PROCESS_CPUTIME_ID\n");
        fpe("\tC  clock of a child process that burns CPU time\n");
        fpe("\tT  clock of a sub-thread that burns CPU time\n");
        fpe("<flags> is one of:\n");
        fpe("\ta  absolute timer (<num-secs> is added to current\n");
        fpe("\t   clock value)\n")
        fpe("\t-  relative timer\n")
        fpe("<num-secs> is the initial expiration time for the timer\n");
        fpe("<clock-adj-secs> is number of seconds by which clock\n");
        fpe("\tshould be adjusted (clock_settime()) after timer\n");
        fpe("\thas started\n")
        exit(EXIT_FAILURE);
    }

    if (argv[1][0] == 'C') {
        pid_t cpid;

        cpid = fork();
        if (cpid == -1)
            errExit("fork");

        if (cpid == 0) {
            usleep(10000);
            printf("Child process burning CPU time\n");
            alarm(100); /* Ensure child eventually dies */
            for (;;)
                ;
        } else {        /* Parent gets CPU clock ID of child and
                           falls through */
            if (clock_getcpuclockid(cpid, &clock_id) == -1)
                errExit("clock_getcpuclockid");
        }

    } else if (argv[1][0] == 'T') {
        pthread_t t;

        errno = pthread_create(&t, NULL, thread_func, NULL);
        if (errno != 0)
            errExit("pthread_create");
        errno = pthread_getcpuclockid(t, &clock_id);
        if (errno != 0)
            errExit("pthread_getcpuclockid");

    } else {
        clock_id = (argv[1][0] == 'm') ? CLOCK_MONOTONIC :
                   (argv[1][0] == 'r') ? CLOCK_REALTIME :
                   (argv[1][0] == 'p') ? CLOCK_PROCESS_CPUTIME_ID :
                   -999999;     /* Unlikely to be a valid clock ID */

        if (clock_id == CLOCK_PROCESS_CPUTIME_ID) {
            pthread_t t;

            errno = pthread_create(&t, NULL, thread_func, NULL);
            if (errno != 0)
                errExit("pthread_create");
        }
    }

    fd = timerfd_create(clock_id, 0);
    if (fd == -1)
        errExit("timerfd_create");

    printf("clock ID is 0x%lx\n", (long) clock_id);
    printf("timer ID is 0x%lx\n", (long) timer_id);

    flags = (argv[2][0] == 'a') ? TFD_TIMER_ABSTIME : 0;

    if (flags & TFD_TIMER_ABSTIME) {
        printf("Absolute timer\n");
        if (clock_gettime(clock_id, &ts) == -1)
            errExit("clock_gettime");
        printf("Current clock value = %ld\n", (long) ts.tv_sec);
        its.it_value.tv_sec = ts.tv_sec + atoi(argv[3]);
        its.it_value.tv_nsec = ts.tv_nsec;
    } else {
        its.it_value.tv_sec = atoi(argv[3]);
        its.it_value.tv_nsec = 0;
    }

    its.it_interval.tv_sec = 1;
    its.it_interval.tv_nsec = 0;

    printf("its.it_value.tv_sec = %ld\n",
            (long) its.it_value.tv_sec);

    if (timerfd_settime(fd, flags, &its, NULL) == -1)
         errExit("timer_settime");

    if (argc > 4) {
        if (clock_gettime(clock_id, &ts) == -1)
            errExit("clock_gettime");
        ts.tv_sec += atoi(argv[4]);
        printf("About to adjust clock to %ld\n", (long) ts.tv_sec);
        if (clock_settime(clock_id, &ts) == -1)
            errExit("clock_settime");
    }

    s = read(fd, &numExp, sizeof(uint64_t));
    if (s != sizeof(uint64_t))
        errExit("read");
    printf("number of expirations = %lld\n",
            (unsigned long long) numExp);
}

Re: [patch 2/2] timerfd extend clockid support

[Davide Libenzi]

On Mon, 9 Feb 2009, Michael Kerrisk wrote:

> On Mon, Feb 9, 2009 at 12:23 PM, Davide Libenzi <davidel@xmailserver.org> wrote:
> > The following patch extends timerfd clockid support to cover the ones
> > supported by timer_create().
> > It exports the invalid_clockid() function outside posix-timers.c to allow
> > timerfd to properly check input parameters.
> > Andrew, this is (eventually) .30 material, and do not take the patch
> > until you have Thomas sign off.
> > Thomas, timerfd uses core hrtimer functions for its tasks. By extending
> > the clockid support, I assume that the clockids other than CLOCK_MONOTONIC
> > and CLOCK_REALTIME, behaves the same from a hrtimer caller POV. Right?
> 
> Hi Davide,
> 
> Have you done any testing of this patch?  My attempts at testing with
> clocks other than REALTIME and MONOTONIC all don't work so far.  In
> some cases, my test programs causes the system to hang.  Try
> experimenting with the (not yet well tested) test program below, to
> check the behavior with other clocks.

Michael, no I did not test it and I did not have even the time to peek 
into the core timer code to see if the usage of other timer types in 
eventfd would create problems. That's why I asked Thomas if they'd behave 
differently from an hrtimer caller POV.
I'll try to take a look by myself today or tomorrow.


- Davide

Re: [patch 2/2] timerfd extend clockid support

[Michael Kerrisk]

On Tue, Feb 10, 2009 at 6:42 AM, Davide Libenzi <davidel@xmailserver.org> wrote:
> On Mon, 9 Feb 2009, Michael Kerrisk wrote:
>
>> On Mon, Feb 9, 2009 at 12:23 PM, Davide Libenzi <davidel@xmailserver.org> wrote:
>> > The following patch extends timerfd clockid support to cover the ones
>> > supported by timer_create().
>> > It exports the invalid_clockid() function outside posix-timers.c to allow
>> > timerfd to properly check input parameters.
>> > Andrew, this is (eventually) .30 material, and do not take the patch
>> > until you have Thomas sign off.
>> > Thomas, timerfd uses core hrtimer functions for its tasks. By extending
>> > the clockid support, I assume that the clockids other than CLOCK_MONOTONIC
>> > and CLOCK_REALTIME, behaves the same from a hrtimer caller POV. Right?
>>
>> Hi Davide,
>>
>> Have you done any testing of this patch?  My attempts at testing with
>> clocks other than REALTIME and MONOTONIC all don't work so far.  In
>> some cases, my test programs causes the system to hang.  Try
>> experimenting with the (not yet well tested) test program below, to
>> check the behavior with other clocks.
>
> Michael, no I did not test it

Yep -- I realised afterwards that you had already said that...

> and I did not have even the time to peek
> into the core timer code to see if the usage of other timer types in
> eventfd would create problems. That's why I asked Thomas if they'd behave
> differently from an hrtimer caller POV.
> I'll try to take a look by myself today or tomorrow.

Okay -- hopefully my test program may be useful (even if it is not
itself fully tested yet, it's patterned after a similar test program I
wrote fot the POSIX timers API, so it should mostly work).

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [patch 2/2] timerfd extend clockid support

[Davide Libenzi]

On Tue, 10 Feb 2009, Michael Kerrisk wrote:

> > and I did not have even the time to peek
> > into the core timer code to see if the usage of other timer types in
> > eventfd would create problems. That's why I asked Thomas if they'd behave
> > differently from an hrtimer caller POV.
> > I'll try to take a look by myself today or tomorrow.
> 
> Okay -- hopefully my test program may be useful (even if it is not
> itself fully tested yet, it's patterned after a similar test program I
> wrote fot the POSIX timers API, so it should mostly work).

Answer was pretty easy once you look at the code :)
Timerfd uses core hrtimer functions, and clockids different from the ones 
timerfd already handles, fall into the CPU-timers domain. Domain that is 
not handled by hrtimer.
Changes to timerfd to support CPU-based timers are really deep (more than 
changes, is a total rewrite). not only to timerfd, but also to CPU-based 
timers to deliver notification by means different than signals.
Given the amount of code change, and given that a posix-timers->signalfd 
bridge could solve the problem, I'm not going even close to suggest such a 
change.



- Davide

Re: [patch 2/2] timerfd extend clockid support

[Michael Kerrisk]

On Tue, Feb 10, 2009 at 9:10 AM, Davide Libenzi <davidel@xmailserver.org> wrote:
> On Tue, 10 Feb 2009, Michael Kerrisk wrote:
>
>> > and I did not have even the time to peek
>> > into the core timer code to see if the usage of other timer types in
>> > eventfd would create problems. That's why I asked Thomas if they'd behave
>> > differently from an hrtimer caller POV.
>> > I'll try to take a look by myself today or tomorrow.
>>
>> Okay -- hopefully my test program may be useful (even if it is not
>> itself fully tested yet, it's patterned after a similar test program I
>> wrote fot the POSIX timers API, so it should mostly work).
>
> Answer was pretty easy once you look at the code :)
> Timerfd uses core hrtimer functions, and clockids different from the ones
> timerfd already handles, fall into the CPU-timers domain. Domain that is
> not handled by hrtimer.
> Changes to timerfd to support CPU-based timers are really deep (more than
> changes, is a total rewrite). not only to timerfd, but also to CPU-based
> timers to deliver notification by means different than signals.
> Given the amount of code change, and given that a posix-timers->signalfd
> bridge could solve the problem, I'm not going even close to suggest such a
> change.

Okay, thanks for the info.  I've added some text to the
timerfd_create(2) page to explicitly note that the range of clocks
supported by timerfd_create() is smaller than POSIX timers.

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [patch 2/2] timerfd extend clockid support

[Michael Kerrisk]

On Tue, Feb 10, 2009 at 9:10 AM, Davide Libenzi <davidel@xmailserver.org> wrote:
> On Tue, 10 Feb 2009, Michael Kerrisk wrote:
>
>> > and I did not have even the time to peek
>> > into the core timer code to see if the usage of other timer types in
>> > eventfd would create problems. That's why I asked Thomas if they'd behave
>> > differently from an hrtimer caller POV.
>> > I'll try to take a look by myself today or tomorrow.
>>
>> Okay -- hopefully my test program may be useful (even if it is not
>> itself fully tested yet, it's patterned after a similar test program I
>> wrote fot the POSIX timers API, so it should mostly work).
>
> Answer was pretty easy once you look at the code :)
> Timerfd uses core hrtimer functions, and clockids different from the ones
> timerfd already handles, fall into the CPU-timers domain. Domain that is
> not handled by hrtimer.
> Changes to timerfd to support CPU-based timers are really deep (more than
> changes, is a total rewrite). not only to timerfd, but also to CPU-based
> timers to deliver notification by means different than signals.
> Given the amount of code change, and given that a posix-timers->signalfd
> bridge could solve the problem, I'm not going even close to suggest such a
> change.

Davide,

Will you nevertheless push the patch that adds the EINVAL flags checks?

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [patch 2/2] timerfd extend clockid support

[Davide Libenzi]

On Tue, 10 Feb 2009, Michael Kerrisk wrote:

> On Tue, Feb 10, 2009 at 9:10 AM, Davide Libenzi <davidel@xmailserver.org> wrote:
> > On Tue, 10 Feb 2009, Michael Kerrisk wrote:
> >
> >> > and I did not have even the time to peek
> >> > into the core timer code to see if the usage of other timer types in
> >> > eventfd would create problems. That's why I asked Thomas if they'd behave
> >> > differently from an hrtimer caller POV.
> >> > I'll try to take a look by myself today or tomorrow.
> >>
> >> Okay -- hopefully my test program may be useful (even if it is not
> >> itself fully tested yet, it's patterned after a similar test program I
> >> wrote fot the POSIX timers API, so it should mostly work).
> >
> > Answer was pretty easy once you look at the code :)
> > Timerfd uses core hrtimer functions, and clockids different from the ones
> > timerfd already handles, fall into the CPU-timers domain. Domain that is
> > not handled by hrtimer.
> > Changes to timerfd to support CPU-based timers are really deep (more than
> > changes, is a total rewrite). not only to timerfd, but also to CPU-based
> > timers to deliver notification by means different than signals.
> > Given the amount of code change, and given that a posix-timers->signalfd
> > bridge could solve the problem, I'm not going even close to suggest such a
> > change.
> 
> Davide,
> 
> Will you nevertheless push the patch that adds the EINVAL flags checks?

Sure, I'll send it to Greg and Andrew.


- Davide

Re: [patch 2/2] timerfd extend clockid support

[Michael Kerrisk]

On Tue, Feb 10, 2009 at 12:23 PM, Davide Libenzi
<davidel@xmailserver.org> wrote:
> On Tue, 10 Feb 2009, Michael Kerrisk wrote:
>
>> On Tue, Feb 10, 2009 at 9:10 AM, Davide Libenzi <davidel@xmailserver.org> wrote:
>> > On Tue, 10 Feb 2009, Michael Kerrisk wrote:
>> >
>> >> > and I did not have even the time to peek
>> >> > into the core timer code to see if the usage of other timer types in
>> >> > eventfd would create problems. That's why I asked Thomas if they'd behave
>> >> > differently from an hrtimer caller POV.
>> >> > I'll try to take a look by myself today or tomorrow.
>> >>
>> >> Okay -- hopefully my test program may be useful (even if it is not
>> >> itself fully tested yet, it's patterned after a similar test program I
>> >> wrote fot the POSIX timers API, so it should mostly work).
>> >
>> > Answer was pretty easy once you look at the code :)
>> > Timerfd uses core hrtimer functions, and clockids different from the ones
>> > timerfd already handles, fall into the CPU-timers domain. Domain that is
>> > not handled by hrtimer.
>> > Changes to timerfd to support CPU-based timers are really deep (more than
>> > changes, is a total rewrite). not only to timerfd, but also to CPU-based
>> > timers to deliver notification by means different than signals.
>> > Given the amount of code change, and given that a posix-timers->signalfd
>> > bridge could solve the problem, I'm not going even close to suggest such a
>> > change.
>>
>> Davide,
>>
>> Will you nevertheless push the patch that adds the EINVAL flags checks?
>
> Sure, I'll send it to Greg and Andrew.

Thanks!  Please CC me.

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html