[patch 6/13] signal/timer/event fds v8 - timerfd core ...

[patch 6/13] signal/timer/event fds v8 - 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 flags, const struct itimerspec *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 time specified in the "utmr->it_value" parameter is the expiry
time for the timer.
If the TFD_TIMER_ABSTIME flag is set in "flags", this is an absolute
time, otherwise it's a relative time.
If the time specified in the "utmr->it_interval" is not zero (.tv_sec == 0,
tv_nsec == 0), this is the period at which the following ticks should
be generated.
The "utmr->it_interval" should be set to zero if only one tick is requested.
Setting the "utmr->it_value" to zero will disable the timer, or will create
a timerfd without the timer enabled.
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.21-rc3.quilt/fs/timerfd.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.21-rc3.quilt/fs/timerfd.c	2007-03-19 19:01:52.000000000 -0700
@@ -0,0 +1,233 @@
+/*
+ *  fs/timerfd.c
+ *
+ *  Copyright (C) 2007  Davide Libenzi <davidel@xmailserver.org>
+ *
+ *
+ *  Thanks to Thomas Gleixner for code reviews 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;
+	ktime_t tintv;
+	spinlock_t lock;
+	wait_queue_head_t wqh;
+	unsigned long ticks;
+};
+
+
+static enum hrtimer_restart timerfd_tmrproc(struct hrtimer *htmr);
+static void timerfd_setup(struct timerfd_ctx *ctx, int clockid, int flags,
+			  const struct itimerspec *ktmr);
+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 enum hrtimer_restart timerfd_tmrproc(struct hrtimer *htmr)
+{
+	struct timerfd_ctx *ctx = container_of(htmr, struct timerfd_ctx, tmr);
+	enum hrtimer_restart rval = HRTIMER_NORESTART;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ctx->lock, flags);
+	ctx->ticks++;
+	wake_up_locked(&ctx->wqh);
+	if (ctx->tintv.tv64 != 0) {
+		hrtimer_forward(htmr, hrtimer_cb_get_time(htmr), ctx->tintv);
+		rval = HRTIMER_RESTART;
+	}
+	spin_unlock_irqrestore(&ctx->lock, flags);
+
+	return rval;
+}
+
+static void timerfd_setup(struct timerfd_ctx *ctx, int clockid, int flags,
+			  const struct itimerspec *ktmr)
+{
+	enum hrtimer_mode htmode;
+	ktime_t texp;
+
+	htmode = (flags & TFD_TIMER_ABSTIME) ? HRTIMER_MODE_ABS: HRTIMER_MODE_REL;
+
+	texp = timespec_to_ktime(ktmr->it_value);
+	ctx->ticks = 0;
+	ctx->tintv = timespec_to_ktime(ktmr->it_interval);
+	hrtimer_init(&ctx->tmr, clockid, htmode);
+	ctx->tmr.expires = texp;
+	ctx->tmr.function = timerfd_tmrproc;
+	if (texp.tv64 != 0)
+		hrtimer_start(&ctx->tmr, texp, htmode);
+}
+
+asmlinkage long sys_timerfd(int ufd, int clockid, int flags,
+			    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 (clockid != CLOCK_MONOTONIC &&
+	    clockid != CLOCK_REALTIME)
+		return -EINVAL;
+	if (!timespec_valid(&ktmr.it_value) ||
+	    !timespec_valid(&ktmr.it_interval))
+		return -EINVAL;
+
+	if (ufd == -1) {
+		ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
+		if (!ctx)
+			return -ENOMEM;
+
+		init_waitqueue_head(&ctx->wqh);
+		spin_lock_init(&ctx->lock);
+
+		timerfd_setup(ctx, clockid, flags, &ktmr);
+
+		/*
+		 * 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_tmrcancel;
+	} else {
+		file = fget(ufd);
+		if (!file)
+			return -EBADF;
+		ctx = file->private_data;
+		if (file->f_op != &timerfd_fops) {
+			fput(file);
+			return -EINVAL;
+		}
+		/*
+		 * We need to stop the existing timer before reprogramming
+		 * it to the new values.
+		 */
+		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 ...
+		 */
+		timerfd_setup(ctx, clockid, flags, &ktmr);
+
+		spin_unlock_irq(&ctx->lock);
+		fput(file);
+	}
+
+	return ufd;
+
+err_tmrcancel:
+	hrtimer_cancel(&ctx->tmr);
+	kfree(ctx);
+	return error;
+}
+
+static int timerfd_close(struct inode *inode, struct file *file)
+{
+	struct timerfd_ctx *ctx = file->private_data;
+
+	hrtimer_cancel(&ctx->tmr);
+	kfree(ctx);
+	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;
+}
+
Index: linux-2.6.21-rc3.quilt/include/linux/timerfd.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.21-rc3.quilt/include/linux/timerfd.h	2007-03-19 19:01:52.000000000 -0700
@@ -0,0 +1,17 @@
+/*
+ *  include/linux/timerfd.h
+ *
+ *  Copyright (C) 2007  Davide Libenzi <davidel@xmailserver.org>
+ *
+ */
+
+#ifndef _LINUX_TIMERFD_H
+#define _LINUX_TIMERFD_H
+
+
+#define TFD_TIMER_ABSTIME (1 << 0)
+
+
+
+#endif /* _LINUX_TIMERFD_H */
+
Index: linux-2.6.21-rc3.quilt/fs/Makefile
===================================================================
--- linux-2.6.21-rc3.quilt.orig/fs/Makefile	2007-03-19 19:01:43.000000000 -0700
+++ linux-2.6.21-rc3.quilt/fs/Makefile	2007-03-19 19:01:52.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.21-rc3.quilt/include/linux/syscalls.h
===================================================================
--- linux-2.6.21-rc3.quilt.orig/include/linux/syscalls.h	2007-03-19 19:01:43.000000000 -0700
+++ linux-2.6.21-rc3.quilt/include/linux/syscalls.h	2007-03-19 19:01:52.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 flags,
+			    const struct itimerspec __user *utmr);
 
 int kernel_execve(const char *filename, char *const argv[], char *const envp[]);
 

