Why does timerfd() only support CLOCK_REALTIME and CLOCK_MONOTONIC?

Why does timerfd() only support CLOCK_REALTIME and CLOCK_MONOTONIC?

[Michael Kerrisk]

Hi Davide,

At the moment I'm looking into writing man pages for timer_create(2)
and friends.  (Somewhat bizarrely, these pages do not yet exist.)  As
I looked into the source code of timer_create(), etc., and did some
tests, I saw that timer_create() supports the following clocks:

TIMER_REALTIME
TIMER_MONOTONIC
TIMER_PROCESS_CPUTIME_ID
TIMER_THREAD_CPUTIME_ID
clockid obtained from clock_getcpuclockid(3)
clockid obtained from pthread_getcpuclockid(3)

On the other hand, timerfd() only permits the first two of these.
What's the reason for that limitation of timerfd()?  (It may be worth
adding something to the man page on this point.)

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: Why does timerfd() only support CLOCK_REALTIME and CLOCK_MONOTONIC?

[Davide Libenzi]

On Mon, 9 Feb 2009, Michael Kerrisk wrote:

> Hi Davide,
> 
> At the moment I'm looking into writing man pages for timer_create(2)
> and friends.  (Somewhat bizarrely, these pages do not yet exist.)  As
> I looked into the source code of timer_create(), etc., and did some
> tests, I saw that timer_create() supports the following clocks:
> 
> TIMER_REALTIME
> TIMER_MONOTONIC
> TIMER_PROCESS_CPUTIME_ID
> TIMER_THREAD_CPUTIME_ID
> clockid obtained from clock_getcpuclockid(3)
> clockid obtained from pthread_getcpuclockid(3)
> 
> On the other hand, timerfd() only permits the first two of these.
> What's the reason for that limitation of timerfd()?  (It may be worth
> adding something to the man page on this point.)

No particular reason I can think of. If Thomas makes invalid_clockid() 
available, we could allow timerfd() to support all time timers 
timer_create() supports.
Do you see any reason why this won't work Thomas?



- Davide

Re: Why does timerfd() only support CLOCK_REALTIME and CLOCK_MONOTONIC?

[Davide Libenzi]

On Sun, 8 Feb 2009, Davide Libenzi wrote:

> On Mon, 9 Feb 2009, Michael Kerrisk wrote:
> 
> > Hi Davide,
> > 
> > At the moment I'm looking into writing man pages for timer_create(2)
> > and friends.  (Somewhat bizarrely, these pages do not yet exist.)  As
> > I looked into the source code of timer_create(), etc., and did some
> > tests, I saw that timer_create() supports the following clocks:
> > 
> > TIMER_REALTIME
> > TIMER_MONOTONIC
> > TIMER_PROCESS_CPUTIME_ID
> > TIMER_THREAD_CPUTIME_ID
> > clockid obtained from clock_getcpuclockid(3)
> > clockid obtained from pthread_getcpuclockid(3)
> > 
> > On the other hand, timerfd() only permits the first two of these.
> > What's the reason for that limitation of timerfd()?  (It may be worth
> > adding something to the man page on this point.)
> 
> No particular reason I can think of. If Thomas makes invalid_clockid() 
> available, we could allow timerfd() to support all time timers 
> timer_create() supports.
> Do you see any reason why this won't work Thomas?

Patch, not tested, below.



- Davide


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

Index: linux-2.6.mod/fs/timerfd.c
===================================================================
--- linux-2.6.mod.orig/fs/timerfd.c	2009-02-08 12:32:25.000000000 -0800
+++ linux-2.6.mod/fs/timerfd.c	2009-02-08 12:43:57.000000000 -0800
@@ -18,6 +18,7 @@
 #include <linux/spinlock.h>
 #include <linux/time.h>
 #include <linux/hrtimer.h>
+#include <linux/posix-timers.h>
 #include <linux/anon_inodes.h>
 #include <linux/timerfd.h>
 #include <linux/syscalls.h>
@@ -186,12 +187,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
 	BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
 	BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
 
-	if (flags & ~(TFD_CLOEXEC | TFD_NONBLOCK))
+	if ((flags & ~TFD_FLAGS_SET) ||
+	    invalid_clockid(clockid))
 		return -EINVAL;
-	if (clockid != CLOCK_MONOTONIC &&
-	    clockid != CLOCK_REALTIME)
-		return -EINVAL;
-
 	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
