Re: [patch 1/3] new timerfd API - new timerfd API

[Thomas Gleixner <tglx@linutronix.de>]

Davide,

On Sun, 2007-09-23 at 15:49 -0700, Davide Libenzi wrote:
> This is the new timerfd API as it is implemented by the following patch:
> ---
>  fs/compat.c              |   32 ++++++-
>  fs/timerfd.c             |  199 ++++++++++++++++++++++++++++++-----------------
>  include/linux/compat.h   |    7 +
>  include/linux/syscalls.h |    7 +
>  4 files changed, 168 insertions(+), 77 deletions(-)
> 
> Index: linux-2.6.mod/fs/timerfd.c
> ===================================================================
> --- linux-2.6.mod.orig/fs/timerfd.c	2007-09-23 15:18:09.000000000 -0700
> +++ linux-2.6.mod/fs/timerfd.c	2007-09-23 15:25:55.000000000 -0700
> @@ -23,15 +23,17 @@
>  
>  struct timerfd_ctx {
>  	struct hrtimer tmr;
> +	int clockid;
>  	ktime_t tintv;
>  	wait_queue_head_t wqh;
>  	int expired;
> +	u64 ticks;
>  };

Can you please restructure the struct in a way which does not result in
padding by the compiler ?

struct timerfd_ctx {
	struct hrtimer tmr;
	ktime_t tintv;
	wait_queue_head_t wqh;
	u64 ticks;
	int expired;
	int clockid;
};

> +			ticks += (u64)
>  				hrtimer_forward(&ctx->tmr,
>  						hrtimer_cb_get_time(&ctx->tmr),

You need to use ctx->tmr.base->get_time() here, otherwise you might read
a stale time value (in case that CONFIG_HIGH_RES_TIMERS is off).

> -						ctx->tintv);
> +						ctx->tintv) - 1;
>  			hrtimer_restart(&ctx->tmr);
>
> +asmlinkage long sys_timerfd_create(int clockid)
>  {
> -	int error;
> +	int error, ufd;
>  	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;
> +
> +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	init_waitqueue_head(&ctx->wqh);
> +	ctx->clockid = clockid;
> +	hrtimer_init(&ctx->tmr, clockid, HRTIMER_MODE_ABS);
> +
> +	error = anon_inode_getfd(&ufd, &inode, &file, "[timerfd]",
> +				 &timerfd_fops, ctx);
> +	if (error)
> +		goto err_kfree_ctx;
> +
> +	return ufd;
> +
> +err_kfree_ctx:
> +	kfree(ctx);
> +	return error;

You really can avoid the goto here.

	tglx