Re: [patch 6/13] signal/timer/event fds v8 - timerfd core ...

[Andrew Morton]

On Tue, 20 Mar 2007 11:37:14 -0700
Davide Libenzi <davidel@xmailserver.org> 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 flags, const struct itimerspec *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 time specified in the "utmr->it_value" parameter is the expiry
> time for the timer.
> If the TFD_TIMER_ABSTIME flag is set in "flags", this is an absolute
> time, otherwise it's a relative time.
> If the time specified in the "utmr->it_interval" is not zero (.tv_sec == 0,
> tv_nsec == 0), this is the period at which the following ticks should
> be generated.
> The "utmr->it_interval" should be set to zero if only one tick is requested.
> Setting the "utmr->it_value" to zero will disable the timer, or will create
> a timerfd without the timer enabled.
> 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
> 
> ...
>
> 
> Index: linux-2.6.21-rc3.quilt/fs/timerfd.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.21-rc3.quilt/fs/timerfd.c	2007-03-19 19:01:52.000000000 -0700
> @@ -0,0 +1,233 @@
> +/*
> + *  fs/timerfd.c
> + *
> + *  Copyright (C) 2007  Davide Libenzi <davidel@xmailserver.org>
> + *
> + *
> + *  Thanks to Thomas Gleixner for code reviews 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>
> +
> +
> +

I see blankness.

> +struct timerfd_ctx {
> +	struct hrtimer tmr;
> +	ktime_t tintv;
> +	spinlock_t lock;
> +	wait_queue_head_t wqh;
> +	unsigned long ticks;
> +};

Did you consider using the (presently unused) lock inside wqh instead of
adding a new one?  That's a little bit rude, poking into waitqueue
internals like that, but we do it elsewhere and tricks like that are
acceptable in core-kernel, I guess.

I find that the key to understanding kernel code is to understand the data
structures and the relationships between them.  Once you have that in your
head, the code tends to just fall out.  Hence there is good maintainability
payoff in putting work into documenting the struct, its fields, the
relationship between this struct and other structs, and any and all locking
requirements.

<wonders wtf "ticks" does>

> +
> +static enum hrtimer_restart timerfd_tmrproc(struct hrtimer *htmr);
> +static void timerfd_setup(struct timerfd_ctx *ctx, int clockid, int flags,
> +			  const struct itimerspec *ktmr);
> +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);

It'd be nice to find a way to make these declarations go away.

> +
> +
> +

blankness.

> +static const struct file_operations timerfd_fops = {
> +	.release	= timerfd_close,

Rename to timerfd_release

> +	.poll		= timerfd_poll,
> +	.read		= timerfd_read,
> +};
> +
> +
> +

scroll, scroll

> +static enum hrtimer_restart timerfd_tmrproc(struct hrtimer *htmr)
> +{
> +	struct timerfd_ctx *ctx = container_of(htmr, struct timerfd_ctx, tmr);
> +	enum hrtimer_restart rval = HRTIMER_NORESTART;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ctx->lock, flags);
> +	ctx->ticks++;
> +	wake_up_locked(&ctx->wqh);
> +	if (ctx->tintv.tv64 != 0) {
> +		hrtimer_forward(htmr, hrtimer_cb_get_time(htmr), ctx->tintv);
> +		rval = HRTIMER_RESTART;
> +	}
> +	spin_unlock_irqrestore(&ctx->lock, flags);
> +
> +	return rval;
> +}

What's this do?

> +static void timerfd_setup(struct timerfd_ctx *ctx, int clockid, int flags,
> +			  const struct itimerspec *ktmr)
> +{
> +	enum hrtimer_mode htmode;
> +	ktime_t texp;
> +
> +	htmode = (flags & TFD_TIMER_ABSTIME) ? HRTIMER_MODE_ABS: HRTIMER_MODE_REL;
> +
> +	texp = timespec_to_ktime(ktmr->it_value);
> +	ctx->ticks = 0;
> +	ctx->tintv = timespec_to_ktime(ktmr->it_interval);
> +	hrtimer_init(&ctx->tmr, clockid, htmode);
> +	ctx->tmr.expires = texp;
> +	ctx->tmr.function = timerfd_tmrproc;
> +	if (texp.tv64 != 0)
> +		hrtimer_start(&ctx->tmr, texp, htmode);
> +}

What does the special case texp.tv64 == 0 signify?  Is that obvious to
anyone who understands hrtimers?  Is it something which we can expect
Micheal to immediately understand?  Should it be documented somewhere?


> +asmlinkage long sys_timerfd(int ufd, int clockid, int flags,
> +			    const struct itimerspec __user *utmr)

Somehow we need to get from this to a manpage.

> +{
> +	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 (clockid != CLOCK_MONOTONIC &&
> +	    clockid != CLOCK_REALTIME)
> +		return -EINVAL;
> +	if (!timespec_valid(&ktmr.it_value) ||
> +	    !timespec_valid(&ktmr.it_interval))
> +		return -EINVAL;
> +
> +	if (ufd == -1) {
> +		ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
> +		if (!ctx)
> +			return -ENOMEM;
> +
> +		init_waitqueue_head(&ctx->wqh);
> +		spin_lock_init(&ctx->lock);
> +
> +		timerfd_setup(ctx, clockid, flags, &ktmr);
> +
> +		/*
> +		 * 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_tmrcancel;
> +	} else {
> +		file = fget(ufd);
> +		if (!file)
> +			return -EBADF;
> +		ctx = file->private_data;
> +		if (file->f_op != &timerfd_fops) {
> +			fput(file);
> +			return -EINVAL;
> +		}
> +		/*
> +		 * We need to stop the existing timer before reprogramming
> +		 * it to the new values.
> +		 */
> +		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 ...
> +		 */
> +		timerfd_setup(ctx, clockid, flags, &ktmr);
> +
> +		spin_unlock_irq(&ctx->lock);
> +		fput(file);
> +	}
> +
> +	return ufd;
> +
> +err_tmrcancel:
> +	hrtimer_cancel(&ctx->tmr);
> +	kfree(ctx);
> +	return error;
> +}
> +
>
> ...
>
> +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;
> +}

