lkml.org 
[lkml]   [2007]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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 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-16 00:09    [W:0.196 / U:0.392 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site