lkml.org 
[lkml]   [2007]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [patch 6/13] signalfd/timerfd/asyncfd v5 - timerfd core ...
    From
    Date
    Davide,

    On Wed, 2007-03-14 at 15:19 -0700, Davide Libenzi wrote:

    > +static int timerfd_tmrproc(struct hrtimer *htmr)
    > +{
    > + struct timerfd_ctx *ctx = container_of(htmr, struct timerfd_ctx, tmr);
    > + int 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, htmr->base->softirq_time, ctx->tintv);

    Sorry, I missed that in the first reviews. Please use
    hrtimer_cb_get_time(htmr) instead of htmr->base->softirq_time, so this
    is high res timer safe.

    > + rval = HRTIMER_RESTART;
    > + }
    > + spin_unlock_irqrestore(&ctx->lock, flags);
    > +
    > + return rval;
    > +}
    > +
    > +
    > +static int timerfd_setup(struct timerfd_ctx *ctx, int clockid, int flags,
    > + const struct itimerspec *ktmr)
    > +{

    Make this void, returns 0 anyway

    > + enum hrtimer_mode htmode;
    > +
    > + htmode = (flags & TFD_TIMER_ABSTIME) ? HRTIMER_ABS: HRTIMER_REL;
    > +
    > + ctx->ticks = 0;
    > + ctx->clockid = clockid;
    > + ctx->flags = flags;
    > + ctx->texp = timespec_to_ktime(ktmr->it_value);

    clockid is stored in the timer on setup, so no need to store it again.
    expiry time and flags are not used after setup.

    Please remove those fields.

    > + ctx->tintv = timespec_to_ktime(ktmr->it_interval);
    > + hrtimer_init(&ctx->tmr, ctx->clockid, htmode);
    > + ctx->tmr.expires = ctx->texp;
    > + ctx->tmr.function = timerfd_tmrproc;
    > + if (ctx->texp.tv64 != 0)
    > + hrtimer_start(&ctx->tmr, ctx->texp, htmode);
    > +
    > + return 0;
    > +}
    > +
    > + if (ufd == -1) {
    > + ctx = kmem_cache_alloc(timerfd_ctx_cachep, GFP_KERNEL);
    > + if (!ctx)
    > + return -ENOMEM;
    > +
    > + init_waitqueue_head(&ctx->wqh);
    > + spin_lock_init(&ctx->lock);
    > + ctx->clockid = -1;
    > +
    > + error = timerfd_setup(ctx, clockid, flags, &ktmr);
    > + if (error)
    > + goto err_ctxfree;

    Timer setup can not fail

    > + /*
    > + * 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_ctxfree;

    Again: Please turn this around. No need to start the timer before we
    know, that everything works.

    > + } 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 ...
    > + */
    > + error = timerfd_setup(ctx, clockid, flags, &ktmr);

    Timer setup can not fail

    > + spin_unlock_irq(&ctx->lock);
    > + fput(file);
    > + if (error)
    > + return error;
    > + }
    > +
    > + return ufd;
    > +
    > +err_ctxfree:
    > + timerfd_cleanup(ctx);
    > + return error;
    > +}
    > +
    > +
    > +static void timerfd_cleanup(struct timerfd_ctx *ctx)
    > +{
    > + if (ctx->clockid >= 0)
    > + hrtimer_cancel(&ctx->tmr);

    You don't have a file descriptor, when the setup failed. So the timer is
    always initialized.

    > + kmem_cache_free(timerfd_ctx_cachep, ctx);
    > +}
    > +
    > +
    > +static int timerfd_close(struct inode *inode, struct file *file)
    > +{
    > + timerfd_cleanup(file->private_data);
    > + return 0;
    > +}
    > +

    Please move the timerfd_cleanup code into close().

    tglx


    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-03-15 22:55    [W:0.030 / U:0.152 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site