[patch 6/9] signalfd/timerfd v3 - timerfd core ...

[patch 6/9] signalfd/timerfd v3 - timerfd core ...

[Davide Libenzi]

This patch introduces a new system call for timers events delivered
though file descriptors. This allows timer event to be used with
standard POSIX poll(2), select(2) and read(2). As a consequence of
supporting the Linux f_op->poll subsystem, they can be used with
epoll(2) too.
The system call is defined as:

int timerfd(int ufd, int clockid, int tmrtype, const struct timespec *utmr);

The "ufd" parameter allows for re-use (re-programming) of an existing
timerfd w/out going through the close/open cycle (same as signalfd).
If "ufd" is -1, s new file descriptor will be created, otherwise the
existing "ufd" will be re-programmed.
The "clockid" parameter is either CLOCK_MONOTONIC or CLOCK_REALTIME.
The "tmrtype" parameter allows to specify the timer type. The following
values are supported:

    TFD_TIMER_REL
        The time specified in the "utmr" parameter is a relative time
	from NOW.

    TFD_TIMER_ABS
        The timer specified in the "utmr" parameter is an absolute time.

    TFD_TIMER_SEQ
        The time specified in the "utmr" parameter is an interval at
	which a continuous clock rate will be generated.

The function returns the new (or same, in case "ufd" is a valid timerfd
descriptor) file, or -1 in case of error.
As stated before, the timerfd file descriptor supports poll(2), select(2)
and epoll(2). When a timer event happened on the timerfd, a POLLIN mask
will be returned.
The read(2) call can be used, and it will return a u32 variable holding
the number of "ticks" that happened on the interface since the last call
to read(2). The read(2) call supportes the O_NONBLOCK flag too, and EAGAIN
will be returned if no ticks happened.
A quick test program, shows timerfd working correctly on my amd64 box:

http://www.xmailserver.org/timerfd-test.c




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



- Davide