@@ -201,7 +199,7 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
 	hrtimer_init(&ctx->tmr, clockid, HRTIMER_MODE_ABS);
 
 	ufd = anon_inode_getfd("[timerfd]", &timerfd_fops, ctx,
-			       flags & (O_CLOEXEC | O_NONBLOCK));
+			       flags & TFD_SHARED_FCNTL_FLAGS);
 	if (ufd < 0)
 		kfree(ctx);
 
Index: linux-2.6.mod/include/linux/posix-timers.h
===================================================================
--- linux-2.6.mod.orig/include/linux/posix-timers.h	2009-02-08 12:32:25.000000000 -0800
+++ linux-2.6.mod/include/linux/posix-timers.h	2009-02-08 12:33:12.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 12:32:25.000000000 -0800
+++ linux-2.6.mod/kernel/posix-timers.c	2009-02-08 12:32:57.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;
Index: linux-2.6.mod/include/linux/timerfd.h
===================================================================
--- linux-2.6.mod.orig/include/linux/timerfd.h	2009-02-08 12:34:25.000000000 -0800
+++ linux-2.6.mod/include/linux/timerfd.h	2009-02-08 12:36:09.000000000 -0800
@@ -11,13 +11,20 @@
 /* For O_CLOEXEC and O_NONBLOCK */
 #include <linux/fcntl.h>
 
-/* Flags for timerfd_settime.  */
+/*
+ * CAREFUL: Check include/asm-generic/fcntl.h when defining
+ * new flags, since they might collide with O_* ones. We want
+ * to re-use O_* flags that couldn't possibly have a meaning
+ * from eventfd, in order to leave a free define-space for
+ * shared O_* flags.
+ */
 #define TFD_TIMER_ABSTIME (1 << 0)
-
-/* Flags for timerfd_create.  */
 #define TFD_CLOEXEC O_CLOEXEC
 #define TFD_NONBLOCK O_NONBLOCK
 
+#define TFD_SHARED_FCNTL_FLAGS (O_CLOEXEC | O_NONBLOCK)
+#define TFD_FLAGS_SET (TFD_SHARED_FCNTL_FLAGS | TFD_TIMER_ABSTIME)
+
 
 #endif /* _LINUX_TIMERFD_H */
 

Re: Why does timerfd() only support CLOCK_REALTIME and CLOCK_MONOTONIC?

[Michael Kerrisk]

On Mon, Feb 9, 2009 at 9:46 AM, Davide Libenzi <davidel@xmailserver.org> wrote:
> On Sun, 8 Feb 2009, Davide Libenzi wrote:
>
>> On Mon, 9 Feb 2009, Michael Kerrisk wrote:
>>
>> > Hi Davide,
>> >
>> > At the moment I'm looking into writing man pages for timer_create(2)
>> > and friends.  (Somewhat bizarrely, these pages do not yet exist.)  As
>> > I looked into the source code of timer_create(), etc., and did some
>> > tests, I saw that timer_create() supports the following clocks:
>> >
>> > TIMER_REALTIME
>> > TIMER_MONOTONIC
>> > TIMER_PROCESS_CPUTIME_ID
>> > TIMER_THREAD_CPUTIME_ID
>> > clockid obtained from clock_getcpuclockid(3)
>> > clockid obtained from pthread_getcpuclockid(3)
>> >
>> > On the other hand, timerfd() only permits the first two of these.
>> > What's the reason for that limitation of timerfd()?  (It may be worth
>> > adding something to the man page on this point.)
>>
>> No particular reason I can think of. If Thomas makes invalid_clockid()
>> available, we could allow timerfd() to support all time timers
>> timer_create() supports.
>> Do you see any reason why this won't work Thomas?
>
> Patch, not tested, below.

(Has it been compiled?)

A further comment below.

