Message-Id: <20070330180910.dd16a52f.akpm@linux-foundation.org>
Date:	Fri, 30 Mar 2007 18:09:10 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Davide Libenzi <davidel@...ilserver.org>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [patch 6/13] signal/timer/event fds v8 - timerfd core ...

On Fri, 30 Mar 2007 17:47:28 -0700 (PDT) Davide Libenzi <davidel@...ilserver.org> wrote:

> 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.
> 

The decision is not that obvious - your patch's main use of
timerfd_ctx.lock is to provide locking for wqh - ie: to duplicate the
function of the existing lock which is there for that purpose.

So I think it's a legitimate optimisation to borrow it.

> 
> > > +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.
> 

Look at it from the point of view of someone who knows kernel code but does
not specifically know this subsystem.  That describes the great majority of
people who will be reading your code.

> 
> 
> > > +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 ;)
> 

To some extent, yes - there's a lot of material which is relevant to a
complex system call like this which isn't appropriate to code comments.

But a descrition of the role of texp.tv64 in here is an aid to
understanding the implementation and hence is appropriate and needed.

> 
> > > +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.
> 

A basic description of the inputs, outputs and return value is appropriate
to most high-level kernel funtions.  One here won't hurt.

> 
> 
> > 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.
> 

Patch headers are not maintainable.

Nobody wants to have to go off and waddle though the git repo to understand
the design intent behind each function.


Look, I'm just providing feedback as an experienced kernel developer who is
reading your code for the first time.  I had questions, and I saw things
which I felt were not adequately communicated.  You are the last person who
can judge what is obvious and what is not, because you already understand
it!

I do err on the make-it-easy-for-them side, but that's not a bad thing, I
think.  Very large numbers of people read core kernel code and the actual
change rate of this code will be low.  So we can afford to put the effort
into making these peoples' code-reading as productive as we can.


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

Powered by blists - more mailing lists