Index: linux-2.6.20.ep2/fs/timerfd.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.20.ep2/fs/timerfd.c	2007-03-11 14:32:47.000000000 -0700
@@ -0,0 +1,295 @@
+/*
+ *  fs/timerfd.c
+ *
+ *  Copyright (C) 2007  Davide Libenzi <davidel@xmailserver.org>
+ *
+ *
+ *  Thanks to Thomas Gleixner for code review and useful comments.
+ *
+ */
+
+#include <linux/file.h>
+#include <linux/poll.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/fs.h>
+#include <linux/mount.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/signal.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/time.h>
+#include <linux/hrtimer.h>
+#include <linux/jiffies.h>
+#include <linux/anon_inodes.h>
+#include <linux/timerfd.h>
+
+#include <asm/uaccess.h>
+
+
+
+struct timerfd_ctx {
+	struct hrtimer tmr;
+	int clockid;
+	enum hrtimer_mode htmode;
+	ktime_t texp, tintv;
+	int tmrtype;
+	spinlock_t lock;
+	wait_queue_head_t wqh;
+	unsigned long ticks;
+};
+
+
+static int timerfd_tmrproc(struct hrtimer *htmr);
+static int timerfd_setup(struct timerfd_ctx *ctx, int clockid, int tmrtype,
+			 const struct itimerspec *ktmr);
+static void timerfd_cleanup(struct timerfd_ctx *ctx);
+static int timerfd_close(struct inode *inode, struct file *file);
+static unsigned int timerfd_poll(struct file *file, poll_table *wait);
+static ssize_t timerfd_read(struct file *file, char __user *buf, size_t count,
+			    loff_t *ppos);
+
+
+
+static const struct file_operations timerfd_fops = {
+	.release	= timerfd_close,
+	.poll		= timerfd_poll,
+	.read		= timerfd_read,
+};
+static struct kmem_cache *timerfd_ctx_cachep;
+
+
+
+static int timerfd_tmrproc(struct hrtimer *htmr)
+{
+	struct timerfd_ctx *ctx = container_of(htmr, struct timerfd_ctx, tmr);
+	int rval = HRTIMER_NORESTART;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ctx->lock, flags);
+	ctx->ticks++;
+	wake_up_locked(&ctx->wqh);
+	if (ctx->tmrtype == TFD_TIMER_SEQ) {
+		hrtimer_forward(htmr, htmr->base->softirq_time, ctx->tintv);
+		rval = HRTIMER_RESTART;
+	}
+	spin_unlock_irqrestore(&ctx->lock, flags);
+
+	return rval;
+}
+
+
+static int timerfd_setup(struct timerfd_ctx *ctx, int clockid, int tmrtype,
+			 const struct itimerspec *ktmr)
+{
+	enum hrtimer_mode htmode;
+	ktime_t texp, tintv;
+
+	if (clockid != CLOCK_MONOTONIC &&
+	    clockid != CLOCK_REALTIME)
+		return -EINVAL;
+	switch (tmrtype) {
+	case TFD_TIMER_SEQ:
+		if (!timespec_valid(&ktmr->it_interval))
+			return -EINVAL;
+		tintv = timespec_to_ktime(ktmr->it_interval);
+	case TFD_TIMER_ABS:
+		if (!timespec_valid(&ktmr->it_value))
+			return -EINVAL;
+		htmode = HRTIMER_ABS;
+		texp = timespec_to_ktime(ktmr->it_value);
+		break;
+	case TFD_TIMER_REL:
+		if (!timespec_valid(&ktmr->it_interval))
+			return -EINVAL;
+		texp = timespec_to_ktime(ktmr->it_interval);
+		tintv = ktime_set(0, 0);
+		htmode = HRTIMER_REL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ctx->ticks = 0;
+	ctx->tmrtype = tmrtype;
+	ctx->clockid = clockid;
+	ctx->htmode = htmode;
+	ctx->texp = texp;
+	ctx->tintv = tintv;
+	hrtimer_init(&ctx->tmr, ctx->clockid, htmode);
+	ctx->tmr.expires = ctx->texp;
+	ctx->tmr.function = timerfd_tmrproc;
+
+	hrtimer_start(&ctx->tmr, ctx->texp, htmode);
+
+	return 0;
+}
+
+
+asmlinkage long sys_timerfd(int ufd, int clockid, int tmrtype,
+			    const struct itimerspec __user *utmr)
+{
+	int error;
+	struct timerfd_ctx *ctx;
+	struct file *file;
+	struct inode *inode;
+	struct itimerspec ktmr;
+
+	if (copy_from_user(&ktmr, utmr, sizeof(ktmr)))
+		return -EFAULT;
+
+	if (ufd == -1) {
+		error = -ENOMEM;
+		ctx = kmem_cache_alloc(timerfd_ctx_cachep, GFP_KERNEL);
+		if (!ctx)
+			goto err_exit;
+
+		init_waitqueue_head(&ctx->wqh);
+		spin_lock_init(&ctx->lock);
+		ctx->clockid = -1;
+
+		error = timerfd_setup(ctx, clockid, tmrtype, &ktmr);
+		if (error)
+			goto err_ctxfree;
+
+		/*
+		 * When we call this, the initialization must be complete, since
+		 * aino_getfd() will install the fd.
+		 */
+		error = aino_getfd(&ufd, &inode, &file, "[timerfd]",
+				   &timerfd_fops, ctx);
+		if (error)
+			goto err_ctxfree;
+	} else {
+		error = -EBADF;
+		file = fget(ufd);
+		if (!file)
+			goto err_exit;
+		ctx = file->private_data;
+		error = -EINVAL;
+		if (file->f_op != &timerfd_fops) {
+			fput(file);
+			goto err_exit;
+		}
+		/*
+		 * We need to stop the exiting timer before.
+		 */
+		for (;;) {
+			spin_lock_irq(&ctx->lock);
+			if (hrtimer_try_to_cancel(&ctx->tmr) >= 0)
+				break;
+			spin_unlock_irq(&ctx->lock);
+			cpu_relax();
+		}
+		/*
+		 * Re-program the timer to the new value ...
+		 */
+		error = timerfd_setup(ctx, clockid, tmrtype, &ktmr);
+
+		spin_unlock_irq(&ctx->lock);
+		fput(file);
+		if (error)
+			goto err_exit;
+	}
+
+	return ufd;
+
+err_ctxfree:
+	timerfd_cleanup(ctx);
+err_exit:
+	return error;
+}
+
+
+static void timerfd_cleanup(struct timerfd_ctx *ctx)
+{
+	if (ctx->clockid >= 0)
+		hrtimer_cancel(&ctx->tmr);
+	kmem_cache_free(timerfd_ctx_cachep, ctx);
+}
+
+
+static int timerfd_close(struct inode *inode, struct file *file)
+{
+	timerfd_cleanup(file->private_data);
+	return 0;
+}
+
+
+static unsigned int timerfd_poll(struct file *file, poll_table *wait)
+{
+	struct timerfd_ctx *ctx = file->private_data;
+	unsigned int events = 0;
+	unsigned long flags;
+
+	poll_wait(file, &ctx->wqh, wait);
+
+	spin_lock_irqsave(&ctx->lock, flags);
+	if (ctx->ticks)
+		events |= POLLIN;
+	spin_unlock_irqrestore(&ctx->lock, flags);
+
+	return events;
+}
+
+
+static ssize_t timerfd_read(struct file *file, char __user *buf, size_t count,
+			    loff_t *ppos)
+{
+	struct timerfd_ctx *ctx = file->private_data;
+	ssize_t res;
+	u32 ticks;
+	DECLARE_WAITQUEUE(wait, current);
+
+	if (count < sizeof(ticks))
+		return -EINVAL;
+	spin_lock_irq(&ctx->lock);
+	res = -EAGAIN;
+	if ((ticks = (u32) ctx->ticks) == 0 &&
+	    !(file->f_flags & O_NONBLOCK)) {
+		__add_wait_queue(&ctx->wqh, &wait);
+		for (res = 0;;) {
+			set_current_state(TASK_INTERRUPTIBLE);
+			if ((ticks = (u32) ctx->ticks) != 0) {
+				res = 0;
+				break;
+			}
+			if (signal_pending(current)) {
+				res = -ERESTARTSYS;
+				break;
+			}
+			spin_unlock_irq(&ctx->lock);
+			schedule();
+			spin_lock_irq(&ctx->lock);
+		}
+		__remove_wait_queue(&ctx->wqh, &wait);
+		__set_current_state(TASK_RUNNING);
+	}
+	if (ticks)
+		ctx->ticks = 0;
+	spin_unlock_irq(&ctx->lock);
+	if (ticks)
+		res = put_user(ticks, buf) ? -EFAULT: sizeof(ticks);
+	return res;
+}
+
+
+static int __init timerfd_init(void)
+{
+	timerfd_ctx_cachep = kmem_cache_create("timerfd_ctx_cache",
+						sizeof(struct timerfd_ctx),
+						0, SLAB_PANIC, NULL, NULL);
+	return 0;
+}
+
+
+static void __exit timerfd_exit(void)
+{
+	kmem_cache_destroy(timerfd_ctx_cachep);
+}
+
+module_init(timerfd_init);
+module_exit(timerfd_exit);
+
+MODULE_LICENSE("GPL");
Index: linux-2.6.20.ep2/include/linux/timerfd.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.20.ep2/include/linux/timerfd.h	2007-03-11 14:28:51.000000000 -0700
@@ -0,0 +1,20 @@
+/*
+ *  include/linux/timerfd.h
+ *
+ *  Copyright (C) 2007  Davide Libenzi <davidel@xmailserver.org>
+ *
+ */
+
+#ifndef _LINUX_TIMERFD_H
+#define _LINUX_TIMERFD_H
+
+
+#define TFD_TIMER_REL 1
+#define TFD_TIMER_ABS 2
+#define TFD_TIMER_SEQ 3
+
+
+
+
+#endif /* _LINUX_TIMERFD_H */
+
Index: linux-2.6.20.ep2/fs/Makefile
===================================================================
--- linux-2.6.20.ep2.orig/fs/Makefile	2007-03-11 14:28:37.000000000 -0700
+++ linux-2.6.20.ep2/fs/Makefile	2007-03-11 14:28:51.000000000 -0700
@@ -11,7 +11,7 @@
 		attr.o bad_inode.o file.o filesystems.o namespace.o aio.o \
 		seq_file.o xattr.o libfs.o fs-writeback.o \
 		pnode.o drop_caches.o splice.o sync.o utimes.o \
-		stack.o anon_inodes.o signalfd.o
+		stack.o anon_inodes.o signalfd.o timerfd.o
 
 ifeq ($(CONFIG_BLOCK),y)
 obj-y +=	buffer.o bio.o block_dev.o direct-io.o mpage.o ioprio.o
Index: linux-2.6.20.ep2/include/linux/syscalls.h
===================================================================
--- linux-2.6.20.ep2.orig/include/linux/syscalls.h	2007-03-11 14:28:37.000000000 -0700
+++ linux-2.6.20.ep2/include/linux/syscalls.h	2007-03-11 14:28:51.000000000 -0700
@@ -603,6 +603,8 @@
 				    size_t len);
 asmlinkage long sys_getcpu(unsigned __user *cpu, unsigned __user *node, struct getcpu_cache __user *cache);
 asmlinkage long sys_signalfd(int ufd, sigset_t __user *user_mask, size_t sizemask);