> ---
>  fs/timerfd.c                 |   10 ++++------
>  include/linux/posix-timers.h |    1 +
>  include/linux/timerfd.h      |   13 ++++++++++---
>  kernel/posix-timers.c        |    2 +-
>  4 files changed, 16 insertions(+), 10 deletions(-)
>
> Index: linux-2.6.mod/fs/timerfd.c
> ===================================================================
> --- linux-2.6.mod.orig/fs/timerfd.c     2009-02-08 12:32:25.000000000 -0800
> +++ linux-2.6.mod/fs/timerfd.c  2009-02-08 12:43:57.000000000 -0800
> @@ -18,6 +18,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/time.h>
>  #include <linux/hrtimer.h>
> +#include <linux/posix-timers.h>
>  #include <linux/anon_inodes.h>
>  #include <linux/timerfd.h>
>  #include <linux/syscalls.h>
> @@ -186,12 +187,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
>        BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
>        BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
>
> -       if (flags & ~(TFD_CLOEXEC | TFD_NONBLOCK))
> +       if ((flags & ~TFD_FLAGS_SET) ||
> +           invalid_clockid(clockid))
>                return -EINVAL;
> -       if (clockid != CLOCK_MONOTONIC &&
> -           clockid != CLOCK_REALTIME)
> -               return -EINVAL;
> -
>        ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>        if (!ctx)
>                return -ENOMEM;
> @@ -201,7 +199,7 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
>        hrtimer_init(&ctx->tmr, clockid, HRTIMER_MODE_ABS);
>
>        ufd = anon_inode_getfd("[timerfd]", &timerfd_fops, ctx,
> -                              flags & (O_CLOEXEC | O_NONBLOCK));
> +                              flags & TFD_SHARED_FCNTL_FLAGS);
>        if (ufd < 0)
>                kfree(ctx);
>
> Index: linux-2.6.mod/include/linux/posix-timers.h
> ===================================================================
> --- linux-2.6.mod.orig/include/linux/posix-timers.h     2009-02-08 12:32:25.000000000 -0800
> +++ linux-2.6.mod/include/linux/posix-timers.h  2009-02-08 12:33:12.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 12:32:25.000000000 -0800
> +++ linux-2.6.mod/kernel/posix-timers.c 2009-02-08 12:32:57.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;
> Index: linux-2.6.mod/include/linux/timerfd.h
> ===================================================================
> --- linux-2.6.mod.orig/include/linux/timerfd.h  2009-02-08 12:34:25.000000000 -0800
> +++ linux-2.6.mod/include/linux/timerfd.h       2009-02-08 12:36:09.000000000 -0800
> @@ -11,13 +11,20 @@
>  /* For O_CLOEXEC and O_NONBLOCK */
>  #include <linux/fcntl.h>
>
> -/* Flags for timerfd_settime.  */
> +/*
> + * CAREFUL: Check include/asm-generic/fcntl.h when defining
> + * new flags, since they might collide with O_* ones. We want
> + * to re-use O_* flags that couldn't possibly have a meaning
> + * from eventfd, in order to leave a free define-space for
> + * shared O_* flags.
> + */
>  #define TFD_TIMER_ABSTIME (1 << 0)
> -
> -/* Flags for timerfd_create.  */
>  #define TFD_CLOEXEC O_CLOEXEC
>  #define TFD_NONBLOCK O_NONBLOCK
>
> +#define TFD_SHARED_FCNTL_FLAGS (O_CLOEXEC | O_NONBLOCK)

s/O_/TFD_/g above

> +#define TFD_FLAGS_SET (TFD_SHARED_FCNTL_FLAGS | TFD_TIMER_ABSTIME)
> +
>
>  #endif /* _LINUX_TIMERFD_H */

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: Why does timerfd() only support CLOCK_REALTIME and CLOCK_MONOTONIC?

[Davide Libenzi]

On Mon, 9 Feb 2009, Michael Kerrisk wrote:

> On Mon, Feb 9, 2009 at 9:46 AM, Davide Libenzi <davidel@xmailserver.org> wrote:
> > On Sun, 8 Feb 2009, Davide Libenzi wrote:
> >
> >> On Mon, 9 Feb 2009, Michael Kerrisk wrote:
> >>
> >> > Hi Davide,
> >> >
> >> > At the moment I'm looking into writing man pages for timer_create(2)
> >> > and friends.  (Somewhat bizarrely, these pages do not yet exist.)  As
> >> > I looked into the source code of timer_create(), etc., and did some
> >> > tests, I saw that timer_create() supports the following clocks:
> >> >
> >> > TIMER_REALTIME
> >> > TIMER_MONOTONIC
> >> > TIMER_PROCESS_CPUTIME_ID
> >> > TIMER_THREAD_CPUTIME_ID
> >> > clockid obtained from clock_getcpuclockid(3)
> >> > clockid obtained from pthread_getcpuclockid(3)
> >> >
> >> > On the other hand, timerfd() only permits the first two of these.
> >> > What's the reason for that limitation of timerfd()?  (It may be worth
> >> > adding something to the man page on this point.)
> >>
> >> No particular reason I can think of. If Thomas makes invalid_clockid()
> >> available, we could allow timerfd() to support all time timers
> >> timer_create() supports.
> >> Do you see any reason why this won't work Thomas?
> >
> > Patch, not tested, below.
> 
> (Has it been compiled?)