OK, this is briefly documented in the patch changelog.  That interface
documentation should be fleshed out and moved into the .c file.  a) because
it is easier to find and b) if we change it, it's a bit hard to go back and
alter that changelog!

How come it's OK to truncate 64-bit timerfd_ctx.ticks to 32-bit like this?

Re: [patch 6/13] signal/timer/event fds v8 - timerfd core ...

[Davide Libenzi]

On Fri, 30 Mar 2007, Andrew Morton wrote:

> > +struct timerfd_ctx {
> > +	struct hrtimer tmr;
> > +	ktime_t tintv;
> > +	spinlock_t lock;
> > +	wait_queue_head_t wqh;
> > +	unsigned long ticks;
> > +};
> 
> Did you consider using the (presently unused) lock inside wqh instead of
> adding a new one?  That's a little bit rude, poking into waitqueue
> internals like that, but we do it elsewhere and tricks like that are
> acceptable in core-kernel, I guess.

Please, no. Gain is not worth the plug into the structure design IMO.



> I find that the key to understanding kernel code is to understand the data
> structures and the relationships between them.  Once you have that in your
> head, the code tends to just fall out.  Hence there is good maintainability
> payoff in putting work into documenting the struct, its fields, the
> relationship between this struct and other structs, and any and all locking
> requirements.
> 
> <wonders wtf "ticks" does>