+asmlinkage long sys_timerfd(int ufd, int clockid, int tmrtype,
+			    const struct itimerspec __user *utmr);
 
 int kernel_execve(const char *filename, char *const argv[], char *const envp[]);
 

Re: [patch 6/9] signalfd/timerfd v3 - timerfd core ...

[Davide Libenzi]

On Sun, 11 Mar 2007, Davide Libenzi wrote:

> This patch introduces a new system call for timers events delivered
> though file descriptors. This allows timer event to be used with
> standard POSIX poll(2), select(2) and read(2). As a consequence of
> supporting the Linux f_op->poll subsystem, they can be used with
> epoll(2) too.
> The system call is defined as:
> 
> int timerfd(int ufd, int clockid, int tmrtype, const struct timespec *utmr);
> 
> The "ufd" parameter allows for re-use (re-programming) of an existing
> timerfd w/out going through the close/open cycle (same as signalfd).
> If "ufd" is -1, s new file descriptor will be created, otherwise the
> existing "ufd" will be re-programmed.
> The "clockid" parameter is either CLOCK_MONOTONIC or CLOCK_REALTIME.
> The "tmrtype" parameter allows to specify the timer type. The following
> values are supported:
> 
>     TFD_TIMER_REL
>         The time specified in the "utmr" parameter is a relative time
> 	from NOW.
> 
>     TFD_TIMER_ABS
>         The timer specified in the "utmr" parameter is an absolute time.
> 
>     TFD_TIMER_SEQ
>         The time specified in the "utmr" parameter is an interval at
> 	which a continuous clock rate will be generated.
> 

