Re: [RFC][UPDATE PATCH 2/4] convert soft-timer subsystem to timerintervals

[Nishanth Aravamudan]

On 18.05.2005 [09:59:27 -0600], Jonathan Corbet wrote:
> Hi, Nishanth,
> 
> To my uneducated eye, this patch looks like a useful cleaning-up of the
> timer API.  I do have one question, though...

Thanks! I think one of the best side-effects (beyond a more accurate
execution of sleep requests) of my patch is that the new interfaces are
a heck of a lot saner :)

> > @@ -238,15 +327,41 @@ void add_timer_on(struct timer_list *tim
> >  	check_timer(timer);
> >  
> >  	spin_lock_irqsave(&base->lock, flags);
> > +	timer->expires = jiffies_to_timerintervals(timer->expires);
> 
> It would appear that, depending on where you are, ->expires can be
> expressed in two different units.  Users of add_timer() and mod_timer()
> are expecting jiffies, but the internal code uses timer intervals.  What
> happens when somebody does something like this?
> 
> 	mod_timer(&my_timer, my_timer.expires + additional_delay);
> 
> Might it be better to store the timerintervals value in a different
> field, and leave ->expires as part of the legacy interface only?

This is definitely an option. Currently, it is somewhat vague as to
whether, once a timer has been submitted, whether the expires field is
still valid to the caller. In the new system, it will clearly explicitly
not be (I meant to modify the comment to add_timer(), mod_timer() and
set_timer_nsecs() appropriately, but have not yet.

The problem with the mod_timer() approach you suggest is that there is
no guarantee that my_timer.expires represents anything close to the
current time. And, as far as my experience with reviewing the current
callers is concerned, there is no such usage.

It definitely is feasible and reasonable, though, to make that change. I
will look into it and see what I can do.

Thanks for the feedback!

-Nish
-
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/