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

[Davide Libenzi <davidel@xmailserver.org>]

On Sun, 11 Mar 2007, Thomas Gleixner wrote:

> Davide,
> 
> On Sat, 2007-03-10 at 18:22 -0800, Davide Libenzi wrote:
> 
> Some remarks:
> 
> > +
> > +asmlinkage long sys_timerfd(int ufd, int clockid, int tmrtype,
> > +			    const struct timespec __user *utmr)
> > +{
> > +	int error;
> > +	struct timerfd_ctx *ctx;
> > +	struct file *file;
> > +	struct inode *inode;
> > +	ktime_t tval, tnow;
> > +	struct timespec ktmr, tmrnow;
> > +
> > +	error = -EFAULT;
> > +	if (copy_from_user(&ktmr, utmr, sizeof(ktmr)))
> > +		goto err_exit;
> 
> Please do not use goto for a simple
> 		return -EFAULT;
> 
> Please validate the timespec before converting it.
> 
>         if (!timespec_valid(&ktmr))
>                 return -EINVAL;

Ack.


> > +	tval = timespec_to_ktime(ktmr);
> > +	error = -EINVAL;
> > +	if (clockid != CLOCK_MONOTONIC &&
> > +	    clockid != CLOCK_REALTIME)
> > +		goto err_exit;
> > +	switch (tmrtype) {
> > +	case TFD_TIMER_REL:
> > +	case TFD_TIMER_SEQ:
> > +		break;
> > +	case TFD_TIMER_ABS:
> > +		getnstimeofday(&tmrnow);
> > +		tnow = timespec_to_ktime(tmrnow);
> 
> 		tnow = ktime_get();

Ok, I think this is the wierd function that is declared static, whose 
symbol is exported, but is not declared in any .h file :)
I used that before, because I saw it inside the hrtimer.c file, but then 
gcc was puking on me, and I noticd the wierdness.



> > +		if (ktime_to_ns(tval) <= ktime_to_ns(tnow))
> > +			goto err_exit;
> > +		tval = ktime_sub(tval, tnow);
> 
> Why do you want to do that ? hrtimers handle relative and absolute
> expiry times. You break down everything to relative time and lose the
> accuracy for absolute timers. 

Yes. Those was in need of fixing. The first code I had was not working 
correctly with abs timers. Didn't have time to dig into it yet.
Will verify and fix today...



> > +
> > +		hrtimer_start(&ctx->tmr, ctx->tval, HRTIMER_REL);
> > +
> > +		/*
> > +		 * 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_fdalloc;
> 
> Why is the timer started before we have everything in place ? 

I simplify the error path. The fd does not need to be in place for the 
timer function to be correctly triggered.


> Also if you turn it around then the (re)programming part of the timer
> can be shared.

The two error/exit paths are different. One need to free the ctx, while 
the other one simply to do an fput().



> Please use hrtimer_try_to_cancel()
> 
> retry:
> 	spin_lock_irq(....):
> 	if (hrtimer_try_to_cancel(&ctx->tmr) < 0) {
> 		spin_unlock_irq();
> 		cpu_relax();
> 		goto retry;
> 	}

Ok, I will.



> > +static unsigned int timerfd_poll(struct file *file, poll_table *wait)
> > +{
> > +	struct timerfd_ctx *ctx = file->private_data;
> > +
> > +	poll_wait(file, &ctx->wqh, wait);
> > +
> > +	return ctx->ticks ? POLLIN: 0;
> 
> This is racy:
> 
> 	timer is set up (non periodic)
> 	timer expires
> 	poll 
> 
> 	now poll is stuck for ever !

Duh, yeah. I use the locked version of wakeups. Will fix.



- Davide