Duh! Forgot to update the documenation. Now timerfd() gets an itimerspec.
For TFD_TIMER_REL only the it_interval is valid, and it's the relative 
time. For TFD_TIMER_ABS, only the it_value is valid, and that the expiry 
absolute time. For TFD_TIMER_SEQ, it_value tells when the first tick 
should be generated, and it_interval tells the period of the following 
ticks.



- Davide

Re: [patch 6/9] signalfd/timerfd v3 - timerfd core ...

[Nicholas Miell]

On Sun, 2007-03-11 at 16:13 -0700, Davide Libenzi wrote:
> On Sun, 11 Mar 2007, Davide Libenzi wrote:
> 
> > This patch introduces a new system call for timers events delivered
> > though file descriptors. This allows timer event to be used with
> > standard POSIX poll(2), select(2) and read(2). As a consequence of
> > supporting the Linux f_op->poll subsystem, they can be used with
> > epoll(2) too.
> > The system call is defined as:
> > 
> > int timerfd(int ufd, int clockid, int tmrtype, const struct timespec *utmr);
> > 
> > The "ufd" parameter allows for re-use (re-programming) of an existing
> > timerfd w/out going through the close/open cycle (same as signalfd).
> > If "ufd" is -1, s new file descriptor will be created, otherwise the
> > existing "ufd" will be re-programmed.
> > The "clockid" parameter is either CLOCK_MONOTONIC or CLOCK_REALTIME.
> > The "tmrtype" parameter allows to specify the timer type. The following
> > values are supported:
> > 
> >     TFD_TIMER_REL
> >         The time specified in the "utmr" parameter is a relative time
> > 	from NOW.
> > 
> >     TFD_TIMER_ABS
> >         The timer specified in the "utmr" parameter is an absolute time.
> > 
> >     TFD_TIMER_SEQ
> >         The time specified in the "utmr" parameter is an interval at
> > 	which a continuous clock rate will be generated.
> > 
> 
> Duh! Forgot to update the documenation. Now timerfd() gets an itimerspec.
> For TFD_TIMER_REL only the it_interval is valid, and it's the relative 
> time. For TFD_TIMER_ABS, only the it_value is valid, and that the expiry 
> absolute time. For TFD_TIMER_SEQ, it_value tells when the first tick 
> should be generated, and it_interval tells the period of the following 
> ticks.
> 