Yes, it builds fine. I just haven't had time to test it ATM...



- Davide

Re: Why does timerfd() only support CLOCK_REALTIME and CLOCK_MONOTONIC?

[Michael Kerrisk]

>> > Patch, not tested, below.
>>
>> (Has it been compiled?)
>
> Yes, it builds fine. I just haven't had time to test it ATM...

Ok.

Did you see my other comment in the previous mail?



-- 
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: Why does timerfd() only support CLOCK_REALTIME and CLOCK_MONOTONIC?

[Michael Kerrisk]

> @@ -186,12 +187,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
>        BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
>        BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
>
> -       if (flags & ~(TFD_CLOEXEC | TFD_NONBLOCK))
> +       if ((flags & ~TFD_FLAGS_SET) ||
> +           invalid_clockid(clockid))
>                return -EINVAL;

Oh!  Does this mean that in 2.6.2[789] it wasn't possible to use
TFD_TIMER_ABSTIME?

-- 
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: Why does timerfd() only support CLOCK_REALTIME and CLOCK_MONOTONIC?

[Davide Libenzi]

On Mon, 9 Feb 2009, Michael Kerrisk wrote:

> > @@ -186,12 +187,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
> >        BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
> >        BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
> >
> > -       if (flags & ~(TFD_CLOEXEC | TFD_NONBLOCK))
> > +       if ((flags & ~TFD_FLAGS_SET) ||
> > +           invalid_clockid(clockid))
> >                return -EINVAL;
> 
> Oh!  Does this mean that in 2.6.2[789] it wasn't possible to use
> TFD_TIMER_ABSTIME?

No, sorry, my fault. Patch is wrong. In "create" ATM we accept only 
FCNTL-like flags. In "settime" we get TFD_TIMER_ABSTIME (that needs a 
check too for EINVAL).



- Davide


---
 fs/timerfd.c                 |   13 ++++++-------
 include/linux/posix-timers.h |    1 +
 include/linux/timerfd.h      |   15 ++++++++++++---
 kernel/posix-timers.c        |    2 +-
 4 files changed, 20 insertions(+), 11 deletions(-)

Index: linux-2.6.mod/fs/timerfd.c
===================================================================
--- linux-2.6.mod.orig/fs/timerfd.c	2009-02-08 12:32:25.000000000 -0800
+++ linux-2.6.mod/fs/timerfd.c	2009-02-08 13:44:47.000000000 -0800
@@ -18,6 +18,7 @@
 #include <linux/spinlock.h>
 #include <linux/time.h>
 #include <linux/hrtimer.h>
+#include <linux/posix-timers.h>
 #include <linux/anon_inodes.h>
 #include <linux/timerfd.h>
 #include <linux/syscalls.h>
@@ -186,12 +187,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
 	BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
 	BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
 
-	if (flags & ~(TFD_CLOEXEC | TFD_NONBLOCK))
+	if ((flags & ~TFD_CREATE_FLAGS) ||
+	    invalid_clockid(clockid))
 		return -EINVAL;
-	if (clockid != CLOCK_MONOTONIC &&
-	    clockid != CLOCK_REALTIME)
-		return -EINVAL;
-
 	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
@@ -201,7 +199,7 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
 	hrtimer_init(&ctx->tmr, clockid, HRTIMER_MODE_ABS);
 
 	ufd = anon_inode_getfd("[timerfd]", &timerfd_fops, ctx,
-			       flags & (O_CLOEXEC | O_NONBLOCK));
+			       flags & TFD_SHARED_FCNTL_FLAGS);
 	if (ufd < 0)
 		kfree(ctx);
 
