Re: [patch 6/13] signalfd/timerfd/asyncfd v5 - timerfd core ...
On Thu, 2007-03-15 at 16:02 -0700, Davide Libenzi wrote: > > > + /* > > > + * When we call this, the initialization must be complete, since > > > + * aino_getfd() will install the fd. > > > + */ > > > + error = aino_getfd(, , , "[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. > > The timerfd_setup() is not locked, so we need to make sure everything is > setup, before advertising the fd (and aino_getfd does that). Right. Did not think about the bad boys peeking at file descriptors :) 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/
Re: [patch 6/13] signalfd/timerfd/asyncfd v5 - timerfd core ...
On Thu, 2007-03-15 at 16:02 -0700, Davide Libenzi wrote: + /* + * 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. The timerfd_setup() is not locked, so we need to make sure everything is setup, before advertising the fd (and aino_getfd does that). Right. Did not think about the bad boys peeking at file descriptors :) 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/
Re: [patch 6/13] signalfd/timerfd/asyncfd v5 - timerfd core ...
On Thu, 15 Mar 2007, Thomas Gleixner wrote: > 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(>lock, flags); > > + ctx->ticks++; > > + wake_up_locked(>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. Heh, I was actually looking for a function instead of peeking over the tiemr strcture, but 2.6.20 did not have. Rebased over 2.6.21-rc3 now, so I can use it. > > + rval = HRTIMER_RESTART; > > + } > > + spin_unlock_irqrestore(>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 Ack > > + 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. Ack > > + if (ufd == -1) { > > + ctx = kmem_cache_alloc(timerfd_ctx_cachep, GFP_KERNEL); > > + if (!ctx) > > + return -ENOMEM; > > + > > + init_waitqueue_head(>wqh); > > + spin_lock_init(>lock); > > + ctx->clockid = -1; > > + > > + error = timerfd_setup(ctx, clockid, flags, ); > > + if (error) > > + goto err_ctxfree; > > Timer setup can not fail Ack, the new version can't. > > + /* > > +* When we call this, the initialization must be complete, since > > +* aino_getfd() will install the fd. > > +*/ > > + error = aino_getfd(, , , "[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. The timerfd_setup() is not locked, so we need to make sure everything is setup, before advertising the fd (and aino_getfd does that). > > + 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(). I usually prefer to have a cleanup function that works on the file's data, but I moved the code in the release function now. Thx for the review! I'll repost a new version based on 2.6.21-rc3 ... - 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/
Re: [patch 6/13] signalfd/timerfd/asyncfd v5 - timerfd core ...
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(>lock, flags); > + ctx->ticks++; > + wake_up_locked(>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(>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(>tmr, ctx->clockid, htmode); > + ctx->tmr.expires = ctx->texp; > + ctx->tmr.function = timerfd_tmrproc; > + if (ctx->texp.tv64 != 0) > + hrtimer_start(>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(>wqh); > + spin_lock_init(>lock); > + ctx->clockid = -1; > + > + error = timerfd_setup(ctx, clockid, flags, ); > + 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(, , , "[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 != _fops) { > + fput(file); > + return -EINVAL; > + } > + /* > + * We need to stop the existing timer before reprogramming > + * it to the new values. > + */ > + for (;;) { > + spin_lock_irq(>lock); > + if (hrtimer_try_to_cancel(>tmr) >= 0) > + break; > + spin_unlock_irq(>lock); > + cpu_relax(); > + } > + /* > + * Re-program the timer to the new value ... > + */ > + error = timerfd_setup(ctx, clockid, flags, ); Timer setup can not fail > + spin_unlock_irq(>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(>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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 6/13] signalfd/timerfd/asyncfd v5 - timerfd core ...
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 6/13] signalfd/timerfd/asyncfd v5 - timerfd core ...
On Thu, 15 Mar 2007, Thomas Gleixner wrote: 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. Heh, I was actually looking for a function instead of peeking over the tiemr strcture, but 2.6.20 did not have. Rebased over 2.6.21-rc3 now, so I can use it. + 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 Ack + 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. Ack + 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 Ack, the new version can't. + /* +* 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. The timerfd_setup() is not locked, so we need to make sure everything is setup, before advertising the fd (and aino_getfd does that). + 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(). I usually prefer to have a cleanup function that works on the file's data, but I moved the code in the release function now. Thx for the review! I'll repost a new version based on 2.6.21-rc3 ... - 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/