You should probably make it behave like the other things that use
itimerspec, just to avoid confusion -- i.e. timers are relative by
default, there's a flag that makes them absolute, they expire when
it_value specifies, and repeat every it_interval nanoseconds if
it_interval is non-zero.

i.e.

int timerfd(int ufd, int clockid, int flags, const struct timespec
*utmr);

with TFD_TIMER_ABS in flags making the timer absolute instead of
relative (and no TFD_TIMER_REL or TFD_TIMER_SEQ at all).

-- 
Nicholas Miell <nmiell@comcast.net>

Re: [patch 6/9] signalfd/timerfd v3 - timerfd core ...

[Nicholas Miell]

On Sun, 2007-03-11 at 16:50 -0700, Nicholas Miell wrote:
> You should probably make it behave like the other things that use
> itimerspec, just to avoid confusion -- i.e. timers are relative by
> default, there's a flag that makes them absolute, they expire when
> it_value specifies, and repeat every it_interval nanoseconds if
> it_interval is non-zero.
> 
> i.e.
> 
> int timerfd(int ufd, int clockid, int flags, const struct timespec
> *utmr);
> 
> with TFD_TIMER_ABS in flags making the timer absolute instead of
> relative (and no TFD_TIMER_REL or TFD_TIMER_SEQ at all).
> 

Sorry, that should be

int timerfd(int ufd, int clockid, int flags, const struct itimerspec
*utmr);

and TFD_TIMER_ABSTIME.

-- 
Nicholas Miell <nmiell@comcast.net>

Re: [patch 6/9] signalfd/timerfd v3 - timerfd core ...

[Davide Libenzi]

On Sun, 11 Mar 2007, Nicholas Miell wrote:

> You should probably make it behave like the other things that use
> itimerspec, just to avoid confusion -- i.e. timers are relative by
> default, there's a flag that makes them absolute, they expire when
> it_value specifies, and repeat every it_interval nanoseconds if
> it_interval is non-zero.
> 
> i.e.
> 
> int timerfd(int ufd, int clockid, int flags, const struct timespec
> *utmr);
> 
> with TFD_TIMER_ABS in flags making the timer absolute instead of
> relative (and no TFD_TIMER_REL or TFD_TIMER_SEQ at all).

Sounds sane to me. Will do...


- Davide

Re: [patch 6/9] signalfd/timerfd v3 - timerfd core ...

[Thomas Gleixner]

Davide,