Seemed obvious to me, but comment added.



> > +static enum hrtimer_restart timerfd_tmrproc(struct hrtimer *htmr);
> > +static void timerfd_setup(struct timerfd_ctx *ctx, int clockid, int flags,
> > +			  const struct itimerspec *ktmr);
> > +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);
> 
> It'd be nice to find a way to make these declarations go away.

Gone.


> 
> > +
> > +
> > +
> 
> blankness.

You blank freak! :)



> > +static const struct file_operations timerfd_fops = {
> > +	.release	= timerfd_close,
> 
> Rename to timerfd_release

Done.




> > +static enum hrtimer_restart timerfd_tmrproc(struct hrtimer *htmr)
> > +{
> > +	struct timerfd_ctx *ctx = container_of(htmr, struct timerfd_ctx, tmr);
> > +	enum hrtimer_restart rval = HRTIMER_NORESTART;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&ctx->lock, flags);
> > +	ctx->ticks++;
> > +	wake_up_locked(&ctx->wqh);
> > +	if (ctx->tintv.tv64 != 0) {
> > +		hrtimer_forward(htmr, hrtimer_cb_get_time(htmr), ctx->tintv);
> > +		rval = HRTIMER_RESTART;
> > +	}
> > +	spin_unlock_irqrestore(&ctx->lock, flags);
> > +
> > +	return rval;
> > +}
> 
> What's this do?

Really, do we need to comment such trivial code? There is *nothing* that 
is worth a line of comment in there. IMO useless comment are more annoying 
than blank lines.





> > +static void timerfd_setup(struct timerfd_ctx *ctx, int clockid, int flags,
> > +			  const struct itimerspec *ktmr)
> > +{
> > +	enum hrtimer_mode htmode;
> > +	ktime_t texp;
> > +
> > +	htmode = (flags & TFD_TIMER_ABSTIME) ? HRTIMER_MODE_ABS: HRTIMER_MODE_REL;
> > +
> > +	texp = timespec_to_ktime(ktmr->it_value);
> > +	ctx->ticks = 0;
> > +	ctx->tintv = timespec_to_ktime(ktmr->it_interval);
> > +	hrtimer_init(&ctx->tmr, clockid, htmode);
> > +	ctx->tmr.expires = texp;
> > +	ctx->tmr.function = timerfd_tmrproc;
> > +	if (texp.tv64 != 0)
> > +		hrtimer_start(&ctx->tmr, texp, htmode);
> > +}
> 
> What does the special case texp.tv64 == 0 signify?  Is that obvious to
> anyone who understands hrtimers?  Is it something which we can expect
> Micheal to immediately understand?  Should it be documented somewhere?

Michael should not read the code, but the patch description that comes 
with it ;)




> > +asmlinkage long sys_timerfd(int ufd, int clockid, int flags,
> > +			    const struct itimerspec __user *utmr)
> 
> Somehow we need to get from this to a manpage.

Again, the patch description describes (modulo returned errno's) the API 
pretty well.




> OK, this is briefly documented in the patch changelog.  That interface
> documentation should be fleshed out and moved into the .c file.  a) because
> it is easier to find and b) if we change it, it's a bit hard to go back and
> alter that changelog!

I think it's better to leave it out of the code, and keep it in the patch 
header.