@@ -219,7 +217,8 @@ SYSCALL_DEFINE4(timerfd_settime, int, uf
 	if (copy_from_user(&ktmr, utmr, sizeof(ktmr)))
 		return -EFAULT;
 
-	if (!timespec_valid(&ktmr.it_value) ||
+	if ((flags & ~TFD_SETTIME_FLAGS) ||
+	    !timespec_valid(&ktmr.it_value) ||
 	    !timespec_valid(&ktmr.it_interval))
 		return -EINVAL;
 
Index: linux-2.6.mod/include/linux/posix-timers.h
===================================================================
--- linux-2.6.mod.orig/include/linux/posix-timers.h	2009-02-08 12:32:25.000000000 -0800
+++ linux-2.6.mod/include/linux/posix-timers.h	2009-02-08 12:33:12.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 12:32:25.000000000 -0800
+++ linux-2.6.mod/kernel/posix-timers.c	2009-02-08 12:32:57.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;
Index: linux-2.6.mod/include/linux/timerfd.h
===================================================================
--- linux-2.6.mod.orig/include/linux/timerfd.h	2009-02-08 12:34:25.000000000 -0800
+++ linux-2.6.mod/include/linux/timerfd.h	2009-02-08 13:47:57.000000000 -0800
@@ -11,13 +11,22 @@
 /* For O_CLOEXEC and O_NONBLOCK */
 #include <linux/fcntl.h>
 
-/* Flags for timerfd_settime.  */
+/*
+ * CAREFUL: Check include/asm-generic/fcntl.h when defining
+ * new flags, since they might collide with O_* ones. We want
+ * to re-use O_* flags that couldn't possibly have a meaning
+ * from eventfd, in order to leave a free define-space for
+ * shared O_* flags.
+ */
 #define TFD_TIMER_ABSTIME (1 << 0)
-
-/* Flags for timerfd_create.  */
 #define TFD_CLOEXEC O_CLOEXEC
 #define TFD_NONBLOCK O_NONBLOCK
 
+#define TFD_SHARED_FCNTL_FLAGS (TFD_CLOEXEC | TFD_NONBLOCK)
+#define TFD_CREATE_FLAGS TFD_SHARED_FCNTL_FLAGS
+#define TFD_SETTIME_FLAGS TFD_TIMER_ABSTIME
+#define TFD_FLAGS_SET (TFD_SHARED_FCNTL_FLAGS | TFD_TIMER_ABSTIME)
+
 
 #endif /* _LINUX_TIMERFD_H */
 

Re: Why does timerfd() only support CLOCK_REALTIME and CLOCK_MONOTONIC?

[Michael Kerrisk]

On Mon, Feb 9, 2009 at 10:50 AM, Davide Libenzi <davidel@xmailserver.org> wrote:
> On Mon, 9 Feb 2009, Michael Kerrisk wrote:
>
>> > @@ -186,12 +187,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
>> >        BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
>> >        BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
>> >
>> > -       if (flags & ~(TFD_CLOEXEC | TFD_NONBLOCK))
>> > +       if ((flags & ~TFD_FLAGS_SET) ||
>> > +           invalid_clockid(clockid))
>> >                return -EINVAL;
>>
>> Oh!  Does this mean that in 2.6.2[789] it wasn't possible to use
>> TFD_TIMER_ABSTIME?
>
> No, sorry, my fault. Patch is wrong. In "create" ATM we accept only
> FCNTL-like flags. In "settime" we get TFD_TIMER_ABSTIME (that needs a
> check too for EINVAL).

That last piece should be a separate patch, that also gets pushed back
into -stable.  Do you agree?

M

> ---
>  fs/timerfd.c                 |   13 ++++++-------
>  include/linux/posix-timers.h |    1 +
>  include/linux/timerfd.h      |   15 ++++++++++++---
>  kernel/posix-timers.c        |    2 +-
>  4 files changed, 20 insertions(+), 11 deletions(-)
>
> Index: linux-2.6.mod/fs/timerfd.c
> ===================================================================
> --- linux-2.6.mod.orig/fs/timerfd.c     2009-02-08 12:32:25.000000000 -0800
> +++ linux-2.6.mod/fs/timerfd.c  2009-02-08 13:44:47.000000000 -0800
> @@ -18,6 +18,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/time.h>
>  #include <linux/hrtimer.h>
> +#include <linux/posix-timers.h>
>  #include <linux/anon_inodes.h>
>  #include <linux/timerfd.h>
>  #include <linux/syscalls.h>
> @@ -186,12 +187,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
>        BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
>        BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
>
> -       if (flags & ~(TFD_CLOEXEC | TFD_NONBLOCK))
> +       if ((flags & ~TFD_CREATE_FLAGS) ||
> +           invalid_clockid(clockid))
>                return -EINVAL;
> -       if (clockid != CLOCK_MONOTONIC &&
> -           clockid != CLOCK_REALTIME)
> -               return -EINVAL;
> -
>        ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>        if (!ctx)
>                return -ENOMEM;
> @@ -201,7 +199,7 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
>        hrtimer_init(&ctx->tmr, clockid, HRTIMER_MODE_ABS);
>
>        ufd = anon_inode_getfd("[timerfd]", &timerfd_fops, ctx,
> -                              flags & (O_CLOEXEC | O_NONBLOCK));
> +                              flags & TFD_SHARED_FCNTL_FLAGS);
>        if (ufd < 0)
>                kfree(ctx);
>
> @@ -219,7 +217,8 @@ SYSCALL_DEFINE4(timerfd_settime, int, uf
>        if (copy_from_user(&ktmr, utmr, sizeof(ktmr)))
>                return -EFAULT;
>
> -       if (!timespec_valid(&ktmr.it_value) ||
> +       if ((flags & ~TFD_SETTIME_FLAGS) ||
> +           !timespec_valid(&ktmr.it_value) ||
>            !timespec_valid(&ktmr.it_interval))
>                return -EINVAL;
>
> Index: linux-2.6.mod/include/linux/posix-timers.h
> ===================================================================
> --- linux-2.6.mod.orig/include/linux/posix-timers.h     2009-02-08 12:32:25.000000000 -0800
> +++ linux-2.6.mod/include/linux/posix-timers.h  2009-02-08 12:33:12.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 12:32:25.000000000 -0800
> +++ linux-2.6.mod/kernel/posix-timers.c 2009-02-08 12:32:57.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;
> Index: linux-2.6.mod/include/linux/timerfd.h
> ===================================================================
> --- linux-2.6.mod.orig/include/linux/timerfd.h  2009-02-08 12:34:25.000000000 -0800
> +++ linux-2.6.mod/include/linux/timerfd.h       2009-02-08 13:47:57.000000000 -0800
> @@ -11,13 +11,22 @@
>  /* For O_CLOEXEC and O_NONBLOCK */
>  #include <linux/fcntl.h>
>
> -/* Flags for timerfd_settime.  */
> +/*
> + * CAREFUL: Check include/asm-generic/fcntl.h when defining
> + * new flags, since they might collide with O_* ones. We want
> + * to re-use O_* flags that couldn't possibly have a meaning
> + * from eventfd, in order to leave a free define-space for
> + * shared O_* flags.
> + */
>  #define TFD_TIMER_ABSTIME (1 << 0)
> -
> -/* Flags for timerfd_create.  */
>  #define TFD_CLOEXEC O_CLOEXEC
>  #define TFD_NONBLOCK O_NONBLOCK
>
> +#define TFD_SHARED_FCNTL_FLAGS (TFD_CLOEXEC | TFD_NONBLOCK)
> +#define TFD_CREATE_FLAGS TFD_SHARED_FCNTL_FLAGS
> +#define TFD_SETTIME_FLAGS TFD_TIMER_ABSTIME
> +#define TFD_FLAGS_SET (TFD_SHARED_FCNTL_FLAGS | TFD_TIMER_ABSTIME)
> +
>
>  #endif /* _LINUX_TIMERFD_H */
>
>



-- 
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: Why does timerfd() only support CLOCK_REALTIME and CLOCK_MONOTONIC?

[Davide Libenzi]

On Mon, 9 Feb 2009, Michael Kerrisk wrote:

> On Mon, Feb 9, 2009 at 10:50 AM, Davide Libenzi <davidel@xmailserver.org> wrote:
> > On Mon, 9 Feb 2009, Michael Kerrisk wrote:
> >
> >> > @@ -186,12 +187,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
> >> >        BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
> >> >        BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
> >> >
> >> > -       if (flags & ~(TFD_CLOEXEC | TFD_NONBLOCK))
> >> > +       if ((flags & ~TFD_FLAGS_SET) ||
> >> > +           invalid_clockid(clockid))
> >> >                return -EINVAL;
> >>
> >> Oh!  Does this mean that in 2.6.2[789] it wasn't possible to use
> >> TFD_TIMER_ABSTIME?
> >
> > No, sorry, my fault. Patch is wrong. In "create" ATM we accept only
> > FCNTL-like flags. In "settime" we get TFD_TIMER_ABSTIME (that needs a
> > check too for EINVAL).
> 
> That last piece should be a separate patch, that also gets pushed back
> into -stable.  Do you agree?

Hmm, it's a check for extra bits that do not cause any harm. Dunno if it 
fits -stable requirements. You should ask Greg.



- Davide

Re: [stable] Why does timerfd() only support CLOCK_REALTIME and CLOCK_MONOTONIC?

[Greg KH]

On Sun, Feb 08, 2009 at 02:06:34PM -0800, Davide Libenzi wrote:
> On Mon, 9 Feb 2009, Michael Kerrisk wrote:
> 
> > On Mon, Feb 9, 2009 at 10:50 AM, Davide Libenzi <davidel@xmailserver.org> wrote:
> > > On Mon, 9 Feb 2009, Michael Kerrisk wrote:
> > >
> > >> > @@ -186,12 +187,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
> > >> >        BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
> > >> >        BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
> > >> >
> > >> > -       if (flags & ~(TFD_CLOEXEC | TFD_NONBLOCK))
> > >> > +       if ((flags & ~TFD_FLAGS_SET) ||
> > >> > +           invalid_clockid(clockid))
> > >> >                return -EINVAL;
> > >>
> > >> Oh!  Does this mean that in 2.6.2[789] it wasn't possible to use
> > >> TFD_TIMER_ABSTIME?
> > >
> > > No, sorry, my fault. Patch is wrong. In "create" ATM we accept only
> > > FCNTL-like flags. In "settime" we get TFD_TIMER_ABSTIME (that needs a
> > > check too for EINVAL).
> > 
> > That last piece should be a separate patch, that also gets pushed back
> > into -stable.  Do you agree?
> 
> Hmm, it's a check for extra bits that do not cause any harm. Dunno if it 
> fits -stable requirements. You should ask Greg.

If it fixes a bug, it would go into -stable.  Does this?

thanks,

greg k-h

Re: [stable] Why does timerfd() only support CLOCK_REALTIME and CLOCK_MONOTONIC?

[Michael Kerrisk]

On Mon, Feb 9, 2009 at 11:10 AM, Greg KH <greg@kroah.com> wrote:
> On Sun, Feb 08, 2009 at 02:06:34PM -0800, Davide Libenzi wrote:
>> On Mon, 9 Feb 2009, Michael Kerrisk wrote:
>>
>> > On Mon, Feb 9, 2009 at 10:50 AM, Davide Libenzi <davidel@xmailserver.org> wrote:
>> > > On Mon, 9 Feb 2009, Michael Kerrisk wrote:
>> > >
>> > >> > @@ -186,12 +187,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
>> > >> >        BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
>> > >> >        BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
>> > >> >
>> > >> > -       if (flags & ~(TFD_CLOEXEC | TFD_NONBLOCK))
>> > >> > +       if ((flags & ~TFD_FLAGS_SET) ||
>> > >> > +           invalid_clockid(clockid))
>> > >> >                return -EINVAL;
>> > >>
>> > >> Oh!  Does this mean that in 2.6.2[789] it wasn't possible to use
>> > >> TFD_TIMER_ABSTIME?
>> > >
>> > > No, sorry, my fault. Patch is wrong. In "create" ATM we accept only
>> > > FCNTL-like flags. In "settime" we get TFD_TIMER_ABSTIME (that needs a
>> > > check too for EINVAL).
>> >
>> > That last piece should be a separate patch, that also gets pushed back
>> > into -stable.  Do you agree?
>>
>> Hmm, it's a check for extra bits that do not cause any harm. Dunno if it
>> fits -stable requirements. You should ask Greg.
>
> If it fixes a bug, it would go into -stable.  Does this?

Depends on the definition of bug, I guess ;-).

The issue is this:

a) timerfd_create() (added in kernel 2.6.25) has a flags argument.
b) Currently, one such flag is supported: TFD_TIMER_ABSTIME
c) Eventually, we may add other flag bits to the flags argument.
d) Currently, timerfd_settime() does not check for invalid values in
its flags argument (i.e., bits other than TFD_TIMER_ABSTIME)
e) If/when new flag bits are added, then applications would like to
have a way of determining whether there is support in a given kernel,
something like:

s = timerfd_settime(fd, TFD_XXX, newval, currval);
if (s == 1 && errno == EINVAL)
    printf("TFD_XXX isn't supported by this kernel\n");

Davide, I think, proposes to add the missing check d) in kernel 2.6.29
(or 30).   But, IMO, checks like the above are going to be more useful
to userspace the further back in kernel time that we can push them.
So, it's a philosophical kind of bug.  Your call, I guess.

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: Why does timerfd() only support CLOCK_REALTIME and CLOCK_MONOTONIC?

[Michael Kerrisk]

On Mon, Feb 9, 2009 at 11:06 AM, Davide Libenzi <davidel@xmailserver.org> wrote:
> On Mon, 9 Feb 2009, Michael Kerrisk wrote:
>
>> On Mon, Feb 9, 2009 at 10:50 AM, Davide Libenzi <davidel@xmailserver.org> wrote:
>> > On Mon, 9 Feb 2009, Michael Kerrisk wrote:
>> >
>> >> > @@ -186,12 +187,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
>> >> >        BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
>> >> >        BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
>> >> >
>> >> > -       if (flags & ~(TFD_CLOEXEC | TFD_NONBLOCK))
>> >> > +       if ((flags & ~TFD_FLAGS_SET) ||
>> >> > +           invalid_clockid(clockid))
>> >> >                return -EINVAL;
>> >>
>> >> Oh!  Does this mean that in 2.6.2[789] it wasn't possible to use
>> >> TFD_TIMER_ABSTIME?
>> >
>> > No, sorry, my fault. Patch is wrong. In "create" ATM we accept only
>> > FCNTL-like flags. In "settime" we get TFD_TIMER_ABSTIME (that needs a
>> > check too for EINVAL).
>>
>> That last piece should be a separate patch, that also gets pushed back
>> into -stable.  Do you agree?
>
> Hmm, it's a check for extra bits that do not cause any harm. Dunno if it
> fits -stable requirements. You should ask Greg.

