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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