> How come it's OK to truncate 64-bit timerfd_ctx.ticks to 32-bit like this?

2^32 ticks should be fine. I could make it a 64 bit thing, but IMO 32 bit 
is OK.



- Davide

Re: [patch 6/13] signal/timer/event fds v8 - timerfd core ...

[Andrew Morton]

On Fri, 30 Mar 2007 17:47:28 -0700 (PDT) Davide Libenzi <davidel@xmailserver.org> wrote:

> On Fri, 30 Mar 2007, Andrew Morton wrote:
> 
> > > +struct timerfd_ctx {
> > > +	struct hrtimer tmr;
> > > +	ktime_t tintv;
> > > +	spinlock_t lock;
> > > +	wait_queue_head_t wqh;
> > > +	unsigned long ticks;
> > > +};
> > 
> > Did you consider using the (presently unused) lock inside wqh instead of
> > adding a new one?  That's a little bit rude, poking into waitqueue
> > internals like that, but we do it elsewhere and tricks like that are
> > acceptable in core-kernel, I guess.
> 
> Please, no. Gain is not worth the plug into the structure design IMO.
> 

The decision is not that obvious - your patch's main use of
timerfd_ctx.lock is to provide locking for wqh - ie: to duplicate the
function of the existing lock which is there for that purpose.

So I think it's a legitimate optimisation to borrow it.

> 
> > > +static enum hrtimer_restart timerfd_tmrproc(struct hrtimer *htmr)
> > > +{
> > > +	struct timerfd_ctx *ctx = container_of(htmr, struct timerfd_ctx, tmr);
> > > +	enum hrtimer_restart rval = HRTIMER_NORESTART;
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&ctx->lock, flags);
> > > +	ctx->ticks++;
> > > +	wake_up_locked(&ctx->wqh);
> > > +	if (ctx->tintv.tv64 != 0) {
> > > +		hrtimer_forward(htmr, hrtimer_cb_get_time(htmr), ctx->tintv);
> > > +		rval = HRTIMER_RESTART;
> > > +	}
> > > +	spin_unlock_irqrestore(&ctx->lock, flags);
> > > +
> > > +	return rval;
> > > +}
> > 
> > What's this do?
> 
> Really, do we need to comment such trivial code? There is *nothing* that 
> is worth a line of comment in there. IMO useless comment are more annoying 
> than blank lines.
> 

Look at it from the point of view of someone who knows kernel code but does
not specifically know this subsystem.  That describes the great majority of
people who will be reading your code.

> 
> 
> > > +static void timerfd_setup(struct timerfd_ctx *ctx, int clockid, int flags,
> > > +			  const struct itimerspec *ktmr)
> > > +{
> > > +	enum hrtimer_mode htmode;
> > > +	ktime_t texp;
> > > +
> > > +	htmode = (flags & TFD_TIMER_ABSTIME) ? HRTIMER_MODE_ABS: HRTIMER_MODE_REL;
> > > +
> > > +	texp = timespec_to_ktime(ktmr->it_value);
> > > +	ctx->ticks = 0;
> > > +	ctx->tintv = timespec_to_ktime(ktmr->it_interval);
> > > +	hrtimer_init(&ctx->tmr, clockid, htmode);
> > > +	ctx->tmr.expires = texp;
> > > +	ctx->tmr.function = timerfd_tmrproc;
> > > +	if (texp.tv64 != 0)
> > > +		hrtimer_start(&ctx->tmr, texp, htmode);
> > > +}
> > 
> > What does the special case texp.tv64 == 0 signify?  Is that obvious to
> > anyone who understands hrtimers?  Is it something which we can expect
> > Micheal to immediately understand?  Should it be documented somewhere?
> 
> Michael should not read the code, but the patch description that comes 
> with it ;)
> 

To some extent, yes - there's a lot of material which is relevant to a
complex system call like this which isn't appropriate to code comments.

But a descrition of the role of texp.tv64 in here is an aid to
understanding the implementation and hence is appropriate and needed.

