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;
> + 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();
> + 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.
> + break;
> + default:
> + goto err_exit;
> + }
> +
> + 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->ticks = 0;
> + ctx->tmrtype = tmrtype;
> + ctx->clockid = clockid;
> + ctx->tval = tval;
> + hrtimer_init(&ctx->tmr, ctx->clockid, HRTIMER_REL);
> + ctx->tmr.expires = ctx->tval;
> + ctx->tmr.function = timerfd_tmrproc;
> +
> + 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 ?
Also if you turn it around then the (re)programming part of the timer
can be shared.
> + } 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. We call
> + * hrtimer_cancel() w/out holding our lock.
> + */
> + spin_lock_irq(&ctx->lock);
> + while (hrtimer_active(&ctx->tmr)) {
> + spin_unlock_irq(&ctx->lock);
> + hrtimer_cancel(&ctx->tmr);
> + spin_lock_irq(&ctx->lock);
> + }
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;
}
> +
> +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 !
tglx
-
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/
