Re: [patch 6/13] signal/timer/event fds v8 - timerfd core ...

[Davide Libenzi]

On Fri, 30 Mar 2007, Andrew Morton wrote:

> > +struct timerfd_ctx {
> > +	struct hrtimer tmr;
> > +	ktime_t tintv;
> > +	spinlock_t lock;
> > +	wait_queue_head_t wqh;
> > +	unsigned long ticks;
> > +};
> 
> Did you consider using the (presently unused) lock inside wqh instead of
> adding a new one?  That's a little bit rude, poking into waitqueue
> internals like that, but we do it elsewhere and tricks like that are
> acceptable in core-kernel, I guess.

Please, no. Gain is not worth the plug into the structure design IMO.



> I find that the key to understanding kernel code is to understand the data
> structures and the relationships between them.  Once you have that in your
> head, the code tends to just fall out.  Hence there is good maintainability
> payoff in putting work into documenting the struct, its fields, the
> relationship between this struct and other structs, and any and all locking
> requirements.
> 
> <wonders wtf "ticks" does>

Seemed obvious to me, but comment added.



> > +static enum hrtimer_restart timerfd_tmrproc(struct hrtimer *htmr);
> > +static void timerfd_setup(struct timerfd_ctx *ctx, int clockid, int flags,
> > +			  const struct itimerspec *ktmr);
> > +static int timerfd_close(struct inode *inode, struct file *file);
> > +static unsigned int timerfd_poll(struct file *file, poll_table *wait);
> > +static ssize_t timerfd_read(struct file *file, char __user *buf, size_t count,
> > +			    loff_t *ppos);
> 
> It'd be nice to find a way to make these declarations go away.

Gone.


> 
> > +
> > +
> > +
> 
> blankness.

You blank freak! :)



> > +static const struct file_operations timerfd_fops = {
> > +	.release	= timerfd_close,
> 
> Rename to timerfd_release

Done.




> > +static enum hrtimer_restart timerfd_tmrproc(struct hrtimer *htmr)
> > +{
> > +	struct timerfd_ctx *ctx = container_of(htmr, struct timerfd_ctx, tmr);
> > +	enum hrtimer_restart 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, hrtimer_cb_get_time(htmr), ctx->tintv);
> > +		rval = HRTIMER_RESTART;
> > +	}
> > +	spin_unlock_irqrestore(&ctx->lock, flags);
> > +
> > +	return rval;
> > +}
> 
> What's this do?

Really, do we need to comment such trivial code? There is *nothing* that 
is worth a line of comment in there. IMO useless comment are more annoying 
than blank lines.





> > +static void timerfd_setup(struct timerfd_ctx *ctx, int clockid, int flags,
> > +			  const struct itimerspec *ktmr)
> > +{
> > +	enum hrtimer_mode htmode;
> > +	ktime_t texp;
> > +
> > +	htmode = (flags & TFD_TIMER_ABSTIME) ? HRTIMER_MODE_ABS: HRTIMER_MODE_REL;
> > +
> > +	texp = timespec_to_ktime(ktmr->it_value);
> > +	ctx->ticks = 0;
> > +	ctx->tintv = timespec_to_ktime(ktmr->it_interval);
> > +	hrtimer_init(&ctx->tmr, clockid, htmode);
> > +	ctx->tmr.expires = texp;
> > +	ctx->tmr.function = timerfd_tmrproc;
> > +	if (texp.tv64 != 0)
> > +		hrtimer_start(&ctx->tmr, texp, htmode);
> > +}
> 
> What does the special case texp.tv64 == 0 signify?  Is that obvious to
> anyone who understands hrtimers?  Is it something which we can expect
> Micheal to immediately understand?  Should it be documented somewhere?

Michael should not read the code, but the patch description that comes 
with it ;)




> > +asmlinkage long sys_timerfd(int ufd, int clockid, int flags,
> > +			    const struct itimerspec __user *utmr)
> 
> Somehow we need to get from this to a manpage.

Again, the patch description describes (modulo returned errno's) the API 
pretty well.




> OK, this is briefly documented in the patch changelog.  That interface
> documentation should be fleshed out and moved into the .c file.  a) because
> it is easier to find and b) if we change it, it's a bit hard to go back and
> alter that changelog!

I think it's better to leave it out of the code, and keep it in the patch 
header.



> How come it's OK to truncate 64-bit timerfd_ctx.ticks to 32-bit like this?

2^32 ticks should be fine. I could make it a 64 bit thing, but IMO 32 bit 
is OK.



- Davide


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