On Fri, 30 Mar 2007, Andrew Morton wrote:
> On Fri, 30 Mar 2007 17:47:28 -0700 (PDT) Davide Libenzi
> <davidel@xmailserver.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.
As I see it, the lock protects the ticks and the timer structure, and the
wait queue *happen* to have a lock inside. That's the whole purpose in
having data modeled by structures with accessor functions.
IMO, given that we'd get by plugging into the wait_queue_head_t lock is
not much (if at all), it'd be better to to peek at the wait_queue_head_t
directly.
> > > > +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.
Okie, I added a comment ;)
> 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.
I agree. I thought we were talking about the "poor smuck" having to
document it, and the patch header is a pretty good start (and maybe
finish ;).
- 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/