I agree that it's not a critical fix.  The point is of course that if
other flags are evntually added to timerfd_settime(), an application
needs a way of checking for kernel support / lack of support (a la the
recent discussion of eventfd "[patch/rfc] eventfd semaphore-like
behavior").  It seems to me it would be good to have that check work
as far back as possible in older kernels. If that check is to be added
to 2.6.29, it seems reasonable to push it back to the current -stable
kernels.

Greg?


-- 
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: Why does timerfd() only support CLOCK_REALTIME and CLOCK_MONOTONIC?

[Greg KH]

On Mon, Feb 09, 2009 at 11:12:21AM +1300, Michael Kerrisk wrote:
> On Mon, Feb 9, 2009 at 11:06 AM, Davide Libenzi <davidel@xmailserver.org> wrote:
> > On Mon, 9 Feb 2009, Michael Kerrisk wrote:
> >
> >> On Mon, Feb 9, 2009 at 10:50 AM, Davide Libenzi <davidel@xmailserver.org> wrote:
> >> > On Mon, 9 Feb 2009, Michael Kerrisk wrote:
> >> >
> >> >> > @@ -186,12 +187,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
> >> >> >        BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
> >> >> >        BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
> >> >> >
> >> >> > -       if (flags & ~(TFD_CLOEXEC | TFD_NONBLOCK))
> >> >> > +       if ((flags & ~TFD_FLAGS_SET) ||
> >> >> > +           invalid_clockid(clockid))
> >> >> >                return -EINVAL;
> >> >>
> >> >> Oh!  Does this mean that in 2.6.2[789] it wasn't possible to use
> >> >> TFD_TIMER_ABSTIME?
> >> >
> >> > No, sorry, my fault. Patch is wrong. In "create" ATM we accept only
> >> > FCNTL-like flags. In "settime" we get TFD_TIMER_ABSTIME (that needs a
> >> > check too for EINVAL).
> >>
> >> That last piece should be a separate patch, that also gets pushed back
> >> into -stable.  Do you agree?
> >
> > Hmm, it's a check for extra bits that do not cause any harm. Dunno if it
> > fits -stable requirements. You should ask Greg.
> 
> I agree that it's not a critical fix.  The point is of course that if
> other flags are evntually added to timerfd_settime(), an application
> needs a way of checking for kernel support / lack of support (a la the
> recent discussion of eventfd "[patch/rfc] eventfd semaphore-like
> behavior").  It seems to me it would be good to have that check work
> as far back as possible in older kernels. If that check is to be added
> to 2.6.29, it seems reasonable to push it back to the current -stable
> kernels.
> 
> Greg?

Sounds reasonable to me.

thanks,

greg k-h