On Sun, 2007-03-11 at 16:04 -0700, Davide Libenzi wrote:
> +static int timerfd_setup(struct timerfd_ctx *ctx, int clockid, int tmrtype,
> +			 const struct itimerspec *ktmr)
> +{
> +	enum hrtimer_mode htmode;
> +	ktime_t texp, tintv;
> +
> +	if (clockid != CLOCK_MONOTONIC &&
> +	    clockid != CLOCK_REALTIME)
> +		return -EINVAL;

Please move the validation for clockid, tmrtype and the timerspec into
sys_timerfd. Do it before anything else. Also please validate both
it_value and it_interval unconditionally. Userspace should not send
uninitialized stuff at all.

The TFD_TIMER_SEQ thing is quite different to all other timer interfaces
which POSIX provides. Both itimers and posixtimers use the it_interval
value to distinguish between one shot and periodic timers.

I think we should keep this new interface analogous, so programmers
don't get more confused, than they are already. :)

This also allows relative and absolute starting points for both one shot
and sequential timers.

Please use it_value == 0 to stop the timer. This is the same as for
itimers and posixtimers. Right now you have to close the fd to stop a
timer, but that's not necessarily what you want. 

Why do you want to store information, which is only relevant for setup
in ctx ?

If you do the validation right in sys_timerfd and get rid of
TFD_TIMER_SEQ and the various useless fields, then timerfd_setup() boils
down to

	ctx->ticks = 0;
	ctx->tintv = tintv;
	hrtimer_init(&ctx->tmr, clockid, htmode);
	ctx->tmr.function = timerfd_tmrproc;

	if (texp.tv64 != 0)
		hrtimer_start(&ctx->tmr, texp, htmode);

and in the timer function you simply check for

	if (ctx->tintv.tv64 != 0) 

instead of the TIMER_SEQ mode.

> +asmlinkage long sys_timerfd(int ufd, int clockid, int tmrtype,
> +			    const struct itimerspec __user *utmr)
> +{
> +	int error;
> +	struct timerfd_ctx *ctx;
> +	struct file *file;
> +	struct inode *inode;
> +	struct itimerspec ktmr;
> +
> +	if (copy_from_user(&ktmr, utmr, sizeof(ktmr)))
> +		return -EFAULT;

Do validation of clockid, tmrtype and ktmr here.

> +	if (ufd == -1) {
> +		error = -ENOMEM;
> +		ctx = kmem_cache_alloc(timerfd_ctx_cachep, GFP_KERNEL);
> +		if (!ctx)
> +			goto err_exit;

	return -ENOMEM;

> +		init_waitqueue_head(&ctx->wqh);
> +		spin_lock_init(&ctx->lock);
> +		ctx->clockid = -1;
> +
> +		error = timerfd_setup(ctx, clockid, tmrtype, &ktmr);
> +		if (error)
> +			goto err_ctxfree;
> +
> +		/*
> +		 * When we call this, the initialization must be complete, since
> +		 * aino_getfd() will install the fd.
> +		 */
> +		error = aino_getfd(&ufd, &inode, &file, "[timerfd]",
> +				   &timerfd_fops, ctx);
> +		if (error)
> +			goto err_ctxfree;
> +	} else {
> +		error = -EBADF;
> +		file = fget(ufd);
> +		if (!file)
> +			goto err_exit;

	return -EBADF;

> +		ctx = file->private_data;
> +		error = -EINVAL;
> +		if (file->f_op != &timerfd_fops) {
> +			fput(file);
> +			goto err_exit;

	return -EINVAL;

> +		}
> +		/*
> +		 * We need to stop the exiting timer before.
> +		 */

	-ENOPARSE. You probably mean: We need to stop an already running timer
before we do a new setup.

> +		for (;;) {
> +			spin_lock_irq(&ctx->lock);
> +			if (hrtimer_try_to_cancel(&ctx->tmr) >= 0)
> +				break;
> +			spin_unlock_irq(&ctx->lock);
> +			cpu_relax();
> +		}
> +		/*
> +		 * Re-program the timer to the new value ...
> +		 */
> +		error = timerfd_setup(ctx, clockid, tmrtype, &ktmr);
> +
> +		spin_unlock_irq(&ctx->lock);
> +		fput(file);
> +		if (error)
> +			goto err_exit;

		return error;

> +	}
> +
> +	return ufd;
> +
> +err_ctxfree:
> +	timerfd_cleanup(ctx);
> +err_exit:
> +	return error;
> +}
> +

	tglx

Re: [patch 6/9] signalfd/timerfd v3 - timerfd core ...

[Davide Libenzi]

On Mon, 12 Mar 2007, Thomas Gleixner wrote:

> Davide,
> 
> On Sun, 2007-03-11 at 16:04 -0700, Davide Libenzi wrote:
> > +static int timerfd_setup(struct timerfd_ctx *ctx, int clockid, int tmrtype,
> > +			 const struct itimerspec *ktmr)
> > +{
> > +	enum hrtimer_mode htmode;
> > +	ktime_t texp, tintv;
> > +
> > +	if (clockid != CLOCK_MONOTONIC &&
> > +	    clockid != CLOCK_REALTIME)
> > +		return -EINVAL;
> 
> Please move the validation for clockid, tmrtype and the timerspec into
> sys_timerfd. Do it before anything else. Also please validate both
> it_value and it_interval unconditionally. Userspace should not send
> uninitialized stuff at all.

Ok.


> The TFD_TIMER_SEQ thing is quite different to all other timer interfaces
> which POSIX provides. Both itimers and posixtimers use the it_interval
> value to distinguish between one shot and periodic timers.
> 
> I think we should keep this new interface analogous, so programmers
> don't get more confused, than they are already. :)

Yeah, this was already suggested by  Nicholas Miell and was already in my 
code.



> This also allows relative and absolute starting points for both one shot
> and sequential timers.
> 
> Please use it_value == 0 to stop the timer. This is the same as for
> itimers and posixtimers. Right now you have to close the fd to stop a
> timer, but that's not necessarily what you want. 

This too.



> If you do the validation right in sys_timerfd and get rid of
> TFD_TIMER_SEQ and the various useless fields, then timerfd_setup() boils
> down to
> 
> 	ctx->ticks = 0;
> 	ctx->tintv = tintv;
> 	hrtimer_init(&ctx->tmr, clockid, htmode);
> 	ctx->tmr.function = timerfd_tmrproc;
> 
> 	if (texp.tv64 != 0)
> 		hrtimer_start(&ctx->tmr, texp, htmode);
> 
> and in the timer function you simply check for
> 
> 	if (ctx->tintv.tv64 != 0) 
> 
> instead of the TIMER_SEQ mode.

This was done too, although I did not know I could reference the tv64 
member directly, so I had a macro checking the timespec.



> > +	if (ufd == -1) {
> > +		error = -ENOMEM;
> > +		ctx = kmem_cache_alloc(timerfd_ctx_cachep, GFP_KERNEL);
> > +		if (!ctx)
> > +			goto err_exit;
> 
> 	return -ENOMEM;

Ok.



> > +		}
> > +		/*
> > +		 * We need to stop the exiting timer before.
> > +		 */
> 
> 	-ENOPARSE. You probably mean: We need to stop an already running timer
> before we do a new setup.

Bad description :)



- Davide

Re: [patch 6/9] signalfd/timerfd v3 - timerfd core ...

[Davide Libenzi]

On Mon, 12 Mar 2007, Davide Libenzi wrote:

> On Mon, 12 Mar 2007, Thomas Gleixner wrote:
> 
> > Davide,
> > 
> > On Sun, 2007-03-11 at 16:04 -0700, Davide Libenzi wrote:
> > > +static int timerfd_setup(struct timerfd_ctx *ctx, int clockid, int tmrtype,
> > > +			 const struct itimerspec *ktmr)
> > > +{
> > > +	enum hrtimer_mode htmode;
> > > +	ktime_t texp, tintv;
> > > +
> > > +	if (clockid != CLOCK_MONOTONIC &&
> > > +	    clockid != CLOCK_REALTIME)
> > > +		return -EINVAL;

Maybe a function in hrtimer.c/h like hrtimer_valid_clockid(clockid_t id) 
to avoid bolting in clockid_t values in timerfd.c?



- Davide