> 
> > > +asmlinkage long sys_timerfd(int ufd, int clockid, int flags,
> > > +			    const struct itimerspec __user *utmr)
> > 
> > Somehow we need to get from this to a manpage.
> 
> Again, the patch description describes (modulo returned errno's) the API 
> pretty well.
> 

A basic description of the inputs, outputs and return value is appropriate
to most high-level kernel funtions.  One here won't hurt.

> 
> 
> > OK, this is briefly documented in the patch changelog.  That interface
> > documentation should be fleshed out and moved into the .c file.  a) because
> > it is easier to find and b) if we change it, it's a bit hard to go back and
> > alter that changelog!
> 
> I think it's better to leave it out of the code, and keep it in the patch 
> header.
> 

Patch headers are not maintainable.

Nobody wants to have to go off and waddle though the git repo to understand
the design intent behind each function.


Look, I'm just providing feedback as an experienced kernel developer who is
reading your code for the first time.  I had questions, and I saw things
which I felt were not adequately communicated.  You are the last person who
can judge what is obvious and what is not, because you already understand
it!

I do err on the make-it-easy-for-them side, but that's not a bad thing, I
think.  Very large numbers of people read core kernel code and the actual
change rate of this code will be low.  So we can afford to put the effort
into making these peoples' code-reading as productive as we can.

Re: [patch 6/13] signal/timer/event fds v8 - timerfd core ...

[Davide Libenzi]

On Fri, 30 Mar 2007, Andrew Morton wrote:

> On Fri, 30 Mar 2007 17:47:28 -0700 (PDT) Davide Libenzi <davidel@xmailserver.org> wrote:
> 
> > On Fri, 30 Mar 2007, Andrew Morton wrote:
> > 
> > > > +struct timerfd_ctx {
> > > > +	struct hrtimer tmr;
> > > > +	ktime_t tintv;
> > > > +	spinlock_t lock;
> > > > +	wait_queue_head_t wqh;
> > > > +	unsigned long ticks;
> > > > +};
> > > 
> > > Did you consider using the (presently unused) lock inside wqh instead of
> > > adding a new one?  That's a little bit rude, poking into waitqueue
> > > internals like that, but we do it elsewhere and tricks like that are
> > > acceptable in core-kernel, I guess.
> > 
> > Please, no. Gain is not worth the plug into the structure design IMO.
> > 
> 
> The decision is not that obvious - your patch's main use of
> timerfd_ctx.lock is to provide locking for wqh - ie: to duplicate the
> function of the existing lock which is there for that purpose.
> 
> So I think it's a legitimate optimisation to borrow it.

As I see it, the lock protects the ticks and the timer structure, and the 
wait queue *happen* to have a lock inside. That's the whole purpose in 
having data modeled by structures with accessor functions.
IMO, given that we'd get by plugging into the wait_queue_head_t lock is 
not much (if at all), it'd be better to to peek at the wait_queue_head_t 
directly.




> > > > +static enum hrtimer_restart timerfd_tmrproc(struct hrtimer *htmr)
> > > > +{
> > > > +	struct timerfd_ctx *ctx = container_of(htmr, struct timerfd_ctx, tmr);
> > > > +	enum hrtimer_restart rval = HRTIMER_NORESTART;
> > > > +	unsigned long flags;
> > > > +
> > > > +	spin_lock_irqsave(&ctx->lock, flags);
> > > > +	ctx->ticks++;
> > > > +	wake_up_locked(&ctx->wqh);
> > > > +	if (ctx->tintv.tv64 != 0) {
> > > > +		hrtimer_forward(htmr, hrtimer_cb_get_time(htmr), ctx->tintv);
> > > > +		rval = HRTIMER_RESTART;
> > > > +	}
> > > > +	spin_unlock_irqrestore(&ctx->lock, flags);
> > > > +
> > > > +	return rval;
> > > > +}
> > > 
> > > What's this do?
> > 
> > Really, do we need to comment such trivial code? There is *nothing* that 
> > is worth a line of comment in there. IMO useless comment are more annoying 
> > than blank lines.
> > 
> 
> Look at it from the point of view of someone who knows kernel code but does
> not specifically know this subsystem.  That describes the great majority of
> people who will be reading your code.

Okie, I added a comment ;)



> Patch headers are not maintainable.
> 
> Nobody wants to have to go off and waddle though the git repo to understand
> the design intent behind each function.

I agree. I thought we were talking about the "poor smuck" having to 
document it, and the patch header is a pretty good start (and maybe 
finish ;).



- Davide